CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb)
 
 
(43 intermediate revisions by 3 users not shown)
Line 1: Line 1:
This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)
=== Team Members ===
* Palvit Garg (pgarg5)
* Varun Garg (vgarg4)
* Jagdish Kini (jkini)


== Introduction ==
== Introduction ==
Line 6: Line 14:
== Peer Review Information ==
== Peer Review Information ==


For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:
For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project http://152.7.98.81:8080/
* Instructor login: username -> instructor6,  password -> password
* Instructor login: username -> instructor6,  password -> password
* Student login: username -> student5899,  password -> password
* Student login: username -> student5899,  password -> password
Line 19: Line 28:
1. 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.
1. 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.


2. Create method has and 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 and rectify it. [This may have already been fixed.]
2. 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]
 
3. Update method has a plethora of instance variables defined before updating. These might not be necessary (For 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.]
 
4. Add_signup_topics_staggered does not do anything different from add add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set.


5. Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
3. Several method names can be improved.


6. 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.
4. 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.


7. 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.
5. 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.


8. Refactor participants variable in  load_add_signup_topics
6. Signup_as_instructor_action has if-else ladder. It can be made more elegant.


9. Signup_as_instructor_action has if-else ladder. It can be made more elegant.
7.Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.


10. Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.
8.Giving elegant and more understandable names to flash in flash_delete_signup_message function


== Solutions ==
== Solutions ==
1. Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
1. Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.


2. Changed the create method to only create a new topic if that topic doesn't already exist. If it does exist, the program flashes an error message and backs out.  
https://github.com/palvitgarg99/expertiza/commit/9e99f56c9f8051fa5ee6b6a4bdf3659ec50852dc
https://github.com/palvitgarg99/expertiza/commit/66fc8bc5fc3850639306ad2be79afaa3de1fad70


3. Used a single update_attributes method in place of the many instance variables.
2. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed.


'''Before'''
https://github.com/palvitgarg99/expertiza/commit/db12bf9c05831576183363b6bfe68bbad64e9c5a
<pre>
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
      params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
      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
</pre>


'''After'''
3. Renaming function ad_info
<pre>
def update
    @topic = SignUpTopic.find(params[:id])
    if @topic
      update_max_choosers @topic
      updated_max_choosers = @topic.max_choosers
      @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
      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
</pre>


4. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed.
https://github.com/palvitgarg99/expertiza/commit/6507f15f6d23f44900465536a91cc6c80f90ad75


5. Load_add_signup_topics was removed and combined with add_signup_topics.  Ad_info was renamed to ads_for_topic and refactored to remove instance variables.  
4. Remove signup_as_instructor, since its not being used anywhere as such.


'''Before'''
https://github.com/palvitgarg99/expertiza/commit/e92e704f5cb6c549d440784339bcce6eba408a6b
<pre>
def add_signup_topics
    load_add_signup_topics(params[:id])
    SignUpSheet.add_signup_topic(params[:id])
  end


  def add_signup_topics_staggered
5. The list method is broken into 2 functions. The small part of computing signup topics is created. Also comments has been added to understand the function better.
    add_signup_topics
  end


  # retrieves all the data associated with the given assignment. Includes all topics,
https://github.com/palvitgarg99/expertiza/commit/4b7bb6113dfb8984d7aa4dca467b55f67de3c93a
def load_add_signup_topics(assignment_id)
https://github.com/palvitgarg99/expertiza/commit/bd86f5a4ec8aa02c27a0cd34c85bd21776feca2c
    @id = assignment_id
https://github.com/palvitgarg99/expertiza/commit/56be9eed8ac89632041608947108f87dfbcb28ab
    @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)
6. Signup_as_instructor_action's if-else ladder has been made more elegant.  
    # 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 recored are also counted)
    ip = session[:ip]
    @participants = SignedUpTeam.find_team_participants(@id, ip)
  end


