CSC/ECE 517 Fall 2020 - E2066. Refactor lottery controller.rb

This page provides a description of the Expertiza based OSS project.

Introduction

About Lottery Controller

The lottery controller assigns teams to topics based on the priorities the team gave to each signup topic during the bidding process. When the lottery controller is called to run its method run_intelligent_assignment, a web service is sent the bidding data from each team and returns a new list of teams, each of which is close to the maximum team size specified for the assignment. The web service coalesces teams that have similar bid data and then assigns those coalesced teams to topics, giving each team their top bid on a topic that hasn't been assigned yet. Teams with larger team sizes and more bids are assigned their topics first.

Motivation

This class contains logic to merge bids. It also contains code that creates teams, add and removes users from team. These methods are more suited to being model methods. The As much as possible, the controller should only contain standard crud actions.

Task Identified

1. Fix bug which created multiple bids with same topic, team and priority.
2. Call existing AssignmentTeam.rb method to create team and team node, instead of creating them separately. (DRY principle)
3. Call existing Team.rb method to add members in team.
4. Remove code that explicitly deletes dependents before deleting object and rely on dependent destroy instead.
5. Move logic to delete empty teams belonging to an assignment into assignment.rb.
6. Move logic for creation of team with members to Team.rb.
7. Move logic that merges bids of different users to Bid.rb

Implementation

Problem 1: Multiple bids created with same topic, team and priority

The function create_new_teams_for_bidding_response calls merge bids once for each team member. This creates multiple bids with same topic, team and priority.

   #before
   teams.each do |user_ids|
     ... (code omitted)
     user_ids.each do |user_id|
       ... (code omitted)
       merge_bids_from_different_previous_teams(assignment.sign_up_topics, new_team.id, user_ids, users_bidding_info)
     end
   end

Solution

Move function call to merge_bids_from_different_previous_teams in outer loop, so that its called once per team.

   #after
   teams.each do |user_ids|
     ... (code omitted)
     user_ids.each do |user_id|
       ... (code omitted)
     end
     merge_bids_from_different_previous_teams(assignment.sign_up_topics, new_team.id, user_ids, users_bidding_info)
   end

Problem 2: Code duplicated to create team with team nodes, add members in team

Part A The code to create an assignment team, name the team and then create a team node for the team is already handled by function AssignmentTeam.create_team_and_node. This has been duplicated in lottery_controller.rb

   #Before
   new_team = AssignmentTeam.create(name: 'Team_' + rand(10_000).to_s, parent_id: assignment.id)
   team_node = TeamNode.create(parent_id: assignment.id, node_object_id: new_team.id)

Part B AssignmentTeam also has a function add_member(user), which creates a TeamUser and TeamUserNode for the given user.

   #Before
   new_team_user = TeamsUser.create(user_id: user_id, team_id: new_team.id)
   TeamUserNode.create(parent_id: team_node.id, node_object_id: new_team_user.id)

Solution

Part A Use AssignmentTeam.create_team_and_node.

   #After
   new_team = AssignmentTeam.create_team_and_node(assignment.id)

Part B Use add_member(user).

   #After
   new_team.add_member(User.find(user_id))

Problem 3: Unnecessary code to explicitly delete dependents before parent is deleted

The parent objects being discussed here have association callbacks dependent destroy to correctly dispose dependents when it itself gets destroyed. We don't need to explicitly delete child objects before parent is deleted.

Solutions

Example 1

   #Before
   team_user = TeamsUser.where(user_id: user_id).find {|team_user| team_user.team.parent_id == assignment_id }
   team_user.team_user_node.destroy rescue nil
   team_user.destroy rescue nil

The function remove_user_from_previous_team explicitly deletes team_user_node before team_user is deleted. TeamUser contains code has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy to destroy related node when team user is destroyed.

   #After
   team_user = TeamsUser.where(user_id: user_id).find {|team_user| team_user.team.parent_id == assignment_id }
   team_user.destroy rescue nil

Example 2

   #Before
   if team.teams_users.empty?
     TeamNode.where(parent_id: assignment.id, node_object_id: team.id).destroy_all rescue nil
     team.destroy
   end

Team contains code has_one :team_node, foreign_key: :node_object_id, dependent: :destroy to destroy related node when team is destroyed.

   #After
   if team.teams_users.empty?
     team.destroy
   end

Problem 4: Handle deletion of empty teams related to an assignment in Assignment.rb

It seems more appropriate for class Assignment to know how to delete empty teams that belong it. If in future there is a new workflow that requires deletion of empty teams which belong to an assignment, this would help dry out code.

Solution

Move function remove_empty_teams to assignment.rb.

view code changes

Problem 5: Creation of Team with Team Members should be handled in Team.rb

Creation and Manipulation of Team are more suited to be model methods. Creation of team involves naming the team, creating nodes and addition of members involves creation of team user nodes. This logic should not be part of controller.

Solution

Create a new function create_team_with_users in file team.rb and invoke it from controller.

view code changes

Problem 6: Logic of merging and creating new bids should be handled by Bid.rb

Controller should not contain code to generate new bids by calculate priorities of topics using previous individual bids of users. This kind of complex logic is suited to being a model method in Bid class.

Solution

Create a class method merge_bids_from_different_users which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

view code changes

Testing Changes

RSpec

Test which belong to code that has been refactored have been refactored, most of the other test relating to lottery controller remain unchanged. The following commands can be run to test lottery controller and other functions that relate to E2066.

   rspec ./spec/controllers/lottery_controller_spec.rb
   rspec ./spec/models/bid_spec.rb
   rspec ./spec/models/assignment_spec.rb -e '#remove_empty_teams'
   rspec ./spec/models/assignment_team_spec.rb -e 'create team with users'

UI Testing

Expertiza with E2066

By following the steps below, you will be able to manually test Expertiza with E2066 fixes.

• Login as instructor. (Username: instructor6, Password: password)
• From the menu bar, hover on Manage and select Assignments.

Steps to create an assignment with topics:

• Create a new assignment with Has teams? selected.
• Go back, and then edit assignment by clicking on the pencil icon.
• Select Has topics?
• Go to Topics tab and add a few of them.
• Enable bidding for topics.
• Go to Other stuff tab, select Add Participants and then Copy participants from course.

Files Modified

• app/controllers/lottery_controller.rb
• app/models/assignment.rb
• app/models/bid.rb
• app/models/team.rb
• spec/controllers/lottery_controller_spec.rb
• spec/models/assignment_spec.rb
• spec/models/assignment_team_spec.rb
• spec/models/bid_spec.rb

References

1. Expertiza on GitHub
2. E2066 Repository Fork
3. E2066 Pull Request
4. Expertiza YouTube Channel
5. YouTube tutorial for assignment with topics