-
Notifications
You must be signed in to change notification settings - Fork 196
[WHIT-2788] Configurable document form objects WIP #10972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
ChrisBAshton
wants to merge
3
commits into
main
Choose a base branch
from
configurable-document-form-objects--all-in-one
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+1,526
−816
Conversation
This file contains hidden or 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
d205372 to
c41b916
Compare
ChrisBAshton
commented
Jan 7, 2026
| # end | ||
| # Is this test needed anymore? We think now that we're using flattened data structures, | ||
| # the above tests cover all addition/overwriting/deletion scenarios, and this 'nesting' | ||
| # test is now redundant. |
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this yesterday - even though we only have flattened top level properties now, we need to make sure that the values are retained, even if the values are nested structures. So make sure we have a test for something like
"foo" => { "bar" => { "baz" => 123 } }
e640534 to
dc80288
Compare
f22d3d4 to
76a00d5
Compare
This now replaces the need for all the `publishing_api_payload` methods defined on all the config driven blocks. There is no need to define a payload behaviour for the `default_object` block, as we always send a flat details hash to publishing API. Only "leaf" blocks will be included in the payload. We plan to maintain both implementations in the code to de-risk the release. At a later point, we will be removing all the `publishing_api_payload` methods and only use the new flow. All the tests covering the blocks' `publishing_api_payload` behaviour have been ported over.
76a00d5 to
25addb1
Compare
ChrisBAshton
commented
Jan 8, 2026
f83680d to
d7218de
Compare
This commit refactors the schema design for config-driven document types by separating concerns that were previously conflated under edit_screens. We have: - Updated the configurable document type schema with new properties such as `presenters` and `forms`. - Refactored the schema to include `attributes` specifying `type` and `validations`, rather than a hash of properties. This will help us separate the model schema from what we want to render, and send to publishing API, respectively. - Introduced an alternative configurable block factory builder. It builds uniquely named blocks and abandons the `type > format` nested structure we had in the `blocks` method. Since we now have an easy flat structure of computing the schema/publishing api payload, the `wrapper` object has been dropped in the new method `build_block` as it will be no longer needed. - Introduced a new way to compute standard edition presenter payload. We want to specify which attributes we're going to be including in the presenter, separately from defining the model attributes, or the forms to render. - Updated the rendering of standard edition with the new schema format - Updated the feature tests to support the new schema - We now "skip" nested objects when rendering; this means that whereas before the form name would have looked like `edition[block_content][wrapper_object][leaf_block]`, it now looks like `edition[block_content][leaf_block]`. - The way we compose the path when rendering child blocks in the new schema style has also changed. - Rendering the "required" labels require accessing the `validations`. Validations now live in a different segment of the schema; previously we were fetching them straight from the top-level `schema`; now the "schema" passed for rendering a block only contains `fields` with title/block/description. This was fixed by passing the array of required fields through any `default_object` block, so it's available down the tree. We compute the required fields values in the `ConfigurableDocumentType` model - `required_attributes` method. - The required attrs are now being passed for the images and the translations edit form. As-is behaviour was missing that too - Converted all StandardEdition schemas to use new format Co-authored-by: Olayinka Oladele <olayinka.oladele@digital.cabinet-office.gov.uk> Co-authored-by: Jamie Stamp <jamie.stamp@digital.cabinet-office.gov.uk> Co-authored-by: Chris Ashton <christopher.ashton@digital.cabinet-office.gov.uk> Co-authored-by: Laura Ghiorghisor <laura.ghiorghisor@digital.cabinet-office.gov.uk> Co-authored-by: Ryan Brown <ryan.brown@digital.cabinet-office.gov.uk>
e4b1b4c to
23f85a8
Compare
We tried testing setting an object value under the new 'flattened
attributes' structure, and the object was only ever being set as
`{}`. This is because we had special case handling for type "object"
which is no longer necessary - we blindly set the value to the
object, no type casting necessary.
We also no longer have the concept of a nested 'validations'
property on a nested attribute. All validations are declared at
the root level now and can incorporate multiple attributes, as
we're doing with topical event date validation:
https://github.com/alphagov/whitehall/blob/25addb1ce63d150731560b7a99efc89e731ecf21/app/models/configurable_document_types/topical_event.json#L60-L62
Note: There's a failing test in `test/unit/app/models/standard_edition_test.rb:154`. Need to discuss with the team if it's fine to get rid of the test since it looks redundant now.
23f85a8 to
56d815a
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.
What
WIP: exploring what it would look like to do the entire refactor in one PR, as opposed to PR 10965 which explores rolling it out in stages.
This PR refactors the schema design for config-driven document types by separating concerns that were previously conflated under
edit_screens. We now:settings.edit_screens, in favour of a new top-levelformshash. It describes things at the UI level: which fields appear on which forms.presentersfield, describing which field values get sent to Publishing API (and how to map them)schema.propertiesin favour ofschema.attributes: effectively a slimmed down version of the former.schema.validationsremains unchanged, but we no longer have 'nested' validation (e.g.schema.properties.duration.validations) - the 'nested' validation has moved up into theschema.validations(in this example,schema.validations.duration). This isn't represented in the overview below as we only had one example and it's more of a side-effect than a deliberate change.Overview of schema changes:
"schema": { - "properties": { - "body": { - "title": "Body", - "type": "string", - "format": "govspeak" - } + "attributes": { + "body": { + "type": "string", + } + } } + "forms": { + "documents": { + "fields": { + "body": { + "title": "Body", + "description": "The main content of the page", + "block": "govspeak" + } + } + } + }, + "presenters": { + "publishing_api": { + "body": "govspeak" + } +} "settings": { - "edit_screens": { - "document": ["body"] - }Why
Bunching up properties into 'edit screens' was a good first step in #10786, allowing us to update different block content fields in isolation on separate tabs. But it was too restrictive: we had no way of denoting which fields should be 'grouped' (e.g. start & end date fields for an overall 'duration') and also we were having to wrestle with wanting to present the fields in one way but present them differently when presenting to Publishing API. The previous
propertiesandedit_screensconcepts were conflating too much.The refactoring of the schemas we've done here should make it quicker and easier to iterate document types in future.
Jira: https://gov-uk.atlassian.net/browse/WHIT-2788
This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.