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
- Fix bug which created multiple bids with same topic, team and priority.
- Call existing AssignmentTeam.rb method to create team and team node, instead of creating them separately. (DRY principle)
- Call existing Team.rb method to add members in team.
- Remove code that explicitly deletes dependents before deleting object and rely on dependent destroy instead.
- Move logic to delete empty teams belonging to an assignment into assignment.rb.
- Move logic for creation of team with members to Team.rb.
- 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 5: Creation of Team with Team Members should be handled by Team.rb
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.
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
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