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: show Purchase button properly on artwork screen when there is a partner offer #10144

Merged
merged 6 commits into from
Apr 25, 2024

Conversation

starsirius
Copy link
Member

@starsirius starsirius commented Apr 24, 2024

This PR resolves EMI-1844

Description

Purchase button was not properly displayed on artwork screen in some cases when the artwork had a partner offer. This PR updates the logic and expands the test cases to cover (hopefully) all major scenarios.

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 properly on artwork screen when there is a partner offer - starsirius

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
Copy link
Contributor

ArtsyOpenSource commented Apr 24, 2024

This PR contains the following changes:

  • Cross-platform user-facing changes (Show Purchase button properly on artwork screen when there is a partner offer - starsirius)

Generated by 🚫 dangerJS against 1f607f7

@@ -60,7 +60,119 @@ describe("ArtworkCommercialButtons", () => {
`,
})

it("renders Make Offer button if isOfferable", async () => {
describe("with partner offer on artwork screen feature flag turned off", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Grouping the test cases when feature flag is disabled here to hopefully just delete it entirely when the feature flag is retired.

})

it("renders Purchase button if isAcquireable", async () => {
it("renders Contact Gallery button if artwork is only inquireable", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The diff might be confusing as I reorganized test cases in 1a8bb8d. It might be easier to review new test cases added in b2f9dd3 and 3400c81.

Copy link
Member Author

Choose a reason for hiding this comment

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

The updated test cases are

ArtworkCommercialButtons
  ✓ renders Purchase button if artwork is only acquireable
  ✓ renders Purchase button if artwork is only acquireable and with a partner offer
  ✓ renders Make an Offer button if artwork is only offerable
  ✓ renders Contact Gallery button if artwork is only inquireable
  ✓ renders Purchase and Make an Offer buttons if artwork is offerable and acquireable
  ✓ renders Purchase and Make an Offer buttons if artwork is only offerable and with a partner offer
  ✓ renders Purchase and Make an Offer buttons if artwork is offerable and acquireable and with a partner offer
  ✓ renders Make an Offer and Contact Gallery buttons if artwork is offerable and inquireable
  ✓ renders Purchase and Contact Gallery buttons if artwork is offerable and inquireable and with a partner offer
  ✓ renders Bid button if artwork is in acution and biddable
  ✓ renders Purchase and Bid buttons if artwork is in auction and buynowable
  with partner offer on artwork screen feature flag turned off
    ✓ renders Purchase button if artwork is only acquireable and with a partner offer
    ✓ renders Make an Offer button if artwork is only offerable and with a partner offer
    ✓ renders Purchase and Make an Offer buttons if artwork is offerable and acquireable and with a partner offer
    ✓ renders Make an Offer and Contact Gallery buttons if artwork is offerable and inquireable and with a partner offer

expect(screen.getByText("Make an Offer")).toBeTruthy()
expect(screen.queryByText("Contact Gallery")).toBeFalsy()
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the tests to explicitly check inapplicable CTAs are not present.

})

it("renders Bid button if isInAuction & isBiddable", async () => {
it("renders Purchase and Make an Offer buttons if artwork is only offerable and with a partner offer", async () => {
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 is the first behavioral change.

Before After
Simulator Screenshot - iPhone 15 Pro - 2024-04-24 at 14 16 33 Simulator Screenshot - iPhone 15 Pro - 2024-04-24 at 14 16 53

expect(screen.queryByText("Contact Gallery")).toBeFalsy()
})

it("renders Make an Offer and Contact Gallery buttons if artwork is offerable and inquireable", async () => {
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 is the second behavioral change. When there is already Make Offer and Contact Gallery buttons, and there is a partner offer associated, we want to prioritize the Purchase button (for the partner offer) over Make an Offer (see discussion).

Before After
Simulator Screenshot - iPhone 15 Pro - 2024-04-24 at 14 16 07 Simulator Screenshot - iPhone 15 Pro - 2024-04-24 at 14 15 37

const artworkData = useFragment(artworkFragment, artwork)
const meData = useFragment(meFragment, me)
const partnerOfferData = useFragment(partnerOfferFragment, partnerOffer)
const isPartnerOffered = AREnablePartnerOfferOnArtworkScreen && !!partnerOfferData
Copy link
Member Author

Choose a reason for hiding this comment

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

Guard it behind the feature flag.

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

if (artworkData.isInquireable && artworkData.isOfferable) {
if (isPartnerOffered) {
Copy link
Member Author

@starsirius starsirius Apr 24, 2024

Choose a reason for hiding this comment

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

The existing if/else approach to exhaust all possibilities seems easy to miss some cases. I'm not doing big structural changes in this PR but could be an opportunity for improvement in a followup.

@starsirius
Copy link
Member Author

@MrSltun Since we are approaching code freeze tomorrow and I'm in timezone that's behind, I assigned this PR to you. Before I sign on tomorrow, please feel free to push new changes or merge! 🙏

@starsirius starsirius changed the title Show Purchase button properly on artwork screen when there is a partner offer feat: show Purchase button properly on artwork screen when there is a partner offer Apr 24, 2024
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

@narikazu narikazu merged commit a83c7b8 into main Apr 25, 2024
7 checks passed
@narikazu narikazu deleted the starsirius/artwork-commercial-buttons branch April 25, 2024 05:10
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.

4 participants