Remote render email newsletters #25876
Conversation
| private def getShouldUseRemoteRender()(implicit | ||
| request: RequestHeader, | ||
| ): Boolean = { | ||
| request.getQueryString("dcr").contains("true") | ||
| } |
There was a problem hiding this comment.
As I understand we shouldn't need this function, you should be able to use the request.forceDCR property as this is an implicit added to RequestHeader.
There was a problem hiding this comment.
Thanks - I wasn't familiar with implicits, but I'll push a change to make use of it.
| @@ -53,4 +53,21 @@ object StaticPages { | |||
| ), | |||
| groupedNewsletterResponses, | |||
| ) | |||
|
|
|||
| def dcrSimpleNewsletterPage( | |||
There was a problem hiding this comment.
Is there any background info on how this works? It's not something I'm particularly familiar with?
There was a problem hiding this comment.
As I understand it, SimplePage is a class which extends the Page and uses the StandalonePage trait and they are used to represent pages on the site which have metaData but no "content" (ie its not a ContentPage or a GalleryPage etc). They get constructed with a few fields and the class provide a set of helper methods return the meta data values based on the Configuration setting etc
I think the idea behind them is to provide an extendable container or helper class for keeping the meta data for all pages in a consistent format. For the purpose of this PR, the dcrSimpleNewsletterPage passed to the dcr service to build the rendering model.
The StaticPages object is a collection of these SimplePages - the existing simpleNewslettersPage is extended to include the groupedNewsletterResponses which the current frontend-rendered view needs as page for the page model. We don't need this for DCR, so it seemed to make sense to define the dcrSimpleNewsletterPage (same metadata but no groupedNewsletterResponses ) inStaticPages to avoid building it unnecessarily for a DCR request.
| val dataModel = | ||
| DotcomNewslettersPageRenderingDataModel.apply(page, newsletters, request) | ||
| val dataJson = DotcomNewslettersPageRenderingDataModel.toJson(dataModel) | ||
| common.renderJson(dataJson, page).as("application/json") |
There was a problem hiding this comment.
An interesting note on this is that we've never had a .json endpoint return DCR optimised data without requiring .json?dcr before. However unlike other cases, these pages didn't support .json as others have (generally /some-page.json returns the HTML in a .json with the html & the config object).
I'm not sure whether we should continue this restriction of requiring ?dcr or not. I will ask around and see if it might be a problem!
There was a problem hiding this comment.
Update: advice is to keep the .json?dcr pattern, so I guess 404 on .json & return the DCR json on .json?dcr :)
There was a problem hiding this comment.
I've made an update - it will now return a 404 for .json - however I couldn't find any example of a similar pattern so I'm not 100% sure I'm doing it right.
Thanks! I think it was just out of sync - I've merge main and its passing now. |
prout-bot
added
Pending-on-ADMIN-PROD
Pending-on-FRONTS-PROD
Seen-on-ADMIN-PROD
and removed
Pending-on-ADMIN-PROD
labels
|
Seen on ADMIN-PROD (merged by @dblatcher 16 minutes and 43 seconds ago)
|
|
Seen on FRONTS-PROD (merged by @dblatcher 17 minutes and 1 second ago)
|
* throw an error if the path requests dcr=true * add stub service module for requesting the page from dcr * RemoteRenderPage can await Futures * can post test json to DCR and get a response * provide newsletterData list to DCR * adjust timing * scalafmt * use capitalised path for Dcr request * provide some metaData * build a Page object, hard code the JSON string properties, add TO DO's * delegate creating the simple page to the StaticPages module * use some page metaData for the JSON * rename renderer object * add edtion to page model * add urls from config * provide the full config object * provide essentials for navMenu data * construct nav menu * build rendering data model for newsletters page * move the data model building to the DCR service * rename method * all dcr service directly from controller * fix casing * tidy imports * provide canonicalUrl * use forceDCR from RichRequestHeader * add a render json method * 404 error for .json without ?dcr
What does this change?
DotcomNewslettersPageRenderingDataModelfor the Newsletters pageDotcomNewslettersPageRenderingDataModelfor requests to "/email-newsletters.json"Does this change need to be reproduced in dotcom-rendering ?
See guardian/dotcom-rendering#7169
What is the value of this and can you measure success?
We have a new design of the newsletters page to implement in DCR. Adding this option for DCR to render a placeholder version of the page while the design is implemented will be the first step replacing the frontend-rendered version.
By using the existing pattern for dotcom data models, the new methods provide the metadata, config, Nav and PageFooter data etc needed for the DCR page template, as well as the newsletters data needed for the main content.
As well as modernising the page design, this will allow us to remove a lot of legacy rendering code from frontend used to generate the current version of the page.
Checklist
Does this affect other platforms?
Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?
Does this change break ad-free?
Does this change update the version of CAPI we're using?
Accessibility test checklist
Tested