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
 
(43 intermediate revisions by 3 users not shown)
Line 16: Line 16:


== Description of Project ==
== 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.
<code>course</code> model stores information about the instructor and institution and is associated with other models such as User, CourseParticipant, CourseTeam, Assignment, AssignmentParticipant, and Notification. The course model is responsible for completing a variety of tasks, including returning course teams, returning the submission directory for the course, viewing participants enrolled in the course, adding a participant to the course, copying the Assignment Participants to Course Participants, and checking whether a user is part of any CourseTeam. The problem description lists code smells that need to be fixed. We fixed some issues, such as deleting the unused methods and renaming methods to indicate their action. The renaming problem required us to change a few methods calls where the function to be renamed was used. Also, to reduce the cognitive complexity of the copy_assignment_participants method, we created a method to separate the raising error functionality. Comments were added to indicate the action of the methods.


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.
The <code>course_team</code> is a subclass of the team class. CourseTeams are a type of team that an instructor can use throughout an entire semester, providing consistency in team membership over time. The course_team model is responsible for completing various tasks, including returning the course of the team, adding a participant to the CourseTeam, and copying the CourseTeam Participants to AssignmentTeam Participants. There are methods for importing and exporting data to/from CSV files for CourseTeam objects. The problem description lists code smells/issues that need to be fixed. The issues that we fixed were deleting unused methods, renaming methods, and changing method calls, changing rspec expectations, adding meaningful comments. To refactor methods, DRY principles were used. In addition, analogous changes were made in the assignment_team model.


