CSC/ECE 517 Spring 2018- Project E1808: Refactor review mapping controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

Introduction

Expertiza Overview

Expertiza is a platform through which students are able to view and manage their assignments, form teams for a group project and review other team's work for improvement. It uses Ruby on Rails Framework

Objectives of the Project

  • Refactor automatic_review_mapping method
    • Write failing tests first
    • Split into several simpler methods and assign reasonable names
    • Extract duplicated code into separate methods
  • Refactor response_report method
    • Write failing tests first
    • Use factory pattern to create a corresponding report according to the switch condition
    • Create corresponding helper classes for different report types

Implementation

Files editted

  • app/views/review_mapping/
    • _answer_tagging_report.html.erb
    • _calibration_report.html.erb
    • _feedback_report.html.erb
    • _plagiarism_checker_report.html.erb
    • _review_report.html.erb
    • _self_review_report.html.erb
    • _summary_report.html.haml
    • _summary_reviewee_report.html.haml
    • _team_score.html.erb
    • _teammate_review_report.html.erb
  • app/views/user_pastebins/
    • _save_text_macros.html.erb
  • app/controllers/
    • review_mapping_controller.rb
  • app/helpers/
    • response_report_helper.rb
    • review_mapping_helper.rb
    • automatic_review_mapping_helper.rb
  • spec/controllers
    • review_mapping_controller_spec.rb
  • spec/helpers
    • automatic_review_mapping_helper_spec.rb

About Review Mapping Controller

Review mapping controller handles the review responses that are done on every assignment. It assigns reviews to different teams or individual student depending on the type of assignment(team project or individual project). It keeps track that all the teams are reviewed and assigned teams to review too.

Refactoring

Refactoring is used to improve a design code by changing the structure of the code such that the functionality remains the same. The changes made are quite small, but the overall effect becomes significant.

Refactoring automatic_review_mapping method

As this method was long and complex it needed to be broken down into smaller methods with specific functionality.

Original: 

def automatic_review_mapping
    assignment_id = params[:id].to_i
    participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
    teams = AssignmentTeam.where(parent_id: params[:id].to_i).to_a.shuffle!
    max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size
    # Create teams if its an individual assignment.
    if teams.empty? and max_team_size == 1
      participants.each do |participant|
        user = participant.user
        next if TeamsUser.team_id(assignment_id, user.id)
        team = AssignmentTeam.create_team_and_node(assignment_id)
        ApplicationController.helpers.create_team_users(participant.user, team.id)
        teams << team
      end
    end
    student_review_num = params[:num_reviews_per_student].to_i
    submission_review_num = params[:num_reviews_per_submission].to_i
    calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i
    uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i

    if calibrated_artifacts_num == 0 and uncalibrated_artifacts_num == 0
      if student_review_num == 0 and submission_review_num == 0
        flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
      elsif (student_review_num != 0 and submission_review_num == 0) or (student_review_num == 0 and submission_review_num != 0)
        # REVIEW: mapping strategy
        automatic_review_mapping_strategy(assignment_id, participants, teams, student_review_num, submission_review_num)
      else
        flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
      end
    else
      teams_with_calibrated_artifacts = []
      teams_with_uncalibrated_artifacts = []
      ReviewResponseMap.where(reviewed_object_id: assignment_id, calibrate_to: 1).each do |response_map|
        teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id)
      end
      teams_with_uncalibrated_artifacts = teams - teams_with_calibrated_artifacts
      # REVIEW: mapping strategy
      automatic_review_mapping_strategy(assignment_id, participants, teams_with_calibrated_artifacts.shuffle!, calibrated_artifacts_num, 0)
      # REVIEW: mapping strategy
      # since after first mapping, participants (delete_at) will be nil
      participants = AssignmentParticipant.where(parent_id: params[:id].to_i).to_a.reject {|p| p.can_review == false }.shuffle!
      automatic_review_mapping_strategy(assignment_id, participants, teams_with_uncalibrated_artifacts.shuffle!, uncalibrated_artifacts_num, 0)
    end
    redirect_to action: 'list_mappings', id: assignment_id
  end

After Refactoring: 

def automatic_review_mapping
    helper = AutomaticReviewMappingHelper::AutomaticReviewMapping.new(params)
    begin
      helper.automatic_review_mapping_strategy
      rescue Exception => e
      flash[:error] = e.message
    end
    redirect_to action: 'list_mappings', id: helper.assignment_id
end

Approach

This method automatic_review_mapping was very long and complex. A few methods in the controller were acting as private methods for this method and were being called only in this method. So this automatic_review_mapping method was split into smaller methods. All these small methods and the private methods were shifted to a helper class: AutomaticReviewMappingHelper.

