CSC/ECE 517 Fall 2023 - E2357. Refactor sign up sheet controller.rb

This wiki page is for the information regarding the changes made for the E2257 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 student 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 sign up sheet controller contains all functions related to the management of the signup sheet for an assignment:

• Allows an instructor to add/remove topics to an assignment.
• Allows an instructor to edit properties of topics.
• Allows an instructor to assign/remove students to topics
• Allows a student to see the list of available topics which they can bid on for a given OSS assignment.

Issues

• Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
• Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out. [This may have already been fixed, but save_topic_deadline also has these problems.]
• Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
• Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
• What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
• The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
• Refactor participants variable in load_add_signup_topics

1. [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.

• Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
• Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.

Previous Version of Project

1. CSC/ECE_517_Spring_2022_-_E2223._Refactor_sign_up_sheet_controller.rb
2. https://github.com/palvitgarg99/expertiza
3. https://github.com/expertiza/expertiza/pull/2345

Solutions

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

• 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

• Improved Method names as follows:

• 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

• The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods. This issue is fixed by refactoring the List Method using helper methods. Here the 'list' method is doing a lot of different things, making it quite lengthy and potentially harder to maintain. So by refactoring and restructuring we can improve readability and maintainability.

Before Refactoring

 def list 
    @participant = AssignmentParticipant.find(params[:id].to_i)
    @assignment = @participant.assignment
    @slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
    @slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
    @show_actions = true
    @priority = 0
    @sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
    @max_team_size = @assignment.max_team_size
    team_id = @participant.team.try(:id)
    @use_bookmark = @assignment.use_bookmark
    if @assignment.is_intelligent
      @bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
      signed_up_topics = []
      @bids.each do |bid|
        sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
        signed_up_topics << sign_up_topic if sign_up_topic
      end
      signed_up_topics &= @sign_up_topics
      @sign_up_topics -= signed_up_topics
      @bids = signed_up_topics
    end
    @num_of_topics = @sign_up_topics.size
    @signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
    @drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
    @student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
    unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
      @show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)
      # Find whether the user has signed up for any topics; if so the user won't be able to
      # sign up again unless the former was a waitlisted topic
      # if team assignment, then team id needs to be passed as parameter else the user's id
      users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
      @selected_topics = if users_team.empty?
                           nil
                         else
                           SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
                         end
    end
    render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
  end

After Refactoring

#Controller method
def display_topics_and_signup_info
  find_participant_and_assignment
  load_signup_topic_details
  process_intelligent_topics if @assignment.is_intelligent
  load_deadlines
  check_user_signup_topics
  render_intelligent_topic_selection if @assignment.is_intelligent
 end
 private
 # Helper method: Find participant and assignment
 def find_participant_and_assignment
  @participant = find_participant
  @assignment = @participant.assignment
 end
 # Helper method: Find participant based on the given ID
 def find_participant
  AssignmentParticipant.find(params[:id].to_i)
 end
 # Helper method: Find participant based on the given ID
 def find_participant
  AssignmentParticipant.find(params[:id].to_i)
 end
 # Helper method: Load sign-up topic details
 def load_signup_topic_details
  @slots_filled = SignUpTopic.slots_filled_for(@assignment.id)
  @slots_waitlisted = SignUpTopic.slots_waitlisted_for(@assignment.id)
  @signup_topics = SignUpTopic.public_topics_for_assignment(@assignment.id)
  @max_team_size = @assignment.max_team_size
  @use_bookmark = @assignment.use_bookmark
 end
 # Helper method: Process intelligent topics
 def process_intelligent_topics
  @bids, signed_up_topics = retrieve_bids_and_signed_topics
  @signup_topics -= signed_up_topics
  @bids = signed_up_topics
  @num_of_topics = @signup_topics.size
 end
# Helper method: Retrieve bids and signed topics
def retrieve_bids_and_signed_topics
 team_id = @participant.team.try(:id)
 bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
 signed_up_topics = bids.each_with_object([]) do |bid, topics|
   topic = SignUpTopic.find_by(id: bid.topic_id)
   topics << topic if topic
 end
  [bids, signed_up_topics & @signup_topics]
 end
 # Helper method: Load deadlines
 def load_deadlines
  @signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
  @drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
  @student_bids = @participant.team.nil? ? [] : Bid.where(team_id: @participant.team.id)
 end
 # Helper method: Check user signup topics
 def check_user_signup_topics
  return unless (due_date = @assignment.due_dates.find_by(deadline_type_id: 1))
  @show_actions = !@assignment.staggered_deadline? && (due_date.due_at < Time.now)
  users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
  @selected_topics = users_team.empty? ? nil : SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
 end
# Helper method: Render intelligent topic selection
def render_intelligent_topic_selection
 render('sign_up_sheet/intelligent_topic_selection') && return
end

• Signup_as_instructor_action has if-else ladder. Made this function more elegant. The `assignment_id` and `topic_id` are assigned to variables at the beginning of the method. This makes the code more concise and improves clarity. The refactored code reduces nesting by breaking down the conditions into separate if statements. This avoids deep nesting and makes the code more linear.

Before refactoring 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

