CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(64 intermediate revisions by 3 users not shown)
Line 1: Line 1:
==Background==
=Background=


==Current Implementation and Problems==
This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.  
# 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.
# Method response_report has some sql - like code.  Rewrite with Active Record.
# 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.
# 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.
# 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.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# 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.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.


==Changes Implemented==
=Current Implementation and Problems=
#  
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.
#  
 
#  
The problems are:
# 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.
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. 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.
# 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).
# Method ''response_report'' has some SQL - like code.  Rewrite with Active Record.
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# 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.
# 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.
<source lang="ruby">
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
# 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.
    if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.
                                assignment_id, \
 
                                @@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# 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).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.
 
=Code=
 
==Refactor SQL query code==
 
The rails query we change in this controller:
 
1. From
 
  ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])
 
To
 
  ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
 
2. From
 
  MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])
 
To
 
  MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)
 
3. From
 
                  @assignments = Assignment
                  .where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
                  .order('name')
                  .page(params[:page])
                  .per_page(10)
 
To
 
                  @assignments = Assignment
                  .where(instructor_id:session[:user].id)
                  .where("substring(name,1,1) = :letter",{letter:letter})
                  .order('name')
                  .page(params[:page])
                  .per_page(10)
 
4.  From
 
  ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])
 
To
 
  ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)
 
5. From
 
  ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])
 
To
 
  ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})
 
6. From
 
  AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])
 
To
  AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)
 
7. From
 
  SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])
 
To
 
  SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])
 
==Refactor long line method==
 
  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:assignment_id,calibrate_to:0)
            .where("created_at > :time",
          {time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
       participants_with_insufficient_review_num = []
       participants_with_insufficient_review_num = []
       participants_hash.each do |participant_id, review_num|
       participants_hash.each do |participant_id, review_num|
Line 30: Line 136:
       end
       end
       unsorted_teams_hash = {}
       unsorted_teams_hash = {}
      ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
    ReviewResponseMap.where(reviewed_object_id:assignment_id,
                              assignment_id, 0]).each do |response_map|
                  calibrate_to:0).each do |response_map|
         unsorted_teams_hash[response_map.reviewee_id] = \
         if unsorted_teams_hash.key? response_map.reviewee_id
           calculate_unsorted_teams_hash_reviewee_id(unsorted_teams_hash, response_map)
          unsorted_teams_hash[response_map.reviewee_id] += 1
        else
           unsorted_teams_hash[response_map.reviewee_id] = 1
        end
       end
       end
       teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
       teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
       participants_with_insufficient_review_num.each do |participant_id|
       participants_with_insufficient_review_num.each do |participant_id|
         teams_hash.each do |team_id, _num_review_received|
         teams_hash.each do |team_id, _num_review_received|
           next if TeamsUser.exists?(team_id: team_id, \
           next if TeamsUser.exists?(team_id: team_id,  
                                     user_id: Participant.find(participant_id).user_id)
                                     user_id: Participant.find(participant_id).user_id)
           ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
           ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,  
                                   reviewed_object_id: assignment_id).first_or_create
                                   reviewed_object_id: assignment_id).first_or_create
           teams_hash[team_id] += 1
           teams_hash[team_id] += 1
Line 48: Line 157:
       end
       end
     end
     end
     @@time_create_last_review_mapping_record = ReviewResponseMap.\
     @@time_create_last_review_mapping_record = ReviewResponseMap.
                                               where(reviewed_object_id: assignment_id).\
                                               where(reviewed_object_id: assignment_id).
                                               last.created_at
                                               last.created_at
   end
   end
</source>


==Test==
  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
 
== Refactor add_reviewer ==
def add_reviewer
    msg = ''
    case params[:object]
    when 'reviewer'
      topic_id = params[:topic_id]
      user_id = User.where(name: params[:user][:name]).first.id
      # If instructor want to assign one student to review his/her own artifact,
      # it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
      if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
        flash[:error] = "You cannot assign this student to review his/her own artifact."
      else
        # Team lazy initialization
        assignment = Assignment.find(params[:id])
        assignment_id = assignment.id
        SignUpSheet.signup_team(assignment.id, user_id, topic_id)
        create_reviewer(assignment)
      end
    when 'metareviewer'
      mapping = ResponseMap.find(params[:id])
      assignment_id = mapping.assignment.id
      create_reviewer(mapping)
    end
    redirect_to action: 'list_mappings', id: assignment_id, msg: msg
  end
 
