CSC/ECE 517 Fall 2017/E1750 Refactor review mapping controller.rb

About Expertiza

Expertiza is an open source project base on Ruby on Rails framework. It allows students interact with each other through different assignment. It provide service as bidding on project topics, reviewing your teammate for different categories, and submitting assignments online. Expertiza also allows instructors to create assignment with different rubric and creating different topics for assignments that student can bid on.

Test Plan

Test-Driven Development(TDD)

A software development process begin by writes an (initially failing) automated test case that defines a desired improvement or a new function, and generate the minimum amount of code to pass that test. It help the developer to focus on a smaller portions of functionality at a time. Also the refactoring process is safe since it happens after the test is written. Test can also serve as a documentation to a developer.

Project Goal

review_mapping_controller.rb handles peer review response, and author-feedback response which is a complex file. It contains long methods that would be hard to understand. This project focused on breaking down some of the method in a form that would be easier to read/understand. Before starting the refracting task, failing test cases needs to be completed for the corresponding method in the review mapping controller. After completing the refactoring certain method if the review mapping controller then failing test should have a new status of pass.

Steps to Follow

• Complete pending tests in review_mapping_controller_spec.rb, and write integration tests for newly-created methods.
• Refactor automatic_review_mapping method

• Refactor response_report method

• Use find_by instead of dynamic method (L135)

• Use sort_by(&:name) instead of sort { |a, b| a.name <=> b.name } (L275)

List of Changes

Files modified

1. review mapping controller

• Refactor methods

2.review mapping_controller spec.rb

• Finished corresponding test for exiting method as well as created new test for newly added method.

Examples of Tests

Here are two of the examples of the test cases. Tests on assign_metareviewer_dynamically method which assigns a assignment to the corresponding student. The describe section of the test introduce the controller method this test focus on. Followed by an description of the result on would be expecting from this test for developers to follow. The different expect conditions expressed as each test should make only one assertion which helps us finding different errors in the code.

describe '#assign_metareviewer_dynamically' do
   it 'redirects to student_review#list page' do
     expect(Assignment).to receive(:find).and_return(assignment)
     expect(AssignmentParticipant).to receive(:where).and_return([participant])
     expect(assignment).to receive(:assign_metareviewer_dynamically)
     get :assign_metareviewer_dynamically
     expect(response).to redirect_to ('/student_review/list?id=' +participant.id.to_s)
   end
 end

Here is another example tests on add_reviewer and get_reviewer method which assigns reviewer to student's work. The test is divided into two different sections, and each section specifies one (and only one) behavior. This way will delivery a test that test all different possibilities and give the exact location of the error when it happens. This test followed the same step as the above test.

describe '#add_reviewer and #get_reviewer' do
   context 'when team_user does not exist' do
     it 'shows an error message and redirects to review_mapping#list_mappings page' do
       expect(User).to receive_message_chain("where.first.id").with(any_args).and_return(user.id)
       expect(TeamsUser).to receive(:exists?).with(any_args).and_return(true)
       #expect(flash[:error]).to match "You cannot assign this student to review his/her own artifact"
       post :add_reviewer, :contributor_id => '1', :id =>'1', :topic_id =>'2', user: {name: '2'}
       expect(flash[:error]) =~ /you cannot assign this student to review his.*her own artifact/i
       expect(response).to redirect_to action: 'list_mappings', id: assignment.id
     end
   end
   context 'when team_user exists and get_reviewer method returns a reviewer' do
     it 'creates a whole bunch of objects and redirects to review_mapping#list_mappings page' do
       expect(User).to receive_message_chain("where.first.id").with(any_args).and_return(user.id)
       expect(TeamsUser).to receive(:exists?).with(any_args).and_return(false)
       expect(SignUpSheet).to receive(:signup_team).with(any_args)
       expect(User).to receive(:from_params).with(any_args).and_return(user)
       expect(AssignmentParticipant).to receive_message_chain("where.first").with(any_args).and_return(participant)
       expect(ReviewResponseMap).to receive(:create).with(any_args)
       post :add_reviewer, :contributor_id => '1', :id =>'1', :topic_id =>'2', user: {name: '2'}
       expect(response).to redirect_to action: 'list_mappings', id: assignment.id, msg: 
     end
   end
 end