def show_team
https://github.com/palvitgarg99/expertiza/commit/50aae43edb7fcec3804deb93df28471db1b31c15
    if !(assignment = Assignment.find(params[:assignment_id])).nil? and !(topic = SignUpTopic.find(params[:id])).nil?
      @results = ad_info(assignment.id, topic.id)
    .........


def ad_info(_assignment_id, topic_id)
7. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements. To solve this issue, flash_delete_signup_message function was created.
    # List that contains individual result object
    @result_list = []
    # Get the results
    @results = SignedUpTeam.where("topic_id = ?", topic_id.to_s)
    # Iterate through the results of the query and get the required attributes
    @results.each do |result|
      team = result.team
      topic = result.topic
      resultMap = {}
      resultMap[:team_id] = team.id
      resultMap[:comments_for_advertisement] = team.comments_for_advertisement
      resultMap[:name] = team.name
      resultMap[:assignment_id] = topic.assignment_id
      resultMap[:advertise_for_partner] = team.advertise_for_partner


      # Append to the list
https://github.com/palvitgarg99/expertiza/commit/ddf35e88a2486d5ca748d3d93b15d9f6f4984af9
      @result_list.append(resultMap)
https://github.com/palvitgarg99/expertiza/commit/df1342b5239bb4e49214149b877a5ccf9c6117ae
    end
    @result_list
  end
</pre>


'''After'''
8. Flash messages names have been changed and given for meaningful names.
<pre>
def add_signup_topics
    / load_add_signup_topics(params[:id])/
    # retrieves all the data associated with the given assignment. Includes all topics,
    @id = params[:id]
    @sign_up_topics = SignUpTopic.where('assignment_id = ?', params[:id])
    @slots_filled = SignUpTopic.find_slots_filled(params[:id])
    @slots_waitlisted = SignUpTopic.find_slots_waitlisted(params[:id])


    @assignment = Assignment.find(params[:id])
https://github.com/palvitgarg99/expertiza/commit/47b4b11e4b4b2e6793dbd54259ae36749ed113a7
    # 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 recored are also counted)
    @participants = SignedUpTeam.find_team_participants(params[:id], session[:ip])
    SignUpSheet.add_signup_topic(params[:id])
  end


def add_signup_topics_staggered
== Testing ==
    add_signup_topics
; RSpec
  end
: 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


def show_team
    if !(assignment = Assignment.find(params[:assignment_id])).nil? and !(topic = SignUpTopic.find(params[:id])).nil?
      @results = ads_for_topic(assignment.id, topic.id)
    .........


def ads_for_topic(_assignment_id, topic_id)
    # List that contains individual result object
    ads = []
    # Get the results
    signed_up_teams = SignedUpTeam.where("topic_id = ?", topic_id.to_s)
    # Iterate through the results of the query and get the required attributes
    signed_up_teams.each do |result|
      team = result.team
      topic = result.topic
      new_ad = {}
      new_ad[:team_id] = team.id
      new_ad[:comments_for_advertisement] = team.comments_for_advertisement
      new_ad[:name] = team.name
      new_ad[:assignment_id] = topic.assignment_id
      new_ad[:advertise_for_partner] = team.advertise_for_partner


      # Append to the list
; Physical Testing
      ads.append(new_ad)
    end
    ads
  end
</pre>


6. signup_as_instructor is not needed and has been removed.
We found that add_signup_topics_staggered was simply calling add_signup_topics function. So we eliminated the add_signup_topics_staggered function and manually checked and found that it is working perfectly fine.


7. Comments on the list method have been provided, but the code is best left in the list method.
Delete signup and delete signup as instructor method successfully broken down to remove the dry code by creating a new helper function. This functionality has not been broken due to any changes and has been checked by running rspec and testing manually.
 
:  
8. Refactored the participants variable and improved readability.
=== Edge Cases and Pre-Conditions ===
 
# When dropping topic if submission already done.
'''Before'''
# When deadline from dropping topic has passed.
<pre>
# Deleting topic when topic cannot be found.
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
# Signup in case when user cannot be found.
</pre>
:
 
