Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-28T01:42:41Z

Cmehta: /* Test Plan */

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

__TOC__
==Introduction==
Rounds of peer reviews may be implemented between submissions for assignments on Expertiza. In order to better track the implementation of reviewer's suggestions, a Revision Planning Tool should be implemented.

===Problem Statement===
In the first round of Expertiza reviews, we ask reviewers to give authors some guidance on how to improve their work. Then in the second round, reviewers rate how well authors have followed their suggestions. We could carry the interaction one step further if we asked authors to make up a revision plan based on the first-round reviews. That is, authors would say what they were planning to do to improve their work. Then second-round reviewers would assess how well they did it. In essence, this means that authors would be adding criteria to the second-round rubric that applied only to their submission. We are interested in having this implemented and used in a class so that we can study its effect.

===Previous Implementations===
Revision planning has been implemented twice before, once in [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875] and once in [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016]. While the functionality worked and effectively minimized changes to the code, they also had the following problems:
*Hardcoded “round” numbers in many places of the code.
*Documentation does not reflect the new changes they made.
*Revision planning responses and responses to the other items are not distinguished in heatgrid view.
*The idea of adding a <code>team_id</code> field to each question is intuitive. However, they failed to come up with a clean implementation of this idea. Specifically, they had passed some trailing parameters several methods down before reaching the place that needs them.

===Rationale===
To implement revision planning, [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016] added <code>EnableRevisionPlan</code> (boolean) to <code>Assignment</code> and <code>TeamID</code> to <code>Question</code>. Adding a boolean to enable revision planning for an assignment makes sense, but having certain questions belong to a team is not a clean implementation. Instead, we will create a new questionnaire that will belong to each team and keep track of the set of questions. We will use this new questionnaire to separate the revision plan responses and responses to other items in heatgrid view by creating a different table for each.

==Design==
===Database Design===
Items in green are additions.
[[File:E2083_Proposed_Design.png|1000px|center]]
 
In the assignment table we will add <code>is_revision_enabled?</code> column to indicate whether the assignment accepts a revision plan along with review rubric.

<code>RevisionPlanQuestionnaire</code> maps a questionnaire to an assignment team and round. This will map to a questionnaire that will be created by the reviewee.

===Proposed Changes===
# A team would be allowed to create a <code>RevisionPlanQuestionnaire</code> after completion of round 1 of review. This revision plan questionnaire would be linked to the next round and to the team. Team would be able to add questions to this revision plan questionnaire.
# Reviewer would be displayed questions of both the original assignment review rubric and reviewee created revision plan. To achieve this we will create a <code>CompositeQuestionnaire</code> by combining the two questionnaires. This composite questionnaire would not have any revision plan or review rubric based code.
#* Composite questionnaire will be created in the following:
## When revision plan is enabled and a questionnaire is requested from <code>ReviewResponseMap</code>.
## When revision plan is enabled and questionnaire is requested from response.
#* Composite questionnaire would have a function <code>questions</code> to return questions of contained questionnaires.
# The Response currently displays section headings using <code>SectionHeader</code>. SectionHeader is a child class of <code>QuestionnaireHeader</code> which itself derives from <code>Question</code>. A new <code>QuestionnaireNameHeader</code> class which derives from QuestionnaireHeader will be created to display name of Questionnaire (Improvement Plan/Assignment) in a way which is similar to how we show section headings in UI.

===User Interface===
====Enable Revision Planning====
In order to enable '''Revision Planning''', the setting must be enabled when creating or editing an assignment under the '''General''' tab. The wireframe below demonstrates creating an assignment, and editing the assignment functions similarly. '''Improvement plan?''' should be checked to enable this option.

