feat: Infer artist series when creating alert form artwork screen #9631

olerichter00 · 2023-12-06T15:21:22Z

This PR resolves ONYX-566

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

Description

This change adds artist series as an initial criterion when creating the alert from the artwork screen.

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

Infer artist series when creating alert form artwork screen - ole

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

ArtsyOpenSource · 2023-12-06T15:26:51Z

This PR contains the following changes:

Cross-platform user-facing changes (Infer artist series when creating alert form artwork screen - ole)

Generated by 🚫 dangerJS against 4dd602e

olerichter00 · 2023-12-08T11:26:26Z

src/app/Components/Artist/ArtistArtworks/CreateArtworkAlertModal.tsx

@@ -30,6 +31,13 @@ export const CreateArtworkAlertModal: React.FC<CreateArtworkAlertModalProps> = (
  const data = useFragment<CreateArtworkAlertModalProps["artwork"]>(
    graphql`
      fragment CreateArtworkAlertModal_artwork on Artwork {
+        artistSeriesConnection(first: 5) {


Using 5 as the maximum number of artist series used when creating an alert would cover 99.98% of the cases.(context: artsy/force#13244 (comment))

olerichter00 · 2023-12-08T11:27:46Z

src/app/Components/Artist/ArtistArtworks/CreateArtworkAlertModal.tsx

+  if (!artworkAlert) {
+    return null
+  }


Refactoring this component to be able to remove the forbidden non-null assertions.

olerichter00 · 2023-12-08T11:30:28Z

src/app/Scenes/SavedSearchAlert/pillExtractors.ts

+export const extractPillFromArtistSeriesIDs = (values: string[]): SavedSearchPill[] => {
+  return values.map((value) => {
+    return {
+      label: toTitleCase(value.replaceAll("-", " ")),


This isn't ideal, but it should be good enough for now. We have already moved the pill/label generation to Metaphysics and will remove this code soon in favor of using the backend-generated labels.

Because of this, I have decided not to write any additional code to make the name of the artist series accessible here and use this quick workaround until removing this code.

Agree with this, let's push to work on the migration asap.
A small adjustment I would suggest here is to rename values to slugs and also value to slug. It would be nice to rename the method as well since you are querying for slugs instead of ids but I will leave that to you to decide since we do the same thing in other areas

also, do you mind if you add a test for this as we do for other existing extractors

olerichter00 · 2023-12-08T11:40:44Z

src/app/Components/Artist/ArtistArtworks/CreateArtworkAlertModal.tsx

  artwork: CreateArtworkAlertModal_artwork$data | BrowseSimilarWorks_artwork$data
 ) => {
+  const enableArtistSeriesFilter = useFeatureFlag("AREnableArtistSeriesFilter")


Still an open discussion whether to use a second feature flag here (Slack Thread)

MounirDhahri

LGTM! small comments none are blocking

MounirDhahri · 2023-12-08T13:14:30Z

src/app/Components/Artist/ArtistArtworks/CreateArtworkAlertModal.tests.tsx

@@ -11,6 +12,10 @@ jest.mock("app/Components/Artist/ArtistArtworks/CreateSavedSearchModal", () => (
  CreateSavedSearchModal: () => "CreateSavedSearchModal",
 }))

+jest.mock("app/utils/hooks/useFeatureFlag", () => ({
+  useFeatureFlag: () => true,


nitpick: I would avoid this in favour of using our own __globalStoreTestUtils__?.injectFeatureFlags helper.

MounirDhahri · 2023-12-08T13:42:02Z

src/app/Scenes/SavedSearchAlert/pillExtractors.ts

+export const extractPillFromArtistSeriesIDs = (values: string[]): SavedSearchPill[] => {
+  return values.map((value) => {
+    return {
+      label: toTitleCase(value.replaceAll("-", " ")),


Agree with this, let's push to work on the migration asap.
A small adjustment I would suggest here is to rename values to slugs and also value to slug. It would be nice to rename the method as well since you are querying for slugs instead of ids but I will leave that to you to decide since we do the same thing in other areas

MounirDhahri · 2023-12-08T14:12:30Z

src/app/Scenes/SavedSearchAlert/pillExtractors.ts

+export const extractPillFromArtistSeriesIDs = (values: string[]): SavedSearchPill[] => {
+  return values.map((value) => {
+    return {
+      label: toTitleCase(value.replaceAll("-", " ")),


also, do you mind if you add a test for this as we do for other existing extractors

olerichter00 · 2023-12-14T10:15:08Z

Closing this PR because we only want to suggest artist series (Slack Thread)

olerichter00 self-assigned this Dec 6, 2023

olerichter00 marked this pull request as draft December 6, 2023 15:21

artsy-peril bot added the Jira Synced label Dec 6, 2023

feat: Infer artist series when creating alert form artwork screen

f42d81d

olerichter00 commented Dec 8, 2023

View reviewed changes

ff and tests

4dd602e

olerichter00 force-pushed the olerichter00/ONYX-566/infer-artist-series-when-creating-alert-form-artwork-screen branch from dcc3003 to 4dd602e Compare December 8, 2023 11:36

olerichter00 marked this pull request as ready for review December 8, 2023 11:38

olerichter00 requested review from anandaroop and MounirDhahri December 8, 2023 11:38

olerichter00 commented Dec 8, 2023

View reviewed changes

MounirDhahri approved these changes Dec 8, 2023

View reviewed changes

olerichter00 closed this Dec 14, 2023

anandaroop mentioned this pull request Dec 14, 2023

feat(alerts): enable artist series suggestions #9663

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Infer artist series when creating alert form artwork screen #9631

feat: Infer artist series when creating alert form artwork screen #9631

olerichter00 commented Dec 6, 2023 •

edited

Loading

ArtsyOpenSource commented Dec 6, 2023 •

edited

Loading

olerichter00 Dec 8, 2023

olerichter00 Dec 8, 2023

olerichter00 Dec 8, 2023 •

edited

Loading

MounirDhahri Dec 8, 2023

MounirDhahri Dec 8, 2023

olerichter00 Dec 8, 2023

MounirDhahri left a comment

MounirDhahri Dec 8, 2023

MounirDhahri Dec 8, 2023

MounirDhahri Dec 8, 2023

olerichter00 commented Dec 14, 2023

feat: Infer artist series when creating alert form artwork screen #9631

feat: Infer artist series when creating alert form artwork screen #9631

Conversation

olerichter00 commented Dec 6, 2023 • edited Loading

Description

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

ArtsyOpenSource commented Dec 6, 2023 • edited Loading

This PR contains the following changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olerichter00 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounirDhahri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olerichter00 commented Dec 14, 2023

olerichter00 commented Dec 6, 2023 •

edited

Loading

ArtsyOpenSource commented Dec 6, 2023 •

edited

Loading

olerichter00 Dec 8, 2023 •

edited

Loading