CSC/ECE 517 Fall 2024 - E2455. Refactor sign up sheet controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

This wiki page is for the information regarding the changes made for the E2409 OSS assignment for Spring 2024, 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 comprises all functions related to the management of the signup sheet for an assignment.

  • Instructors can add and remove topics from assignments, as well as alter their attributes.
  • Allows an instructor to assign and remove students to subjects.
  • Allows a student to view a list of available subjects on which they can bid for an OSS assignment.

Issues/Problem Statement

  • 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 [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 do not make sense.
  • Signup_as_instructor_action has an 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.
  • Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb [We are trying to fix how you access Code Climate, but haven’t fixed it yet.]

Design Plan

Proposed Changes

  • Update the list method. The list method is too big and can be splitted to make use of helper functions. The if and unless checks can be handled by separate functions to reduce the cognitive complexity of it.
  • Reduce the if-else ladder in signup_as_instructor_action method to reduce complexity, simplify program flow and make it readable.
  • Refactor the delete_signup and delete_signup_as_instructor to remove common functionality to private methods and simplify readability.
  • Refactor save_topic_deadlines to remove variables that are not necessary.
  • Refactor the save_topic_deadlines method by converting any instance variables that are not essential for maintaining the state of the instance into local variables..

Design Objectives

In refactoring the sign_up_sheet_controller.rb, we focused on improving the overall design of the codebase by adhering to several fundamental design principles.

  • Single Responsibility Principle (SRP):

The `SignUpSheet` controller has been refactored to improve adherence to the Single Responsibility Principle (SRP). The `list` method’s logic has been distributed to helper classes for better modularity and readability. This restructuring keeps each class focused on a single responsibility and allows for easier future modifications. The delete_signup and delete_signup_as_instructor methods have also been refactored to make sure they only handle their respective functions. All common functionality has been removed to private methods.

  • Don't Repeat Yourself (DRY):

Throughout the refactor, we emphasized reducing redundancy and promoting code reuse. For instance, in the delete_signup_as_instructor and delete_signup methods, we reduced the function length and complexity by extracting common functionality into helper functions.

  • Readability and Maintainability:

We prioritized enhancing code readability and maintainability by using clear, descriptive variable names like `assignment_id` and `topic_id`, which improve understanding at a glance. Detailed comments were added to explain each code section, clarifying the purpose of methods and conditional blocks for easier comprehension of the overall functionality. In the `signup_as_instructor_action` method, we streamlined the logic by introducing early returns, reducing nested structures, and simplifying flow. Additionally, the `list` method was improved with thorough comments, making this complex section more understandable and readable. Also by converting the unnecessary instance variable to local in save_topic_deadlines method increased the maintainability as now the scope of those variables will be confined to the methods itself.

Plan of Work

TBD based on review of Project 3

Solutions

  • Refactored signup_as_instructor_action method for if else ladder simplification Commit

signup_as_instructor_action: Now, whenever a condition fails, it immediately returns and stops further execution. There are no nested if-else blocks anymore, making the logic flow cleaner. The code processes each validation step independently, returning early for error cases. Only if all validations pass does it proceed to signing up the student.

After refactoring 

  def signup_as_instructor_action
  user = User.find_by(name: params[:username])

  # Early return if user is not found
  if user.nil?
    flash[:error] = 'That student does not exist!'
    ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Student does not exist')
    return redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end

  # Early return if user is not registered for the assignment
  unless AssignmentParticipant.exists?(user_id: user.id, parent_id: params[:assignment_id])
    flash[:error] = 'The student is not registered for the assignment!'
    ExpertizaLogger.info LoggerMessage.new(controller_name, '', "Student is not registered for the assignment: #{user.id}")
    return redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end

  # Check if signup is successful or not
  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]}")
  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

  redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
end
  • Refactored delete_signup and delete_signup_as_instructor Commit

We have introduced three private methods to implement common functionality between the methods delete_signup and delete_signup_as_instructor. These methods support delete_signup and delete_signup_as_instructor. 

   *fetch_participant_and_assignment: to fetch participant, assignment, and drop topic deadline
   *fetch_participant_by_team_and_assignment: to fetch assignment, participant, and drop topic deadline by team
   *can_drop_topic?: to check if the topic can be dropped based on submission and deadline

