-
Notifications
You must be signed in to change notification settings - Fork 121
Stripe Terminal SDK update 4.2.0 #15377
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
Conversation
…connectLocalMobileReader
Updating based on SDK update: SCPPaymentIntentParameters and SCPSetupIntentParameters now keep payment method types as values of the SCPPaymentMethodType enum rather than strings.
|
|
…connect In Stripe Terminal SDK, the method for handling reader disconnects has changed. Removed terminal:didReportUnexpectedReaderDisconnect: from the SCPTerminalDelegate. Use reader:didDisconnect: to be informed of reader disconnects.
|
👋 Apology for the delayed response, I started the review but haven't finished it and tested it yet. I will make sure to prioritize this Wednesday and finish the review by EOD Wednesday. Additional review(s) would also be helpful. |
jaclync
left a comment
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 for taking on this non-trivial SDK update, and the detailed PR description and testing scenarios! LGTM ![]()
The tasks I tested (I have an Interac test card):
IPP
| Tasks | US | CA |
|---|---|---|
| Connection from Order payment flow | ✅ | |
| Connection from Payments menu | ✅ | |
| Connection of a reader with a mandatory update | ✅ (simulator) | |
| Connection of M2 reader | ✅ | N/A |
| Connection of Chipper reader | ✅ | N/A |
| Connection of WisePad 3 reader | N/A | ✅ |
| Connection from switched off state | ✅ | |
| Take a payment | ✅ | |
| Refund a payment | ✅ | |
| Take payment after the in-line connection process | ✅ | |
| Take payment with a previously-connected reader | ✅ | |
| Take TPP payment | ✅ | N/A |
| Refund TTP payment | ✅ | N/A |
| Take card-inserted payment | ✅ | |
| Take Interac payment | N/A | ✅ |
| Refund Interac payment | N/A | ✅ |
| Reader update: error message when attempted with low battery | ✅ (simulator) | |
| Reader update: successfully updates software and starts payment (in that flow) when completed | ✅ (simulator) | ✅ with WisePad 3, the update took about 5 minutes |
POS
| Tasks | US |
|---|---|
| Connection of M2 reader | ✅ |
| Connection of Chipper reader | ✅ |
| Connection of WisePad 3 reader | N/A |
| Connection from switched off state | ✅ |
| Take payment after the in-line connection process | ✅ |
| Take payment with a previously-connected reader | ✅ |
| Reader update: error message when attempted with low battery | ✅ (simulator) |
| Reader update: successfully updates software and starts payment (in that flow) when completed | ✅ (simulator) |
| Payment cancelations (going back, disconnecting. exiting POS) | ✅ |
| @@ -0,0 +1,4 @@ | |||
| public enum PaymentMethodType { | |||
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: maybe we can add a comment potentially with a link to the Stripe doc, as it seems to mirror a subset of StripeTerminal.PaymentMethodType?
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.
Good idea. I will.
I borrowed the approach of CardReaderType and CardReaderType+Stripe, it's a similar type used in a similar way.
| } | ||
|
|
||
| public func reader(_ reader: Reader, didDisconnect reason: DisconnectReason) { | ||
| connectedReadersSubject.send([]) |
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.
Just wondering, does TTP delegate have a similar method to didDisconnect in the bluetooth reader delegate now that TerminalDelegate is removed? Or TTP doesn't really have a "disconnecting" state?
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.
Yes.
Both TapToPayReaderDelegate and MobileReaderDelegate have didDisconnect. These delegates are both SCPReaderDelegate, which defines didDisconnect.
It means that when we set delegate to self, this method is shared by both TTP and Reader delegates.
BluetoothConnectionConfigurationBuilder(delegate: self, locationId: locationId)
TapToPayConnectionConfigurationBuilder(delegate: self, locationId: locationId)
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 for the explanation, makes sense now. I wasn't aware both delegates extend the same base delegate.
| public struct CardPresentPaymentsConfiguration: Equatable { | ||
| public let countryCode: CountryCode | ||
| public let paymentMethods: [WCPayPaymentMethodType] | ||
| public let paymentMethods: [PaymentMethodType] |
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.
❓ Is WCPayPaymentMethodType still used? Is it only meant to be used for WooPayments by its name?
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.
Yes. It's used for wider WooPayments case. WCPayPaymentMethodType is used by Networking layer to represent payment charges. It contains more payment types than are supported by IPP. It looks like it's aprt of WCPayCharge object that is used at least for the refunds.
PaymentMethodType is meant to be used by Hardware layer and represents payment types that are supported by IPP. We only used WCPayPaymentMethodType here to later change this value to String. However, Stripe SDK now accepts an enum, so it makes sense to define one within Hardware and ask in collectPayment method.
|
Nice one on this @staskus – I read through it all and did some general testing, the changes look great to me. |
|
Sorry I'm late to the party: I was planning to test this earlier as part of my Beta testing rotation but I had to set myself AFK unexpectedly. I ran multiple of the cases above for IPP and POS using M2, as well as TTP cases and saw no issues 👍 Thanks for handling this @staskus |
Description
Updating Stripe Terminal SDK update 4.2.0.
Breaking Changes
SDK has had breaking changes in the API, some of which are affecting the Woo app:
In my testing, replacing
didReportUnexpectedReaderDisconnectwithreader:didDisconnect:works. However, in practice, this method is only triggered 1 minute after the reader connection is lost (e.g. reader is turned off). As I understand, it happens because due to automatic reconnection attempts in the background.Most notably,
appleBuiltInandlocalMobileis now consistently calledtapToPay. I will make the same change in Woo codebase for consistency:Other possibly relevant changes
discoveringstate and enforcing a single discovery operationWe already have our own
discoveringstate when starting the card reader process.We also have logic that uses locks to make sure another discovery process is not starting or canceling. I'll test separately if this change allows us to remove the locks developed in this PR.
I don't know if this is worth exploring. Have we been missing some flows where cancelation should be supported?
I tested payment cancelation and it continues to work.
Steps to reproduce
Feel free to pick a few scenarios, especially the ones that were tested with a simulator (e.g. Interac payments). Also, one of the larger changes was done around reporting of unexpected disconnection, so it's good to test some these scenarios.
Testing information
I tested IPP scenarios on iPhone 14 Pro (17.7) and POS on iPad Air M2 (18.2). I also used a simulated card reader for scenarios that weren't easily reproducible.
IPP
POS
Screenshots
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: