Skip to content

Commit d9ffd67

Browse files
joshhealdjoshheald
authored
[Mobile Payments] Show network error when onboarding requests fail (#16304)
2 parents e712965 + eb5c1b9 commit d9ffd67

File tree

5 files changed

+191
-23
lines changed

5 files changed

+191
-23
lines changed

‎Modules/Sources/Yosemite/Stores/CardPresentPaymentStore.swift‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,7 @@ private extension CardPresentPaymentStore {
444444
///
445445
func loadAccounts(siteID: Int64, onCompletion: @escaping (Result<Void, Error>) -> Void) {
446446
var error: Error? = nil
447+
var hasSuccess: Bool? = nil
447448

448449
let group = DispatchGroup()
449450
group.enter()
@@ -453,7 +454,7 @@ private extension CardPresentPaymentStore {
453454
DDLogError("⛔️ Error synchronizing WCPay Account: \(loadError)")
454455
error = loadError
455456
case .success:
456-
break
457+
hasSuccess = true
457458
}
458459
group.leave()
459460
})
@@ -465,17 +466,23 @@ private extension CardPresentPaymentStore {
465466
DDLogError("⛔️ Error synchronizing Stripe Account: \(loadError)")
466467
error = loadError
467468
case .success:
468-
break
469+
hasSuccess = true
469470
}
470471
group.leave()
471472
})
472473

473474
group.notify(queue: .main) {
474-
guard let error = error else {
475+
switch (hasSuccess, error) {
476+
case (true, _):
477+
// If either succeeds, the load is successful
475478
onCompletion(.success(()))
476-
return
479+
case (_, .some(let error)):
480+
// If we have an error, and no success, the load fails
481+
onCompletion(.failure(error))
482+
case (_, .none):
483+
// This... shouldn't really happen.
484+
onCompletion(.failure(CardPresentPaymentStoreError.unknownErrorFetchingAccounts))
477485
}
478-
onCompletion(.failure(error))
479486
}
480487
}
481488

@@ -676,6 +683,11 @@ public enum ServerSidePaymentCaptureError: Error, LocalizedError {
676683
}
677684
}
678685

686+
//periphery:ignore - logging this error detail in WooCommerce is useful, if it ever happens. It's part of the public API here.
687+
public enum CardPresentPaymentStoreError: Error {
688+
case unknownErrorFetchingAccounts
689+
}
690+
679691
private extension PaymentGatewayAccount {
680692
var isWCPay: Bool {
681693
self.gatewayID == WCPayAccount.gatewayID

‎Modules/Tests/YosemiteTests/Stores/CardPresentPaymentStoreTests.swift‎

Lines changed: 99 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -252,38 +252,40 @@ final class CardPresentPaymentStoreTests: XCTestCase {
252252
/// Verifies that the PaymentGatewayAccountStore hits the network when loading a WCPay Account and places nothing in storage in case of error.
253253
///
254254
func test_loadAccounts_handles_failure() throws {
255-
let expectation = self.expectation(description: "Load Account error response")
256255
network.simulateResponse(requestUrlSuffix: "payments/accounts",
257256
filename: "generic_error")
258257
network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary",
259258
filename: "generic_error")
260259

261-
let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in
262-
XCTAssertTrue(result.isFailure)
263-
expectation.fulfill()
264-
})
260+
// When
261+
let result = waitFor { promise in
262+
self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in
263+
promise(result)
264+
}))
265+
}
265266

266-
cardPresentStore.onAction(action)
267-
wait(for: [expectation], timeout: Constants.expectationTimeout)
267+
// Then
268+
XCTAssertTrue(result.isFailure)
268269

269270
XCTAssertNil(viewStorage.firstObject(ofType: Storage.PaymentGatewayAccount.self, matching: nil))
270271
}
271272

272273
/// Verifies that the PaymentGatewayAccountStore hits the network when loading a WCPay Account, propagates success and upserts the account into storage.
273274
///
274275
func test_loadAccounts_returns_expected_data() throws {
275-
let expectation = self.expectation(description: "Load Account fetch response")
276276
network.simulateResponse(requestUrlSuffix: "payments/accounts",
277277
filename: "wcpay-account-complete")
278278
network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary",
279279
filename: "stripe-account-complete")
280-
let action = CardPresentPaymentAction.loadAccounts(siteID: sampleSiteID, onCompletion: { result in
281-
XCTAssertTrue(result.isSuccess)
282-
expectation.fulfill()
283-
})
280+
// When
281+
let result = waitFor { promise in
282+
self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in
283+
promise(result)
284+
}))
285+
}
284286

