fix(EMI-2224): Refactor Register to Bid flow #11522

MrSltun · 2025-02-10T11:27:15Z

This PR resolves EMI-2224

Description

This PR fixes an issue with the credit card token not persisting in the app when bidding on an artwork before registering for the auction.
This also refactors the Auction registration flow to be able to add the fix, which should make the flow more performant (will add proofs later)

Note

I kept the old code for comparison and to not break the Github diff (hence the big addition and subtraction), but will remove it before merging this change
Would love your inputs on it :)

Tip

To review the code changes, I recommend chceking the commits of this PR

Videos

Videos Here

Before

	Android	iOS
Register without CC	register-no-cc-android-before-low.mp4	register-no-cc-ios-before-low.mp4
Register with CC	register-with-cc-android-before-low.mp4	register-with-cc-ios-before-low.mp4
Bid without CC	bid-no-cc-android-before-low.mp4	bid-no-cc-ios-before-low.mp4
Bid with CC	bid-with-cc-android-before-low.mp4	bid-with-cc-ios-before-low.mp4

After

	Android	iOS
Register without CC	register-no-cc-android-after-low.mp4	register-no-cc-ios-after-low.mp4
Register with CC	register-with-cc-android-after-low.mp4	register-with-cc-ios-after-low.mp4
Bid without CC	bid-no-cc-android-after-low.mp4	bid-no-cc-ios-after-low.mp4
Bid with CC	bid-with-cc-android-after-low.mp4	bid-with-cc-ios-after-low.mp4

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

Refactor Auction Registration flow
Fix removing credit card token when updating max bid

iOS user-facing changes

Android user-facing changes

Dev changes

TODO

Compare performance changes on both platforms
Add Sentry tracking for errors

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

MounirDhahri · 2025-02-10T15:16:41Z

🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥

src/app/Components/Bidding/Context/BidFlowContextProvider.tsx

MounirDhahri · 2025-02-10T15:23:52Z

src/app/Components/Bidding/Context/BidFlowContextProvider.tsx

+  setBiddingEndAt: Action<this, string | undefined | null>
+}
+
+export const getBidFlowContextStore = (): BidFlowContextModel => ({


This is a great intermediate step especially for refactoring, later we can move all this logic to be pulled from MP to avoid having data not in sync and decouple further these screens

MounirDhahri · 2025-02-10T15:26:27Z

src/app/Components/Bidding/Screens/CreditCardForm.tsx

@@ -33,6 +32,8 @@ interface CreditCardFormProps {
  onSubmit: (t: Token.Result, a: Address) => void
 }

+const MemoizedInput = memo(Input)


nice!. let's move this to palette-mobile though. This should be the default behaviour imo

Or use formik?

it is using formik 😅

Oh i mean the Formik FastField component! https://formik.org/docs/api/fastfield

I don't think it works on React Native since inputs are native elements 😢

ahh dang 👍 all good

didn't know about FastField before. Thanks for sharing about it

ArtsyOpenSource · 2025-02-11T17:28:27Z

This PR contains the following changes:

Cross-platform user-facing changes (Refactor Auction Registration flow,Fix removing credit card token when updating max bid - MrSltun)

Generated by 🚫 dangerJS against 50234cc

damassi · 2025-02-11T17:51:00Z

Can't comment too much on the particulars, but amazing PR @MrSltun 🔥

damassi · 2025-02-11T17:52:15Z

src/app/Components/Bidding/Context/BidFlowContextProvider.tsx

+import { Address, PaymentCardTextFieldParams } from "app/Components/Bidding/types"
+import { action, Action, Computed, computed, createContextStore } from "easy-peasy"
+
+type Increments = NonNullable<NonNullable<SelectMaxBid_saleArtwork$data["increments"]>[number]>[]


👍 👍 Thanks for pulling this from the generated type

damassi · 2025-02-11T18:02:10Z

src/app/Components/Containers/BiddingNavigator.tsx

+      </BiddingNavigationStack.Navigator>
+    </NavigationContainer>
+  )
+}


@MrSltun, @MounirDhahri - curious to get y'alls take here. (I'm mostly referencing Force and the ease in which we can discover most things, at a glance, just from viewing the file tree).

This bidding navigator is effectively an "app" with "routes", but its buried in components/containers. While this follows existing patterns, its still quite confusing at a glance, when compared to how we can navigate all around in folio (as an imperfect example) and "see" the structure of the app just from the file tree (no need to look at code!).

It'd be great if we could lay down some patterns to help here, as i think it could naturally simplify the app and make it more tree like -- apps contain routes and screens; makes sense to colocate and make self-describing.

No need to address here! just figured this is a good example of what I mean and curious about y'alls take.

Very good comment. I am going to add a comment about that to the practice mobile to discuss this together further as a team.
We could definitely do a better job in our documentation to limit the use of NavigationContainer in Eigen. In almost all cases, it's not really required and if it's there, it's a symptom of high coupling.

Decoupling the screens at the level of this PR might be so hard. I do though believe that these screens would benefit a lot from being decoupled from the context store that is wrapping them and be independent routes at some point in the future.

The main issue that made this screens hard to maintain, along with the navigation, is the state management and how it's being synchronized across the different screens. I believe the synching issue would be entirely solved if we make each of the screens an independent QueryRenderer that queries for data itself instead of rely on other screens passing it. It will also give us more confidence in the future to make changes and make QA easier

Great point! I think it makes sense to move it to navigation instead of components/containers since it's just a navigation container and not a component container, I will update that

