CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

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

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

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

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

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

• Github Repository: https://github.com/greyfiles/expertiza
• Pull Request: https://github.com/expertiza/expertiza/pull/2460
• VCL Server: http://152.7.98.115:8080/

Contributors to this project

• Grey Files (unityid: mgfiles, github: greyfiles)
• Colin O'Dowd (unityid: cdodowd, github: colin-odowd)
• Pradyumna Khawas (unityid: ppkhawas, github: therealppk)