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

fix: follow up on Purchase button on artwork screen with partner offer #10148

Conversation

MrSltun
Copy link
Member

@MrSltun MrSltun commented Apr 25, 2024

This PR resolves []

Description

This PR is a follow-up on #10144

This PR includes a missing state when the artwork is only inquiriable with a partner offer to show the Purchase button
It also refactors the tests for the ArtworkCommercialButtons component

Screenshots

Details

States

State
isAcquireable + isOfferable
isAcquireable
isOfferable + isInquireable
isOfferable
isInquireable

iOS

Before After
isAcquireable + isOfferable image image
isAcquireable image image
isOfferable + isInquireable image image
isOfferable image image
isInquireable image image

Android

Before After
isOfferable + isAcquireable image image
isAcquireable image image
isOfferable + isInquireable image image
isOfferable image image
isInquireable image image

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

Changelog updates

Cross-platform user-facing changes

  • show purchase button for artwork with partner offer in case artwork is only inquireable - mrsltun

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.

@MrSltun MrSltun self-assigned this Apr 25, 2024
@ArtsyOpenSource
Copy link
Contributor

Warnings
⚠️

An error occurred while validating your changelog, please make sure you provided a valid changelog.

Generated by 🚫 dangerJS against 1af6806

Copy link
Member

@narikazu narikazu left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -50,11 +53,14 @@ export const ArtworkCommercialButtons: React.FC<ArtworkCommercialButtonsProps> =
isBiddableInAuction
const noEditions = !artworkData.editionSets || artworkData.editionSets.length === 0

const hasActivePartnerOffer =
Copy link
Member

Choose a reason for hiding this comment

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

praise: better naming!

Me: () => meWithPartnerOfferFixture,
})

expect(screen.getByText("Purchase")).toBeOnTheScreen()
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@erikdstock erikdstock left a comment

Choose a reason for hiding this comment

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

LGTM - one non-blocking comment.

},
}),
})
)
Copy link
Contributor

Choose a reason for hiding this comment

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

#minor: mockResolveLastOperation does not return a promise, so this might change behavior slightly. I think it's possible that waitFor will wait until the operation resolver is called without error (ie protect from the test running before the operation is queued in the relay environment) but before the result triggers the side effect.

We might want to waitFor the expectation after it but if the test runs reliably as-is, non-blocking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wrapping the expectation inside a waitFor always gives a false positive.
At first, I wrapped the expectation inside a waitFor and it passed, but I had a suspicion and expected "sup" to be on the screen, and it passed 😅 then I removed the waitFor and it failed due to the wrong expectation

const selectedEditionId = ArtworkStore.useStoreState((state) => state.selectedEditionId)
const auctionState = ArtworkStore.useStoreState((state) => state.auctionState)

const AREnablePartnerOfferOnArtworkScreen = useFeatureFlag("AREnablePartnerOfferOnArtworkScreen")
const { hasEnded: partnerOfferEnded } = getTimer(partnerOfferData?.endAt || "")
Copy link
Member

Choose a reason for hiding this comment

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

#non-blocking. Not sure if we need to check the time again given that the endpoint only returns active partner offers.

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be new, but I mentioned an issue in Slack 🔐 that the offer shows up even if it's sold already

@@ -156,6 +162,19 @@ export const ArtworkCommercialButtons: React.FC<ArtworkCommercialButtonsProps> =
}

if (artworkData.isInquireable) {
if (hasActivePartnerOffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch! I missed it in the previous PR as I thought it would have MOOAEA enabled, but I guess I was wong.

Copy link
Member

@starsirius starsirius left a comment

Choose a reason for hiding this comment

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

LGTM! And thank you for the 25 screenshots!! 🥇

@MrSltun MrSltun merged commit 25573cd into main Apr 25, 2024
7 checks passed
@MrSltun MrSltun deleted the mrsltun/fix/update-artwork-commercial-buttons-with-active-partner-offer branch April 25, 2024 14:16
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.

None yet

5 participants