285-
cardPresentStore.onAction(action)
286-
wait(for: [expectation], timeout: Constants.expectationTimeout)
287+
// Then
288+
XCTAssertTrue(result.isSuccess)
287289

288290
XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 2)
289291

@@ -297,6 +299,89 @@ final class CardPresentPaymentStoreTests: XCTestCase {
297299
XCTAssert(storageAccount?.status == "complete")
298300
}
299301

302+
/// Verifies that loadAccounts succeeds when only WCPay succeeds and Stripe fails.
303+
///
304+
func test_loadAccounts_succeeds_when_only_wcpay_succeeds() throws {
305+
// Given
306+
network.simulateResponse(requestUrlSuffix: "payments/accounts",
307+
filename: "wcpay-account-complete")
308+
network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary",
309+
filename: "generic_error")
310+
311+
// When
312+
let result = waitFor { promise in
313+
self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in
314+
promise(result)
315+
}))
316+
}
317+
318+
// Then
319+
XCTAssertTrue(result.isSuccess)
320+
321+
XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1)
322+
323+
let storageAccount = viewStorage.loadPaymentGatewayAccount(
324+
siteID: sampleSiteID,
325+
gatewayID: WCPayAccount.gatewayID
326+
)
327+
328+
XCTAssert(storageAccount?.siteID == sampleSiteID)
329+
XCTAssert(storageAccount?.gatewayID == WCPayAccount.gatewayID)
330+
XCTAssert(storageAccount?.status == "complete")
331+
}
332+
333+
/// Verifies that loadAccounts succeeds when only Stripe succeeds and WCPay fails.
334+
///
335+
func test_loadAccounts_succeeds_when_only_stripe_succeeds() throws {
336+
// Given
337+
network.simulateResponse(requestUrlSuffix: "payments/accounts",
338+
filename: "generic_error")
339+
network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary",
340+
filename: "stripe-account-complete")
341+
342+
// When
343+
let result = waitFor { promise in
344+
self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in
345+
promise(result)
346+
}))
347+
}
348+
349+
// Then
350+
XCTAssertTrue(result.isSuccess)
351+
352+
XCTAssert(viewStorage.countObjects(ofType: Storage.PaymentGatewayAccount.self, matching: nil) == 1)
353+
354+
let storageAccount = viewStorage.loadPaymentGatewayAccount(
355+
siteID: sampleSiteID,
356+
gatewayID: StripeAccount.gatewayID
357+
)
358+
359+
XCTAssert(storageAccount?.siteID == sampleSiteID)
360+
XCTAssert(storageAccount?.gatewayID == StripeAccount.gatewayID)
361+
XCTAssert(storageAccount?.status == "complete")
362+
}
363+
364+
/// Verifies that loadAccounts returns an error when both WCPay and Stripe fail.
365+
///
366+
func test_loadAccounts_fails_when_both_wcpay_and_stripe_fail() throws {
367+
// Given
368+
network.simulateResponse(requestUrlSuffix: "payments/accounts",
369+
filename: "generic_error")
370+
network.simulateResponse(requestUrlSuffix: "wc_stripe/account/summary",
371+
filename: "generic_error")
372+
// When
373+
let result = waitFor { promise in
374+
self.cardPresentStore.onAction(CardPresentPaymentAction.loadAccounts(siteID: self.sampleSiteID, onCompletion: { result in
375+
promise(result)
376+
}))
377+
}
378+
379+
// Then
380+
XCTAssertTrue(result.isFailure)
381+
382+
XCTAssertNil(viewStorage.firstObject(ofType: Storage.PaymentGatewayAccount.self, matching: nil))
383+
}
384+
300385
/// Verifies that the store hits the network when fetching a charge, and propagates success.
301386
///
302387
func test_fetchWCPayCharge_returns_expected_data() throws {

‎RELEASE-NOTES.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
23.7
55
-----
6-
6+
- [*] Improve card payments onboarding error handling to show network errors correctly [https://github.com/woocommerce/woocommerce-ios/pull/16304]
77

88
23.6
99
-----

‎WooCommerce/Classes/ViewRelated/Dashboard/Settings/In-Person Payments/CardPresentPaymentsOnboardingUseCase.swift‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import Storage
44
import Yosemite
55
import Experiments
66
import WooFoundation
7+
import enum Alamofire.AFError
78

89
private typealias SystemPlugin = Yosemite.SystemPlugin
910
private typealias PaymentGatewayAccount = Yosemite.PaymentGatewayAccount
@@ -180,8 +181,17 @@ final class CardPresentPaymentsOnboardingUseCase: CardPresentPaymentsOnboardingU
180181
return
181182
}
182183

183-
self.updateState()
184-
CardPresentPaymentOnboardingStateCache.shared.update(self.state)
184+
switch result {
185+
case .success:
186+
updateState()
187+
CardPresentPaymentOnboardingStateCache.shared.update(state)
188+
case .failure(let error):
189+
if isNetworkError(error) {
190+
state = .noConnectionError
191+
} else {
192+
state = .genericError
193+
}
194+
}
185195
}
186196
stores.dispatch(paymentGatewayAccountsAction)
187197
}
@@ -548,7 +558,11 @@ private extension CardPresentPaymentsOnboardingUseCase {
548558
}
549559

