-
Notifications
You must be signed in to change notification settings - Fork 113
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
Send ipp_channel
metadata in payment intent from store management/POS use cases
#14479
Conversation
…ses to differentiate POS and store management source.
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Thanks, works as described!
POS | Store Management |
---|---|
One note is that ipp_channel
appears twice within payment details:
- charges -> data -> metadata -> ipp_channel
- metadata -> ipp_channel
On a technical note, I was thinking if passing a channel all around could be avoided. If POS and Store Management would rely on different instances of dependencies, then we could inject the channel through an initializer. However, that's not an option now so I don't have good suggestions. Given this code rarely has a new entry point maybe thats a non-issue.
@@ -106,7 +111,8 @@ public extension PaymentIntent { | |||
siteURL: String? = nil, | |||
orderID: Int64? = nil, | |||
orderKey: String? = nil, | |||
paymentType: PaymentTypes? = nil | |||
paymentType: PaymentTypes? = nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit;
There's broken alignment for the rest of the parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the indentation of these misaligned parameters in b601a7c
* trunk: (132 commits) Update tests Make sure self is weakly captured Add a clarifiyng comment regarding the nullability of site customPackageViewModel as StateObject Simplify expression Add a release note Make sure to wait for site to be available before handling the magic link Rename to WooShippingAccountSettings Make bindings private Freeze strings for localization Update metadata strings Update release notes with @joshheald's thoughtful rewrite. Do not dismiss success modals automatically when email receipt action is selected Restructure presentBackendReceiptAlert to align implementation for success and failed alerts Use CardPresentModalBuiltIn modals for BuiltInCardReaderPaymentAlertsProvider Restructure the code to set paymentSuccessEmailSent when presentSendReceiptAfterPayment succeeds Add missing weak self when presentSendReceiptAfterPayment is called Update closure naming in CardPresentModalErrorTests Clarify closure naming for CardPresentModalNonRetryableError Clarify closure naming for CardPresentModalErrorEmailSent and Clarify closure naming for CardPresentModalErrorWithoutEmail ... # Conflicts: # WooCommerce/Classes/ViewRelated/Orders/Collect Payments/CollectOrderPaymentUseCase.swift # WooCommerce/WooCommerceTests/ViewModels/CardPresentPayments/CollectOrderPaymentUseCaseTests.swift
Yea I agree that it's not pretty seeing a bunch of places where the channel is passed around. Initially, I tried DI'ing the channel to the initializers but decided to move it to the |
Merging this PR as it's approved to avoid merge conflicts, with the possibility of adding a feature flag or condition on the WooPayments version per decision pdfdoF-5UW-p2#comment-6983. |
Closes: #14478
Why
As we are launching POS to external merchants soon, we want to identify if a transaction is from POS or IPP in the mobile apps for data analysis purposes.
How
As shared in pdfdoF-5QB-p2 > Apps, we want to add a new metadata
ipp_channel: mobile_store_management | mobile_pos
to the payment intent from IPP and POS respectively.I thought about giving the channel parameter a default value of
.storeManagement
, but I'm afraid that this parameter will be forgotten and be the wrong value if we add more use cases in the future. I'm open to adding a default value if you think it will simplify the code though.Steps to reproduce
ipp_channel: mobile_store_management
in the Stripe account (* check instructions below)ipp_channel: mobile_pos
in the Stripe account (* check instructions below)*
How to check the transaction payment intentPrerequisite: you have access to the WooPayments Stripe account (ask in
#payment-operations
if not)MC WCPay
to see the payments info for the siteStripe account details
> Account ID, tap on the link to open the Stripe account (make sure you're logged in to WooPayments in Stripe, with test mode enabled)ipp_channel
and verify the value is as expectedTesting information
I also tested with local server-side changes.
Screenshots
RELEASE-NOTES.txt
if 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: