E2409 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 delete_signup_common to improve the readability of the code and solve the SRP issue pointed out in Program 3 review. Reuse the log_message() function created in part-3 to log application messages and enure DRY principle is followed
  • Rename methods and variables - delete_signup(delete_signup_as_student), load_add_signup_topics(function does not add anything as such hence shall be renamed)
  • Reduce the if-else ladder in switch_original_topic_to_approved_suggested_topic method into separate functions to improve readability, testability and understandability of the code.
  • Implement Builder method for SignUpTopic and TopicDueDate objects to reduce multiple assignment operations
  • Fix code climate issues
  • Update test specs to ensure the test build passes.

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):

Responsibility for Object creation for SignUpTopic and TopicDueDate can be delegated to Builder class. We will be addressing the issue of the delete_signup_common method not adhering to SRP. In order to solve the issue we will be creating a class DeleteSignupAction which will have methods like raise_flash_if_work_submitted, raise_flash_if_deadline_passed and raise_flash_after_topic_dropped. We will have two classes InstructorDeleteSignUpAction and ParticipantDeleteSignupAction extending the above class having respective flash generation code.

  • Don't Repeat Yourself (DRY):

Throughout the refactor, we emphasized reducing redundancy and promoting code reuse. For instance, in the signup_as_instructor_action method, we reduced the function length and complexity by extracting common functionality into helper functions. By doing so, we minimized repetition, improved readability, and ensured consistency across the codebase.We can reuse the log_message() created in part 3 to write application logs. Delete_signup_common is also introduced to ensure the DRY principle is maintained in the code.

  • Readability and Maintainability:

We prioritized enhancing the readability and maintainability of the codebase by introducing meaningful variable names, refactoring conditional blocks, and adding descriptive comments. Clear and descriptive variable names such as assignment_id and topic_id were used to improve code readability and understanding. Additionally, detailed comments were added to describe each section of the code and explain the purpose of methods and conditional blocks, making it easier for developers to understand the logic and functionality.

Plan of Work

Based on the feedback given in the OSS project we will change some method names, such as 'delete_signup_common' renamed for better clarity , adding comments for longer methods to explain the flow of method. Additionally add the test cases to the existing framework to accommodate the changes made.

Solutions

  • The previous version of the project fixed the naming discrepancy issue. We have validated it and found no other changes.
  • signup_as_instructor and signup_as_instructor_action methods functionalities breakdown

signup_as_instructor: This function appears to be an empty function that doesn't perform any action. It is not called anywhere in the provided code. Hence, it seems unnecessary and can be removed.

signup_as_instructor_action: This function is used to process the instructor's request to sign up a student for a particular topic for an assignment. It handles the form submission and performs the necessary actions based on the submitted data.

  • Refactored delete_signup and delete_signup_as_instructor Commit

By centralizing the shared functionality, delete_signup_common, a new method, lessens redundancy and enhances maintainability. A single function can handle both scenarios thanks to the refactored code, which uses the who_user parameter to identify the type of user doing the delete operation.

The error handling logic has been enhanced to provide more descriptive error messages and to log errors appropriately using the ExpertizaLogger. Logging has been improved to include more detailed information about the errors and actions being performed. Comments have been added to describe each section of the code and explain the purpose of the methods and conditional blocks.

After refactoring 

 def delete_signup_common(who_user, participant, assignment, drop_topic_deadline, topic_id)
 # This method centralizes the shared functionality, reducing redundancy and improving maintainability
 #this function is used to delete a previous signup
 if !participant.team.submitted_files.empty? || !participant.team.hyperlinks.empty?
   # Handle case where participant has already submitted work
   if who_user == "instructor"
     flash[:error] = 'The student has already submitted their work, so you are not allowed to remove them.'
   else
     flash[:error] = 'You have already submitted your work, so you are not allowed to drop your topic.'
   end
   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)
   # Handle case where drop topic deadline has passed
   if who_user == "instructor"
     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
     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)
   end
 else
   # No errors, proceed with dropping topic
   if who_user == "instructor"
     delete_signup_for_topic(assignment.id, topic_id, participant.user_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: ' + topic_id.to_s)
   else
     delete_signup_for_topic(assignment.id, 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: ' + topic_id.to_s)
   end
  end
 end

def delete_signup
 who_user = "student"
 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(who_user, participant, assignment, drop_topic_deadline, topic_id)
 redirect_to action: 'list', id: params[:id]
end

def delete_signup_as_instructor
 who_user = "instructor"
 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(who_user, participant, assignment, drop_topic_deadline, topic_id)
 redirect_to controller: 'assignments', action: 'edit', id: assignment.id
 end
  • Refactoring the Update method Commit

The refactored version is more concise and readable. It uses the Rails 'update_attributes' method to update multiple attributes of the @topic object in a single call. It also sets a success flash message when the topic is updated successfully, which was missing in the original code.

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] )
     flash[:success] = 'The topic has been successfully updated.'
     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
  • Refactoring the signup_as_instructor_action method Commit

