feat(alerts): include artist series in suggested filters #5471
Conversation
| "editor.defaultFormatter": "esbenp.prettier-vscode", | ||
| "editor.codeActionsOnSave": { | ||
| "source.fixAll.eslint": true | ||
| "source.fixAll.eslint": "explicit" |
There was a problem hiding this comment.
this is coming from VSCode latest version https://code.visualstudio.com/updates/v1_85#_code-actions-on-save-and-auto. It's the same as true, it should be safe to commit it
There was a problem hiding this comment.
Good catch! I noticed and un-committed this a couple of times but it still snuck through 🙈
I hadn't looked into the cause yet, thanks @MounirDhahri. Sounds like it's okay to actually leave this in.
| source: { | ||
| type: AlertSourceType, | ||
| description: "The context from which the alert originates", | ||
| }, |
There was a problem hiding this comment.
Should we maybe default this to Artist?
There was a problem hiding this comment.
I think this can be left to the client, esp since the default behavior (sourcing the suggestions from the artist aggregation) will always be in effect when the Artwork source is not supplied.
dblandin
left a comment
dblandin
left a comment
There was a problem hiding this comment.
Most of the diff seems to be refactoring so I zoomed in to last couple commits which look good to me! Left one non-blocking clarifying comment.
| aggregations["artist_series"], | ||
| MAX_SUGGESTIONS | ||
| ) | ||
| } |
There was a problem hiding this comment.
question / non-blocking: Just to make sure I'm understanding this correctly — when creating an alert from an artwork source we return up to 5 artist series suggestions and when creating an alert from an artist source we return up to 2 artist series suggestion. Is that right?
Also, something that we've discussed but we should document/align on is what precedence these suggestions have over the existing medium+rarity suggestions. I think you made a good point recently that when creating an alert from an artwork source, the artist series suggestions are more relevant as they relate directly to the source artwork and not general inventory. So I might recommend something like this in terms of precedence and limit:
- Artwork source:
<artist series| max 5>, <medium | max 2>, <rarity | max 2> - Artist source:
<medium | max 2>, <rarity | max 2>, <artist series | max 2>
There was a problem hiding this comment.
Yeah, these numbers are certainly debatable, but good idea to clarify the reasoning here:
-
Artist source: up to 2 artist series suggestions, in order to be consistent with the existing pattern for medium and rarity
-
Artwork source: up to 5 artist series suggestions, based on (a) the assumption that these will be highly relevant to the work in question, and (b) analysis of existing artist series inventory stats (see here — tldr 95% of works in artist series belong to no more than 2 series; 99.98% belong to no more than 5 series)
And yes, I should have noted that I prepend the artist series suggestions to the list of all suggestions in this PR, based on that same assumption that these will be highly relevant to the artwork in question (though perhaps less so when based on overall artist aggregations.)
There was a problem hiding this comment.
Thanks! I started a thread here in Slack to continue aligning on this.
|
Going to merge this to be able to make some more FE progress, but happy to make updates in light of the active discussions. |
https://artsyproduct.atlassian.net/browse/ONYX-603
This PR mainly adds this —
previewSavedSearch.suggestedFiltersnow returns artist series suggestions from the artist aggregation (9d8c6a0)also allows for
suggestedFiltersto take an optional new argumentsource: { type: ARTWORK, id: "abc123" }(8be328e)when that^ is supplied it derives artist series suggestions from the supplied artwork, instead of from artist aggregations (9086cbd)
In order to tackle that I decided to refactor the
previewSavedSearchcode a bit, as it was growing unwieldy. (This is mostly busywork, feel free to skip and review the main commits highlighted above ☝🏽) —previewSavedSearch.tsinto a separate directory (1794eb7)Without the new
sourceargumentIt sources suggestions from the artist (e.g. Toys, Companions)
With the new
sourceargument referring to an artworkIt sources suggestions from the artwork (e.g. SpongeBob)