Warn delete options multiple branches#2853
Conversation
thomasiles
force-pushed
the
warn-delete-options-multiple-branches
branch
2 times, most recently
from
315e698 to
e2c350d
Compare
thomasiles
force-pushed
the
warn-delete-options-multiple-branches
branch
from
e2c350d to
fa29e8b
Compare
|
|
||
| # option_indexes is either :number or :answer_value | ||
| def selection_options_in_routes_banner(draft_question, selection_options, include_none_of_the_above, option_indexes: :number) | ||
| answer_values_from_options = selection_options.pluck(:name) + [Condition::NONE_OF_THE_ABOVE] |
There was a problem hiding this comment.
| answer_values_from_options = selection_options.pluck(:name) + [Condition::NONE_OF_THE_ABOVE] | |
| answer_values_from_options = selection_options.pluck(:value) + [Condition::NONE_OF_THE_ABOVE] |
There was a problem hiding this comment.
👍 I've changed this to use the selection_options_with_value method and the :value attribute rather than :name
| end | ||
|
|
||
| if options_in_routes.count == 1 && option_indexes == :number | ||
| option_index = selection_options.index { |option| option[:name] == options_in_routes.first.answer_value } + 1 |
There was a problem hiding this comment.
| option_index = selection_options.index { |option| option[:name] == options_in_routes.first.answer_value } + 1 | |
| option_index = selection_options.index { |option| option[:value] == options_in_routes.first.answer_value } + 1 |
| return { heading: "There is a route from ‘#{option_index}’", text: "If you remove or change this option, the route will be deleted. #{edit_routes_link}" } | ||
| end | ||
|
|
||
| if selection_options.length == options_in_routes.count + (include_none_of_the_above ? 1 : 0) |
There was a problem hiding this comment.
I couldn't get the "There are routes fro these options" banner to show with a question which doesn't include a 'none of the above' option, I think this line might need looking at again?
There was a problem hiding this comment.
Yes, I'd mucked up the order of this. I've fixed it and given it a name to hopefully make it clearer.
| let(:form) { create :form, :ready_for_routing } | ||
| let(:draft_question) { build :selection_draft_question, form_id: form.id, page_id: form.pages.first.id } | ||
| let(:selection_options) { (1..3).to_a.map { |i| { name: i.to_s } } } | ||
| let(:include_none_of_the_above) { false } |
There was a problem hiding this comment.
Pages::Selection::BaseOptionsInput#selected_none_of_the_above_option returns a string, not a boolean.
There was a problem hiding this comment.
good catch, I've changed the value being passed in to is_optional, which is a boolean.
When changing the options in a selection question, we need to remove any conditions which no longer have a valid answer value. We add a method which will only be called when the draft question is saved.
We'll need to check if draft question supports none of the above outside of the input object.
thomasiles
force-pushed
the
warn-delete-options-multiple-branches
branch
from
f3a3548 to
5c6b767
Compare
When changing options, we want to show a banner which tells the user if they are removing or changing an option which is in a route. The text for the banner varies depending on how many options are in routes. We refer to options in routes as either the option number or the option's answer value. The bulk options page uses the answer value, the regular options page uses the option number.
Add the banner to the normal options page.
Add the banner warning users about options in conditions to the bulk options page.
thomasiles
force-pushed
the
warn-delete-options-multiple-branches
branch
from
5c6b767 to
3f631ed
Compare
|
🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2853.admin.review.forms.service.gov.uk/ It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready For the sign in details and more information, see the review apps wiki page. |
|
I've made some changes and checked everything works as designed. I did look at moving the helper into the input object because they depend on input object methods. It didn't come out much clearer though because it still depended on path helpers and code to generate the link. It might be better as a presenter/service but I'm leaving it here as I'm not confident I'm making things much better by more fiddling. |
2 participants
Delete conditions which reference options which don't exist and add banner to options page about routes
Trello card: https://trello.com/c/vaePaWB5/3141-delete-routes-when-changing-selection-options-makes-it-invalid-when-multiple-branches-is-enabled
This PR adds a banner to the selection options page to warn the user that options are being used in routes. The banner changes the wording based on how many options are involved in routes and whether the "none of the above" is activated for the page.
When options are changed or removed, it removes any conditions which have answer_values which no longer have matching options.
Things to consider when reviewing