CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

Current Implementation and Problems

1. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
2. Method response_report has some sql - like code. Rewrite with Active Record.
3. Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
4. There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
5. Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
6. Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
7. Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
8. Method automatic_review_mapping_strategy is too long. Please refactor and test it.

Changes Implemented

4. By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
5. We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
6. By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
7. By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
8. This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

 def automatic_review_mapping_strategy(assignment_id,
                                       participants, teams, student_review_num = 0,
                                       submission_review_num = 0)
   participants_hash = {}
   participants.each {|participant| participants_hash[participant.id] = 0 }
   # calculate reviewers for each team
   num_participants = participants.size
   if student_review_num != 0 and submission_review_num == 0
     num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
     student_review_num = student_review_num
     exact_num_of_review_needed = participants.size * student_review_num
   elsif student_review_num == 0 and submission_review_num != 0
     num_reviews_per_team = submission_review_num
     student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
     exact_num_of_review_needed = teams.size * submission_review_num
   end
   execute_peer_review_strategy(assignment_id, teams, num_participants,
                                    student_review_num, num_reviews_per_team,
                                    participants, participants_hash)
   # after assigning peer reviews for each team,
   # if there are still some peer reviewers not obtain enough peer review,
   # just assign them to valid teams
   assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
                             exact_num_of_review_needed)
 end

 def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
                               exact_num_of_review_needed)
   if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 
                               assignment_id, 
                               @@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
     participants_with_insufficient_review_num = []
     participants_hash.each do |participant_id, review_num|
       participants_with_insufficient_review_num << participant_id if review_num < student_review_num
     end
     unsorted_teams_hash = {}
     ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", 
                              assignment_id, 0]).each do |response_map|
       if unsorted_teams_hash.key? response_map.reviewee_id
         unsorted_teams_hash[response_map.reviewee_id] += 1
       else
         unsorted_teams_hash[response_map.reviewee_id] = 1
       end
     end
     teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
     participants_with_insufficient_review_num.each do |participant_id|
       teams_hash.each do |team_id, _num_review_received|
         next if TeamsUser.exists?(team_id: team_id, 
                                   user_id: Participant.find(participant_id).user_id)
         ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, 
                                 reviewed_object_id: assignment_id).first_or_create
         teams_hash[team_id] += 1
         teams_hash = teams_hash.sort_by {|_, v| v }.to_h
         break
       end
     end
   end
   @@time_create_last_review_mapping_record = ReviewResponseMap.
                                              where(reviewed_object_id: assignment_id).
                                              last.created_at
 end

 def execute_peer_review_strategy(assignment_id, teams, num_participants, 
                                  student_review_num, num_reviews_per_team, 
                                  participants, participants_hash)
   # Exception detection: If instructor want to assign too many reviews done
   # by each student, there will be an error msg.
   if student_review_num >= teams.size
     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].'
   end
   peer_review_strategy(assignment_id, teams, num_participants, 
                        student_review_num, num_reviews_per_team, 
                        participants, participants_hash)
 end

Test

We write RSpec feature tests to prove that all changes are correct and the project performs well as before. The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice. Case 2. Instructor set both numbers (They are both not 0). The page will show a notice. Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times. Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'

 describe "review mapping", js: true do

   before(:each) do
     #create test data
   end

   it "show error when assign both 0" do
     login_as("instructor6")
     visit '/assignments/1/edit'
     find_link('ReviewStrategy').click
     select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
     fill_in 'num_reviews_per_student', with: 0
     choose 'num_reviews_submission'
     fill_in 'num_reviews_per_submission', with: 0
     click_button 'second_submit_tag'
     #click_button 'Save'
     expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')

   end
   it "show error when assign both numbers" do
     login_as("instructor6")
     visit '/assignments/1/edit'
     find_link('ReviewStrategy').click
     select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
     fill_in 'num_reviews_per_student', with: 1
     choose 'num_reviews_submission'
     fill_in 'num_reviews_per_submission', with: 1
     click_button 'second_submit_tag'
     #click_button 'Save'
     expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')

   end
   it "calculate reviewmapping from given review number per student" do
     login_as("instructor6")
     visit '/assignments/1/edit'
     find_link('ReviewStrategy').click
     select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
     fill_in 'num_reviews_per_student', with: 2
     click_button 'first_submit_tag'
     num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
     expect(num).to eq(7)
     num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
     expect(num2).to eq(6)

   end
   it "calculate reviewmapping from given review number per submission" do
     login_as("instructor6")
     visit '/assignments/1/edit'
     find_link('ReviewStrategy').click
     select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
     choose 'num_reviews_submission'
     fill_in 'num_reviews_per_submission', with: 7
     click_button 'second_submit_tag'
     #click_button 'Save'
     num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
     expect(num).to eq(2)

   end

 end