Spike - simple child pages (shared content ID) [WHIT-3520] - #11513
Spike - simple child pages (shared content ID) [WHIT-3520]#11513ChrisBAshton wants to merge 13 commits into
Conversation
Other fields on the page render like this, so why not default_string?
Sometimes we want a multi-line input that isn't Govspeak and should not have the 'inline preview' feature.
Until now, 'additional_routes' have only been used for publishing atom and RSS equivalents of pages. The base path prefix and the additional route have therefore been joined by a hardcoded `.`. In the next few commits we're going to publish additional routes for topical event about pages, i.e. the topical event page and a topical event 'about' page that shares its based path prefix but has a "/about" suffix. Without this commit, that would come out as something like "/topical/events/foo./about" - we don't want the '.'
Any StandardEdition document can publish to more than one route, by specifying a `settings.additional_routes` property. This will be an array of paths which are to be combined with the `settings.base_path_prefix` property to determine the full route.
Following the approach of Travel Advice Pages (TAP), we can use a single content item to serve multiple pages/routes. Example: https://www.gov.uk/api/content/foreign-travel-advice/afghanistan What's special about these routes: 1. More than one exact route is included in the payload (https://github.com/alphagov/travel-advice-publisher/blob/c285ea5cd2a364ca986c072baa0b246f94af00ed/app/presenters/edition_presenter.rb#L91-L99) 2. The contents of each child 'page' is included under `details.parts` (https://github.com/alphagov/travel-advice-publisher/blob/c285ea5cd2a364ca986c072baa0b246f94af00ed/app/presenters/edition_presenter.rb#L52) Frontend has some fairly [generic support](https://github.com/alphagov/frontend/blob/0a6b871bd125fd80d34a8f22f649d71ebed28f8c/app/models/concerns/parts.rb#L1) for 'parts' via a concern. In order for this to extend to Topical Events and their About pages, the [FlexiblePage model](https://github.com/alphagov/frontend/blob/46b2be223a029be5c161f572f4bccbaec5c7be1d/app/models/flexible_page.rb#L1) would need to include the Parts concern like the [TravelAdvice model](https://github.com/alphagov/frontend/blob/1f6acf73c6901ee6779b9351714488a39fc99705/app/models/travel_advice.rb#L2) does. On the Whitehall side, we just have to pass additional_routes, and find a way of passing 'parts' to the details hash. Our current abstraction for 'presenters' in the JSON configs is a bit too restrictive at the moment - it doesn't support passing parameters - so currently the generic 'parts' PayloadBuilder method has a hardcoded reference to 'about_page_parts'. To be production ready we'd have to iterate the JSON schema to allow for specifying said parameter in the config itself.
We can now define arbitrary routes (and their equivalent arbitrary parts) by simply declaring a different `"part": "/route"` property under the `forms`. In this case we only want one route and part - topical event about pages - but you can imagine how we'd extend this to support all possible Corporate Information Pages, for example.
b96317a to
bd18a63
Compare
lauraghiorghisor-tw
left a comment
There was a problem hiding this comment.
Good enough, I think.
Some concerns arounds why we now have to do custom things on the schema and payload builder and whether that means we have some limitations stemming from old decisions we've made around the use of the object block (that I do not remember).
| end | ||
| end | ||
|
|
||
| def parts(_attribute) |
There was a problem hiding this comment.
I don't know that I like injecting new flavours of things in the logic of these classes like this. The config driven approach becomes a bit un-hold-in-ones-head-able. If we expect the methods here to be mappers to keys that are attributes, let's try to follow that pattern.
There was a problem hiding this comment.
Plus, wouldn't the rest of the presenter logic also loop through the about field and add it to the payload?
There was a problem hiding this comment.
I think this is a case for an object block. Not sure whether we said we were doing those or not anymore 😅 It's a blur.
Or alternatively we can add an array of fields with a single option. And then it is truly obvious how that would extend for multiple pages (though add another is not exactly great for this case).
There was a problem hiding this comment.
Plus, wouldn't the rest of the presenter logic also loop through the about field and add it to the payload?
...no? By my understanding, everything in the presenter payload has to exist in the presenters.publishing_api.details property. If we drop parts: parts, then the parts aren't included in the payload.
"presenters": {
"publishing_api": {
"details": {
"body": "govspeak",
"social_media_links": "social_media_links",
"parts": "parts"
},
But to your first point - I agree. It would be much nicer if we could be more explicit in the presenter config, something like this:
"presenters": {
"publishing_api": {
"details": {
"body": { "_source": "body", "_processor": "govspeak" },
"social_media_links": { "_source": "social_media_links", "_processor": "social_media_links" },
"parts": {
"_sources": {
"title": "{ "_source": "about_title", "_processor": "raw" },
"summary": "{ "_source": "about_summary", "_processor": "raw" },
"body": "{ "_source": "about_body", "_processor": "string" },
}
}
},
...
}
},That would mean we could drop the parts presenter method and also drop the part_name thing that was only included to drop the namespacing when presenting.
(Have gone for underscore "_source" notation in case for whatever reason you'd have a page with a details.source, we don't want to conflict with that).
There was a problem hiding this comment.
Ah yes yes "everything in the presenter payload has to exist..." - this is correct 🤦🏻♀️
I think what I meant with the "keys that are attributes" was that - most of those payload builder methods have the block_content calling that attribute.
So since our form is already called "about", if we map about: parts in the presenter section in the schema, wouldn't that call our "parts" builder with an "about" attribute, and then we get the "part" as "about" without further work? It depends if the code would actually save this "about" block/form to the block_content correctly. Yeah, I am basically describing a nested, non-root, object. And I don't think that works anymore.
There was a problem hiding this comment.
What does this save like now, in the block_content? It doesn't wrap an "about" object over the fields, does it? 🤔 I imagine it would be block_content: { about_title: ..., about_summary etc}
There was a problem hiding this comment.
Have pushed up some commits changing the schema to use the default_object with an "about" namespace, rather than several flat "about_x" properties. Meant we can kill the parts method etc :)
| "block": "default_string", | ||
| "attribute_path": ["block_content", "about_title"], | ||
| "part_of": "about_page_parts", | ||
| "part": "/about", |
There was a problem hiding this comment.
I'd probably store the plain "about" here and inject the "/" where needed?
There was a problem hiding this comment.
Also, I find it odd that we have no mechanism to know where this block sits - like we shouldn't have to even do something like this. The tree traversal should give us this, and we should have the option to map this in the presenters section.
Is it that our interpretation of Path has now veered towards leaf nodes so much? Like, if we had an attribute_path that had the block's title, we could use that.
Something about you having to do this suggests the current implementation is lacking something. I just would not like to add extraordinary rules to what we already have, if possible.
There was a problem hiding this comment.
Hmmm true - perhaps we can infer this by setting something like "attribute_path": ["block_content", "about", "title"], instead of the nasty namespacing 🤔
There was a problem hiding this comment.
There definitely seems to be a pattern/rule? of only using leaves in that attribute_path array now.
We can explore some options when implementing this.
There was a problem hiding this comment.
Fixed in latest commits.
| } | ||
| } | ||
| }, | ||
| "about": { |
There was a problem hiding this comment.
but you can imagine how we'd extend this to support all possible Corporate Information Pages, for example.
It appears I cannot, in fact, imagine 😅
How would you extend this for multiple corporate information pages? Add all the types there as new forms, with their own fields, and part "/corporate-thing-blah"? 💀
There was a problem hiding this comment.
Yep, exactly! Or every corporate information page on one tab (like we have for Travel Advice pages):
"corporate_information_pages": {
"dynamic": true,
"label": "Corporate information pages",
"fields": {
"recruitment_summary": {
"title": "Recruitment page summary (leave blank if not needed)",
"block": "govspeak",
"attribute_path": ["block_content", "recruitment_summary"],
"part": "/recruitment",
"part_name": "summary",
"translatable": true
},
"recruitment_body": {
"title": "Recruitment page body (leave blank if not needed)",
"block": "govspeak",
"attribute_path": ["block_content", "recruitment_body"],
"part": "/recruitment",
"part_name": "body",
"translatable": true
},
"accessibility_statement_summary": {
"title": "Accessibility statement summary (leave blank if not needed)",
...etcThere was a problem hiding this comment.
That's really busy 😵💫
The array solution might work better, and then you can just let the user add... another.
There was a problem hiding this comment.
Less bad in latest commits (each 'part' would be its own nested default_object, and there are fewer properties needed, e.g. part_name/part). But agree it's still not great.
As we touched on in 1:1 this morning, not entirely convinced this would scale to corporate information pages now I know CIP pages have an Attachments tab requirement too.
There was a problem hiding this comment.
Though could very well scale to Travel Advice 🤔
This _almost_ works - but Publishing API rejects the save, because the parts[n].body is expected to be an array (of string and govspeak) but is in fact only a string. This also underlines the fact that we don't currently support any 'processing' on a given property within the default_object scope - we need to initialise `PublishingApi::PayloadBuilder::BlockContent.new(item)` from the 'MultipleParts' class. See next commit. Also had to drop the `summary` property since this isn't allowed by the schema. It expects only title, body and slug.
This commit does a few things: - Makes it possible to call conversion methods (such as `govspeak`) on nested fields (such as `about.title`) - Introduces a way of specifying a hardcoded value in the payload (in this case, a part's "slug") - Kills the `MultipleParts` payload builder as a result - no longer needed - Kills the 'fields_for_part' logic - Simplifies the 'parts' logic - Makes the `presenters.publishing_api.details` config more expressive - Introduces new `compiled_and_raw_govspeak` converter, as some schemas expect Whitehall to send both (in this case, when sending a 'body' for a 'part'). - Renames `govspeak` to `compiled_govspeak` to disambiguate - it otherwise read as though we were sending the raw GovSpeak.
580c165 to
67fb4e1
Compare
67fb4e1 to
02ec9c0
Compare
lauraghiorghisor-tw
left a comment
There was a problem hiding this comment.
LGTM overall. Still a bit unsure on the shape of the schema - particularly in the presenters bit. Would have been nice to be able to run the spike.
| "fields": { | ||
| "about": { | ||
| "block": "default_object", | ||
| "attribute_path": ["block_content", "about"], |
There was a problem hiding this comment.
What does the block_content save as, now, in wh? You seem to call block_content -> namespace -> field in the builder so I suppose the "about" is now stored in the block_content.
Sorry don't seem to be able to save an edition when I run the app - still getting pub api errors even with the branch checked out, not sure if that was meant to work now?
There was a problem hiding this comment.
Yep, it should work locally 🤔
It stores as:
block_content: {
about: {
title: ...,
body: ...,
}
}
| end | ||
|
|
||
| def parts | ||
| (presenter("publishing_api")["details"]["parts"] || []).map { |part| part["slug"]["hardcoded_value"] } |
There was a problem hiding this comment.
This parts method is now only called from the payload builder right?
There was a problem hiding this comment.
Indirectly - it's only called by the additional_routes method, which is called by the presenter only in determining what routes to send.
The actual content for the parts is now just using our standard payload methods (raw etc)
|
|
||
| mapping.each_with_object({}) { |(attribute, builder), details| | ||
| details[attribute.to_sym] = send(builder, attribute) | ||
| details[attribute.to_sym] = if builder.is_a?(Array) |
There was a problem hiding this comment.
For the implementation version:
- I'd probably rename some of these variables, feels weird to have an array as a
builder. But I imagine that will change anyway when you rewrite it to be recursive. - Also let's rename some of the arg names elsewhere in the class - attribute -> now content.
| "social_media_links": "social_media_links" | ||
| "body": { "field": "body", "type": "compiled_govspeak" }, | ||
| "social_media_links": { "field": "social_media_links", "type": "social_media_links" }, | ||
| "parts": [ |
There was a problem hiding this comment.
Is parts what we want to go for? Doesn't read very semantically. The way this is defined above in the forms, where you just have the single "about", doesn't even imply it is a plural. And then we come here in the presenters and it's parts all of a sudden.
I appreciate it's handy to fetch the parts for the additional routes as well, but could we try something like this, for the actual implementation?
presenters > publishing_api > details >
"about": {
"title": {...}
"body": {...}
"slug": {...}
}
And then you'd have, say, pages here instead of parts for a thing that might genuinely have a tab called pages, and multiple such pages. And then that could map to an array.
Maybe trying to fix the recursion in the payload builder would make this easy to do.
There was a problem hiding this comment.
I went with parts since that's what Frontend expects.
This is needed for About pages - it's rendered on the page. We're going to move away from 'parts' in the next commit - so are no longer constrained by trying to conform to that schema.
[Deduced through conversation](https://gds.slack.com/archives/C08B3CJGD3N/p1780926290028789?thread_ts=1780397116.381579&cid=C08B3CJGD3N) that Travel-Advice-like 'parts' won't work for Topical Events because Travel Advice expects every page to be in the 'parts', whereas the implementation we've worked with for Topical Events expects only the about page in the 'parts' array. We could look at moving the main Topical Event page into the parts array but that has it diverging from all other StandardEdition formats in a way that is more difficult to roll back later. The alternative is to keep separate content items for Topical Event page and About page, using the fully editionable workflow (2x StandardEdition instances) we're planning to use for landing pages. The middle ground is to include the About page details in the details hash of the Topical Event page, but not try to abstract it through the 'pages' property - just keep a top-level 'about' property instead. That's what we're implementing here.
We no longer dynamically derive this from the 'parts'.
What
This PR explores how we might follow the Travel Advice Publisher approach to multi-page architecture, storing the parent and all of its children under the same content item and making use of 1) Publishing API's multiple
routesfeature and 2) logic in Frontend to match the incoming URL to a givenpartin the details hash.Dependent on alphagov/publishing-api#4088
Why
For simple child pages such as Topical Event About pages, the hypothesis is that a very lightweight implementation might be preferable to the fully editionable workflow implemented in #11431, which comes with overhead around having to coordinate publishing the parent and children separately.
Jira: https://gov-uk.atlassian.net/browse/WHIT-3520
Screenshots
Output
Notice the
details.partsproperty and the top-levelroutesproperty.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.