After refactoring 

  def delete_signup

    participant, assignment, drop_topic_deadline = fetch_participant_and_assignment(params[:id])
    submission_error = 'You have already submitted your work, so you are not allowed to drop your topic.'
    deadline_error = 'You cannot drop your topic after the drop topic deadline!'
    if can_drop_topic?(participant, drop_topic_deadline, submission_error, deadline_error)
      users_team = Team.find_team_users(assignment.id, session[:user].id)
      delete_signup_for_topic(params[:topic_id], users_team[0].t_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

    team = Team.find(params[:id])
    assignment, participant, drop_topic_deadline = fetch_participant_by_team_and_assignment(team)
    submission_error = 'The student has already submitted their work, so you are not allowed to remove them.'
    deadline_error = 'You cannot drop a student after the drop topic deadline!'

    if can_drop_topic?(participant, drop_topic_deadline, submission_error, deadline_error)
      delete_signup_for_topic(params[:topic_id], team.id)
      flash[:success] = 'You have successfully dropped the student from the topic!'
      ExpertizaLogger.info 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

  private

  # Method to fetch participant, assignment, and drop topic deadline

  def fetch_participant_and_assignment(participant_id)
    participant = AssignmentParticipant.find(participant_id)
    assignment = participant.assignment
    drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
    [participant, assignment, drop_topic_deadline]
  end



  # Method to fetch assignment, participant, and drop topic deadline by team

  def fetch_participant_by_team_and_assignment(team)
    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)
    [assignment, participant, drop_topic_deadline]
  end



  # Method to check if the topic can be dropped based on submission and deadline

  def can_drop_topic?(participant, drop_topic_deadline, submission_error, deadline_error)
    if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
      flash[:error] = submission_error
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted work: ' + params[:topic_id].to_s)
      false
    elsif !drop_topic_deadline.nil? && (Time.now > drop_topic_deadline.due_at)
      flash[:error] = deadline_error
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
      false
    else
      true
   end
  end

end
  • Refactored the List method Commit

To address the issue of the `list` method being overly long and sparsely commented, we added clear, descriptive comments to improve readability and understanding. Additionally, we split the method into smaller, focused helper methods, moving code to more appropriate locations. This restructuring made the `list` method more concise, easier to maintain, and enhanced its overall clarity.

After refactoring 

   def list
    #fetch the participant and related assignment
    @participant = AssignmentParticipant.find(params[:id].to_i)
    assignment_details = fetch_assignment_details(@participant)
    @assignment = assignment_details[:assignment]

    #retrieve slot information
    @slots_filled = assignment_details[:slots_filled]
    @slots_waitlisted = assignment_details[:slots_waitlisted]


    @show_actions = true
    @priority = 0
    @sign_up_topics = assignment_details[:sign_up_topics]
    @max_team_size = assignment_details[:max_team_size]
    team_id = @participant.team.try(:id)
    @use_bookmark = assignment_details[:use_bookmark]

    #If the assignment is intelligent, get topics based on biding
    if @assignment.is_intelligent
      @bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
      #Collect all sign up topics based on bids
      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
      #Filter and update signup topic list
      signed_up_topics &= @sign_up_topics
      @sign_up_topics -= signed_up_topics
      @bids = signed_up_topics
    end

    #Calculating the size of sign up topic list
    @num_of_topics = @sign_up_topics.size

    #Storing deadline information
    deadlines = fetch_deadlines(@assignment)
    @signup_topic_deadline = deadlines[:signup_topic_deadline]
    @drop_topic_deadline = deadlines[:drop_topic_deadline]

    @student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

    #Handle topic sign up restrictions based on dealines
    @show_actions = false if set_action_display_status(@assignment)

    @selected_topics = user_sign_up_status(@assignment, session[:user].id)
    
    render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
    
  end

Added Helper Methods 

def fetch_assignment_details(participant)
    @assignment = participant.assignment

      assignment: @assignment,
      slots_filled: SignUpTopic.find_slots_filled(@assignment.id),
      slots_waitlisted: SignUpTopic.find_slots_waitlisted(@assignment.id),
      sign_up_topics: SignUpTopic.where(assignment_id: @assignment.id, private_to: nil),
      max_team_size: @assignment.max_team_size,
      use_bookmark: @assignment.use_bookmark
    }
  end

  def fetch_deadlines(assignment)
    {
      signup_topic_deadline: assignment.due_dates.find_by(deadline_type_id: 7),
      drop_topic_deadline: assignment.due_dates.find_by(deadline_type_id: 6)
    }
  end

  def set_action_display_status(assignment)
          # 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
    signup_deadline = assignment.due_dates.find_by(deadline_type_id: 1)
    return true if signup_deadline.nil?
    !assignment.staggered_deadline? && (signup_deadline.due_at < Time.now)
  end

  def user_sign_up_status(assignment, user_id)
    users_team = Team.find_team_users(assignment.id, user_id)
    if users_team.empty?
      nil
    else
      SignedUpTeam.find_user_signup_topics(assignment.id, users_team.first.t_id)
    end
  end

end


  • Refactoring the save_topic_deadlines method Commit

In our refactor of the save_topic_deadlines method we identified several variables that had been previously declared as instance variables but were not necessary for maintaining the instance's state. These variables were subsequently converted into local variables, thereby enhancing the clarity of the method and effectively encapsulating its operational context. Furthermore, we have included detailed comments throughout the code. These annotations serve to elucidate the purpose of the method, outline its logic, and document the specific changes made during the refactoring process.

After refactoring 

