CSC/ECE 517 Fall 2021 - E2124. Refactor review mapping controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

This wiki page is for the description of changes made under E2124 OSS assignment for Fall 2021, CSC/ECE 517.

About Expertiza

Expertiza is an open source project based on 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.

Description of the project

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing its observable behavior. This can be achieved through refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.
Link to the Pull Request Submitted: [1]
link to the deployed project: [2]
Link to the Repository: [3]

Functionality of review_mapping_controller

The functionality of review_mapping_controller is to provide mapping for reviewer and assignment. Basically, the controller handles assignment of reviews to different teams or single student user, such as the event of peer review and self review. Also, this controller is responsible to respond student user request for extra bonus reviews based on assignment policy.

Problem Statement

The review_mapping_controller is a long and complex file. Most of the methods are sparsely commented on. Some methods are way too long to understand, please break them down into pieces for better understanding. Also, the few instances of code duplication that exist should also be removed.

Tasks

-Refactor the long methods in review_mapping_controller.rb
-Rename variable names such as student_review_num, submission_review_num, calibrated_artifacts_num, participants_hash to convey what they are actually used for
-Replace switch statements with subclasses methods
-Create models for the subclasses
-Remove hardcoded parameters

Flow Chart & Design Pattern



We were asked to refactor the long methods in review_mapping_controller.rb. We followed the flow chart above during the refactoring process. It happened many times when the Rspec test suit and Cucumber tests passed locally, but ran into an issue when we commit changes on GitHub. The error log from the TRAVIS CI helped us identify the issue. Then we debugged on local machine and followed the whole implementation process again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimal issues.

Files modified/created in the current project

1. review_mapping_controller.rb
2. review_mapping_controller_spec.rb
3. select_reviewer.html.haml
4. app/views/assignments/edit/_calibration.html.erb
5. app/views/review_mapping/select_reviewer.html.haml
6. app/views/student_quizzes/_set_dynamic_quiz.html.erb
7. app/views/student_review/_set_dynamic_review.html.erb
8. config/routes.rb
9. db/schema.rb
10. spec/controllers/review_mapping_controller_spec.rb
11. spec/features/assignment_creation_spec.rb
12. spec/features/review_assignment_spec.rb
13. spec/features/review_mapping_spec.rb

ReviewMappingController

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

review_mapping_controller_spec.rb

Added a test in this file.

_set_dynamic_review.html.erb & review_assignment_spec.rb & review_mapping_spec.rb & routes.rb & assignment_creation_review_strategy_spec.rb & assignment_creation_spec.rb

Modified due to the variable name change.

views/partials

Routes were changed in the views and partials.

Modified View Files:

app/views/review_mapping/select_reviewer.html.haml
app/views/student_quizzes/_set_dynamic_quiz.html.erb

Details of the changes made

1. A couple of long and complex methods such as peer_review_strategy were refactored from this controller.
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything.
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability.
       *Removed code redundancy from review_mapping_controller#peer_review_strategy. Long and reusable code were sorted out to form a new helper function.
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required.


2. Rename variable names and remove hardcoded paramters.
       *Changed :i_dont_care to :no_particular_topic.
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review.
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic.

       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores.



       *Changed :add_reviewer to :assign_reviewer_dynamically.



       *Changed :student_review_num to :num_reviews_per_students.
       *Changed :submission_review_num to :num_reviews_per_submission.



       *Changed :participants_hash to :team_participants_hash.



       *Changed :replace hardcoded parameter (e.g.,0 and 1) with meaningful zero_review and one_review naming scheme.




3. Replace switch statements with subclasses methods
       We used "check_num_reviews_args" function to represent the switch statements in "automatic review mapping" function to simply it.       

4. Create models for the subclasses
       We created 3 modules and put relative subclasses methods in to make the controller more organized.       

5. Added one test case and modified "select_metaviewver" to check if a mapping can be found correctly.

Test Plan

Manual UI Testing

According to the introduction, the modification of this project is about review_mapping_controller.rb. The main job of this controller is to correctly assign the review to each student who is requesting them for a certain assignment. Unfortunately, the deployed project database is not complete, and student accounts cannot be created. Therefore, we cannot use the deployed project to give a complete UI test plan.

In fact, due to system reasons, the internal logic of these controllers cannot be tested using UI tests because they are work flow and logic that have nothing to do with the UI. (The work flow can be found here [4]) So we will re-introduce and emphasis the tests in Rspec unit test section to explain why the project is still complete after modification.

Rspec Unit Tests

Since this is a refactoring project, We followed the implementation flowchart provided in the above section step by step. In the refactoring process, we made sure that the changes did not break any functionality and passed all the existing test cases. Additionally, we added select_metareviewer to capture one edge case scenario. New test case select_metareviewer makes sure that ReponseMap can be correctly found.


The current test suit carefully follows Test Driven Development (TDD) approach, and offers testers automated testing environment using rSpec. The tests take advantages of double and stub features of rSpec gems, and pre-define different user types (e.g., instructor and student) in each case for different purpose. Testers can enter "rspec spec/controllers/review_mapping_controller_spec.rb" command in the terminal to execute the automated rspec test suit. After execution of the test cases, result of passing all test cases is shown as below.



Note: If you are interested in what each test case does, the link below is highly recommended for you to review as you could get better understanding by reading the assertion per test case:
https://github.com/JesseChen1031/expertiza/blob/master/spec/controllers/review_mapping_controller_spec.rb

Capybara Integration and Functional Tests

Our project passed the Travis CI build test.

Code Coverage

The code Coverage for Controllers has increased as shown below.

[5] # Link for the COVERALLS stats of our pull request.

Project Mentor

Jialin Cui (jcui9@ncsu.edu)

Team Members

Yi Li (yli273@ncsu.edu)

Zijun Lu (zlu5@ncsu.edu)

Huangxing Chen (hchen63@ncsu.edu)

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2021_-_E2124._Refactor_review_mapping_controller.rb&oldid=140986"