The refactored version is more concise and readable. We have reduced the function length which was raised in the Code Climate issue. Also added multiple helper function to reduce the if-else ladder complexity. We added a new user_registered_for_assignment, process_signup_as_instructor_request helper functions as well as log_message function to make sure DRY principle is followed while logging messages. The refactored version assigns params[:assignment_id] and params[:topic_id] to assignment_id and topic_id variables, respectively, to avoid repetitive use of these parameters and increase code readability. The refactored version enhances readability by using meaningful variable names (assignment_id and topic_id), the string interpolation ("The student is not registered for the assignment: #{user.id}") is used, providing a more elegant way of embedding user.id within the string. The redirection statement is cleaner, referencing the previously defined assignment_id variable (redirect_to(controller: 'assignments', action: 'edit', id: assignment_id)), reducing repetition and enhancing code clarity.

After refactoring 

  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
     assignment_id = params[:assignment_id]
     topic_id = params[:topic_id]    
     if user_registered_for_assignment?(user, assignment_id)
       process_signup_as_instructor_request(assignment_id,user,topic_id)
     else
       log_message("The student is not registered for the assignment: #{user.id}")
     end
   end
   redirect_to controller: 'assignments', action: 'edit', id: assignment_id
 end

 def user_registered_for_assignment?(user, assignment_id) # to check if user has registered for the assignment or not.
   if AssignmentParticipant.exists?(user_id: user.id, parent_id: assignment_id)
     true
   else
     flash[:error] = 'The student is not registered for the assignment!'
     false
   end
 end

 def log_message(message) # function to log the message
   ExpertizaLogger.info LoggerMessage.new(controller_name, , message)
 end

 def process_signup_as_instructor_request(assignment_id,user,topic_id) # function to add user for a given assignment and given topic
   if SignUpSheet.signup_team(assignment_id, user.id, topic_id)
     flash[:success] = 'You have successfully signed up the student for the topic!'
     log_message("Instructor signed up student for topic: #{topic_id}")
   else
     flash[:error] = 'The student has already signed up for a topic!'
     log_message('Instructor is signing up a student who already has a topic')
   end
 end
  • Refactoring the save_topic_deadlines method Part-1 Commit

As part 1 of refactoring save_topic_deadlines we will be reducing the number of lines of code for this function as it exceeds the limit which was raised in code climate. For this we gave create separate create and update methods of creating and saving TopicDueDate object. Additionally instance_variable_get('@assignment_' + deadline_type + '_due_dates')[i - 1] and instance_variable_get('@topic_' + deadline_type + '_due_date') were used multiple number of times. Hence by applying DRY principle we created variables for the same only one time and used the varaible at different locations of the method.

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[topics[index].id.to_s + '_submission_' + i.to_s + '_due_date']
       @topic_review_due_date = due_dates[topics[index].id.to_s + '_review_' + i.to_s + '_due_date']
       @assignment_submission_due_date = DateTime.parse(@assignment_submission_due_dates[i - 1].due_at.to_s).strftime('%Y-%m-%d %H:%M')
       @assignment_review_due_date = DateTime.parse(@assignment_review_due_dates[i - 1].due_at.to_s).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 instance_variable_get('@topic_' + deadline_type + '_due_date') == instance_variable_get('@assignment_' + deadline_type + '_due_date')

         topic_due_date = begin
                            TopicDueDate.where(parent_id: topic.id, deadline_type_id: deadline_type_id, round: i).first
                          rescue StandardError
                            nil
                          end
         due_date_obj=instance_variable_get('@assignment_' + deadline_type + '_due_dates')[i - 1]  #applying DRY principle and removing multiple instance_variable_get calls
         due_at=instance_variable_get('@topic_' + deadline_type + '_due_date')                 
         if topic_due_date.nil? # create a new record
           create_topic_due_date(i,topic,deadline_type_id,due_date_obj,due_at)
         else # update an existed record 
           topic_due_date.update_attributes(due_at: due_at,submission_allowed_id: due_date_obj.submission_allowed_id,review_allowed_id: due_date_obj.review_allowed_id,
           review_of_review_allowed_id: due_date_obj.review_of_review_allowed_id,quiz_allowed_id: due_date_obj.quiz_allowed_id,teammate_review_allowed_id: due_date_obj.teammate_review_allowed_id)
         end
       end
     end
   end
   redirect_to_assignment_edit(params[:assignment_id])
 end
 
 def create_topic_due_date(index,topic,deadline_type_id,due_date_obj,due_at)
   TopicDueDate.create(
             due_at: due_at,
             deadline_type_id: deadline_type_id,
             parent_id: topic.id,
             submission_allowed_id: due_date_obj.submission_allowed_id,
             review_allowed_id: due_date_obj.review_allowed_id,
             review_of_review_allowed_id: due_date_obj.review_of_review_allowed_id,
             round: index,
             flag: due_date_obj.flag,
             threshold: due_date_obj.threshold,
             delayed_job_id: due_date_obj.delayed_job_id,
             deadline_name: due_date_obj.deadline_name,
             description_url: due_date_obj.description_url,
             quiz_allowed_id: due_date_obj.quiz_allowed_id,
             teammate_review_allowed_id: due_date_obj.teammate_review_allowed_id,
             type: 'TopicDueDate'
           )
 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
  • sign_up_sheet_controller_spec.rb

Relevant Links

Team

Mentor: Anvitha Reddy Gutha - agutha@ncsu.edu

  • Mitul Patel - mpatel27@ncsu.edu
  • Harshil Sanghavi - hsangha@ncsu.edu
  • Sagar Dama - sudama2@ncsu.edu
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=E2409_Refactor_sign_up_sheet_controller.rb&oldid=156211"