def create_reviewer(object)
  begin
    user = User.from_params(params)
    # contributor_id is team_id
    case params[:object]
    when 'reviewer'
      assignment = object # assignment
      reviewee_id = params[:contributor_id]
      reviewed_object_id = assignment.id
      response_map = ReviewResponseMap
      id_symbol = :reviewee_id
      id = reviewee_id
      assignment = assignment
      regurl = url_for action: 'add_user_to_assignment',
                      id: assignment.id,
                      user_id: user.id,
                      contributor_id: params[:contributor_id]
    when 'metareviewer'
      mapping = object # mapping
      reviewee_id = mapping.reviewer.id
      reviewed_object_id = mapping.map_id
      response_map = MetareviewResponseMap
      id_symbol = :reviewed_object_id
      id = reviewed_object_id
      assignment = mapping.assignment
      regurl = url_for action: 'add_user_to_assignment',
                      id: mapping.map_id,
                      user_id: user.id
    end
    # Get the assignment's participant corresponding to the user
    reviewer = get_reviewer(user, assignment, regurl)
    # ACS Removed the if condition(and corressponding else) which differentiate
    # assignments as team and individual assignments
    # to treat all assignments as team assignments
    if  response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
      response_map.create(reviewee_id: reviewee_id,
                          reviewer_id: reviewer.id,
                          reviewed_object_id: reviewed_object_id)
    else
      case params[:object]
      when 'reviewer'
        raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
      when 'metareviewer'
        raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
      end
    end
  rescue
    msg = $ERROR_INFO
  end
end
 
== Refactor delete_reviewer ==
 
def delete_reviewer
    case params[:object]
    when 'reviewer'
      response_map = ReviewResponseMap
    when 'metareviewer'
      response_map = MetareviewResponseMap
    end
    # duplicate code
    mapping = response_map.find(params[:id])
    assignment_id = mapping.assignment.id
    case params[:object]
    when 'reviewer'
      if !Response.exists?(map_id: mapping.id)
        mapping.destroy
        flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
      else
        flash[:error] = "This review has already been done. It cannot been deleted."
      end
    when 'metareviewer'
      flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
      begin
        mapping.delete
      rescue
        flash[:error] = "A delete action failed:<br/>"
        + $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
        + mapping.map_id.to_s + "'>Delete this mapping anyway>?"
      end
    end
    redirect_to action: 'list_mappings', id: assignment_id
  end
 
== Refactor delete_all_reviewer ==
 
def delete_all_reviewers
    mapping = ResponseMap.find(params[:id])
    case params[:object]
    when 'all_metareviewers'
      response_map = MetareviewResponseMap
      id_symbol = :reviewed_object_id
      id = mapping.map_id
    end
    mmappings = response_map.where(id_symbol => id)
    failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
    if failedCount > 0
      url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
      url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
      flash[:error] = "A delete action failed:<br/>#{failedCount} metareviews exist for these mappings. Delete these mappings anyway?&nbsp;<a href='#{url_yes}'>Yes</a>&nbsp;|&nbsp;<a href='#{url_no}'>No</a><BR/>"
    else
      flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
    end
    redirect_to action: 'list_mappings', id: mapping.assignment.id
  end
 
=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console
  ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
  ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)
 
  Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
  Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')
 
  ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
  ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})
 
# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method
 
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
 
Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
    it "can add reviewer then delete it" do
      @student_reviewer = create :student,name:'student_reviewer'
      @participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
      @student_reviewer2 = create :student,name:'student_reviewer2'
      @participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
      login_and_assign_reviewer("instructor6",@assignment.id,0,0)
      first(:link,'add reviewer').click
      add_reviewer(@student_reviewer.name)
      expect(page).to have_content @student_reviewer.name
      click_link('delete')
      expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")
      first(:link,'add reviewer').click
      add_reviewer(@student_reviewer.name)
      click_link('add metareviewer')
      add_matareviewer(@student_reviewer2.name)
      expect(page).to have_content @student_reviewer2.name
      find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
      expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")
      click_link('add metareviewer')
      add_matareviewer(@student_reviewer2.name)
      click_link('delete all metareviewers')
      expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")
      first(:link,'delete outstanding reviewers').click
      expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")
    end

Latest revision as of 07:20, 5 November 2016

Background

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

Current Implementation and Problems

The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:

  1. The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. 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. Please remove this method and caller.
  4. There was a self-review feature, the method add_self_reviewer, get_team_from_submission are related to this. Two views calls add_self_reviewer are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to self_review feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
  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

  1. Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
  2. Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
  3. We search the function name(add_user_to_assignment) in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of add_reviewer and add_metareviewer in the controller of review_mapping_controller
  4. By searching the whole project and routes, we verify that methods add_self_reviewer and get_team_from_submission in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". 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. Method automatic_review_mapping_strategy have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

Code

Refactor SQL query code

The rails query we change in this controller:

1. From

 ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) 

To

 ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

 MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

 MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

                  @assignments = Assignment
                  .where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
                  .order('name')
                  .page(params[:page])
                  .per_page(10)

To

                  @assignments = Assignment
                  .where(instructor_id:session[:user].id)
                  .where("substring(name,1,1) = :letter",{letter:letter})
                  .order('name')
                  .page(params[:page])
                  .per_page(10)

4. From

 ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

 ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

 ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

 ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

 AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

 AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

 SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

 SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

Refactor long line method

 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:assignment_id,calibrate_to:0)
            .where("created_at > :time",
         {time:@@time_create_last_review_mapping_record}).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:assignment_id,
                 calibrate_to: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