Examples of Refactoring

In this example, we divided automatic_review_mapping method into 4 different simpler method so the code will be easier to follow. As before, the whole method is filled with nested if else statements when testing for certain conditions. We started by dividing the code from the if else statement, so an nested if else statement occurs we would divided the code into 2 method each performs a different set of function. We further divided the code by categorized their functions so that each method can perform the same kind of function or function belongs to the same controller. By doing this, our code is successfully divided into 4 simpler method with a name that describe their functionalities.

• Original Code

 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

• Refactored Code

 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.
   team_size(teams, max_team_size)
   artifacts_num(params[:num_reviews_per_student].to_i,
                 params[:num_reviews_per_submission].to_i,
                 params[:num_calibrated_artifacts].to_i,
                 params[:num_uncalibrated_artifacts].to_i,
                 teams)
   redirect_to action: 'list_mappings', id: assignment_id
 end
 def team_size (teams, max_team_size)
   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
       return teams
     end
   end
 end
 def artifacts_num (calibrated_artifacts_num, uncalibrated_artifacts_num, student_review_num, submission_review_num, teams)
   if calibrated_artifacts_num == 0 and uncalibrated_artifacts_num == 0
     review_num student_review_num, submission_review_num, teams
   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
 end
 def review_num (student_review_num, submission_review_num, teams)
   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
 end

Examples of Refactoring

Response Report method had different switch statement testing different conditions. We split it into simpler method in different model to achieve a better cohesive in our code. We place different portions of code in models that related to their function. By just calling the corresponding method decreased the length of the review mapping controller. Updated the test after the code is completed.

 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_id = @id
   review_type = @type
   review_assignment = @assignment
   summary_ws_url = WEBSERVICE_CONFIG["summary_webservice_url"]
   current_user = session[:user]
   return SummaryHelper::Summary.response_report_by_review_and_criteria(review_assignment, summary_ws_url) if @type=="SummaryByRevieweeAndCriteria"
   return SummaryHelper::Summary.response_report_by_criteria(review_assignment, summary_ws_url) if @type=="SummaryByCriteria"
   @review_user = params[:user]
   review_user = @review_user
   return ReviewResponseMap.response_report(review_id, review_assignment, review_type, review_user) if @type== "ReviewResponseMap"
   return FeedbackResponseMap.response_report(review_assignment, review_id, review_type) if @type == "FeedbackResponseMap"
   return TeammateReviewResponseMap.response_report(review_id) if @type == "TeammateReviewResponseMap"
   return PlagiarismCheckerComparison.response_report(review_id) if @type == "PlagiarismCheckerReport"
   return TagPromptDeployment.response_report(review_id) if @type == "AnswerTaggingReport"
   if @type == "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
     @assignment = Assignment.find(params[:id])
     @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)
   end
   @user_pastebins = UserPastebin.get_current_user_pastebin current_user
 end

• Original Code

 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)
     @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
     @assignment = Assignment.find(params[:id])
     @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
   end
   @user_pastebins = UserPastebin.get_current_user_pastebin current_user
 end

for example this portion of the code summarizes the reviews of each reviewee by each rubric criterion. Which is placed in the summary class with a method name def self.response_report_by_review_and_criteria.

  when "SummaryByRevieweeAndCriteria"
     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

Result

• 39 examples, 0 failures, 39 passed
• Finished in 12.099842971 seconds
• Randomized with seed 22837
• Coverage report generated for RSpec to /home/expertiza_developer/RubymineProjects/expertiza/coverage. 1400 / 4988 LOC (28.07%) covered.