Add warnings when deleting questions that are associated with routes #1663
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lfdebrux
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
2 times, most recently
from
0648d1a to
bfa33bc
Compare
thomasiles
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
from
bfa33bc to
32089fe
Compare
lfdebrux
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
from
32089fe to
735ef5b
Compare
lfdebrux
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
2 times, most recently
from
5cc8521 to
a09c4e9
Compare
lfdebrux
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
2 times, most recently
from
3ee5b11 to
28b9617
Compare
If the form resource object is not persisted then ActiveResource won't make an API request if `form.pages` is accessed.
Co-authored-by: hannahkc <[email protected]>
Design System Guidance says don't show both notification banner and error summary [[1]]. [1]: https://design-system.service.gov.uk/components/notification-banner#when-not-to-use-this-component
Co-authored-by: hannahkc <[email protected]>
Co-authored-by: hannahkc <[email protected]>
Co-authored-by: hannahkc <[email protected]>
Move logic around routing from the view to the controller for the page#delete action. The main aim is to reduce the repetition of the notification banner call in the view; to achieve this we rely on the consistency of the content, where the warning refers to up to two different questions; the question about to be deleted, and the question that "owns" the route (which depends on the kind of route involved). A future refactor might appropriately move this logic out of the controller to a service, or maybe something more generally useful like a model that expresses the relationships between conditions and routes, but before that we need to make this view work with cases where the page to be deleted is associated with more than two routes, so for now I think this is a good place to stop.
lfdebrux
force-pushed
the
ldeb-add-warning-when-deleting-page-with-routes
branch
from
28b9617 to
e45d4e3
Compare
|
DavidBiddle
reviewed
Jan 17, 2025
|
|
||
| describe "when page to delete is not associated with any routes" do | ||
| it "does not render a notification banner" do | ||
| expect(rendered).not_to have_css "govuk-notification-banner" |
There was a problem hiding this comment.
I think this is missing a dot which means it'll always pass:
Suggested change
| expect(rendered).not_to have_css "govuk-notification-banner" | |
| expect(rendered).not_to have_css ".govuk-notification-banner" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this pull request solve?
Trello card: https://trello.com/c/UThSZ8ga/2037-fix-behaviour-of-deleting-page-with-secondary-skip-route
We've decided that we want to delete routes when a question is deleted, rather than leave the form in an invalid state.
As part of this we will show a notification banner about routes associated with the question to form creators when they are on the page to confirm that they want to delete a question. The content of the banner varies depending on the route associated; see the commits for details.
Note that currently the routes are not always actually deleted just yet, there needs to be a change to forms-api for that which is in alphagov/forms-api#654.
Screenshots
Warning if the question is the start of a "route 1" route
https://admin.dev.forms.service.gov.uk/forms/12788/pages/15016/delete
Warning if the question is the end of a "route 1" route
https://admin.dev.forms.service.gov.uk/forms/12788/pages/15017/delete
Warning if the question is the end of a "route 2" route
https://admin.dev.forms.service.gov.uk/forms/12788/pages/15018/delete
Warning if the question is the start of a "route 2" route
https://admin.dev.forms.service.gov.uk/forms/12788/pages/15023/delete
Things to consider when reviewing