E1502: Questionnaire Controller Refactoring
Introduction to Expertiza
Project Description
What it does:
Used on the admin side of Expertiza for creating/ editing questionnaires (rubrics, surveys and quizzes). It helps in add/removing questions, options, etc for a questionnaire.
Very big controller that handles a lot more than the name suggests. Functionalities need to be moved to appropriate controllers.
Quiz methods are should be treated the same as any other type of questionnaire; differences between quiz questionnaires and other questionnaires should be implemented in the model class, quiz_questionnaire
Turn the questionnaire into a “form object.” The ..._questions methods: save_new_questions, delete_questions, save_questions should be in a separate class.
Other classes involved:
questionnaire.rb
quiz_questionnaire.rb
questions_controller.rb
What needs to be done:
Move quiz related functions to quiz_questionnaire.rb.
copy, update_quiz, valid_quiz methods, clone_questionnaire_details is too long.
Debug output (print statements) should be removed.
Understand the functions in the controller and comment them. Ensure that the code is understandable to the next programmer who works on it.
What We Have Done
Method Refactoring
| Method Name
|
Changes Made
|
Reason For Change
|
| copy
|
Extracted the content of this method as copy_questionnaires method and put it in questionnaire.rb
|
The content of this method is about operations on the database (coping a questionnaire), it is better to put it in the model
|
| valid_quiz
|
Moved this method to quiz_questionnaire.rb
|
This method is about validation of the quiz, it shouldn't appear in the controller
|
| export
|
Moved this method to questionnaire.rb
|
This method exports the questionnaires as csv file, it should't appear in the controller
|
| import
|
Moved this method to questionnaire.rb
|
This method imports the questionnaires from csv file, it should't appear in the controller
|
| clone_questionnaire_details
|
Deleted this method due to the duplication
|
Substituted by copy_questionnaires method in questionnaire.rb
|
Format Refactoring
| Before
|
After
|
# Remove a given questionnaire
def delete
@questionnaire = Questionnaire.find(params[:id])
if @questionnaire
begin
name = @questionnaire.name
for question in @questionnaire.questions
current_q_type = QuestionType.find_by_question_id(question.id)
unless current_q_type.nil?
current_q_type.delete
end
end
@questionnaire.assignments.each{
| assignment |
raise "The assignment #{assignment.name} uses this questionnaire. Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
}
@questionnaire.destroy
undo_link("Questionnaire \"#{name}\" has been deleted successfully. ")
rescue
flash[:error] = $!
end
end
redirect_to :action => 'list', :controller => 'tree_display'
end
|
# Remove a given questionnaire
def delete
@questionnaire = Questionnaire.find(params[:id])
if @questionnaire
begin
name = @questionnaire.name
@questionnaire.questions.each do |question|
current_q_type = QuestionType.find_by_question_id(question.id)
if current_q_type
current_q_type.delete
end
end
@questionnaire.assignments.each{
| assignment |
raise "The assignment #{assignment.name} uses this questionnaire. Do you want to <A href='../assignment/delete/#{assignment.id}'>delete</A> the assignment?"
}
@questionnaire.destroy
undo_link("Questionnaire \"#{name}\" has been deleted successfully. ")
rescue
flash[:error] = $!
end
end
redirect_to :action => 'list', :controller => 'tree_display'
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
|