== Files Modified ==
== Files Modified ==
== CHANGES IN MODEL FILES ==
=== Changes to <code>app/models/course.rb</code> ===
=== Changes to <code>app/models/course.rb</code> ===
{| class="wikitable" style="width: 100%;
{| class="wikitable" style="width: 100%;
Line 36: Line 37:
|-
|-
|3
|3
|Change token expiration time to constant in line 41
|Removed <code>add_member</code> method from <code>course_team</code> model
|This time should not be hardwired; it should be a constant or a parameter.  
|Assignment_team and Course_team are subclasses of Team. Both should follow the similar logic of adding members of respective teams. As of for, add_participants, usage is different for course.rb and course_team.rb, hence it should be present in both. However, to make the code more consistent among classes, team, course_team, and assignment_team, the add_member method should only be present in the team class.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|[https://github.com/expertiza/expertiza/pull/2548/commits/8fb9bf9a793b8c154bc2b640daa630624e008a86 Commit]
|-
|-
|4
|4
|Bugfix: Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|Created method <code>raise_errors</code>
|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.
|To reduce Cognitive Complexity of method <code>copy_participants</code>
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|[https://github.com/kartikrawool/expertiza/commit/7c1ae764d2d64e0246422934f292b9b2fe43a031 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]
|}
|}


Line 56: Line 52:
|-
|-
|1
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Removed <code>assignment_id</code>  
|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.
|CourseTeam and AssignmentTeam inherit from Team class. The teams' table has an attribute <code>parent_id</code> that helps determine whether the team is AssigmentTeam or CourseTeam. Thus, there is no need for <code>assignment_id</code> in the CourseTeam class and no need for <code>course_id</code> in AssignmentTeam class. It will be a bad coding practice to call the assignment_id method on the CourseTeam object, calling the course_id method on the AssignmentTeam object as it would be inappropriate.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|[https://github.com/kartikrawool/expertiza/commit/ef9727542999c6cdbf2334f5b62ebb30fbfe8cdd Commit]
|-
|-
|2
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|Renamed <code>copy</code> method as <code>copy_to_assignment_team</code>
|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>.
|As this method is copying a CourseTeam to an AssignmentTeam, renamed it as <code>copy_to_assignment_team</code>. A suggestion for copy method in AssignmentTeam would be to rename it as <code>copy_to_course_team</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b#diff-56ab3d3b28f0be68663223f0a5ef9daa5c8d106caca412481562272ab2edfae1 Commit]<br>
|-
|-
|3
|3
|Improved helper function names
|Added Import Functionality 
|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.
|Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|[https://github.com/kartikrawool/expertiza/commit/972c66e47fbc22965490cc4da3ae69a56d076d6b Commit]
|}
 
=== Changes to <code>app/models/assignment_team.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Renamed <code>copy</code> method for AssignmentTeam
|Renamed <code>copy</code> for copying current AssignmentTeam to CourseTeam as <code>copy_to_course_team</code>
|[https://github.com/kartikrawool/expertiza/commit/b93df2590a7ab52f87a1df00512afe626fa81520 Commit]
|-
|2
|Added Import Functionality 
|Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController to get required and optional fields from the respective model.
|[https://github.com/kartikrawool/expertiza/commit/2bae1785a15caede251f81517ecf944302f80d89 Commit]
|}
|}
=== Changes to <code>app/models/team.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Added Import Functionality 
|Refactored import_team_members and added import_helper, that is used for importing CourseTeam and AssignmentTeam objects.
|[https://github.com/kartikrawool/expertiza/commit/53a86107644d3a0b0bef28a42d499a0b7b386ebb Commit]
|}
== CHANGES IN RSPEC FILES ==


=== Changes to <code>spec/models/course_spec.rb</code> ===
=== Changes to <code>spec/models/course_spec.rb</code> ===
Line 84: Line 107:
|Renamed the respective method of <code>copy_participants</code> to <code>copy_assignment_participants</code>in rspec file as well.
|Renamed the respective method of <code>copy_participants</code> to <code>copy_assignment_participants</code>in rspec file as well.
|[https://github.com/kartikrawool/expertiza/commit/5177376c0f9a4f1890f49879bbbbfee0b28b8263 Commit]<br>
|[https://github.com/kartikrawool/expertiza/commit/5177376c0f9a4f1890f49879bbbbfee0b28b8263 Commit]<br>
|}
=== Changes to <code>spec/models/course_team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|-
|3
|1
|Added <code>User</code> and <code>PasswordReset</code> factories and removed hardcoded variables
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>
|Cleaned up hardcoded <code>User</code> and <code>PasswordReset</code> models with premade factories to improve readability of the code
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>in rspec file as well.
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]<br>[https://github.com/expertiza/expertiza/commit/96376e0b633e6b6e08d472a4cb4600d20e0c024f Commit]
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b#diff-8b753735f47ae84fdf88f7e9f15e08b8dd13ba1bc8fd16abad11148800aa8954 Commit]<br>
|-
|}
|}


Line 97: Line 124:
|-
|-
|1
|1
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
|Import functionality changes</code>
|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.
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|[https://github.com/kartikrawool/expertiza/commit/972c66e47fbc22965490cc4da3ae69a56d076d6b Commit]<br>
|}
 
=== Changes to <code>spec/models/assignment_team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|-
|2
|1
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|Import functionality changes</code>
|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.
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|[https://github.com/kartikrawool/expertiza/commit/2bae1785a15caede251f81517ecf944302f80d89 Commit]<br>
|}
 
=== Changes to <code>spec/models/team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|-
|3
|1
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|Import functionality changes</code>
|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.
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|[https://github.com/kartikrawool/expertiza/commit/53a86107644d3a0b0bef28a42d499a0b7b386ebb#diff-638390a8686ba4d2e18c1fc9dfbef22271c8a02bb5d56bf78ded42e428fcf8b1 Commit]<br>
|}
|}
== CHANGES IN CONTROLLER FILES ==


