Send json data to DCR /Cards endpoint when show more button gets clicked

[ioannakok]

Use case

• Given a front is rendered by DCR

• When a user clicks a show more button

• DCR calls facia endpoint /*path/show-more/*id.json?dcr=true:

• And frontend returns JSON to DCR /Cards endpoint:

{
    cards: 
    startIndex: 
    config:
}

Related DCR PR: https://github.com/guardian/dotcom-rendering/pull/5116 Related DCR Issue: https://github.com/guardian/dotcom-rendering/issues/4604

What does this change?

This PR:

• Adds a new type of fullDCR PressedCollection in facia-press. When /*path/show-more/*id.json?dcr=true gets hit, FaciaController.renderShowMore method will request pressed.v2.full.dcr.json from the aws-frondend-store S3 bucket. The current purpose of the fullDCR version is to persist data about which cards are visible 'above the fold' in a given container, but the intention is that other DCR-specific content may eventually be added here, too.
• Adds a DotcomCardsRenderingDataModel
• Adds a getCards method in DotcomRenderingService. It converts a list of PressedContent and a startIndex to json and makes a POST request to DCR /Cards endpoint to send the data

Does this change need to be reproduced in dotcom-rendering ?

• [ ] No
• [x] Yes (please indicate your plans for DCR Implementation)

Not 'reproduced' as such, but this change should be deployed alongside the corresponding [DCR PR](Related DCR PR: https://github.com/guardian/dotcom-rendering/pull/5116).

Screenshots

Before	After
facia-press stores in S3
/*path/show-more/*id.json?dcr=true returns
	TODO: Add html key

What is the value of this and can you measure success?

Part of the Fronts migration -- supporting existing behaviour in DCR.

Checklist

Does this affect other platforms?

• [ ] AMP
• [ ] Apps
• [ ] Other (please specify)

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

• [x] No
• [ ] Yes (please give details)

Does this change break ad-free?

• [ ] No
• [ ] It did, but tests caught it and I fixed it
• [ ] It did, but there was no test coverage so I added that then fixed it
• [x] No, all DCR Fronts containers are currently ad-free.

Does this change update the version of CAPI we're using?

• [x] No, all the existing database files are just fine
• [ ] Yes, and I have re-run all the tests locally and checked in all the updated data/database/xyz files

Accessibility test checklist

• [ ] Tested with screen reader
• [ ] Navigable with keyboard
• [ ] Colour contrast passed

Tested

• [x] Locally
• [x] On CODE (optional)

TODO

• Add unit tests
• Test e2e in CODE: Facia Tool --> frontend --> DCR

[ioannakok]

We logged into the facia-press CODE instance using ssm and curled the endpoint to press a page https://github.com/guardian/frontend/blob/main/facia-press/conf/routes#L8 and were able to see the new pressed.v2.full.dcr.json 🎉

[bryophyta]

Thanks for this Olly!

I'll try to address your questions as best I can, but very happy to discuss further either on here or in a call next week!

The tl;dr is that I think I agree with you on a lot of the points, but most of them felt like they were out of scope for this particular issue because they would have required a much broader refactor of the existing facia and facia-press logic. So I don't want it to seem like I'm brushing them off -- I'm very keen to continue these conversations!

1. The lite and visible constructs are already baked in to facia-press, so we are using them in our new pressed page type here in order to fit with the existing logic and terminology.
2. That said, I agree that it is not especially clear in and of itself (and it took us quite a long time to properly understand what was going on here) so I would be in favour of refactoring at some point if we can, but it seemed like it was out of scope for this issue given that it's a baked-in pattern.
3. I also agree that we may not want Frontend to determine how many cards we get, but again this does seem to be the existing situation as things stand; as far as I've been able to tell DCR only ever gets the lite version of pressed fronts from Frontend, and the visible calculation is used to determine that. There are some complicating factors, too, which again made this feel like it was out of scope for this issue: i. How many cards are used from curated and backfill for a container depends partly on deduplication, which is something that can happen at the page level, as well as the container level. Deduplication feels to me like it lives more naturally in Frontend, but I'm not sure about this. Either way, the existing bugs in deduplication logic suggest that this is not an easy problem to solve. (The recent rota issue could be a good opportunity for us to dig into the existing implementation a bit more?) ii. As far as we can gather, the separate lite and full versions of pressed fronts were initially introduced because the full fronts were too heavy and creating performance issues. I think it's definitely worth seeing if that still holds, because it's possible that the constraint has been lifted by other changes in the meantime, but it seems worth checking out. (I believe that @philmcmahon told us about the reasons for introducing the two versions, but wasn't sure exactly what the original performance constraints were? Perhaps we can ask around other alumni, or try a test spike?)
4. We did discuss the idea of passing more information from the client with the show-more request, which I was quite keen on too, but from what I remember @jamesgorrie was quite strong on the idea that this would present problems for caching. That said, I'm not sure whether we considered the specific query style that you suggest here, so it's possible that it might escape the caching issues? (Caching is a bit over my head tbh!)

Sorry for the essay! As you said, there is a lot going on here, so I thought it was worth answering at some length to start to document some of these points. And my main take-away from this piece of work has been that it would be great for us as a team to build up a bit more familiarity with how some of these parts of Frontend work and why they're structured as they are, so these discussions seem really valuable in themselves even if we don't end up changing much code for the time being. (Some of the points you raise might also be good material for discussion in your working group on Frontend/DCR as well, right? I'd especially like to hear from more dotcom alumni on some of these questions.)

---

(p.s. the points I'm trying to outline here are the result of quite a few discussions with others, so please anyone feel free to correct me, especially @ioannakok and @jamesgorrie!)

[bryophyta]

If so, I find the naming confusing - things like fullDCR and getCards are very generic and are not suggestive that they are only used for show more.

getCards was named to match the naming of the Cards/ endpoint in DCR, but following the discussion on the DCR PR I think we'll change that endpoint so I will update the naming of this method accordingly.

The fullDCR name is quite generic, but I think this is defensible. We don't want a proliferation of pressed page types, so when we decided to introduce a new one, the idea was that if we need to press any other DCR-specific information in the future then it should also go in fullDCR. It's also worth noting that again this seems to follow the status quo -- I haven't been able to find any use for the existing full pressed page type other than for supporting the show more functionality.

As with my longer comment above, I agree that some of these don't seem like ideal patterns for a post-DCR world, but my feeling is that properly understanding and potentially changing some of these patterns falls outside the scope of this issue. Perhaps it makes sense to arrange a sync conversation with @oliverlloyd, @OllysCoding and anyone else who's interested next week to go over some of these broader questions?

[oliverlloyd]

If so, I find the naming confusing - things like fullDCR and getCards are very generic and are not suggestive that they are only used for show more.

getCards was named to match the naming of the Cards/ endpoint in DCR, but following the discussion on the DCR PR I think we'll change that endpoint so I will update the naming of this method accordingly.

The fullDCR name is quite generic, but I think this is defensible. We don't want a proliferation of pressed page types, so when we decided to introduce a new one, the idea was that if we need to press any other DCR-specific information in the future then it should also go in fullDCR. It's also worth noting that again this seems to follow the status quo -- I haven't been able to find any use for the existing full pressed page type other than for supporting the show more functionality.

As with my longer comment above, I agree that some of these don't seem like ideal patterns for a post-DCR world, but my feeling is that properly understanding and potentially changing some of these patterns falls outside the scope of this issue. Perhaps it makes sense to arrange a sync conversation with @oliverlloyd, @OllysCoding and anyone else who's interested next week to go over some of these broader questions?

Thanks for the extra info. If getCards could get renamed to something like getShowMore that would be aces. Regarding fullDCR, I don't have a good view on this area of the code and it sounds like you've already put some thought into this and discussed it with others so I'm happy to defer to the outcome of that.

[github-actions[bot]]

"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days"

[github-actions[bot]]

This PR was closed because it has been stalled for 3 days with no activity.