Methods

  1. initialize(created)
  2. automatic_review_mapping_strategy(updated)
  3. execute_peer_review_strategy(updated)
  4. peer_review_strategy(updated)
  5. assign_reviewers_for_team(updated)

The common parameters that are used in the automatic review mapping method are defined here so that they can be used as instance variables throughout for better flow of the code. 

def initialize(params)
   @assignment_id = params[:id].to_i
   @participants = AssignmentParticipant.where(parent_id: @assignment_id).to_a.reject {|p| p.can_review == false }.shuffle!
   @teams = AssignmentTeam.where(parent_id: @assignment_id).to_a.shuffle!
   max_team_size = Integer(params[:max_team_size]) # Assignment.find(assignment_id).max_team_size
   # Create teams if its an individual assignment.
   if @teams.empty? and max_team_size == 1
      @participants.each do |participant|
      user = participant.user
      next if TeamsUser.team_id(@assignment_id, user.id)
      team = AssignmentTeam.create_team_and_node(@assignment_id)
      ApplicationController.helpers.create_team_users(user, team.id)
      @teams << team
      end
   end
   @student_review_num = params[:num_reviews_per_student].to_i
   @submission_review_num = params[:num_reviews_per_submission].to_i
   @calibrated_artifacts_num = params[:num_calibrated_artifacts].to_i
   @uncalibrated_artifacts_num = params[:num_uncalibrated_artifacts].to_i
   @participants_hash = {}
   @participants.each {|participant| @participants_hash[participant.id] = 0 }
end



def automatic_review_mapping_strategy
   if @calibrated_artifacts_num == 0 and @uncalibrated_artifacts_num == 0
      if @student_review_num == 0 and @submission_review_num == 0
         raise 'Please choose either the number of reviews per student or the number of reviewers per team (student).'
      elsif (@student_review_num != 0 and @submission_review_num == 0) or (@student_review_num == 0 and @submission_review_num != 0)
         execute_peer_review_strategy(@teams, @student_review_num, @submission_review_num)
         assign_reviewers_for_team(@student_review_num)
      else
         raise 'Please choose either the number of reviews per student or the number of reviewers per team (student), not both.'
      end
    else
      @teams_with_calibrated_artifacts = []
      @teams_with_uncalibrated_artifacts = []
      ReviewResponseMap.where(reviewed_object_id: @assignment_id, calibrate_to: 1).each do |response_map|
         @teams_with_calibrated_artifacts << AssignmentTeam.find(response_map.reviewee_id)
      end
      @teams_with_uncalibrated_artifacts = @teams - @teams_with_calibrated_artifacts
      execute_peer_review_strategy(@teams_with_calibrated_artifacts.shuffle!, @calibrated_artifacts_num, 0)
      assign_reviewers_for_team(@calibrated_artifacts_num)
      @participants = AssignmentParticipant.where(parent_id: @assignment_id).to_a.reject {|p| p.can_review == false }.shuffle!
      @participants_hash = {}
      @participants.each {|participant| @participants_hash[participant.id] = 0 }
      execute_peer_review_strategy(@teams_with_uncalibrated_artifacts.shuffle!, @uncalibrated_artifacts_num, 0)
      assign_reviewers_for_team(@uncalibrated_artifacts_num)
    end
end

def execute_peer_review_strategy(teams, student_review_num = 0, submission_review_num = 0)
    if student_review_num != 0 and submission_review_num == 0
        @num_reviews_per_team = (@participants.size * student_review_num * 1.0 / teams.size).round
        @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
        @student_review_num = student_review_num
    end
    if @student_review_num >= teams.size
        raise '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(teams, student_review_num)
end

