Skip to content
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

filter blog by topics on render article route #25118

Merged
merged 24 commits into from
Jun 22, 2022

Conversation

marjisound
Copy link
Contributor

@marjisound marjisound commented Jun 16, 2022

What does this change?

This PR adds logic for filtering liveblog on render article route. It'd add implementation for pagination when top mention result is provided.

  • only gets top mentions when key event filter is switched off
  • Adds TopicsLiveBlog BlockRange for when first page is requested with topics
  • Updates firstPage logic to have correct pagination when top mentions result is provided
  • Updates findPageWithBlock to have correct pagination when top mentions result is provided
  • Refactors firstPage implementation and extracts the details into separate private functions
  • Updates applyFilters implementation for when top mentions is provided
  • Adds detailed tests for firstPage & findPageWithBlock for when top mentions is provided
  • Updates DCRFake in order to enable testing of the the data passed to DCR
  • Adds test for renderArticle endpoint

Note
This PR does not cover the renderJson route and as a result doesn't add logic for updates. This would be done in upcoming PRs.

paired with @joecowton1

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

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

Screenshots

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

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?

  • 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

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

Accessibility test checklist

Tested

  • Locally
  • On CODE (optional)

Base automatically changed from marji/integrate-mentions-in-liveblog-controller to main June 20, 2022 09:57
@marjisound marjisound force-pushed the jc/filter-capi-data-auto-filters branch from 01ff13b to 2666317 Compare June 20, 2022 14:29
@marjisound marjisound changed the title Jc/filter capi data auto filters filter blog by topics on render article route Jun 21, 2022
@marjisound marjisound force-pushed the jc/filter-capi-data-auto-filters branch from 016f495 to d53c18b Compare June 21, 2022 08:17
@marjisound marjisound marked this pull request as ready for review June 21, 2022 13:29
@marjisound marjisound requested a review from a team as a code owner June 21, 2022 13:29
@marjisound marjisound force-pushed the jc/filter-capi-data-auto-filters branch from 0256876 to bac6189 Compare June 21, 2022 13:51
@marjisound
Copy link
Contributor Author

Hi all, I know there's been discussion about naming of different models or classes. There's an upcoming PR that we're currently working on which is supposed to factor all these name changes #25133

article/app/controllers/LiveBlogController.scala Outdated Show resolved Hide resolved
article/app/controllers/LiveBlogController.scala Outdated Show resolved Hide resolved
article/app/controllers/LiveBlogController.scala Outdated Show resolved Hide resolved
article/test/DCRFake.scala Outdated Show resolved Hide resolved
common/app/model/LiveBlogCurrentPage.scala Show resolved Hide resolved
@marjisound
Copy link
Contributor Author

This PR was fully tested in code so I'll merge it to unblock the rest of the work. But if you have any feedback after the merge, please add them here and I'll make sure to address them in the next PR :)

@marjisound marjisound merged commit b027859 into main Jun 22, 2022
@marjisound marjisound deleted the jc/filter-capi-data-auto-filters branch June 22, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants