From 63974e91c8947df351b9a77b5c8b52d92c9d097d Mon Sep 17 00:00:00 2001
From: xsrust <xsrust@gmail.com>
Date: Mon, 6 Nov 2017 15:36:21 +0000
Subject: [PATCH] updated answers and notes controller for #798

---
 app/controllers/answers_controller.rb | 44 ++++++++++++++-------------
 app/controllers/notes_controller.rb   | 23 ++++++++------
 app/views/notes/_add.html.erb         |  1 -
 3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb
index 7d48ddf5b7..406d75fd6f 100644
--- a/app/controllers/answers_controller.rb
+++ b/app/controllers/answers_controller.rb
@@ -3,31 +3,33 @@ class AnswersController < ApplicationController
   respond_to :html
 
 	# PUT/PATCH /answers/[:id]
-	def update
+  def update
     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
+    Answer.transaction do
+      @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]})
+      begin
+        if @answer.present?
+          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
+          # NOTE: save! and destroy! must be used for transactions as they raise errors instead of returning false
+          @answer.save!
         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)
+      rescue ActiveRecord::StaleObjectError
+        @stale_answer = @answer
+        @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]})
       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: [ 
+      sections: {
+        questions: [
           :answers,
           :question_format
         ]
@@ -37,7 +39,7 @@ def update
     @section = @plan.get_section(@question.section_id)
 
     respond_to do |format|
-      format.js {} 
+      format.js {}
     end
   end # End update
 
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 474281a18e..42b2c6d621 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -8,22 +8,25 @@ def create
     @note = Note.new
     user_id = params[:new_note][:user_id]
     @note.user_id = user_id
-    answer_id = params[:new_note][:answer_id]
     question_id = params[:new_note][:question_id]
     plan_id = params[:new_note][:plan_id]
 
     # create answer if we dont already have one
-    if answer_id.present?
-      answer = Answer.find(answer_id)
-    else
-      answer = Answer.new
-      answer.plan_id = plan_id
-      answer.question_id = question_id
-      answer.user_id = user_id
-      answer.save!
+    answer=nil  # if defined within transaction block, was not accessable after
+    # ensure user has access to plan BEFORE creating/finding an answer
+    raise Pundit::NotAuthorizedError unless Plan.find(plan_id).readable_by?(user_id)
+    Answer.transaction do
+      answer = Answer.find_by(question_id: question_id, plan_id: plan_id)
+      if answer.blank?
+        answer = Answer.new
+        answer.plan_id = plan_id
+        answer.question_id = question_id
+        answer.user_id = user_id
+        answer.save!
+      end
     end
 
-    @note.answer= answer
+    @note.answer = answer
     @note.text = params["#{question_id}new_note_text"]
 
     authorize @note
diff --git a/app/views/notes/_add.html.erb b/app/views/notes/_add.html.erb
index 2817b90020..e335daaf93 100644
--- a/app/views/notes/_add.html.erb
+++ b/app/views/notes/_add.html.erb
@@ -12,7 +12,6 @@
              id: "new_note_form_#{questionid}") do |f| %>
 	<%= f.hidden_field :user_id, value: current_user.id %>
 	<%= f.hidden_field :question_id, value: questionid %>
-	<%= f.hidden_field :answer_id, value: answer.id %>
 	<%= f.hidden_field :plan_id, value: plan_id %>
 
 	<fieldset class="standard">