After refactoring Signup_as_instructor_action

 # Signs up a student for a specific topic in an assignment by an instructor
 def signup_as_instructor_action
   user = User.find_by(name: params[:username])
 # validate invalid user
   if user.nil?
     flash[:error] = 'That student does not exist!'
   else
     #assign params[:assignment_id] and params[:topic_id] to variables and use these variables to avoid repetition 
     assignment_id = params[:assignment_id]
     topic_id = params[:topic_id]
     if AssignmentParticipant.exists?(user_id: user.id, parent_id: assignment_id)
       if SignUpSheet.signup_team(assignment_id, user.id, 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: #{topic_id}"))
       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: assignment_id)
 end
 

• 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_common(participant, assignment, drop_topic_deadline, topic_id)
 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 work: #{topic_id}")
 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: #{topic_id}")
 else
   delete_signup_for_topic(assignment.id, topic_id, participant.user_id)
   flash[:success] = 'You have successfully dropped your topic!'
   ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, "Student has dropped the topic: #{topic_id}")
 end

end

def delete_signup

 # delete_signup method
 participant = AssignmentParticipant.find(params[:id])
 assignment = participant.assignment
 drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
 topic_id = params[:topic_id]
 delete_signup_common(participant, assignment, drop_topic_deadline, topic_id)
 redirect_to action: 'list', id: params[:id]

end

def delete_signup_as_instructor

 # delete_signup_as_instructor method
 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)
 topic_id = params[:topic_id]
 delete_signup_common(participant, assignment, drop_topic_deadline, topic_id)
 redirect_to controller: 'assignments', action: 'edit', id: assignment.id

end

Modified Files

• sign_up_sheet_controller.rb

Tests

 describe '#update' do
   context 'when topic cannot be found' do
     it 'shows an error flash message and redirects to assignment#edit page' do
       allow(SignUpTopic).to receive(:find).with('1').and_return(nil)
       request_params = { id: 1, assignment_id: 1 }
       post :update, params: request_params
       expect(flash[:error]).to eq('The topic could not be updated.')
       expect(response).to redirect_to('/assignments/1/edit#tabs-2')
     end
   end
 describe '#delete_signup_as_instructor' do
   before(:each) do
     allow(Team).to receive(:find).with('1').and_return(team)
     allow(TeamsUser).to receive(:find_by).with(team_id: 1).and_return(double('TeamsUser', user: student))
     allow(AssignmentParticipant).to receive(:find_by).with(user_id: 8, parent_id: 1).and_return(participant)
     allow(participant).to receive(:team).and_return(team)
   end

context 'when either submitted files or hyperlinks of current team are not empty' do

     it 'shows a flash error message and redirects to assignment#edit page' do
       allow(assignment).to receive(:instructor).and_return(instructor)
       request_params = { id: 1 }
       user_session = { user: instructor }
       get :delete_signup_as_instructor, params: request_params, session: user_session
       expect(flash[:error]).to eq('The student has already submitted their work, so you are not allowed to remove them.')
       expect(response).to redirect_to('/assignments/1/edit')
     end
   end

   context 'when both submitted files and hyperlinks of current team are empty and drop topic deadline is not nil and its due date has already passed' do
     it 'shows a flash error message and redirects to assignment#edit page' do
       due_date.due_at = DateTime.now.in_time_zone - 1.day
       allow(assignment).to receive(:due_dates).and_return(due_date)
       allow(due_date).to receive(:find_by).with(deadline_type_id: 6).and_return(due_date)
       allow(team).to receive(:submitted_files).and_return([])
       allow(team).to receive(:hyperlinks).and_return([])
       request_params = { 
         id: 1,
         due_date: {
           '1_submission_1_due_date' => nil,
           '1_review_1_due_date' => nil
         }
       }
       user_session = { user: instructor }
       get :delete_signup_as_instructor, params: request_params, session: user_session
       expect(flash[:error]).to eq('You cannot drop a student after the drop topic deadline!')
       expect(response).to redirect_to('/assignments/1/edit')
     end
   end

   context 'when both submitted files and hyperlinks of current team are empty and drop topic deadline is nil' do
     it 'shows a flash success message and redirects to assignment#edit page' do
       allow(team).to receive(:submitted_files).and_return([])
       allow(team).to receive(:hyperlinks).and_return([])
       allow(SignedUpTeam).to receive(:find_team_users).with(1, 6).and_return([team])
       allow(team).to receive(:t_id).and_return(1)
       request_params = { id: 1, topic_id: 1 }
       user_session = { user: instructor }
       get :delete_signup_as_instructor, params: request_params, session: user_session
       expect(flash[:success]).to eq('You have successfully dropped the student from the topic!')
       expect(response).to redirect_to('/assignments/1/edit')
     end
   end
 end

Team

Mentor

• Ameya Vaichalikar (agvaicha)

Team Members

• Sravya Karanam (skarana)
• Sucharita Nadendla (snadend3)
• Abhishek Desai (adesai7)

Pull Request

• https://github.com/expertiza/expertiza/pull/2684
• https://github.com/expertiza/expertiza/pull/2678
• https://github.com/expertiza/expertiza/pull/2677
• https://github.com/expertiza/expertiza/pull/2669