[[File:Enable_Revision_Planning_Wireframe.png|700px|thumb|center|Wireframe of Enabling Assignment's Revision Planning]]

====Assignment Overview Page (contains the Link to the revision planning page)====
The '''Revision Planning''' link is available to students during every submission period (except the first round submission) and not available during every review period. As shown in the wireframe, by clicking '''Revision Planning''' students would be redirected to a page explained under the ‘Revision planning page’ subsection.

[[File:Link_to_the_revision_planning.jpeg|600px|thumb|center|Wireframe of Assignment Overview Page]]

====Editing the Revision Plan Questionnaire====
After creating the Revision Plan Questionnaire, it must be edited. Questions can be added by specifying the amount of questions and their type. Questions can be removed by clicking '''Remove''' in the leftmost column. Once the questionnaire is complete, it can be saved. This page will be visible during each submission period after the first and will be unavailable during all review periods.

[[File:Edit_Revision_Plan_Wireframe.png|600px|thumb|center|Wireframe of Editing an Assignment's Revision Plan]]

====Summary Report Page====
When a project has been reviewed at least once, a participant will be able to view their team's score. The wireframe below shows what this will look like after the second round of reviews. For the second and all subsequent reviews, the results of questions that were created by the instructor will be shown under '''Assignment Questionnaire.''' The results of the questions created by the team will be shown under '''Improvement Plan.'''

[[File:View_Scores_Wireframe.png|400px|thumb|center|Wireframe of Summary Report for an Assignment]]

===Control Flow Diagram===
[[File:Revision Planning Control Flow Diagram.jpeg|400px|thumb|center|Control Flow of the Revision Planning Function]]

===Test Plan===

====RSpec Testing====
The RSpec tests would test both controller and models.

Controllers
* spec/controllers/questionnaires_controller_spec.rb: test revision plan questionnaire can be created and edited.
* spec/controllers/assignments_controller_spec.rb: create and validate an assignment with revision planning enabled.

Models
Test validations and logic present in revision plan and composite questionnaire models.
* spec/models/revision_plan_questionnaire_spec.rb
* spec/models/composite_questionnaire_spec.rb

====Manual Testing====
Manual testing will aim to verify the following:
* Create an assignment with revision planning enabled.
* Are participants allowed to create/edit revision plan when round 1+ (1 or greater than 1) reviews have finished.
* Is revision plan editing disabled when assignment is in review stage.
* Are reviewers shown questions created by reviewees.
* Are participants shown summary of score for revision plan after review deadline has expired.

==Team Members==
Chaitanya Mehta (cmehta)
 
Darby Madewell (demadewe)
 
Dongni Yang (dyang23)
 
Sidharth Mehta (smehta22)
 
'''Mentor:''' Yulin Zhang (yzhan114)

==References==
# Previous Implementation: [https://expertiza.csc.ncsu.edu/index.php/E1875_Revision_Planning_Tool E1875 wiki], [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2020_E2016_Revision_planning_tool E2016 wiki]
# Forked Repository: [https://github.com/SidharthMehta/expertiza/tree/beta E2083 github]

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-23T21:06:44Z

Cmehta: /* Database Design */

CSC/ECE 517 Fall 2020 - E2083. Revision planning tool E2016

2020-10-21T01:46:03Z

Cmehta: /* Database Design */

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

2020-10-12T22:01:26Z

Cmehta: /* Test Plan */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
Expertiza already had well written test cases for lottery controller. While moving methods to model some of these test cases were moved as well. After refactoring the controller, it was tested manually and using previously written RSpec tests to ensure that no bugs were introduced.

'''Note:''' Although there is a drop of 0.004% in coverage all the code that belongs to lottery_controller and which was moved to models has 100% test coverage. Overall coverage might have reduced because total lines have reduced.

===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
 Sidharth Mehta (smehta22)
 '''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T22:00:03Z

Cmehta: /* Test Plan */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
Expertiza already had well written test cases for lottery controller. While moving methods to model some of these test cases were moved as well. After refactoring the controller, it was tested manually and using previously written RSpec tests to ensure that no bugs were introduced.
Note: Although there is a drop of 0.004% in coverage all the code that belongs to lottery_controller and which was moved to models has 100% test coverage. Overall coverage might have reduced because total lines have reduced.

===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
 Sidharth Mehta (smehta22)
 '''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T21:03:30Z

Cmehta: /* Test Plan */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
Expertiza already had well written test cases for lottery controller. While moving methods to model some of these test cases were moved as well. After refactoring the controller, it was tested manually and using previously written RSpec tests to ensure that no bugs were introduced.

===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
 Sidharth Mehta (smehta22)
 '''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T21:01:17Z

Cmehta: /* Test Plan */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
Expertiza already has well written test cases for lottery controller. While moving methods to model some of these test cases were moved as well. After making changes the application was tested manually and using previously written RSpec test to ensure that no bugs were introduced.

===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
 Sidharth Mehta (smehta22)
 '''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T20:52:59Z

Cmehta: /* Team Members */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
 Sidharth Mehta (smehta22)
 '''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T20:45:21Z

Cmehta: /* Testing Changes */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 view code changes]

==Test Plan==
===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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
Sidharth Mehta (smehta22)
'''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T20:44:54Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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

==Team Members==
Chaitanya Mehta (cmehta)
Sidharth Mehta (smehta22)
'''Mentor:''' Sahil Papalkar (spapalk)

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T20:38:18Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T00:37:41Z

Cmehta: /* UI Testing */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics, then a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T00:36:03Z

Cmehta: /* UI Testing */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:'''
# If there are no non signed up teams that have bids for topics. a 500 Internal Server Error is displayed.
# When intelligent assignment finishes it disables bidding on topics, bidding on topics needs to be enabled to run intelligent assignment.

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

'''Steps to signup students for topics:'''
* Impersonate a participant.
* Select assignment with topics. (for example lottery assignment)
* Open '''Signup sheet'''.
* Drag and drop topics under selection section.

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T00:12:18Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click on the orange '''Intelligent Assignment''' button.
'''Note:''' If there are no teams that have bids for topics a 500 Internal Server Error is displayed.

'''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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T00:11:50Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click the orange '''Intelligent Assignment''' button.
'''Note:''' If there are no teams that have bids for topics a 500 Internal Server Error is displayed.

'''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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-12T00:11:00Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
* Scroll till you reach assignment named '''lottery'''. [[File:E2066_run_intelligent_assignment.png|600px]]
* Click the orange '''Intelligent Assignment''' button.
'''Note:''' If there is no team that has bids for topics a 500 Internal Server Error is displayed.

'''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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

File:E2066 run intelligent assignment.png

2020-10-11T23:52:53Z

Cmehta:

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

2020-10-11T23:51:18Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ 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'''.
[[File:Example.jpg]]

'''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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-11T21:31:23Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 <code>remove_empty_teams</code> to assignment.rb.

[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e 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 <code>create_team_with_users</code> in file team.rb and invoke it from controller.

[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ Expertiza with E2066]

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-11T18:42:09Z

Cmehta: /* RSpec */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 ===
[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e code changes]

=== Problem 5: Creation of Team with Team Members should be handled by Team.rb ===
[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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===
[http://152.46.19.69:8080/ Expertiza with E2066]

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-11T18:40:20Z

Cmehta: /* Files Modified */

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 ===
[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e code changes]

=== Problem 5: Creation of Team with Team Members should be handled by Team.rb ===
[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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_team_spec.rb -e 'create team with users'

===UI Testing===
[http://152.46.19.69:8080/ Expertiza with E2066]

==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==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-11T18:38:29Z

Cmehta:

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

__TOC__

==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 <code>create_new_teams_for_bidding_response</code> 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 <code>merge_bids_from_different_previous_teams</code> 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 <code>AssignmentTeam.create_team_and_node</code>. 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 <code>add_member(user)</code>, 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 <code>AssignmentTeam.create_team_and_node</code>.
#After
new_team = AssignmentTeam.create_team_and_node(assignment.id)

'''Part B'''
Use <code>add_member(user)</code>.
#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 <code>remove_user_from_previous_team</code> explicitly deletes team_user_node before team_user is deleted. TeamUser contains code <code>has_one :team_user_node, foreign_key: 'node_object_id', dependent: :destroy</code> 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 <code>has_one :team_node, foreign_key: :node_object_id, dependent: :destroy</code> 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 ===
[https://github.com/expertiza/expertiza/pull/1761/commits/c61850b1d05d7ad3b44c2ba7e1fac4f4c516922e code changes]

=== Problem 5: Creation of Team with Team Members should be handled by Team.rb ===
[https://github.com/expertiza/expertiza/pull/1761/commits/be38d9ff56053c9315c2bf57aeea4a00f574918c 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 <code>merge_bids_from_different_users</code> which accepts team id, signup topics and bids of users, and contains logic to create bids and get priorities for new bids.

[https://github.com/expertiza/expertiza/pull/1761/commits/8d6973c1f1d3c37e31cd46c34fe316d1b38b43c2 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_team_spec.rb -e 'create team with users'

===UI Testing===
[http://152.46.19.69:8080/ Expertiza with E2066]

==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/assignment_team_spec.rb

==References==
#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/chaitanyamehta/expertiza E2066 Repository Fork]
#[https://github.com/expertiza/expertiza/pull/1761 E2066 Pull Request]
#[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg/videos Expertiza YouTube Channel]
#[https://www.youtube.com/watch?v=sg8n-oEIjdE YouTube tutorial for assignment with topics]

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

2020-10-11T04:08:03Z

Cmehta:

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

2020-10-11T03:43:36Z

Cmehta:

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

2020-10-11T03:35:57Z

Cmehta:

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

2020-10-11T02:43:30Z

Cmehta:

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

2020-10-11T00:59:35Z

Cmehta:

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

2020-10-11T00:51:23Z

Cmehta:

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

2020-10-11T00:44:04Z

Cmehta:

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

2020-10-11T00:39:54Z

Cmehta:

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

2020-10-11T00:07:19Z

Cmehta:

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

__TOC__

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

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

2020-10-09T22:46:15Z

Cmehta:

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

__TOC__

CSC/ECE 517 Fall 2020

2020-10-09T22:45:19Z

Cmehta:

* [[Dummy URL]]
* [[Dummy Url2]]
* [[E2068]]
* [[CSC/ECE 517 Fall 2020 - E2062. Add test cases to review_mapping_helper.rb]]
* [[CSC/ECE 517 Fall 2020 - E2066. Refactor lottery controller.rb]]

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

2020-10-09T22:41:21Z

Cmehta: Created page with "==CSC/ECE 517 Fall 2019 - E2066. Refactor lottery controller.rb== This page provides a description of the Expertiza based OSS project. __TOC__"

==CSC/ECE 517 Fall 2019 - E2066. Refactor lottery controller.rb==
This page provides a description of the Expertiza based OSS project.

__TOC__