From 7e474011ed1e0c2447a78f15da49f3cbd2f2f0fe Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 14:29:11 +0000 Subject: [PATCH 1/8] Show network error when onboarding requests fail --- .../Stores/CardPresentPaymentStore.swift | 22 ++++++++++++++----- ...CardPresentPaymentsOnboardingUseCase.swift | 20 ++++++++++++++--- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift index e7cf55566ff..5f896513780 100644 --- a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift +++ b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift @@ -444,6 +444,7 @@ private extension CardPresentPaymentStore { /// func loadAccounts(siteID: Int64, onCompletion: @escaping (Result) -> Void) { var error: Error? = nil + var hasSuccess: Bool? = nil let group = DispatchGroup() group.enter() @@ -453,7 +454,7 @@ private extension CardPresentPaymentStore { DDLogError("⛔️ Error synchronizing WCPay Account: \(loadError)") error = loadError case .success: - break + hasSuccess = true } group.leave() }) @@ -465,17 +466,24 @@ private extension CardPresentPaymentStore { DDLogError("⛔️ Error synchronizing Stripe Account: \(loadError)") error = loadError case .success: - break + hasSuccess = true } group.leave() }) group.notify(queue: .main) { - guard let error = error else { + switch (hasSuccess, error) { + case (true, _): + // If either succeeds, the load is successful onCompletion(.success(())) - return + case (false, .some(let error)), + (.none, .some(let error)): + // If we have an error, and no success, the load fails + onCompletion(.failure(error)) + case (_, .none): + // This... shouldn't really happen. + onCompletion(.failure(CardPresentPaymentStoreError.unknownErrorFetchingAccounts)) } - onCompletion(.failure(error)) } } @@ -676,6 +684,10 @@ public enum ServerSidePaymentCaptureError: Error, LocalizedError { } } +public enum CardPresentPaymentStoreError: Error { + case unknownErrorFetchingAccounts +} + private extension PaymentGatewayAccount { var isWCPay: Bool { self.gatewayID == WCPayAccount.gatewayID diff --git a/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift b/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift index 97f19fe621c..71dbdd781ca 100644 --- a/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift +++ b/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift @@ -4,6 +4,7 @@ import Storage import Yosemite import Experiments import WooFoundation +import Alamofire private typealias SystemPlugin = Yosemite.SystemPlugin private typealias PaymentGatewayAccount = Yosemite.PaymentGatewayAccount @@ -180,8 +181,17 @@ final class CardPresentPaymentsOnboardingUseCase: CardPresentPaymentsOnboardingU return } - self.updateState() - CardPresentPaymentOnboardingStateCache.shared.update(self.state) + switch result { + case .success: + updateState() + CardPresentPaymentOnboardingStateCache.shared.update(state) + case .failure(let error): + if isNetworkError(error) { + state = .noConnectionError + } else { + state = .genericError + } + } } stores.dispatch(paymentGatewayAccountsAction) } @@ -548,7 +558,11 @@ private extension CardPresentPaymentsOnboardingUseCase { } func isNetworkError(_ error: Error) -> Bool { - (error as NSError).domain == NSURLErrorDomain + if let afError = error as? AFError { + return true + } else { + return (error as NSError).domain == NSURLErrorDomain + } } } From e28b79d02f9756cbaff2d3f14ca1e89ddb58d5fb Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 15:45:17 +0000 Subject: [PATCH 2/8] Add card payments error handling to 23.7 release --- RELEASE-NOTES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 1ea56c7bdd5..431bef4a976 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -3,7 +3,7 @@ 23.7 ----- - +- [*] Improve card payments onboarding error handling to show network errors correctly [https://github.com/woocommerce/woocommerce-ios/pull/16304] 23.6 ----- From efbef875f051fe430066794de781869bd82b58e2 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 15:58:56 +0000 Subject: [PATCH 3/8] Simplify switch --- Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift index 5f896513780..051b2452667 100644 --- a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift +++ b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift @@ -476,8 +476,7 @@ private extension CardPresentPaymentStore { case (true, _): // If either succeeds, the load is successful onCompletion(.success(())) - case (false, .some(let error)), - (.none, .some(let error)): + case (_, .some(let error)): // If we have an error, and no success, the load fails onCompletion(.failure(error)) case (_, .none): From 8878969545d8fc0e92ad2cf3137a86d969f0a428 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 16:21:08 +0000 Subject: [PATCH 4/8] Add tests for loadAccounts result propagation --- .../Stores/CardPresentPaymentStoreTests.swift | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift b/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift index 364ef86d1b0..b7b4b2ff1e9 100644 --- a/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift +++ b/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift @@ -297,6 +297,84 @@ final class CardPresentPaymentStoreTests: XCTestCase { XCTAssert(storageAccount?.status == "complete") } + /// Verifies that loadAccounts succeeds when only WCPay succeeds and Stripe fails. + /// + func test_loadAccounts_succeeds_when_only_wcpay_succeeds() throws { + let expectation = self.expectation(description: "Load Account succeeds with WCPay only") + network.simulateResponse(requestUrlSuffix: "payments/accounts", + filename: "wcpay-account-complete") + network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", + filename: "generic_error") + + let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in + XCTAssertTrue(result.isSuccess) + expectation.fulfill() + }) + + cardPresentStore.onAction(action) + wait(for: [expectation], timeout: Constants.expectationTimeout) + + XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1) + + let storageAccount = viewStorage.loadPaymentGatewayAccount( + siteID: sampleSiteID, + gatewayID: WCPayAccount.gatewayID + ) + + XCTAssert(storageAccount?.siteID == sampleSiteID) + XCTAssert(storageAccount?.gatewayID == WCPayAccount.gatewayID) + XCTAssert(storageAccount?.status == "complete") + } + + /// Verifies that loadAccounts succeeds when only Stripe succeeds and WCPay fails. + /// + func test_loadAccounts_succeeds_when_only_stripe_succeeds() throws { + let expectation = self.expectation(description: "Load Account succeeds with Stripe only") + network.simulateResponse(requestUrlSuffix: "payments/accounts", + filename: "generic_error") + network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", + filename: "stripe-account-complete") + + let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in + XCTAssertTrue(result.isSuccess) + expectation.fulfill() + }) + + cardPresentStore.onAction(action) + wait(for: [expectation], timeout: Constants.expectationTimeout) + + XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1) + + let storageAccount = viewStorage.loadPaymentGatewayAccount( + siteID: sampleSiteID, + gatewayID: StripeAccount.gatewayID + ) + + XCTAssert(storageAccount?.siteID == sampleSiteID) + XCTAssert(storageAccount?.gatewayID == StripeAccount.gatewayID) + XCTAssert(storageAccount?.status == "complete") + } + + /// Verifies that loadAccounts returns an error when both WCPay and Stripe fail. + /// + func test_loadAccounts_fails_when_both_wcpay_and_stripe_fail() throws { + let expectation = self.expectation(description: "Load Account fails when both fail") + network.simulateResponse(requestUrlSuffix: "payments/accounts", + filename: "generic_error") + network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", + filename: "generic_error") + + let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in + XCTAssertTrue(result.isFailure) + expectation.fulfill() + }) + + cardPresentStore.onAction(action) + wait(for: [expectation], timeout: Constants.expectationTimeout) + + XCTAssertNil(viewStorage.firstObject(ofType: Storage.PaymentGatewayAccount.self, matching: nil)) + } + /// Verifies that the store hits the network when fetching a charge, and propagates success. /// func test_fetchWCPayCharge_returns_expected_data() throws { From 903e6d40cf878cdfc1029d74ab6f23fa304f2b2c Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 16:48:50 +0000 Subject: [PATCH 5/8] Add tests for onboarding fixes --- ...resentPaymentsOnboardingUseCaseTests.swift | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/WooCommerce/WooCommerceTests/ViewRelated/CardPresentPayments/CardPresentPaymentsOnboardingUseCaseTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/CardPresentPayments/CardPresentPaymentsOnboardingUseCaseTests.swift index 5168f586e44..17b2f63bfdd 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/CardPresentPayments/CardPresentPaymentsOnboardingUseCaseTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/CardPresentPayments/CardPresentPaymentsOnboardingUseCaseTests.swift @@ -1,6 +1,7 @@ import XCTest import Fakes import Yosemite +import enum Alamofire.AFError @testable import WooCommerce class CardPresentPaymentsOnboardingUseCaseTests: XCTestCase { @@ -1226,6 +1227,62 @@ class CardPresentPaymentsOnboardingUseCaseTests: XCTestCase { XCTAssertEqual(CardPresentPaymentsPlugin.stripe.plugin, .stripe) XCTAssertEqual(CardPresentPaymentsPlugin.stripe.fileNameWithPathExtension, "woocommerce-gateway-stripe/woocommerce-gateway-stripe") } + + // MARK: - loadAccounts error handling tests + + func test_onboarding_handles_network_error_when_loading_accounts() { + // Given + setupCountry(country: .us) + setupWCPayPlugin(status: .active, version: WCPayPluginVersion.minimumSupportedVersion) + + // Use AFError.sessionTaskFailed which is what Alamofire returns for network errors + let urlError = URLError(.notConnectedToInternet) + let networkError = AFError.sessionTaskFailed(error: urlError) + stores.whenReceivingAction(ofType: CardPresentPaymentAction.self) { action in + switch action { + case let .loadAccounts(_, onCompletion): + onCompletion(.failure(networkError)) + default: + break + } + } + + // When + let useCase = CardPresentPaymentsOnboardingUseCase(storageManager: storageManager, + stores: stores, + cardPresentPaymentOnboardingStateCache: onboardingStateCache) + useCase.updateAccounts() + + // Then - Should show no connection error + XCTAssertEqual(useCase.state, .noConnectionError) + } + + func test_onboarding_handles_generic_error_when_both_accounts_fail_to_load() { + // Given + setupCountry(country: .us) + setupWCPayPlugin(status: .active, version: WCPayPluginVersion.minimumSupportedVersion) + setupStripePlugin(status: .active, version: StripePluginVersion.minimumSupportedVersion) + + let genericError = NSError(domain: "test.error", code: 500, userInfo: nil) + stores.whenReceivingAction(ofType: CardPresentPaymentAction.self) { action in + switch action { + case let .loadAccounts(_, onCompletion): + // Both accounts fail to load + onCompletion(.failure(genericError)) + default: + break + } + } + + // When + let useCase = CardPresentPaymentsOnboardingUseCase(storageManager: storageManager, + stores: stores, + cardPresentPaymentOnboardingStateCache: onboardingStateCache) + useCase.updateAccounts() + + // Then - Should show generic error + XCTAssertEqual(useCase.state, .genericError) + } } // MARK: - Settings helpers From d1f8d8a4abb0460d3c5f42a43c7b3cb79f95a03f Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 31 Oct 2025 16:49:55 +0000 Subject: [PATCH 6/8] Ignore periphery --- Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift index 051b2452667..6619d05c423 100644 --- a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift +++ b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift @@ -683,6 +683,7 @@ public enum ServerSidePaymentCaptureError: Error, LocalizedError { } } +//periphery:ignore - logging this error detail in WooCommerce is useful, if it ever happens. It's part of the public API here. public enum CardPresentPaymentStoreError: Error { case unknownErrorFetchingAccounts } From 5c17b789921069d9d96b617717c16ac21c3a3715 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Mon, 3 Nov 2025 09:25:33 +0000 Subject: [PATCH 7/8] Only import AFError Co-authored-by: Jaclyn Chen --- .../CardPresentPaymentsOnboardingUseCase.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift b/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift index 71dbdd781ca..bb8563d1cf8 100644 --- a/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift +++ b/WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift @@ -4,7 +4,7 @@ import Storage import Yosemite import Experiments import WooFoundation -import Alamofire +import enum Alamofire.AFError private typealias SystemPlugin = Yosemite.SystemPlugin private typealias PaymentGatewayAccount = Yosemite.PaymentGatewayAccount From eb5c1b92811b974ee19fa5895104a024dd198427 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Mon, 3 Nov 2025 09:38:07 +0000 Subject: [PATCH 8/8] Update tests to use `waitFor` helper --- .../Stores/CardPresentPaymentStoreTests.swift | 79 ++++++++++--------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift b/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift index b7b4b2ff1e9..ab3e112a0a3 100644 --- a/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift +++ b/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift @@ -252,19 +252,20 @@ final class CardPresentPaymentStoreTests: XCTestCase { /// Verifies that the PaymentGatewayAccountStore hits the network when loading a WCPay Account and places nothing in storage in case of error. /// func test_loadAccounts_handles_failure() throws { - let expectation = self.expectation(description: "Load Account error response") network.simulateResponse(requestUrlSuffix: "payments/accounts", filename: "generic_error") network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", filename: "generic_error") - let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in - XCTAssertTrue(result.isFailure) - expectation.fulfill() - }) + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } - cardPresentStore.onAction(action) - wait(for: [expectation], timeout: Constants.expectationTimeout) + // Then + XCTAssertTrue(result.isFailure) XCTAssertNil(viewStorage.firstObject(ofType: Storage.PaymentGatewayAccount.self, matching: nil)) } @@ -272,18 +273,19 @@ final class CardPresentPaymentStoreTests: XCTestCase { /// Verifies that the PaymentGatewayAccountStore hits the network when loading a WCPay Account, propagates success and upserts the account into storage. /// func test_loadAccounts_returns_expected_data() throws { - let expectation = self.expectation(description: "Load Account fetch response") network.simulateResponse(requestUrlSuffix: "payments/accounts", filename: "wcpay-account-complete") network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", filename: "stripe-account-complete") - let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in - XCTAssertTrue(result.isSuccess) - expectation.fulfill() - }) + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } - cardPresentStore.onAction(action) - wait(for: [expectation], timeout: Constants.expectationTimeout) + // Then + XCTAssertTrue(result.isSuccess) XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 2) @@ -300,19 +302,21 @@ final class CardPresentPaymentStoreTests: XCTestCase { /// Verifies that loadAccounts succeeds when only WCPay succeeds and Stripe fails. /// func test_loadAccounts_succeeds_when_only_wcpay_succeeds() throws { - let expectation = self.expectation(description: "Load Account succeeds with WCPay only") + // Given network.simulateResponse(requestUrlSuffix: "payments/accounts", filename: "wcpay-account-complete") network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", filename: "generic_error") - let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in - XCTAssertTrue(result.isSuccess) - expectation.fulfill() - }) + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } - cardPresentStore.onAction(action) - wait(for: [expectation], timeout: Constants.expectationTimeout) + // Then + XCTAssertTrue(result.isSuccess) XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1) @@ -329,19 +333,21 @@ final class CardPresentPaymentStoreTests: XCTestCase { /// Verifies that loadAccounts succeeds when only Stripe succeeds and WCPay fails. /// func test_loadAccounts_succeeds_when_only_stripe_succeeds() throws { - let expectation = self.expectation(description: "Load Account succeeds with Stripe only") + // Given network.simulateResponse(requestUrlSuffix: "payments/accounts", filename: "generic_error") network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", filename: "stripe-account-complete") - let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in - XCTAssertTrue(result.isSuccess) - expectation.fulfill() - }) + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } - cardPresentStore.onAction(action) - wait(for: [expectation], timeout: Constants.expectationTimeout) + // Then + XCTAssertTrue(result.isSuccess) XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1) @@ -358,19 +364,20 @@ final class CardPresentPaymentStoreTests: XCTestCase { /// Verifies that loadAccounts returns an error when both WCPay and Stripe fail. /// func test_loadAccounts_fails_when_both_wcpay_and_stripe_fail() throws { - let expectation = self.expectation(description: "Load Account fails when both fail") + // Given network.simulateResponse(requestUrlSuffix: "payments/accounts", filename: "generic_error") network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", filename: "generic_error") + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } - let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in - XCTAssertTrue(result.isFailure) - expectation.fulfill() - }) - - cardPresentStore.onAction(action) - wait(for: [expectation], timeout: Constants.expectationTimeout) + // Then + XCTAssertTrue(result.isFailure) XCTAssertNil(viewStorage.firstObject(ofType: Storage.PaymentGatewayAccount.self, matching: nil)) }