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

E2471. E2455. Refactor sign up sheet controller.rb

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.

Final Design Document

Current Status

We have successfully completed the following refactoring and functionality improvements:

1. Update Method: Reviewed and removed unnecessary instance variables in the update method, making it more efficient and streamlined. 2. Signup Methods: Refined the `signup_as_instructor` and `signup_as_instructor_action` methods by investigating their distinct roles, renaming them for consistency, and adding comprehensive comments to improve readability. 3. List Method: Simplified and documented the `list` method, breaking down the code into smaller helper functions where possible to enhance readability and maintainability. 4. Save Topic Deadline Method: Removed unnecessary variables and converted instance variables that were not required to be instance variables into local variables and tested the functionality thoroughly.

These changes have been implemented and tested to ensure they meet the requirements for clarity, functionality, and performance.

Issues and Plan of Action

1. Consistent Naming Conventions

  Current Issue: Some methods and variables use inconsistent naming conventions for "sign up" and "signup," leading to potential confusion in understanding their roles.  
  Plan: We will review the codebase to ensure that "sign up" is consistently used as a verb, while "signup" is used as a noun or adjective. This will include changes to calling methods and any documentation impacted by the renaming.

2. Method Consolidation for Signup Topics

  Current Issue: The `add_signup_topics_staggered` method duplicates much of the functionality found in `add_signup_topics`, despite being intended for staggered deadlines (e.g., Assignment 1042).  
  Plan: We will consolidate these functions by ensuring that staggered deadlines are handled within `add_signup_topics_staggered` as a distinct feature, streamlining the code and reducing redundancy.

3. Improving Method Names

  Current Issue: Several methods, including `load_add_signup_topics`, `ad_info`, and `list`, could benefit from more descriptive names to clarify their purpose and enhance code readability.  
  Plan: We will rename these methods to better reflect their functions, making the code more intuitive for future developers and maintainers.

4. Clarifying the `@participants` Variable Usage

  Current Issue: The `@participants` variable in `load_add_signup_topics` is used ambiguously. Initially, this variable represented individual students signing up for topics, but now only teams can sign up, making the `@participants` terminology obsolete.  
  Plan: We will refactor the code to replace `@participants` with a variable name that more accurately reflects its current role. Additionally, we’ll add comments to clarify its function across different views.

5. Improving the Signup Action Method’s If-Else Ladder

  Current Issue: The `signup_as_instructor_action` method contains an if-else ladder that could be simplified for better readability and maintainability.  
  Plan: We will refactor the if-else ladder using more elegant conditional logic or helper methods, aiming for a cleaner, more modular approach.

6. DRY Principle for Delete Methods

  Current Issue: The `delete_signup` and `delete_signup_as_instructor` methods have significant overlap, violating the DRY (Don’t Repeat Yourself) principle.  
  Plan: We will extract shared functionality from these methods into a common helper function, making the code more modular and easier to maintain.

7. Code Climate Issues and Method Relocation

  Current Issue: Some methods could be moved to `sign_up_sheet_helper.rb` to reduce the size of the main file and improve code organization.  
  Plan: We will review Code Climate issues and move appropriate methods to `sign_up_sheet_helper.rb` as a step towards better code organization and file structure.

By addressing these remaining issues, we will work toward a cleaner, more efficient, and well-documented codebase that adheres to best practices.

Motivations

1. Single Responsibility Principle (SRP)

   Consistent Naming Conventions
       Ensures each method and variable has a clear, singular responsibility, reducing ambiguity.

   Improving Method Names
       Provides clarity by giving each method a name that directly reflects its purpose.

   Clarifying the @participants Variable Usage
       Refactors ambiguous terminology, giving each variable a clear role and responsibility.

   DRY Principle for Delete Methods
       Consolidates shared functionality into a helper function, allowing each method to perform a single, specific task.

   Code Climate Issues and Method Relocation
       Moves methods to helper files, promoting separation of concerns and keeping each file focused on specific tasks.

2. Open/Closed Principle (OCP)

   Improving the Signup Action Method’s If-Else Ladder
       Refactors the if-else ladder to facilitate easier extension of functionality without modifying existing logic, promoting modularity.

3. Interface Segregation Principle (ISP)

   Method Consolidation for Signup Topics
       Ensures each method is specialized for a distinct purpose, preventing overlapping or unnecessary functionality.

4. Dependency Inversion Principle (DIP)

   Code Climate Issues and Method Relocation
       Organizes the codebase into helper files, creating a modular structure where the main code depends on specialized, independent helper classes or files.

Modified Files

• sign_up_sheet_controller.rb
• sign_up_sheet_helper.rb

Relevant Links

• Github Repository: https://github.com/DarthAsh/expertiza
• Pull Request: https://github.com/expertiza/expertiza/pull/2868
• Video Link: https://drive.google.com/file/d/1MTfX_cHa6zy34WQRNgXOybQXDya4J1R2/view?usp=drive_link

Team

Mentor:Devansh Shah - dshah8@ncsu.edu

• Bhushan Patil - bpatil@ncsu.edu
• Mragisha Jain - mjain22@ncsu.edu
• Ashwin Satpute - aasatput@ncsu.edu