Skip to content

Commit

Permalink
Add warning when user is about to delete condition with secondary skip
Browse files Browse the repository at this point in the history
Currently (when branching is enabled) deleting a skip route for a
question that has a secondary skip route puts the form in a state where
there is a route with an error that can’t be deleted.

We do want users to be able to delete individual routes for a question.

So we’ve agreed that we will delete the secondary skip for a question
when deleting any other routes for that question. And we’ll let the user
know before the delete a route, with a warning.

This commit adds the warning; note that the deletion will take place in
forms-api, see alphagov/forms-api#686. To enable this we add a method
`#has_secondary_skip?` to the DeleteConfirmationInput object... I'm not
sure if this is the best place for this, but it'll do for now.
  • Loading branch information
lfdebrux committed Feb 6, 2025
1 parent f49001e commit da2d5fb
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 3 deletions.
14 changes: 13 additions & 1 deletion app/input_objects/pages/delete_condition_input.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ def submit
def goto_page_question_text
return I18n.t("page_conditions.check_your_answers") if goto_page_id.nil? && record.skip_to_end

FormRepository.pages(form).filter { |p| p.id == goto_page_id }.first&.question_text
pages.filter { |p| p.id == goto_page_id }.first&.question_text
end

def has_secondary_skip?
check_page = pages.find(proc { raise "Cannot find page with id #{check_page_id}" }) { it.id == check_page_id }
page_conditions_service = PageConditionsService.new(form:, pages:, page: check_page)
page_conditions_service.check_conditions.any? { it != record && it.routing_page_id != it.check_page_id }
end

private

def pages
@pages ||= FormRepository.pages(form)
end
end
4 changes: 4 additions & 0 deletions app/views/pages/conditions/delete.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
<%= form_with(model: delete_condition_input, url: delete_condition_path(delete_condition_input.form.id, delete_condition_input.page.id, delete_condition_input.record.id), method: :delete) do |f| %>
<% if delete_condition_input.errors.any? %>
<%= f.govuk_error_summary %>
<% elsif delete_condition_input.has_secondary_skip? %>
<%= govuk_notification_banner(title_text: t("banner.default.title")) do |banner| %>
<% banner.with_heading(text: t(".any_other_answer_warning")) %>
<% end %>
<% end %>

<h1 class="govuk-heading-l">
Expand Down
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,9 @@ en:
your_form_is_live: Your form is live
pages:
answer_settings: Answer settings
conditions:
delete:
any_other_answer_warning: If you delete this route, the route for any other answer will also be deleted
delete:
notification_banner:
end_of_route:
Expand Down
30 changes: 30 additions & 0 deletions spec/input_objects/pages/delete_condition_input_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,34 @@
end
end
end

describe "#has_secondary_skip?" do
context "when the condition does not have a secondary skip condition" do
subject(:has_secondary_skip?) { delete_condition_input.has_secondary_skip? }

it { is_expected.to be false }
end

context "when the condition has a secondary skip condition" do
subject(:has_secondary_skip?) { delete_condition_input.has_secondary_skip? }

let(:condition) do
condition = build :condition, id: 1, routing_page_id: start_of_branches.id, check_page_id: start_of_branches.id, answer_value: "Wales", goto_page_id: start_of_second_branch.id
start_of_branches.routing_conditions << condition
condition
end

let(:start_of_branches) { pages.first }
let(:end_of_first_branch) { pages.second }
let(:start_of_second_branch) { pages.third }
let(:end_of_branches) { pages.last }

before do
secondary_skip_condition = build :condition, id: 2, routing_page_id: end_of_first_branch.id, check_page_id: start_of_branches.id, answer_value: nil, goto_page_id: end_of_branches.id
end_of_first_branch.routing_conditions << secondary_skip_condition
end

it { is_expected.to be true }
end
end
end
2 changes: 1 addition & 1 deletion spec/requests/pages/conditions_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@
end

describe "#delete" do
let(:condition) { build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: 3 }
let(:condition) { build :condition, id: 1, routing_page_id: selected_page.id, check_page_id: selected_page.id, answer_value: "Wales", goto_page_id: 3 }

before do
selected_page.routing_conditions = [condition]
Expand Down
52 changes: 51 additions & 1 deletion spec/views/pages/conditions/delete.html.erb_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
describe "pages/conditions/delete.html.erb" do
let(:delete_condition_input) { Pages::DeleteConditionInput.new(form:, page:, record: condition) }
let(:form) { build :form, :ready_for_routing, id: 1 }
let(:condition) { build :condition, id: 1, routing_page_id: 1, check_page_id: 1, answer_value: "Wales", goto_page_id: pages.last.id }
let(:condition) { build :condition, id: 1, routing_page_id: pages.first.id, check_page_id: pages.first.id, answer_value: "Wales", goto_page_id: pages.last.id }
let(:secondary_skip_condition) { nil }
let(:pages) { form.pages }
let(:page) { pages.first }

before do
page.position = 1
secondary_skip_condition
render template: "pages/conditions/delete", locals: { delete_condition_input: }
end

Expand Down Expand Up @@ -43,4 +45,52 @@
expect(rendered).to have_css ".govuk-error-summary"
end
end

context "when the condition does not have a secondary skip condition" do
let(:secondary_skip_condition) { nil }

it "does not render a warning" do
expect(rendered).not_to have_css ".govuk-notification-banner"
end
end

context "when the condition has a secondary skip condition" do
let(:condition) do
condition = build :condition, id: 1, routing_page_id: start_of_branches.id, check_page_id: start_of_branches.id, answer_value: "Wales", goto_page_id: start_of_second_branch.id
start_of_branches.routing_conditions << condition
condition
end

let(:secondary_skip_condition) do
condition = build :condition, id: 2, routing_page_id: end_of_first_branch.id, check_page_id: start_of_branches.id, answer_value: nil, goto_page_id: end_of_branches.id
end_of_first_branch.routing_conditions << condition
condition
end

let(:start_of_branches) { pages.first }
let(:end_of_first_branch) { pages.second }
let(:start_of_second_branch) { pages.third }
let(:end_of_branches) { pages.last }

it "renders a warning" do
expect(rendered).to have_css ".govuk-notification-banner", text: "If you delete this route, the route for any other answer will also be deleted"
end

context "when there is a validation error" do
let(:delete_condition_input) do
delete_condition_input = Pages::DeleteConditionInput.new(form:, page:, record: condition)
delete_condition_input.confirm = nil
delete_condition_input.validate
delete_condition_input
end

it "renders an error summary" do
expect(rendered).to have_css ".govuk-error-summary"
end

it "does not render a warning" do
expect(rendered).not_to have_css ".govuk-notification-banner", text: "If you delete this route, the route for any other answer will also be deleted"
end
end
end
end

0 comments on commit da2d5fb

Please sign in to comment.