From a056b60fcedfef89b7f34b51cc00bc9d3970c6ad Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 25 Apr 2025 11:50:24 -0600 Subject: [PATCH 01/10] Refactor: small cleanup --- app/controllers/answers_controller.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 4445428b0d..48bcd4f1bb 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -20,19 +20,18 @@ def create_or_update begin p = Plan.find(p_params[:plan_id]) unless p.question_exists?(p_params[:question_id]) - # rubocop:disable Layout/LineLength render(status: :not_found, json: { - msg: format(_('There is no question with id %{question_id} associated to plan id %{plan_id} for which to create or update an answer'), question_id: p_params[:question_id], plan_id: p_params[:plan_id]) + msg: format(_('There is no question with id %{question_id} associated to ' \ + 'plan id %{plan_id} for which to create or update an answer'), + question_id: p_params[:question_id], plan_id: p_params[:plan_id]) }) - # rubocop:enable Layout/LineLength return end rescue ActiveRecord::RecordNotFound - # rubocop:disable Layout/LineLength render(status: :not_found, json: { - msg: format(_('There is no plan with id %{id} for which to create or update an answer'), id: p_params[:plan_id]) + msg: format(_('There is no plan with id %{id} for which to create or update an answer'), + id: p_params[:plan_id]) }) - # rubocop:enable Layout/LineLength return end q = Question.find(p_params[:question_id]) @@ -130,8 +129,6 @@ def create_or_update @plan.sections.each do |section| next if section.number < @section.number - # rubocop pointed out that these variables are not used - # n_qs, n_ans = check_answered(section, qn_data[:to_show], all_answers) this_section_info = { sec_id: section.id, no_qns: num_section_questions(@plan, section), From d5bde03933e6c83402194103ce9b9322a402760e Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 25 Apr 2025 12:49:28 -0600 Subject: [PATCH 02/10] Refactor: Address Style/GuardClause offense - This refactor addresses the Style/GuardClause offense. It also makes the code easier to read by replacing the large `if @answer.present?` block with the early `return unless @answer.present?` - The code comment has also been updated to correspond with the refactor. --- app/controllers/answers_controller.rb | 167 +++++++++++++------------- 1 file changed, 82 insertions(+), 85 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 48bcd4f1bb..d87627c1a8 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -80,100 +80,97 @@ def create_or_update end # rubocop:enable Metrics/BlockLength - # TODO: Seems really strange to do this check. If its false it returns an + # TODO: Seems really strange to do this check. If it's true it returns a # 200 with an empty body. We should update to send back some JSON. The # check should probably happen on create/update - # rubocop:disable Style/GuardClause - if @answer.present? - @plan = Plan.includes( - sections: { - questions: %i[ - answers - question_format - ] - } - ).find(p_params[:plan_id]) - @question = @answer.question - @section = @plan.sections.find_by(id: @question.section_id) - template = @section.phase.template - - # Get list of questions to be removed from the plan based on any conditional questions. - questions_remove_list_before_destroying_answers = remove_list(@plan) - all_question_ids = @plan.questions.pluck(:id) - - # Destroy all answers for removed questions - questions_remove_list_before_destroying_answers.each do |id| - Answer.where(question_id: id, plan: @plan).each do |a| - Answer.destroy(a.id) - end + return unless @answer.present? + + @plan = Plan.includes( + sections: { + questions: %i[ + answers + question_format + ] + } + ).find(p_params[:plan_id]) + @question = @answer.question + @section = @plan.sections.find_by(id: @question.section_id) + template = @section.phase.template + + # Get list of questions to be removed from the plan based on any conditional questions. + questions_remove_list_before_destroying_answers = remove_list(@plan) + all_question_ids = @plan.questions.pluck(:id) + + # Destroy all answers for removed questions + questions_remove_list_before_destroying_answers.each do |id| + Answer.where(question_id: id, plan: @plan).each do |a| + Answer.destroy(a.id) end - # Now update @plan after removing answers of questions removed from the plan. - @plan = Plan.includes( - sections: { - questions: %i[ - answers - question_format - ] - } - ).find(p_params[:plan_id]) - - # Now get list of question ids to remove based on remaining answers. - remove_list_question_ids = remove_list(@plan) - - qn_data = { - to_show: all_question_ids - remove_list_question_ids, - to_hide: remove_list_question_ids + end + # Now update @plan after removing answers of questions removed from the plan. + @plan = Plan.includes( + sections: { + questions: %i[ + answers + question_format + ] } + ).find(p_params[:plan_id]) - section_data = [] - @plan.sections.each do |section| - next if section.number < @section.number + # Now get list of question ids to remove based on remaining answers. + remove_list_question_ids = remove_list(@plan) - this_section_info = { - sec_id: section.id, - no_qns: num_section_questions(@plan, section), - no_ans: num_section_answers(@plan, section) - } - section_data << this_section_info - end + qn_data = { + to_show: all_question_ids - remove_list_question_ids, + to_hide: remove_list_question_ids + } - send_webhooks(current_user, @answer) - render json: { - qn_data: qn_data, - section_data: section_data, - 'question' => { - 'id' => @question.id, - 'answer_lock_version' => @answer.lock_version, - 'locking' => if @stale_answer - render_to_string(partial: 'answers/locking', locals: { - question: @question, - answer: @stale_answer, - user: @answer.user - }, formats: [:html]) - end, - 'form' => render_to_string(partial: 'answers/new_edit', locals: { - template: template, - question: @question, - answer: @answer, - readonly: false, - locking: false, - base_template_org: template.base_org - }, formats: [:html]), - 'answer_status' => render_to_string(partial: 'answers/status', locals: { - answer: @answer - }, formats: [:html]) - }, - 'plan' => { - 'id' => @plan.id, - 'progress' => render_to_string(partial: 'plans/progress', locals: { - plan: @plan, - current_phase: @section.phase - }, formats: [:html]) - } - }.to_json + section_data = [] + @plan.sections.each do |section| + next if section.number < @section.number + this_section_info = { + sec_id: section.id, + no_qns: num_section_questions(@plan, section), + no_ans: num_section_answers(@plan, section) + } + section_data << this_section_info end - # rubocop:enable Style/GuardClause + + send_webhooks(current_user, @answer) + render json: { + qn_data: qn_data, + section_data: section_data, + 'question' => { + 'id' => @question.id, + 'answer_lock_version' => @answer.lock_version, + 'locking' => if @stale_answer + render_to_string(partial: 'answers/locking', locals: { + question: @question, + answer: @stale_answer, + user: @answer.user + }, formats: [:html]) + end, + 'form' => render_to_string(partial: 'answers/new_edit', locals: { + template: template, + question: @question, + answer: @answer, + readonly: false, + locking: false, + base_template_org: template.base_org + }, formats: [:html]), + 'answer_status' => render_to_string(partial: 'answers/status', locals: { + answer: @answer + }, 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 # rubocop:enable Metrics/AbcSize, Metrics/MethodLength # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity From 609d5df3b8a66d1a2adc739a9b8fde038a3de72d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 28 Apr 2025 11:59:06 -0600 Subject: [PATCH 03/10] Refactor plan fetching in AnswersController action - This refactor simply applies DRY principles to some of the plan fetching code in `AnswersController#create_or_update`. --- app/controllers/answers_controller.rb | 29 ++++++++++++--------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index d87627c1a8..5feabfc68c 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -85,14 +85,7 @@ def create_or_update # check should probably happen on create/update return unless @answer.present? - @plan = Plan.includes( - sections: { - questions: %i[ - answers - question_format - ] - } - ).find(p_params[:plan_id]) + @plan = fetch_plan_with_associations(p_params[:plan_id]) @question = @answer.question @section = @plan.sections.find_by(id: @question.section_id) template = @section.phase.template @@ -108,14 +101,7 @@ def create_or_update end end # Now update @plan after removing answers of questions removed from the plan. - @plan = Plan.includes( - sections: { - questions: %i[ - answers - question_format - ] - } - ).find(p_params[:plan_id]) + @plan = fetch_plan_with_associations(p_params[:plan_id]) # Now get list of question ids to remove based on remaining answers. remove_list_question_ids = remove_list(@plan) @@ -196,6 +182,17 @@ def permitted_params end # rubocop:enable Metrics/AbcSize + def fetch_plan_with_associations(plan_id) + Plan.includes( + sections: { + questions: %i[ + answers + question_format + ] + } + ).find(plan_id) + end + def check_answered(section, q_array, all_answers) n_qs = section.questions.count { |question| q_array.include?(question.id) } n_ans = all_answers.count { |ans| q_array.include?(ans.question.id) and ans.answered? } From efecb337795f999de6679576cd3fd7ee0a5683f3 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 28 Apr 2025 15:21:20 -0600 Subject: [PATCH 04/10] Refactor: Extract answer block to helper method - Cut and pasted existing answer transaction logic to new `handle_answer_transaction` helper method. - This refactor is intended to isolate the complex logic for the `Answer.transaction` block and to make the underlying `create_or_update` action easier to read and maintain. --- app/controllers/answers_controller.rb | 91 ++++++++++++++------------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 5feabfc68c..1371bd1f00 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -36,49 +36,8 @@ def create_or_update end q = Question.find(p_params[:question_id]) - # rubocop:disable Metrics/BlockLength - Answer.transaction do - args = p_params - # Answer model does not understand :standards so remove it from the params - standards = args[:standards] - args.delete(:standards) - - @answer = Answer.find_by!( - plan_id: args[:plan_id], - question_id: args[:question_id] - ) - authorize @answer - - @answer.update(args.merge(user_id: current_user.id)) - if args[:question_option_ids].present? - # Saves the record with the updated_at set to the current time. - # Needed if only answer.question_options is updated - @answer.touch - end - if q.question_format.rda_metadata? - @answer.update_answer_hash( - JSON.parse(standards.to_json), args[:text] - ) - @answer.save! - end - rescue ActiveRecord::RecordNotFound - @answer = Answer.new(args.merge(user_id: current_user.id)) - @answer.lock_version = 1 - authorize @answer - if q.question_format.rda_metadata? - @answer.update_answer_hash( - JSON.parse(standards.to_json), args[:text] - ) - end - @answer.save! - rescue ActiveRecord::StaleObjectError - @stale_answer = @answer - @answer = Answer.find_by( - plan_id: args[:plan_id], - question_id: args[:question_id] - ) - end - # rubocop:enable Metrics/BlockLength + # Execute transaction block to create, update, or handle stale answer object + handle_answer_transaction(p_params, q) # TODO: Seems really strange to do this check. If it's true it returns a # 200 with an empty body. We should update to send back some JSON. The @@ -163,6 +122,52 @@ def create_or_update private + def handle_answer_transaction(p_params, q) + # rubocop:disable Metrics/BlockLength + Answer.transaction do + args = p_params + # Answer model does not understand :standards so remove it from the params + standards = args[:standards] + args.delete(:standards) + + @answer = Answer.find_by!( + plan_id: args[:plan_id], + question_id: args[:question_id] + ) + authorize @answer + + @answer.update(args.merge(user_id: current_user.id)) + if args[:question_option_ids].present? + # Saves the record with the updated_at set to the current time. + # Needed if only answer.question_options is updated + @answer.touch + end + if q.question_format.rda_metadata? + @answer.update_answer_hash( + JSON.parse(standards.to_json), args[:text] + ) + @answer.save! + end + rescue ActiveRecord::RecordNotFound + @answer = Answer.new(args.merge(user_id: current_user.id)) + @answer.lock_version = 1 + authorize @answer + if q.question_format.rda_metadata? + @answer.update_answer_hash( + JSON.parse(standards.to_json), args[:text] + ) + end + @answer.save! + rescue ActiveRecord::StaleObjectError + @stale_answer = @answer + @answer = Answer.find_by( + plan_id: args[:plan_id], + question_id: args[:question_id] + ) + end + # rubocop:enable Metrics/BlockLength + end + # rubocop:disable Metrics/AbcSize def permitted_params permitted = params.require(:answer) From a840abb1415e64888e51b1fdc268f550e8f1f4bb Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 28 Apr 2025 15:43:04 -0600 Subject: [PATCH 05/10] Refactor answer transaction into separate methods These changes refactor the `Answer.transaction do` block into separate `create_answer`, `update_answer`, and `handle_stale_answer_error` methods. The changes are almost entirely a direct cut/paste from the original block into the corresponding methods. The only alteration is the prior `if q.question_format.rda_metadata?` check to `return unless q.question_format.rda_metadata?`. This early return check was auto-applied by rubocop. --- app/controllers/answers_controller.rb | 68 +++++++++++++++------------ 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 1371bd1f00..09ef2a6db8 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -123,7 +123,6 @@ def create_or_update private def handle_answer_transaction(p_params, q) - # rubocop:disable Metrics/BlockLength Answer.transaction do args = p_params # Answer model does not understand :standards so remove it from the params @@ -134,38 +133,49 @@ def handle_answer_transaction(p_params, q) plan_id: args[:plan_id], question_id: args[:question_id] ) - authorize @answer - - @answer.update(args.merge(user_id: current_user.id)) - if args[:question_option_ids].present? - # Saves the record with the updated_at set to the current time. - # Needed if only answer.question_options is updated - @answer.touch - end - if q.question_format.rda_metadata? - @answer.update_answer_hash( - JSON.parse(standards.to_json), args[:text] - ) - @answer.save! - end + update_answer(args, q, standards) rescue ActiveRecord::RecordNotFound - @answer = Answer.new(args.merge(user_id: current_user.id)) - @answer.lock_version = 1 - authorize @answer - if q.question_format.rda_metadata? - @answer.update_answer_hash( - JSON.parse(standards.to_json), args[:text] - ) - end - @answer.save! + create_answer(args, q, standards) rescue ActiveRecord::StaleObjectError - @stale_answer = @answer - @answer = Answer.find_by( - plan_id: args[:plan_id], - question_id: args[:question_id] + handle_stale_answer_error(args) + end + end + + def create_answer(args, q, standards) + @answer = Answer.new(args.merge(user_id: current_user.id)) + @answer.lock_version = 1 + authorize @answer + if q.question_format.rda_metadata? + @answer.update_answer_hash( + JSON.parse(standards.to_json), args[:text] ) end - # rubocop:enable Metrics/BlockLength + @answer.save! + end + + def update_answer(args, q, standards) + authorize @answer + + @answer.update(args.merge(user_id: current_user.id)) + if args[:question_option_ids].present? + # Saves the record with the updated_at set to the current time. + # Needed if only answer.question_options is updated + @answer.touch + end + return unless q.question_format.rda_metadata? + + @answer.update_answer_hash( + JSON.parse(standards.to_json), args[:text] + ) + @answer.save! + end + + def handle_stale_answer_error(args) + @stale_answer = @answer + @answer = Answer.find_by( + plan_id: args[:plan_id], + question_id: args[:question_id] + ) end # rubocop:disable Metrics/AbcSize From bed03ec2b442d6e01ec66271deadc9a7a0b074ec Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 28 Apr 2025 15:57:27 -0600 Subject: [PATCH 06/10] Address `Naming/MethodParameterName` offences - This change addresses the following rubocop offences: ``` app/controllers/answers_controller.rb:125:43: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long. def handle_answer_transaction(p_params, q) ^ app/controllers/answers_controller.rb:144:27: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long. def create_answer(args, q, standards) ^ app/controllers/answers_controller.rb:156:27: C: Naming/MethodParameterName: Method parameter must be at least 3 characters long. def update_answer(args, q, standards) ``` --- app/controllers/answers_controller.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 09ef2a6db8..8f9f22e121 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -122,7 +122,7 @@ def create_or_update private - def handle_answer_transaction(p_params, q) + def handle_answer_transaction(p_params, question) Answer.transaction do args = p_params # Answer model does not understand :standards so remove it from the params @@ -133,19 +133,19 @@ def handle_answer_transaction(p_params, q) plan_id: args[:plan_id], question_id: args[:question_id] ) - update_answer(args, q, standards) + update_answer(args, question, standards) rescue ActiveRecord::RecordNotFound - create_answer(args, q, standards) + create_answer(args, question, standards) rescue ActiveRecord::StaleObjectError handle_stale_answer_error(args) end end - def create_answer(args, q, standards) + def create_answer(args, question, standards) @answer = Answer.new(args.merge(user_id: current_user.id)) @answer.lock_version = 1 authorize @answer - if q.question_format.rda_metadata? + if question.question_format.rda_metadata? @answer.update_answer_hash( JSON.parse(standards.to_json), args[:text] ) @@ -153,7 +153,7 @@ def create_answer(args, q, standards) @answer.save! end - def update_answer(args, q, standards) + def update_answer(args, question, standards) authorize @answer @answer.update(args.merge(user_id: current_user.id)) @@ -162,7 +162,7 @@ def update_answer(args, q, standards) # Needed if only answer.question_options is updated @answer.touch end - return unless q.question_format.rda_metadata? + return unless question.question_format.rda_metadata? @answer.update_answer_hash( JSON.parse(standards.to_json), args[:text] From 3534ee055e00be5f22d5c8702dcee8b68ec2de69 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 28 Apr 2025 16:22:22 -0600 Subject: [PATCH 07/10] Replace iterated answer .destroy with .destroy_all -This change maintains the functional logic of the deletion of answers (based on questions to be removed) while also optimising its efficiency. --- app/controllers/answers_controller.rb | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 8f9f22e121..f5295923bf 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -12,7 +12,6 @@ class AnswersController < ApplicationController # `remote: true` in the
tag and just send back the ERB. # Consider using ActionCable for the progress bar(s) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength - # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def create_or_update p_params = permitted_params @@ -48,17 +47,11 @@ def create_or_update @question = @answer.question @section = @plan.sections.find_by(id: @question.section_id) template = @section.phase.template - - # Get list of questions to be removed from the plan based on any conditional questions. - questions_remove_list_before_destroying_answers = remove_list(@plan) all_question_ids = @plan.questions.pluck(:id) # Destroy all answers for removed questions - questions_remove_list_before_destroying_answers.each do |id| - Answer.where(question_id: id, plan: @plan).each do |a| - Answer.destroy(a.id) - end - end + # - remove_list(@plan) returns a list of question to be removed from the plan based on any conditional questions. + Answer.where(question_id: remove_list(@plan), plan: @plan).destroy_all # Now update @plan after removing answers of questions removed from the plan. @plan = fetch_plan_with_associations(p_params[:plan_id]) @@ -118,7 +111,6 @@ def create_or_update }.to_json end # rubocop:enable Metrics/AbcSize, Metrics/MethodLength - # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity private From b40a04c92ee9a6dd4af4fcbeb47f9e4c2feca84b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 29 Apr 2025 10:11:34 -0600 Subject: [PATCH 08/10] Remove unused variable --- app/javascript/src/utils/sectionUpdate.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/javascript/src/utils/sectionUpdate.js b/app/javascript/src/utils/sectionUpdate.js index 8b9568e357..fde85fb207 100644 --- a/app/javascript/src/utils/sectionUpdate.js +++ b/app/javascript/src/utils/sectionUpdate.js @@ -12,7 +12,6 @@ export const getQuestionDiv = (id) => $(`#answer-form-${id}`).closest('.question // Clear an answers for a given question id. export const deleteAllAnswersForQuestion = (questionid) => { - const answerFormDiv = $(`#answer-form-${questionid}`); const editAnswerForm = $(`#answer-form-${questionid}`).find('.form-answer'); editAnswerForm.find('input:checkbox').prop('checked', false); From b025dfe56de5fc23ad95667c5d487ccb515a4b73 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 29 Apr 2025 10:20:54 -0600 Subject: [PATCH 09/10] Fix locals comment in answers/_new_edit partial - This change updates/corrects the commented locals in `app/views/answers/_new_edit.html.erb`. - Prior to this change, there were conflicting comments listing the available locals. `plan` does not appear to be one of them. Also, `app/controllers/answers_controller.rb` reveals that `base_template_org` is one of them. --- app/views/answers/_new_edit.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/answers/_new_edit.html.erb b/app/views/answers/_new_edit.html.erb index 24e47f10cc..55d64393f8 100644 --- a/app/views/answers/_new_edit.html.erb +++ b/app/views/answers/_new_edit.html.erb @@ -1,6 +1,6 @@ -<%# locals: { template, question, answer, readonly, locking } %> +<%# locals: { template, question, answer, readonly, locking, base_template_org } %> <% q_format = question.question_format %> From 2d2ca05e17cd6a3b7543784674d246a8be217ad9 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 29 Apr 2025 11:32:57 -0600 Subject: [PATCH 10/10] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be5bc9d8d3..244cb22a90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Fix failing eslint workflow / upgrade `actions/checkout` & `actions/setup-node` to v3 [#3503](https://github.com/DMPRoadmap/roadmap/pull/3503) - Fix rendering of `confirm_merge` partial [#3515](https://github.com/DMPRoadmap/roadmap/pull/3515) - Remove Auto-Generated TinyMCE Skins and Add `public/tinymce/skins/` to `.gitignore` [#3466](https://github.com/DMPRoadmap/roadmap/pull/3466) +- Refactor `AnswersController#create_or_update` [#3519](https://github.com/DMPRoadmap/roadmap/pull/3519) ## v5.0.0