From 7b353bb89d799ed801f65278f2c73441533e421f Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 08:22:38 +0200 Subject: [PATCH 1/4] Add specs for association between Page and Condition A `Page` should have zero or more `Condition`s associated via the `routing_page_id` field on the condition record; we call these `routing_conditions`. This commit adds tests of this expected behaviour to document a bug in our model, where ActiveResource creates `Page::RoutingCondition` objects because we haven't told it about the association. --- spec/models/page_spec.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/models/page_spec.rb b/spec/models/page_spec.rb index f3be7bfa0..31e41b012 100644 --- a/spec/models/page_spec.rb +++ b/spec/models/page_spec.rb @@ -5,6 +5,34 @@ let(:page) { build :page, question_text: } let(:question_text) { "What is your address?" } + describe "associations" do + describe "#routing_conditions" do + let(:page) { build :page, id: 10, routing_conditions: build_list(:condition, 3, routing_page_id: 10) } + + it "has many routing conditions" do + expect(page.routing_conditions.length).to eq 3 + expect(page.routing_conditions).to all be_a Condition + expect(page.routing_conditions).to all have_attributes(routing_page_id: 10) + end + + context "when accessing a page through ActiveResource" do + let(:page_resource) { described_class.find(10, params: { form_id: 1 }) } + + before do + ActiveResource::HttpMock.respond_to do |mock| + mock.get "/api/v1/forms/1/pages/10", headers, page.to_json, 200 + end + end + + it "has many routing conditions" do + expect(page_resource.routing_conditions.length).to eq 3 + expect(page_resource.routing_conditions).to all be_a Condition + expect(page_resource.routing_conditions).to all have_attributes(routing_page_id: 10) + end + end + end + end + describe "#question_text" do [nil, ""].each do |question_text| it "is invalid given {question_text} question text" do From 41c4dfb815e989542276ead1704c30c7a0f4f8d6 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 08:28:05 +0200 Subject: [PATCH 2/4] Add one-to-many association between `Page`s and `Condition`s A `Page` should have zero or more `Condition`s associated via the `routing_page_id` field on the condition record; we call these `routing_conditions`. The response for a page from forms-api already contains the condition records, this commit just lets ActiveResource know about it so it uses the correct class. This fixes the failing test in the previous commit. --- app/models/page.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/page.rb b/app/models/page.rb index f9058dda6..5bae9e1d5 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -14,6 +14,7 @@ class Page < ActiveResource::Base ANSWER_TYPES_WITH_SETTINGS = %w[selection text date address name].freeze belongs_to :form + has_many :routing_conditions, class_name: :Condition validates :hint_text, length: { maximum: 500 } From 2a29f508c678ecddeecd61856575263c1b4aae66 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 08:33:48 +0200 Subject: [PATCH 3/4] Add deprecation warning to Page#conditions --- app/models/page.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/page.rb b/app/models/page.rb index 5bae9e1d5..b5b28bc1a 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -51,6 +51,7 @@ def show_selection_options end def conditions + ActiveSupport::Deprecation.new.warn("Prefer Page#routing_conditions to Page#conditions") routing_conditions.map { |routing_condition| Condition.new(routing_condition.attributes) } end From e67f503af662afaf0bd3494d821a8443d66da708 Mon Sep 17 00:00:00 2001 From: Laurence de Bruxelles Date: Wed, 11 Dec 2024 09:09:47 +0200 Subject: [PATCH 4/4] Use Page#routing_conditions instead of Page#conditions Now that Page#routing_conditions returns Condition objects as expected we can use that instead of Page#conditions; and then maybe in the future we can use that method for something else. This commit replaces all the uses of Page#condition I found using the deprecation warning from the previous commit. --- app/components/page_list_component/error_summary/view.rb | 2 +- app/components/page_list_component/view.rb | 2 +- app/controllers/pages/secondary_skip_controller.rb | 4 ++-- app/input_objects/pages/secondary_skip_input.rb | 2 +- app/models/form.rb | 2 +- app/models/page.rb | 2 +- app/presenters/route_summary_card_data_presenter.rb | 2 +- app/views/pages/routes/show.html.erb | 4 ++-- app/views/pages/selection/type.html.erb | 2 +- app/views/pages/type_of_answer.html.erb | 2 +- spec/views/pages/type_of_answer.html.erb_spec.rb | 8 ++++---- 11 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/components/page_list_component/error_summary/view.rb b/app/components/page_list_component/error_summary/view.rb index cfaa10e0d..72224466f 100644 --- a/app/components/page_list_component/error_summary/view.rb +++ b/app/components/page_list_component/error_summary/view.rb @@ -15,7 +15,7 @@ def error_object(error_name:, condition_id:, page_index:) end def conditions_with_page_indexes - @pages.map { |page| page.conditions.map { |condition| OpenStruct.new(condition:, page_index: page.position) } } + @pages.map { |page| page.routing_conditions.map { |condition| OpenStruct.new(condition:, page_index: page.position) } } .flatten end diff --git a/app/components/page_list_component/view.rb b/app/components/page_list_component/view.rb index f5532a166..ede5bd66c 100644 --- a/app/components/page_list_component/view.rb +++ b/app/components/page_list_component/view.rb @@ -75,7 +75,7 @@ def skip_condition_route_page_text(condition) # where index is the index of the condition in the array of conditions for # the page referenced by check_page_id def process_routing_conditions - all_form_conditions = @pages.flat_map(&:conditions).compact_blank + all_form_conditions = @pages.flat_map(&:routing_conditions).compact_blank all_form_conditions .group_by(&:check_page_id) diff --git a/app/controllers/pages/secondary_skip_controller.rb b/app/controllers/pages/secondary_skip_controller.rb index fe815e200..cfda28336 100644 --- a/app/controllers/pages/secondary_skip_controller.rb +++ b/app/controllers/pages/secondary_skip_controller.rb @@ -83,7 +83,7 @@ def ensure_branch_routing_feature_enabled end def ensure_page_has_skip_condition - unless page.conditions.any? { |c| c.answer_value.present? } + unless page.routing_conditions.any? { |c| c.answer_value.present? } redirect_to form_pages_path(current_form.id) end end @@ -97,6 +97,6 @@ def ensure_secondary_skip_blank end def secondary_skip_condition - @secondary_skip_condition ||= current_form.pages.flat_map(&:conditions).compact_blank.find { |c| c.secondary_skip? && c.check_page_id == page.id } + @secondary_skip_condition ||= current_form.pages.flat_map(&:routing_conditions).compact_blank.find { |c| c.secondary_skip? && c.check_page_id == page.id } end end diff --git a/app/input_objects/pages/secondary_skip_input.rb b/app/input_objects/pages/secondary_skip_input.rb index e11a37371..15753ceaf 100644 --- a/app/input_objects/pages/secondary_skip_input.rb +++ b/app/input_objects/pages/secondary_skip_input.rb @@ -69,7 +69,7 @@ def end_page_name end def answer_value - page.conditions.find { |rc| rc.answer_value.present? }.answer_value + page.routing_conditions.find { |rc| rc.answer_value.present? }.answer_value end def continue_to diff --git a/app/models/form.rb b/app/models/form.rb index 06050df81..62dafa7dc 100644 --- a/app/models/form.rb +++ b/app/models/form.rb @@ -113,7 +113,7 @@ def metrics_data private def has_routing_conditions - pages.filter { |p| p.conditions.any? }.any? + pages.filter { |p| p.routing_conditions.any? }.any? end def email_task_status_service diff --git a/app/models/page.rb b/app/models/page.rb index b5b28bc1a..3f97843f9 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -66,7 +66,7 @@ def show_optional_suffix? def self.qualifying_route_pages(pages) pages.filter do |page| page.answer_type == "selection" && page.answer_settings.only_one_option == "true" && - page.position != pages.length && page.conditions.empty? + page.position != pages.length && page.routing_conditions.empty? end end end diff --git a/app/presenters/route_summary_card_data_presenter.rb b/app/presenters/route_summary_card_data_presenter.rb index 3363bbf4a..075ca5c60 100644 --- a/app/presenters/route_summary_card_data_presenter.rb +++ b/app/presenters/route_summary_card_data_presenter.rb @@ -23,7 +23,7 @@ def summary_card_data end def all_routes - all_form_routing_conditions = pages.flat_map(&:conditions).compact_blank + all_form_routing_conditions = pages.flat_map(&:routing_conditions).compact_blank all_form_routing_conditions.select { |rc| rc.check_page_id == page.id } end diff --git a/app/views/pages/routes/show.html.erb b/app/views/pages/routes/show.html.erb index d9922bb14..48dd7c82d 100644 --- a/app/views/pages/routes/show.html.erb +++ b/app/views/pages/routes/show.html.erb @@ -20,10 +20,10 @@ <%= govuk_summary_list(**card) %> <% end %> - <% if page.conditions.present? %> + <% if page.routing_conditions.present? %>
  • - <%= govuk_button_link_to t("page_route_card.delete_route"), destroy_condition_path(form_id: current_form.id, page_id: page.id, condition_id: page.conditions.first.id), warning: true %> + <%= govuk_button_link_to t("page_route_card.delete_route"), destroy_condition_path(form_id: current_form.id, page_id: page.id, condition_id: page.routing_conditions.first.id), warning: true %>
  • <%= govuk_link_to t("pages.go_to_your_questions"), form_pages_path(current_form.id) %> diff --git a/app/views/pages/selection/type.html.erb b/app/views/pages/selection/type.html.erb index 4929e2bae..3aeb47a23 100644 --- a/app/views/pages/selection/type.html.erb +++ b/app/views/pages/selection/type.html.erb @@ -4,7 +4,7 @@
    - <% if @page.present? && @page.conditions.any? %> + <% if @page.present? && @page.routing_conditions.any? %> <% if @selection_type_input.need_to_reduce_options? %> <%= govuk_notification_banner(title_text: t("banner.default.title")) do |banner| %> <% banner.with_heading(text: t("selection_type.routing_and_reduce_your_options_combined_warning.heading"), tag: "h3") %> diff --git a/app/views/pages/type_of_answer.html.erb b/app/views/pages/type_of_answer.html.erb index 45c71e8f2..89e657f36 100644 --- a/app/views/pages/type_of_answer.html.erb +++ b/app/views/pages/type_of_answer.html.erb @@ -3,7 +3,7 @@
    - <% if @page.present? && @page.conditions.any? %> + <% if @page.present? && @page.routing_conditions.any? %> <%= govuk_notification_banner(title_text: t("banner.default.title")) do |banner| %> <% banner.with_heading(text: t("type_of_answer.routing_warning_about_change_answer_type_heading"), tag: "h3") %> <%= t("type_of_answer.routing_warning_about_change_answer_type_html", pages_link_url: form_pages_path(current_form)) %> diff --git a/spec/views/pages/type_of_answer.html.erb_spec.rb b/spec/views/pages/type_of_answer.html.erb_spec.rb index 030eeb30a..730a9c932 100644 --- a/spec/views/pages/type_of_answer.html.erb_spec.rb +++ b/spec/views/pages/type_of_answer.html.erb_spec.rb @@ -4,7 +4,7 @@ let(:form) { build :form, id: 1 } let(:type_of_answer_input) { build :type_of_answer_input } let(:answer_types) { Page::ANSWER_TYPES_EXCLUDING_FILE } - let(:page) { OpenStruct.new(conditions: [], answer_type: "number") } + let(:page) { OpenStruct.new(routing_conditions: [], answer_type: "number") } let(:question_number) { 1 } let(:is_new_page) { true } @@ -69,9 +69,9 @@ end context "when editing an existing" do - let(:page) { OpenStruct.new(conditions:, answer_type:) } + let(:page) { OpenStruct.new(routing_conditions:, answer_type:) } let(:answer_type) { "selection" } - let(:conditions) { [(build :condition)] } + let(:routing_conditions) { [(build :condition)] } it "displays a warning about routes being deleted if answer type changes" do expect(Capybara.string(rendered.html).find(".govuk-notification-banner__content").text(normalize_ws: true)) @@ -80,7 +80,7 @@ end context "when no routing conditions set" do - let(:conditions) { [] } + let(:routing_conditions) { [] } it "does not display a warning about routes being deleted if answer type changes" do expect(rendered).not_to have_selector(".govuk-notification-banner__content")