Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-11-02T23:05:04Z

Panand4: /* What all was tested */

==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 [https://github.com/expertiza/expertiza/pull/1025]

==='''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 :-
<pre>

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

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

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

</pre>

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

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

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

</pre>

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

<pre>

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

</pre>

==='''Test Plan and Execution'''===
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.
These tests check for the basic functionality of the controller response_controller.rb and whether these function calls redirect to the right place with the correct status code. Consider the following example.

<pre>
context 'when params action is view' do
context 'when response_map is a ReviewResponseMap and current user is the instructor of current assignment' do
it 'allows certain action' do
params = {action: "view", id: review_response.id}
controller.params = params
expect(controller.action_allowed?).to eq(true)
end
end
end
</pre>

The above given test checks that if the instructor wants to view a review, he is given permission to go ahead and view it. This is checked by setting params = {action: "view", id: review_response.id}.
To do so, we set these params and expect the function action_allowed to return true.
The current user is set in the before(:each) part of the test file because that is used commonly by a number of test cases.

===What all was tested===
We tested the existing functionality of the controller like view, edit, action_allowed, redirection, etc since we are changing only the implementation of the code. And a general rule of writing test cases is that changing the implementation should not have any effect on tests. Test case scenarios were provided by our mentor and our job was to write the integration tests for those scenarios. the test cases were pretty exhaustive. We didn't have to write extra cases for the parts we refactored as the refactored portions are an extraction of an existing function in smaller ones, in which case the existing function is already calling the new written code. So in a way writing the test cases for existing parts automatically tests the new refactored code.

===Stubs and mocks===
We wrote a number of stubs for the test cases. We separated out stubs into two parts, the ones that are being commonly used by multiple tests and wrote them in the before(:each) part. The other uncommon ones are written in the respective test cases to avoid extra computation. Stubs allowed us to replicate the behaviour of the database without actually calling, thus avoiding cost of accessing the database and avoiding the pitfall of having a test case fail because of an incorrect database layer logic rather than the controller logic.

CSC/ECE 517 Fall 2017

2017-10-24T21:37:17Z

Panand4:

'''Writing Assignment 2'''
*[[CSC/ECE 517 Fall 2017/E1779. Fix teammate advertisements and requests to join a team]]
*[[CSC/ECE_517_Fall_2017/E1773 Investigate and Fix Expertiza Production Version Runtime Exceptions.rb]]
*[[CSC/ECE_517_Fall_2017/E1774 Metareview fixes and improvements.rb]]
*[[CSC/ECE_517_Fall_2017/E1788 OSS project Maroon Heatmap fixes]]
*[[CSC/ECE 517 Fall 2017/E1781 Topic Management]]
*[[CSC/ECE_517_Fall_2017/E1745_Refactor_response_controller.rb]]

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-24T21:32:32Z

Panand4:

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

==='''Tasks operated by Team'''===
The tasks completed by the team consisted of refactoring response_controller.rb which is mainly responsible for handling the response for peer reviews/questionnaires/survey with tasks such as creating, saving, editing, updating, deletion. The functionality is further ensured stable by the team with 23 integration tests being written for the same.

==='''Team Members'''===
1) Pavneet Singh Anand

2) Dipansha Gupta

3) Kshittiz Kumar

==='''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 :-
<pre>

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

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

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

</pre>

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

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

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

</pre>

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

<pre>

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

</pre>

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

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-24T21:30:34Z

Panand4: /* Team Members */

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

==='''Tasks operated by Team'''===
The tasks completed by the team consisted of refactoring response_controller.rb which is mainly responsible for handling the response for peer reviews/questionnaires/survey with tasks such as creating, saving, editing, updating, deletion. The functionality is further ensured stable by the team with 23 integration tests being written for the same.

==='''Team Members'''===
1) Pavneet Singh Anand

2) Dipansha Gupta

3) Kshittiz Kumar

==='''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 :-
<pre>

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

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

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

</pre>

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

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

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

</pre>

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

<pre>

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

</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-24T21:29:41Z

Panand4:

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

==='''Tasks operated by Team'''===
The tasks completed by the team consisted of refactoring response_controller.rb which is mainly responsible for handling the response for peer reviews/questionnaires/survey with tasks such as creating, saving, editing, updating, deletion. The functionality is further ensured stable by the team with 23 integration tests being written for the same.

==='''Team Members'''===
1) Pavneet Singh Anand
2) Dipansha Gupta
3) Kshittiz Kumar

==='''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 :-
<pre>

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

</pre>

The view case is extracted into a separate method and some common variables have been extracted out of the cases

<pre>

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

</pre>

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

<pre>

@map = Response.find_by_map_id(params[:id])

</pre>

Moving on with latest Rails conventions, function is being modified as

<pre>

@map = Response.find_by(map_id: params[:id])

</pre>

This will avoid any unexpected behaviour when further upgrading the Rails framework.

3) Replacing sorting technique

<pre>

questions.sort {|a, b| a.seq <=> b.seq }

</pre>

has been replaced with

<pre>

questions.sort_by(&:seq)

</pre>

This is the way to sort based on an object attribute.

4) Refactoring pending_surveys method

<pre>

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

</pre>

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

<pre>

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

</pre>

CSC/ECE 517 Fall 2017/E1745 Refactor response controller.rb

2017-10-24T20:51:11Z

Panand4:

Expertiza

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.