CSC/ECE 517 Spring 2022 - E2214: Refactor teams controller

This wiki page is for the description of changes made under E2214 OSS assignment for Spring 2022, CSC/ECE 517.

Peer Review Information

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

Instructor Login: username -> instructor6, password -> password

Expertiza Background

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU. It’s an open-source project developed on the Ruby on Rails platform and its code is available on Github. It allows students to review each other’s work and improve their work upon this feedback.

Description of the current project

The following is an Expertiza based OSS project which deals primarily with the models, views, and controllers of Teams. Core to any online learning platform is the ability of students and instructors to create and delete teams. Teams_Controller primarily handles the creation, deletion, and other behaviors of course teams and assignment teams. Most of its methods are called from the instructor’s point of view in expertiza. This controller has some problems that violate some essential Rails design principles. There are a few methods in this controller that should have been in model classes. Some methods share code, which creates code repetition. Our project primarily focuses on refactoring some complex methods that involve high branching, removing redundant and dead code, modularising common functionalities, and condensing the existing bad code into a few lines to achieve the same functionality. The goal of this project is to attempt to make this part of the application easier to read and maintain.

Files modified in current project

Controller, model and views were modified for this project namely:
1. Teams Controller: teams_controller
2. Teams Model: team.rb
3. Teams Views

• list.html.erb
• new.html.erb
• _team.html.erb

4. (Generic)Tree Display Views

• _page_footer.html.erb
• _entry_html.erb
• _folder.html.erb
• _listing.html.erb

Teams Controller

The controller contains all the methods to perform CRUD operations on the teams when seen from the instructor's point of view in the UI.

List of changes

We worked on the following work items(WIs)
WI1 : Move allowed types list constant in list method to model as it is a best practice to keep all the constants at one place.
WI2 : Refactor delete method. This method deletes the team and erases the records from teams_users and signed_up_teams table.
WI3 : Refactor inherit and bequeath_all methods. Both the methods contain duplicated code with nested branching statements.
WI4 : Introduce new methods like init_team_type, get_parent_by_id, get_parent_from_child which commonly operate on fetching team type to render UI elements accordingly.
WI5 : Fix for a flaky test case on creating teams with the random members method.

Solutions Implemented and Delivered

1. Refactoring in the list method. The list method contains a list of constants to check if the team_type from the session variables is a valid one or not.

Existing code snippet in teams_controller.rb:

allowed_types = %w[Assignment Course]
session[:team_type] = params[:type] if params[:type] && allowed_types.include?(params[:type])

We have moved the constant allowed_types to the teams model so that it can be used wherever needed in the teams_controller.rb.
Code snippet after refactoring in team.rb:

# Allowed types of teams -- ASSIGNMENT teams or COURSE teams
def self.allowed_types
  # non-interpolated array of single-quoted strings
  %w[Assignment Course]
end

Code snippet after refactoring showing the usage in teams_controller.rb:

session[:team_type] = params[:type] if params[:type] && Team.allowed_types.include?(params[:type])

2. Refactor delete method. This method contains an if block in which the condition is never satisfied. It is considered a dead code. Related piazza post: https://piazza.com/class/kxwmkrv8djq573?cid=210

So we have simply removed that if block. Below is the removed code snippet.

if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
   # if there is another team in waitlist, make this team hold this topic
   topic_id = @signed_up_team.first.topic_id
   next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
   # if slot exist, then confirm the topic for this team and delete all waitlists for this team
   SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
end

And also there is a statement at line 89 which we replaced with

@team.destroy

instead of

@team.destroy if @team

because we already have a nil check for this variable in line 74, so we can remove the redundant ones.

3. Refactor inherit and bequeath_all methods. Both methods contain a piece of code in common. They also have high branching in them which makes the code readability much more difficult. Existing Code Snippet:

# Copies existing teams from a course down to an assignment
# The team and team members are all copied.
def inherit
 assignment = Assignment.find(params[:id])
 if assignment.course_id
   course = Course.find(assignment.course_id)
   teams = course.get_teams
   if teams.empty?
     flash[:note] = 'No teams were found when trying to inherit.'
   else
     teams.each do |team|
       team.copy(assignment.id)
     end
   end
 else
   flash[:error] = 'No course was found for this assignment.'
 end
 redirect_to controller: 'teams', action: 'list', id: assignment.id
end

def bequeath_all
 if session[:team_type] == 'Course'
   flash[:error] = 'Invalid team type for bequeathal'
   redirect_to controller: 'teams', action: 'list', id: params[:id]
   return
 end
 assignment = Assignment.find(params[:id])
 if assignment.course_id
   course = Course.find(assignment.course_id)
   if course.course_teams.any?
     flash[:error] = 'The course already has associated teams'
     redirect_to controller: 'teams', action: 'list', id: assignment.id
     return
   end
   teams = assignment.teams
   teams.each do |team|
     team.copy(course.id)
   end
   flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
 else
   flash[:error] = 'No course was found for this assignment.'
 end
 redirect_to controller: 'teams', action: 'list', id: assignment.id
end

As a solution for this problem, we have separated the bigger functions and broken them into smaller ones as below. Code snippet after refactoring in teams_controller.rb.

# Copies existing teams from a course down to an assignment
# The team and team members are all copied.
def inherit
 copy_teams(Team.team_operation[:inherit])
end

# Handovers all teams to the course that contains the corresponding assignment
# The team and team members are all copied.
def bequeath_all
 if session[:team_type] == 'Course'
   flash[:error] = 'Invalid team type for bequeath all'
   redirect_to controller: 'teams', action: 'list', id: params[:id]
 else
   copy_teams(Team.team_operation[:bequeath])
 end
end

private

