feat(EMI-2112): failed payment order history #10839

erikdstock · 2024-09-24T19:40:50Z

This PR resolves EMI-2112

Description

This PR adds a payment failed state to the order history along with a CTA to fix the failed payment. The work to expose that new payment failed state is still in progress, so there are no user-facing changes yet.

The getOrderStatus() method now returns a string | null rather than the string 'unknown'. Unhandled (null) values are not rendered in the app - this should never happen.

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.

📱Order history (failed payment state simulated)

📱After clicking CTA

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

Enhancements to the order history screen when an order has a failed payment

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.

src/app/utils/getOrderStatus.ts

erikdstock · 2024-09-24T19:42:59Z

src/app/utils/getOrderStatus.ts

+    case "PAYMENT_FAILED":
+      return "payment failed"
    default:
      return "unknown"


1: Need to check where else this function is used to make sure this text won't render in an unexpected place.
2: The 'unknown' text feels not ideal. I considered suppressing it on the OrderHistoryRow and just rendering no status in those cases.

+1 to the second part. I do think that not displaying anything if state is not known is better than displaying 'unknown' which just look like a bug to me :)

ArtsyOpenSource · 2024-09-24T20:00:55Z

This PR contains the following changes:

Cross-platform user-facing changes (Enhancements to the order history screen when an order has a failed payment)

Generated by 🚫 dangerJS against 2076a7b

oxaudo

This looks good to me. Only had couple of neat picky comments here. Though def defer to our app specialists to weight in on best practices.

oxaudo · 2024-09-24T20:35:30Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

+  const hasFailedPayment = orderStatus === "payment failed"
+  const isActive =
+    !hasFailedPayment &&
+    orderStatus &&


Interesting as I believe looking at getOrderStatus below that orderStatus will always be present as there is a default value. So checking it seems redundant?

Ah, at first I was returning null as the default instead of unknown. Then decided that was out of scope since we are handling all of the known cases, so best to leave it as I found it.

oxaudo · 2024-09-24T20:39:53Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

+    : "black60"
+
+  const hasFailedPayment = orderStatus === "payment failed"
+  const isActive =


#minor - think this isActive is a bit misleading now as it controls if we are showing the link. And we are showing the button in more cases now just a different button. Maybe something more descriptive as showDetailsButton and showPaymentUpdateButton ?

oxaudo · 2024-09-24T20:51:42Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

+  const isActive =
+    !hasFailedPayment &&
+    orderStatus &&
+    !(["canceled", "refunded"] as string[]).includes(orderStatus)


if we are doing it array style here maybe we can just include payment failed here?

oxaudo · 2024-09-24T20:53:14Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

-            >
-              {orderStatus}
-            </Text>
+            {!!orderStatus && (


This would make sense if orderStatus could possibly be nil, but don't think is a valid pass now. Though I also think that not displaying anything for status is better than displaying the word 'unknown'

oxaudo · 2024-09-24T20:56:47Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

+            variant="fillDark"
+            onPress={() =>
+              navigate(`/orders/${order.internalID}/payment/new`, {
+                modal: true,


Interesting that there are modals in the app. Did not even know those exist.

This is a style of system-level navigation where instead of adding a view to a stack you are popping up a single web view that acts independently.

oxaudo · 2024-09-24T20:58:09Z

src/app/utils/getOrderStatus.ts

+    case "PAYMENT_FAILED":
+      return "payment failed"
    default:
      return "unknown"


+1 to the second part. I do think that not displaying anything if state is not known is better than displaying 'unknown' which just look like a bug to me :)

MrSltun

Looks good! 🌟 I just added a few comments

MrSltun · 2024-09-25T09:43:18Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tests.tsx

+    renderWithRelay({ CommerceOrder: () => mockOrder })

-    expect(tree.queryByTestId("artist-names")?.children[0]).toBe("Torbjørn Rødland")
+    expect(screen.getByTestId("artist-names")).toHaveTextContent("Torbjørn Rødland")
  })

  it("displays the partner name", () => {
-    const tree = renderWithRelay({ CommerceOrder: () => mockOrder })
+    renderWithRelay({ CommerceOrder: () => mockOrder })

-    expect(tree.queryByTestId("partner-name")?.children[0]).toBe("Andrea Festa Fine Art")
+    expect(screen.getByTestId("partner-name")).toHaveTextContent("Andrea Festa Fine Art")
  })

  it("displays the order creation date", () => {
-    const tree = renderWithRelay({ CommerceOrder: () => mockOrder })
+    renderWithRelay({ CommerceOrder: () => mockOrder })

-    expect(tree.queryByTestId("date")?.children[0]).toBe("5/18/2021")
+    expect(screen.queryByTestId("date")).toHaveTextContent("5/18/2021")
  })

  it("displays the price", () => {
-    const tree = renderWithRelay({ CommerceOrder: () => mockOrder })
+    renderWithRelay({ CommerceOrder: () => mockOrder })

-    expect(tree.queryByTestId("price")?.children[0]).toBe("11,200")
+    expect(screen.getByTestId("price")).toHaveTextContent("11,200")
  })

  it("displays the display state", () => {
-    const tree = renderWithRelay({ CommerceOrder: () => mockOrder })
+    renderWithRelay({ CommerceOrder: () => mockOrder })

-    expect(tree.queryByTestId("order-status")?.children[0]).toBe("pending")
+    expect(screen.getByTestId("order-status")).toHaveTextContent("pending")


Thank you for refactoring this! 🙏

src/app/Scenes/OrderHistory/OrderHistoryRow.tests.tsx

MrSltun · 2024-09-25T09:47:46Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

+  const isActive =
+    !hasFailedPayment &&
+    orderStatus &&
+    !(["canceled", "refunded"] as string[]).includes(orderStatus)


Question: do we need the type casting as string[] here?

MrSltun · 2024-09-25T09:48:35Z

src/app/Scenes/OrderHistory/OrderHistoryRow.tsx

-            >
-              {orderStatus}
-            </Text>
+            {!!orderStatus && (


This will always return true, since getOrderStatus always returns a string that is not empty, maybe we could change the getOrderStatus return to give back null or an empty string?

Yeah, the checks like this are because I was returning null at first, then decided not to mess with it. I'll take a closer look and see if that looks safe in the other places we use it.

MrSltun · 2024-09-25T09:53:44Z

src/app/utils/getOrderStatus.ts

@@ -1,6 +1,6 @@
 import { CommerceOrderDisplayStateEnum } from "__generated__/OrderDetailsHeader_info.graphql"

-export function getOrderStatus(displayState: CommerceOrderDisplayStateEnum) {
+export function getOrderStatus(displayState: CommerceOrderDisplayStateEnum): string {


Question: Do we need the type for the return type to be string? I believe Typescript infers a type based on the returning value of the function on its own

Another relic of when I added types because it was string | null - when I changed it I also changed the types.

MrSltun · 2024-09-25T09:54:58Z

@erikdstock can we also test these changes on Android and add screenshots?

erikdstock · 2024-09-25T12:43:58Z

@erikdstock can we also test these changes on Android and add screenshots?

I did test them on android - will see if i can get some screenshots together after responding to feedback

- clean up linter errors in test

src/app/utils/getOrderStatus.ts

Co-authored-by: Sultan Al-Maari <[email protected]>

erikdstock requested a review from MrSltun September 24, 2024 19:40

erikdstock self-assigned this Sep 24, 2024

artsy-peril bot added the Jira Synced label Sep 24, 2024

erikdstock commented Sep 24, 2024

View reviewed changes

erikdstock marked this pull request as ready for review September 24, 2024 19:59

erikdstock force-pushed the erik.emi2112-failed-payment-order-history branch from 6c34604 to cd17866 Compare September 24, 2024 20:58

oxaudo previously approved these changes Sep 24, 2024

View reviewed changes

MrSltun reviewed Sep 25, 2024

View reviewed changes

MrSltun previously approved these changes Sep 25, 2024

View reviewed changes

erikdstock dismissed stale reviews from MrSltun and oxaudo via 5183487 September 25, 2024 19:57

erikdstock added 4 commits September 25, 2024 17:14

feat: display payment failed with cta on order history

9330d00

- clean up linter errors in test

update modal title

23e93af

nullable order status and handling

a80b41a

cleanup

019047b

erikdstock force-pushed the erik.emi2112-failed-payment-order-history branch from a63ceef to 019047b Compare September 25, 2024 22:14

MrSltun reviewed Sep 26, 2024

View reviewed changes

src/app/utils/getOrderStatus.ts Outdated Show resolved Hide resolved

erikdstock and others added 3 commits September 26, 2024 09:13

respond to feedback

55c00a4

Update src/app/Scenes/OrderHistory/OrderHistoryRow.tests.tsx

2f641e2

Co-authored-by: Sultan Al-Maari <[email protected]>

responed to feedback

2076a7b

erikdstock merged commit 19ce985 into main Sep 26, 2024
7 checks passed

erikdstock deleted the erik.emi2112-failed-payment-order-history branch September 26, 2024 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(EMI-2112): failed payment order history #10839

feat(EMI-2112): failed payment order history #10839

erikdstock commented Sep 24, 2024 •

edited

Loading

erikdstock Sep 24, 2024

oxaudo Sep 24, 2024

ArtsyOpenSource commented Sep 24, 2024 •

edited

Loading

oxaudo left a comment

oxaudo Sep 24, 2024

erikdstock Sep 25, 2024

oxaudo Sep 24, 2024

oxaudo Sep 24, 2024

oxaudo Sep 24, 2024

oxaudo Sep 24, 2024

erikdstock Sep 25, 2024

oxaudo Sep 24, 2024

MrSltun left a comment

MrSltun Sep 25, 2024

MrSltun Sep 25, 2024

MrSltun Sep 25, 2024

erikdstock Sep 25, 2024

MrSltun Sep 25, 2024

erikdstock Sep 25, 2024

MrSltun commented Sep 25, 2024

erikdstock commented Sep 25, 2024

feat(EMI-2112): failed payment order history #10839

feat(EMI-2112): failed payment order history #10839

Conversation

erikdstock commented Sep 24, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtsyOpenSource commented Sep 24, 2024 • edited Loading

This PR contains the following changes:

oxaudo 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

MrSltun 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

MrSltun commented Sep 25, 2024

erikdstock commented Sep 25, 2024

erikdstock commented Sep 24, 2024 •

edited

Loading

ArtsyOpenSource commented Sep 24, 2024 •

edited

Loading