CSC/ECE 517/Spring 2023/E2308. Refactor course.rb and course team.rb models: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
Line 1: Line 1:
==E2308. Refactor course.rb and course_team.rb models==
=== E2308. Refactor course.rb and course_team.rb models ===
 
This page provides a description of the Expertiza based OSS project.
 


__TOC__
__TOC__


== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updated at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.


===About Expertiza===
The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.
 
[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
 
===Problem Statement===
The following tasks were accomplished in this project:
 
* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller
 
===About Versions Controller===
This class manages different versions of reviews. If a reviewer reviews a submission, and after that, the author revises the submission, the next time the reviewer does a review (s)he will create a new version. Sometimes it’s necessary to find the current version of a review; sometimes it’s necessary to find all versions. Similarly, a user may want to delete the current version of a review, or all versions of a review. Pagination of versions helps the user to view a subset of versions at a time. Considering the huge number of versions in the system, it is very useful to have a pagination mechanism and a filtering mechanism which can be applied on the whole set of versions. The idea is to display the versions in an ordered, comprehensible and logical manner. In Expertiza the gem ‘will_paginate’ is used to achieve pagination.
 
===Current Implementation===
 


=====Functionality=====
== Files Modified ==
* Any user irrespective of his/ her privileges can view all the versions.
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
::The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
{| class="wikitable" style="width: 100%;
* Any user can delete any version
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
::The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
|-
* Filtering of versions were restricted to the current user
|1
::The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.  
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|style="width: 30%"| Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Bugfix: Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze. This was beyond the scope of our work but we wanted to improve the page functionality.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}


=====Drawbacks and Solutions=====
=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
* '''Problem 1''': The method paginate_list is doing more than one thing.
{| class="wikitable" style="width: 100%;
::The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
<pre>
|-
# For filtering the versions list with proper search and pagination.
|1
  def paginate_list(id, user_id, item_type, event, datetime)
|Added two new RSpec tests for the <code>update_password</code> method
    # Set up the search criteria
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
    criteria = ''
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
    criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
|-
    if current_user_role? == 'Super-Administrator'
|2
      criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
    end
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
    criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit]<br>[https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
    criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
|-
    criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
|3
    criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
|Added <code>User</code> and <code>PasswordReset</code> factories and removed hardcoded variables
    criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "
|Cleaned up hardcoded <code>User</code> and <code>PasswordReset</code> models with premade factories to improve readability of the code
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]<br>[https://github.com/expertiza/expertiza/commit/96376e0b633e6b6e08d472a4cb4600d20e0c024f Commit]
|-
|}


    if current_role == 'Instructor' || current_role == 'Administrator'
=== Changes to <code>spec/factories/password_retrieval_factory.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Created a file to host the <code>password_retrieval_controller_spec.rb</code> fixtures
|We implemented factories in the rspec test file to improve modularity and code readability.
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]
|-
|}


    end
=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}


    # Remove the last ' AND '
=== Changes to <code>app/controllers/auth_controller.rb</code> ===
    criteria = criteria[0..-5]
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}


    versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
    versions
{| class="wikitable" style="width: 100%;
  end
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
</pre>
|-
* '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
|1
**An administrator can see all the versions
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
**An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
**A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
**A Student can see all the versions created by him/ her.
|-
* '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
|2
::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
* '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
* '''Problem 3''': The paginate method can be moved to a helper class.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
|-
* '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}


===New Implementation===
== Testing ==
*The method paginate_list has been split into 2 methods now.
** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
** paginate_list – this method will call the paginate API.
:First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
<pre>
  # pagination.
  def paginate_list(versions)
    paginate(versions, VERSIONS_PER_PAGE);
  end


  def BuildSearchCriteria(id, user_id, item_type, event)
'''Everything in the testing segment was beyond the scope of our work. However, we wanted to validate our code through RSpec testing before merging into the <code>main</code> Expertiza branch. In the second submission phase, we plan to further enhance the testing suite of the <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> through factories and fixtures.'''
    # Set up the search criteria
    search_criteria = ''
    search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
    if current_user_role? == 'Super-Administrator'
      search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
    end
    search_criteria = search_criteria + add_user_filter
    search_criteria = search_criteria + add_version_type_filter(item_type).to_s
    search_criteria = search_criteria + add_event_filter(event).to_s
    search_criteria = search_criteria + add_date_time_filter
    search_criteria
  end
</pre>
* The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
<pre>
  def add_id_filter_if_valid (id)
    "id = #{id} AND " if id && id.to_i > 0
  end


  def add_user_filter_for_super_admin (user_id)
=== Test Plan - Manual/System Test Cases ===
    "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
Along with the unit tests that we have written to test our files, we conducted a few system tests manually to verify the functionality works as expected. The <code>password_retrieval_controller.rb</code> is difficult for us to test because we don't have access to the email used to make the sample account, but the <code>auth_controller.rb</code> is very testable. Below is the tests we completed listed in the order of completion.
  end


  def add_user_filter
==== 1) Test instructor login & redirect to home page ====
    "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