'''After'''
<pre>
ip = session[:ip]
@participants = SignedUpTeam.find_team_participants(@id, ip)
</pre>
 
9.  Previously signup_as_instructor_action provided unhelpful feedback to the instructor by only stating if the instructor could/couldn't assign a topic for a student.  The refactored code provides the instructor with the student id of the person that they are assigning, the team that the student is on, and the topic that the instructor is assigning to the team.
 
'''Before'''
  <pre>
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
      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
    end
    redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end
  </pre>
 
'''After'''
  <pre>
def signup_as_instructor_action
    #put name of student and team in log
    # flash name of student and team
    user = User.find_by(name: params[:username])
    team = Team.find_team_for_assignment_and_user(params[:assignment_id], user.id).first
    assignment = Assignment.find(params[:assignment_id])
    if user.nil? # validate invalid user
      flash[:error] = user.name + " does not exist!"
    else #If the user is not null and the student exists then sign up student for topic
      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 " + user.name + " on " + team.name + "  for the topic " + params[:topic_id].to_s
          ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up ' + user.name + ' on team ' + team.name + ' for topic: ' + params[:topic_id].to_s)
        else #If the students team already has a topic then cancel the sign up
          flash[:error] = user.name + " on " + team.name + " has already signed up for a topic!"
          ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a ' + user.name + 'on ' + team.name + ' who already has a topic')
        end
      else #If the student isn't a part of the assignment then cancel the sign up
        flash[:error] = user.name + " is not registered for the assignment!"
        ExpertizaLogger.info LoggerMessage.new(controller_name, '', user.name + ' is not registered for the assignment: ' + assignment.name)
      end
    end
    redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end
  </pre>
 
10. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements.  To solve this issue, delete_signup_heleper function was created.
 
'''Before'''
<pre>
def delete_signup
    participant = AssignmentParticipant.find(params[:id])
    assignment = participant.assignment
    drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
    # A student who has already submitted work should not be allowed to drop his/her topic!
    # (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
      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)
    end
    redirect_to action: 'list', id: params[:id]
  end


  def delete_signup_as_instructor
=== Login as instructor ===
    # find participant using assignment using team and topic ids
    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)
    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
      delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
      flash[:success] = "You have successfully dropped the student from the topic!"
      ExpertizaLogger.error 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
</pre>
 
'''After'''
<pre>
def delete_signup_helper(isInstructor, participant, assignment, drop_topic_deadline)
    messages = ["You have already submitted your work, so you are not allowed to drop your topic.",
                "You cannot drop your topic after the drop topic deadline!",
                "You have successfully dropped your topic!",
                "The team has already submitted their work, so you are not allowed to remove them.",
                "You cannot drop a student after the drop topic deadline!",
                "You have successfully dropped the student from the topic!"
    ]
    if isInstructor == false
      flash1 = messages[0]
      flash2 = messages[1]
      flash3 = messages[2]
      id_param = session[:user]
    else
      flash1 = messages[3]
      flash2 = messages[4]
      flash3 = messages[5]
      id_param = participant
    end
 
    if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
      flash[:error] = flash1
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping failed 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] = flash2
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping failed for ended work: ' + params[:topic_id].to_s)
    else
      delete_signup_for_topic(assignment.id, params[:topic_id], id_param.id)
      flash[:success] = flash3
      ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
    end
  end
 
  # this function is used to delete a previous signup
  def delete_signup
    participant = AssignmentParticipant.find(params[:id])
    assignment = participant.assignment
    drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
    # A student who has already submitted work should not be allowed to drop his/her topic!
    # (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.
    delete_signup_helper(false, participant, assignment, drop_topic_deadline)
    redirect_to action: 'list', id: params[:id]
  end
 
  def delete_signup_as_instructor
    # find participant using assignment using team and topic ids
    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)
    delete_signup_helper(true, participant, assignment, drop_topic_deadline)
    redirect_to controller: 'assignments', action: 'edit', id: assignment.id
  end
