Skip to content
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.

Sign up for GitHub

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

[Mobile Payments] Show description for trial tap to pay line items #14022

Merged
merged 5 commits into from
Sep 30, 2024
Merged

[Mobile Payments] Show description for trial tap to pay line items #14022

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
74a25b0
Show description for trial tap to pay line items
joshheald Sep 23, 2024
5d2f923
Add TTP trial payment name to 20.6 release notes
joshheald Sep 23, 2024
4ac7845
Fix lint issues
joshheald Sep 23, 2024
0fc6343
Merge branch 'trunk' into hack/name-ttp-test-amounts
joshheald Sep 30, 2024
aa099f9
Move name for TTP test amounts to 20.7 release
joshheald Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- [*] Jetpack setup: fixed issue checking site info after activating Jetpack for sites with Jetpack connection package. [https://github.com/woocommerce/woocommerce-ios/pull/13993]
- [*] Blaze: Campaign status is now updated immediately after being canceled. [https://github.com/woocommerce/woocommerce-ios/pull/13992]
- [internal] Logging for app storage size added in Core Data crash logs [https://github.com/woocommerce/woocommerce-ios/pull/14008]
- [*] Payments: Give a specific label to Tap to Pay trial payments [https://github.com/woocommerce/woocommerce-ios/pull/14022]

20.5
-----
Expand Down
7 changes: 7 additions & 0 deletions ...ViewRelated/Dashboard/Settings/CardReadersV2/SetUpTapToPayTryPaymentPromptViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ final class SetUpTapToPayTryPaymentPromptViewModel: PaymentSettingsFlowPresented
self.summaryViewModel = TryAPaymentSummaryViewModel(
simplePaymentSummaryViewModel: SimplePaymentsSummaryViewModel(order: order,
providedAmount: order.total,
amountName: Localization.tryAPaymentAmountName,
presentNoticeSubject: self.presentNoticeSubject,
analyticsFlow: .tapToPayTryAPayment),
siteID: self.siteID,
Expand Down Expand Up @@ -236,6 +237,12 @@ extension SetUpTapToPayTryPaymentPromptViewModel {
value: "Trial Tap to Pay payment auto refund",
comment: "After a trial Tap to Pay payment, we attempt to automatically refund the test amount. When " +
"this is sent to the server, we provide a reason for later identification.")

static let tryAPaymentAmountName = NSLocalizedString(
"tap.to.pay.try.payment.amount.name",
value: "Try out Tap to Pay on iPhone",
comment: "On a trial Tap to Pay order, this is the name of the line item for the trial amount."
)
}
}

Expand Down
1 change: 1 addition & 0 deletions WooCommerce/Classes/ViewRelated/Orders/Simple Payments/Summary/SimplePaymentsSummary.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ struct SimplePaymentsSummary_Preview: PreviewProvider {
title: "State Tax (5.55%)",
value: taxAmount)
return .init(providedAmount: "40.0",
amountName: nil,
totalWithTaxes: "$42.3",
taxLines: [taxLine],
noteContent: noteContent)
Expand Down
7 changes: 7 additions & 0 deletions ...e/Classes/ViewRelated/Orders/Simple Payments/Summary/SimplePaymentsSummaryViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {
///
let providedAmount: String

let amountName: String?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came up from the test_update_simple_payments_order_sends_default_name_when_none_provided test, as we set amountName: nil but we assert against name: "Simple Payments":

By this property I would assume that a Simple Payments order either has a given string as name, or has no name, but without this property they still get the "Simple Payment" name by default, given in the OrderFactory. Should we rename this to some sort of "overrideFeeLineName", or similar? Perhaps move the default value somewhere else outside the factory? Perhaps make it non-nil? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so... I don't feel super strongly, but I think it's best just to leave this as is.

At risk of bikeshedding a minor point in a part of the code which should be removed (because we don't do Simple Payments any more): the default is implemented in business logic. The UI/view model doesn't need to care about what happens in business logic in this case, just that the user has the option of providing an amount name, or not.

It makes sense to me to have a default in business logic – an unnamed fee would be confusing when reviewed.

Naming it overrideFeeLineName makes the view model harder to read/understand, and prompting questions that the callers probably don't need to care about or intuit – what's it overriding? Is it the OverrideFee's line name, or is it an override for the Fee's line name? Why's there a fee anyway, this is a payment? It feels like we're leaking implementation details with this sort of approach.

Making it non-nillable is simple, but would mean that we lose the ability to rely on a default value, and have to provide it separately for anything that uses that order action.

Putting the default value elsewhere in code is probably the most compelling alternative, and I wouldn't be against doing that. It does mean that every caller of the relevant order action needs to look up or have its own default though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, and I have no strong opinion about it neither, just cached my attention because of the test, and changing it feels too much hassle for such a little detail.

PD: Learned a new word today with bikeshedding, gonna use that one 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://en.wikipedia.org/wiki/Law_of_triviality


private let unformattedProvidedAmount: String

/// Store tax lines.
Expand Down Expand Up @@ -157,6 +159,7 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {
lazy private(set) var noteViewModel = { SimplePaymentsNoteViewModel(analytics: analytics) }()

init(providedAmount: String,
amountName: String? = nil,
totalWithTaxes: String,
taxLines: [TaxLine],
noteContent: String? = nil,
Expand All @@ -183,6 +186,7 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {
self.analytics = analytics
self.flow = analyticsFlow
self.providedAmount = currencyFormatter.formatAmount(providedAmount) ?? providedAmount
self.amountName = amountName
self.unformattedProvidedAmount = providedAmount
self.totalWithTaxes = currencyFormatter.formatAmount(totalWithTaxes) ?? totalWithTaxes
self.unformattedTotalWithTaxes = totalWithTaxes
Expand All @@ -205,6 +209,7 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {

convenience init(order: Order,
providedAmount: String,
amountName: String? = nil,
presentNoticeSubject: PassthroughSubject<PaymentMethodsNotice, Never> = PassthroughSubject(),
currencyFormatter: CurrencyFormatter = CurrencyFormatter(currencySettings: ServiceLocator.currencySettings),
stores: StoresManager = ServiceLocator.stores,
Expand All @@ -217,6 +222,7 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {
})

self.init(providedAmount: providedAmount,
amountName: amountName,
totalWithTaxes: order.total,
taxLines: taxLines,
siteID: order.siteID,
Expand Down Expand Up @@ -254,6 +260,7 @@ final class SimplePaymentsSummaryViewModel: ObservableObject {
feeID: feeID,
status: .pending, // Force .pending status to properly generate the payment link in the next screen.
amount: removeCurrencySymbolFromAmount(providedAmount),
amountName: amountName,
taxable: enableTaxes,
orderNote: noteContent,
email: email.isEmpty ? nil : email) { [weak self] result in
Expand Down
43 changes: 33 additions & 10 deletions ...ommerceTests/ViewRelated/Orders/Simple Payments/SimplePaymentsSummaryViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let _: String = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _):
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _, _):
promise(amount)
default:
XCTFail("Unexpected action: \(action)")
Expand Down Expand Up @@ -109,7 +109,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let _: String = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _):
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _, _):
promise(amount)
default:
XCTFail("Unexpected action: \(action)")
Expand All @@ -132,7 +132,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let updatedAmount: String = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _):
case let .updateSimplePaymentsOrder(_, _, _, _, amount, _, _, _, _, _):
promise(amount)
default:
XCTFail("Unexpected action: \(action)")
Expand All @@ -147,6 +147,29 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
assertEqual("1.00", updatedAmount)
}

func test_when_updateOrder_then_amount_name_is_added_to_order_when_provided() {
// Given
let mockStores = MockStoresManager(sessionManager: .testingInstance)
let viewModel = SimplePaymentsSummaryViewModel(providedAmount: "1", amountName: "One dollar fee", totalWithTaxes: "1", taxLines: [], stores: mockStores)

// When
let updatedAmountName = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, amountName, _, _, _, _):
promise(amountName)
default:
XCTFail("Unexpected action: \(action)")
}
}
viewModel.updateOrder()
}

// Then
assertEqual("One dollar fee", viewModel.amountName)
assertEqual("One dollar fee", updatedAmountName)
}

func test_provided_amount_with_taxes_gets_properly_formatted() {
// Given
let currencyFormatter = CurrencyFormatter(currencySettings: CurrencySettings()) // Default is US.
Expand Down Expand Up @@ -249,7 +272,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
stores: mockStores)
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, onCompletion):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, _, onCompletion):
onCompletion(.success(Order.fake()))
default:
XCTFail("Unexpected action: \(action)")
Expand Down Expand Up @@ -284,7 +307,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
stores: mockStores)
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, onCompletion):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, _, onCompletion):
onCompletion(.failure(NSError(domain: "Error", code: 0)))
default:
XCTFail("Received unsupported action: \(action)")
Expand Down Expand Up @@ -347,7 +370,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let updateStatus: OrderStatusEnum = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, status, _, _, _, _, _):
case let .updateSimplePaymentsOrder(_, _, _, status, _, _, _, _, _, _):
promise(status)
default:
XCTFail("Unexpected action: \(action)")
Expand All @@ -370,7 +393,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
stores: mockStores)
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, onCompletion):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, _, onCompletion):
onCompletion(.success(Order.fake()))
default:
XCTFail("Unexpected action: \(action)")
Expand Down Expand Up @@ -398,7 +421,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let trimmedEmail: String? = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, email, _):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, email, _):
promise(email)
default:
XCTFail("Unexpected action: \(action)")
Expand All @@ -425,7 +448,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let emailSent: String? = waitFor { promise in
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, email, _):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, email, _):
promise(email)
default:
XCTFail("Unexpected action: \(action)")
Expand Down Expand Up @@ -513,7 +536,7 @@ final class SimplePaymentsSummaryViewModelTests: XCTestCase {
let mockStores = MockStoresManager(sessionManager: .testingInstance)
mockStores.whenReceivingAction(ofType: OrderAction.self) { action in
switch action {
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, onCompletion):
case let .updateSimplePaymentsOrder(_, _, _, _, _, _, _, _, _, onCompletion):
onCompletion(.failure(NSError(domain: "", code: 0, userInfo: nil)))
default:
XCTFail("Unexpected action: \(action)")
Expand Down
1 change: 1 addition & 0 deletions Yosemite/Yosemite/Actions/OrderAction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public enum OrderAction: Action {
feeID: Int64,
status: OrderStatusEnum,
amount: String,
amountName: String?,
taxable: Bool,
orderNote: String?,
email: String?,
Expand Down
4 changes: 2 additions & 2 deletions Yosemite/Yosemite/Stores/Order/OrderFactory.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ public enum OrderFactory {

/// Creates a fee line suitable to be used within a simple payments order.
///
static func simplePaymentFee(feeID: Int64, amount: String, taxable: Bool) -> OrderFeeLine {
static func simplePaymentFee(feeID: Int64, amount: String, name: String? = nil, taxable: Bool) -> OrderFeeLine {
.init(feeID: feeID,
name: "Simple Payments",
name: name ?? "Simple Payments",
taxClass: "",
taxStatus: taxable ? .taxable : .none,
total: amount,
Expand Down
6 changes: 4 additions & 2 deletions Yosemite/Yosemite/Stores/OrderStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,13 @@ public class OrderStore: Store {
case let .createOrder(siteID, order, giftCard, onCompletion):
createOrder(siteID: siteID, order: order, giftCard: giftCard, onCompletion: onCompletion)

case let .updateSimplePaymentsOrder(siteID, orderID, feeID, status, amount, taxable, orderNote, email, onCompletion):
case let .updateSimplePaymentsOrder(siteID, orderID, feeID, status, amount, amountName, taxable, orderNote, email, onCompletion):
updateSimplePaymentsOrder(siteID: siteID,
orderID: orderID,
feeID: feeID,
status: status,
amount: amount,
amountName: amountName,
taxable: taxable,
orderNote: orderNote,
email: email,
Expand Down Expand Up @@ -400,6 +401,7 @@ private extension OrderStore {
feeID: Int64,
status: OrderStatusEnum,
amount: String,
amountName: String?,
taxable: Bool,
orderNote: String?,
email: String?,
Expand All @@ -409,7 +411,7 @@ private extension OrderStore {
let originalOrder = OrderFactory.simplePaymentsOrder(status: status, amount: amount, taxable: taxable)

// Create updated fields
let newFee = OrderFactory.simplePaymentFee(feeID: feeID, amount: amount, taxable: taxable)
let newFee = OrderFactory.simplePaymentFee(feeID: feeID, amount: amount, name: amountName, taxable: taxable)
let newBillingAddress = Address(firstName: "",
lastName: "",
company: nil,
Expand Down
40 changes: 39 additions & 1 deletion Yosemite/YosemiteTests/Stores/OrderStoreTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,7 @@ final class OrderStoreTests: XCTestCase {
// Given
let feeID: Int64 = 1234
let amount = "100.00"
let amountName = "A simple amount"
let taxable = true
let note = "This is a note"
let email = "[email protected]"
Expand All @@ -1178,6 +1179,7 @@ final class OrderStoreTests: XCTestCase {
feeID: feeID,
status: .pending,
amount: amount,
amountName: amountName,
taxable: taxable,
orderNote: note,
email: email) { _ in }
Expand All @@ -1188,7 +1190,7 @@ final class OrderStoreTests: XCTestCase {
let receivedFees = try XCTUnwrap(request.parameters["fee_lines"] as? [[String: AnyHashable]]).first
let expectedFees: [String: AnyHashable] = [
"id": 1234,
"name": "Simple Payments",
"name": "A simple amount",
"tax_status": "taxable",
"tax_class": "",
"total": "100.00"
Expand All @@ -1212,6 +1214,42 @@ final class OrderStoreTests: XCTestCase {
assertEqual(receivedNote, note)
}

func test_update_simple_payments_order_sends_default_name_when_none_provided() throws {
// Given
let feeID: Int64 = 1234
let amount = "100.00"
let taxable = true
let note = "This is a note"
let email = "[email protected]"

let store = OrderStore(dispatcher: dispatcher, storageManager: storageManager, network: network)
network.simulateResponse(requestUrlSuffix: "orders/963", filename: "order")

// When
let action = OrderAction.updateSimplePaymentsOrder(siteID: sampleSiteID,
orderID: sampleOrderID,
feeID: feeID,
status: .pending,
amount: amount,
amountName: nil,
taxable: taxable,
orderNote: note,
email: email) { _ in }
store.onAction(action)

// Then
let request = try XCTUnwrap(network.requestsForResponseData.last as? JetpackRequest)
let receivedFees = try XCTUnwrap(request.parameters["fee_lines"] as? [[String: AnyHashable]]).first
let expectedFees: [String: AnyHashable] = [
"id": 1234,
"name": "Simple Payments",
"tax_status": "taxable",
"tax_class": "",
"total": "100.00"
]
assertEqual(expectedFees, receivedFees)
}

func test_create_order_sends_expected_fields() throws {
// Given
let store = OrderStore(dispatcher: dispatcher, storageManager: storageManager, network: network)
Expand Down