Add `Format` to the Scala CAPI client #312

buck06191 · 2021-02-24T15:55:11Z

What does this change?

Rendering platforms have moved to use the Format type to describe the
format that rendered data should take. This includes three specific
fields:

Display
Design
Theme (Pillar/Special)

Currently the platforms handle the decision logic around how to
construct this object on platform but ideally this will be done upstream
in order to standardise across platforms and to simplify the parsing of
data.

Here I upstream the Format type to the CAPI Scala client.

Changes:

Introduce the Display, Theme and Design traits and their
associated case objects.
Refactor CAPIModelEnrichment to move code shared by RichContent
and Format to the upper scope.
Reproduce the decision logic used by platforms to construct the
Format object.

How to test

I've added unit tests for this and they can be run using whatever testing process you prefer e.g. sbt test

How can we measure success?

Improved DevX for anyone working with the Format type, and ideally tidy refactors of the platform code to use this instead of multiple checks and flags.

Have we considered potential risks?

I don't think there are any risks. This doesn't affect the raw data and shouldn't remove backwards compatibility.

Rendering platforms have moved to use the `Format` type to describe the format that rendered data should take. This includes three specific fields: - `Display` - `Design` - `Theme (Pillar/Special)` Currently the platforms handle the decision logic around how to construct this object on platform but ideally this will be done upstream in order to standardise across platforms and to simplify the parsing of data. In this commit I upstream the `Format` type to the CAPI Scala client. Changes: - Introduce the `Display`, `Theme` and `Design` traits and their associated `case object`s. - Refactor `CAPIModelEnrichment` to move code shared by `RichContent` and `Format` to the upper scope. - Reproduce the decision logic used by platforms to construct the `Format` object.

oliverlloyd

This seems like one of those nice clean input > output pieces of work where it might be useful to have some tests laid out at an earlier stage so that you work to make the tests pass.

I'm not normally a strict TDD evangelist but this does feel like a great place to take advantage of that pattern

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

- Replicate the code used in Frontend and apps-rendering to determine the `ShowcaseDisplay` `Display`. - Tidy up code to make it easier to parse.

shtukas · 2021-02-26T12:57:13Z

Looks good +1

tonytw1 · 2021-02-26T13:11:55Z

This textbook rejection of a PR like this is "putting domain logic like this in a language specific API client is bad".

But this PR has been extensively reviewed by the submitting teams and their motive is deduplication of code which is already in CAPI consumers.
This PR does the hard work of getting the different consumers to agree what their domain model should look like.
We should encourage this and offer to upstream these new domain model from the client into CAPI.

CAPI should approve this and we can roll forward to get it into the ideal place.

tonytw1 · 2021-02-26T13:17:37Z

Furthermore.... This will be a versioned release so if the teams submitting this PR can consume it immediately they will be doing the regression testing. Other consumers can stay on or revert back last good release if this causes issues.

gtrufitt · 2021-02-26T15:20:07Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala


-      def isComment: ContentFilter = c => tagExistsWithId("tone/comment")(c) || tagExistsWithId("tone/letters")(c)
+  val isDeadBlog: ContentFilter = content => !isLiveBloggingNow(content) && tagExistsWithId("tone/minutebyminute")(content)


We'll need guardian/types#83 to match up with Types

gtrufitt · 2021-02-26T15:22:18Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+        isReview -> ReviewDesign,
+        tagExistsWithId("tone/analysis") -> AnalysisDesign,
+        isCommentDesign -> CommentDesign,
+        tagExistsWithId("tone/features") -> FeatureDesign,


I think we are missing Letter Design? https://github.com/guardian/types/blob/main/src/format.ts#L24

That's a good question. We don't currently have Letter as a Design in our decideDesign.ts function.
@oliverlloyd do you know if whether @guardian/types is correct or if DCR is?

@guardian/types trumps DCR. We haven't yet updated the version of the /types package we're using but when we do we will add support for Letter

Letter is a recent addition: guardian/types#69

gtrufitt · 2021-02-26T15:23:05Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+        tagExistsWithId("tone/matchreports") -> MatchReportDesign,
+        tagExistsWithId("tone/interview") -> InterviewDesign,
+        tagExistsWithId("tone/editorials") -> EditorialDesign,
+        tagExistsWithId("tone/quizzes") -> QuizDesign,


Are we also missing Interactive design? https://github.com/guardian/types/blob/main/src/format.ts#L32

