From f58c6e7beb919f96799a20148cff58dccbb448a5 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 8 Jan 2025 09:45:47 -0700 Subject: [PATCH 1/5] Fix ambiguity/Remove 1-to-many `.select()` columns "Fix ambiguity": - Prior to this commit, both 'phases.title' and 'sections.title' were included in the `.select()` clause. Since 'sections.title' was listed after 'phases.title', `phases.first.title`, would resolve to the corresponding `sections.title`. "Remove 1-to-many `.select()` columns": - A phase can have multiple sections, questions, and question_options, so including columns like `sections.title` in `.select()` is not appropriate. - No additional handling is needed since we're already using `.includes(sections: { questions: :question_options })` to preload the associations, and `phase.sections.each` etc. handles the associations properly within the view. --- .../org_admin/templates_controller.rb | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/app/controllers/org_admin/templates_controller.rb b/app/controllers/org_admin/templates_controller.rb index eb177fc149..d1de6c5e8b 100644 --- a/app/controllers/org_admin/templates_controller.rb +++ b/app/controllers/org_admin/templates_controller.rb @@ -2,7 +2,6 @@ module OrgAdmin # Controller that handles templates - # rubocop:disable Metrics/ClassLength class TemplatesController < ApplicationController include Paginable include Versionable @@ -109,8 +108,7 @@ def show .includes(sections: { questions: :question_options }) .order('phases.number', 'sections.number', 'questions.number', 'question_options.number') - .select('phases.title', 'phases.description', 'phases.modifiable', - 'sections.title', 'questions.text', 'question_options.text') + .select(:title, :description, :modifiable) unless template.latest? # rubocop:disable Layout/LineLength flash[:notice] = _('You are viewing a historical version of this template. You will not be able to make changes.') @@ -125,7 +123,7 @@ def show end # GET /org_admin/templates/:id/edit - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + # rubocop:disable Metrics/AbcSize def edit template = Template.includes(:org, :phases).find(params[:id]) authorize template @@ -135,12 +133,7 @@ def edit 'sections.number', 'questions.number', 'question_options.number') - .select('phases.title', - 'phases.description', - 'phases.modifiable', - 'sections.title', - 'questions.text', - 'question_options.text') + .select(:title, :description, :modifiable) if template.latest? render 'container', locals: { partial_path: 'edit', @@ -152,7 +145,7 @@ def edit redirect_to org_admin_template_path(id: template.id) end end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize # GET /org_admin/templates/new def new @@ -415,5 +408,4 @@ def get_referrer(template, referrer) end end end - # rubocop:enable Metrics/ClassLength end From 013f98d2f282b2d85b550adc82b232837b96a90a Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 8 Jan 2025 10:16:20 -0700 Subject: [PATCH 2/5] Refactor fetching of `template.phases` Lines 107-111 and 131-136 were identical. Created `def fetch_template_phases` to be called instead. --- .../org_admin/templates_controller.rb | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/app/controllers/org_admin/templates_controller.rb b/app/controllers/org_admin/templates_controller.rb index d1de6c5e8b..b58f007497 100644 --- a/app/controllers/org_admin/templates_controller.rb +++ b/app/controllers/org_admin/templates_controller.rb @@ -104,11 +104,7 @@ def show template = Template.find(params[:id]) authorize template # Load the info needed for the overview section if the authorization check passes! - phases = template.phases - .includes(sections: { questions: :question_options }) - .order('phases.number', 'sections.number', 'questions.number', - 'question_options.number') - .select(:title, :description, :modifiable) + phases = fetch_template_phases(template) unless template.latest? # rubocop:disable Layout/LineLength flash[:notice] = _('You are viewing a historical version of this template. You will not be able to make changes.') @@ -123,17 +119,11 @@ def show end # GET /org_admin/templates/:id/edit - # rubocop:disable Metrics/AbcSize def edit template = Template.includes(:org, :phases).find(params[:id]) authorize template # Load the info needed for the overview section if the authorization check passes! - phases = template.phases.includes(sections: { questions: :question_options }) - .order('phases.number', - 'sections.number', - 'questions.number', - 'question_options.number') - .select(:title, :description, :modifiable) + phases = fetch_template_phases(template) if template.latest? render 'container', locals: { partial_path: 'edit', @@ -145,7 +135,6 @@ def edit redirect_to org_admin_template_path(id: template.id) end end - # rubocop:enable Metrics/AbcSize # GET /org_admin/templates/new def new @@ -358,6 +347,16 @@ def template_export private + def fetch_template_phases(template) + template.phases + .includes(sections: { questions: :question_options }) + .order('phases.number', + 'sections.number', + 'questions.number', + 'question_options.number') + .select(:title, :description, :modifiable) + end + def template_params # TODO: For some reason the sample plans and funder links are sent outside # the context of the form as :template-links like this: From a28b65bd271ef2a3fc7b7b262e75b981b6a12808 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 8 Jan 2025 12:08:20 -0700 Subject: [PATCH 3/5] Fix and refactor ordering of template associations - Removed `.order()` as it was only applied to the main phases query and did not guarantee the order of associated records (e.g., `question_options`), which are loaded in separate queries. - `default_scope { order(number: :asc) }` already exists for the `Phase`, `Section`, and `Question` models, ensuring the desired ordering by default. - The `QuestionOption` model includes `scope :by_number, -> { order(:number) }`, which is now explicitly applied when fetching the `question_options` association to enable proper ordering. - `.order()` must've been auto-including `:id` in the `.select()` clause. Since it is required, we are explicitly adding it now. --- app/controllers/org_admin/templates_controller.rb | 6 +----- app/views/org_admin/phases/_phase.html.erb | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/controllers/org_admin/templates_controller.rb b/app/controllers/org_admin/templates_controller.rb index b58f007497..1ee0c4d221 100644 --- a/app/controllers/org_admin/templates_controller.rb +++ b/app/controllers/org_admin/templates_controller.rb @@ -350,11 +350,7 @@ def template_export def fetch_template_phases(template) template.phases .includes(sections: { questions: :question_options }) - .order('phases.number', - 'sections.number', - 'questions.number', - 'question_options.number') - .select(:title, :description, :modifiable) + .select(:id, :title, :description, :modifiable) end def template_params diff --git a/app/views/org_admin/phases/_phase.html.erb b/app/views/org_admin/phases/_phase.html.erb index 530fe9512a..406961fafa 100644 --- a/app/views/org_admin/phases/_phase.html.erb +++ b/app/views/org_admin/phases/_phase.html.erb @@ -55,7 +55,7 @@ <%= sanitize question.text %> <% if question.question_options.length > 0 %>
    - <% question.question_options.each do |option| %> + <% question.question_options.by_number.each do |option| %>
  • <%= option.text %>
    • <% end %>
From 6d7e4f87d0b49b1892a0f5d70d07170e23e20d6b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 8 Jan 2025 09:51:39 -0700 Subject: [PATCH 4/5] Refactor/optimise check for `question_options` - Replaced `.length > 0` with `.any?` to improve readability and performance. - `.any?` is more efficient for checking if a collection has elements, especially for ActiveRecord associations, as it doesn't require loading the entire collection into memory. --- app/views/org_admin/phases/_phase.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/org_admin/phases/_phase.html.erb b/app/views/org_admin/phases/_phase.html.erb index 406961fafa..2833efba31 100644 --- a/app/views/org_admin/phases/_phase.html.erb +++ b/app/views/org_admin/phases/_phase.html.erb @@ -53,7 +53,7 @@ <% section.questions.each do |question| %>
  • <%= sanitize question.text %> - <% if question.question_options.length > 0 %> + <% if question.question_options.any? %>
      <% question.question_options.by_number.each do |option| %>
    • <%= option.text %>
      • From 153b36bbaae838db55865bdfb45beb1f8297d8e0 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 8 Jan 2025 14:40:22 -0700 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c1959f48f..2937b4e7be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +- Fix and Refactor Querying of `template.phases` and Associations Within `OrgAdmin TemplatesController` [#3472](https://github.com/DMPRoadmap/roadmap/pull/3472) - Refactor Plan.deep_copy(plan) [#3469](https://github.com/DMPRoadmap/roadmap/pull/3469) - Fixed a bug in the deep copy of plans where the old identifier was being copied into the new plan. We now copy the generated id of the new plan to the identifier field. - Fixed bar chart click function in the Usage dashboard (GitHub issue #3443)