Skip to content

Commit c0b6e12

Browse files
joshhealdjoshheald
committed
8079 Remove plugins.sync to fix payment cancellation
The `ReceiptEmailParameterDeterminer` synchronized plugins every time `receiptEmail` was called, i.e. when setting up the payment parameters at the start of every payment flow. This added an async call to the reader preparation flow, and if the flow was cancelled during that flow, the payment continued. This happened because `CardPresentPaymentAction.collectPayment` hadn’t been sent yet, so we didn’t have a payment to cancel. This was sent when the plugin sync completed, even if we’d cancelled the flow in the meantime. We also sync these plugins in the onboarding flow, which is checked at the start of every payment. Syncing again on the payment flow’s critical path is unneccesary. This change removes the extra sync, and thus fixes the cancellation issue.
1 parent 487f2eb commit c0b6e12

File tree

4 files changed

+81
-194
lines changed

4 files changed

+81
-194
lines changed

‎WooCommerce/Classes/ViewModels/CardPresentPayments/PaymentCaptureOrchestrator.swift‎

Lines changed: 59 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -50,57 +50,48 @@ final class PaymentCaptureOrchestrator {
5050

5151
stores.dispatch(setAccount)
5252

53-
paymentParameters(
54-
order: order,
55-
orderTotal: orderTotal,
56-
country: paymentGatewayAccount.country,
57-
statementDescriptor: paymentGatewayAccount.statementDescriptor,
58-
paymentMethodTypes: paymentMethodTypes,
59-
stripeSmallestCurrencyUnitMultiplier: stripeSmallestCurrencyUnitMultiplier
60-
) { [weak self] result in
61-
guard let self = self else { return }
62-
63-
switch result {
64-
case let .success(parameters):
65-
/// Briefly suppress pass (wallet) presentation so that the merchant doesn't attempt to pay for the buyer's order when the
66-
/// reader begins to collect payment.
67-
///
68-
self.suppressPassPresentation()
69-
70-
let paymentAction = CardPresentPaymentAction.collectPayment(
71-
siteID: order.siteID,
72-
orderID: order.orderID,
73-
parameters: parameters,
74-
onCardReaderMessage: { event in
75-
switch event {
76-
case .waitingForInput:
77-
onWaitingForInput()
78-
case .displayMessage(let message):
79-
onDisplayMessage(message)
80-
case .cardRemovedAfterClientSidePaymentCapture:
81-
onProcessingMessage()
82-
default:
83-
break
84-
}
85-
},
86-
onProcessingCompletion: { intent in
87-
onProcessingCompletion(intent)
88-
},
89-
onCompletion: { [weak self] result in
90-
self?.allowPassPresentation()
91-
self?.completePaymentIntentCapture(
92-
order: order,
93-
captureResult: result,
94-
onCompletion: onCompletion
95-
)
96-
}
53+
let parameters = paymentParameters(order: order,
54+
orderTotal: orderTotal,
55+
country: paymentGatewayAccount.country,
56+
statementDescriptor: paymentGatewayAccount.statementDescriptor,
57+
paymentMethodTypes: paymentMethodTypes,
58+
stripeSmallestCurrencyUnitMultiplier: stripeSmallestCurrencyUnitMultiplier)
59+
60+
/// Briefly suppress pass (wallet) presentation so that the merchant doesn't attempt to pay for the buyer's order when the
61+
/// reader begins to collect payment.
62+
///
63+
suppressPassPresentation()
64+
65+
let paymentAction = CardPresentPaymentAction.collectPayment(
66+
siteID: order.siteID,
67+
orderID: order.orderID,
68+
parameters: parameters,
69+
onCardReaderMessage: { event in
70+
switch event {
71+
case .waitingForInput:
72+
onWaitingForInput()
73+
case .displayMessage(let message):
74+
onDisplayMessage(message)
75+
case .cardRemovedAfterClientSidePaymentCapture:
76+
onProcessingMessage()
77+
default:
78+
break
79+
}
80+
},
81+
onProcessingCompletion: { intent in
82+
onProcessingCompletion(intent)
83+
},
84+
onCompletion: { [weak self] result in
85+
self?.allowPassPresentation()
86+
self?.completePaymentIntentCapture(
87+
order: order,
88+
captureResult: result,
89+
onCompletion: onCompletion
9790
)
98-
99-
self.stores.dispatch(paymentAction)
100-
case let .failure(error):
101-
onCompletion(Result.failure(error))
10291
}
103-
}
92+
)
93+
94+
stores.dispatch(paymentAction)
10495
}
10596

10697
func cancelPayment(onCompletion: @escaping (Result<Void, Error>) -> Void) {
@@ -204,37 +195,25 @@ private extension PaymentCaptureOrchestrator {
204195
country: String,
205196
statementDescriptor: String?,
206197
paymentMethodTypes: [String],
207-
stripeSmallestCurrencyUnitMultiplier: Decimal,
208-
onCompletion: @escaping ((Result<PaymentParameters, Error>) -> Void)) {
209-
paymentReceiptEmailParameterDeterminer.receiptEmail(from: order) { [weak self] result in
210-
guard let self = self else { return }
211-
212-
var receiptEmail: String?
213-
if case let .success(email) = result {
214-
receiptEmail = email
215-
}
216-
217-
let metadata = PaymentIntent.initMetadata(
218-
store: self.stores.sessionManager.defaultSite?.name,
219-
customerName: self.buildCustomerNameFromBillingAddress(order.billingAddress),
220-
customerEmail: order.billingAddress?.email,
221-
siteURL: self.stores.sessionManager.defaultSite?.url,
222-
orderID: order.orderID,
223-
paymentType: PaymentIntent.PaymentTypes.single
224-
)
225-
226-
let parameters = PaymentParameters(amount: orderTotal as Decimal,
227-
currency: order.currency,
228-
stripeSmallestCurrencyUnitMultiplier: stripeSmallestCurrencyUnitMultiplier,
229-
applicationFee: self.applicationFee(for: orderTotal, country: country),
230-
receiptDescription: self.receiptDescription(orderNumber: order.number),
231-
statementDescription: statementDescriptor,
232-
receiptEmail: receiptEmail,
233-
paymentMethodTypes: paymentMethodTypes,
234-
metadata: metadata)
235-
236-
onCompletion(Result.success(parameters))
237-
}
198+
stripeSmallestCurrencyUnitMultiplier: Decimal) -> PaymentParameters {
199+
let metadata = PaymentIntent.initMetadata(
200+
store: stores.sessionManager.defaultSite?.name,
201+
customerName: buildCustomerNameFromBillingAddress(order.billingAddress),
202+
customerEmail: order.billingAddress?.email,
203+
siteURL: stores.sessionManager.defaultSite?.url,
204+
orderID: order.orderID,
205+
paymentType: PaymentIntent.PaymentTypes.single
206+
)
207+
208+
return PaymentParameters(amount: orderTotal as Decimal,
209+
currency: order.currency,
210+
stripeSmallestCurrencyUnitMultiplier: stripeSmallestCurrencyUnitMultiplier,
211+
applicationFee: applicationFee(for: orderTotal, country: country),
212+
receiptDescription: receiptDescription(orderNumber: order.number),
213+
statementDescription: statementDescriptor,
214+
receiptEmail: paymentReceiptEmailParameterDeterminer.receiptEmail(from: order),
215+
paymentMethodTypes: paymentMethodTypes,
216+
metadata: metadata)
238217
}
239218

240219
private func applicationFee(for orderTotal: NSDecimalNumber, country: String) -> Decimal? {

‎WooCommerce/Classes/ViewModels/CardPresentPayments/PaymentReceiptEmailParameterDeterminer.swift‎

Lines changed: 10 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,74 +4,47 @@ import Yosemite
44
/// Determines the email to be set (if any) on a receipt
55
///
66
protocol ReceiptEmailParameterDeterminer {
7-
func receiptEmail(from order: Order, onCompletion: @escaping ((Result<String?, Error>) -> Void))
7+
func receiptEmail(from order: Order) -> String?
88
}
99

1010
/// Determines the email to be set (if any) on a payment receipt depending on the current payment plugins (WCPay, Stripe) configuration
1111
///
1212
struct PaymentReceiptEmailParameterDeterminer: ReceiptEmailParameterDeterminer {
1313
private let cardPresentPluginsDataProvider: CardPresentPluginsDataProviderProtocol
14-
private let stores: StoresManager
1514
private static let defaultConfiguration = CardPresentConfigurationLoader(stores: ServiceLocator.stores).configuration
1615

17-
init(cardPresentPluginsDataProvider: CardPresentPluginsDataProviderProtocol = CardPresentPluginsDataProvider(configuration: Self.defaultConfiguration),
18-
stores: StoresManager = ServiceLocator.stores) {
16+
init(cardPresentPluginsDataProvider: CardPresentPluginsDataProviderProtocol = CardPresentPluginsDataProvider(configuration: Self.defaultConfiguration)) {
1917
self.cardPresentPluginsDataProvider = cardPresentPluginsDataProvider
20-
self.stores = stores
2118
}
2219

2320
/// We do not need to set the receipt email if WCPay is installed and active
2421
/// and its version is higher or equal than 4.0.0, as it does it itself in that case.
2522
///
2623
/// - Parameters:
2724
/// - order: the order associated with the payment
28-
/// - onCompletion: closure invoked with the result of the inquiry, containing the email (if any) or error
25+
/// - Returns:
26+
/// - `String?`: the email for the reciept, if any. Even if there is an email, this will return `nil` for stores which send the receipt server-side.
2927
///
30-
func receiptEmail(from order: Order, onCompletion: @escaping ((Result<String?, Error>) -> Void)) {
31-
synchronizePlugins(from: order.siteID) { result in
32-
switch result {
33-
case .success():
34-
onCompletion(Result.success(receiptEmail(from: order)))
35-
case let .failure(error):
36-
onCompletion(Result.failure(error))
37-
}
38-
}
39-
}
40-
41-
private func receiptEmail(from order: Order) -> String? {
28+
func receiptEmail(from order: Order) -> String? {
4229
let wcPay = cardPresentPluginsDataProvider.getWCPayPlugin()
4330
let stripe = cardPresentPluginsDataProvider.getStripePlugin()
44-
let paymentPluginsInstalledAndActiveStatus = cardPresentPluginsDataProvider.paymentPluginsInstalledAndActiveStatus(wcPay: wcPay, stripe: stripe)
31+
let paymentPluginsStatus = cardPresentPluginsDataProvider.paymentPluginsInstalledAndActiveStatus(wcPay: wcPay, stripe: stripe)
4532

46-
guard paymentPluginsInstalledAndActiveStatus != .bothAreInstalledAndActive else {
33+
guard paymentPluginsStatus != .bothAreInstalledAndActive else {
4734
return nil
4835
}
4936

5037
guard let wcPay = wcPay,
51-
paymentPluginsInstalledAndActiveStatus == .onlyWCPayIsInstalledAndActive else {
38+
paymentPluginsStatus == .onlyWCPayIsInstalledAndActive else {
5239
return order.billingAddress?.email
5340
}
5441

5542
return wcPayPluginSendsReceiptEmail(version: wcPay.version) ? nil : order.billingAddress?.email
5643
}
5744

58-
private func synchronizePlugins(from siteID: Int64, onCompletion: @escaping ((Result<Void, Error>) -> Void)) {
59-
let systemPluginsAction = SystemStatusAction.synchronizeSystemPlugins(siteID: siteID) { result in
60-
if case let .failure(error) = result {
61-
DDLogError("[PaymentReceiptEmailParameterDeterminer] Error syncing system plugins: \(error)")
62-
onCompletion(Result.failure(error))
63-
} else {
64-
onCompletion(Result.success(()))
65-
}
66-
}
67-
68-
stores.dispatch(systemPluginsAction)
69-
}
70-
7145
private func wcPayPluginSendsReceiptEmail(version: String) -> Bool {
72-
let comparisonResult = VersionHelpers.compare(version, Constants.minimumWCPayPluginVersionThatSendsReceiptEmail)
73-
74-
return comparisonResult == .orderedDescending || comparisonResult == .orderedSame
46+
VersionHelpers.isVersionSupported(version: version,
47+
minimumRequired: Constants.minimumWCPayPluginVersionThatSendsReceiptEmail)
7548
}
7649
}
7750

‎WooCommerce/WooCommerceTests/ViewModels/CardPresentPayments/PaymentCaptureOrchestratorTests.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ final class PaymentCaptureOrchestratorTests: XCTestCase {
9494
}
9595

9696
struct MockReceiptEmailParameterDeterminer: ReceiptEmailParameterDeterminer {
97-
func receiptEmail(from order: Order, onCompletion: @escaping ((Result<String?, Error>) -> Void)) {
98-
onCompletion(.success(nil))
97+
func receiptEmail(from order: Order) -> String? {
98+
return nil
9999
}
100100
}

‎WooCommerce/WooCommerceTests/ViewModels/CardPresentPayments/PaymentReceiptEmailParameterDeterminerTests.swift‎

Lines changed: 10 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -4,47 +4,18 @@ import TestKit
44
@testable import Yosemite
55

66
final class PaymentReceiptEmailParameterDeterminerTests: XCTestCase {
7-
private var stores: MockStoresManager!
8-
9-
override func setUp() {
10-
super.setUp()
11-
12-
stores = MockStoresManager(sessionManager: SessionManager.makeForTesting())
13-
stores.whenReceivingAction(ofType: SystemStatusAction.self) { action in
14-
guard case let .synchronizeSystemPlugins(_, onCompletion) = action else {
15-
return
16-
}
17-
18-
onCompletion(.success(()))
19-
}
20-
}
21-
22-
override func tearDown() {
23-
stores = nil
24-
super.tearDown()
25-
}
26-
277
func test_when_only_WCPay_is_active_and_version_is_higher_than_minimum_that_sends_email_then_returns_nil() {
288
// Given
299
let order = Order.fake()
3010
let wcPayPlugin = SystemPlugin.fake().copy(version: "4.3.4")
3111
let cardPresentPluginsDataProvider = MockCardPresentPluginsDataProvider(wcPayPlugin: wcPayPlugin,
3212
paymentPluginsInstalledAndActiveStatus: .onlyWCPayIsInstalledAndActive)
33-
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider, stores: stores)
13+
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider)
3414

3515
// When
36-
let result: Result<String?, Error> = waitFor { promise in
37-
sut.receiptEmail(from: order) { result in
38-
promise(result)
39-
}
40-
}
16+
let email = sut.receiptEmail(from: order)
4117

4218
// Then
43-
guard case let .success(email) = result else {
44-
XCTFail()
45-
return
46-
}
47-
4819
XCTAssertNil(email)
4920
}
5021

@@ -55,21 +26,12 @@ final class PaymentReceiptEmailParameterDeterminerTests: XCTestCase {
5526
let wcPayPlugin = SystemPlugin.fake().copy(version: "4.0.0")
5627
let cardPresentPluginsDataProvider = MockCardPresentPluginsDataProvider(wcPayPlugin: wcPayPlugin,
5728
paymentPluginsInstalledAndActiveStatus: .onlyWCPayIsInstalledAndActive)
58-
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider, stores: stores)
29+
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider)
5930

6031
// When
61-
let result: Result<String?, Error> = waitFor { promise in
62-
sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress)) { result in
63-
promise(result)
64-
}
65-
}
32+
let email = sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress))
6633

6734
// Then
68-
guard case let .success(email) = result else {
69-
XCTFail()
70-
return
71-
}
72-
7335
XCTAssertNil(email)
7436
}
7537

@@ -80,21 +42,12 @@ final class PaymentReceiptEmailParameterDeterminerTests: XCTestCase {
8042
let wcPayPlugin = SystemPlugin.fake().copy(version: "3.9.9")
8143
let cardPresentPluginsDataProvider = MockCardPresentPluginsDataProvider(wcPayPlugin: wcPayPlugin,
8244
paymentPluginsInstalledAndActiveStatus: .onlyWCPayIsInstalledAndActive)
83-
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider, stores: stores)
45+
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider)
8446

8547
// When
86-
let result: Result<String?, Error> = waitFor { promise in
87-
sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress)) { result in
88-
promise(result)
89-
}
90-
}
48+
let returnedEmail = sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress))
9149

