CSC/ECE 517 Fall 2021 - E2127. Refactor teams controller
E2127: Refactor 'Teams_Controller' Controller
This page provides a brief description of the Expertiza project. The project is aimed at refactoring the 'Teams_Controller' Controller, which contains the methods to create new teams, list the existing teams, create new teams by inheriting existing teams, deleting existing teams and creating multiple teams at once by assigning them random topics. The project entailed, refactoring some part of the controller to improve the readability of code and then creating test cases in RSpec to verify the methods it possesses.
Introduction to Expertiza
Expertiza is a peer review based system which provides incremental learning from the class. This project has been developed together by faculty and students using Ruby on Rails framework. Expertiza allows the instructor to create, edit and delete assignments, create new assignment topics, assign them to a particular class or selected students, have students work on teams and then review each other's assignments at the end. For the students, they can signup for topics, form teams, and submit their projects and assignments. Students then review the work done by other students and give suggestions to improve. Teams after reviews are allotted scores and they can refer to the peer comments to further improve their work. It also supports submission of different file types for assignments, including the URLs and wiki pages.
Project Description
Teams_Controller primarily handles the team creation, deletion and other behaviours. Most of those are called from the instructor’s view. When manual testing is done, most of the methods can be called by clicking the “Create teams” icon from both assignments and courses. The following tasks have been performed as per the requirements.
Create Method
The method Create is called when an instructor tries to create a team manually. This works for both, creating assignment teams and course teams. Assignment teams are teams made to do a particular assignment together and Course Teams are teams which are made for the whole course.
Delete Method
The method Delete is called when an instructor tries to delete a team manually. This works for both, deleting assignment teams and course teams.
List Method
The method List lists all the team nodes and the instructor is able to expand each team node to see the user nodes as well.
Inherit Method
The method Inherit inherits teams from course to assignments, but the “Inherit Teams From Course” option should not display when either 1) we are editing a course object or 2) the current assignment object does not have a course associated with it.
Running Tests
rspec spec/controllers/teams_controller_spec.rb
Files Modified
1. teams_controller.rb 2. team.rb 3. signed_up_team.rb 4. routes.rb 5. app/views/teams/new.html.erb
Refactoring
Refactoring is restructuring of code without the need of changing any external behavior. It reduces complexity and improves readability. It also becomes easy to extend the application with respect to different modules and their functionalities. Some common techniques to refactor are:
- Moving methods to appropriate modules
- Breaking methods into more meaningful functionality
- Creating more generalized code.
- Renaming methods and variable.
- Inheritance
Tasks
Refactoring delete method
Description: The delete method manipulates a waitlist. This code is moved to signed_up_team.rb model class.
Initially, the code was:
def delete
# delete records in team, teams_users, signed_up_teams table
@team = Team.find_by(id: params[:id])
unless @team.nil?
course = Object.const_get(session[:team_type]).find(@team.parent_id)
@signed_up_team = SignedUpTeam.where(team_id: @team.id)
@teams_users = TeamsUser.where(team_id: @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
@sign_up_team.destroy_all if @sign_up_team
@teams_users.destroy_all if @teams_users
@team.destroy if @team
undo_link("The team \"#{@team.name}\" has been successfully deleted.")
end
redirect_to :back
end
Which was changed to:
def delete
# delete records in team, teams_users, signed_up_teams table
@team = Team.find_by(id: params[:id])
unless @team.nil?
course = Object.const_get(session[:team_type]).find(@team.parent_id)
@signed_up_team = SignedUpTeam.where(team_id: @team.id)
@teams_users = TeamsUser.where(team_id: @team.id)
# On team deletion topic team was holding will be assigned to first team in waitlist.
SignedUpTeam.assign_topic_to_first_in_waitlist_post_team_deletion(@signed_up_team, @signups)
@sign_up_team.destroy_all if @sign_up_team
@teams_users.destroy_all if @teams_users
@team.destroy if @team
undo_link("The team \"#{@team.name}\" has been successfully deleted.")
end
redirect_to :back
end
and a new method was introduced in signed_up_team.rb model:
# this method checks when a team is deleted if there is a team in waitlist for the topic
# deleted team was holding, then assign topic to first team in waitlist
def self.assign_topic_to_first_in_waitlist_post_team_deletion (signed_up_team, signups)
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
end
Refactoring inherit method
Description: The code that copies a list is moved to team.rb model class.
Initially, the code was:
def inherit
assignment = Assignment.find(params[:id])
if assignment.course_id >= 0
course = Course.find(assignment.course_id)
teams = course.get_teams
unless teams.empty?
teams.each do |team|
team.copy(assignment.id)
end
else
flash[:note] = "No teams were found when trying to inherit."
end
else
flash[:error] = "No course was found for this assignment."
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end
Which was changed to:
def inherit
assignment = Assignment.find(params[:id])
if assignment.course_id >= 0
course = Course.find(assignment.course_id)
teams = course.get_teams
unless teams.empty?
# copy_assignment method copies teams to assignment
Team.copy_assignment(teams,assignment)
else
flash[:note] = "No teams were found when trying to inherit."
end
else
flash[:error] = "No course was found for this assignment."
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end
and a new method was introduced in team.rb model:
def self.copy_assignment(teams,assignment)
teams.each do |team|
team.copy(assignment.id)
end
end
Refactoring create method
Description: It creates new teams against a parent id
Initially the create method was:
def create
parent = Object.const_get(session[:team_type]).find(params[:id])
begin
Team.check_for_existing(parent, params[:team][:name], session[:team_type])
@team = Object.const_get(session[:team_type] + 'Team').create(name: params[:team][:name], parent_id: parent.id)
TeamNode.create(parent_id: parent.id, node_object_id: @team.id)
undo_link("The team \"#{@team.name}\" has been successfully created.")
redirect_to action: 'list', id: parent.id
rescue TeamExistsError
flash[:error] = $ERROR_INFO
redirect_to action: 'new', id: parent.id
end
end
Which changed to:
def create
begin
parent = get_parent_and_check_if_exists(params[:id])
@team = Object.const_get(session[:team_type] + 'Team').create(name: params[:team][:name], parent_id: parent.id)
TeamNode.create(parent_id: parent.id, node_object_id: @team.id)
undo_link("The team \"#{@team.name}\" has been successfully created.")
redirect_to action: 'list', id: parent.id
rescue TeamExistsError
flash_and_redirect_on_update_or_create_error('new', parent_id)
end
end
# called to fetch parent and check if team with same name and type already exists. def get_parent_and_check_if_exists(parent_id) parent = Object.const_get(session[:team_type]).find(parent_id) Team.check_for_existing(parent, params[:team][:name], session[:team_type]) return parent end
# to flash and redirect the user when there is any update or create error def flash_and_redirect_on_update_or_create_error(action, id) flash[:error] = $ERROR_INFO redirect_to action: action, id: id end
Refactoring update method
Description: It updates an existing team name
Initially the update method was:
def update
@team = Team.find(params[:id])
parent = Object.const_get(session[:team_type]).find(@team.parent_id)
begin
Team.check_for_existing(parent, params[:team][:name], session[:team_type])
@team.name = params[:team][:name]
@team.save
flash[:success] = "The team \"#{@team.name}\" has been successfully updated."
undo_link("")
redirect_to action: 'list', id: parent.id
rescue TeamExistsError
flash[:error] = $ERROR_INFO
redirect_to action: 'edit', id: @team.id
end
end
Which changed to:
# It updates an existing team name
def update
@team = Team.find(params[:id])
begin
parent = get_parent_and_check_if_exists(@team.parent_id)
@team.name = params[:team][:name]
@team.save
flash[:success] = "The team \"#{@team.name}\" has been successfully updated."
undo_link("")
redirect_to action: 'list', id: parent.id
rescue TeamExistsError
flash_and_redirect_on_update_or_create_error('edit', @team.id)
end
end
# called to fetch parent and check if team with same name and type already exists. def get_parent_and_check_if_exists(parent_id) parent = Object.const_get(session[:team_type]).find(parent_id) Team.check_for_existing(parent, params[:team][:name], session[:team_type]) return parent end
# to flash and redirect the user when there is any update or create error def flash_and_redirect_on_update_or_create_error(action, id) flash[:error] = $ERROR_INFO redirect_to action: action, id: id end
Create random teams
The new method self.create_teams() is created in Team.rb model and placed the common of create_teams method.
# This function is used to create teams with random names. # Instructors can call by clicking "Create temas" icon anc then click "Create teams" at the bottom. def self.create_teams(session,params) parent = Object.const_get(session[:team_type]).find(params[:id]) Team.randomize_all_by_parent(parent, session[:team_type], params[:team_size].to_i) end
Initially the exiting code of create_teams() method in teams_controller.rb
# This function is used to create teams with random names.
# Instructors can call by clicking "Create temas" icon anc then click "Create teams" at the bottom.
def create_teams
parent = Object.const_get(session[:team_type]).find(params[:id])
Team.randomize_all_by_parent(parent, session[:team_type], params[:team_size].to_i)
undo_link("Random teams have been successfully created.")
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Random teams have been successfully created', request)
redirect_to action: 'list', id: parent.id
end
Now modified the teams_controller.rb file and changed the function name to 'random_teams' and it is now using the Team.create_teams(session,params) present in Team.rb model to create teams.
# This function is used to create teams with random names.
# Instructors can call by clicking "Create teams" icon and then click "Create teams" at the bottom.
def random_teams
Team.create_teams(session,params)
undo_link("Random teams have been successfully created.")
ExpertizaLogger.info LoggerMessage.new(controller_name, , 'Random teams have been successfully created', request)
redirect_to action: 'list', id: parent.id
end
Also modified the form tag present in teams/new.html.erb
Initially:
<%= form_tag :action => 'create_teams' do %>
<% if @parent.is_a?(Course)%>
Team Size:<%= select_tag 'team_size', options_for_select(2..4), size: 1 %>
<% else %>
Team Size:<%= select_tag 'team_size', options_for_select(2..@parent.max_team_size), size: 1 %>
<% end %>
<%= hidden_field_tag 'id', @parent.id %>
<%= submit_tag "Create Teams" %>
<% end %>
Modified to:
<%= form_tag :action => 'random_teams' do %>
<% if @parent.is_a?(Course)%>
Team Size:<%= select_tag 'team_size', options_for_select(2..4), size: 1 %>
<% else %>
Team Size:<%= select_tag 'team_size', options_for_select(2..@parent.max_team_size), size: 1 %>
<% end %>
<%= hidden_field_tag 'id', @parent.id %>
<%= submit_tag "Create Teams" %>
<% end %>
signUps should be signups
Existing code for the signUps nouns are modified to signups.
Existing code teams_controller.rb
def delete
# delete records in team, teams_users, signed_up_teams table
@team = Team.find_by(id: params[:id])
unless @team.nil?
course = Object.const_get(session[:team_type]).find(@team.parent_id)
@signed_up_team = SignedUpTeam.where(team_id: @team.id)
@teams_users = TeamsUser.where(team_id: @team.id)
SignedUpTeam.assign_topic_to_first_in_waitlist_post_team_deletion(@signed_up_team, @signUps)
@sign_up_team.destroy_all if @sign_up_team
@teams_users.destroy_all if @teams_users
@team.destroy if @team
undo_link("The team \"#{@team.name}\" has been successfully deleted.")
end
redirect_to :back
end
Modified code in teams_controller.rb
def delete
# delete records in team, teams_users, signed_up_teams table
@team = Team.find_by(id: params[:id])
unless @team.nil?
course = Object.const_get(session[:team_type]).find(@team.parent_id)
@signed_up_team = SignedUpTeam.where(team_id: @team.id)
@teams_users = TeamsUser.where(team_id: @team.id)
SignedUpTeam.assign_topic_to_first_in_waitlist_post_team_deletion(@signed_up_team, @signups)
@sign_up_team.destroy_all if @sign_up_team
@teams_users.destroy_all if @teams_users
@team.destroy if @team
undo_link("The team \"#{@team.name}\" has been successfully deleted.")
end
redirect_to :back
end
Testing the Teams_Controller
Instructions for testing Rspecs
1. git clone https://github.com/sidhantarora/expertiza.git
2. Change to the expertiza directory and then perform "bundle install" and rake db:migrate.
3. Start the rails server
4. In a new terminal and in the expertiza directory, perform the following commands:
i. rspec spec/controllers/teams_controller_spec.rb
Testing Inherit Method
To test that while creating an assignment team, the inherit teams section should be displayed, we log in as instructor got to list page and click on Create Team Link. On this page, we test that it should contain the string 'Inherit Teams From Course'. To do this test manually, user need lo log in as an Instructor and navigate to assignments page for any course. There the user can click on 'Create Teams' icon and will be redirected to the Create teams page for an assignment, there the user can check for the 'inherit teams' button in the view, ideally, it should be there.
it 'should display inherit teams while creating an assignment team' do
create(:assignment)
create(:assignment_node)
create(:assignment_team)
login_as("instructor6")
visit '/teams/list?id=1&type=Assignment'
click_link 'Create Team'
expect(page).to have_content('Inherit Teams From Course')
end
To test that while creating a course team, the inherit teams section should not be displayed, we do same steps in the previous case but finally, we test that the page does NOT contain 'Inherit Teams from Course'. To do this test manually, user need lo log in as an Instructor and navigate to courses page. There the user can click on 'Create Teams' icon and will be redirected to 'create team' page, ideally, the 'Inherit Teams' button should not be displayed.
it 'should not display inherit teams while creating a course team' do
create(:course)
create(:course_node)
create(:course_team)
login_as("instructor6")
visit '/teams/list?id=1&type=Course'
click_link 'Create Team'
expect(page).to have_no_content('Inherit Teams From Course')
end
Similarly, for creating a team for an assignment without a course, we test that the page does not contain 'Inherit Teams From Course'. For this particular test case, we added a new factory object defined as an assignment with the course set to nil. This can not be tested manually because assignment cannot be created without a course parent.
it 'should not display inherit teams while creating team for an assignment without a course' do
create(:assignment_without_course)
create(:assignment_without_course_node)
create(:assignment_without_course_team)
login_as("instructor6")
visit '/teams/list?id=1&type=Assignment'
click_link 'Create Team'
expect(page).to have_no_content('Inherit Teams From Course')
end
Testing Create Method
This test checks that after creating an assignment team the count in the teams table increase by 1. This test cannot be performed manually as it is testing the count of a database table. But, the user can check this functionality online by logging in as an instructor, going to the assignment page of any course and clicking on 'create teams' icon. Following up with entering the required details and clicking on 'create team'.
describe "POST #create" do
context "with an assignment team" do
it "increases count by 1" do
expect{create :assignment_team, assignment: @assignment}.to change(Team,:count).by(1)
end
it "redirects to the list page" do
end
end
end
The same test an be applied to create a course team as well.
context "with a course team" do
it "increases the count by 1" do
expect{create :course_team, course: @course}.to change(Team,:count).by(1)
end
end
Testing Delete Method
The delete method should work for deleting both assignment and course teams. We check this functionality by deleting the respective team and then check if the count of Teams goes down by 1. The user can check this test manually by logging in as an instructor navigating to the page of any team for an assignment and clicking on 'Delete Team' icon.
context "with an assignment team " do
it "deletes an assignment team" do
@assignment = create(:assignment)
@a_team = create(:assignment_team)
expect{ @a_team.delete }.to change(Team, :count).by(-1)
end
end
Same Delete test has been applied to the course team The manual check can also be performed in a similar way by logging in as an instructor navigating to the page of any team for an assignment and clicking on 'Delete Team' icon.
context "with a course team " do
it "deletes a course team" do
@course = create(:course)
@c_team = create(:course_team)
expect{ @c_team.delete }.to change(Team, :count).by(-1)
end
end
Testing List Method
The list method lists all the team nodes. This test that we have written, checks whether the instructor is able to view the team nodes in the list view. To do this test manually user can log into the system as an instructor and click on any course, the course teams will be displayed. Also, the user can click on any team node to view information about that team.
describe 'List Team' do
it 'should list all team nodes' do
create(:assignment)
create(:assignment_node)
assignment_team = create(:assignment_team)
team_user = create(:team_user)
login_as("instructor6")
visit '/teams/list?id=1&type=Assignment'
page.all('#theTable tr').each do |tr|
expect(tr).to have_content?(assignment_team.name)
end
end
end