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}
| facet["allowed_values"] = facet["allowed_values"].split("\n").map do |str| | ||
| label = str.match(/^(.+){/) | ||
| label = label.nil? ? str.strip : label[1].strip | ||
| value = str.match(/{(.+)}/) |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
Display facets on the finder admin page.
Introduce route and form to modify the facets for a finder.
Introduce page to confirm changes to the facets of a finder.
Need to think about how to handle keys/values of facet options. Could either: 1. Derive them from the label (requiring developer care if a facet key changes) 2. Include them in the form as a hidden input (for existing facets) and preserve them on submission. Needs some way of associating the hidden keys and the labels, given the current solution bundles them all into a textarea. 3. Include them in the form as a visible (but disabled?) form input, for full transparency. --- With that done, we'll still need to: - Add tests - Improve the confirmation/summary page to make it clearer what has changed - Add the grey background to each 'add another' form group, as per the design, to make it clear which facet all of the various form fields are associated with.
This seemed 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 isn't modelled in the finder schema - it is instead modelled in the specialist document model (e.g. https://github.com/alphagov/specialist-publisher/blob/ec317d198ea68cc779cca7b0b85b2a3a05b13788/app/models/cma_case.rb#L2). To retain this answer, we'd have to invent some sort of "editorial remarks" parameter to then inject into the Zendesk ticket. We can look into that in a future PR. Better still, we should move the presence check modelling into the finder schemas so that this can be handled without a special case.
Looks like it doesn't play nicely with the conditional reveal aspect of the radio component
ChrisBAshton
force-pushed
the
create-edit-finder-form-filters-and-options
branch
from
fa29d88 to
e053e68
Compare
We've had to stop using the conditional reveal. Shame
Make use of the new specialist_publisher_properties hash, and switch back to <select> now we no longer need a conditional reveal
Hide 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!)
...with the exception of deleted facets, which are a bit fiddly to slot into the correct spot. Jotting them onto the end is sufficient.
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.
Supersedes #2840.
Still to do:
Also need to write up issues/cards capturing:
Trello: https://trello.com/c/KNf2zNFI/3037-create-edit-finder-form-filters-and-options
Follow these steps if you are doing a Rails upgrade.