diff --git a/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift b/Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift index e7cf55566ff..6619d05c423 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,23 @@ 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 (_, .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 +683,11 @@ 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 +} + private extension PaymentGatewayAccount { var isWCPay: Bool { self.gatewayID == WCPayAccount.gatewayID diff --git a/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift b/Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift index 364ef86d1b0..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) @@ -297,6 +299,89 @@ 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 { + // Given + network.simulateResponse(requestUrlSuffix: "payments/accounts", + filename: "wcpay-account-complete") + 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) + })) + } + + // Then + XCTAssertTrue(result.isSuccess) + + 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 { + // Given + network.simulateResponse(requestUrlSuffix: "payments/accounts", + filename: "generic_error") + network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary", + filename: "stripe-account-complete") + + // When + let result = waitFor { promise in + self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in + promise(result) + })) + } + + // Then + XCTAssertTrue(result.isSuccess) + + 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 { + // 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) + })) + } + + // Then + XCTAssertTrue(result.isFailure) + + 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 { 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 ----- 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..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,6 +4,7 @@ import Storage import Yosemite import Experiments import WooFoundation +import enum Alamofire.AFError 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 + } } } 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