Refactor add_reviewer

def add_reviewer
   msg = 
   case params[:object]
   when 'reviewer'
     topic_id = params[:topic_id]
     user_id = User.where(name: params[:user][:name]).first.id
     # If instructor want to assign one student to review his/her own artifact,
     # it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
     if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
       flash[:error] = "You cannot assign this student to review his/her own artifact."
     else
       # Team lazy initialization
       assignment = Assignment.find(params[:id])
       assignment_id = assignment.id
       SignUpSheet.signup_team(assignment.id, user_id, topic_id)
       create_reviewer(assignment)
     end
   when 'metareviewer'
     mapping = ResponseMap.find(params[:id])
     assignment_id = mapping.assignment.id
     create_reviewer(mapping)
   end
   redirect_to action: 'list_mappings', id: assignment_id, msg: msg
 end
def create_reviewer(object)
 begin
   user = User.from_params(params)
   # contributor_id is team_id
   case params[:object]
   when 'reviewer'
     assignment = object # assignment
     reviewee_id = params[:contributor_id]
     reviewed_object_id = assignment.id
     response_map = ReviewResponseMap
     id_symbol = :reviewee_id
     id = reviewee_id
     assignment = assignment
     regurl = url_for action: 'add_user_to_assignment',
                      id: assignment.id,
                      user_id: user.id,
                      contributor_id: params[:contributor_id]
   when 'metareviewer'
     mapping = object # mapping
     reviewee_id = mapping.reviewer.id
     reviewed_object_id = mapping.map_id
     response_map = MetareviewResponseMap
     id_symbol = :reviewed_object_id
     id = reviewed_object_id
     assignment = mapping.assignment
     regurl = url_for action: 'add_user_to_assignment',
                      id: mapping.map_id,
                      user_id: user.id
   end
   # Get the assignment's participant corresponding to the user
   reviewer = get_reviewer(user, assignment, regurl)
   # ACS Removed the if condition(and corressponding else) which differentiate
   # assignments as team and individual assignments
   # to treat all assignments as team assignments
   if  response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
     response_map.create(reviewee_id: reviewee_id,
                         reviewer_id: reviewer.id,
                         reviewed_object_id: reviewed_object_id)
   else
     case params[:object]
     when 'reviewer'
       raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
     when 'metareviewer'
       raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
     end
   end
 rescue
   msg = $ERROR_INFO
 end
end

Refactor delete_reviewer

def delete_reviewer

   case params[:object]
   when 'reviewer'
     response_map = ReviewResponseMap
   when 'metareviewer'
     response_map = MetareviewResponseMap
   end
   # duplicate code
   mapping = response_map.find(params[:id])
   assignment_id = mapping.assignment.id
   case params[:object]
   when 'reviewer'
     if !Response.exists?(map_id: mapping.id)
       mapping.destroy
       flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
     else
       flash[:error] = "This review has already been done. It cannot been deleted."
     end
   when 'metareviewer'
     flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
     begin
       mapping.delete
     rescue
       flash[:error] = "A delete action failed:
" + $ERROR_INFO + "<a href='/review_mapping/delete_metareview/" + mapping.map_id.to_s + "'>Delete this mapping anyway>?" end end redirect_to action: 'list_mappings', id: assignment_id end

Refactor delete_all_reviewer

def delete_all_reviewers

   mapping = ResponseMap.find(params[:id])
   case params[:object]
   when 'all_metareviewers'
     response_map = MetareviewResponseMap
     id_symbol = :reviewed_object_id
     id = mapping.map_id
   end
   mmappings = response_map.where(id_symbol => id)
   failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
   if failedCount > 0
     url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
     url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
     flash[:error] = "A delete action failed:
#{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a>
" else flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted." end redirect_to action: 'list_mappings', id: mapping.assignment.id end

Test

  1. We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
  2. We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
  3. We use some data to test the result of old query and new query in rails console
 ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
 ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)
 Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
 Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')
 ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
 ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})
  1. We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.

Automatic_review_mapping method

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

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.

   it "can add reviewer then delete it" do

      @student_reviewer = create :student,name:'student_reviewer'
      @participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
      @student_reviewer2 = create :student,name:'student_reviewer2'
      @participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
      login_and_assign_reviewer("instructor6",@assignment.id,0,0)

      first(:link,'add reviewer').click
      add_reviewer(@student_reviewer.name)
      expect(page).to have_content @student_reviewer.name
      click_link('delete')
      expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

      first(:link,'add reviewer').click
      add_reviewer(@student_reviewer.name)
      click_link('add metareviewer')
      add_matareviewer(@student_reviewer2.name)
      expect(page).to have_content @student_reviewer2.name
      find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
      expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

      click_link('add metareviewer')
      add_matareviewer(@student_reviewer2.name)
      click_link('delete all metareviewers')
      expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

      first(:link,'delete outstanding reviewers').click
      expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

    end