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

4. 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. The significance of each method is as follows.

• init_team_type method. The purpose of this method is to provide team_type initialization as a common functionality to the methods in teams_controller.

Existing code snippet:

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

The above line is commonly being used in multiple teams_controller.rb methods. So in order to modularize it, we have moved it to a separate method. And the if condition is no longer a one-liner because one-liner if conditions in Ruby are better suited to checking for a sole condition. Code snippet after refactoring the teams_controller.rb:

def init_team_type(type)
  if type and Team.allowed_types.include?(type)
    session[:team_type] = type
  end
end

• get_parent_id method. The purpose of this method is to improve readability and maintainability and reduce duplication (thereby reducing the risk of error!).

Existing code snippet:

parent = Object.const_get(session[:team_type]).find(params[:id])

Code snippet after refactoring the teams_controller.rb:

def get_parent_by_id(id)
  Object.const_get(team_type).find(id)
end

and function call would be like

parent = get_parent_by_id(params[:id])

• get_parent_from_child method. The purpose of this method is to improve readability and maintainability and reduce duplication (thereby reducing the risk of error!).

Existing code snippet:

parent = Object.const_get(session[:team_type]).find(@team.parent_id)

Code snippet after refactoring the teams_controller.rb:

def get_parent_from_child(child)
  Object.const_get(team_type).find(child.parent_id)
end

and function call would be like

parent = get_parent_from_child(@team)

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

Teams Model

This is a model class that contains all the reusable class methods and has the necessary checks and attributes for performing checks on teams.

List of changes

We worked on the following work items(WIs)
WI1 : Refactor randomize_all_by_parent method to modify the existing logic which contains a lot of branching.
WI2 : Refactor assign_single_users_to_teams method. Made changes to perform an early return and modified the looping logic.
WI3 : Refactor create_team_from_single_users method. Made changes to modify the looping logic in a better way to avoid a few extra variables.

Solutions Implemented and Delivered

1. Refactor randomize_all_by_parent method. The purpose of this block is to find teams that still need team members and users who are not in any team. In Order to achieve that, we have modified the existing logic to avoid branching and achieve the same result as follows.

Existing code snippet:

teams_num = teams.size
i = 0
teams_num.times do
    teams_users = TeamsUser.where(team_id: teams[i].id)
    teams_users.each do |teams_user|
       users.delete(User.find(teams_user.user_id))
    end
    if Team.size(teams.first.id) >= min_team_size
       teams.delete(teams.first)
    else
       i += 1
    end
end

Code snippet after refactoring the randomize_all_by_parent method in team.rb file.

teams.each { |team| TeamsUser.where(team_id: team.id).each { |teams_user| users.delete(User.find(teams_user.user_id)) } }
teams.reject! { |team| Team.size(team.id) >= min_team_size }

2. Refactor assign_single_users_to_teams method. The purpose of this block is to fill the teams with users to meet the minimum number of students requirement. In this loop, once the users are empty we can simply return without any additional work which is being done in the existing logic. Instead of two break statements at lines 152 and 154, we can simply have one statement with “return” at line 152 to stop the function early. In summary, we can replace the existing block of code as shown below.

Existing code snippet:

teams.each do |team|
    curr_team_size = Team.size(team.id)
    member_num_difference = min_team_size - curr_team_size
    while member_num_difference > 0
        team.add_member(users.first, parent.id)
        users.delete(users.first)
        member_num_difference -= 1
        break if users.empty?
    end
    break if users.empty?
end

Code snippet after refactoring the assign_single_users_to_teams method in team.rb file.

teams.each do |team|
    curr_team_size = Team.size(team.id)
    member_num_difference = min_team_size - curr_team_size
    member_num_difference.times do
        team.add_member(users.first, parent.id)
        users.delete(users.first)
        return if users.empty?
    end
end

3. Refactor create_team_from_single_users method. This block works on adding single users who are not in any team by creating a new team for them. The block can be rewritten as below.

Existing code snippet:

min_team_size.times do
   break if next_team_member_index >= users.length

   user = users[next_team_member_index]
   team.add_member(user, parent.id)
   next_team_member_index += 1
end

Code snippet after refactoring the team.rb:

(0..[min_team_size, users.length].min).each do |index|
     user = users[index]
     team.add_member(user, parent.id)
end

Teams Views

These are the views that are rendered in the frontend when the team's related web pages are loaded.

List of changes

We worked on the following work items(WIs)
WI1 : Move a conditional check in list.html.erb to the list method in the controller.
WI2 : Refactor an if-else block using ternary operator in new.html.erb.
WI3 : Refactor an if-else block using the ternary operator in _team.html.erb.

Solutions Implemented and Delivered

1. Move a conditional check used in multiple places in list.html.erb to the list method in the controller, so that it can simply be used as a boolean variable in the view.

Existing code snippet:

<% if session[:team_type] == 'Assignment'&& @assignment.max_team_size > 1 %>

We moved the condition in the if-block to the list method in the controller and use it here as a boolean variable so that the code becomes short and more readable. Code snippet after refactoring in list.html.erb

<% if @is_valid_assignment %>

Code snippet after refactoring in teams_controller.rb list method.

@is_valid_assignment = session[:team_type] == Team.allowed_types[0] && @assignment.max_team_size > 1

Testing Details

RSpec

• We have thoroughly tested all the test cases in teams_controller_spec.rb. And we have also fixed one of the flaky test cases which are mentioned above in one of the code changes. In addition to the unit tests, we have also made sure that there are no issues in the UI with the changes in the teams_controller.rb.
• In addition to the controller, we have also thoroughly tested the model using all the test cases in team_spec.rb. We have also verified the same in the expertiza UI.

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.