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

DISCO-3202 - Enrich the curated-recommendations endpoint with icon-urls #784

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

gruberb
Copy link
Member

@gruberb gruberb commented Feb 5, 2025

References

JIRA:

Description

Enriching the /curated-recommendations endpoint with a iconUrl field for each article recommendation. The iconUrl is a HttpUrl, taken from the Manifest provider, and if the favicon for the domain is available, we attach it to a recommendation. Otherwise, we return an empty string.

In the background, the Manifest is updated once a week, and therefore the icons will be updated in this interval. Which means newly added domains to the Manifest will be available to this endpoint at that time.

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format example [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|skip|warn)] keywords are applied to the last commit message (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@gruberb gruberb requested review from mmiermans and a team February 5, 2025 19:53
@gruberb gruberb force-pushed the DISCO-3202-enrich-currated-recommendations branch from f18430a to b975b8e Compare February 5, 2025 19:57
merino/services/__init__.py Outdated Show resolved Hide resolved
merino/providers/manifest/provider.py Outdated Show resolved Hide resolved
@gruberb gruberb force-pushed the DISCO-3202-enrich-currated-recommendations branch 2 times, most recently from ca2687b to 0cedcab Compare February 6, 2025 13:21
merino/web/api_v1.py Outdated Show resolved Hide resolved
@gruberb gruberb force-pushed the DISCO-3202-enrich-currated-recommendations branch 2 times, most recently from b72fc31 to 9c206a6 Compare February 6, 2025 20:04
Copy link
Contributor

@Herraj Herraj left a comment

Choose a reason for hiding this comment

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

🖼️

merino/providers/manifest/provider.py Show resolved Hide resolved
@gruberb gruberb force-pushed the DISCO-3202-enrich-currated-recommendations branch from 9c206a6 to 31448f3 Compare February 7, 2025 17:54
@ncloudioj
Copy link
Contributor

ncloudioj commented Feb 7, 2025

LGTM, let's confirm a few things before the merge.

  • Since this would add a new filed to the recommendation endpoint, let's confirm that the API consumer(s) are ready to consume (or ignore) it without affecting anything there. I think @mmiermans should be a blocking reviewer for this patch.
  • The icon lookup is tied to the image manifest which in turn is subject to future updates with the manifest, meaning that the icon could appear / disappear over time. We need to confirm if that's an acceptable behavior for the consumer as it'll affect the user experience in the browser. If certain icons should always be present, then we need to de-couple those from the manifest and use other ways to fulfill the icon requests (e.g. allow to "pin" certain icons in the manifest provider).
  • Lastly, this establishes direct dependence between the curation recommendation provider and the manifest provider, meaning that the latter must be initialized sooner than the former, otherwise, icons might not be available. Coincidentally, the manifest provider gets initialized prior to the recommendation provider, but since they both rely on the cron jobs to download data in the background, it's not guaranteed that order. We also need to confirm that is OK for the consumer.

Copy link
Collaborator

@mmiermans mmiermans left a comment

Choose a reason for hiding this comment

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

Only have some questions related to testing. The rest looks good. Thanks for making this change! 👍

@mmiermans
Copy link
Collaborator

Since this would add a new filed to the recommendation endpoint, let's confirm that the API consumer(s) are ready to consume (or ignore) it without affecting anything there. I think @mmiermans should be a blocking reviewer for this patch.

We set the expectation that adding attributes is not a breaking change, but that clients (Desktop and Android for now) will be notified of any changes regardless, to avoid surprises. If they're not already aware, could you send them a Slack message before merging this?

@gruberb gruberb force-pushed the DISCO-3202-enrich-currated-recommendations branch from 31448f3 to 6eb12b9 Compare February 7, 2025 22:01
@gruberb gruberb added this pull request to the merge queue Feb 7, 2025
Merged via the queue into main with commit ca836d1 Feb 7, 2025
11 checks passed
@gruberb gruberb deleted the DISCO-3202-enrich-currated-recommendations branch February 7, 2025 22:18
@@ -140,7 +151,7 @@ def provider(


@pytest.fixture(autouse=True)
def setup_providers(corpus_provider):
def setup_suggest_providers(corpus_provider):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def setup_suggest_providers(corpus_provider):
def setup_curated_recommendations_provider(corpus_provider):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants