E1908 signupsheet

E1908. Refactoring the Sign-up sheet Controller

This page provides a description of the Expertiza based OSS project.

About Expertiza

[1] is an open source project dependent on on Rails structure. Expertiza enables the teacher to make new assignments and alter new or existing assignments. It additionally enables the educator to make a rundown of subjects the students can agree to accept. Students can shape groups in Expertiza to chip away at different undertakings and projects. Students can likewise peer audit other students' entries. Expertiza underpins accommodation crosswise over different record types, including the URLs and wiki pages.

Problem Statement

The following tasks were accomplished in this project:

• Improved the clarity of code by improving the variable and parameter names.
• Followed naming conventions throughout and renamed methods with inconsistent names including the calling methods.
• Rectified several unwanted if-else conditions in methods and optimized the code.
• Refactored all instance variables and removed unnecessarily defined variables.
• Removed certain unwanted flash messages that occur for some user actions.
• Included comments for functionalities throughout for better understanding.

About Sign-up sheet Controller

Sign-up sheet controller contains all functions related to management of the signup sheet for an assignment function to add new topics to an assignment, edit properties of a particular topic, delete a topic, etc are included here.

Drawbacks and Solutions

• Problem 1: Create method has an if-else condition determining if create or update should be called. Create method should not be responsible for calling update. Identify why the if-else condition exists. The if-else condition exists because the current implementation calls update if a signup sheet with the same name already exists.
• Solution: Rectified this method by removing the call to update and flashing an error instead.

 if topic.nil?
      setup_new_topic
    else
      update_existing_topic topic
      flash[:error] = "The topic already exists."
    end
  end

• Problem 2: Update method has a plethora of instance variables defined before updating. These are not necessary (For e.g., look at update method of bookmarks_controller).
• Solution: Refactored the variables not needed out.

 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
      # Replaced unnecessary variables and save with a single update call for all the parameters

• Problem 3: Destroy has a misleading else flash message.
• Solution: Refactored the mislleading flash messages not needed out.

   @topic.destroy
      undo_link("The topic: \"#{@topic.topic_name}\" has been successfully deleted. ")
    else
      flash[:error] = "The topic could not be deleted."
      flash[:error] = "This topic could not be found." # error message mage more specific
    end
    # changing the redirection url to topics tab in edit assignment view.
    redirect_to edit_assignment_path(params[:assignment_id]) + "#tabs-5"

• Problem 4: Add_signup_topics_staggered does not do anything.
• Solution: Renamed participants variable to 'teams'.

 #add_signup_topics_staggered calls add_signup_topics and does nothing else.
 def add_signup_topics_staggered
    add_signup_topics 
  end

• Problem 5: Several method names are renamed to be more intuitive.
• Solution: load_add_signup_topics is renamed to get_assignment_data and ad_info is renamed to get_ad.

  # Contains links that let an admin or Instructor edit, delete, view enrolled/waitlisted members for each topic
  # Also contains links to delete topics and modify the deadlines for individual topics. Staggered means that different topics can have different deadlines.
  def add_signup_topics
    load_add_signup_topics(params[:id])
    get_assignment_data(params[:id])
    SignUpSheet.add_signup_topic(params[:id])
  end

  @@ -109,7 +109,7 @@ def add_signup_topics_staggered
  end

  # retrieves all the data associated with the given assignment. Includes all topics,
  def load_add_signup_topics(assignment_id)
  def get_assignment_data(assignment_id)
    @id = assignment_id
    @sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
    @slots_filled = SignUpTopic.find_slots_filled(assignment_id)
  @@ -374,7 +374,7 @@ def save_topic_deadlines
  # This method is called when a student click on the trumpet icon. So this is a bad method name. --Yang
  def show_team
    if !(assignment = Assignment.find(params[:assignment_id])).nil? and !(topic = SignUpTopic.find(params[:id])).nil?
      @results = ad_info(assignment.id, topic.id)
      @results = get_ad(assignment.id, topic.id)
      @results.each do |result|
        result.keys.each do |key|
          @current_team_name = result[key] if key.equal? :name

