Skip to content

Commit

Permalink
Merge pull request #1662 from alphagov/ldeb-fix-pages-routing-conditions
Browse files Browse the repository at this point in the history
Fix association between pages and routing conditions
  • Loading branch information
lfdebrux authored Dec 11, 2024
2 parents d8804e8 + e67f503 commit ed26c82
Show file tree
Hide file tree
Showing 12 changed files with 46 additions and 16 deletions.
2 changes: 1 addition & 1 deletion app/components/page_list_component/error_summary/view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion app/components/page_list_component/view.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/pages/secondary_skip_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
2 changes: 1 addition & 1 deletion app/input_objects/pages/secondary_skip_input.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion app/models/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -50,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

Expand All @@ -64,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
2 changes: 1 addition & 1 deletion app/presenters/route_summary_card_data_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions app/views/pages/routes/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@
<%= govuk_summary_list(**card) %>
<% end %>

<% if page.conditions.present? %>
<% if page.routing_conditions.present? %>
<ul class="govuk-list govuk-list--spaced">
<li>
<%= 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 %>
</li>
<li>
<%= govuk_link_to t("pages.go_to_your_questions"), form_pages_path(current_form.id) %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/pages/selection/type.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<% 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") %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/pages/type_of_answer.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<% 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)) %>
Expand Down
28 changes: 28 additions & 0 deletions spec/models/page_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions spec/views/pages/type_of_answer.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -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))
Expand All @@ -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")
Expand Down

0 comments on commit ed26c82

Please sign in to comment.