I don't think we technically support an Interactive design on DCR? It's not in our decideDesign function at least. If it does need supporting, I can add it in here and that way we'll just need to add it to the decideDesign function to translate it from the Scala JSON output to whatever Typescript needs.

Should the be an Interactive Design type? I'm not sure what it represents? @JamieB-gu is there a need for this in Apps or could we remove it?

It's in the apps-rendering decision functions, so I'll add it in. If it's in here and then removed, it's not a difficult thing to rip out (which is part of the motivation for this work in the first place).

I think that the work here should be done with a focus on targeting a specific version of @guardian/types, and at this point this is ever so slightly ahead of the current version by having different Designs for dead and live blogs, as well as removing the Column display type. But after this initial step of aligning the two, the goal should be to update the two in parallel, with references to the appropriate version of each within the changelog

Design.Interactive is at the moment derived from ContentType.INTERACTIVE. I believe it's there so that we can build the empty placeholder articles for full-page interactives (ng-interactive/type/interactive - example here). Note that we explicitly render these without any styles because the interactive code takes over.

I'm curious as to how DCR models these kinds of articles if it doesn't use the Design 🤔?

gtrufitt · 2021-02-26T15:25:29Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+        isImmersive -> ImmersiveDisplay,
+        isShowcase -> ShowcaseDisplay,
+        isNumberedList -> NumberedListDisplay


A note here that we need to update https://github.com/guardian/types/blob/main/src/format.ts#L37-L43 to remove Column Display.

JustinPinner

I agree with the commentary where we acknowledge that this isn't the ideal resting place for this functionality, and I have no objections to using it as a temporary measure prior to relocating into tools and CAPI. It looks like a well enough isolated change and test coverage looks good too. I can't comment on the intricacies of relationships between types / DCR / dotcom etc, so I'll approve on the basis that you're all happy that everything is covered before you merge 👍

JamieB-gu · 2021-03-01T13:27:13Z

We should encourage this and offer to upstream these new domain model from the client into CAPI.

temporary measure prior to relocating into tools and CAPI

Does this mean moving it into the Thrift definitions?

JamieB-gu

Really nice! Very pleased that this is starting to make its way upstream 😀

JamieB-gu · 2021-03-01T13:31:16Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala


-      val isImmersive: ContentFilter = c => c.fields.flatMap(_.displayHint).contains("immersive")
+  val isMedia: ContentFilter = content => tagExistsWithId("type/audio")(content) || tagExistsWithId("type/video")(content) || tagExistsWithId("type/gallery")(content)


AR also has type/picture in this category, see guardian/apps-rendering#1146.

Thanks for this. I'll add it to a card for changes to make on a future version

JamieB-gu · 2021-03-01T13:37:29Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+
+      val defaultDesign: Design = ArticleDesign
+
+      val isPhotoEssay: ContentFilter = content => content.fields.flatMap(_.displayHint).contains("photoessay")


Did you want to reuse your displayHintExistsWithName function here?

val isPhotoEssay: ContentFilter = content => displayHintExistsWithName("photoessay")(content)

JamieB-gu · 2021-03-01T13:43:59Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+        tagExistsWithId("tone/editorials") -> EditorialDesign,
+        tagExistsWithId("tone/quizzes") -> QuizDesign,
+        isInteractive -> InteractiveDesign,
+        isPhotoEssay -> PhotoEssayDesign,


@oliverlloyd are we keeping Photo Essays as a separate Design, or is this temporary until we subsume MultiImage into a normal body element?

There is still at least one design aspects of Photo Essays that is unique: coloured captions. So unless we lose that it's here to stay

JamieB-gu · 2021-03-01T13:45:17Z

client/src/main/scala/com.gu.contentapi.client/utils/CapiModelEnrichment.scala

+
+      def isPillar(pillar: String): ContentFilter = content => content.pillarName.contains(pillar)
+
+      val isSpecialReport: ContentFilter = content => content.tags.exists(t => specialReportTags(t.id))


What happens for special reports assigned in the targeting tool?

Hmm, I did this off the back of different decision functions I found across the different codebases. If there's one missing, then we'll need to implement it in a future version. I imagine the process of integrating this all into the platforms will highlight missing areas and then we can update this to reflect that .

buck06191 · 2021-03-01T14:09:33Z

We should encourage this and offer to upstream these new domain model from the client into CAPI.

temporary measure prior to relocating into tools and CAPI

Does this mean moving it into the Thrift definitions?