• Problem 6: The list method is too long and is sparsely commented.
• Solution: Added comments.

  # function to list all topics and bids to a participant
  def list
    @participant = AssignmentParticipant.find(params[:id].to_i)
    @assignment = @participant.assignment
    @@ -155,6 +156,8 @@ def list
    @max_team_size = @assignment.max_team_size
    team_id = @participant.team.try(:id)

    # If the assignment supports bidding, add all the bids of an 
    # individual or team to the list of signed topics
    if @assignment.is_intelligent
      @bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
      signed_up_topics = []

• Problem 7: What are the 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.
• Solution: Removed metod which is not required

 end

  # routes to new page to specficy student
  # Its just to specify the student, its a get request. Other than that it doesnt do anything. 
  def signup_as_instructor; end

  # its an action that usually follows the aove action, its a post request to signup the student.
  def signup_as_instructor_action
    user = User.find_by(name: params[:username])
    if user.nil? # validate invalid user

• Problem 8: Participants variable in load_add_signup_topics actually means teams that signed up for a topic.
• Solution: Renamed participants variable to 'teams'.

    # to treat all assignments as team assignments
    # Though called participants, @participants are actually records in signed_up_teams table, which
    # is a mapping table between teams and topics (waitlisted recored are also counted)
    @participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
    @teams = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
  end

• Problem 9: Signup_as_instructor_action has if-else ladder.
• Solution: It has been made more elegant using a helper function.

 def signup_as_instructor; end

  def signup_student user
    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
  end

  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!"
    end
    if !user.nil? and AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
      signup_student(user);
    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
      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
    redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end

  # Checks if participant has permission to delete a topic, reports errors otherwise
  def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
  def can_delete_topic? is_instructor, participant, assignment, drop_topic_deadline
    submission_error_message = ""
    deadline_error_message = ""

   @@ -237,7 +239,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
    else
      submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
      deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

    end
    if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
      flash[:error] = submission_error_message
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
   @@ -246,7 +248,7 @@ def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadli
      flash[:error] = deadline_error_message
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
      return false

    end
    return true
  end

   @@ -274,7 +276,6 @@ def delete_signup_as_instructor
    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 can_delete_topic?(true, participant, assignment, drop_topic_deadline)
      delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
      flash[:success] = "You have successfully dropped the student from the topic!"
   @@ -481,4 +482,4 @@ def ad_info(_assignment_id, topic_id)
  def delete_signup_for_topic(assignment_id, topic_id, user_id)
    SignUpTopic.reassign_topic(user_id, assignment_id, topic_id)
  end
end
end 

• Problem 10: Delete_signup and delete_signup_as_instructor have much in common and violates the DRY principle.
• Solution: Refactored them by moving the duplicate code to a helper function.

 def can_delete_topic?(is_instructor?, participant, assignment, drop_topic_deadline)
    submission_error_message = ""
    deadline_error_message = ""

    if is_instructor?
      submission_error_message = "The student has already submitted their work, so you are not allowed to remove them"
      deadline_error_message = "You cannot drop a student after the drop topic deadline!"
    else
      submission_error_message = "You have already submitted your work, so you are not allowed to drop your topic."
      deadline_error_message = "You cannot drop your topic after the drop topic deadline!"

    if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
      flash[:error] = submission_error_message
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
      return false
    elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
      flash[:error] = deadline_error_message
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
      return false

    return true
  end

  # this function is used to delete a previous signup
  def delete_signup
    participant = AssignmentParticipant.find(params[:id])
@@ -235,13 +259,7 @@ def delete_signup
    # (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? or !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? and 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
    if can_delete_topic?(false, participant, assignment, drop_topic_deadline)
      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)
@@ -256,13 +274,8 @@ def delete_signup_as_instructor
    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? or !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? and 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

    if can_delete_topic?(true, participant, assignment, drop_topic_deadline)
      delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
      flash[:success] = "You have successfully dropped the student from the topic!"

References

1. https://github.com/expertiza/expertiza
2. http://expertiza.ncsu.edu/ The live Expertiza website