DRY up tests #2934
Draft
DRY up tests #2934
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
I noticed that there were several facet fields defined in the tribunal related finders, which aren't actually displayed in the publishing UI, nor are they referenced in Publishing API. (Presumably they were superseded by something else but never removed). This commit adds validation that checks for that case, cross-referencing every facet specified in the finder schema with a corresponding property on https://github.com/alphagov/publishing-api/blob/main/content_schemas/formats/shared/definitions/_specialist_document.jsonnet The closest equivalent we have to this kind of validation so far is the 'find_content_item_presenter_spec.rb'. I did consider adding this test into that, but we're really just sense-checking that the finder schemas are valid, not testing the `FinderContentItemPresenter`, so it didn't feel appropriate lumping it into that.
See previous commit
The _only_ finder that doesn't seem to follow the
`{finder_name}_metadata` pattern in Publishing API is the
flood and coastal erosion risk finder. I've added a specific
case to handle that, and a TODO for us to tidy it up later.
There was one property defined on the flood and coastal finder,
which is not referenced in Publishing API, so can be safely
removed.
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.
This was unnecessarily 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). There's no reason for that. 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 ...therefore there's little value in abstracting it into the "specialist_publisher_only" hash.
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.
Now that these finders are using the same shared form, there's little value in repeating the same tests across them all. I've left a subset of "creating a X specialist document" test files which all exhibit slightly different things - these should be tidied up and consolidated later.
Simplifies tests by: - Removing StatutoryInstrument access control tests, and AAIB access control tests. These tests didn't seem to be testing any behaviour different to the existing CMA access tests. - I understand the logic behind testing the 'inverse' of the CMA tests (i.e. that an AAIB editor can access AAIB and not CMA, to verify that the CMA cases aren't "failing open") but given we don't test access controls of ANY of the other document types, it doesn't make a lot of sense to retain anything other than the simple set of CMA access tests. - Moves the general permissions error to the top of the file. It is the simplest case, so should really come first. - Also adds a proper explanation of what's being tested.
ChrisBAshton
force-pushed
the
dry-views-and-validation
branch
3 times, most recently
from
b7f0188 to
a0a5c5d
Compare
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.
Related to https://trello.com/c/PArCm42y/3292-dry-up-specialist-publisher-validation, but may warrant its own card.
We can't really confidently delete all these tests until we've changed how we approach the validation logic in these models.
PS it may be a good idea to build on (or change) the schema test file added in 2933, to also have a quick sense-check loop that tries to create a document for every finder. (On the shared form, we can apply classes to the input fields to make it clear to the tests what example values we could provide. And thus the test loop would just visit the form, fill in the example values, submit, and assert that it was successful).
Follow these steps if you are doing a Rails upgrade.