gkartalis

looks awesome, left some minor comments about some todos but other than that THANK you

gkartalis · 2025-02-11T18:17:55Z

src/app/Components/Bidding/Screens/BidResult.tsx

+// TODO: Clean up old code
+/*


Yeah I left those so it's easier to see the changes without breaking the GitHub diff, I added a note in the PR description about it 😅 but I will remove it before merging this PR!

gkartalis · 2025-02-11T18:18:31Z

src/app/Components/Bidding/Screens/ConfirmBid/PriceSummary.tsx

+  }
+`
+
+// TODO: Clean up old code


MounirDhahri

This is amazing! Great job.

araujobarret

Awesome work 💪

Left a comment for future pr 🌮

araujobarret · 2025-02-12T13:36:24Z

src/app/Components/Bidding/Screens/CreditCardForm.tsx

-  onSubmit,
-  billingAddress,
-  navigator,
+  navigation,


[future-work]: we should remove this drilling from navigation params between these components and rely on the navigation's hooks. Maybe the class components hinder us from doing it but we also can use withNavigation HoC.

araujobarret · 2025-02-12T13:39:49Z

src/app/Components/Bidding/Screens/BidResult.tsx

+  const backHandler = () => {
+    if (canBidAgain) {
+      return false
+    } else {


you don't need the else block here:

const backHandler = () => { if (canBidAgain) { return false } dismissModal() return true }

MrSltun self-assigned this Feb 10, 2025

artsy-peril bot added the Jira Synced label Feb 10, 2025

damassi reviewed Feb 10, 2025

View reviewed changes

src/app/Components/Bidding/Context/BidFlowContextProvider.tsx Show resolved Hide resolved

MounirDhahri reviewed Feb 10, 2025

View reviewed changes

MrSltun added 15 commits February 11, 2025 11:59

fix(EMI-2224): Refactor Register to Bid flow

bf74725

refactor confirm bid screen

2ee7590

refactor bid results screen

89b4fac

improve credit card screen performance

dca0ed7

add BidFlowContextStore

87eea9a

refactor Price Summary

45470c2

fix confirmBid tests

b448a21

fix BidResult tests

671b639

refactor bidding navigation

19e8d36

fix issue with Select options not rendering

fcdc699

refactor and fix select max bid tests

2226d5f

fix credit card form tests

7c77093

fix phone number form tests

b8ecd16

fix registration result tests

bf37ede

refactor bid flow tests

ca4dbe5

MrSltun force-pushed the mrsltun/EMI-2224/refactor-register-to-bid-flow branch from 035d663 to ca4dbe5 Compare February 11, 2025 11:09

MrSltun added 4 commits February 11, 2025 13:47

fix payment info tests

20a337e

fix registration tests

d580218

fix bid result tests

d5eec97

update confirm bid tests

6064a9e

MrSltun requested review from MounirDhahri, damassi, mzikherman and gkartalis February 11, 2025 17:17

MrSltun requested review from brainbicycle and a team February 11, 2025 17:18

MrSltun marked this pull request as ready for review February 11, 2025 17:26

damassi reviewed Feb 11, 2025

View reviewed changes

gkartalis previously approved these changes Feb 12, 2025

View reviewed changes

MounirDhahri previously approved these changes Feb 12, 2025

View reviewed changes

move BiddingNavigator to Navigation

50234cc

MrSltun dismissed stale reviews from MounirDhahri and gkartalis via 50234cc February 12, 2025 13:11

araujobarret approved these changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(EMI-2224): Refactor Register to Bid flow #11522

fix(EMI-2224): Refactor Register to Bid flow #11522

MrSltun commented Feb 10, 2025 •

edited

Loading

MounirDhahri commented Feb 10, 2025

MounirDhahri Feb 10, 2025

MounirDhahri Feb 10, 2025

damassi Feb 10, 2025

MrSltun Feb 10, 2025

damassi Feb 10, 2025

MrSltun Feb 10, 2025

damassi Feb 10, 2025

MounirDhahri Feb 10, 2025

ArtsyOpenSource commented Feb 11, 2025 •

edited

Loading

damassi commented Feb 11, 2025

damassi Feb 11, 2025

damassi Feb 11, 2025 •

edited

Loading

MounirDhahri Feb 12, 2025

MounirDhahri Feb 12, 2025

MrSltun Feb 12, 2025

gkartalis left a comment

gkartalis Feb 11, 2025

MrSltun Feb 12, 2025

gkartalis Feb 11, 2025

MounirDhahri left a comment

araujobarret left a comment

araujobarret Feb 12, 2025

araujobarret Feb 12, 2025

fix(EMI-2224): Refactor Register to Bid flow #11522

Are you sure you want to change the base?

fix(EMI-2224): Refactor Register to Bid flow #11522

Conversation

MrSltun commented Feb 10, 2025 • edited Loading

Description

Videos

Before

After

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

TODO

MounirDhahri commented Feb 10, 2025

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

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

ArtsyOpenSource commented Feb 11, 2025 • edited Loading

This PR contains the following changes:

damassi commented Feb 11, 2025

Choose a reason for hiding this comment

damassi Feb 11, 2025 • 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

gkartalis 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

MounirDhahri left a comment

Choose a reason for hiding this comment

araujobarret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSltun commented Feb 10, 2025 •

edited

Loading

ArtsyOpenSource commented Feb 11, 2025 •

edited

Loading

damassi Feb 11, 2025 •

edited

Loading