9250
// Then
93-
guard case let .success(returnedEmail) = result else {
94-
XCTFail()
95-
return
96-
}
97-
9851
XCTAssertEqual(returnedEmail, receiptEmail)
9952
}
10053

@@ -103,21 +56,12 @@ final class PaymentReceiptEmailParameterDeterminerTests: XCTestCase {
10356
let receiptEmail = "[email protected]"
10457
let billingAddress = Address.fake().copy(email: receiptEmail)
10558
let cardPresentPluginsDataProvider = MockCardPresentPluginsDataProvider(paymentPluginsInstalledAndActiveStatus: .bothAreInstalledAndActive)
106-
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider, stores: stores)
59+
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider)
10760

10861
// When
109-
let result: Result<String?, Error> = waitFor { promise in
110-
sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress)) { result in
111-
promise(result)
112-
}
113-
}
62+
let email = sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress))
11463

11564
// Then
116-
guard case let .success(email) = result else {
117-
XCTFail()
118-
return
119-
}
120-
12165
XCTAssertNil(email)
12266
}
12367

@@ -126,21 +70,12 @@ final class PaymentReceiptEmailParameterDeterminerTests: XCTestCase {
12670
let receiptEmail = "[email protected]"
12771
let billingAddress = Address.fake().copy(email: receiptEmail)
12872
let cardPresentPluginsDataProvider = MockCardPresentPluginsDataProvider(paymentPluginsInstalledAndActiveStatus: .onlyStripeIsInstalledAndActive)
129-
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider, stores: stores)
73+
let sut = PaymentReceiptEmailParameterDeterminer(cardPresentPluginsDataProvider: cardPresentPluginsDataProvider)
13074

13175
// When
132-
let result: Result<String?, Error> = waitFor { promise in
133-
sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress)) { result in
134-
promise(result)
135-
}
136-
}
76+
let returnedEmail = sut.receiptEmail(from: Order.fake().copy(billingAddress: billingAddress))
13777

13878
// Then
139-
guard case let .success(returnedEmail) = result else {
140-
XCTFail()
141-
return
142-
}
143-
14479
XCTAssertEqual(returnedEmail, receiptEmail)
14580
}
14681
}

0 commit comments

Comments
 (0)