Create 'edit finder' form: "Filters and options" #2929
Draft
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
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
from
e79273a to
22bc4c8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
7 times, most recently
from
fa8cdcf to
fafecc7
Compare
| <%= | ||
| Diffy::Diff.new( | ||
| JSON.pretty_generate(@current_format.finder_schema.as_json).to_s, | ||
| JSON.pretty_generate(@proposed_schema.as_json).to_s, |
Check notice
Code scanning / Brakeman
Unescaped model attribute. Note
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
2 times, most recently
from
13060ce to
cec6c03
Compare
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
3 times, most recently
from
5b539fe to
31986bf
Compare
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
from
31986bf to
3e387e9
Compare
Display facets on the finder admin page.
Introduce route and form to modify the facets for a finder. We did attempt to have a nice "conditional reveal" on the filter options for multi/single select, but that doesn't play nicely with the "Add another" component, so we had to revert it. The code where we tried it out is in commit: fcd2e8f The other area where the form differs from the design is in the handling of the 'keys'/'values' (as opposed to 'labels') of the filter options. The result is the least worst solution to the problem of: "how do we retain the original filter option key for _existing_ filter options?". The design called for a simple textarea comma-separated list of filter option _labels_ (i.e. "Commercial - fixed wing"), with nowhere to specify the _key_ / _value_ ("commercial-fixed-wing"). Whilst in that example we can easily convert the label into the key, there are plenty of examples of existing filter options where the key is markedly different from the label, and thus submitting the form would be "lossy" and lead to a larger and unwanted diff. (Moreover it could lead to real problems if a dev were to go ahead and merge the diff). I did explore exposing the key / value pairs in dedicated input fields, and in theory could then turn the key into an `input type=hidden` so that it's not exposed to the user. This was explored in e8810b6 but the component doesn't currently allow nesting, so this isn't a viable way forward for now. What I've arrived at is I think a good compromise: it is the label that appears first (followed by the key), and there is no expectation for the user to edit the key nor to provide a key for any new filter options they add. The controller automatically defaults to a kebab-cased key derived from the label, if one is not provided up front. (Side note: I couldn't find a Rails native kebabcase method, to my surprise, so took the regex from https://jonathanyeong.com/changing-to-kebabcase/).
This uses the same dependency, and [copies the same CSS](https://github.com/alphagov/travel-advice-publisher/blob/767e4308ff2be37ca3497efd43eeb2105699561a/app/assets/stylesheets/diff.scss), as is used in Travel Advice Publisher. This will be used on the form confirmation screen in one of the subsequent commits.
Following the same pattern as the EmailAlert model introduced in ae63aa7, we've added a Facet model which provides a translation layer between the form inputs we expect to add in subsequent commits, and the eventual representation of the facet in the finder schema JSON. This logic started off life inline in the AdminController, which added bloat and complexity to the controller and would have been extremely difficult to test. The logic is now instead encapsulated in "Facet" and has an accompanying test suite, so we can be confident that form inputs are being correctly converted to their finder schema JSON form. See next commit for where this class is actually used.
Introduces page to confirm changes to the facets of a finder: - Hides diff / generated schema in a details component. - Have a shortened summary, don't bother attempting to summarise for humans, they can always click on view diff if needed. - Minimises the diff by sorting the properties as per the most common ordering in the existing JSON files (we don't want any diffs if someone submits without making any changes!) - Has summary list facets in the order they're defined (with the exception of deleted facets, which are a bit fiddly to slot into the correct spot. Jotting them onto the end is sufficient).
The form depends on JS, which we haven't enabled in our test suite yet. So I have a basic no-JS test for the bit of the form that works without JS (filter editing).
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
from
3e387e9 to
1c7cca0
Compare
ryanb-gds
reviewed
Jan 3, 2025
| @@ -75,4 +92,21 @@ def email_alert_params | |||
| :signup_link, | |||
| ) | |||
| end | |||
|
|
|||
| def facets_params | |||
| allowed_facets = %i[ | |||
There was a problem hiding this comment.
Suggesting slightly more specificity - allowed_facets to me suggests we're specifying what facets are allowed rather than what attributes a facet can have:
Suggested change
| allowed_facets = %i[ | |
| allowed_facet_params = %i[ |
Comment on lines
+55
to
+61
| def str_to_bool(str) | ||
| if str == "true" | ||
| true | ||
| elsif str == "false" | ||
| false | ||
| end | ||
| end |
There was a problem hiding this comment.
You could use Active Model for casting (docs) if you were up for changing your attribute accessors to full Active Model attribute definitions. I did do this for the finder schema and was a bit equivocal about the outcome, appreciate you may decide/have decided already it's not worth the overhead.
| end | ||
|
|
||
| def facet_allowed_values(values, type) | ||
| return nil if values.nil? || facet_types_that_allow_enum_values.exclude?(type) |
There was a problem hiding this comment.
I do wonder if we should be using validation here rather than ignoring invalid values, but can understand why we might choose not to. I can't remember what I did for the email facets in the end - I think I might have been lazy and assumed valid input because of the conditional behaviour making it difficult to provide invalid data, but of course that's not really very good because never trust client input/user could have JS disabled etc.
| hint: sanitize("Put each option on a new line. The underlying name for existing (live) options will appear in curly braces: please don't edit these, and don't add them for new options (they'll be created automatically later on in the process). Example:<br><strong>Pre-existing value {pre-existing-value}</strong><br><strong>New value</strong>"), | ||
| name: "facets[#{index}][allowed_values]", | ||
| rows: 5, | ||
| value: facet["allowed_values"]&.map { |opt| "#{opt["label"]} {#{opt["value"]}}" }&.join("\n"), |
There was a problem hiding this comment.
What's the value in showing requestors the machine value here? Seems like it would simplify our facet model code to omit it, and they're presumably going to be generated anyway using some kind of snake case/kebab case conversion.
Comment on lines
+17
to
+24
| previous_schema_facet_names = previous_schema.facets.map { |facet| facet["name"] } | ||
| proposed_schema_facet_names = schema.facets.map { |facet| facet["name"] } | ||
|
|
||
| summary_rows = proposed_schema_facet_names.map do |facet_name| | ||
| if (previous_schema_facet_names.include?(facet_name)) | ||
| old_facet_config = previous_schema.facets.find { |f| f["name"] == facet_name } | ||
| new_facet_config = schema.facets.find { |f| f["name"] == facet_name } | ||
| updated = !facet_name.in?(removed_facets) && !old_facet_config.nil? && old_facet_config != new_facet_config |
There was a problem hiding this comment.
Feels like there ought to be a less verbose way to do this comparison stuff, or maybe we just create a schema diff model and move it elsewhere. Outside the scope of this PR though
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.
Still to do:
Supersedes #2840. This PR:
Trello: https://trello.com/c/KNf2zNFI/3037-create-edit-finder-form-filters-and-options
Screenshot
Summary page:
Edit facets page:
Confirmation page:
With expanded diff:
With expanded schema:
Caveats
The form doesn't work without JS.
The question "Do publishers need to select an option...?" in the design is blocked on moving validation logic into the schema, so has been omitted for the time being.
All properties in the finder.jsonnet are supported, with the exception of the following barely-used filter properties:
show_option_select_filter(used only in licence_transactions.json)large(used only in licence_transactions.json)open_on_load(used only in licence_transactions.json)Follow these steps if you are doing a Rails upgrade.