</pre>
 
== Testing ==
; RSpec
: signup_as_instructor_action is the only function updated.  All other functions has remained the same.  To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
; Physical Testing
:
# Login as instructor
# Hover on Manage Tab
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Assignments Tab
Line 387: Line 107:
# Enter a student UnityID and click submit
# Enter a student UnityID and click submit


# Login as student
=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# Click on Signup sheet
# The list must be visible, which indiactes functionality is as before after refactoring list fucntion.
# The list must be visible, which indicates functionality is as before after refactoring list function.

Latest revision as of 03:52, 27 March 2022

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

Team

Mentor

  • Ed Gehringer (efg)

Team Members

  • Palvit Garg (pgarg5)
  • Varun Garg (vgarg4)
  • Jagdish Kini (jkini)

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 http://152.7.98.81:8080/
  • Instructor login: username -> instructor6, password -> password
  • Student login: username -> student5899, password -> password

About Controller

The sign up sheet controller

  • Allows an instructor to add/remove topics
  • Allows an instructor to assign/remove students to topics
  • Allows an student to see the list of available topics which can be bid on for given OSS assignment.

Issues

1. 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.

2. 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]

3. Several method names can be improved.

4. 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.

5. 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.

6. Signup_as_instructor_action has if-else ladder. It can be made more elegant.

7.Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.

8.Giving elegant and more understandable names to flash in flash_delete_signup_message function

Solutions

1. Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.

https://github.com/palvitgarg99/expertiza/commit/9e99f56c9f8051fa5ee6b6a4bdf3659ec50852dc https://github.com/palvitgarg99/expertiza/commit/66fc8bc5fc3850639306ad2be79afaa3de1fad70

2. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed.

https://github.com/palvitgarg99/expertiza/commit/db12bf9c05831576183363b6bfe68bbad64e9c5a

3. Renaming function ad_info

https://github.com/palvitgarg99/expertiza/commit/6507f15f6d23f44900465536a91cc6c80f90ad75

4. Remove signup_as_instructor, since its not being used anywhere as such.

https://github.com/palvitgarg99/expertiza/commit/e92e704f5cb6c549d440784339bcce6eba408a6b

5. The list method is broken into 2 functions. The small part of computing signup topics is created. Also comments has been added to understand the function better.

https://github.com/palvitgarg99/expertiza/commit/4b7bb6113dfb8984d7aa4dca467b55f67de3c93a https://github.com/palvitgarg99/expertiza/commit/bd86f5a4ec8aa02c27a0cd34c85bd21776feca2c https://github.com/palvitgarg99/expertiza/commit/56be9eed8ac89632041608947108f87dfbcb28ab

6. Signup_as_instructor_action's if-else ladder has been made more elegant.

https://github.com/palvitgarg99/expertiza/commit/50aae43edb7fcec3804deb93df28471db1b31c15

7. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements. To solve this issue, flash_delete_signup_message function was created.

https://github.com/palvitgarg99/expertiza/commit/ddf35e88a2486d5ca748d3d93b15d9f6f4984af9 https://github.com/palvitgarg99/expertiza/commit/df1342b5239bb4e49214149b877a5ccf9c6117ae

8. Flash messages names have been changed and given for meaningful names.

https://github.com/palvitgarg99/expertiza/commit/47b4b11e4b4b2e6793dbd54259ae36749ed113a7

Testing

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


Physical Testing

We found that add_signup_topics_staggered was simply calling add_signup_topics function. So we eliminated the add_signup_topics_staggered function and manually checked and found that it is working perfectly fine.

Delete signup and delete signup as instructor method successfully broken down to remove the dry code by creating a new helper function. This functionality has not been broken due to any changes and has been checked by running rspec and testing manually.

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.

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. Click the Green Check for any topic
  6. Enter a student UnityID and click submit

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.
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2022_-_E2223._Refactor_sign_up_sheet_controller.rb&oldid=143589"