When navigating to the expertiza website, our branch correctly displayed the login page as shown below.
  end
[[File:before_login.png|center|border|Form shown to the user before logging in to expertiza]]
After typing in the sample login credentials of "instructor6" and "password," the user is then correctly logged in and redirected to the home page for instructors shown below.
[[File:after_login.png|center|border|1400px|Page shown to the user after logging in to expertiza]]
==== 2) Test instructor logout & redirect to login page ====
Then, when the user clicks the logout button, they are correctly logged out and redirected once again to the login page as shown below.
[[File:after_logout.png|center|border|Form shown to the user after logging out of expertiza]]
==== 3) Test incorrect login ====
When the user attempts to login but enters the incorrect login information of "instructor6" and "incorrect," the user is shown the error message and then correctly redirected to the forgot password page shown below.
[[File:after_incorrect_login.png|center|border|1400px|Form shown to the user after an incorrect login attempt to expertiza]]


  def add_event_filter (event)
    "event = '#{event}' AND " if event && !(event.eql? 'Any')
  end


  def add_date_time_filter
=== Automated Testing of <code>auth_controller_.rb</code> ===
    "created_at >= '#{time_to_string(params[:start_time])}' AND " +
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>
        "created_at <= '#{time_to_string(params[:end_time])}'"
  end


  def add_version_type_filter (version_type)
[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]
    "item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
  end
</pre>
* The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.


<pre>
After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>
module PaginationHelper


  def paginate (items, number_of_items_per_page)
[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]
    items.page(params[:page]).per_page(number_of_items_per_page)
  end


end
We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>
</pre>


===Code improvements===
[[File:after_improving_auth_controller_spec.png|center|frame|rspec tests continuing to all pass after improving and adding to the auth_controller tests]]
* Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
** It is not easy to understand what 25 is unless the programmer takes a close look at the code.
** In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
* The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.


<pre>
Tests prior to the changes covered 91.94% of <code>auth_controller.rb</code>.
def action_allowed?
[[File:coverage_before.png|center|frame|rspec test coverage report before the improvements to the auth_controller unit tests]]
    true
  end
</pre>


:With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’,  ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.
This is already a good coverage but after my changes I wanted to ensure my changes were also tested thoroughly. After adding tests, the tests covered 95.24% of <code>auth_controller.rb</code>. It improved slightly to be an even better coverage.
[[File:coverage_after.png|center|frame|rspec test coverage report after the improvements to the auth_controller unit tests]]


<pre>
=== Automated Testing of <code>password_retrieval_controller.rb</code> ===
  def action_allowed?
Before any refactoring to <code>passsword_retrieval_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>
    case params[:action]
    when 'new', 'create', 'edit', 'update'
    #Modifications can only be done by papertrail
      return false
    when 'destroy_all'
      ['Super-Administrator',
      'Administrator'].include? current_role_name
    else
      #Allow all others
      ['Super-Administrator',
      'Administrator',
      'Instructor',
      'Teaching Assistant',
      'Student'].include? current_role_name
    end
  end
</pre>


===Automated Testing using RSPEC===
[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]
The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
<pre>
user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures


Randomized with seed 19254
Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]
.
</pre>


===Testing from UI===
After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
Following are a few testcases with respectto our code changes that can be tried from UI:
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]
1. To go to versions index page, type in the following url after logging in:
  http://152.46.16.81:3000/versions


2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users.
We implemented factories in the rspec test file to improve modularity and code readability. Factories provide more flexibility in generating models to meet the requirements of the test, as opposed to fixtures. As you can see from the below images, the code is significantly cleaner as many lines of hardcoded strings have been removed.  
  http://152.46.16.81:3000/versions/new
[[File:prefactory.png|center|frame|rspec tests before factories were implemented]] <br>
  This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.
[[File:postfactory.png|center|frame|rspec tests after factories were implemented]]


3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>
  http://152.46.16.81:3000/versions/search


4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.
[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]


===References===
== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/


#[https://github.com/expertiza/expertiza Expertiza on GitHub]
== Contributors to this project ==
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
* Grey Files (unityid: mgfiles, github: greyfiles)
#[http://expertiza.ncsu.edu/ The live Expertiza website]
* Colin O'Dowd (unityid: cdodowd, github: colin-odowd)
#[http://bit.ly/myexpertiza  Demo link]
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

Revision as of 16:06, 19 March 2023

E2308. Refactor course.rb and course_team.rb models

Overview of Expertiza

Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The auth_controller.rb and password_retrieval_controller.rb which are the files primarily addressed in this project are both critical controllers in providing this functionality.

Description of Project

auth_controller is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updated at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in before_action methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The password_retrieval_controller deals with the process of updating and resetting a user password. The send_password method generates a token and appends it to a password reset URL. If a user submits a valid email address on the password_retrieval/forgotten view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the password_retrieval/reset_password view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the update_password and send_password methods.

Files Modified

Changes to app/controllers/password_retrieval_controller.rb

 #  Change Rationale Commit Link
1 Updated check_reset_url method name to check_token_validity The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name. Commit
2 Replaced repeated code in lines 35-36 and 62-63 The use of repeated code violates the DRY principle and so it was moved to a new method. Commit
3 Change token expiration time to constant in line 41 This time should not be hardwired; it should be a constant or a parameter. Commit
4 Bugfix: Reload page if email is nil or empty on password_retrieval/forgotten view An empty email parameter was causing the send password button to freeze. This was beyond the scope of our work but we wanted to improve the page functionality. Commit
5 Improve overall comments and rewrite error messages The comments and error messages in the controller need to be more meaningful, specific and clear. Commit

Changes to spec/controllers/password_retrieval_controller_spec.rb

 #  Change Rationale Commit Link
1 Added two new RSpec tests for the update_password method There were no tests for the update_password method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests. Commit
2 Added two new RSpec tests for the send_password method to check nil or blank input for email There were no tests for the send_password method pertaining to checking invalid inputs in the request params Commit
Commit
3 Added User and PasswordReset factories and removed hardcoded variables Cleaned up hardcoded User and PasswordReset models with premade factories to improve readability of the code Commit
Commit

Changes to spec/factories/password_retrieval_factory.rb

 #  Change Rationale Commit Link
1 Created a file to host the password_retrieval_controller_spec.rb fixtures We implemented factories in the rspec test file to improve modularity and code readability. Commit

Changes to config/routes.rb

 #  Change Rationale Commit Link
1 Updated URL path and controller action to updated method name check_token_validity The action and URL path must be renamed to generate pathing to the controller method and views. Commit

Changes to app/controllers/auth_controller.rb

 #  Change Rationale Commit Link
1 Move logger messages to before_action blocks wherever possible Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either before_action or after_action blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality. Commit
2 *Not in required chagnes* Replaced repeated code for re-caching the user role We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called self.rebuild_role_cache. Commit
3 Improved helper function names Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable. Commit

Changes to spec/controllers/auth_controller_spec.rb

 #  Change Rationale Commit Link
