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.

Sign up for GitHub

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

Add EditionIdCAPIMapper #26830

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Add EditionIdCAPIMapper #26830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions common/app/common/Edition.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ abstract class Edition(
val homePagePath: String = s"/$networkFrontId"

def isEditionalised(sectionId: String): Boolean = editionalisedSections.contains(sectionId)
def matchesCookie(cookieValue: String): Boolean = id.equalsIgnoreCase(cookieValue)
def timezoneId = ZoneId.of(timezone.getID)
}

Expand Down Expand Up @@ -82,8 +81,8 @@ object Edition {
}

def apply(request: RequestHeader): Edition = {
val cookieValue = editionFromRequest(request)
Copy link
Contributor Author

@ioannakok ioannakok Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming because cookieValue is misleading. This method returns the edition from a parameter, a header or a cookie.

allEditions.find(_.matchesCookie(cookieValue)).getOrElse(defaultEdition)
val edition = editionFromRequest(request)
allEditions.find(_.id.equalsIgnoreCase(edition)).getOrElse(defaultEdition)
}

def others(implicit request: RequestHeader): Seq[Edition] = {
Expand Down
14 changes: 13 additions & 1 deletion common/app/contentapi/ContentApiClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ trait ApiQueryDefaults extends GuLogging {

def item(id: String): ItemQuery = CapiContentApiClient.item(id)

def item(id: String, edition: Edition): ItemQuery = item(id, edition.id)
def item(id: String, edition: Edition): ItemQuery = item(id, EditionIdCAPIMapper.mapEditionId(edition))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mapping looks good to me as Content-api's Conceirge reads us, uk, au, europe, international so it has to be this to give the content.
Mapping done in this PR should be fine. Still will get CoPip team to give review on this as well.

Just being curious, some que on this:

  1. Can we try changing Edition id from EUR to EUROPE?

    frontend/common/app/common/editions/Europe.scala

    Line 10 in 85fccaf

    id = "EUR",

  2. If not above, then can we try accessing networkfrontId here as that refers to europe, so can we try checking if edition.id != edition.networkfrontId (which is true in this case) so then access networkFrontId instead? (I know here priority should be edition.id but just curious to know if we can do these cases, then we might not need mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @Divs-B for the review! Re point 2, networkFrontId achieves indeed what we want without having to map. However, I would prefer to avoid this solution as it’s more subtle and it wouldn't be super clear from its name why we're passing the network id instead of the id. Whereas at least the EditionIdCAPIMapper is more descriptive. I agree though the ideal solution would be option 1, to change the id. Unfortunately this requires a big refactoring in at least 3 repos from what I can tell. Haven't done full analysis but here are some examples:

For these reasons we would prefer to go with the mapper for now but I have created a ticket to rename the IDs as this is the best approach:

And one question: Is Concierge the interface to all CAPI requests? Does that mean that all of CAPI's endpoints expect the long form id i.e. europe, international?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ioannakok for your answers.
Details about the work it requires to refactor in many places, I think it does help in justifying to go ahead with the mapping at the moment in this PR as a best solution.
Indeed we would want to keep a consistent name (example "EUROPE") as id across all Guardian repos hence #26851 would be a great idea. Thanks for that, Ioanna.
We are happy to approve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your query:
Is Concierge the interface to all CAPI requests? Does that mean that all of CAPI's endpoints expect the long form id i.e. europe, international?

Yes
(but I will confirm with my team as well) 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Team also has confirmed the same for Concierge 👍
As aforementioned, consistent name would be great idea to keep and as long as Concierge reading correct form of id it should return the content whether any long form in it. Thankyou.

Copy link
Member

@arelra arelra Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Divs-B thanks for your review!

Out of an abundance of caution we checked in Kibana whether any other requests (other than most-viewed) are made to CAPI from frontend using the short edition ids of EUR or INT. Using the query in the Content Platforms space and logstash-capi-* index we could see a variety of requests using EUR or INT

Here are few example queries to demonstrate the different frontend applications making requests to CAPI using the short form ids of either EUR or INT:

EUR form the facia app
INT from the article app
INT from applications
INT from applications-crosswords

Manually updating the edition id to the long form id returns the same response so we assume the edition parameter does not actually affect the query response.

Could you please confirm our understanding that the queries above do not use the edition parameter so updating the edition id to the long form should have no effect.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello. CAPI only becomes interested in the edition parameter if the query is for what looks like a network front (from days before the Fronts API existed IIRC) or an editionalised section, e.g. sport. In the examples you've linked, these appear to be mostly individual content item or tag searches, and those will ignore the edition. If you're curious about which sections are editionalised, you can go to content.guardianapis.com/sections and look for any with multiple editions listed. Hope this helps 🤞

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JustinPinner and @Divs-B very helpful


//Strip unnecessary leading slash in path, as this affects signing of IAM requests
def item(id: String, edition: String): ItemQuery =
Expand Down Expand Up @@ -194,3 +194,15 @@ class PreviewContentApi(httpClient: HttpClient)(implicit executionContext: Execu
apiKey = contentApi.key.getOrElse(""),
)
}

object EditionIdCAPIMapper {
def mapEditionId(edition: Edition): String = {
edition match {
case editions.Uk => "UK"
case editions.Us => "US"
case editions.Au => "AU"
case editions.International => "INTERNATIONAL"
case editions.Europe => "EUROPE"
}
}
}
Binary file added data/database/7177dff2011f02f1a411feeed50467e712588ceec08d5a8a333236050ee5f2e1
View file
Open in desktop
Binary file not shown.
Loading