chore: fix remaining shipping2 tests, complete initial refactor #13324

erikdstock · 2023-12-22T22:24:11Z

The type of this PR is: chore

It completes EMI-1526 and relates to EMI-1520. After this PR all existing legacy tests will have been implemented, opening the door to more refactoring. All skipped tests have been unskipped (there will be failures while in draft), but TODOs remain in the test file.

Summary of changes

User address updates are now handled consistently for different flows, always happening before the relevant mutation on the exchange order.
The hook enabling user address updates has been updated to handle multiple flows.
All integration tests in Shipping2.jest.tsx are working.
Added tests at the component level, migrating some to use react testing library.
Recently-pended tests have been left in the pending state (owing to timeout issues in CI), but they are passing in development. The ideal here will be to lean more on a mix of unit tests and integrity for testing in the long term.

The diff is quite large because of fixing these final specs revealed some bigger changes that were necessary.

old notes

Functionality

Because the fulfillment details steps has two separate form layouts (saved addresses with a modal, or no saved addresses with a form on the page itself) there are multiple different flows to account for:

With saved addresses we defer form validation to the saved addresses formik context (a single formik context could make this easier)
With saved addresses, we perform user address updates first, then save fulfillment details. with new address it is the opposite
- if this were not necessary, it could be easier - ideally call the most restrictive mutation first
With saved addresses we do not perform address verification.
- Someday we may want to add address verification when editing or creating a new address - on the old shipping form this was too complicated to implement.
Saved addresses are persisted to the order when they are selected, without needing to click save and continue. Thus the user must click save and continue again to proceed [with no mutation].
On load, SavedAddresses automatically selects an initial address value based on a) what is already saved on the order or b) the user's default address, triggering the save to the order. a) is new functionality designed to avoid a bug mentioned in EMI-1520 and below and it does this by comparing address fields.
- Does the logic to match existing order fulfillment data with a user's saved addresses need to exist up in ShippingContext, or could it all happen in the SavedAddresses component?

Edge cases and possible bugs

So far in this draft PR I've covered most of these cases, but there may be more, especially with the edge cases noted in EMI-1520.

When the route loads, we want to re-save the fulfillment details on the order if it is an artsy shipping order (noted above). This causes shipping quotes to be re-fetched, but does not un-save a previously-saved shipping quote, which will no longer be visible and may have a different price or destination. (Wont fix)
Our current logic doesn't respect whether an artwork can ship to that automatically-selected address, relying on an error from the server to immediately tell the user to choose a different address. Likewise we do not disable address radio buttons that aren't valid for shipping. (Partially fixed - we only automatically select from countries that can be shipped-to)
Once in SavedAddresses mode (ie the user has a saved address) there is no option to not save a new address (Wont fix)
In this flow:
- User applies shipping address to order, but doesn't save address for later use on their user
- User adds a new, different saved address to their user elsewhere
- User reloads shipping page, now in saved addresses mode
- => The originally-saved address may be clobbered by the auto-saving feature, since their original address won't be in the saved address list.

erikdstock · 2023-12-22T22:36:42Z

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx

+  const handleSelectSavedAddress = async (
+    address: ShippingAddressFormValues
+  ) => {
+    await setValues({


I had to upgrade the formik version to give this the correct Promise return type (so that submitForm() will hopefully work as expected)

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx

src/Apps/Order/Routes/Shipping2/Components/SavedAddresses2.tsx

src/Apps/Order/Routes/Shipping2/index.tsx

yarn.lock

damassi

Once tests are green, LGTM 👍

(And congrats on wrapping this all up 🎉)

erikdstock · 2024-02-01T02:44:29Z

src/Apps/Order/Routes/Shipping2/Hooks/useSaveSelectedShippingQuote.tsx

@@ -36,12 +36,21 @@ export const useSaveSelectedShippingQuote = (
    try {
      shippingContext.actions.setIsPerformingOperation(true)

+      if (
+        shippingContext.state.fulfillmentDetailsCtx?.values.meta.mode ===


todo: can this be stored/pulled directly from shippingContext.state and not used in the form ctx?

I'll look into this but don't think it needs to block this PR.

I'm going to leave this for now because it also includes the pickup mode, which isn't controlled from outside the component.

update: I went ahead and did it. cleaner and also caught an invisible bug.

erikdstock · 2024-02-01T02:48:14Z

src/Apps/Order/Routes/Shipping2/Components/SavedAddresses2.tsx

-      selectedSavedAddressId,
-      shippingContext.orderData.availableShippingCountries
-    )?.internalID
+  const addressList = compact<SavedAddressType>(


Todo: Added addressList to shippingContext because it was used in hooks, and painful to thread in. But maybe this isn't necessary - a helper to compact an address list from anywhere could accomplish the same thing... Alternately it could be used here too instead of using local data from props.

erikdstock

Comments on approach and major changes.

erikdstock · 2024-02-01T02:56:47Z

src/Apps/Order/Routes/Shipping2/Hooks/useUserAddressUpdates.ts

+  const updateDefaultAddress = useUpdateUserDefaultAddress()
+  const shippingContext = useShippingContext()
+
+  const handleNewUserAddressUpdates = async (


This file is key to the changes because the hook controls all updates to user addresses. It includes

handleNewUserAddressUpdates: deals with creating/updating/deleting a user's address when it is their first saved address (address form is still present, and they may edit it or even uncheck the box before saving shipping quotes)

executeUserAddressAction: Takes an action type (create/edit/delete) and performs that action, as well as setting as default if applicable. Used by the first method above.

Following this change all flows perform actions in the same order, Performing any user address updates before submitting fulfillment details/shipping quotes to exchange.

erikdstock · 2024-02-01T02:59:39Z

src/Apps/Order/Routes/Shipping2/Hooks/useSelectFirstShippingQuote.tsx

    : null

  useEffect(() => {
    if (
      bestArtsyShippingQuote &&
      bestArtsyShippingQuote !==
-        shippingContext.orderData.selectedShippingQuoteId
+        shippingContext.orderData.selectedShippingQuoteID


A lot of diff in this PR is me making _ID capitalization consistent. I chose the format used for internalID

erikdstock · 2024-02-01T03:03:10Z

src/Apps/Order/Routes/Shipping2/Hooks/useSaveSelectedShippingQuote.tsx

+        "new_address"
+      ) {
+        await handleNewUserAddressUpdates(
+          shippingContext.state.fulfillmentDetailsCtx.values


An effect of letting a user edit values in 2 steps at once - if they change the address, the shipping quotes will disappear and they will be back on the fulfillment details step. But if they check/uncheck the box we just need to save/delete the address - hence this line, which for consistency now happens before saving the shipping quote.

Another way to do this could be to have the save/unsave mutation triggered from inside fulfillment details immediately upon clicking the box. Might be a good follow-up.

erikdstock · 2024-02-01T03:03:53Z

src/Apps/Order/Routes/Shipping2/Hooks/useHandleUserAddressUpdates.ts

-  "Order/Routes/Shipping/Hooks/useHandleUserAddressUpdates.tsx"
-)
-
-export const useHandleUserAddressUpdates = () => {


moved to useUserAddressUpdates

erikdstock · 2024-02-01T03:05:22Z

src/Apps/Order/Routes/Shipping2/Hooks/useBackToFulfillmentDetails.tsx

- * Go back to fulfillment details stage if the user edits the address or
- * deletes a saved address.
- */
-export const useBackToFullfillmentDetails = (me: Shipping2_me$data) => {


Moved this logic into a wrapper around specific handleChange callbacks that should trigger the change. It's cleaner, less code and easier to trace.

erikdstock · 2024-02-01T03:26:10Z

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx

-                  onChange={trackAutoCompleteEdits("postalCode", handleChange)}
+                  onChange={withBackToFulfillmentDetails(
+                    trackAutoCompleteEdits("postalCode", handleChange)
+                  )}


useBackToFulfillmentDetails hook behavior now wraps specific fields we want to track.

erikdstock · 2024-02-01T03:29:46Z

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx

+        },
+      })
+
+      await formikContext.submitForm()


After saving a user's address, the form receives it in this callback, sets all the values (because it is in saved address mode the inputs are not visible) and submits the form. FulfillmentDetails controlls submitting the values to exchange only after SavedAddresses2 performs the user address updates.

erikdstock · 2024-02-01T03:34:14Z

src/Apps/Order/Routes/Shipping2/Components/AddressModal2.tsx

 export const GENERIC_FAIL_MESSAGE =
  "Sorry, there has been an issue saving your address. Please try again."

-const validationSchema = Yup.object().shape(ADDRESS_VALIDATION_SHAPE)
+const handleGravityErrors = (
+  errors: ReadonlyArray<{ message: string }>,


Gravity mutation helpers in useUserAddressUpdates now always return a type something like {data: T} | {errors: [{message?: string}]} | null (null if no action was performed). This lets us handle the errors a bit more consistently.

erikdstock · 2024-02-01T03:35:40Z

src/Apps/Order/Routes/Shipping2/Components/AddressModal2.tsx

+                title="Full name"
+                placeholder="Full name"
+                id="name"
+                name="attributes.name"


I copied these fields in so i could give them the attributes key nesting. On reflection, I'm not sure if this was necessary or was part of an earlier attempt to get mutation-calling consistent.

erikdstock · 2024-02-01T03:36:33Z

src/Apps/Order/Routes/Shipping2/Components/AddressModal2.tsx

+        <Form data-testid="AddressModal">
+          {formikContext.status && (
+            <Banner my={2} data-testid="form-banner-error" variant="error">
+              {formikContext.status}


using Formik's status for this now.

damassi · 2024-02-01T20:36:46Z

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx

-     * TODO: This could be accomplished with useImperativeHandle(ref, formikContext)
-     */
-    shippingContext.actions.setFormHelpers(formikContext)
+    shippingContext.actions.setFulfillmentDetailsCtx(formikContext)


We should be more descriptive in the name of this function, just so we can see that its a formik context versus some other type of context:

shippingContext.actions.setFulfillmentDetailsFormikContext(formikContext)

damassi · 2024-02-01T20:43:26Z

src/Apps/Order/Routes/Shipping2/Components/__tests__/SavedAddresses2.jest.tsx

+      }
+      const addAddressButton = screen.getByText("Add a new address")
+      await userEvent.click(addAddressButton)
+      logTime("clicked button")


Might want to remove these loggers before merge (though i can see how they might be helpful for future devs, too)

Yeah, maybe I can just link to this comment and make a ticket for it. Basically what I found was that the fillAddressForm helper takes a long time, not so much things related to relay.

damassi · 2024-02-01T20:44:55Z

src/Apps/Order/Routes/Shipping2/Hooks/useHandleSaveFulfillmentDetails.ts

+      type: string
+    }
+
+export const useHandleSaveFulfillmentDetails = () => {


Maybe just useSaveFulfillmentDetails, since the handleSaveFulfillmentDetails return value is just below? No need to double up

yeah, why did I do this ... 🤔 ...

useSaveFulfillmentDetails is the useMutation hook for just that operation

This hook includes prepping the values for the different mutation inputs (pickup, ship, artsy shipping) as well as the error handling
I'll try to think about better naming.

damassi · 2024-02-01T20:47:39Z

src/System/useFeatureFlag.tsx

@@ -30,7 +30,7 @@ export function useFeatureFlag(featureName: string): boolean | null {

  if (flagEnabled === undefined) {
    warnInDevelopment(
-      "[Force] Warning: cannot find flagName in featureFlags: ",
+      `[Force] Warning: cannot find ${featureName} in featureFlags: `,


damassi

Beautiful @erikdstock 💯

cleanup: remove console logs

erikdstock requested a review from damassi December 22, 2023 22:24

erikdstock self-assigned this Dec 22, 2023

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

erikdstock changed the title ~~chore: fix remaining shipping2 tests~~ DRAFT: chore: fix remaining shipping2 tests Dec 22, 2023

erikdstock force-pushed the erik.emi1526-saved-addresses branch from fb61cce to 17ac742 Compare December 22, 2023 22:34

erikdstock commented Dec 22, 2023

View reviewed changes

damassi reviewed Dec 22, 2023

View reviewed changes

src/Apps/Order/Routes/Shipping2/Components/FulfillmentDetailsForm.tsx Outdated Show resolved Hide resolved

damassi reviewed Dec 22, 2023

View reviewed changes

src/Apps/Order/Routes/Shipping2/Components/SavedAddresses2.tsx Outdated Show resolved Hide resolved

damassi reviewed Dec 22, 2023

View reviewed changes

src/Apps/Order/Routes/Shipping2/index.tsx Outdated Show resolved Hide resolved

damassi reviewed Dec 22, 2023

View reviewed changes

yarn.lock Show resolved Hide resolved

damassi previously approved these changes Dec 22, 2023

View reviewed changes

erikdstock dismissed damassi’s stale review via e510542 December 24, 2023 22:16

erikdstock force-pushed the erik.emi1526-saved-addresses branch from e510542 to cc9bd83 Compare February 1, 2024 00:28

erikdstock commented Feb 1, 2024

View reviewed changes

erikdstock changed the title ~~DRAFT: chore: fix remaining shipping2 tests~~ chore: fix remaining shipping2 tests, complete initial refactor Feb 1, 2024

erikdstock requested a review from damassi February 1, 2024 20:27

damassi reviewed Feb 1, 2024

View reviewed changes

damassi previously approved these changes Feb 1, 2024

View reviewed changes

erikdstock dismissed damassi’s stale review via 4fd4675 February 1, 2024 22:16

erikdstock force-pushed the erik.emi1526-saved-addresses branch from d851497 to 4fd4675 Compare February 1, 2024 22:16

erikdstock added 5 commits February 2, 2024 08:17

Refactor: updating saved addresses and saving to order

fa09830

fix loop for saved addresses

0d65960

fix partner shipping saved address tests

aafa1ef

fix more tests

df0139d

rework going back to fulfillment details after edit

32d20a5

erikdstock added 13 commits February 2, 2024 08:17

new tests for SavedAddresses

c124507

re-write SavedAddresses2 tests in RTL

fe4f54c

fix tests

ed50c19

fix more tests, dont disable unshippable addresses

8557d62

cleanup: remove console logs

keep selected address radio value in shipping ctx

01e9e27

fix final tests

8126b15

fix and keep skipped AddressModal2 tests

b850f8a

cleanup

6688d47

disable spec

42ce1eb

explicit naming for hoisted formik context & run relay

a300e49

simplify & move shipping form mode out of formik context

a3558bd

re-disable tests

a6cbb3c

fix case-sensitive typo

e429d41

erikdstock force-pushed the erik.emi1526-saved-addresses branch from b8f6f08 to e429d41 Compare February 2, 2024 14:17

erikdstock merged commit 78d4083 into main Feb 2, 2024
11 checks passed

erikdstock deleted the erik.emi1526-saved-addresses branch February 2, 2024 15:02

artsy-peril bot mentioned this pull request Feb 2, 2024

Deploy #13457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix remaining shipping2 tests, complete initial refactor #13324

chore: fix remaining shipping2 tests, complete initial refactor #13324

erikdstock commented Dec 22, 2023 •

edited

Loading

erikdstock Dec 22, 2023

damassi left a comment

erikdstock Feb 1, 2024 •

edited

Loading

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024 •

edited

Loading

erikdstock left a comment

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

erikdstock Feb 1, 2024

damassi Feb 1, 2024

damassi Feb 1, 2024 •

edited

Loading

erikdstock Feb 1, 2024

damassi Feb 1, 2024

erikdstock Feb 1, 2024

damassi Feb 1, 2024

damassi left a comment

chore: fix remaining shipping2 tests, complete initial refactor #13324

chore: fix remaining shipping2 tests, complete initial refactor #13324

Conversation

erikdstock commented Dec 22, 2023 • edited Loading

Summary of changes

Functionality

Edge cases and possible bugs

Choose a reason for hiding this comment

damassi left a comment

Choose a reason for hiding this comment

erikdstock Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdstock Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

erikdstock 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

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

damassi Feb 1, 2024 • 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

Choose a reason for hiding this comment

damassi left a comment

Choose a reason for hiding this comment

erikdstock commented Dec 22, 2023 •

edited

Loading

erikdstock Feb 1, 2024 •

edited

Loading

erikdstock Feb 1, 2024 •

edited

Loading

damassi Feb 1, 2024 •

edited

Loading