1 Improved existing tests for after_login to explicitly test for a redirect When looking over the existing test cases, I noticed that the test that was verifying that the after_login method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working. Commit
2 Added a test for set_current_role when the role is found to make sure it rebuilds the role cache In the unit tests for the set_current_role method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set. Commit
3 Added a test for clear_user_info to make sure it rebuilds the role cache In the unit tests for the clear_user_info method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared. Commit

Testing

Everything in the testing segment was beyond the scope of our work. However, we wanted to validate our code through RSpec testing before merging into the main Expertiza branch. In the second submission phase, we plan to further enhance the testing suite of the auth_controller.rb and password_retrieval_controller.rb through factories and fixtures.

Test Plan - Manual/System Test Cases

Along with the unit tests that we have written to test our files, we conducted a few system tests manually to verify the functionality works as expected. The password_retrieval_controller.rb is difficult for us to test because we don't have access to the email used to make the sample account, but the auth_controller.rb is very testable. Below is the tests we completed listed in the order of completion.

1) Test instructor login & redirect to home page

When navigating to the expertiza website, our branch correctly displayed the login page as shown below.

Form shown to the user before logging in to expertiza
Form shown to the user before logging in to expertiza

After typing in the sample login credentials of "instructor6" and "password," the user is then correctly logged in and redirected to the home page for instructors shown below.

Page shown to the user after logging in to expertiza
Page shown to the user after logging in to expertiza

2) Test instructor logout & redirect to login page

Then, when the user clicks the logout button, they are correctly logged out and redirected once again to the login page as shown below.

Form shown to the user after logging out of expertiza
Form shown to the user after logging out of expertiza

3) Test incorrect login

When the user attempts to login but enters the incorrect login information of "instructor6" and "incorrect," the user is shown the error message and then correctly redirected to the forgot password page shown below.

Form shown to the user after an incorrect login attempt to expertiza
Form shown to the user after an incorrect login attempt to expertiza


Automated Testing of auth_controller_.rb

Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: rspec spec/controllers/auth_controller_spec.rb

rspec tests all passing before the refactoring was completed

After making all of the above changes to auth_controller.rb, we ran the rspec tests for the controller again with the command: rspec spec/controllers/auth_controller_spec.rb

rspec tests continuing to all pass after completing the refactoring

We have successfully preserved the passing tests after the improvements we made to the auth_controller.rb. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to auth_controller_spec.rb. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: rspec spec/controllers/auth_controller_spec.rb

rspec tests continuing to all pass after improving and adding to the auth_controller tests

Tests prior to the changes covered 91.94% of auth_controller.rb.

rspec test coverage report before the improvements to the auth_controller unit tests

This is already a good coverage but after my changes I wanted to ensure my changes were also tested thoroughly. After adding tests, the tests covered 95.24% of auth_controller.rb. It improved slightly to be an even better coverage.

rspec test coverage report after the improvements to the auth_controller unit tests

Automated Testing of password_retrieval_controller.rb

Before any refactoring to passsword_retrieval_controller.rb was done, we ran the rspec tests created for the controller with the following command: rspec spec/controllers/password_retrieval_controller_spec.rb

rspec tests all passing before the refactoring was completed

Tests prior to the changes covered 63.3% of password_retrieval_controller.rb.

rspec test coverage report before the refactoring was completed

After adding tests, the tests covered 91.1% of password_retrieval_controller.rb.

rspec test coverage report after the refactoring was completed

We implemented factories in the rspec test file to improve modularity and code readability. Factories provide more flexibility in generating models to meet the requirements of the test, as opposed to fixtures. As you can see from the below images, the code is significantly cleaner as many lines of hardcoded strings have been removed.

rspec tests before factories were implemented


rspec tests after factories were implemented

The below image shows the output of the following command after all tests were added: rspec spec/controllers/password_retrieval_controller_spec.rb

rspec tests all passing after the refactoring was completed

Relevant Links

Contributors to this project

  • Grey Files (unityid: mgfiles, github: greyfiles)
  • Colin O'Dowd (unityid: cdodowd, github: colin-odowd)
  • Pradyumna Khawas (unityid: ppkhawas, github: therealppk)
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517/Spring_2023/E2308._Refactor_course.rb_and_course_team.rb_models&oldid=147410"