CSC/ECE 517/Spring 2023/E2308. Refactor course.rb and course team.rb models
Team Contact
Team Members
- Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
- Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
- Vikram Pande, (unityID:vspande, GitHub:vikrampande7)
Mentor
- Kartiki Bhandakkar, kbhanda3
Overview of Expertiza
Expertiza is an open-source software written using Ruby on Rails which functions as a learning management software system. It has many 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 containing information about the course and its associated teams respectively. The Course and CourseTeam which are the models primarily addressed in this project are both critical components 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/models/course.rb
| # | Change | Rationale | Commit Link |
|---|---|---|---|
| 1 | Checked and Removed get_participant method
|
Checked the usage of Method through IDE whether the function is used, the only call was from course_spec.rb and not from anywhere else. Hence, removed from course.rb
|
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/models/course_spec.rb
| # | Change | Rationale | Commit Link |
|---|---|---|---|
| 1 | Removed the test case for get_paprticipant method
|
Removed the test case for get_participant method as the method was not used anywhere else in the code.
|
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 |
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)