CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.

Team

Mentor

• Ed Gehringer (efg)

Team Members

• Juan Benavides (jdbenavi)
• Wei-Chi Chen (wchen37)
• Swetha Sairamakrishnan (ssairam)

Introduction

Expertiza is a 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.

Peer Review Information

For users intending to view the deployed Expertiza associated with this assignment, the details are below:

• We have deployed the project at http://152.7.99.122:8080/
• Instructor login: username -> instructor6, password -> password
• Student login: username -> student5899, password -> password

About Controller

The sign up sheet controller performs the following functions:

• Allows an instructor to add/remove topics to an assignment.
• Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
• Allows a student to see the list of available topics which can be bid on for 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.
• 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 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. delete_signup_as_instructor

Solutions

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

Before changing variable names

 def new
   @id = params[:id]
   @sign_up_topic = SignUpTopic.new
   @sign_up_topic.assignment = Assignment.find(params[:id])
   @topic = @sign_up_topic
 end
 
 def set_values_for_new_topic
   @sign_up_topic = SignUpTopic.new
   @sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
   @sign_up_topic.topic_name = params[:topic][:topic_name]
   @sign_up_topic.max_choosers = params[:topic][:max_choosers]
   @sign_up_topic.category = params[:topic][:category]
   @sign_up_topic.assignment_id = params[:id]
   @assignment = Assignment.find(params[:id])
 end

After changing variable names

 def new
   @id = params[:id]
   @signup_topic = SignUpTopic.new
   @signup_topic.assignment = Assignment.find(params[:id])
   @topic = @signup_topic
 end
 
 def set_values_for_new_topic
   @signup_topic = SignUpTopic.new
   @signup_topic.topic_identifier = params[:topic][:topic_identifier]
   @signup_topic.topic_name = params[:topic][:topic_name]
   @signup_topic.max_choosers = params[:topic][:max_choosers]
   @signup_topic.category = params[:topic][:category]
   @signup_topic.assignment_id = params[:id]
   @assignment = Assignment.find(params[:id])
 end

Before changing controller name

 class SignUpSheetController < ApplicationController
 ...
   render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
 ...

After changing controller name

 class SignupSheetController < ApplicationController
 ...
   render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
 ...

Before changing helper function names

 <%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'), 
     signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
 ...
 <%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
     sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

After changing helper function names

 <%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
     signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
 ...
 <%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
     signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored based on DRY principle by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined. Based on DRY principle, hence existing methods have been re-used instead of adding more code.

Previous version of update:

 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

Refactored update:

 def update
   @topic = SignUpTopic.find(params[:id])
   if @topic
     update_waitlist @topic
     @topic.update_attributes(topic_params)
     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

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update. This change reduces side effects and makes the function's responsibility clear.

 def update_existing_topic(topic)
   update_waitlist topic
   topic.update_attributes(topic_params)
   redirect_to_sign_up(params[:id])
 end

 def update_waitlist(topic)
   # While saving the max choosers you should be careful; if there are users who have signed up for this particular
   # topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
   # there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
   # it.
   if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
     return
   elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
     topic.update_waitlisted_users topic_params[:max_choosers]
   else
     flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
   end
 end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics(). This change is done based on DRY principle to remove redundant methods.

Before

 def add_signup_topics_staggered
   add_signup_topics
 end
 
 # simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
 # staggered means that different topics can have different deadlines.
 def redirect_to_sign_up(assignment_id)
   assignment = Assignment.find(assignment_id)
   assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id) 
                                         : (redirect_to action: 'add_signup_topics', id: assignment_id)
 end

After

 # simple function that redirects ti the /add_signup_topics page
 def redirect_to_sign_up(assignment_id)
   assignment = Assignment.find(assignment_id)
   redirect_to action: 'add_signup_topics', id: assignment_id
 end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics(). This change is done based on DRY principle to remove redundant methods.

Before

 def add_signup_topics
   load_add_signup_topics(params[:id])
   SignUpSheet.add_signup_topic(params[:id])
 end
 
 # retrieves all the data associated with the given assignment. Includes all topics,
 def load_add_signup_topics(assignment_id)
   @id = assignment_id
   @sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
   @slots_filled = SignUpTopic.find_slots_filled(assignment_id)
   @slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)
 
   @assignment = Assignment.find(assignment_id)
   # ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
   # 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 recorded are also counted)
   @participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
 end

After

 def add_signup_topics
   load_add_signup_topics(params[:id])
   SignUpSheet.add_signup_topic(params[:id])
 end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. Based on YAGNI principle (You Ain't Gonna Need It), the feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. Also, in the current version, Drop Student (delete signup as instructor) seems to be broken (leads to non-existent path). Hence all references to sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed in its associated views, routes and tests for signup_sheet. The topic tab under edit assignments does not contain the corresponding check and drop icons against the participant list to perform this task anymore.

https://github.com/weichic-ncsu/expertiza/commit/6853860454c4e6eb3bcfd97ce46b8cd8e0f7b87e

https://github.com/weichic-ncsu/expertiza/commit/2a8d371237936fc03589b58b413a9babb426c1af

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

Before

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

In views:

...
 <%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...

...
 <% if @use_bookmark %>
...

After

# method to return a list of topics for which a bid has been made by a team
  def compute_signed_up_topics
    signed_up_topics = []
    @bids.each do |bid|
      signup_topic = SignUpTopic.find_by(id: bid.topic_id)
      signed_up_topics << signup_topic if signup_topic
    end
    signed_up_topics &= @signup_topics
    return signed_up_topics
  end

  def find_topic_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)
  end

  def list
    @participant = AssignmentParticipant.find(params[:id].to_i)
    @assignment = @participant.assignment
    @signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
    @slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
    @slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
    @show_actions = true
    @priority = 0
    team_id = @participant.team.try(:id)

    # if assignment is intelligent, want to know which topics the team has already bid on
    if @assignment.is_intelligent
      @bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
      @bids = compute_signed_up_topics()
      @signup_topics -= @bids
    end

    @num_of_topics = @signup_topics.size
    @student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
    find_topic_deadlines()

    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('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
  end

In views:

...
 <%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...

...
 <% if @assignment.use_bookmark %>
...

Test Plan

Edge Cases and Pre-Conditions

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

RSpec

As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:

rspec spec/controllers/signup_sheet_controller_spec.rb

We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.

rspec spec/helpers/signup_sheet_helper_spec.rb

rspec spec/models/signup_sheet_spec.rb

New test cases

Destroy signup_sheet when there are other topics in the assignment

New, more general test case added for rendering list view, as we noticed that some functionalities we broke while refactoring did not result in failed test cases.

Mandatory

Login as instructor

1. Hover on Manage Tab
2. Click on Assignments Tab
3. Click on Edit Button for "OSS project & documentation" assignment
4. Select the Topic Tab
5. Add New Topic or Delete topic using links in the form below

Login as student

1. Click on OSS project/Writing assignment 2
2. Click on Signup sheet
3. The list must be visible, which indicates functionality is as before after refactoring list function.