CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb
Introduction
Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.
About response_controller.rb
The file response_controller.rb handles the operations on responses based on user permissions and redirects the user to the appropriate place after the action is complete. The responses here constitute of peer reviews/questionnaires/survey. The controller takes care of tasks such as creating, saving, editing, updating and deleting these responses.
Tasks accomplished
1. Refactoring response_controller.rb
Included:
- Breaking down large methods into smaller chunks of readable reusable code.
- Changing code to adhere to latest Ruby conventions
- Creating functions to reuse chunks of code that were previously written multiple times
2. Testing response_controller.rb
Included writing integration test cases to ensure that response_controller redirects to the right places with a given parameter set.
Pull request
The pull request for this task can be viewed at [1]
Refactoring Explained
Without wasting much time, lets jump into the refactoring by describing code which existed previously followed by the changed code.
1) action_allowed? method
Previous Code :-
def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = Response.find(params[:id])
return false if response.is_submitted
return current_user_id?(response.map.reviewer.user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
response = Response.find(params[:id])
return current_user_id?(response.map.reviewer.user_id)
when 'view'
response = Response.find(params[:id])
map = response.map
assignment = response.map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(response.map.reviewer.user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(response.map.reviewer.user_id)
end
else
current_user
end
end
The view case is extracted into a separate method and some common variables have been extracted out of the cases
def action_allowed?
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
response = Response.find(params[:id])
user_id = response.map.reviewer.user_id if (response.map.reviewer)
end
case action
when 'edit' # If response has been submitted, no further editing allowed
return false if response.is_submitted
return current_user_id?(user_id)
# Deny access to anyone except reviewer & author's team
when 'delete', 'update'
return current_user_id?(user_id)
when 'view'
return edit_allowed?(response.map, user_id)
else
current_user
end
end
def edit_allowed?(map, user_id)
assignment = map.reviewer.assignment
# if it is a review response map, all the members of reviewee team should be able to view the reponse (can be done from heat map)
if map.is_a? ReviewResponseMap
reviewee_team = AssignmentTeam.find(map.reviewee_id)
return current_user_id?(user_id) || reviewee_team.has_user(current_user) || current_user.role.name == 'Administrator' || (current_user.role.name == 'Instructor' and assignment.instructor_id == current_user.id) || (current_user.role.name == 'Teaching Assistant' and TaMapping.exists?(ta_id: current_user.id, course_id: assignment.course.id))
else
return current_user_id?(user_id)
end
end
This has extracted an independent functionality and enhanced readability apart from sticking to guidelines of short methods.
2) Replaced find_by_map_id with find_by
@map = Response.find_by_map_id(params[:id])
Moving on with latest Rails conventions, function is being modified as
@map = Response.find_by(map_id: params[:id])
This will avoid any unexpected behaviour when further upgrading the Rails framework.
3) Replacing sorting technique
questions.sort {|a, b| a.seq <=> b.seq }
has been replaced with
questions.sort_by(&:seq)
This is the way to sort based on an object attribute.
4) Refactoring pending_surveys method
def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end
# Get all the participant(course or assignment) entries for this user
course_participants = CourseParticipant.where(user_id: session[:user].id)
assignment_participants = AssignmentParticipant.where(user_id: session[:user].id)
# Get all the course survey deployments for this user
@surveys = []
if course_participants
course_participants.each do |cp|
survey_deployments = CourseSurveyDeployment.where(parent_id: cp.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => cp.parent_id,
'participant_id' => cp.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
# Get all the assignment survey deployments for this user
if assignment_participants
assignment_participants.each do |ap|
survey_deployments = AssignmentSurveyDeployment.where(parent_id: ap.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end
This method involved a lot of code which violates guideline of short methods. Moreover, the lines of code can be reduced along with readability enhanced as similar functionalities in multiple lines can be combined.
53 lines of code have been reduced to 32. The refactored method is given below:-
def pending_surveys
unless session[:user] # Check for a valid user
redirect_to '/'
return
end
# Get all the course survey deployments for this user
@surveys = []
[CourseParticipant, AssignmentParticipant].each do |item|
# Get all the participant(course or assignment) entries for this user
participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
survey_deployments = survey_deployment_type.where(parent_id: p.parent_id)
next unless survey_deployments
survey_deployments.each do |survey_deployment|
next unless survey_deployment && Time.now > survey_deployment.start_date && Time.now < survey_deployment.end_date
@surveys <<
[
'survey' => Questionnaire.find(survey_deployment.questionnaire_id),
'survey_deployment_id' => survey_deployment.id,
'start_date' => survey_deployment.start_date,
'end_date' => survey_deployment.end_date,
'parent_id' => p.parent_id,
'participant_id' => p.id,
'global_survey_id' => survey_deployment.global_survey_id
]
end
end
end
end
Integration Tests
23 integration tests were written to ensure the functionality works and also to make sure the refactoring does not break any of the existing functionality. Writing the test including writing stubs and mocks to ensure that the tests are fed what is required.
The following are the stubs and mocks written for the testing:
success_response = Net::HTTPResponse.new(1.0, 200, "OK")
current_round = 1
stage = nil
let(:assignment) { build(:assignment, instructor_id: 6) }
let(:instructor) { build(:instructor, id: 6) }
let(:participant) { build(:participant, id: 1, user_id: 6, assignment: assignment) }
let(:review_response) { build(:response, id: 1, map_id: 1) }
let(:review_response_map) { build(:review_response_map, id: 1, reviewer: participant) }
let(:questionnaire) { build(:questionnaire, id: 1, questions: [question]) }
let(:question) { Criterion.new(id: 1, weight: 2, break_before: true) }
let(:assignment_questionnaire) { build(:assignment_questionnaire) }
let(:answer) { double('Answer') }
let(:assignment_due_date) { build(:assignment_due_date) }
before(:each) do
stub_current_user(instructor, instructor.role.name, instructor.role)
allow(Assignment).to receive(:find).with('1').and_return(assignment)
allow(Assignment).to receive(:find).and_return(assignment)
allow(Response).to receive(:find).with('1').and_return(review_response)
allow(Response).to receive(:find).and_return(review_response)
allow(Response).to receive(:find_by).and_return(review_response)
allow(ResponseMap).to receive(:find).with('1').and_return(review_response_map)
allow(ResponseMap).to receive(:find).with(1).and_return(review_response_map)
allow(Participant).to receive(:find).with(1).and_return(participant)
allow(Questionnaire).to receive(:find).with('1').and_return(questionnaire)
allow(assignment).to receive(:number_of_current_round).and_return(current_round)
allow(assignment).to receive(:get_current_stage).and_return(stage)
allow(review_response).to receive(:delete).and_return(success_response)
allow(review_response).to receive(:map).and_return(review_response_map)
allow(review_response).to receive(:questionnaire_by_answer).and_return(questionnaire)
allow(review_response_map).to receive(:assignment).and_return(assignment)
allow(review_response_map).to receive(:save).and_return(true)
allow(review_response_map).to receive(:questionnaire).with(current_round).and_return(questionnaire)
request.env['HTTP_REFERER'] = 'www.google.com'
end
The following are a few test cases written by the team:
describe '#edit' do
it 'renders response#response page' do
params = {id: review_response.id, return: ''}
post :edit, params: params
expect(response).to render_template("response")
end
end
describe '#pending_surveys' do
context 'when session[:user] is nil' do
it 'redirects to root path (/)' do
get "pending_surveys"
expect(response).to redirect_to('/')
end
end
context 'when session[:user] is not nil' do
it 'renders pending_surveys page' do
session[:user] = participant
get "pending_surveys"
expect(response).to have_http_status(200)
expect(response).to render_template("pending_surveys")
end
end
end