# Method to abstract the functionality to copy teams.
def copy_teams(operation)
 assignment = Assignment.find(params[:id])
 if assignment.course_id
   choose_copy_type(assignment, operation)
 else
   flash[:error] = 'No course was found for this assignment.'
 end
 redirect_to controller: 'teams', action: 'list', id: assignment.id
end

# Method to choose copy technique based on the operation type.
def choose_copy_type(assignment, operation)
 course = Course.find(assignment.course_id)
 if operation == Team.team_operation[:bequeath]
   bequeath_copy(assignment, course)
 else
   inherit_copy(assignment, course)
 end
end

# Method to perform a copy of assignment teams to course
def bequeath_copy(assignment, course)
 teams = assignment.teams
 if course.course_teams.any?
   flash[:error] = 'The course already has associated teams'
 else
   Team.copy_content(teams, course)
   flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
 end
end

# Method to inherit teams from course by copying
def inherit_copy(assignment, course)
 teams = course.course_teams
 if teams.empty?
   flash[:error] = 'No teams were found when trying to inherit.'
 else
   Team.copy_content(teams, assignment)
   flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + assignment.name + '"'
 end
end

Code snippet after refactoring in team.rb.

# copies content of one object to the another
def self.copy_content(source, destination)
 source.each do |each_element|
   each_element.copy(destination.id)
 end
end

# enum method for team clone operations
def self.team_operation
 { inherit: 'inherit', bequeath: 'bequeath' }.freeze
end

5. Fix for a flaky test case due to rails environment setup configuration. Reference: https://tinyurl.com/y64bupbk. Every time we run tests for teams_controller. There was a test case related to the create_teams method which is getting failed due to the below-attached code.
Existing code snippet causing issue:

    undo_link('Random teams have been successfully created.')
    ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Random teams have been successfully created', request)

In order to fix that we have referred to the above-attached reference and made the following code changes.
Code snippet after refactoring the teams_controller.rb

    message = 'Random teams have been successfully created'
    undo_link(message)
    # To do: Move this check to a application level commons file.
    # For now this is the only usage of this check.
    # If a similar use case pops up "To do" action needs to be performed.
    # Fix link: https://tinyurl.com/y64bupbk
    if Rails.env.development?
      ExpertizaLogger.info LoggerMessage.new(controller_name, '', message, request)
    end

GradesHelper

This is a helper class which contains methods for constructing a table(construct_table) and to check whether an assignment has a team and metareveiw(has_team_and_metareview)

List of changes

We worked on the following work items(WIs)
WI1 : Move allowed types list constant to model as it is a best practice to keep all the constants at one place.
WI2 : Refactor delete method. This method deletes the team and erases the records from teams_users and signed_up_teams table. It contains a code block which is not being executed at all (Dead Code). We have simply removed the block
WI3 : Refactor the code as per the Ruby style guidelines and incorporate the good practices
WI4 : Test the conflict_notification method to test the changes made.
WI5 : Move the repeated code in view and view_my_scores methods to a separate method retrieve_questions

Solutions Implemented and Delivered

• Refactoring calculate_all_penalties method

This is used to calculate various penalty values for each assignment if penalty is applicable.

The following changes were made:

1. This method was very complex, performing too many functions within a single method and had to be broken into 3 smaller methods each having a more well defined function. 2. The following 3 methods were created after splitting the first method

  i.  calculate_all_penalties

  ii. calculate_penatly_attributes

  iii. assign_all_penalties

3. Changes were also made to make the code follow ruby style.The language was made more ruby friendly. 4. Finally some redundant code was commented out as it was non-functional.

Refactoring into smaller more specific methods:

Removal of non-functional code :

Change of language to make it more Ruby friendly:

• Move the redundant piece of code from conflict_notification & edit methods to a new method list_questions

The conflict_notification method is used to help the instructors decide if one of the reviews are unfair or inaccurate. This was again split into 2 methods with some part of the code which is repeated in another method refactored into a new method.

Refactored #Created a method which was a duplicate in conflict_notification and edit methods

edit method:

This method is used to edit the questionnaires. This method again has code which is repeated in the conflict_notification method and thus the repeated section was split into a new method.

New method: Refactored #Created a method which was a duplicate in conflict_notification and edit methods

Similar refactoring was performed to obtain the retrieve_questions method:

This is the new method created after the above refactoring:

Testing Details

RSpec

There were no existing test cases for the GradesController. We have added a new spec file 'grades_spec.rb' which covers testing scenario for the newly added method. The specs were run on the previous and current files and they return the same results implying that the refactored code does not break anything. As the model was not changed, no test cases were added for the model.

UI Testing

Following steps needs to be performed to test this code from UI:
1. Login as instructor. Create a course and an assignment under that course.
2. Keep the has team checkbox checked while creating the assignment. Add a grading rubric to it. Add at least two students as participants to the assignment.
3. Create topics for the assignment.
4. Sign in as one of the students who were added to the assignment.
5. Go to the assignment and sign up for a topic.
6. Submit student's work by clicking 'Your work' under that assignment.
7. Sign in as a different student which is participant of the assignment.
8. Go to Assignments--><assignment name>-->Others' work (If the link is disabled, login as instructor and change the due date of the assignment to current time).
9. Give reviews on first student's work.
10. Login as instructor or first student to look at the review grades.

Scope for future improvement

1. The construct_table method in GradesHelper is not used anywhere. It has no reference in the project. So we feel it can be safely removed.
2. The has_team_and_metareview? method in GradesHelper can be broken down into separate methods, one each for team and metareview. This will provide improved flexibility. It needs some analysis though, as both the entities(team & metareview) are currently checked in conjuction from all the views they are referenced from.