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

feat(alerts): add artist series to initial criteria for artwork-based alerts #13244

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anandaroop
Copy link
Member

@anandaroop anandaroop commented Dec 7, 2023

The type of this PR is: Feat

This PR solves ONYX-563

Description

Note

Opened as Draft PR while we discuss whether this is the desired approach (Slack thread)

Adds the artwork's artist series to the initial inferred criteria for an alert created for an artwork page.

@anandaroop anandaroop self-assigned this Dec 7, 2023
olerichter00
olerichter00 previously approved these changes Dec 7, 2023
Copy link
Contributor

@olerichter00 olerichter00 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

I've just added a comment about potentially limiting the number of artist series.

@@ -358,6 +378,13 @@ export const ArtworkAppFragmentContainer = createFragmentContainer(
attributionClass {
internalID
}
artistSeriesConnection(first: 20) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it is important and if we should limit the artist series criteria to an overseeable limit to avoid showing up to 20 criteria in the "Create Alert" flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye, I didn't pick that number carefully, I just remembered that the max # of series per artwork was 12(!)

The actual distribution looks like this:

distr

The vast majority 94.5% of works in artist series belong to no more than 2 series.

And if we allow up to 5 series, that covers 99.98%. So I think 5 is a good limit here, with the understanding that a tiny handful of works might not reveal all of their series as initial criteria.

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.

2 participants