DRY up the views for creating/editing specialist documents #2930
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
dry-views-and-validation
branch
9 times, most recently
from
6d144e2 to
ca0c9bf
Compare
ChrisBAshton
force-pushed
the
dry-views-and-validation
branch
3 times, most recently
from
6b6abf3 to
ab92846
Compare
ChrisBAshton
changed the title
ChrisBAshton
force-pushed
the
dry-views-and-validation
branch
2 times, most recently
from
c4b1a20 to
b7f0188
Compare
ryanb-gds
approved these changes
Jan 2, 2025
| facet.delete("specialist_publisher_properties") | ||
| facet | ||
| end | ||
| end |
There was a problem hiding this comment.
At some point I'm planning to pass this presenter our schema model instead of the raw content from the config file, at which point we will be able to move this logic there
| <% format_name = f.object.class.finder_schema.filter["format"].to_sym %> | ||
| <%= render layout: "shared/date_fields", locals: { f: f, field: field, format: format_name, label: field_config["name"] } do %> | ||
| <% end %> | ||
| <% elsif !field_config["allowed_values"] %> |
There was a problem hiding this comment.
I do wonder if we should consider giving every facet a type rather than relying on this sort of indirect way of knowing that we have a select input, but that would be one for another day
There was a problem hiding this comment.
Agreed! It felt like too big of a refactor to include here, but that is the direction I'd like to see us go in 👍
ChrisBAshton
force-pushed
the
validate-finder-schemas
branch
from
0a1654b to
7e6d2b0
Compare
This will encapsulate properties we don't intend to surface to Publishing API nor to Search API - they're used only in the Specialist Publisher interface. This hash presents an opportunity to DRY up our views and validation logic, by encapsulating as much data as possible in the finder schema. This should mean that minimal / zero edits will be required outside of the schema (in this repo at least) when editing fields.
There's no use hard-coding these layouts when we can use the finder schema to inform the layout. This commit is a POC applied to just the AAIB Report document type, as per the previous commit. In a future commit we'll apply this same principle en masse. Note that this uses the fancier searchable dropdown for "select one" field type as per the Alogorithmic Transparency Records form (see f0e914c). This is in contrast to the simpler dropdown (e.g. in AAIB Report form in https://github.com/alphagov/specialist-publisher/blob/a12db2c9643c2f3be8c34e25cdba6b2ad284b93c/app/views/metadata_fields/_aaib_reports.html.erb#L25-L32) because this is presumably the better / more modern / more consistent version of the "select one" single option dropdown. Also note that I considered driving the view using the `FORMAT_SPECIFIC_FIELDS` const to denote which fields to display, but in testing I found some inconsistencies between these and the finder schema, so let's make the latter the source of truth. Hopefully this way we can one day find a way of getting rid of the model consts altogether.
Cross-referencing with the schema in Publishing API: https://github.com/alphagov/publishing-api/blob/8d04f4b8629c9834e0d6cb2bcdc8c86ea77372eb/content_schemas/formats/shared/definitions/_specialist_document.jsonnet For every JSON file in lib/documents/schemas, for every "facets" hash, where there is a hash with an "allowed_values" property, we've added an additional property below "allowed_values": ``` "specialist_publisher_properties": { "select": "multiple" } ``` It will be `"select": "multiple"` or `"select": "one"`, depending on the corresponding config in publishing-api. If the corresponding config is `type: "array"`, then select multiple, but if it's just `type: "string"`, then select one. This follows the example in aaib_reports.json where I applied this in an earlier commit. Note that at the time of this commit, not every finder is using the new specialist publisher properties config, but there's no harm in getting the properties in ready, and they do help to document the intended behaviour for each field.
These tests eliminate the risk that a facet with "allowed_values" is missing a corresponding "specialist_publisher_properties" property (which is depended on by the shared view), and also eliminate the risk of said "specialist_publisher_properties" property having an invalid value.
These are the specialist document types that we were able to have pointing to the new generic shared template, with little or no change in how the form is rendered. It also meant we were able to delete a "dropdown.html.erb" partial which is no longer being referenced anywhere. The handful of views that we haven't touched yet are a bit more bespoke and will be tackled in subsequent commits.
Switching to the generic form template does lead to some small differences in the rendered form - nothing big enough to worry about fixing on a per-finder basis. It's still clear to the publisher what each form field should do, and it's easy for us to change (via the finder schema) should we need to. What we have now is more consistency between what's in the schema, and what's in the form the publisher sees.
We're going to tackle hidden_indexable_content in the next few commits. To do that, we want to retain the existing behaviour, which is that hidden_indexable_content facets aren't represented in the finder content item.
This was omitted from the schemas, instead being declared only in the FORMAT_SPECIFIC_FIELDS const in the relevant models (e.g. https://github.com/alphagov/specialist-publisher/blob/ec317d198ea68cc779cca7b0b85b2a3a05b13788/app/models/asylum_support_decision.rb#L10). This was to stop the facet from being included in the finder content item, but we can now achieve the same thing by using the "specialist_publisher_properties" configuration. Moving the field definitions into the finder schemas means we'll be able to delete all of the views for these and use the shared view instead (treating it as a common text input like any other field). NB, the field is a hard-coded concept in several places: 1. Publishing API: https://github.com/alphagov/publishing-api/blob/303baf6b3f7b083722043054e6af94b0c3b50323/content_schemas/formats/shared/definitions/_specialist_document.jsonnet#L343-L345 2. Search API: https://github.com/alphagov/search-api/blob/7b823c6e369fd27cd5f8f9d80d9095542b420c44/lib/govuk_index/presenters/indexable_content_presenter.rb#L34 3. Specialist Publisher: https://github.com/alphagov/specialist-publisher/blob/58b5b5ea0d5fb34c3de56f43a1264b7effa90012/app/presenters/email_alert_presenter.rb#L36
See previous commits. Now that the hidden_indexable_content field has been added to the finder schemas themselves, there's nothing unqiue about the views for these finder publishing UIs, so they can start using the shared view. Note that hidden_indexable_content fields tend to have a lot of text input (see https://www.gov.uk/api/content/asylum-support-tribunal-decisions/aw-and-am-and-bg-v-secretary-of-state-home-office-as-slash-23-slash-11-slash-45804-and-as-slash-23-slash-11-slash-45824-and-as-slash-23-slash-11-slash-45796 for an example). So I've swapped out the input type for text, from `input type="text"` to an adjustable `textarea`. This should have a negligible impact elsewhere - publishers can still just put a little bit of text in rather than a lot - but this also enables publishers to continue to use hidden_indexable_content without restriction. Moreover, these forms aren't yet on the Design System - so let's not worry too much about how they look. We can revisit this properly when we migrate to the new design system.
31 out of 36 views now use the shared "specialist_document_form.html.erb" view exclusively. These remaining five views can't yet use the shared form (for a variety of reasons), though in the case of the "flood and coastal erosion risk management" finder I was able to pull in the shared view for the main bulk of the form, and then there's just a small bit of proprietary code underneath. These five views now have TODO comments to explore later. It would be great to eliminate all of the edge cases and have everything using the shared view. I can't afford to spend any more time on these edge cases at present.
ChrisBAshton
force-pushed
the
dry-views-and-validation
branch
from
b7f0188 to
a0a5c5d
Compare
|
(Rebased against main) |
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.
Best reviewed commit by commit. This PR:
specialist_publisher_propertieshash to facets in thelib/documents/schemasfolder, where they have anallowed_valuesproperty. This is so that we can describe, in the schema, whether said facet should allow multiple choices, or only one.omit_from_finder_content_itemproperty, which indicates whether or not the facet should be stripped from the finder configuration when presented to publishing API. This allows us to strip away a load of hardcoded logic related to "hidden indexable content".specialist_publisher_propertiesproperty is stripped out when publishing the finder schema itself, so no changes are required downstream to Publishing API / Search API.)Result
With this merged, it should make the work to request edits to facets/options much more effective, as, without this PR, adding or deleting facets would still require a developer to edit the corresponding view. With this PR, that should no longer be needed - they should be able to copy and paste the generated schema and voila, that's the Specialist Publisher part done±! (Leave aside Publishing API and Search API for the minute...)
± I mean, there's always something else to sort out in Specialist Publisher. In this case, FORMAT_SPECIFIC_FIELDS, validation, etc, might mean dev work is still required. There's a card up next to make the validation logic schema-driven which should tackle a lot of that.
Once we have every form using the same shared view, there is a real opportunity to massively DRY up our test coverage too. We have a lot of tests doing much the same thing: testing that a particular type of specialist document can be created. This may have been necessary in a world where every part of that specialist document workflow was bespoke, but once they're using the same form, we can reasonably just test the creation of one specialist document and assume the rest are also fine.
Screenshots
Trello: https://trello.com/c/KijYVpyo/3290-dry-up-specialist-publisher-views
Follow these steps if you are doing a Rails upgrade.