diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 51f76b2..7d48ddf 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -2,95 +2,54 @@ after_action :verify_authorized respond_to :html - # PUT/PATCH /[:locale]/answer/[:id] + # PUT/PATCH /answers/[:id] def update - question_id = params[:answer][:question_id] - plan_id = params[:answer][:plan_id] - @question = Question.find(question_id); - # If an answer id is present load that answer otherwise load by plan/question - @answer = Answer.find_by(plan_id: plan_id, question_id: question_id) - - @old_answer = nil - @timestamp = nil - - if @answer.nil? # If there is no answer for plan/question - @answer = Answer.new(permitted_params) - @answer.text = params["answer-text-#{@answer.question_id}".to_sym] - - authorize @answer - - if @answer.save - @timestamp = @answer.updated_at.iso8601 - end - @lock_version = @answer.lock_version - elsif params[:answer][:id].blank? # Someone else already added an answer while the user was working - @old_answer = Marshal::load(Marshal.dump(@answer)) - @answer.text = params["answer-text-#{@answer.question_id}".to_sym] - - authorize @answer - - @lock_version = @answer.lock_version - else # We're about updating an answer (let ActiveRecord check for a race condition) - @old_answer = Marshal::load(Marshal.dump(@answer)) - @answer.text = params["answer-text-#{@answer.question_id}".to_sym] - - authorize @answer - - if @answer.update(permitted_params) - @answer.touch # Saves the record with the updated_at set to the current time. Needed if only answer.question_options is updated - @timestamp = @answer.updated_at.iso8601 - end - @lock_version = @answer.lock_version - @old_answer = nil - end - - @section_id = @answer.question.section.id - @username = @answer.user.name - - @nquestions = 0 - @nanswers = 0 - @n_section_questions = 0 - @n_section_answers = 0 - - plan = Plan.find(plan_id) - # Problem of N+1 queries below - plan.template.phases.each do |phase| - phase.sections.each do |section| - section.questions.each do |question| - @nquestions += 1 - if section.id == @section_id - @n_section_questions += 1 - end - question.answers = question.answers.to_a.select {|answer| answer.plan_id == plan.id} - if question.answers.present? && question.answers.first.text.present? - @nanswers += 1 - if section.id == @section_id - @n_section_answers += 1 - end - end + p_params = permitted_params() + @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id], }) + begin + if @answer + authorize @answer + @answer.update(p_params) + if p_params[:question_option_ids].present? + @answer.touch() # Saves the record with the updated_at set to the current time. Needed if only answer.question_options is updated end + else + @answer = Answer.new(p_params) + @answer.lock_version = 1 + authorize @answer + @answer.save() # NOTE, there is a chance to create multiple answer associated for a plan/question (IF any concurrent thread) INSERTS an answer after checking the existence of an answer (Line 8) + # In order to avoid that edge-case, it is recommended to create answers whenever a new plan is created (e.g. after_create callback) end + rescue ActiveRecord::StaleObjectError + @stale_answer = @answer + @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]}) end + + @plan = Plan.includes({ + sections: { + questions: [ + :answers, + :question_format + ] + } + }).find(p_params[:plan_id]) + @question = @answer.question + @section = @plan.get_section(@question.section_id) respond_to do |format| format.js {} end - - rescue ActiveRecord::StaleObjectError - @username = @old_answer.user.name - @lock_version = @old_answer.lock_version - respond_to do |format| - format.js {} - end - end # End update private def permitted_params - permitted = params.require(:answer).permit(:id, :plan_id, :user_id, :question_id, :lock_version, :question_option_ids => []) - if !permitted[:question_option_ids].present? #If question_option_ids has been filtered out because it was a scalar value (e.g. radiobutton answer) + permitted = params.require(:answer).permit(:id, :text, :plan_id, :user_id, :question_id, :lock_version, :question_option_ids => []) + if !params[:answer][:question_option_ids].nil? && !permitted[:question_option_ids].present? #If question_option_ids has been filtered out because it was a scalar value (e.g. radiobutton answer) permitted[:question_option_ids] = [params[:answer][:question_option_ids]] # then convert to an Array end + if !permitted[:id].present? + permitted.delete(:id) + end return permitted end # End permitted_params end diff --git a/app/controllers/phases_controller.rb b/app/controllers/phases_controller.rb index 99406e9..b9d9685 100644 --- a/app/controllers/phases_controller.rb +++ b/app/controllers/phases_controller.rb @@ -15,24 +15,6 @@ @phase = @plan.template.phases.select {|p| p.id == phase_id}.first @readonly = !@plan.editable_by?(current_user.id) - # the eager_load pulls in ALL answers - # need to restrict to just ones for this plan - # at the same time count the questions and answers for the status - @nquestions = 0 - @nanswers = 0 - - @plan.template.phases.each do |phase| - phase.sections.each do |section| - section.questions.each do |question| - @nquestions += 1 - question.answers = question.answers.to_a.select {|answer| answer.plan_id == @plan.id} - if question.answers.present? && question.answers.first.text.present? - @nanswers += 1 - end - end - end - end - # Now we need to get all the themed guidance for the plan. # TODO: think this through again, there may be a better way to do this. # diff --git a/app/models/answer.rb b/app/models/answer.rb index 8267948..d46ad59 100644 --- a/app/models/answer.rb +++ b/app/models/answer.rb @@ -42,4 +42,18 @@ def has_question_option(option_id) self.question_option_ids.include?(option_id) end + + # Returns true if the answer is valid and false otherwise. If the answer's question is option_based, it is checked if exist + # any question_option selected. For non option_based (e.g. textarea or textfield), it is checked the presence of text + def is_valid? + if self.question.present? + if self.question.question_format.option_based? + return !self.question_options.empty? + else # (e.g. textarea or textfield question formats) + return self.text.present? + end + else + return false + end + end end diff --git a/app/models/plan.rb b/app/models/plan.rb index 549b4fe..5db4cde 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -949,8 +949,29 @@ end =end + # Returns the number of answered questions from the entire plan + def num_answered_questions + n = 0 + self.sections.each do |s| + n+= s.num_answered_questions(self.id) + end + return n + end + # Returns a section given its id or nil if does not exist for the current plan + def get_section(section_id) + self.sections.find { |s| s.id == section_id } + end + # Returns the number of questions for a plan. Note, this method becomes useful + # for when sections and their questions are eager loaded so that avoids SQL queries. + def num_questions + n = 0 + self.sections.each do |s| + n+= s.questions.size() + end + return n + end # the following two methods are for eager loading. One gets used for the plan/show # page and the oter for the plan/edit. The difference is just that one pulls in more than # the other. diff --git a/app/models/section.rb b/app/models/section.rb index abf1327..4d4562b 100644 --- a/app/models/section.rb +++ b/app/models/section.rb @@ -23,10 +23,11 @@ "#{title}" end + # Returns the number of answered questions for a given plan id def num_answered_questions(plan_id) n = 0 self.questions.each do |question| - n += question.plan_answers(plan_id).select{|answer| answer.text.present?}.count + n += question.plan_answers(plan_id).select{|answer| answer.is_valid?}.count end return n end diff --git a/app/views/answers/_form_fields.html.erb b/app/views/answers/_form_fields.html.erb new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/app/views/answers/_form_fields.html.erb diff --git a/app/views/answers/_locking.html.erb b/app/views/answers/_locking.html.erb new file mode 100644 index 0000000..0bc9cc2 --- /dev/null +++ b/app/views/answers/_locking.html.erb @@ -0,0 +1,5 @@ +
<%= _('The following answer cannot be persisted') %>
+ <%= render partial: '/answers/new_edit', locals: { question: question, answer: answer, readonly: true } %> +<%= _('since %{name} saved the answer below while you were editing. Please, combine your changes and then save the answer again.') % { name: user.name} %>
+<%= _("While you were editing ") + answer.user.name + _(" saved the following answer:") %>
- -<%= semantic_form_for answer, :url => {:controller => :answers, :action => :update }, :remote => true do |af| %> - <%= af.inputs do %> - - - <% if question.option_based? %> - <% options = question.options.order("number") %> - - - <% if qformat.checkbox? %> - <%= af.input :options, - :as => :check_boxes, - :collection => options, - :label => false, - :input_html => { :disabled => :true } - %> - <% elsif qformat.multiselectbox? %> - <%= af.input :options, - :as => :select, - :collection => options, - :label => false, - :input_html => { :multiple => true , :disabled => :true } %> - <% elsif qformat.radiobuttons?%> -<%= _('Combine their changes with your answer below and then save the answer again.') %>
<%= _('The edits in the box below will overwrite the existing answer from ') + answer.user.name + _(' once you click save!')%>
<%= comments_label_with_count %>
+<%= comments_label_with_count %>
<% else %> -<%= _('Share note') %>
+<%= _('Share note') %>
<% end %>