CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb: Difference between revisions
(12 intermediate revisions by 3 users not shown) | |||
Line 1: | Line 1: | ||
=Introduction= | ==Introduction== | ||
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution. | In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution. | ||
=Issues Fixed= | ==Issues Fixed== | ||
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code> | # Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code> | ||
# Changed method name of <code>create_teams</code> to a better name: <code> | # Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code> | ||
# Found areas in the code where the DRY principle can be applied and did so | # Found areas in the code where the DRY principle can be applied and did so | ||
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated. | # The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated. | ||
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists | |||
# Removed all extra added gems | # Removed all extra added gems | ||
# Renamed variable signUps to signups because it's a noun | # Renamed variable signUps to signups because it's a noun | ||
#* Actually ended up removing this variable in the process of doing fix 4 above | |||
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method. | # Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method. | ||
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose | # Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose | ||
Line 15: | Line 16: | ||
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods | # Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods | ||
=Files Changed= | ==Files Changed== | ||
* app/controllers/teams_controller.rb | * app/controllers/teams_controller.rb | ||
* app/models/team.rb | * app/models/team.rb | ||
Line 25: | Line 26: | ||
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb | * spec/controllers/airbrake_exception_errors_controller_tests_spec.rb | ||
=Changes= | ==Changes== | ||
===Fix #2=== | |||
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function. This also needed to be updated in the rspec test <code>teams_controller_spec.rb</code>: | |||
<nowiki> describe 'create teams method' do | |||
context 'when correct parameters are passed' do | |||
it 'creates teams with random names' do | |||
allow(ExpertizaLogger).to receive(:info).and_return(nil) | |||
... | |||
user_session = { user: instructor, team_type: 'Assignment' } | |||
result = get :randomize_teams, params: request_params, session: user_session | |||
# status code 302: Redirect url | |||
... | |||
end | |||
end | |||
end</nowiki> | |||
===Fix #3=== | |||
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params</code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model. | |||
<nowiki> | |||
# Gets the model representing the parent of the team | |||
def team_type | |||
if session[:team_type] == 'Assignment' | |||
Assignment | |||
elsif session[:team_type] == 'Course' | |||
Course | |||
end | |||
end | |||
# Finds the object containing the students | |||
# which the team will be generated from | |||
def team_parent | |||
@team_parent ||= team_type.find(params[:id]) | |||
end</nowiki> | |||
===Fixes #4 and #6=== | |||
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>. | |||
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic | |||
# # if there is another team in waitlist, make this team hold this topic | |||
# topic_id = @signed_up_team.first.topic_id | |||
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first | |||
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team | |||
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team | |||
# end</nowiki> | |||
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well. | |||
The change to being able to delete from the waitlist needed to be accounted for in the <code>teams_controller_spec.rb</code> rspec test, which can be seen here: | |||
<nowiki>context 'when called and team is not nil and it does not hold a topic' do | |||
it 'deletes the team' do | |||
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil) | |||
... | |||
end | |||
end</nowiki> | |||
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed: | |||
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do | |||
... | |||
controller.session[:team_type] = 'Assignment' | |||
allow(SignedUpTeam).to receive(:destroy).and_return(nil) | |||
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil) | |||
allow(Team).to receive(:find).with(any_args).and_return(team) | |||
allow(Team).to receive(:find_by).with(any_args).and_return(team) | |||
... | |||
end</nowiki> | |||
===Fixes #7 and #8=== | |||
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added.<br> | One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added.<br> | ||
< | <nowiki># E2254 : This method copies a list of teams to another collection. It is mainly | ||
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying | |||
# teams from assignments to courses and vice versa. | |||
def self.copy_teams_to_collection(teams, recipient_collection_id) | |||
teams.each do |team| | |||
team.copy(recipient_collection_id) | |||
end | |||
end</nowiki> | |||
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods. | This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods. | ||
< | <nowiki>def copy_to_assignment | ||
... | |||
if teams.empty? | |||
flash[:note] = 'No teams were found when trying to copy to assignment.' | |||
else | |||
Team.copy_teams_to_collection(teams, assignment.id) | |||
end | |||
... | |||
end</ | end</nowiki> | ||
< | <nowiki>def bequeath_all | ||
... | |||
teams = assignment.teams | |||
Team.copy_teams_to_collection(teams, course.id) | |||
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"' | |||
... | |||
end</nowiki> | |||
===Fix #9=== | |||
Refactored bequeath_all to get rid of all the nested if statements | |||
<nowiki> # Copies existing teams from an assignment to a | |||
# course if the course doesn't already have teams | |||
def bequeath_all | |||
assignment = Assignment.find(params[:id]) | |||
course = Course.find(assignment.course_id) if assignment.course_id | |||
if !assignment.course_id | |||
flash[:error] = 'No course was found for this assignment.' | |||
elsif course.course_teams.any? | |||
flash[:error] = 'The course already has associated teams' | |||
else | |||
teams = assignment.teams | teams = assignment.teams | ||
Team.copy_teams_to_collection(teams, course.id) | Team.copy_teams_to_collection(teams, course.id) | ||
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"' | flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"' | ||
end | |||
redirect_to controller: 'teams', action: 'list', id: assignment.id | |||
end</nowiki> | |||
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure | |||
a course team was being used was moved to it's own method: | |||
<nowiki> # Redirects if the team is not a CourseTeam | |||
def ensure_course_team | |||
if session[:team_type] == 'Course' | |||
flash[:error] = 'Invalid team type for bequeathal' | |||
redirect_to controller: 'teams', action: 'list', id: params[:id] | |||
end | |||
end</nowiki> | |||
and specified to run at before the <code>bequeath_all</code> method at the start of the file: | |||
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki> | |||
==Test Plan== | |||
===Test Coverage=== | |||
Rspec test coverage of <code>app/controllers/teams_controller.rb</code> fromt the main branch of expertiza: | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage before refactoring.png]] | |||
Rspec test coverage of <code>app/controllers/teams_controller.rb</code> after our refactoring: | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage after refactoring.png]] | |||
===Randomize Teams=== | |||
Given you are logged in as an instructor | |||
* Username: ''instructor6'' Password ''password'' | |||
And you are on the Manage Assignments page | |||
[[File:E2254_AssignmentsPage.png]] | |||
When you click on create teams for an assignment with participants | |||
[[File:E2254_CreateTeams.png]] | |||
And you click "Create Team" | |||
[[File:E2254_Toolbar.png]] | |||
And enter appropriate information | |||
Then teams should be randomized and created | |||
===Bequeath all=== | |||
Given a team has been created for an assignment | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png]] | |||
And I am on the create teams page for an assignment | |||
When I click "Bequeath All Teams" | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png]] | |||
Then I should be able to move the team from an assignment to a course | |||
Course teams before bequeath all: | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png]] | |||
Course teams after bequeath all: | |||
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png]] |
Latest revision as of 23:26, 31 October 2022
Introduction
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.
Issues Fixed
- Added more comments and briefly explained the functionality of custom methods like
action_allowed
, andlist
- Changed method name of
create_teams
to a better name:randomize_teams
- Found areas in the code where the DRY principle can be applied and did so
- The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
- Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
- Removed all extra added gems
- Renamed variable signUps to signups because it's a noun
- Actually ended up removing this variable in the process of doing fix 4 above
- Moved list copying code from the
inherit
method to be a class method of the Team model:copy_teams_to_collection
. Also replaced identical code in bequeath_all to also use this class method. - Changed the name of the
inherit
method tocopy_to_assignment
so the name is more descriptive of it's purpose - Removed nested if statments and removed other return points from bequeath_all
- Removed unnecessary comments for methods like
update
and added meaningful comments to other methods
Files Changed
- app/controllers/teams_controller.rb
- app/models/team.rb
- app/views/teams/new.html.erb
- config/locales/en_US.yml
- config/locales/en_IN.yml
- config/routes.rb
- spec/controllers/teams_controller_spec.rb
- spec/controllers/airbrake_exception_errors_controller_tests_spec.rb
Changes
Fix #2
The create_teams
method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to randomize_teams
and a comment was added briefly describing the function. This also needed to be updated in the rspec test teams_controller_spec.rb
:
describe 'create teams method' do context 'when correct parameters are passed' do it 'creates teams with random names' do allow(ExpertizaLogger).to receive(:info).and_return(nil) ... user_session = { user: instructor, team_type: 'Assignment' } result = get :randomize_teams, params: request_params, session: user_session # status code 302: Redirect url ... end end end
Fix #3
Several places used a complicated line to find the parent of a team. The parent is either a Course
or an Assignment
. The type of parent is stored in the session
and the id
of the parent in the params
. We created two new private methods, team_type
and team_parent
to DRY out the code. The team_type
method returns either Course
or Assignment
and the team_parent
method returns the the actual parent rather than the model.
# Gets the model representing the parent of the team def team_type if session[:team_type] == 'Assignment' Assignment elsif session[:team_type] == 'Course' Course end end # Finds the object containing the students # which the team will be generated from def team_parent @team_parent ||= team_type.find(params[:id]) end
Fixes #4 and #6
Removed the following if statement editing a waitlist in a controller and replaced it with Waitlist.remove_from_waitlists(@team.id)
.
# if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic # # if there is another team in waitlist, make this team hold this topic # topic_id = @signed_up_team.first.topic_id # next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first # # if slot exist, then confirm the topic for this team and delete all waitlists for this team # SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team # end
In the process of making this change, the unused @signUps
variable is removed, taking care of Fix #6 as well.
The change to being able to delete from the waitlist needed to be accounted for in the teams_controller_spec.rb
rspec test, which can be seen here:
context 'when called and team is not nil and it does not hold a topic' do it 'deletes the team' do allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil) ... end end
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in airbrake_exception_errors_controller_tests_spec.rb
. To fix this, we allowed functionality to account for what was changed:
it 'will delete the team if current team did not involve in any other reviews' do ... controller.session[:team_type] = 'Assignment' allow(SignedUpTeam).to receive(:destroy).and_return(nil) allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil) allow(Team).to receive(:find).with(any_args).and_return(team) allow(Team).to receive(:find_by).with(any_args).and_return(team) ... end
Fixes #7 and #8
One of the main changes made to implement this was in Team.rb
where the class method copy_teams_to_collection
was added.
# E2254 : This method copies a list of teams to another collection. It is mainly # used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying # teams from assignments to courses and vice versa. def self.copy_teams_to_collection(teams, recipient_collection_id) teams.each do |team| team.copy(recipient_collection_id) end end
This method was called in teams_controller.rb
in the copy_to_assignment
(method used to be named inherit
) and bequeath_all
methods.
def copy_to_assignment ... if teams.empty? flash[:note] = 'No teams were found when trying to copy to assignment.' else Team.copy_teams_to_collection(teams, assignment.id) end ... end def bequeath_all ... teams = assignment.teams Team.copy_teams_to_collection(teams, course.id) flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"' ... end
Fix #9
Refactored bequeath_all to get rid of all the nested if statements
# Copies existing teams from an assignment to a # course if the course doesn't already have teams def bequeath_all assignment = Assignment.find(params[:id]) course = Course.find(assignment.course_id) if assignment.course_id if !assignment.course_id flash[:error] = 'No course was found for this assignment.' elsif course.course_teams.any? flash[:error] = 'The course already has associated teams' else teams = assignment.teams Team.copy_teams_to_collection(teams, course.id) flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"' end redirect_to controller: 'teams', action: 'list', id: assignment.id end
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure a course team was being used was moved to it's own method:
# Redirects if the team is not a CourseTeam def ensure_course_team if session[:team_type] == 'Course' flash[:error] = 'Invalid team type for bequeathal' redirect_to controller: 'teams', action: 'list', id: params[:id] end end
and specified to run at before the bequeath_all
method at the start of the file:
before_action :ensure_course_team, only: %i[bequeath_all]
Test Plan
Test Coverage
Rspec test coverage of app/controllers/teams_controller.rb
fromt the main branch of expertiza:
Rspec test coverage of app/controllers/teams_controller.rb
after our refactoring:
Randomize Teams
Given you are logged in as an instructor
- Username: instructor6 Password password
And you are on the Manage Assignments page
When you click on create teams for an assignment with participants
And you click "Create Team"
And enter appropriate information
Then teams should be randomized and created
Bequeath all
Given a team has been created for an assignment
And I am on the create teams page for an assignment
When I click "Bequeath All Teams"
Then I should be able to move the team from an assignment to a course