diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index d8f09ca..a8b6093 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -5,55 +5,54 @@ # PUT/PATCH /answers/[:id] 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 - 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) + @answer = Answer.find_by!({ plan_id: p_params[:plan_id], question_id: p_params[:question_id] }) + 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 + rescue ActiveRecord::RecordNotFound => e + skip_authorization + render json: { detail: e.message }, status: :not_found 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) - render json: { - "question" => { - "id" => @question.id, - "answer_lock_version" => @answer.lock_version, - "locking" => @stale_answer ? - render_to_string(partial: 'answers/locking', locals: { question: @question, answer: @stale_answer, user: @answer.user }, formats: [:html]) : - nil, - "answer_status" => render_to_string(partial: 'answers/status', locals: { answer: @answer}, formats: [:html]) - }, - "section" => { - "id" => @section.id, - "progress" => render_to_string(partial: '/sections/progress', locals: { section: @section, plan: @plan }, formats: [:html]) - }, - "plan" => { - "id" => @plan.id, - "progress" => render_to_string(:partial => 'plans/progress', locals: { plan: @plan, current_phase: @section.phase }, formats: [:html]) - } - }.to_json + if @answer.present? + @plan = Plan.includes({ + sections: { + questions: [ + :answers, + :question_format + ] + } + }).find(p_params[:plan_id]) + @question = @answer.question + @section = @plan.get_section(@question.section_id) + + render json: { + "question" => { + "id" => @question.id, + "answer_lock_version" => @answer.lock_version, + "locking" => @stale_answer ? + render_to_string(partial: 'answers/locking', locals: { question: @question, answer: @stale_answer, user: @answer.user }, formats: [:html]) : + nil, + "form" => render_to_string(partial: 'answers/new_edit', locals: { question: @question, answer: @answer, readonly: false }, formats: [:html]), + "answer_status" => render_to_string(partial: 'answers/status', locals: { answer: @answer}, formats: [:html]) + }, + "section" => { + "id" => @section.id, + "progress" => render_to_string(partial: '/sections/progress', locals: { section: @section, plan: @plan }, formats: [:html]) + }, + "plan" => { + "id" => @plan.id, + "progress" => render_to_string(:partial => 'plans/progress', locals: { plan: @plan, current_phase: @section.phase }, formats: [:html]) + } + }.to_json + end + end # End update private diff --git a/app/models/plan.rb b/app/models/plan.rb index 989d63c..05a7b1d 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -20,6 +20,12 @@ has_many :roles + # Active Record Callbacks + # Creates answers for plan question and persists them whenever a new plan is created and successfully saved + after_create do + Answer.create( + self.questions.map{ |q| { lock_version: 1, text: q.default_value, plan_id: self.id, question_id: q.id }}) + end # COMMENTED OUT THE DIRECT CONNECTION HERE TO Users to prevent assignment of users without an access_level specified (currently defaults to creator) # has_many :users, through: :roles diff --git a/app/views/answers/_new_edit.html.erb b/app/views/answers/_new_edit.html.erb index 0aa8384..95de7fb 100644 --- a/app/views/answers/_new_edit.html.erb +++ b/app/views/answers/_new_edit.html.erb @@ -11,11 +11,11 @@ <% end %>
<% if question.option_based? %> - <%= render(partial: 'questions/new_edit_question_option_based', locals: { f: f, question: question, answer: answer, readonly: @readonly }) %> + <%= render(partial: 'questions/new_edit_question_option_based', locals: { f: f, question: question, answer: answer, readonly: readonly }) %> <% elsif question.question_format.textfield?%> - <%= render(partial: 'questions/new_edit_question_textfield', locals: { f: f, question: question, answer: answer, readonly: @readonly }) %> + <%= render(partial: 'questions/new_edit_question_textfield', locals: { f: f, question: question, answer: answer, readonly: readonly }) %> <% elsif question.question_format.textarea? %> - <%= render(partial: 'questions/new_edit_question_textarea', locals: { f: f, question: question, answer: answer, readonly: @readonly }) %> + <%= render(partial: 'questions/new_edit_question_textarea', locals: { f: f, question: question, answer: answer, readonly: readonly }) %> <% end %> <% if !readonly && question.annotations.where(type: Annotation.types[:example_answer]).any? %> diff --git a/app/views/answers/_status.html.erb b/app/views/answers/_status.html.erb index 27c8e48..8828c40 100644 --- a/app/views/answers/_status.html.erb +++ b/app/views/answers/_status.html.erb @@ -1,8 +1,9 @@ - -<% if answer.updated_at.present? %> + +<% if answer.is_valid? && answer.user.present? %>

