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

2. Refactor an if-else statement using the ternary operator in new.html.erb.
Existing code snippet:

<% if @parent.is_a?(Course)%>
      Team Size:<%= select_tag 'team_size', options_for_select(2..4), size: 1 %>
<% else %>
      Team Size:<%= select_tag 'team_size', options_for_select(2..@parent.max_team_size), size: 1 %>
<% end %>

Code snippet after refactoring new.html.erb:

<% maxsize = @parent.is_a?(Course) ? 4 : @parent.max_team_size %>
Team Size:<%= select_tag 'team_size', options_for_select(2..maxsize), size: 1 %>

3. Refactor an if-else statement using the ternary operator in _team.html.erb.
Existing code snippet:

<% if @root_node.class == AssignmentNode %>
    <% modelType = ‘AssignmentTeam’ %>
<% else %>
    <% modelType = ‘CourseTeam’ %>
<%end>

Code snippet after refactoring _team.html.erb.

<% modelType = @root_node.class == AssignmentNode ? ‘AssignmentTeam’ : ‘CourseTeam’ %>

(Generic)Tree Display Views

These are the views that are rendered in the frontend and are generic throughout the expertiza for all the web pages.

List of changes

We worked on the following work items(WIs)
WI1 : Refactor an if-else block using ternary operator in _page_footer.html.erb.
WI2 : Refactor an if-else block using ternary operator in _entry_html.erb.
WI3 : Refactor an if-else block using the ternary operator in _folder.html.erb.
WI4 : Refactor an if-else block using the ternary operator in _listing.html.erb.

Solutions Implemented and Delivered

1. Refactor an if-else statement using the ternary operator in _page_footer.html.erb.
Existing code snippet:

<%
    @child_nodes.each_with_index do |node,index|
      if index.odd?
        rowtype = "odd"
      else
        rowtype = "even"
      end
%>

Code snippet after refactoring _page_footer.html.erb:

<%
    @child_nodes.each_with_index do |node,index|
      rowtype = (index % 2 == 0) ? "even" : "odd"
%>

2. Refactor an if-else statement using the ternary operator in _entry_html.erb.
Existing code snippet:

<% if node.get_partial_name %>
  <%if node.get_partial_name == 'courses_folder_actions' && session[:user].role_id == 6 %>
  <%else%>
    <%= render :partial => '/tree_display/actions/'+node.get_partial_name,
      :locals => {:node => node} %>
  <%end%>
<% else %>
  <%= render :partial=> '/tree_display/actions/no_actions' %>
<% end %>

Code snippet after refactoring _entry_html.erb:

<% if not (node.get_partial_name == 'courses_folder_actions' && session[:user].role_id == 6) then %> 
<%= render :partial => '/tree_display/actions/' + node.get_partial_name, :locals => {:node => node} %> 
<% end %>

3. Refactor an if-else statement using the ternary operator in _folder.html.erb.
Existing code snippet:

<% if action_partial %>
  <%= render :partial=> '/tree_display/actions/'+action_partial+'_folder_actions' %>
<% else %>
  <%= render :partial=> '/tree_display/actions/no_actions' %>
<% end %>

Code snippet after refactoring _folder.html.erb:

<% actions_url = '/tree_display/actions/' %>
<% partial_url = (action_partial) ? action_partial + '_folder_actions' : 'no_actions' %>
<%= render :partial=> actions_url + partial_url %>
<%= render :partial=> 'row_footer', :locals => {:depth => 0, :prefix => prefix, :nodes => nodes} %>

4. Refactor an if-else statement using the ternary operator in _listing.html.erb.
Existing code snippet:

<% if node.get_partial_name %>
  <%= render :partial => '/tree_display/actions/'+node.get_partial_name,
    :locals => {:id => node.node_object_id} %>
<% else %>
  <%= render :partial=> '/tree_display/actions/no_actions' %>
<% end %>

Code snippet after refactoring _listing.html.erb:

<% node.get_partial_name ? %>
  <%= render :partial => '/tree_display/actions/'+node.get_partial_name,
    :locals => {:id => node.node_object_id} %>
<% : <%= render :partial=> '/tree_display/actions/no_actions' %>

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

Pre-conditions:

1. You must be logged in using the previously provided credentials.
2. You must have a course with participants. To make a new course and add its participants, follow the instructions below.
   1. Upon login, you will see a list of courses saved to this instructor's account.
      1. If you do not see this list, hover over Manage... in the navbar.
      2. Click on Courses.
   2. To create a new course, click on the white and blue plus button to the top right of the Courses table.
   3. Select an institution, enter a course name, enter a course directory, enter any course information, select a default language, and uncheck Private course.
   4. Press the Create button.
   5. You will return to the list of courses. Find the course you just created, and click the Add participants icon, which is an image of a person in a blue shirt with a green plus sign.
   6. Enter the login name for a student you want to add as a participant to the course into the text field, and then press Add. Add as many students as you wish.
   7. When you are finished, click Back to return to the Course list.
3. You must have an assignment associated with the course you made or chose to use in Pre-conditions Step #2. To make a new assignment and add its participants, follow the instructions below.
   1. Find the course in the table and click the Create Assignment button, which is an image of a paper with a green plus sign.
   2. Enter an Assignment name, select the Course you made or chose, enter a Description URL, and check the Has teams? checkbox.
   3. Click the Create button.
   4. Click the Save button.
   5. Click the link in the red flash message to add participants to the new assignment.
      1. If you don't see that flash message, you may hover over Manage... in the navbar and then select Assignments.
      2. Locate the assignment in the table and click the Add participants icon which is an image of a person in a blue shirt with a green plus sign.
   6. Click the link that says Copy participants from course.

Create Randomly-Chosen Teams:

1. Navigate to the Courses table, and locate the course you made or chose previously.
2. Click the Create teams icon, which is an image of three people with a green plus sign.
3. This is the Course Teams table.
4. Click the Create Team link.
5. Keep the default value of 2 in the Team Size select, and press Create Teams.
6. You should see some new teams populated in the Course Teams table.

Delete Individual Teams:

1. Navigate to the Course Teams table.
2. Click on any red X next to a team name to delete that team.
3. The page should refresh and show that the deleted team no longer exists.

Delete All Teams:

1. Navigate to the Course Teams table.
2. Click the Delete All Teams link to delete all teams associated with the course.
3. The page should refresh and show that there are no more teams associated with the course.

Manually Create a Team:

1. Navigate to the Course Teams table.
2. Click the Create Team link.
3. Enter a Team Name into the text field and press Save'.
4. The Course Teams table should be shown with the new team present.
5. Click the green plus button to Add Team Member.
6. Enter the login name for a student you want to add to the team, and press Add.
7. You can confirm this addition but pressing the black and white plus button to the left of the team's table entry. The student should be shown as a team member.

Inherit Teams:

1. Ensure your course has at least one team.
2. Navigate to the assignment you chose or created previously.
3. Click the Create teams icon, which is an image of three people with a green plus sign.
4. This is the Assignment Teams table.
5. If the assignment already has teams, delete them.
6. Click the Create Team link.
7. Press the Inherit button.
8. You should now see the teams from the course displayed in the Assignment Teams table.

Bequeath All Teams:

1. Ensure your course has no teams.
2. Navigate to the assignment you chose or created previously.
3. Ensure your assignment has at least one team.
4. Click Bequeath All Teams.
5. Navigate to your course's Course Teams table.
6. You should now see the teams from the assignment displayed in the Course Teams table.

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 conjunction from all the views they are referenced from.