Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E1745. [TLD] Refactor response_controller.rb #1025

Merged
merged 20 commits into from Nov 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 39 additions & 55 deletions app/controllers/response_controller.rb
Expand Up @@ -3,31 +3,37 @@ class ResponseController < ApplicationController
helper :file

def action_allowed?
case params[:action]
when 'edit' # If response has been submitted, no further editing allowed
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good refactoring!

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?(response.map.reviewer.user_id)
return current_user_id?(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)
return current_user_id?(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
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

def delete
@response = Response.find(params[:id])
# user cannot delete other people's responses. Needs to be authenticated.
Expand Down Expand Up @@ -163,8 +169,7 @@ def saving
def redirection
flash[:error] = params[:error_msg] unless params[:error_msg] and params[:error_msg].empty?
flash[:note] = params[:msg] unless params[:msg] and params[:msg].empty?
@map = Response.find_by_map_id(params[:id])

@map = Response.find_by(map_id: params[:id])
if params[:return] == "feedback"
redirect_to controller: 'grades', action: 'view_my_scores', id: @map.reviewer.id
elsif params[:return] == "teammate"
Expand Down Expand Up @@ -200,49 +205,28 @@ def pending_surveys
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)
[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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the logic here is correct. participant_type is an array of participants, instead of the type of participant. I think this mistake is due to your misleading variable names. And you just refactor code without varifying the correctness.

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' => ap.parent_id,
'participant_id' => ap.id,
'global_survey_id' => survey_deployment.global_survey_id
]
[
'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
Expand Down Expand Up @@ -313,7 +297,7 @@ def set_dropdown_or_scale
end

def sort_questions(questions)
questions.sort {|a, b| a.seq <=> b.seq }
questions.sort_by(&:seq)
end

def create_answers(params, questions)
Expand Down