<%= _('Answered')%> <%= _(' by')%> <%= answer.user.name %>

diff --git a/app/views/phases/_edit_plan_answers.html.erb b/app/views/phases/_edit_plan_answers.html.erb index d4b142e..27bc304 100644 --- a/app/views/phases/_edit_plan_answers.html.erb +++ b/app/views/phases/_edit_plan_answers.html.erb @@ -58,11 +58,9 @@ answers = question.plan_answers(@plan.id) if answers.present? answer = answers.first + answer.user_id = current_user.id else - answer = Answer.new({plan_id: @plan.id, question_id: question.id, user_id: current_user.id }) - if question.default_value.present? - answer.text = question.default_value - end + answer = Answer.new({ plan: @plan, question: question, user: current_user, text: question.default_value }) end %>
@@ -70,7 +68,9 @@
" class="answer-locking">
- <%= render(partial: 'answers/new_edit', locals: { question: question, answer: answer, readonly: @readonly }) %> +
" class="answer-form"> + <%= render(partial: 'answers/new_edit', locals: { question: question, answer: answer, readonly: @readonly }) %> +
"> <%= render(partial: 'answers/status', locals: { answer: answer }) %>
diff --git a/lib/assets/javascripts/application.js b/lib/assets/javascripts/application.js index 5a774c7..673b5fe 100644 --- a/lib/assets/javascripts/application.js +++ b/lib/assets/javascripts/application.js @@ -5,7 +5,7 @@ import './utils/tooltipHelper'; // Page specific JS -import './views/answers/status'; +import './views/answers/edit'; import './views/contacts/new'; import './views/devise/invitations/edit'; import './views/devise/passwords/edit'; diff --git a/lib/assets/javascripts/views/answers/edit.js b/lib/assets/javascripts/views/answers/edit.js new file mode 100644 index 0000000..f7d8f25 --- /dev/null +++ b/lib/assets/javascripts/views/answers/edit.js @@ -0,0 +1,160 @@ +import { + isObject, + isNumber, + isString } from '../../utils/isType'; +import { Tinymce } from '../../utils/tinymce'; +import debounce from '../../utils/debounce'; +import TimeagoFactory from '../../utils/timeagoFactory'; + +$(() => { + const editorClass = 'tinymce_answer'; + const showSavingMessage = jQuery => jQuery.closest('.question-form').find('.saving-message').show(); + const hideSavingMessage = jQuery => jQuery.closest('.question-form').find('.saving-message').hide(); + const closestUnsavedMessage = jQuery => jQuery.closest('.question-form').find('.unsaved-message'); + const questionId = jQuery => jQuery.closest('.form-answer').attr('data-autosave'); + /* + * A map of debounced functions, one for each input, textarea or select change at any + * form with class form-answer. The key represents a question id and the value holds + * the debounced function for a given input, textarea or select. Note, this map is + * populated on demand, i.e. the first time a change is made at a given input, textarea + * or select within the form, a new key-value should be created. Succesive times, the + * debounced function should be retrieved instead. + */ + const debounceMap = {}; + const autoSaving = (jQuery) => { + if (jQuery.closest('.question-form').find('.answer-locking').html().length === 0) { + jQuery.closest('.form-answer').trigger('submit'); + } + }; + const doneCallback = (data, jQuery) => { + const form = jQuery.closest('form'); + // Validation for the data object received + if (isObject(data)) { + if (isObject(data.question)) { // Object related to question within data received + if (isNumber(data.question.id)) { + if (isString(data.question.answer_status)) { + $(`#answer-status-${data.question.id}`).html(data.question.answer_status); + TimeagoFactory.render($('time.timeago')); + } + if (isString(data.question.locking)) { // When an answer is stale... + detachEventHandlers(form); // eslint-disable-line no-use-before-define + // Reflesh views for this context + $(`#answer-locking-${data.question.id}`).html(data.question.locking); + $(`#answer-form-${data.question.id}`).html(data.question.form); + const newForm = $(`#answer-form-${data.question.id}`).find('form'); + attachEventHandlers(newForm); // eslint-disable-line no-use-before-define + } else { // When answer is NOT stale... + $(`#answer-locking-${data.question.id}`).html(''); + if (isNumber(data.question.answer_lock_version)) { + form.find('#answer_lock_version').val(data.question.answer_lock_version); + } + } + } + }// End Object related to question within data received + if (isObject(data.plan)) { // Object related to plan within data received + if (isString(data.plan.progress)) { + $('.progress').html(data.plan.progress); + } + } + if (isObject(data.section)) { // Object related to section within data received + if (isNumber(data.section.id)) { + if (isString(data.section.progress)) { + $(`.section-progress-${data.section.id}`).html(data.section.progress); + } + } + } + } + }; + const failCallback = (error, jQuery) => { + hideSavingMessage(jQuery); // TODO function expects elem not jQuery + closestUnsavedMessage(jQuery).html( // TODO function expects elem not jQuery + (isObject(error.responseJSON) && isString(error.responseJSON.detail)) ? + error.responseJSON.detail : error.statusText).show(); + }; + const changeHandler = (e) => { + const target = $(e.target); + const id = questionId(target); + if (!debounceMap[id]) { + debounceMap[id] = debounce(autoSaving); + } + debounceMap[id](target); + }; + const submitHandler = (e) => { + e.preventDefault(); + const target = $(e.target); + const form = target.closest('form'); + const id = questionId(target); + if (debounceMap[id]) { + // Cancels the delated execution of autoSaving + // (e.g. user clicks the button before the delay is met) + debounceMap[id].cancel(); + } + showSavingMessage(target); + const formElements = form.serializeArray(); + const answerId = formElements.find(el => el.name === 'answer[id]'); + + if (answerId) { + $.ajax({ + method: form.attr('method'), + url: form.attr('action'), + data: formElements, + }).done((data) => { + doneCallback(data, target); + }).fail((error) => { + failCallback(error, target); + }); + } + }; + const blurHandler = (editor) => { + const id = questionId($(`#${editor.id}`)); + $(`#${editor.id}`).val(editor.getContent()); // Updates target element of editor with its content + if (!debounceMap[id]) { + debounceMap[id] = debounce(autoSaving); + } + debounceMap[id]($(`#${editor.id}`)); + }; + const focusHandler = (editor) => { + const id = questionId($(`#${editor.id}`)); + if (debounceMap[id]) { + /* Cancels the delayed execution of autoSaving, either because user + * transitioned from an option_based question to the comment or + * because the target element triggered blur and focus before + * the delayed execution of autoSaving. + */ + debounceMap[id].cancel(); + } + }; + const formHandlers = ({ jQuery, attachment = 'off' }) => { + // Listeners to change and submit for a form + jQuery[attachment]('change', changeHandler); + jQuery[attachment]('submit', submitHandler); + }; + const editorHandlers = (editor) => { + // Listeners to blur and focus events for a tinymce instance + editor.on('Blur', () => blurHandler(editor)); + editor.on('Focus', () => focusHandler(editor)); + }; + /* + Detaches events from a specific form including its tinymce editor + @param { objecg } - jQueryForm to remove events + */ + const detachEventHandlers = (jQueryForm) => { + formHandlers({ jQuery: jQueryForm, attachment: 'off' }); + const tinymceId = jQueryForm.find(`.${editorClass}`).attr('id'); + Tinymce.destroyEditorById(tinymceId); + }; + /* + Attaches events for a specific form including its tinymce editor + @param { objecg } - jQueryForm to add events + */ + const attachEventHandlers = (jQueryForm) => { + formHandlers({ jQuery: jQueryForm, attachment: 'on' }); + const tinymceId = jQueryForm.find(`.${editorClass}`).attr('id'); + Tinymce.init({ selector: `#${tinymceId}` }); + editorHandlers(Tinymce.findEditorById(tinymceId)); + }; + TimeagoFactory.render($('time.timeago')); + Tinymce.init({ selector: `.${editorClass}` }); + Tinymce.findEditorsByClassName(editorClass).forEach(editorHandlers); + formHandlers({ jQuery: $('.form-answer'), attachment: 'on' }); +}); diff --git a/lib/assets/javascripts/views/answers/status.js b/lib/assets/javascripts/views/answers/status.js deleted file mode 100644 index 1be58c5..0000000 --- a/lib/assets/javascripts/views/answers/status.js +++ /dev/null @@ -1,122 +0,0 @@ -import { - isObject, - isNumber, - isString } from '../../utils/isType'; -import { Tinymce } from '../../utils/tinymce'; -import debounce from '../../utils/debounce'; -import TimeagoFactory from '../../utils/timeagoFactory'; - -$(() => { - /* - * Shows the closest saving-message HTML element within a question-form - * @param { Strin } selector - A valid CSS selector to look for - * @return { jQuery } - */ - const showSavingMessage = selector => $(selector).closest('.question-form').find('.saving-message').show(); - /* - * Retrieves the question id for the closest form-answer - * @param { String } selector - A valid CSS selector to look for - * @return { String } representing the question id for a given answer, otherwise undefined - */ - const questionId = selector => $(selector).closest('.form-answer').attr('data-autosave'); - /* - * A map of debounced functions, one for each input, textarea or select change at any - * form with class form-answer. The key represents a question id and the value holds - * the debounced function for a given input, textarea or select. Note, this map is - * populated on demand, i.e. the first time a change is made at a given input, textarea - * or select within the form, a new key-value should be created. Succesive times, the - * debounced function should be retrieved instead. - */ - const debounceMap = {}; - const autoSaving = (selector) => { - if ($(selector).closest('.question-form').find('.answer-locking').html().length === 0) { - $(selector).closest('.form-answer').trigger('submit'); - } - }; - // Initialises tinymce for any target element with class tinymce_answer - Tinymce.init({ selector: '.tinymce_answer' }); - // Listeners for change, blur and focus at any target element with class tinymce_answer - Tinymce.findEditorsByClassName('tinymce_answer').forEach((editor) => { - editor.on('Blur', () => { - const id = questionId(`#${editor.id}`); - $(`#${editor.id}`).val(editor.getContent()); // Updates target element of editor with its content - if (!debounceMap[id]) { - debounceMap[id] = debounce(autoSaving); - } - debounceMap[id]($(`#${editor.id}`)); - }); - editor.on('Focus', () => { - const id = questionId(`#${editor.id}`); - if (debounceMap[id]) { - /* Cancels the delayed execution of autoSaving, either because user - * transitioned from an option_based question to the comment or - * because the target element triggered blur and focus before - * the delayed execution of autoSaving. - */ - debounceMap[id].cancel(); - } - }); - }); - // Listener for input or select field - $('.form-answer').on('change', 'input, select', (e) => { - const id = questionId(e.target); - if (!debounceMap[id]) { - debounceMap[id] = debounce(autoSaving); - } - debounceMap[id]($(e.target)); - }); - // Listener for submit button - $('.form-answer').on('submit', (e) => { - e.preventDefault(); - const id = questionId(e.target); - if (debounceMap[id]) { - // Cancels the delated execution of autoSaving - // (e.g. user clicks the button before the delay is met) - debounceMap[id].cancel(); - } - showSavingMessage(e.target); - const formElements = $(e.target).closest('.form-answer').serializeArray(); - const answerId = formElements.find(el => el.name === 'answer[id]'); - if (answerId) { - // TODO centralise AJAX calls - $.ajax({ - method: 'PUT', - url: `/answers/${answerId}`, - data: formElements, - }).done((data) => { - // Validation for the data object received - if (isObject(data)) { - if (isObject(data.question)) { // Object related to question within data received - if (isNumber(data.question.id)) { - if (isString(data.question.answer_status)) { - $(`#answer-status-${data.question.id}`).html(data.question.answer_status); // TODO check partial render of this view on the server - TimeagoFactory.render($('time.timeago')); - } - if (isString(data.question.locking)) { - $(`#answer-locking-${data.question.id}`).html(data.question.locking); - } - if (isNumber(data.question.answer_lock_version)) { - $(e.target).closest('.form-answer').find('#answer_lock_version').val(data.question.answer_lock_version); - } - } - } - if (isObject(data.plan)) { // Object related to plan within data received - if (isString(data.plan.progress)) { - $('.progress').html(data.plan.progress); - } - } - if (isObject(data.section)) { // Object related to section within data received - if (isNumber(data.section.id)) { - if (isString(data.section.progress)) { - $(`.section-progress-${data.section.id}`).html(data.section.progress); - } - } - } - } - }, () => { - // TODO adequate error handling for network error - }); - } - }); - TimeagoFactory.render($('time.timeago')); -}); diff --git a/test/functional/answers_controller_test.rb b/test/functional/answers_controller_test.rb index 372ed47..d9c5c53 100644 --- a/test/functional/answers_controller_test.rb +++ b/test/functional/answers_controller_test.rb @@ -10,7 +10,7 @@ scaffold_plan end - # PUT/PATCH /[:locale]/answer/[:id] + # PUT/PATCH /answer/[:id] # ---------------------------------------------------------- test "should be able to update an answer" do sign_in @user @@ -27,42 +27,24 @@ plan.reload referrer = "/#{FastGettext.locale}/plans/#{plan.id}/phases/#{question.section.phase.id}/edit" - - if format.option_based - - else - # Try creating one first - form_attributes = { - answer: {user_id: @user.id, - plan_id: plan.id, - question_id: question.id, - text: "#{format.title} Tester", - lock_version: 0} - } - - put_answer(Answer.new(), form_attributes, referrer) - - answer = Answer.find_by(user: @user, plan: plan, question: question) - assert_not answer.id.nil?, "expected the answer to have been created and for an id to be present after creating a #{format.title} question!" - - # Try editing it - form_attributes = { - answer: {id: answer.id, - user_id: answer.user.id, - plan_id: answer.plan.id, - question_id: answer.question.id, - text: "Tested", - lock_version: answer.lock_version} + + answer = Answer.find_by(plan: plan, question: question) + assert_not answer.id.nil?, "expected the answer to have been created and for an id to be present after creating a #{format.title} question!" + + # Try editing it + form_attributes = { + answer: {id: answer.id, + user_id: @user.id, + plan_id: answer.plan.id, + question_id: answer.question.id, + text: "Tested", + lock_version: answer.lock_version} } - put_answer(answer, form_attributes, referrer) - - answer.reload - - assert_not answer.id.nil?, "expected the answer to have been updated and for an id to be present after creating a #{format.title} question!" - assert_equal "Tested", answer.text, "expected the text to have been updated for a #{format.title} question!" - - end + put_answer(answer, form_attributes, referrer) + answer.reload + assert_not answer.id.nil?, "expected the answer to have been updated and for an id to be present after creating a #{format.title} question!" + assert_equal "Tested", answer.text, "expected the text to have been updated for a #{format.title} question!" end end diff --git a/test/integration/answer_locking_test.rb b/test/integration/answer_locking_test.rb index 6dfe674..00af733 100644 --- a/test/integration/answer_locking_test.rb +++ b/test/integration/answer_locking_test.rb @@ -17,7 +17,7 @@ end # ---------------------------------------------------------- - test 'user receives a lock notification if the answer was CREATED while they were working' do + test 'user receives not found when trying to save a non-existent answer' do userA = Answer.new(user: @plan.owner, plan: @plan, question: @question, text: "Initial answer - by UserA") @@ -27,30 +27,14 @@ # Signin as UserA and insert the new answer sign_in @plan.owner put answer_path(FastGettext.locale, userA, format: "json"), obj_to_params(userA.attributes) - assert_response :success + assert_response :not_found assert_equal "application/json", @response.content_type - updated = Answer.find_by(plan: @plan, question: @question) - assert_equal "Initial answer - by UserA", updated.text - assert_equal @plan.owner.id, updated.user_id - - # Make sure the answers/locking partial is NOT displayed - assert_not @response.body.include?(_('The following answer cannot be saved')), "expected there to be no lock error messaging" - assert @response.body.include?(_('Answered')) - assert @response.body.include?("#{_(' by')} #{@plan.owner.name}"), "expected the messaging to say the plan was updated by the plan owner" # Signin as UserB and try to insert the new answer but fail sign_in @collaborator put answer_path(FastGettext.locale, userB, format: "json"), obj_to_params(userB.attributes) - assert_response :success + assert_response :not_found assert_equal "application/json", @response.content_type - updated = Answer.find_by(plan: @plan, question: @question) - assert_equal "Initial answer - by UserA", updated.text - assert_equal @plan.owner.id, updated.user_id - - # Make sure the answer-notice IS displayed - assert @response.body.include?(_('The following answer cannot be saved')), "expected there to be lock error messaging" - assert @response.body.include?(_('since %{name} saved the answer below while you were editing. Please, combine your changes and then save the answer again.') % { name: @plan.owner.name}), "expected the messaging to STILL say the plan was updated by the plan owner" - assert @response.body.include?(_('Answered')), "expected the messaging to include the status" end # ---------------------------------------------------------- diff --git a/test/unit/plan_test.rb b/test/unit/plan_test.rb index b3ad047..f191860 100644 --- a/test/unit/plan_test.rb +++ b/test/unit/plan_test.rb @@ -64,23 +64,6 @@ answr = @plan.answer(q.id) assert_not answr.id.nil?, "expected the latest Answer" assert_equal "testing", answr.text, "expected the Answer returned to have the correct text" - - # Check an option based question - q = QuestionFormat.find_by(option_based: true).questions.first - p = Plan.create(template: q.section.phase.template, title: 'Testing an option based answer') - o = QuestionOption.find_by(question: q) - d = QuestionOption.create(question: q, text: 'default', number: 99, is_default: true) - a = Answer.create(plan: p, question: q, user: @creator, question_options: [o]) - - answr = p.answer(q.id, false) - - assert_not answr.id.nil?, "expected the Option Based Answer" - assert_equal [o], answr.question_options, "expected the Answer returned to have the correct options selected" - - # Make sure that default options are selected if creating a new Answer - a.destroy - answr = p.answer(q.id) - assert_equal [d], answr.question_options, "expected the Answer returned to have the correct options selected" end # --------------------------------------------------- @@ -196,10 +179,6 @@ test "checks that status returns the correct information" do q = 0 @template.phases.first.sections.map{|s| q += s.questions.count } - - @plan.answers << Answer.new(user: User.last, text: "testing status", - question: @template.phases.first.sections.first.questions.first) - hash = @plan.status # Expecting the hash to look something like this: @@ -219,7 +198,6 @@ # "space_used"=>30} assert_equal q, hash["num_questions"], "expected the number of questions to match" - assert_equal @plan.answers.count, hash["num_answers"], "expected the number of answers to match" @template.phases.first.sections.each do |s| assert_not hash["sections"][s.id].nil?, "expected section #{s.id} to be in sections portion"