[Order Details] Allow failed orders to show receipt if eligible #15558

iamgabrielma · 2025-04-28T05:32:45Z

Description

This PR updates the logic around rendering "See Receipt" in Order Details to take into account that now failed orders can also contain receipts. Before this, we only shows receipts for orders with datePaid != nil, but this is unreliable.

Screen.Recording.2025-04-28.at.13.37.39.mov

Changes

Made explicit isOrderStatusEligibleForReceipt by checking specific order statuses, rather than rely on datePaid
Added a case to handle showing .failed receipts if the site is eligible for sending them.
Removed references to wcPluginDevVersion, we no longer need to check dev versions.
Refactored isEligibleForFailedPaymentEmailReceipts to not use async await. The mix between this bit and the rest of the closure-based logic in Order Details seems to produce inconsistent results. Ref: p1745894442252789-slack-CGPNUU63E

Testing information

In order to mock a failed transaction via card present, you can simulate a payment for an order that ends in $0.05, eg: product valued at $1.05 taxes included.
Confirm that "See Receipt" appears and the receipt can be opened, confirm the status is "Failed"
Confirm that you receive the failed email as well:

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

Right now receipt eligibility relies on datePaid not being nil, but this is unreliable: - failed purchases won’t have a datePaid value - successful purchases that have switched to a different order status will keep having the datePaid value.

wpmobilebot · 2025-04-28T05:46:54Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`29849`
	Version	`PR #15558`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`3862c54`
	Installation URL	0ibrh1vfk9soo

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

We no longer need to check if supported for dev versions

…n-receipt

The async-await version seems to be thread-unreliable with the Order Details logic: p1745897000519949-slack-CGPNUU63E

jaclync

Thanks for starting the discussion yesterday, I didn't fully understand the issue until I reviewed the PR. While the closure based approach works, I think we can address the root cause in order details regarding the async receipt eligibility check. I'd also suggest adding a new function in the receipt eligibility use case to check for receipt eligibility given an order status for easier unit testing and simpler call from order details.

WooCommerce/Classes/ViewModels/Order Details/OrderDetailsDataSource.swift

WooCommerce/Classes/ViewModels/Order Details/Receipts/ReceiptEligibilityUseCase.swift

wpmobilebot · 2025-05-02T03:34:41Z

Version 22.3 has now entered code-freeze, so the milestone of this PR has been updated to 22.4.

…n-receipt # Conflicts: # WooCommerce/Classes/ViewModels/Order Details/OrderDetailsDataSource.swift

jaclync · 2025-05-07T08:12:29Z

Hi @iamgabrielma, lemme know if the PR is ready for review and I'll get to it tomorrow the latest!

…n-receipt

iamgabrielma · 2025-05-09T03:14:24Z

Hi @iamgabrielma, lemme know if the PR is ready for review and I'll get to it tomorrow the latest!

Thanks! While testing it after the latest changes to async rows, we seem to have introduced a bug in the isEligibleForBackendReceipt logic: since date.paid property is set during processing before we have the chance to reach receipt eligibility use case, we always skip it because does not pass the guard. There's also the side effect of any order that already has a date.paid set but later on switches state (either failed again or a correct state) won't generate a receipt for the same reason.

Should be fine to remove this guard now that we rely on specific order status when checking the order requirements, but I'll test this today before trying to wrap up this PR 👍

iamgabrielma · 2025-05-09T03:40:22Z

This is ready for review @jaclync , this is simplified from the first approach:

Receipts eligibility only relies on meetsOrderStatusRequirement, I've restored the original async-await implementation since the order details now can handle it.
Removed the logic to handle dev woocommerce versions

I'm not sure why the diff for ReceiptEligibilityUseCaseTests shows the some tests as they show, for example test_when_WooCommerce_version_is_below_minimum_then_returns_false shows the version 8.5 has been updated to ...8.5. I suspect because I removed some dev-related tests at the beginning of the file, if you check for the tests that shows these modifications you'll see that haven't changed:

