Introduce ContentFormat into FAPI Client

buck06191 commented 3 years ago

What does this change?

Both Apps and Dotcom platforms currently use a Format type to determine how to style content. This type is defined in the @guardian/types library and, as of a recent PR, the Scala CAPI Client.

In order for this information to be used in Trails and on Cards the FAPI client needs to be updated to use the new information from the Scala client and pass it through to the consumers.

I here add the Format type to the CuratedContent, SupportingCuratedContent and LatestSnap classes to allow it to be consumed on-platform.

Changes:

Bump CAPI client version to the most recent version as of writing (v17.11)
Add a format field to the above listed classes and create a ContentFormat case class to type this field.
- This case class is copied over from Frontend, but there is future work to move both Format and its types to the top level rather than in the client. At this point the duplication will no longer be necessary.

Related PRs:

Introduction of Format to the CAPI client: https://github.com/guardian/content-api-scala-client/pull/312
Implementation of Format in Frontend: https://github.com/guardian/frontend/pull/23577

How to test

Integrate new version into consumers and confirm it works (this feels a bit circular, but I can't see a better way to test this).

How can we measure success?

Ability to add Format data to trails on platforms.

Have we considered potential risks?

This is additive and so shouldn't break existing content, but anyone bumping version might need to update their types. Compilation should catch this however

rtyley commented 3 years ago

@buck06191 asked:

The README says I need to set a target URL but I've no idea what that should be

...blooming good question. I didn't know either, but surprisingly, it turns out that the targetUrl field overrides a field on the CAPI GuardianContentClient client (via the constructor on TestContentApiClient):

https://github.com/guardian/facia-scala-client/blob/796b5211078ba86be46a13f01b4ac2a4f03d0c49/fapi-client/src/test/scala/lib/IntegrationTestConfig.scala#L9-L15

So it turns out that field is just the url of the CAPI endpoint (aside, I think FACIA_CLIENT_TARGET_URL is not a great name for the env variable because it does not in at all suggest it's anything to do with CAPI):

  /** The url of the CAPI endpoint */
  def targetUrl: String = "https://content.guardianapis.com"

...if you don't override the field, everything will still work - I believe this field was only introduced (see https://github.com/guardian/facia-scala-client/pull/35) because back then, there was a super-secret internal domain-name for the Content API which did not require an API key, had slightly faster direct performance, and was decommissioned in February 2019 (see eg https://github.com/guardian/ophan/issues/3173).

These days everyone uses the public endpoint of https://content.guardianapis.com, so the FACIA_CLIENT_TARGET_URL environment variable should be superfluous - and you shouldn't need to set it. It would be great if someone could update this project to remove the targetUrl override and remove the associated documentation!

rtyley commented 3 years ago

As far as the Ophan team goes, thanks for the heads up! We've had problems in the past where FAPI has added new values to an enum (eg US-West-Coast), started sending that new value, and Ophan's not-yet-updated client has then crashed trying to read the FAPI data, so it's good to know when changes are coming.

In the case of this change, there are no new enum values, and I think it's going to be all-additive? So hopefully no problem for us, and we don't need to couple your rollout with an update to the version of Ophan's FAPI client.

Ophan uses the FAPI client for 'promotion' data (eg, "where did this article get promoted on the UK Network front?"):

If errors occur, they should be visible in the ELK:

guardian / facia-scala-client