550560
func isNetworkError(_ error: Error) -> Bool {
551-
(error as NSError).domain == NSURLErrorDomain
561+
if let afError = error as? AFError {
562+
return true
563+
} else {
564+
return (error as NSError).domain == NSURLErrorDomain
565+
}
552566
}
553567
}
554568

‎WooCommerce/WooCommerceTests/ViewRelated/CardPresentPayments/CardPresentPaymentsOnboardingUseCaseTests.swift‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import XCTest
22
import Fakes
33
import Yosemite
4+
import enum Alamofire.AFError
45
@testable import WooCommerce
56

67
class CardPresentPaymentsOnboardingUseCaseTests: XCTestCase {
@@ -1226,6 +1227,62 @@ class CardPresentPaymentsOnboardingUseCaseTests: XCTestCase {
12261227
XCTAssertEqual(CardPresentPaymentsPlugin.stripe.plugin, .stripe)
12271228
XCTAssertEqual(CardPresentPaymentsPlugin.stripe.fileNameWithPathExtension, "woocommerce-gateway-stripe/woocommerce-gateway-stripe")
12281229
}
1230+
1231+
// MARK: - loadAccounts error handling tests
1232+
1233+
func test_onboarding_handles_network_error_when_loading_accounts() {
1234+
// Given
1235+
setupCountry(country: .us)
1236+
setupWCPayPlugin(status: .active, version: WCPayPluginVersion.minimumSupportedVersion)
1237+
1238+
// Use AFError.sessionTaskFailed which is what Alamofire returns for network errors
1239+
let urlError = URLError(.notConnectedToInternet)
1240+
let networkError = AFError.sessionTaskFailed(error: urlError)
1241+
stores.whenReceivingAction(ofType: CardPresentPaymentAction.self) { action in
1242+
switch action {
1243+
case let .loadAccounts(_, onCompletion):
1244+
onCompletion(.failure(networkError))
1245+
default:
1246+
break
1247+
}
1248+
}
1249+
1250+
// When
1251+
let useCase = CardPresentPaymentsOnboardingUseCase(storageManager: storageManager,
1252+
stores: stores,
1253+
cardPresentPaymentOnboardingStateCache: onboardingStateCache)
1254+
useCase.updateAccounts()
1255+
1256+
// Then - Should show no connection error
1257+
XCTAssertEqual(useCase.state, .noConnectionError)
1258+
}
1259+
1260+
func test_onboarding_handles_generic_error_when_both_accounts_fail_to_load() {
1261+
// Given
1262+
setupCountry(country: .us)
1263+
setupWCPayPlugin(status: .active, version: WCPayPluginVersion.minimumSupportedVersion)
1264+
setupStripePlugin(status: .active, version: StripePluginVersion.minimumSupportedVersion)
1265+
1266+
let genericError = NSError(domain: "test.error", code: 500, userInfo: nil)
1267+
stores.whenReceivingAction(ofType: CardPresentPaymentAction.self) { action in
1268+
switch action {
1269+
case let .loadAccounts(_, onCompletion):
1270+
// Both accounts fail to load
1271+
onCompletion(.failure(genericError))
1272+
default:
1273+
break
1274+
}
1275+
}
1276+
1277+
// When
1278+
let useCase = CardPresentPaymentsOnboardingUseCase(storageManager: storageManager,
1279+
stores: stores,
1280+
cardPresentPaymentOnboardingStateCache: onboardingStateCache)
1281+
useCase.updateAccounts()
1282+
1283+
// Then - Should show generic error
1284+
XCTAssertEqual(useCase.state, .genericError)
1285+
}
12291286
}
12301287

12311288
// MARK: - Settings helpers

0 commit comments

Comments
 (0)