def save_topic_deadlines
   assignment = Assignment.find(params[:assignment_id])
   assignment_submission_due_dates = assignment.due_dates.select { |due_date| due_date.deadline_type_id == 1 }
   assignment_review_due_dates = assignment.due_dates.select { |due_date| due_date.deadline_type_id == 2 }
   due_dates = params[:due_date]
   topics = SignUpTopic.where(assignment_id: params[:assignment_id])
   review_rounds = assignment.num_review_rounds
   
   topics.each_with_index do |topic, index|
     (1..review_rounds).each do |i|
       topic_submission_due_date = due_dates["#{topic.id}_submission_#{i}_due_date"]
       topic_review_due_date = due_dates["#{topic.id}_review_#{i}_due_date"]
       assignment_submission_due_date = assignment_submission_due_dates[i - 1].due_at.strftime('%Y-%m-%d %H:%M')
       assignment_review_due_date = assignment_review_due_dates[i - 1].due_at.strftime('%Y-%m-%d %H:%M')
 
       %w[submission review].each do |deadline_type|
         deadline_type_id = DeadlineType.find_by_name(deadline_type).id
         next if topic_submission_due_date == assignment_submission_due_date && deadline_type == 'submission'
         next if topic_review_due_date == assignment_review_due_date && deadline_type == 'review'
 
         topic_due_date = TopicDueDate.where(parent_id: topic.id, deadline_type_id: deadline_type_id, round: i).first
 
         if topic_due_date.nil? # create a new record
           TopicDueDate.create(
             due_at: deadline_type == 'submission' ? topic_submission_due_date : topic_review_due_date,
             deadline_type_id: deadline_type_id,
             parent_id: topic.id,
             submission_allowed_id: assignment_submission_due_dates[i - 1].submission_allowed_id,
             review_allowed_id: assignment_review_due_dates[i - 1].review_allowed_id,
             review_of_review_allowed_id: assignment_review_due_dates[i - 1].review_of_review_allowed_id,
             round: i,
             flag: assignment_review_due_dates[i - 1].flag,
             threshold: assignment_review_due_dates[i - 1].threshold,
             delayed_job_id: assignment_review_due_dates[i - 1].delayed_job_id,
             deadline_name: assignment_review_due_dates[i - 1].deadline_name,
             description_url: assignment_review_due_dates[i - 1].description_url,
             quiz_allowed_id: assignment_review_due_dates[i - 1].quiz_allowed_id,
             teammate_review_allowed_id: assignment_review_due_dates[i - 1].teammate_review_allowed_id,
             type: 'TopicDueDate'
           )
         else # update an existing record
           topic_due_date.update_attributes(
             due_at: deadline_type == 'submission' ? topic_submission_due_date : topic_review_due_date,
             submission_allowed_id: assignment_submission_due_dates[i - 1].submission_allowed_id,
             review_allowed_id: assignment_review_due_dates[i - 1].review_allowed_id,
             review_of_review_allowed_id: assignment_review_due_dates[i - 1].review_of_review_allowed_id,
             quiz_allowed_id: assignment_review_due_dates[i - 1].quiz_allowed_id,
             teammate_review_allowed_id: assignment_review_due_dates[i - 1].teammate_review_allowed_id
           )
         end
       end
     end
   end
 
   redirect_to_assignment_edit(params[:assignment_id])
end

Test Cases

  • When dropping topic if submission already done.
  • When deadline from dropping topic has passed.
  • Deleting topic when topic cannot be found.
  • Signup in case when user cannot be found.
  • Create signup_sheet when sign_up_topic cannot be found.
  • Destroy signup_sheet when other topics with the same assignment exists.

Manual Testing

Follow these instructions to manually test the below functionalities:

  • The instructor assigns a student to a topic:
  1. Log in as an instructor using the credentials: username - instructor6, password - password.
  2. Select Manage -> Assignments, go to Etc, there add the student to participant.
  3. Then select a topic which you want to assign to that student.
  • Set/updates deadlines to that topic
  1. To set/update deadlines, select Manage -> Assignments
  2. Enable the staggered deadline checkbox
  3. Go to Topics, then select start show/ due date
  4. There you can assign the deadline, and click on save
  • Drop/Delete student from the topic
  1. Now go to Manage-> Assignments ->Topics
  2. We can see the assigned users to the topic
  3. Click on the ‘drop student’, then that student would be dropped from the topic.

Rspec

The RSpec test cases for 'signup_sheet_controller.rb' are intended to fully examine the functionality of handling sign-up topics for assignments. These test cases cover a variety of scenarios, such as adding new topics, modifying topic properties, deleting topics, switching from original to suggested subjects, and more. They ensure that the controller correctly manages activities taken by students and teachers, including meeting deadlines and maintaining submission status. Furthermore, the test cases confirm that activities such as topic priority setting and topic deadline saving function properly.

All of these tests are designed to ensure that the controller's actions respond effectively to user input and that the underlying logic, database interactions, and authorization checks work as expected. With these tests, developers can be certain that the 'SignUpSheetController' is functional and provides a solid basis for managing sign-up topics in an educational setting.

Modified Files

  • sign_up_sheet_controller.rb
  • sign_up_sheet_helper.rb

Relevant Links

Team

Mentor:Devansh Shah - dshah8@ncsu.edu

  • Bhushan Patil - bpatil@ncsu.edu
  • Mragisha Jain - mjain22@ncsu.edu
  • Ashwin Satpute - aasatput@ncsu.edu
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2024_-_E2455._Refactor_sign_up_sheet_controller.rb&oldid=158271"