It should do, yes. I guess we (platforms) all need to have a discussion as to how we do this, then standardise on it and move forwards

oliverlloyd reviewed Feb 24, 2021

View reviewed changes

buck06191 added 5 commits February 25, 2021 12:00

Add Display handler for ShowcaseDisplay

4f3d909

- Replicate the code used in Frontend and apps-rendering to determine the `ShowcaseDisplay` `Display`. - Tidy up code to make it easier to parse.

Add tests for the format work

8f64102

Fix failing tests

daeb03c

Add showcasedisplay tests

6647d62

Expand on function argument

9df2764

buck06191 marked this pull request as ready for review February 26, 2021 12:40

tonytw1 approved these changes Feb 26, 2021

View reviewed changes

gtrufitt reviewed Feb 26, 2021

View reviewed changes

buck06191 added 3 commits March 1, 2021 09:58

Add support for LetterDesign

d6938e2

Add support for InteractiveDesign

fa4b130

Remove autoimport by IntelliJ

5597e9e

JustinPinner approved these changes Mar 1, 2021

View reviewed changes

Update changelog

b8dc1c4

tonytw1 merged commit 0c40ed2 into guardian:master Mar 1, 2021

JamieB-gu reviewed Mar 1, 2021

View reviewed changes

This was referenced Mar 1, 2021

Implement Format from CAPI client guardian/frontend#23577

Merged

Introduce ContentFormat into FAPI Client guardian/facia-scala-client#243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Format` to the Scala CAPI client #312

Add `Format` to the Scala CAPI client #312

buck06191 commented Feb 24, 2021 •

edited

Loading

oliverlloyd left a comment

shtukas commented Feb 26, 2021

tonytw1 commented Feb 26, 2021 •

edited

Loading

tonytw1 commented Feb 26, 2021

gtrufitt Feb 26, 2021

gtrufitt Feb 26, 2021

buck06191 Mar 1, 2021

oliverlloyd Mar 1, 2021

JamieB-gu Mar 1, 2021

gtrufitt Feb 26, 2021

buck06191 Mar 1, 2021

oliverlloyd Mar 1, 2021

buck06191 Mar 1, 2021

JamieB-gu Mar 1, 2021

gtrufitt Feb 26, 2021

JustinPinner left a comment

JamieB-gu commented Mar 1, 2021

JamieB-gu left a comment

JamieB-gu Mar 1, 2021

buck06191 Mar 1, 2021

JamieB-gu Mar 1, 2021

JamieB-gu Mar 1, 2021

oliverlloyd Mar 1, 2021

JamieB-gu Mar 1, 2021

buck06191 Mar 1, 2021

buck06191 commented Mar 1, 2021


		def isComment: ContentFilter = c => tagExistsWithId("tone/comment")(c) \|\| tagExistsWithId("tone/letters")(c)
		val isDeadBlog: ContentFilter = content => !isLiveBloggingNow(content) && tagExistsWithId("tone/minutebyminute")(content)


		val isImmersive: ContentFilter = c => c.fields.flatMap(_.displayHint).contains("immersive")
		val isMedia: ContentFilter = content => tagExistsWithId("type/audio")(content) \|\| tagExistsWithId("type/video")(content) \|\| tagExistsWithId("type/gallery")(content)


		val defaultDesign: Design = ArticleDesign

		val isPhotoEssay: ContentFilter = content => content.fields.flatMap(_.displayHint).contains("photoessay")


		def isPillar(pillar: String): ContentFilter = content => content.pillarName.contains(pillar)

		val isSpecialReport: ContentFilter = content => content.tags.exists(t => specialReportTags(t.id))

Add Format to the Scala CAPI client #312

Add Format to the Scala CAPI client #312

Conversation

buck06191 commented Feb 24, 2021 • edited Loading

What does this change?

Changes:

How to test

How can we measure success?

Have we considered potential risks?

oliverlloyd left a comment

Choose a reason for hiding this comment

shtukas commented Feb 26, 2021

tonytw1 commented Feb 26, 2021 • edited Loading

tonytw1 commented Feb 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinPinner left a comment

Choose a reason for hiding this comment

JamieB-gu commented Mar 1, 2021

JamieB-gu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck06191 commented Mar 1, 2021

Add `Format` to the Scala CAPI client #312

Add `Format` to the Scala CAPI client #312

buck06191 commented Feb 24, 2021 •

edited

Loading

tonytw1 commented Feb 26, 2021 •

edited

Loading