CSC/ECE 517 Fall 2023 - E2356. Refactor review mapping controller.rb

This wiki page is for information regarding the changes made for the E2356 Refactor review_mapping_controller.rb OSS assignment for Fall 2023, CSC/ECE 517.

Introduction

Expertiza is an open-source project developed on Ruby on Rails. This web application is maintained by the students and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well-developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

About Controller

The functionality of review_mapping_controller is to provide mapping for the reviewer and assignment. Basically, the controller handles the assignment of reviews to different teams or single student users, such as the event of peer review and self-review. Also, this controller is responsible to respond student user requests for extra bonus reviews based on assignment policy.

Problem Statement

• Refactor the long methods in review_mapping_controller.rb like assign_reviewer_dynamically, add_reviewer, automatic_review_mapping,peer_review_strategy etc.
• Some variables in review_mapping_controller.rb are not named properly. Rename variable names to convey what they are used for.
• Functions are lengthy and difficult to read. Replace switch statements with subclasses methods, if any.
• Create subclasses and models wherever necessary.
• Add meaningful comments and edit/remove unnecessary comments.
• Ensure that code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

Design Pattern

We followed several design patterns while refactoring our code. One of the most common ones is the “Extract Method.” There were several instances where the method was too long and complex. We moved some of the functionality to a different method to make it more readable. This way, it became easier and clearer to understand what the method achieved. Another issue we observed with the code was that it had too many conditional statements. We used the “Refactoring conditionals” design pattern, which aims to place the block of code in the conditional statement in a different method and call that method. This improves the readability of our code.

File(s) Modified

1. review_mapping_controller.rb

Solutions/Details of Changes Made

1. Changes to variable names to make them reflect what they actually do.

i) Changed variable name from mmappings to meta_review_mappings to provide more context.

ii) Used assignment_id which is declared before, instead of hardcoding params[:id].to_i

2. Added/Edited comments to improve the readability of code and make commenting proper.

a) Adding comments

i) Explains the functionality of add_calibration method in a simple manner.

ii) Inform that an assignment can have multiple topics which need to be taken care of.

iii) Explains the functionality of automatic_review_mapping

iv) Describes an error encountered in the stable version of Expertiza, and suggests potential fix.

b) Edit Comments

i) Describes the working of assign_reviewer_dynamically in a concise manner

ii) Explains the function review_allowed? better, the previous comment was too repetitive.

iii) Precisely mentions the criteria for requesting reviews by the user.

c) Deleting Comments

Comments about previous teams progress and other redundant comments were removed to improve readability and maintainability.

3. Reducing cyclomatic complexity of functions by dividing them into smaller methods.

i)

ii)

iii)

iv)

v)

4. Reshuffling parts of code to make the method look consistent and easy to read and understand

5. Test Report

1. The naming inconsistency issue has already been fixed by the previous version of the project. It has been verified by us.

2. Refactoring the Update method

Before refactoring

 def update
   @topic = SignUpTopic.find(params[:id])
   if @topic
     @topic.topic_identifier = params[:topic][:topic_identifier]
     update_max_choosers @topic
     @topic.category = params[:topic][:category]
     @topic.topic_name = params[:topic][:topic_name]
     @topic.micropayment = params[:topic][:micropayment]
     @topic.description = params[:topic][:description]
     @topic.link = params[:topic][:link]
     @topic.save
     undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
   else
     flash[:error] = 'The topic could not be updated.'
   end
   # Akshay - correctly changing the redirection url to topics tab in edit assignment view.
   redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
 end

After refactoring

def update
   @topic = SignUpTopic.find(params[:id])
   if @topic
     update_max_choosers @topic
     @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], category: params[:topic][:category],
     topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description],link:params[:topic][:link] )
     undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
     flash[:success] = 'The topic has been updated.'
   else
     flash[:error] = 'The topic could not be updated.'
   end
   # Akshay - correctly changing the redirection url to topics tab in edit assignment view.
   redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
 end

3.

4.

5. signup_as_instructor_action is used to sign up a student for a particular topic for an assignment as an instructor. signup_as_instructor function does nothing. It is an empty function. The function is not being called anywhere in the code. The method is not needed and hence removed.

signup_as_instructor_action

 def signup_as_instructor_action
   user = User.find_by(name: params[:username])
   if user.nil? # validate invalid user
     flash[:error] = 'That student does not exist!'
   else
     if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
       if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
         flash[:success] = 'You have successfully signed up the student for the topic!'
         ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
       else
         flash[:error] = 'The student has already signed up for a topic!'
         ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Instructor is signing up a student who already has a topic')
       end
     else
       flash[:error] = 'The student is not registered for the assignment!'
       ExpertizaLogger.info LoggerMessage.new(controller_name, , 'The student is not registered for the assignment: ' << user.id)
     end
   end
   redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
 end

signup_as_instructor function

def signup_as_instructor; end

6.

7.

8.

9. Delete_signup and delete_signup_as_instructor violate the DRY principle. Refactored this. Created a new private function handle_signup_deletion to handle deletion for both instructor as well as student. A new function, check_signup_eligibility, generates the error message that the topic cannot be dropped. The redirect_path function is used to redirect the user.

Before Refactoring Delete_signup and delete_signup_as_instructor

def delete_signup
   participant = AssignmentParticipant.find(params[:id])
   assignment = participant.assignment
   drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
   # A student who has already submitted work should not be allowed to drop his/her topic!
   # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
   # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
   # If there is a drop topic deadline, student cannot drop topic after this deadline.
   if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
     flash[:error] = 'You have already submitted your work, so you are not allowed to drop your topic.'
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
   elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
     flash[:error] = 'You cannot drop your topic after the drop topic deadline!'
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
   else
     delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
     flash[:success] = 'You have successfully dropped your topic!'
     ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
   end
   redirect_to action: 'list', id: params[:id]
 end

 def delete_signup_as_instructor
   # find participant using assignment using team and topic ids
   team = Team.find(params[:id])
   assignment = Assignment.find(team.parent_id)
   user = TeamsUser.find_by(team_id: team.id).user
   participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
   drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
   if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
     flash[:error] = 'The student has already submitted their work, so you are not allowed to remove them.'
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
   elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
     flash[:error] = 'You cannot drop a student after the drop topic deadline!'
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
   else
     delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
     flash[:success] = 'You have successfully dropped the student from the topic!'
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
   end
   redirect_to controller: 'assignments', action: 'edit', id: assignment.id
 end

After Refactoring Delete_signup and delete_signup_as_instructor

 def delete_signup
   handle_signup_deletion(params[:id], session[:user].id)
 end

 def delete_signup_as_instructor
   team = Team.find(params[:id])
   user = TeamsUser.find_by(team_id: team.id).user
   assignment = Assignment.find(team.parent_id)
   handle_signup_deletion(user.id, session[:user].id, assignment.id)
 end

 private def handle_signup_deletion(participant_id, user_id, assignment_id = nil)
   participant = AssignmentParticipant.find(participant_id)
   assignment ||= participant.assignment
   drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
   error_message = check_signup_eligibility(participant.team, drop_topic_deadline, user_id, params[:topic_id])
   if error_message
     flash[:error] = error_message
     ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, "Drop failed for #{error_message}: #{params[:topic_id]}")
   else
     delete_signup_for_topic(assignment.id, params[:topic_id], user_id)
     flash[:success] = 'You have successfully dropped the topic!'
     ExpertizaLogger.info LoggerMessage.new(controller_name, user_id, "Student has dropped the topic: #{params[:topic_id]}")
   end
   redirect_to redirect_path(participant_id, assignment_id)
 end
 
 def check_signup_eligibility(team, drop_topic_deadline, user_id, topic_id)
   if !team.submitted_files.empty? || !team.hyperlinks.empty?
     'You have already submitted your work, so you are not allowed to drop your topic.'
   elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
     'You cannot drop your topic after the drop topic deadline!'
   end
 end

 def redirect_path(participant_id, assignment_id)
   if assignment_id
     controller: 'assignments', action: 'edit', id: assignment_id
   else
     action: 'list', id: participant_id
   end
 end

10.

Team

Mentor

• Ameya Vaichalikar (agvaicha)

Team Members

• Chirag Bheemaiah Palanganda Karumbaiah (cpalang)
• Shanmukh Pawan Moparthi (smopart2)
• Amit Bhujbal (abhujba)