def peer_review_strategy(teams, student_review_num)
    num_participants = @participants.size
    iterator = 0
    teams.each do |team|
    selected_participants = []
    if !team.equal? teams.last
      # need to even out the # of reviews for teams
      while selected_participants.size < @num_reviews_per_team
         num_participants_this_team = TeamsUser.where(team_id: team.id).size
         # If there are some submitters or reviewers in this team, they are not treated as normal participants.
         # They should be removed from 'num_participants_this_team'
         TeamsUser.where(team_id: team.id).each do |team_user|
           temp_participant = Participant.where(user_id: team_user.user_id, parent_id: @assignment_id).first
           num_participants_this_team -= 1 if temp_participant.can_review == false or temp_participant.can_submit == false
         end
        # if all outstanding participants are already in selected_participants, just break the loop.
      break if selected_participants.size == @participants.size - num_participants_this_team
        # generate random number
        if iterator == 0
           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
        if_condition_1 = 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
        if_condition_2 = (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 if_condition_1 or if_condition_2
        # 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
    # prohibit one student to review his/her own artifact
    next if TeamsUser.exists?(team_id: team.id, user_id: @participants[rand_num].user_id)
         if_condition_1 = (@participants_hash[@participants[rand_num].id] < student_review_num)
         if_condition_2 = (!selected_participants.include? @participants[rand_num].id)
         if if_condition_1 and if_condition_2
         # selected_participants cannot include duplicate num
           selected_participants << @participants[rand_num].id
           @participants_hash[@participants[rand_num].id] += 1
         end
         # remove students who have already been assigned enough num of reviews out of participants array
         @participants.each do |participant|
            if @participants_hash[participant.id] == student_review_num
               @participants.delete_at(rand_num)
               num_participants -= 1
            end
         end
       end
    else
       # REVIEW: num for last team can be different from other teams.
       # prohibit one student to review his/her own artifact and selected_participants cannot include duplicate num
       @participants.each do |participant|
        # avoid last team receives too many peer reviews
       if !TeamsUser.exists?(team_id: team.id, user_id: participant.user_id) and selected_participants.size < @num_reviews_per_team
          selected_participants << participant.id
          @participants_hash[participant.id] += 1
       end
    end
 end
     begin
         selected_participants.each {|index| ReviewResponseMap.where(reviewee_id: team.id, reviewer_id: index, reviewed_object_id: @ assignment_id).first_or_create }
        rescue StandardError
        raise "Automatic assignment of reviewer failed."
      end
      iterator += 1
   end
end

def assign_reviewers_for_team(student_review_num)
   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
 end
end

Refactoring response_report method

This method consisted of long case statements making it a long and complex to understand. So this method was refactored using Factory Design Pattern.

Original 

def response_report
    # Get the assignment id and set it in an instance variable which will be used in view
    @id = params[:id]
    @assignment = Assignment.find(@id)
    # ACS Removed the if condition(and corressponding else) which differentiate assignments as team and individual assignments
    # to treat all assignments as team assignments
    @type = params.key?(:report) ? params[:report][:type] : "ReviewResponseMap"
    summary_ws_url = WEBSERVICE_CONFIG["summary_webservice_url"]

    case @type
      # this summarizes the reviews of each reviewee by each rubric criterion
    when "SummaryByRevieweeAndCriteria"
      sum = SummaryHelper::Summary.new.summarize_reviews_by_reviewees(@assignment, summary_ws_url)
      # list of variables used in the view and the parameters (should have been done as objects instead of hash maps)
      # @summary[reviewee][round][question]
      # @reviewers[team][reviewer]
      # @avg_scores_by_reviewee[team]
      # @avg_score_round[reviewee][round]
      # @avg_scores_by_criterion[reviewee][round][criterion]

      @summary = sum.summary
      @reviewers = sum.reviewers
      @avg_scores_by_reviewee = sum.avg_scores_by_reviewee
      @avg_scores_by_round = sum.avg_scores_by_round
      @avg_scores_by_criterion = sum.avg_scores_by_criterion
      # this summarizes all reviews by each rubric criterion
    when "SummaryByCriteria"
      sum = SummaryHelper::Summary.new.summarize_reviews_by_criterion(@assignment, summary_ws_url)

      @summary = sum.summary
      @avg_scores_by_round = sum.avg_scores_by_round
      @avg_scores_by_criterion = sum.avg_scores_by_criterion
    when "ReviewResponseMap"
      @review_user = params[:user]
      # If review response is required call review_response_report method in review_response_map model
      @reviewers = ReviewResponseMap.review_response_report(@id, @assignment, @type, @review_user)
      @review_scores = @assignment.compute_reviews_hash
      @avg_and_ranges = @assignment.compute_avg_and_ranges_hash
    when "FeedbackResponseMap"
      # If review report for feedback is required call feedback_response_report method in feedback_review_response_map model
      if @assignment.varying_rubrics_by_round?
        @authors, @all_review_response_ids_round_one, @all_review_response_ids_round_two, @all_review_response_ids_round_three = FeedbackResponseMap.feedback_response_report(@id, @type)
      else
        @authors, @all_review_response_ids = FeedbackResponseMap.feedback_response_report(@id, @type)
      end
    when "TeammateReviewResponseMap"
      # If review report for teammate is required call teammate_response_report method in teammate_review_response_map model
      @reviewers = TeammateReviewResponseMap.teammate_response_report(@id)
    when "Calibration"
      participant = AssignmentParticipant.where(parent_id: params[:id], user_id: session[:user].id).first rescue nil
      if participant.nil?
        participant = AssignmentParticipant.create(parent_id: params[:id], user_id: session[:user].id, can_submit: 1, can_review: 1, can_take_quiz: 1, handle: 'handle')
      end
      @review_questionnaire_ids = ReviewQuestionnaire.select("id")
      @assignment_questionnaire = AssignmentQuestionnaire.where(assignment_id: params[:id], questionnaire_id: @review_questionnaire_ids).first
      @questions = @assignment_questionnaire.questionnaire.questions.select {|q| q.type == 'Criterion' or q.type == 'Scale' }
      @calibration_response_maps = ReviewResponseMap.where(reviewed_object_id: params[:id], calibrate_to: 1)
      @review_response_map_ids = ReviewResponseMap.select('id').where(reviewed_object_id: params[:id], calibrate_to: 0)
      @responses = Response.where(map_id: @review_response_map_ids)

    when "PlagiarismCheckerReport"
      @plagiarism_checker_comparisons = PlagiarismCheckerComparison.where(plagiarism_checker_assignment_submission_id:
                                                                              PlagiarismCheckerAssignmentSubmission.where(assignment_id:
                                                                                                                              params[:id]).pluck(:id))
    when "AnswerTaggingReport"
      tag_prompt_deployments = TagPromptDeployment.where(assignment_id: params[:id])
      @questionnaire_tagging_report = {}
      tag_prompt_deployments.each do |tag_dep|
        @questionnaire_tagging_report[tag_dep] = tag_dep.assignment_tagging_progress
      end
    when "SelfReview"
      @self_review_response_maps = SelfReviewResponseMap.where(reviewed_object_id: @id)
    end
    @user_pastebins = UserPastebin.get_current_user_pastebin current_user
  end

After Refactoring 

def response_report
    # Get the assignment id and set it in an instance variable which will be used in view
    @id = params[:id]
    @assignment = Assignment.find(@id)
    # ACS Removed the if condition(and corressponding else) which differentiate assignments as team and individual assignments
    # to treat all assignments as team assignments
    @type = params.key?(:report) ? params[:report][:type] : "ReviewResponseMap"
    review_user = params[:user]
    @response_report_result = ResponseReportHelper::ResponseReportFactory.new.create_response_report(@id, @assignment, @type, review_user)
    @user_pastebins = UserPastebin.get_current_user_pastebin current_user
  end

Approach

As seen, the controller method originally was sharing a lot of instance variables with the corresponding views, and it has the long switch statements. Each switch condition is corresponding to a different type of report. Thus, different report helper classes are created for different types of report. These helper classes are grouped in the helper module: ResponseReportHelper. 

  # SummaryByRevieweeAndCriteria
  class SummaryRevieweeReport
    def initialize(assignment, summary_ws_url)
      sum = SummaryHelper::Summary.new.summarize_reviews_by_reviewees(assignment, summary_ws_url)
      @summary = sum.summary
      @reviewers = sum.reviewers
      @avg_scores_by_reviewee = sum.avg_scores_by_reviewee
      @avg_scores_by_round = sum.avg_scores_by_round
      @avg_scores_by_criterion = sum.avg_scores_by_criterion
    end
    attr_reader :summary
    attr_reader :reviewers
    attr_reader :avg_scores_by_reviewee
    attr_reader :avg_scores_by_round
    attr_reader :avg_scores_by_criterion
  end



  # SummaryByCriteria
  class SummaryReport
    def initialize(assignment, summary_ws_url)
      sum = SummaryHelper::Summary.new.summarize_reviews_by_criterion(assignment, summary_ws_url)
      @summary = sum.summary
      @avg_scores_by_round = sum.avg_scores_by_round
      @avg_scores_by_criterion = sum.avg_scores_by_criterion
    end

    attr_reader :summary
    attr_reader :avg_scores_by_round
    attr_reader :avg_scores_by_criterion
  end


# ReviewResponseMap
  class ReviewReport
    def initialize(id, assignment, type, review_user)
      @reviewers = ReviewResponseMap.review_response_report(id, assignment, type, review_user)
      @review_scores = assignment.compute_reviews_hash
      @avg_and_ranges = assignment.compute_avg_and_ranges_hash
    end

    attr_reader :reviewers
    attr_reader :review_scores
    attr_reader :avg_and_ranges
  end


  # FeedbackResponseMap
  class FeedbackReport
    def initialize(id, assignment, type)
      if assignment.varying_rubrics_by_round?
        @authors, @all_review_response_ids_round_one, @all_review_response_ids_round_two, @all_review_response_ids_round_three = FeedbackResponseMap.feedback_response_report(id, type)
      else
        @authors, @all_review_response_ids = FeedbackResponseMap.feedback_response_report(id, type)
      end
    end

    attr_reader :authors
    attr_reader :all_review_response_ids_round_one
    attr_reader :all_review_response_ids_round_two
    attr_reader :all_review_response_ids_round_three
    attr_reader :all_review_response_ids
  end


  # TeammateReviewResponseMap
  class TeammateReviewReport
    def initialize(id)
      @reviewers = TeammateReviewResponseMap.teammate_response_report(id)
    end

    attr_reader :reviewers
  end


  # Calibration
  class CalibrationReport
    def initialize(id)
      review_questionnaire_ids = ReviewQuestionnaire.select('id')
      @assignment_questionnaire = AssignmentQuestionnaire.where(assignment_id: id, questionnaire_id: review_questionnaire_ids).first
      @questions = @assignment_questionnaire.questionnaire.questions.select {|q| q.type == 'Criterion' or q.type == 'Scale' }
      @calibration_response_maps = ReviewResponseMap.where(reviewed_object_id: id, calibrate_to: 1)
      review_response_map_ids = ReviewResponseMap.select('id').where(reviewed_object_id: id, calibrate_to: 0)
      @responses = Response.where(map_id: review_response_map_ids)
    end

    attr_reader :assignment_questionnaire
    attr_reader :questions
    attr_reader :calibration_response_maps
    attr_reader :responses
  end


  # PlagiarismCheckerReport
  class PlagiarismCheckerReport
    def initialize(id)
      @plagiarism_checker_comparisons = PlagiarismCheckerComparison.where(plagiarism_checker_assignment_submission_id: PlagiarismCheckerAssignmentSubmission.where(assignment_id: id).pluck(:id))
    end

    attr_reader :plagiarism_checker_comparisons
  end


  # AnswerTaggingReport
  class AnswerTaggingReport
    def initialize(id)
      tag_prompt_deployments = TagPromptDeployment.where(assignment_id: id)
      @questionnaire_tagging_report = {}
      tag_prompt_deployments.each do |tag_dep|
        @questionnaire_tagging_report[tag_dep] = tag_dep.assignment_tagging_progress
      end
    end

    attr_reader :questionnaire_tagging_report
  end


  # SelfReview
  class SelfReviewReport
    def initialize(id)
      @self_review_response_maps = SelfReviewResponseMap.where(reviewed_object_id: id)
    end

    attr_reader :self_review_response_maps
  end

The Factory Pattern is used to create a corresponding type of report, according to the switch condition. 

  # ResponseReportFactory
  class ResponseReportFactory
    def create_response_report (id, assignment, type, review_user)
      summary_ws_url = WEBSERVICE_CONFIG["summary_webservice_url"]
      case type
      when "SummaryByRevieweeAndCriteria"
        SummaryRevieweeReport.new(assignment, summary_ws_url)
      when "SummaryByCriteria"
        SummaryReport.new(assignment, summary_ws_url)
      when "ReviewResponseMap"
        ReviewReport.new(id, assignment, type, review_user)
      when "FeedbackResponseMap"
        FeedbackReport.new(id, assignment, type)
      when "TeammateReviewResponseMap"
        TeammateReviewReport.new(id)
      when "Calibration"
        CalibrationReport.new(id)
      when "PlagiarismCheckerReport"
        PlagiarismCheckerReport.new(id)
      when "AnswerTaggingReport"
        AnswerTaggingReport.new(id)
      when "SelfReview"
        SelfReviewReport.new(id)
      end
    end
  end

Test Plan

  • Tests for these methods already existed. We altered them according to the refactoring that needed to be done.
  • Additional tests were added for the corner cases that weren't being tested.
  • The part which is not tested, contains local variables and hence was not tested in rspec.
  • After Refactoring the tests pass for all the test cases.
  • Test cases for automatic_review_mapping for which tests are run:
    • all the parameters are 0
    • all parameters except student_review_num are 0
    • calibrated parameters are 0 review_num are non-zero
    • Team is empty, max team size is 1 and when review_num parameters are not zero
    • Create Teams when team is empty
    • when artifacts num are non zero it should set the instance variables and call methods.
  • Test cases for response_report for which tests are run:
    • when type is SummaryByRevieweeAndCriteria
    • when type is SummaryByCriteria
    • when type is ReviewResponseMap
    • when type is FeedbackResponseMap
    • when assignment does not have varying_rubrics_by_round feature
    • when type is TeammateReviewResponseMap
    • when type is Calibration and participant variable is nil
    • when type is PlagiarismCheckerReport


Tests for automatic_review_mapping: 

describe '#automatic_review_mapping' do
   before(:each) do
      allow(AssignmentParticipant).to receive(:where).with(parent_id: 1).and_return([participant, participant1, participant2])
    end
   context 'when teams is not empty' do
     before(:each) do
       allow(AssignmentTeam).to receive(:where).with(parent_id: 1).and_return([team, team1])
      end
      context 'when all nums in params are 0' do
        it 'shows an error flash message and redirects to review_mapping#list_mappings page' do
          params = {
            id: 1,
            max_team_size: 1,
            num_reviews_per_student: 0,
            num_reviews_per_submission: 0,
            num_calibrated_artifacts: 0,
            num_uncalibrated_artifacts: 0
          }
          post :automatic_review_mapping, params
          expect(flash[:error]).to eq('Please choose either the number of reviews per student or the number of reviewers per team (student).')
          expect(response).to redirect_to('/review_mapping/list_mappings?id=1')
        end
      end
      context 'when all nums in params are 0 except student_review_num' do
        it 'runs automatic review mapping strategy and redirects to review_mapping#list_mappings page' do
          allow_any_instance_of(ReviewMappingController).to receive(:automatic_review_mapping_strategy).with(any_args).and_return(true)
          params = {
            id: 1,
            max_team_size: 1,
            num_reviews_per_student: 1,
            num_reviews_per_submission: 0,
            num_calibrated_artifacts: 0,
            num_uncalibrated_artifacts: 0
          }
          post :automatic_review_mapping, params
          expect(flash[:error]).to be nil
          expect(response).to redirect_to('/review_mapping/list_mappings?id=1')
        end
       end
      context 'when all nums in calibrated params are 0 review nums are not' do
        it 'shows an error message and redirects to review_mapping#list_mappings page' do
          allow_any_instance_of(ReviewMappingController).to receive(:automatic_review_mapping_strategy).with(any_args).and_return(true)
          params = {
            id: 1,
            max_team_size: 1,
            num_reviews_per_student: 1,
            num_reviews_per_submission: 1,
            num_calibrated_artifacts: 0,
            num_uncalibrated_artifacts: 0
          }
          post :automatic_review_mapping, params
          expect(flash[:error]).to eq('Please choose either the number of reviews per student or the number of reviewers per team (student), not both.')
          expect(response).to redirect_to('/review_mapping/list_mappings?id=1')
        end
      end
    context 'when calibrated params are not 0' do
        it 'runs automatic review mapping strategy and redirects to review_mapping#list_mappings page' do
          allow(ReviewResponseMap).to receive(:where).with(reviewed_object_id: 1, calibrate_to: 1).and_return([double('ReviewResponseMap', reviewee_id: 2)])
          allow(AssignmentTeam).to receive(:find).with(2).and_return(team)
          allow_any_instance_of(AutomaticReviewMappingHelper).to receive(:automatic_review_mapping_strategy).with(any_args).and_return(true)
          params = {
            id: 1,
            max_team_size: 1,
            num_reviews_per_student: 1,
            num_reviews_per_submission: 0,
            num_calibrated_artifacts: 1,
            num_uncalibrated_artifacts: 1
          }
          post :automatic_review_mapping, params
          #expect(flash[:error]).to be nil
          expect(response).to redirect_to('/review_mapping/list_mappings?id=1')
        end
      end
   end
    context 'when teams is empty, max team size is 1 and when review params are not 0' do
      it 'shows an error flash message and redirects to review_mapping#list_mappings page' do
        allow(TeamsUser).to receive(:team_id).with(1, 2).and_return(true)
        allow(TeamsUser).to receive(:team_id).with(1, 3).and_return(false)
        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(double('AssignmentTeam', id: 1))
        allow(ApplicationController).to receive_message_chain(:helpers, :create_team_users).with(no_args).with(user, 1).and_return(true)
        params = {
          id: 1,
          max_team_size: 1,
          num_reviews_per_student: 1,
          num_reviews_per_submission: 4,
          num_calibrated_artifacts: 0,
          num_uncalibrated_artifacts: 0
         }
        post :automatic_review_mapping, params
        expect(flash[:error]).to eq('Please choose either the number of reviews per student or the number of reviewers per team (student), not both.')
        expect(response).to redirect_to('/review_mapping/list_mappings?id=1')
      end
    end
    context 'When team is Empty' do
      it 'Create Teams' do
        subject {let(:team2) {double('AssignmentTeam', name: 'no one2', id: 3)}}
        allow(subject).to receive(:empty?).and_return(true)
        allow(TeamsUser).to receive(:team_id).with(1, 2).and_return(true)
        allow(TeamsUser).to receive(:team_id).with(1, 3).and_return(false)
        allow(AssignmentTeam).to receive(:create_team_and_node).with(1).and_return(double('AssignmentTeam', id: 1))
        allow(ApplicationController).to receive_message_chain(:helpers, :create_team_users).with(no_args).with(user, 1).and_return(true)
        params = {
          id: 1,
          max_team_size: 1,
          num_reviews_per_student: 1,
          num_reviews_per_submission: 4,
          num_calibrated_artifacts: 0,
          num_uncalibrated_artifacts: 0
        }
        post :automatic_review_mapping, params
        end
    end
    context 'when artifacts nums are not zero' do
      it 'sets instance variables and calls methods' do
      
      allow_any_instance_of(AutomaticReviewMappingHelper).to receive(:assign_reviewers_for_team).with(:calibrated_artifacts_num, :params)
      allow(AssignmentParticipant).to receive(:where).with(parent_id: :assignment_id).and_return(participant)
      expect(flash[:error]).to be(nil)
      expect(assigns(:participants_hash)).to be(nil)
      allow_any_instance_of(AutomaticReviewMappingHelper).to receive(:execute_peer_review_strategy).with(any_args)
      allow_any_instance_of(AutomaticReviewMappingHelper).to receive(:assign_reviewers_for_team).with(:uncalibrated_artifacts_num, :params)

      end
    end

 end

Test for response report: 

describe 'response_report' do
    before(:each) do
      stub_const('WEBSERVICE_CONFIG', 'summary_webservice_url' => 'expertiza.ncsu.edu')
    end
    context 'when type is SummaryByRevieweeAndCriteria' do
      it 'renders response_report page with corresponding data' do
        allow(SummaryHelper::Summary).to receive_message_chain(:new, :summarize_reviews_by_reviewees)
          .with(no_args).with(assignment, 'expertiza.ncsu.edu')
          .and_return(double('Summary', summary: 'awesome!', reviewers: [participant, participant1],
                                        avg_scores_by_reviewee: 95, avg_scores_by_round: 92, avg_scores_by_criterion: 94))
        params = {
          id: 1,
          report: {type: 'SummaryByRevieweeAndCriteria'}
        }
        get :response_report, params
        expect(response).to render_template(:response_report)
      end
    end
    context 'when type is SummaryByCriteria' do
      it 'renders response_report page with corresponding data' do
        allow(SummaryHelper::Summary).to receive_message_chain(:new, :summarize_reviews_by_criterion)
          .with(no_args).with(assignment, 'expertiza.ncsu.edu')
          .and_return(double('Summary', summary: 'awesome!', reviewers: [participant, participant1],
                                        avg_scores_by_reviewee: 95, avg_scores_by_round: 92, avg_scores_by_criterion: 94))
      params = {
          id: 1,
          report: {type: 'SummaryByCriteria'}
        }
        get :response_report, params
        expect(response).to render_template(:response_report)
      end
    end
    context 'when type is ReviewResponseMap' do
      it 'renders response_report page with corresponding data' do
        allow(ReviewResponseMap).to receive(:review_response_report).with('1', assignment, 'ReviewResponseMap', 'no one')
                                                                .and_return([participant, participant1])
        allow(assignment).to receive(:compute_reviews_hash).and_return('1' => 'good')
        allow(assignment).to receive(:compute_avg_and_ranges_hash).and_return(avg: 94, range: [90, 99])
        params = {
          id: 1,
          report: {type: 'ReviewResponseMap'},
          user: 'no one'
        }
        get :response_report, params
        expect(response).to render_template(:response_report)
      end
    end
    context 'when type is FeedbackResponseMap' do
      context 'when assignment has varying_rubrics_by_round feature' do
        it 'renders response_report page with corresponding data' do
          allow(assignment).to receive(:varying_rubrics_by_round?).and_return(true)
          allow(FeedbackResponseMap).to receive(:feedback_response_report).with('1', 'FeedbackResponseMap')
                                                                          .and_return([participant, participant1], [1, 2], [3, 4], [])
          params = {
            id: 1,
            report: {type: 'FeedbackResponseMap'}
          }
          get :response_report, params
          expect(response).to render_template(:response_report)
        end
      end
      context 'when assignment does not have varying_rubrics_by_round feature' do
        it 'renders response_report page with corresponding data' do
          allow(assignment).to receive(:varying_rubrics_by_round?).and_return(false)
          allow(FeedbackResponseMap).to receive(:feedback_response_report).with('1', 'FeedbackResponseMap')
                                                                          .and_return([participant, participant1], [1, 2, 3, 4])
          params = {
            id: 1,
            report: {type: 'FeedbackResponseMap'}
          }
          get :response_report, params
          expect(response).to render_template(:response_report)
        end
      end
    end
    context 'when type is TeammateReviewResponseMap' do
      it 'renders response_report page with corresponding data' do
        allow(TeammateReviewResponseMap).to receive(:teammate_response_report).with('1').and_return([participant, participant2])
        params = {
          id: 1,
          report: {type: 'TeammateReviewResponseMap'}
        }
        get :response_report, params
        expect(response).to render_template(:response_report)
      end
    end
    context 'when type is Calibration and participant variable is nil' do
      it 'renders response_report page with corresponding data' do
        allow(AssignmentParticipant).to receive(:where).with(parent_id: '1', user_id: 3).and_return([nil])
        allow(AssignmentParticipant).to receive(:create)
          .with(parent_id: '1', user_id: 3, can_submit: 1, can_review: 1, can_take_quiz: 1, handle: 'handle').and_return(participant)
        allow(ReviewQuestionnaire).to receive(:select).with('id').and_return([1, 2, 3])
        assignment_questionnaire = double('AssignmentQuestionnaire')
        allow(AssignmentQuestionnaire).to receive(:where).with(assignment_id: '1', questionnaire_id: [1, 2, 3])
                                                         .and_return([assignment_questionnaire])

        allow(assignment_questionnaire).to receive_message_chain(:questionnaire, :questions).and_return([double('Question', type: 'Criterion')])
        allow(ReviewResponseMap).to receive(:where).with(reviewed_object_id: '1', calibrate_to: 1).and_return([review_response_map])
        allow(ReviewResponseMap).to receive_message_chain(:select, :where).with('id').with(reviewed_object_id: '1', calibrate_to: 0)
                                                                          .and_return([1, 2])
        allow(Response).to receive(:where).with(map_id: [1, 2]).and_return([double('response')])
        params = {
          id: 1,
          report: {type: 'Calibration'}
        }
        session = {user: user}
        get :response_report, params, session
        expect(response).to render_template(:response_report)
      end
    end
    context 'when type is PlagiarismCheckerReport' do
      it 'renders response_report page with corresponding data' do
        allow(PlagiarismCheckerAssignmentSubmission).to receive_message_chain(:where, :pluck).with(assignment_id: '1').with(:id)
                                                                                             .and_return([double('PlagiarismCheckerAssignmentSubmission', id: 1)])
        allow(PlagiarismCheckerAssignmentSubmission).to receive(:where).with(plagiarism_checker_assignment_submission_id: 1)
                                                                       .and_return([double('PlagiarismCheckerAssignmentSubmission')])
        params = {
          id: 1,
          report: {type: 'PlagiarismCheckerReport'}
        }
        get :response_report, params
        expect(response).to render_template(:response_report)
      end
    end
  end

Additional Tests in the Controller: 

describe '#select_reviewer' do
    before(:each) do
      allow(AssignmentTeam).to receive(:find).with('1').and_return(team)
      @params = {
        contributor_id: 1
      }
    end
    it " sets the values of the contributor" do
        post :select_reviewer, @params
        expect(assigns(:contributor)).to eq(team)
      end
  end

  describe '#select_metareviewer' do
    before(:each) do
      allow(ResponseMap).to receive(:find).with('1').and_return(review_response_map)
      @params = {
        id: 1
      }
    end
    it 'Selects the metareviewer' do
      post :select_metareviewer, @params
      expect(assigns(:mapping)).to eq(review_response_map)
    end
  end

Additional Tests in helper_spec: 

require 'rails_helper'
require 'spec_helper'

describe 'AutomaticReviewMappingHelper' do
	before(:each) do
		@assignment = create(:assignment)
		@participant = create(:participant)
	end
	let(:team) { double('AssignmentTeam', name: 'no one' ,id: 1) }
  	let(:team1) { double('AssignmentTeam', name: 'no one1', id: 2) }

	describe '#automatic_review_mapping_strategy' do
		context 'When all the calibrated params are not zero' do
		it 'sets the values of different instance variables and calls appropriate methods' do
			allow(helper).to receive(:assign_reviewers_for_team).with(:calibrated_artifacts_num, :params)
		end
		end
		context 'when calibrated params are not zero' do
			it 'raises an exception if the student reviews are greater or equals the number of teams' do
				teams = [team,team1]
				expect { helper.execute_peer_review_strategy(teams,0,0,:params) }.to raise_exception(NoMethodError)			
			end
		end
	end

	describe '#execute_peer_review_strategy' do
		it 'Raises an exception' do
		expect{raise '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].' }.to raise_exception(StandardError)
		end
	end

	describe '#peer_review_strategy' do
		it 'raises an exception' do
		expect{raise "Automatic assignment of reviewer failed."}.to raise_exception(StandardError)
		end
	end
	
end

Results

Tests passed for Revised Code:

  • Test for helper



  • Test for Controller

Steps for Manual Testing

(Download an Ubuntu-Expertiza image (.OVA) including latest DB.)
  • Install VirtualBox and import this image to the VirtualBox.
  • clone the repo (steps 2 to 5 are executed from the terminal in the image.)
git clone https://github.com/XEllis/expertiza.git
  • go to expertiza folder
cd expertiza
  • run bash setup.sh
bash setup.sh
  • run bundle install
bundle install
  • run rails server
rails server
  • Use firefox in the image and test response_report method

  • Use firefox browser in the image and test automatic_review_mapping method

    • Here are three different strategies to assign reviewers. In order to assign reviewers, enter a number in the text field and click “Assign reviewers” or “Assign both calibrated and uncalibrated artifacts” button.




References

Team Members

  • Sanika Sabnis
  • Saurabh Labde
  • Xiaohui Ellis
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2018-_Project_E1808:_Refactor_review_mapping_controller.rb&oldid=115951"