[Mobile Payments] Show description for trial tap to pay line items #14022
Conversation
When a merchant tries out tap to pay on iPhone, we make an order which will remain in their history, even though it gets refunded. To help merchants identify the order, this adds a specific description rather than showing `Simple Payment` for these payments.
joshheald
added
type: task
|
@iamgabrielma again not urgent, just fit it in when you can |
|
|
| @@ -56,6 +56,8 @@ final class SimplePaymentsSummaryViewModel: ObservableObject { | |||
| /// | |||
| let providedAmount: String | |||
|
|
|||
| let amountName: String? | |||
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😂
There was a problem hiding this comment.
Description
When a merchant tries out tap to pay on iPhone, we make an order which will remain in their history, even though it gets refunded.
To help merchants identify the order, this PR adds a specific description rather than showing
Simple Paymentfor these payments.Steps to reproduce
Menu > Payments > Set up/Try out Tap to Pay on iPhoneTry out Tap to Pay on iPhoneTesting information
Note that Simple Payments is only used for Try out Tap to Pay at present, but I've preserved the previous behaviour of having
Simple Paymentas the description when no name is provided. This is covered by unit tests.I have tested the trial payments on an iPhone running iOS 17 and one running iOS 18
Screenshots
try.out.tap.to.pay.label.mp4
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: