CSC/ECE 517 Fall 2021 - E2124. Refactor review mapping controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(94 intermediate revisions by 3 users not shown)
Line 3: Line 3:


[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.
[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.
== 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. <br/>
Link to the Pull Request Submitted: [https://github.com/expertiza/expertiza/pull/2060] <br/>
link to the deployed project: [http://152.7.176.14:8080/] <br/>
Link to the Repository: [https://github.com/JesseChen1031/expertiza] <br/>
== 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 ==
== Problem Statement ==
Line 9: Line 18:


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


== Work-Plan Followed ==
== Flow Chart & Design Pattern ==
[[File:FlowchartOODD.jpg]]
[[File:Work_flow.png|1000px]]
<br/><br/>
<br/><br/>
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.
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 ==
== Files modified/created in the current project ==
Line 24: Line 33:
1. review_mapping_controller.rb <br/>
1. review_mapping_controller.rb <br/>
2. review_mapping_controller_spec.rb <br/>
2. review_mapping_controller_spec.rb <br/>
3. assign_quiz_controller.rb <br/>
3. select_reviewer.html.haml <br/>
4. assign_quiz_controller_spec.rb <br/>
4. app/views/assignments/edit/_calibration.html.erb<br/>
5. review_response_map_controller.rb  <br/>
5. app/views/review_mapping/select_reviewer.html.haml<br/>
6. review_response_map_controller_spec.rb <br/>
6. app/views/student_quizzes/_set_dynamic_quiz.html.erb<br/>
7. routes.rb <br/>
7. app/views/student_review/_set_dynamic_review.html.erb<br/>
8. Other views & partials associated affected by these changes <br/>
8. config/routes.rb<br/>
9. db/schema.rb<br/>
10. spec/controllers/review_mapping_controller_spec.rb<br/>
11. spec/features/assignment_creation_spec.rb<br/>
12. spec/features/review_assignment_spec.rb<br/>
13. spec/features/review_mapping_spec.rb<br/>


=== ReviewMappingController ===
=== ReviewMappingController ===
Line 39: Line 53:
=== review_mapping_controller_spec.rb ===
=== review_mapping_controller_spec.rb ===
   
   
After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.
Added a test in this file.
 
=== AssignQuizController ===
'''assign_quiz_dynamically''' (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.


=== assign_quiz_controller_spec.rb ===
=== _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 ===
   
   
Tests related to assign_quiz_controller were moved into this file.
Modified due to the variable name change.
 
=== ReviewResponseMapController.rb ===
'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.
 
=== review_response_map_controller_spec.rb ===
Tests related to review_response_map were moved into this file.
 
=== routes.rb ===
 
New routes were added to newly created controllers.
 
<pre>
assign_quiz_dynamically_assign_quiz_index POST  /assign_quiz/assign_quiz_dynamically(.:format)  assign_quiz#assign_quiz_dynamically
</pre>


=== views/partials ===
=== views/partials ===
Line 70: Line 64:


==== Modified View Files: ====
==== Modified View Files: ====
app/views/review_mapping/select_reviewer.html.haml  <br>
app/views/student_quizzes/_set_dynamic_quiz.html.erb <br>
app/views/student_quizzes/_set_dynamic_quiz.html.erb <br>
app/views/assignments/edit/_calibration.html.erb <br>


== Details of the changes made==
== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. <br/>
1. A couple of long and complex methods such as peer_review_strategy were refactored from this controller. <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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.  <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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. <br/>
 
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Removed code redundancy from review_mapping_controller#peer_review_strategy. Long and reusable code were sorted out to form a new helper function.<br/>
2. Changed :i_dont_care to :no_particular_topic<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. <br/>
[[File:Peer review.png|1000px]] <br/><br/>
2. Rename variable names and remove hardcoded paramters.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :i_dont_care to :no_particular_topic. <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *: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.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *: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.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *It was also used in review_mapping_controller.rb to check if student has selected any particular topic.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *It was also used in review_mapping_controller.rb to check if student has selected any particular topic.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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.<br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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.<br/><br/>
[[File:i_dont_care1.png|1000px]] <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :add_reviewer to :assign_reviewer_dynamically. <br/><br/>
[[File:Add_Reviewer.png|1000px]] <br/><br/>


3. Removed cascading effects of above change from features spec<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :student_review_num to :num_reviews_per_students. <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :submission_review_num to :num_reviews_per_submission. <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option.<br/>before:<br/> [[File:3bef.jpg]] <br/>after: <br/>[[File:3aft.jpg]]<br/><br/>
[[File:Student_review_num.png|1000px]] <br/><br/>


4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :participants_hash to :team_participants_hash. <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases.<br/>
[[File:Participants_hash.png|1000px]] <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Hence, replaced the switch statements and initialized a list with those switch cases.<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list.<br/>
before:<br/> [[File:4bef.jpg]] <br/>after: <br/>[[File:4aft.jpg]]<br/><br/>


5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Changed :replace hardcoded parameter (e.g.,0 and 1) with meaningful zero_review and one_review naming scheme.  <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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. <br/>
[[File:Hardcoded.png|1000px]] <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *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. <br/><br/>
<pre>
  ## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
  def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
    if iterator.zero?
        rand_num = rand(0..num_participants - 1)
    else
        min_value = participants_hash.values.min
        # get the temp array including indices of participants, each participant has minimum review number in hash table.
        participants_with_min_assigned_reviews = []
        participants.each do |participant|
          participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
        end
    # if participants_with_min_assigned_reviews is blank
    no_particpants = participants_with_min_assigned_reviews.empty?
    # or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
    participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
    rand_num = if no_particpants or participant_is_owner
                # use original method to get random number
                rand(0..num_participants - 1)
              else
                # rand_num should be the position of this participant in original array
                participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
              end
    end
    return rand_num
  end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern  <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method.  <br/><br/>
<pre>
  # Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
  def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
    has_error_not_raised = true
    # check for exit paths first
    if num_reviews_per_student == 0 and num_reviews_per_submission == 0
      flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
      has_error_not_raised = false
    elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
      flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
      has_error_not_raised = false
    elsif num_reviews_per_student >= teams.size
      # Exception detection: If instructor want to assign too many reviews done
      # by each student, there will be an error msg.
      flash[:error] = 'You cannot set the number of reviews done ' \
                      'by each student to be greater than or equal to total number of teams ' \
                      '[or "participants" if it is an individual assignment].'
      has_error_not_raised = false
    end
  end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Only some of the  methods written in the class have external usage i.e called by another controllers, views etc. <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *The other methods are just helpers and as such moved into a Helper method module and mixed in the class <br/><br/>
Before Modularization
<pre>
  class ReviewMappingController < ApplicationController
    ...................
    ...................
    512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
    ...................
    ...................
  end 
</pre>
After Modularization
<pre>
  module Helper_methods
  ...................
  5 Methods and 170 lines
  ...................
  end


  class ReviewMappingController < ApplicationController
    include Helper_methods
    ...................
    340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
    ...................
  end 
</pre>


3. Replace switch statements with subclasses methods <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; We used "check_num_reviews_args" function to represent the switch statements in "automatic review mapping" function to simply it.
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; <br/> [[File:Subfunc_for_switch.png|1000px]] <br/>


8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. <br/>
4. Create models for the subclasses<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Internally used methods were not tested <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; We created 3 modules and put relative subclasses methods in to make the controller more organized.
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *Tests for newly created controllers have been moved into a separate spec files. <br/><br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; <br/> [[File:3_modules.png|1000px]] <br/>


9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. <br/>
5. Added one test case and modified "select_metaviewver" to check if a mapping can be found correctly.
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *This method assigns a quiz(Stored as an assignment object) to the participant. <br/>
<br/>[[File:Test_case.png|1000px]]<br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *This method is not related to ReviewMapping functionality, so it was made into a new controller. <br/><br/>


10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. <br/>
== Test Plan ==
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *The controller paths present in the views/partials have to be changed to not to break the functionality <br/>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically <br/><br/>


View File Affected by the creation of AssinQuizController
=== Manual UI Testing ===
[[File:View.png]]


11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. <br/>
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.  
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission.<br/>
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts.<br/>
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash.<br/>
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission.<br/>
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController.<br/>
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.


== 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 [https://expertiza.csc.ncsu.edu/index.php/Review_mappings]) 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 ===  
=== Rspec Unit Tests ===  


Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.
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. <br/>
<br/>[[File:Test_case.png|1000px]]<br/>


[[File:testplan.png]] 
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: Tests have been run for three controllers (one existing and two new).  
[[File:rspec_test_passed.png|1000px]]<br/><br/>
 
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: <br/>
https://github.com/JesseChen1031/expertiza/blob/master/spec/controllers/review_mapping_controller_spec.rb


=== Capybara Integration and Functional Tests ===  
=== Capybara Integration and Functional Tests ===  


As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.
Our project passed the Travis CI build test.


However, no such failure has been reported by Travis build.
[[File:Build_pass.png|1000px]]
 
[[File:Travis CI.png]]
 
[[File:FeaturesCI.png]]


== Code Coverage ==  
== Code Coverage ==  


Code Coverage for Controllers section climbed up.  
The code Coverage for Controllers has increased as shown below.  


[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.
[https://coveralls.io/builds/43657457/source?filename=app%2Fcontrollers%2Freview_mapping_controller.rb] # Link for the COVERALLS stats of our pull request.
 
[[File:CCC1.png]]


[[File:Coverall.png|1000px]]


== Project Mentor ==
== Project Mentor ==


Ramya Vijayakumar (rvijaya4@ncsu.edu)
Jialin Cui (jcui9@ncsu.edu)


== Team Members ==
== Team Members ==


Yaswanth Soodini (ysoodin@ncsu.edu)
Yi Li (yli273@ncsu.edu)
 
Saurabh Shingte (svshingt@ncsu.edu)
 
Vivek Karri (vkarri@ncsu.edu)
 
== Links for Demo Videos ==
 
Assign Reviewer Manually Demo[https://drive.google.com/file/d/1o9vJQ7fCwk0hwHJdErwpg1z_XjsN_XRT/view?usp=sharing]


Regression and Unit Testing of Controllers Demo[https://youtu.be/XejKHQKtpuA]
Zijun Lu (zlu5@ncsu.edu)


Automatic Assignment of Reviewer Demo [https://drive.google.com/file/d/17wpo7mTDenLvEcgubVflLtF_vmLb623a/view?usp=sharing]
Huangxing Chen (hchen63@ncsu.edu)

Latest revision as of 18:45, 6 November 2021

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"