=== Changes to <code>app/controllers/teams_controller.rb</code> ===
=== Changes to <code>app/controllers/teams_controller.rb</code> ===
Line 117: Line 156:
|-
|-
|1
|1
|Created a file to host the <code>password_retrieval_controller_spec.rb</code> fixtures
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>  
|We implemented factories in the rspec test file to improve modularity and code readability.
|The change was directly related to changes of copy method in <code>course_team.rb</code>.
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b Commit]
|-
|-
|}
|}
== Test Plan ==
Manual Testing
*''' Adding Course Team member'''
** Adding member student1922 to course CSC540
[[File:Before_adding_course_team_member.png|1000px|center|Add course member to team Radical]]
[[File:After_adding_course_team_member.png|1000px|center|Succesffully added member to the team]]
*''' Importing Course Teams'''
** Importing Course Teams
** Import Preview
[[File:Importpreview.png|1000px|center|]]
** Import Error: We have refactored the import functionality according to E1923. The ImportFileController calls the import methods of the models course_team, assignment_team, team, etc. To debug this error, we need to change other models, such as assignment_participant, assignment_team, course_participant, metareview_response_map, question, review_response_map, sign_up_sheet, sign_up_topic, and user so that importing of all models would be generalised and would be functional. Some UI changes would be needed accordingly.
[[File:Importerror.png|1000px|center|Import error]]
Automated Testing
All the suggested changes were made to <code>course.rb</code> and <code>course_team.rb</code>, all these models were amended and as a result we need to update the tests. We added or amended tests in their respective .spec files. The methods were tested for edge cases and positive and negative test cases were written. The refactored methods were updated in the test cases.
*<code>'''course_team'''</code>
[[File:course_team_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
*<code>'''course'''</code>
[[File:course_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
*<code>'''assignment_team'''</code>
[[File:assignment_team_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
*<code>'''team.rb'''</code>
[[File:team_spec_output.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
=== Coverage: ===
'''Before refactoring'''
[[File:coverage_old.jpg|center|frame|Coverage of codebase before refactoring]]
[[File:course_team_coverage.jpg|center|frame|Coverage of <code>course_team.rb</code> before refactoring]]
[[File:course_coverage.jpg|center|frame|Coverage of <code>course.rb</code> before refactoring]]
[[File:assignment_team_coverage.jpg|center|frame|Coverage of <code>assignment_team.rb</code> before refactoring]]
[[File:team_coverage.jpg|center|frame|Coverage of <code>team.rb</code> before refactoring]]
'''After refactoring'''
[[File:coverage_refactor.jpg|center|frame|Coverage of codebase after refactoring]]
[[File:course_team_coverage_refactor.jpg|center|frame|Coverage of <code>course_team.rb</code> after refactoring]]
[[File:course_coverage_refactor.jpg|center|frame|Coverage of <code>course.rb</code> after refactoring]]
[[File: assignment_team_coverage_refactor.jpg|center|frame|Coverage of <code>assignment_team.rb</code> after refactoring]]
[[File:team_coverage_refactor.jpg|center|frame|Coverage of <code>team.rb</code> after refactoring]]


== Relevant Links ==
== Relevant Links ==

Latest revision as of 04:00, 28 March 2023


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

course model stores information about the instructor and institution and is associated with other models such as User, CourseParticipant, CourseTeam, Assignment, AssignmentParticipant, and Notification. The course model is responsible for completing a variety of tasks, including returning course teams, returning the submission directory for the course, viewing participants enrolled in the course, adding a participant to the course, copying the Assignment Participants to Course Participants, and checking whether a user is part of any CourseTeam. The problem description lists code smells that need to be fixed. We fixed some issues, such as deleting the unused methods and renaming methods to indicate their action. The renaming problem required us to change a few methods calls where the function to be renamed was used. Also, to reduce the cognitive complexity of the copy_assignment_participants method, we created a method to separate the raising error functionality. Comments were added to indicate the action of the methods.

The course_team is a subclass of the team class. CourseTeams are a type of team that an instructor can use throughout an entire semester, providing consistency in team membership over time. The course_team model is responsible for completing various tasks, including returning the course of the team, adding a participant to the CourseTeam, and copying the CourseTeam Participants to AssignmentTeam Participants. There are methods for importing and exporting data to/from CSV files for CourseTeam objects. The problem description lists code smells/issues that need to be fixed. The issues that we fixed were deleting unused methods, renaming methods, and changing method calls, changing rspec expectations, adding meaningful comments. To refactor methods, DRY principles were used. In addition, analogous changes were made in the assignment_team model.

Files Modified

CHANGES IN MODEL FILES

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 Renamed copy_participants method Renamed copy_participants to copy_assignment_participants which makes more sense and a clear idea of how method works and what is the purpose of method. Commit
3 Removed add_member method from course_team model Assignment_team and Course_team are subclasses of Team. Both should follow the similar logic of adding members of respective teams. As of for, add_participants, usage is different for course.rb and course_team.rb, hence it should be present in both. However, to make the code more consistent among classes, team, course_team, and assignment_team, the add_member method should only be present in the team class. Commit
4 Created method raise_errors To reduce Cognitive Complexity of method copy_participants Commit

Changes to app/models/course_team.rb

 #  Change Rationale Commit Link
1 Removed assignment_id CourseTeam and AssignmentTeam inherit from Team class. The teams' table has an attribute parent_id that helps determine whether the team is AssigmentTeam or CourseTeam. Thus, there is no need for assignment_id in the CourseTeam class and no need for course_id in AssignmentTeam class. It will be a bad coding practice to call the assignment_id method on the CourseTeam object, calling the course_id method on the AssignmentTeam object as it would be inappropriate. Commit
2 Renamed copy method as copy_to_assignment_team As this method is copying a CourseTeam to an AssignmentTeam, renamed it as copy_to_assignment_team. A suggestion for copy method in AssignmentTeam would be to rename it as copy_to_course_team. Commit
3 Added Import Functionality Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController. Commit

Changes to app/models/assignment_team.rb

 #  Change Rationale Commit Link
1 Renamed copy method for AssignmentTeam Renamed copy for copying current AssignmentTeam to CourseTeam as copy_to_course_team Commit
2 Added Import Functionality Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController to get required and optional fields from the respective model. Commit

Changes to app/models/team.rb

 #  Change Rationale Commit Link
1 Added Import Functionality Refactored import_team_members and added import_helper, that is used for importing CourseTeam and AssignmentTeam objects. Commit

CHANGES IN RSPEC FILES

Changes to spec/models/course_spec.rb

 #  Change Rationale Commit Link
1 Removed the test case for get_paprticipant method test case Removed the test case for get_participant method as the method was not used anywhere else in the code. Commit
2 Renamed copy_participants method test case Renamed the respective method of copy_participants to copy_assignment_participantsin rspec file as well. Commit

Changes to spec/models/course_team_spec.rb

 #  Change Rationale Commit Link
1 Renamed copy to copy_to_assignment_team Renamed copy to copy_to_assignment_teamin rspec file as well. Commit

Changes to spec/models/course_team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

Changes to spec/models/assignment_team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

Changes to spec/models/team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

CHANGES IN CONTROLLER FILES

Changes to app/controllers/teams_controller.rb

 #  Change Rationale Commit Link
1 Renamed copy to copy_to_assignment_team The change was directly related to changes of copy method in course_team.rb. Commit

Test Plan

Manual Testing

  • Adding Course Team member
    • Adding member student1922 to course CSC540
Add course member to team Radical
Add course member to team Radical
Succesffully added member to the team
Succesffully added member to the team
  • Importing Course Teams
    • Importing Course Teams
    • Import Preview
    • Import Error: We have refactored the import functionality according to E1923. The ImportFileController calls the import methods of the models course_team, assignment_team, team, etc. To debug this error, we need to change other models, such as assignment_participant, assignment_team, course_participant, metareview_response_map, question, review_response_map, sign_up_sheet, sign_up_topic, and user so that importing of all models would be generalised and would be functional. Some UI changes would be needed accordingly.
Import error
Import error

Automated Testing All the suggested changes were made to course.rb and course_team.rb, all these models were amended and as a result we need to update the tests. We added or amended tests in their respective .spec files. The methods were tested for edge cases and positive and negative test cases were written. The refactored methods were updated in the test cases.

  • course_team


Test passing for course_team.rb
Test passing for course_team.rb


  • course


Test passing for course_team.rb
Test passing for course_team.rb


  • assignment_team


Test passing for course_team.rb
Test passing for course_team.rb


  • team.rb


Test passing for course_team.rb
Test passing for course_team.rb


Coverage:

Before refactoring

Coverage of codebase before refactoring


Coverage of course_team.rb before refactoring


Coverage of course.rb before refactoring


Coverage of assignment_team.rb before refactoring


Coverage of team.rb before refactoring


After refactoring

Coverage of codebase after refactoring


Coverage of course_team.rb after refactoring


Coverage of course.rb after refactoring


Coverage of assignment_team.rb after refactoring


Coverage of team.rb after refactoring

Relevant Links

Contributors to this project

  • Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
  • Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
  • Vikram Pande, (unityID:vspande, GitHub:vikrampande7)
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=148673"