CSC/ECE 517 Fall 2014/oss E1508 MRS
E1508 Refactoring Response Controller
This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the ResponseController.
Problem Statement
Classes involved:
response_controller.rb response.rb ???????
What they do: Allows the user to create and edit responses to questionnaires … such as performing a review, rating a teammate, or giving feedback to a reviewer.
What's wrong with it:
- It doesn’t do authorization properly.
- It contains duplicated methods.
- Functionality that should be in models is incorporated into the controller.
What needs to be done:
- latestresponseversion seems misnamed. It fetches all previous versions of a response (which is to say, all previous review versions by the current reviewer of the current author for the current assignment).
- get_scores gets all the scores assigned in a particular response. A “response” is created when someone submits a review, a partner evaluation, a survey, etc.
- The rereview method is 98 lines long. Several parts of it should be turned into methods. Sorting review versions is really not a controller responsibility; it would be better to do this in a model class (which class?) Ditto for determining whether a review is current (i.e., was done during the current assignment phase). This is a query that is made about a review (actually, about a response, which may be a review, author feedback, etc.). It should be placed in the appropriate model class.
- rereview contains special code to check whether an assignment is “Jen’s assignment”; this was the first assignment we ever created with a multipart rubric. It was hard-coded into the system, rather than working on a rubric that was created in the normal way. It is probably impossible to remove this code without breaking that assignment, but it should be done in a separate method, and commented appropriately as a kludge.
- Again in rereview, creating a new version of a review is a model responsibility, and should be moved out of the controller.
- There are two (consecutive) copies of the edit method. The second appears to be the newer one, and, according to the rules for method definition, is the one that is currently in use. The first should be removed. Ditto for new_feedback and view--the 2nd version of each appears to be newer & should be kept.
- Authorization to perform actions is not checked correctly. It is supposed to be done through the action_allowed? method at the beginning of the class definition. Different authorization should be required for different operations. For example, someone should be allowed to view a response if they wrote the response, or they are the person or on the team whose work the response applied to, or if they are an instructor or TA for the class. The person who wrote a response should be allowed to edit it, but not the person/team who was being reviewed, nor the instructor or TA for the class. Currently, authorization is denied by the redirect_when_disallowed method, which was an earlier, more error-prone way of controlling access. It should go away, now that the class has an action_allowed? method.
Changes Made
Response Controller
| Method Name | Changes Made | Reason For Change |
|---|---|---|
| latestResponseVersion | Changed name to previous_responses | The name wasn't indicative of what the method did |
| edit | Deleted the old edit method | The old edit method was not getting called due to ruby rules |
| new_feedback | Deleted the old new_feedback method | The old new_feedback method was not getting called due to ruby rules |
| view | Deleted the old view method | The old view method was not getting called due to ruby rules |
| redirect_when_disallowed | Moved all of the code from redirect_when_disallowed to action_allowed and changed all of the references | Authorization to perform actions wasn't being performed correctly, it is supposed to be done through action_allowed? |
| action_allowed? | ||
| rereview | Moved logic for sorting response versions, and for getting the latest response to their own methods | The rereview method was becoming too large and confusing |
Response Model
| Method Name | Changes Made | Reason For Change |
|---|---|---|
| get_scores | Moved from controller (response_controller.rb) to model (response.rb) | The functionality in get_scores makes more sense to be implemented in the Model rather than in the controller |
Re-factored Code Cases
Case 1 : Refactoring the ResponseController
Removing duplicate methods
The ResponseController allows the user to create and edit responses to questionnaires, and the functionality changed over time, causing two new_feedback and view methods to be created; the oldest of which doesn't get called anymore.
| Before Changes | After Changes |
|---|---|
def new_feedback
review = Response.find(params[:id])
if review
reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id: review.map.assignment.id).first
map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id: reviewer.id).first
if map.nil?
map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id,
:reviewee_id => review.map.reviewer.id)
end
redirect_to :action => 'new', :id => map.map_id, :return => "feedback"
else
redirect_to :back
end
end
def new_feedback
review = Response.find(params[:id])
if review
reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id: review.map.assignment.id).first
map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id: reviewer.id).first
if map.nil?
#if no feedback exists by dat user den only create for dat particular response/review
map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id,
:reviewee_id => review.map.reviewer.id)
end
redirect_to :action => 'new', :id => map.id, :return => "feedback"
else
redirect_to :back
end
end
|
def new_feedback
review = Response.find(params[:id])
if review
reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id: review.map.assignment.id).first
map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id: reviewer.id).first
if map.nil?
#if no feedback exists by dat user den only create for dat particular response/review
map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id,
:reviewee_id => review.map.reviewer.id)
end
redirect_to :action => 'new', :id => map.id, :return => "feedback"
else
redirect_to :back
end
end
|
def view
@response = Response.find(params[:id])
return if redirect_when_disallowed(@response)
@map = @response.map
get_content
@review_scores = Array.new
@question_type = Array.new
@questions.each do |question|
@review_scores << Score.where(response_id: @map.response_id, question_id: question.id).first
@question_type << QuestionType.find_by_question_id(question.id)
end
end
def view
@response = Response.find(params[:id])
return if action_allowed?(@response)
@map = @response.map
get_content
get_scores(@response, @questions)
end
|
def view
@response = Response.find(params[:id])
return if action_allowed?(@response)
@map = @response.map
get_content
get_scores(@response, @questions)
end
|
Moving code to the proper place
The ResponseController allows the user to create and edit responses to questionnaires, in doing so, there needs to be some sort of authentication, and the proper place for that is in the action_allowed? method, however the un-refactored code did in the redirect_when_disallowed method. Moreover, once this code was moved to the action_allowed? method, all of the references to redirect_when_disallowed were changed to action_allowed? (left out of wiki for brevity).
| Before Changes | After Changes |
|---|---|
def action_allowed?
current_user
end
def redirect_when_disallowed(response)
# For author feedback, participants need to be able to read feedback submitted by other teammates.
# If response is anything but author feedback, only the person who wrote feedback should be able to see it.
if response.map.read_attribute(:type) == 'FeedbackResponseMap' && response.map.assignment.team_assignment?
team = response.map.reviewer.team
unless team.has_user session[:user]
redirect_to '/denied?reason=You are not on the team that wrote this feedback'
else
return false
end
response.map.read_attribute(:type)
end
!current_user_id?(response.map.reviewer.user_id)
end
|
def action_allowed?(response)
# For author feedback, participants need to be able to read feedback submitted by other teammates.
# If response is anything but author feedback, only the person who wrote feedback should be able to see it.
if response.map.read_attribute(:type) == 'FeedbackResponseMap' && response.map.assignment.team_assignment?
team = response.map.reviewer.team
unless team.has_user session[:user]
redirect_to '/denied?reason=You are not on the team that wrote this feedback'
else
return false
end
response.map.read_attribute(:type)
end
!current_user_id?(response.map.reviewer.user_id)
end
|
Moving get_scores to the Response Model
The get_scores method was being implemented in the controller. Its function fits better in the model. As such, the method was moved and changed to retain the functionality of code that uses it. Since the variables it requires were no longer in scope, they're passed in as arguments. Also due to scope, the result has to be explicitly returned.
| Before Changes | After Changes |
|---|---|
In response_controller.rb def get_scores - @review_scores = [] - @question_type = [] - @questions.each do |question| - @review_scores << Score - .where( - response_id: @response.id, - question_id: question.id - ).first - @question_type << QuestionType.find_by_question_id(question.id) - end - end |
In response.rb
def get_scores(response, questions)
review_scores = []
question_type = []
questions.each do |question|
review_scores << Score
.where(
response_id: response.id,
question_id: question.id
).first
question_type << QuestionType.find_by_question_id(question.id)
end
question_type
end
In response_controller.rb
def view
@response = Response.find(params[:id])
- return if action_allowed?(@response)
+ #return if action_allowed?(@response)
@map = @response.map
get_content
- get_scores(@response, @questions)
+ @question_type = @response.get_scores(@response, @questions)
end
|