-
Notifications
You must be signed in to change notification settings - Fork 118
[Order Details] Allow failed orders to show receipt if eligible #15558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
f6c670d
c0b5a70
7711057
c26e0a8
e8a3690
f353f87
1459a25
c3dc2dd
701421c
26ca6d3
c055fe5
0aa4a1c
66b212a
8f20e0f
c231384
af4010d
843a4fa
bff46d4
a1bbfac
b7e4771
56ad1d0
bb54851
d2fda6c
9eb0773
fcbbcb0
a04d1d7
2b185a6
3862c54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -27,15 +27,11 @@ final class ReceiptEligibilityUseCase: ReceiptEligibilityUseCaseProtocol { | |||||||||||||||
guard let wcPlugin = wcPlugin, wcPlugin.active else { | ||||||||||||||||
return onCompletion(false) | ||||||||||||||||
} | ||||||||||||||||
// 2. If WooCommerce version is any of the specific API development branches, mark as eligible | ||||||||||||||||
if Constants.BackendReceipt.wcPluginDevVersion.contains(wcPlugin.version) { | ||||||||||||||||
onCompletion(true) | ||||||||||||||||
} else { | ||||||||||||||||
// 3. Else, if WooCommerce version is higher than minimum required version, mark as eligible | ||||||||||||||||
let isSupported = VersionHelpers.isVersionSupported(version: wcPlugin.version, | ||||||||||||||||
minimumRequired: Constants.BackendReceipt.wcPluginMinimumVersion) | ||||||||||||||||
onCompletion(isSupported) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// 2. If WooCommerce version is higher than minimum required version, mark as eligible | ||||||||||||||||
let isSupported = VersionHelpers.isVersionSupported(version: wcPlugin.version, | ||||||||||||||||
minimumRequired: Constants.BackendReceipt.wcPluginMinimumVersion) | ||||||||||||||||
onCompletion(isSupported) | ||||||||||||||||
} | ||||||||||||||||
stores.dispatch(action) | ||||||||||||||||
} | ||||||||||||||||
|
@@ -69,30 +65,52 @@ final class ReceiptEligibilityUseCase: ReceiptEligibilityUseCaseProtocol { | |||||||||||||||
/// WooCommerce Stripe Gateway 9.1.0 aligns the app with the web and automatically sets the order as failed when the payment processing fails. | ||||||||||||||||
/// | ||||||||||||||||
func isEligibleForFailedPaymentEmailReceipts(paymentGatewayID: String, onCompletion: @escaping (Bool) -> Void) { | ||||||||||||||||
Task { @MainActor in | ||||||||||||||||
async let wooCommerceSupported = isPluginSupported(Constants.wcPluginName, | ||||||||||||||||
minimumVersion: Constants.ReceiptAfterPayment.wcPluginMinimumVersion) | ||||||||||||||||
|
||||||||||||||||
async let gatewaySupported: Bool = { | ||||||||||||||||
switch paymentGatewayID { | ||||||||||||||||
case CardPresentPaymentsPlugin.wcPay.gatewayID: | ||||||||||||||||
return await isPluginSupported(CardPresentPaymentsPlugin.wcPay.pluginName, | ||||||||||||||||
minimumVersion: Constants.ReceiptAfterPayment.wcPayPluginMinimumVersion) | ||||||||||||||||
case CardPresentPaymentsPlugin.stripe.gatewayID: | ||||||||||||||||
return await isPluginSupported(CardPresentPaymentsPlugin.stripe.pluginName, | ||||||||||||||||
minimumVersion: Constants.ReceiptAfterPayment.stripePluginMinimumVersion) | ||||||||||||||||
default: | ||||||||||||||||
return false | ||||||||||||||||
} | ||||||||||||||||
}() | ||||||||||||||||
isPluginSupported(Constants.wcPluginName, minimumVersion: Constants.FailedReceiptAfterPayment.wcPluginMinimumVersion) { isWooCommerceSupported in | ||||||||||||||||
guard isWooCommerceSupported else { | ||||||||||||||||
return onCompletion(false) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
switch paymentGatewayID { | ||||||||||||||||
case CardPresentPaymentsPlugin.wcPay.gatewayID: | ||||||||||||||||
self.isPluginSupported(CardPresentPaymentsPlugin.wcPay.pluginName, | ||||||||||||||||
minimumVersion: Constants.FailedReceiptAfterPayment.wcPayPluginMinimumVersion, | ||||||||||||||||
onCompletion: onCompletion) | ||||||||||||||||
case CardPresentPaymentsPlugin.stripe.gatewayID: | ||||||||||||||||
self.isPluginSupported(CardPresentPaymentsPlugin.stripe.pluginName, | ||||||||||||||||
minimumVersion: Constants.FailedReceiptAfterPayment.stripePluginMinimumVersion, | ||||||||||||||||
onCompletion: onCompletion) | ||||||||||||||||
default: | ||||||||||||||||
onCompletion(false) | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
iamgabrielma marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
|
||||||||||||||||
let (isWooCommerceSupported, isGatewaySupported) = await (wooCommerceSupported, gatewaySupported) | ||||||||||||||||
onCompletion(isWooCommerceSupported && isGatewaySupported) | ||||||||||||||||
// Returns the current payment gateway ID, needed when checking if a transaction is eligible for receipts | ||||||||||||||||
// based on which plugin and versions are active | ||||||||||||||||
func selectedPaymentGatewayID(onCompletion: @escaping (String?) -> Void) { | ||||||||||||||||
let action = CardPresentPaymentAction.selectedPaymentGatewayAccount { paymentGatewayAccount in | ||||||||||||||||
onCompletion(paymentGatewayAccount?.gatewayID) | ||||||||||||||||
} | ||||||||||||||||
stores.dispatch(action) | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private extension ReceiptEligibilityUseCase { | ||||||||||||||||
func isPluginSupported(_ pluginName: String, minimumVersion: String, onCompletion: @escaping (Bool) -> Void) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: is this still used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is not! d2fda6c |
||||||||||||||||
let action = SystemStatusAction.fetchSystemPlugin(siteID: siteID, systemPluginName: pluginName) { plugin in | ||||||||||||||||
// Plugin must be installed and active | ||||||||||||||||
guard let plugin, plugin.active else { | ||||||||||||||||
return onCompletion(false) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// If plugin version is higher than minimum required version, mark as eligible | ||||||||||||||||
let isSupported = VersionHelpers.isVersionSupported(version: plugin.version, | ||||||||||||||||
minimumRequired: minimumVersion) | ||||||||||||||||
onCompletion(isSupported) | ||||||||||||||||
} | ||||||||||||||||
stores.dispatch(action) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
@MainActor | ||||||||||||||||
func isPluginSupported(_ pluginName: String, minimumVersion: String) async -> Bool { | ||||||||||||||||
await withCheckedContinuation { continuation in | ||||||||||||||||
|
@@ -102,11 +120,6 @@ private extension ReceiptEligibilityUseCase { | |||||||||||||||
return continuation.resume(returning: false) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// Checking for concrete versions to cover dev and beta versions | ||||||||||||||||
if plugin.version.contains(minimumVersion) { | ||||||||||||||||
return continuation.resume(returning: true) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
Comment on lines
-105
to
-109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ What are the reasons for removing the check that supports dev and beta 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was very specific to this project: The internals of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually having a similar plugin version check in a separate PR and removing this check blocks woocommerce-ios/WooCommerce/WooCommerceTests/Extensions/VersionHelpersTests.swift Lines 34 to 40 in d6d6c56
If we want to support alpha/beta/dev/rc builds of the same version, I think we do need to keep this check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
|
||||||||||||||||
// If plugin version is higher than minimum required version, mark as eligible | ||||||||||||||||
let isSupported = VersionHelpers.isVersionSupported(version: plugin.version, | ||||||||||||||||
minimumRequired: minimumVersion) | ||||||||||||||||
|
@@ -123,10 +136,9 @@ private extension ReceiptEligibilityUseCase { | |||||||||||||||
|
||||||||||||||||
enum BackendReceipt { | ||||||||||||||||
static let wcPluginMinimumVersion = "8.7.0" | ||||||||||||||||
static let wcPluginDevVersion: [String] = ["8.7.0-dev", "8.6.0-dev"] | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
enum ReceiptAfterPayment { | ||||||||||||||||
enum FailedReceiptAfterPayment { | ||||||||||||||||
static let wcPluginMinimumVersion = "9.5.0" | ||||||||||||||||
static let wcPayPluginMinimumVersion = "8.6.0" | ||||||||||||||||
static let stripePluginMinimumVersion = "9.1.0" | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,63 +3,6 @@ import Yosemite | |
@testable import WooCommerce | ||
|
||
final class ReceiptEligibilityUseCaseTests: XCTestCase { | ||
|
||
func test_when_WooCommerce_version_is_incorrect_dev_version_then_returns_false() { | ||
// Given | ||
let stores = MockStoresManager(sessionManager: .makeForTesting()) | ||
let plugin = SystemPlugin.fake().copy(name: "WooCommerce", | ||
version: "8.6.0-dev-wrong-version", | ||
active: true) | ||
|
||
stores.whenReceivingAction(ofType: SystemStatusAction.self) { action in | ||
switch action { | ||
case let .fetchSystemPlugin(_, _, onCompletion): | ||
onCompletion(plugin) | ||
default: | ||
XCTFail("Unexpected action") | ||
} | ||
} | ||
let sut = ReceiptEligibilityUseCase(stores: stores) | ||
|
||
// When | ||
let isEligible: Bool = waitFor { promise in | ||
sut.isEligibleForBackendReceipts(onCompletion: { result in | ||
promise(result) | ||
}) | ||
} | ||
|
||
// Then | ||
XCTAssertFalse(isEligible) | ||
} | ||
|
||
func test_when_WooCommerce_version_is_correct_dev_version_then_returns_true() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do decide not to support WC dev version, it might be worth keeping the test and flipping the assertion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// Given | ||
let stores = MockStoresManager(sessionManager: .makeForTesting()) | ||
let plugin = SystemPlugin.fake().copy(name: "WooCommerce", | ||
version: "8.6.0-dev", | ||
active: true) | ||
|
||
stores.whenReceivingAction(ofType: SystemStatusAction.self) { action in | ||
switch action { | ||
case let .fetchSystemPlugin(_, _, onCompletion): | ||
onCompletion(plugin) | ||
default: | ||
XCTFail("Unexpected action") | ||
} | ||
} | ||
let sut = ReceiptEligibilityUseCase(stores: stores) | ||
|
||
// When | ||
let isEligible: Bool = waitFor { promise in | ||
sut.isEligibleForBackendReceipts(onCompletion: { result in | ||
promise(result) | ||
}) | ||
} | ||
|
||
// Then | ||
XCTAssertTrue(isEligible) | ||
} | ||
|
||
func test_when_WooCommerce_version_is_below_minimum_then_returns_false() { | ||
// Given | ||
let stores = MockStoresManager(sessionManager: .makeForTesting()) | ||
|
@@ -182,38 +125,6 @@ final class ReceiptEligibilityUseCaseTests: XCTestCase { | |
XCTAssertTrue(isEligible) | ||
} | ||
|
||
func test_isEligibleForFailedPaymentEmailReceipts_when_plugins_are_supported_dev_then_returns_true() { | ||
// Given | ||
let stores = MockStoresManager(sessionManager: .makeForTesting()) | ||
let wooCommercePlugin = SystemPlugin.fake().copy(name: "WooCommerce", version: "9.6.0-dev-1181231238", active: true) | ||
let wooPaymentsPlugin = SystemPlugin.fake().copy(name: CardPresentPaymentsPlugin.wcPay.pluginName, version: "8.6.0-test-1", active: true) | ||
|
||
stores.whenReceivingAction(ofType: SystemStatusAction.self) { action in | ||
switch action { | ||
case let .fetchSystemPlugin(_, systemPluginName, onCompletion): | ||
if systemPluginName == "WooCommerce" { | ||
onCompletion(wooCommercePlugin) | ||
} else if systemPluginName == CardPresentPaymentsPlugin.wcPay.pluginName { | ||
onCompletion(wooPaymentsPlugin) | ||
} | ||
default: | ||
XCTFail("Unexpected action") | ||
} | ||
} | ||
|
||
let sut = ReceiptEligibilityUseCase(stores: stores) | ||
|
||
// When | ||
let isEligible: Bool = waitFor { promise in | ||
sut.isEligibleForFailedPaymentEmailReceipts(paymentGatewayID: CardPresentPaymentsPlugin.wcPay.gatewayID, onCompletion: { result in | ||
promise(result) | ||
}) | ||
} | ||
|
||
// Then | ||
XCTAssertTrue(isEligible) | ||
} | ||
|
||
func test_isEligibleForFailedPaymentEmailReceipts_when_woopayments_version_is_incorrect_then_returns_false() { | ||
// Given | ||
let stores = MockStoresManager(sessionManager: .makeForTesting()) | ||
|
Uh oh!
There was an error while loading. Please reload this page.