test_when_WooCommerce_version_is_equal_or_above_minimum_then_returns_true
test_isEligibleForFailedPaymentEmailReceipts_when_plugins_are_inactive_then_returns_false
test_isEligibleForFailedPaymentEmailReceipts_when_plugins_are_supported_then_returns_true
test_isEligibleForFailedPaymentEmailReceipts_when_woopayments_version_is_incorrect_then_returns_false
test_isEligibleForFailedPaymentEmailReceipts_when_stripe_gateway_then_returns_true
test_isEligibleForFailedPaymentEmailReceipts_when_stripe_gateway_is_outdated_then_returns_false

jaclync

Works as expected, thanks for the updates it's much easier to review now! Mostly a question about support for dev/beta versions.

jaclync · 2025-05-12T01:57:23Z

WooCommerce/Classes/ViewModels/Order Details/Receipts/ReceiptEligibilityUseCase.swift

@@ -5,6 +5,7 @@ protocol ReceiptEligibilityUseCaseProtocol {
    func isEligibleForBackendReceipts(onCompletion: @escaping (Bool) -> Void)
    func isEligibleForSuccessfulPaymentEmailReceipts(onCompletion: @escaping (Bool) -> Void)
    func isEligibleForFailedPaymentEmailReceipts(paymentGatewayID: String, onCompletion: @escaping (Bool) -> Void)
+    func meetsOrderStatusRequirement(_ orderStatus: OrderStatusEnum, onCompletion: @escaping (Bool) -> Void)


nit: how about keeping a consistent name isEligibleForReceipts(_ orderStatus: OrderStatusEnum, onCompletion: @escaping (Bool) -> Void)? When I saw the function name without the implementation, I thought this function just checks for if the order status meets the requirement.

nit: how about making this new function async/await friendly? It'd simplify the use case in OrderDetailsDataSource.

Agree: 9eb0773

I'll leave the closure based for now, so I don't have to redo the tests 👍

jaclync · 2025-05-12T02:03:35Z

WooCommerce/Classes/ViewModels/Order Details/Receipts/ReceiptEligibilityUseCase.swift

-                // Checking for concrete versions to cover dev and beta versions
-                if plugin.version.contains(minimumVersion) {
-                    return continuation.resume(returning: true)
-                }
-


❓ What are the reasons for removing the check that supports dev and beta versions?
Saw this mentioned in the PR description:

Removed the logic to handle dev woocommerce versions

When testing with Woo Beta Tester plugin or for core development, I thought it's common to have the WC version as the dev/beta versions.

Yes, this was very specific to this project: The internals of VersionHelpers used in the same function already checks for beta and dev version that might come with Woo Beta Tester, so it will pass the check as expected. This additional check was put in place just for testing, due to how the changes were provided by core folks with specific development branches to be installed in the test site, which sometimes were behind the latest versions.

I'm actually having a similar plugin version check in a separate PR and removing this check blocks -beta/-dev/-alpha/-rc. From the test cases for the VersionHelpers, this blocking behavior also seems intentional:

woocommerce-ios/WooCommerce/WooCommerceTests/Extensions/VersionHelpersTests.swift

Lines 34 to 40 in d6d6c56

VersionTestCase(foundVersion: "1.0.0-dev", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-alpha", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-a", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-beta", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-b", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-RC1", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

VersionTestCase(foundVersion: "1.0.0-rc1", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

If we want to support alpha/beta/dev/rc builds of the same version, I think we do need to keep this check.

If we want to support alpha/beta/dev/rc builds of the same version

Interesting, I see what you mean: Anything tagged as -dev or other for the same version we're checking, is always below the non-tagged version:

po VersionHelpers.isVersionSupported(version: "9.0-dev", minimumRequired: "9.0.0") false po VersionHelpers.isVersionSupported(version: "9.0.1-dev", minimumRequired: "9.0.0") true

jaclync · 2025-05-12T02:04:23Z

WooCommerce/Classes/ViewModels/Order Details/Receipts/ReceiptEligibilityUseCase.swift

 }

 private extension ReceiptEligibilityUseCase {
+    func isPluginSupported(_ pluginName: String, minimumVersion: String, onCompletion: @escaping (Bool) -> Void) {


nit: is this still used?

Is not! d2fda6c

jaclync · 2025-05-12T03:24:20Z

WooCommerce/WooCommerceTests/ViewModels/Receipts/ReceiptEligibilityUseCaseTests.swift

@@ -32,11 +31,11 @@ final class ReceiptEligibilityUseCaseTests: XCTestCase {
        XCTAssertFalse(isEligible)
    }

-    func test_when_WooCommerce_version_is_correct_dev_version_then_returns_true() {


If we do decide not to support WC dev version, it might be worth keeping the test and flipping the assertion.

Same as above, the dev/beta versions that follow the actual core deployment process are already covered by the version helper class. This was a very specific case on how the feature was delivered by core (eg: feature released publicly on 9.7 last minute, but we had to work with 9.6 internally) so we added the tests as well. Now are no longer necessary as it's part of the correct core version.

jaclync · 2025-05-12T03:27:49Z

WooCommerce/WooCommerceTests/ViewModels/Receipts/ReceiptEligibilityUseCaseTests.swift

+        XCTAssertFalse(isEligible)
+    }
+
+    func test_meetsOrderStatusRequirement_with_completed_status_returns_true() {


nit: would be nice keeping the Given/When/Then comments for consistency and clearer structure of the test case. Same for other meetsOrderStatusRequirement test cases as well.

True that! fcbbcb0

jaclync · 2025-05-12T03:28:42Z

WooCommerce/WooCommerceTests/ViewModels/Receipts/ReceiptEligibilityUseCaseTests.swift

+        stores.whenReceivingAction(ofType: SystemStatusAction.self) { action in
+            switch action {
+            case let .fetchSystemPlugin(_, _, onCompletion):
+                onCompletion(plugin)
+            default:
+                XCTFail("Unexpected action")
+            }
+        }


nit: this could be extracted to private helper function for reuse.

jaclync · 2025-05-12T03:32:33Z

WooCommerce/WooCommerceTests/ViewModels/Receipts/ReceiptEligibilityUseCaseTests.swift

        XCTAssertTrue(isEligible)
    }

-    func test_isEligibleForFailedPaymentEmailReceipts_when_woopayments_version_is_incorrect_then_returns_false() {
-        // Given
+    func test_meetsOrderStatusRequirement_with_processing_status_returns_true() {


nit: might be worth mentioning the condition when it's true (otherwise it could sound like processing status always meets the requirement) - from my understanding, it's when the WC version is above a minimum version. Same for the refunded status test case.

jaclync · 2025-05-12T03:34:30Z

WooCommerce/Classes/ViewModels/Order Details/Receipts/ReceiptEligibilityUseCase.swift

+            }
+        case .failed:
+            selectedPaymentGatewayID { [weak self] gatewayID in
+                guard let gatewayID = gatewayID else {


super nit:

Suggested change

guard let gatewayID = gatewayID else {

guard let gatewayID else {

2b185a6 🚀

dangermattic · 2025-05-14T11:45:33Z

	1 Warning
⚠️	This PR is assigned to the milestone 22.4. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

…n-receipt

iamgabrielma added 2 commits April 28, 2025 11:14

Update eligibility based on order status

f6c670d

Right now receipt eligibility relies on datePaid not being nil, but this is unreliable: - failed purchases won’t have a datePaid value - successful purchases that have switched to a different order status will keep having the datePaid value.

Handle receipt eligibility when failed order

c0b5a70

iamgabrielma added type: task An internally driven task. feature: order details Related to order details. Enhancement labels Apr 28, 2025

iamgabrielma added this to the 22.3 milestone Apr 28, 2025

move funcs to private extension

7711057

iamgabrielma added 7 commits April 28, 2025 13:05

typo: remove internal comment

c26e0a8

clean up dev version

e8a3690

We no longer need to check if supported for dev versions

Rename and lint

f353f87

Merge branch 'trunk' into issue/woomob-104-allow-failed-orders-to-ope…

1459a25

…n-receipt

dev and beta version checks no longer needed

c3dc2dd

Move gateway check to use case

701421c

refactor isEligibleForFailedPaymentEmailReceipts to closure-based

26ca6d3

The async-await version seems to be thread-unreliable with the Order Details logic: p1745897000519949-slack-CGPNUU63E

iamgabrielma marked this pull request as ready for review April 29, 2025 03:40

update test

c055fe5

iamgabrielma requested a review from jaclync April 29, 2025 03:51

jaclync reviewed Apr 30, 2025

View reviewed changes

wpmobilebot modified the milestones: 22.3, 22.4 May 2, 2025

iamgabrielma mentioned this pull request May 2, 2025

[Order Details] Refactor order detail rows to account for async operations #15588

Merged

3 tasks

iamgabrielma added 5 commits May 6, 2025 17:32

Merge branch 'trunk' into issue/woomob-104-allow-failed-orders-to-ope…

0aa4a1c

…n-receipt # Conflicts: # WooCommerce/Classes/ViewModels/Order Details/OrderDetailsDataSource.swift

restore original check for payment eligibility

66b212a

lint

8f20e0f

encapsulate meetsOrderStatusRequirement in receipt eligibility use case

c231384

update protocol

af4010d

Merge branch 'trunk' into issue/woomob-104-allow-failed-orders-to-ope…

843a4fa

…n-receipt

iamgabrielma added 5 commits May 9, 2025 10:20

check for order status requirements on eligibility

bff46d4

restore previous implementation

a1bbfac

restore enum name. unnecessary change

b7e4771

clean up

56ad1d0

release notes

bb54851

iamgabrielma requested a review from jaclync May 9, 2025 03:40

jaclync approved these changes May 12, 2025

View reviewed changes

iamgabrielma added 5 commits May 14, 2025 18:21

remove unused function

d2fda6c

rename for consistency

9eb0773

structure tests

fcbbcb0

more explicit on test name

a04d1d7

remove unnecessary check

2b185a6

Merge branch 'trunk' into issue/woomob-104-allow-failed-orders-to-ope…

3862c54

…n-receipt

iamgabrielma enabled auto-merge May 14, 2025 11:53

iamgabrielma merged commit 12c4369 into trunk May 14, 2025
13 checks passed

iamgabrielma deleted the issue/woomob-104-allow-failed-orders-to-open-receipt branch May 14, 2025 12:12

	VersionTestCase(foundVersion: "1.0.0-dev", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-alpha", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-a", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-beta", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-b", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-RC1", requiredMinimumVersion: "1.0.0", meetsMinimum: false),
	VersionTestCase(foundVersion: "1.0.0-rc1", requiredMinimumVersion: "1.0.0", meetsMinimum: false),

	guard let gatewayID = gatewayID else {
	guard let gatewayID else {

[Order Details] Allow failed orders to show receipt if eligible #15558

[Order Details] Allow failed orders to show receipt if eligible #15558

Uh oh!

Conversation

iamgabrielma commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing information

Reviewer (or Author, in the case of optional code reviews):

Uh oh!

wpmobilebot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpmobilebot commented May 2, 2025

Uh oh!

jaclync commented May 7, 2025

Uh oh!

iamgabrielma commented May 9, 2025

Uh oh!

iamgabrielma commented May 9, 2025

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangermattic commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

iamgabrielma commented Apr 28, 2025 •

edited

Loading

wpmobilebot commented Apr 28, 2025 •

edited

Loading

jaclync left a comment •

edited

Loading