Bump Terminal SDK to 4.0.0 #847

tim-lin-bbpos · 2024-11-11T08:57:08Z

Summary

Bump Terminal SDK to 4.0.0

Motivation

Change LocalMobile keyword to TapToPay to align Android/iOS sdk.
Remove REPORT_UNEXPECTED_READER_DISCONNECT as we can now use DISCONNECT directly.
Modify Listener structure on Android side.
Increase iOS minimal sdk to 14 to align terminal iOS sdk.
Consolidate all connect* methods to a single connectReader method in js layer.

Testing

I tested this manually
I added automated tests

Documentation

Select one:

Require doc change.
I have added relevant documentation for my changes.
This PR does not result in any developer-facing changes.

android/src/main/java/com/stripeterminalreactnative/Mappers.kt

sjl-stripe · 2024-11-11T21:42:56Z

android/src/main/java/com/stripeterminalreactnative/Mappers.kt

@@ -659,7 +602,7 @@ private fun mapFromWechatPayDetails(wechatPayDetails: WechatPayDetails?): Readab
 private fun mapFromOfflineDetails(offlineDetails: OfflineDetails?): ReadableMap? =
    offlineDetails?.let {
        nativeMapOf {
-            putString("storedAt", offlineDetails.storedAt.toString())
+            putString("storedAt", offlineDetails.storedAtMs.toString())


Does it make sense to update the string name?

Suggested change

putString("storedAt", offlineDetails.storedAtMs.toString())

putString("storedAtMs", offlineDetails.storedAtMs.toString())

Because it's also used on ios side and name only changed in Android now.

Looks like it's still storedAt for iOS. @bric-stripe this could be something to consider for SDKv5?

Android Jira ticket for context.

it makes sense for the RN layer to expose a consistent value though, right? I think RN exposing storedAtMs and then in the swift layer below it can just convert from the s to the ms value so its consistent regardless of platform...and yeah we should fix it up on the native side. we could add the storedAtMs next to storedAt in a minor update and then drop storedAt in v5

Just found RN already had the transform before 4.0, so we can align the name as bric suggested.

https://github.com/stripe/stripe-terminal-react-native/blob/main/ios/Mappers.swift#L530
https://github.com/stripe/stripe-terminal-react-native/blob/main/ios/Mappers.swift#L457-L463

sjl-stripe · 2024-11-11T21:47:15Z

android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt

+                val autoReconnectOnUnexpectedDisconnect = if (discoveryMethod == DiscoveryMethod.BLUETOOTH_SCAN || discoveryMethod == DiscoveryMethod.USB || discoveryMethod == DiscoveryMethod.TAP_TO_PAY) {
                    getBoolean(params, "autoReconnectOnUnexpectedDisconnect")


If no autoReconnectOnUnexpectedDisconnect is provided, it should default to true.

Just want to make sure are your referring to the scope of BLUETOOTH_SCAN, USB, TAP_TO_PAY only or no matter which discoveryMethod.

It's only for BLUETOOTH_SCAN, USB and TAP_TO_PAY. For other connection types, it shouldn't matter if a autoReconnectOnUnexpectedDisconnect value is provided.

sjl-stripe · 2024-11-11T21:49:53Z

android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt

@@ -250,7 +194,7 @@ class StripeTerminalReactNativeModule(reactContext: ReactApplicationContext) :
        params: ReadableMap,
        promise: Promise,
        discoveryMethod: DiscoveryMethod,
-        listener: ReaderListenable? = null
+        listener: ReaderDisconnectListener? = null


I wonder why we pass in ReaderDisconnectListener here. I think the listener should be one of the following:

RNBluetoothReaderListener

RNHandoffReaderListener

RNInternetReaderListener

RNTapToPayReaderListener

ya, but RNInternetReaderListener, RNTapToPayReaderListener are not ReaderListenable, if we want to use the same pattern here we have to find a common parent, or we need to expose new parameter (probably two in this case).
Another idea is to declare listener and discoveryMethod relationship in a sealed class and use that binding for more type safe usage but it require more change the scope of bump sdk version.
What do you think on these two or any other suggestions?
Also add a todo

if we want to use the same pattern here we have to find a common parent, or we need to expose new parameter (probably two in this case).

I think it's fine to expose new parameters in private methods.

Suggested change

listener: ReaderDisconnectListener? = null

connectionConfiguration: RNConnectionConfiguration? = null

Maybe we can do something like this, so it mirrors the Android and iOS SDKs?

refine the listener structure now, please take a look again to see if there's any other suggestions.

sjl-stripe · 2024-11-11T21:52:31Z

android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt

+                AllowRedisplay.ALWAYS,
+//                customerConsentCollected,//TODO: seems removed


We should give an option for the merchant to set the AllowRedisplay parameter. If the merchant doesn't provide one, we should default to AllowRedisplay.UNSPECIFIED

The removal of the customerConsentCollected parameter is expected.

cc @henryx-stripe

So we'll have new parameter for user on AllowRedisplay right?
But what is the different meaning to user/rn sdk? is there other required change corresponding to this attribute?

You can find more details about this specific change in our Stripe docs here. We also have Android docs here and iOS docs here.

yup @tim-lin-bbpos

for collectSetupIntentPaymentMethod the user is required to pass in AllowRedisplay now, they are no longer expected to pass customerConsentCollected

please also include an optional AllowRedisplay field for CollectConfiguration for CollectPaymentMethod (line ~500) and pass that into the SDK as well

Added allowRedisplay into these two feature

sjl-stripe · 2024-11-11T22:11:13Z

android/src/main/java/com/stripeterminalreactnative/ktx/TerminalExtensions.kt

    reader: Reader,
    locationId: String,
    autoReconnectOnUnexpectedDisconnect: Boolean = false,
-    listener: ReaderListenable? = null,
-    reconnectionListener: ReaderReconnectionListener
+    disconnectListener: ReaderDisconnectListener? = null,
+    reconnectionListener: ReaderReconnectionListener,


Is this connectReader(...) method publicly exposed in the RN SDK? If not, can we remove it?

RN user should only use RN sdk via js interface not any native Kotlin/Android code.

sjl-stripe · 2024-11-11T22:12:31Z

android/src/main/java/com/stripeterminalreactnative/ktx/TerminalExtensions.kt

    reader: Reader,
    locationId: String,
    autoReconnectOnUnexpectedDisconnect: Boolean = false,
-    listener: ReaderListenable? = null,
-    reconnectionListener: ReaderReconnectionListener
+    disconnectListener: ReaderDisconnectListener? = null,
+    reconnectionListener: ReaderReconnectionListener,


Wondering if we can make ReaderReconnectionListener optional to pass in, since it's not required by internet and handoff readers.

We can make the declaration optional but it won't help much in this case since there's only one usage.
Maybe best way is to initiate this listener when need if it's not common for all the cases but it also imply a bigger refactoring which I try to avoid in this stage.
Mark a todo here.

sjl-stripe · 2024-11-11T22:14:42Z

android/src/main/java/com/stripeterminalreactnative/listener/RNBluetoothReaderListener.kt


 class RNBluetoothReaderListener(
    private val context: ReactApplicationContext,
    private val onStartInstallingUpdate: (cancelable: Cancelable?) -> Unit
-) : ReaderListener {
+) : MobileReaderListener, ReaderDisconnectListener by readerDisconnectDelete(context) {


Out of curiosity, why are we adding a readerDisconnectDelete delegate here?

To avoid adding same code again and again, as these interface is not required so people can remove if not carefully, for example the RNHandoffReaderListener is not implement the disconnect part before.

sjl-stripe · 2024-11-11T22:16:37Z

android/src/main/java/com/stripeterminalreactnative/listener/RNReaderReconnectionListener.kt

+        reason: DisconnectReason
+    ) {
        onReaderReconnectStarted(cancelReconnect)
        context.sendEvent(START_READER_RECONNECT.listenerName) {
            putMap("reader", mapFromReader(reader))


can we add the disconnect reason to the event?

src/types/Reader.ts

sjl-stripe · 2024-11-11T22:19:19Z

src/types/index.ts

@@ -47,7 +47,7 @@ export type ConnectUsbReaderParams = {
  autoReconnectOnUnexpectedDisconnect?: boolean;
 };

-export type ConnectLocalMobileParams = {
+export type ConnectTapToPayParams = {
  reader: Reader.Type;
  locationId?: string;
  onBehalfOf?: string;


We should update ConnectTapToPayParams so that autoReconnectOnUnexpectedDisconnect defaults to true

in ts layer it's optional but in native it will default to true for tap to pay now.

android/src/main/java/com/stripeterminalreactnative/ktx/TerminalExtensions.kt

sjl-stripe · 2024-11-11T22:32:23Z

android/src/main/java/com/stripeterminalreactnative/ktx/TerminalExtensions.kt

 ): Reader = when (discoveryMethod) {
    DiscoveryMethod.BLUETOOTH_SCAN -> {
        val connConfig = BluetoothConnectionConfiguration(
            locationId,
            autoReconnectOnUnexpectedDisconnect,
-            reconnectionListener
+            (disconnectListener as? MobileReaderListener).bindReconnectionListener(reconnectionListener)


it looks like we don't give an option to pass in a ReaderListenable here

Same thing as above, right now disconnectListener is acted like the original listener but I'm also struggling if there's other better alternative.

sjl-stripe · 2024-11-12T00:29:21Z

ios/Errors.swift

+        case .tapToPayReaderFailedToPrepare:
+            return "TapToPayReaderFailedToPrepare"
+        case .tapToPayReaderDeviceBanned:
+            return "TapToPayReaderDeviceBanned"
+        case .tapToPayReaderTOSNotYetAccepted:
+            return "TapToPayReaderTOSNotYetAccepted"
+        case .tapToPayReaderTOSAcceptanceFailed:
+            return "TapToPayReaderTOSAcceptanceFailed"


We've added a new error code to the iOS and Android SDK. (for iOS, it's SCPErrorGenericReaderError, and for Android it's GENERIC_READER_ERROR). Can we add a new external-facing RN error code for this?

In Android we use toString so it doesn't matter if there's new element or not, but on iOS side besides .genericReaderError we still have all of these:

case .cancelFailedUnavailable: <#code#> case .invalidConnectionConfiguration: <#code#> case .invalidRequiredParameterOnBehalfOf: <#code#> case .requestDynamicCurrencyConversionRequiresUpdatePaymentIntent: <#code#> case .dynamicCurrencyConversionNotAvailable: <#code#> case .surchargeNoticeRequiresUpdatePaymentIntent: <#code#> case .surchargeUnavailableWithDynamicCurrencyConversion: <#code#> case .canceledDueToIntegrationError: <#code#> case .collectInputsInvalidParameter: <#code#> case .collectInputsUnsupported: <#code#> case .readerConnectionOfflineNeedsUpdate: <#code#> case .readerConnectionOfflinePairingUnseenDisabled: <#code#> case .collectInputsTimedOut: <#code#> case .usbDiscoveryTimedOut: <#code#> case .tapToPayReaderAccountDeactivated: <#code#> case .readerMissingEncryptionKeys: <#code#> case .usbDisconnected: <#code#> case .encryptionKeyFailure: <#code#> case .encryptionKeyStillInitializing: <#code#> case .collectInputsApplicationError: <#code#> case .genericReaderError: <#code#> case .commandInvalidAllowRedisplay: <#code#> case .onlinePinNotSupportedOffline: <#code#> case .offlineTestCardInLivemode:

Do we need to add them all or only the one you mentioned?

I think we want to add all these iOS errors (cc @bric-stripe @mindy-stripe to confirm)

Seems we still need some time to merge this, create in separate pr here: #851
Will add new ones here when able to rebase.

Also would like to know if we're consider some ways to automatically convert enum to string without hardcode like Android side?
https://stackoverflow.com/questions/24701075/swift-convert-enum-value-to-string

Also would like to know if we're consider some ways to automatically convert enum to string without hardcode

unfortunately the enum backing ErrorCode.Code in the native SDK is an ObjC enum and so can't be declared as being backed by a String.

in the native SDK we do have a code to string helper already and we could expose that, but it wouldn't be available until 4.2 at the earliest.

added into this pr now.

nazli-stripe · 2024-11-12T17:20:46Z

@tim-lin-bbpos To match the new native commands can we consolidate all connect* methods to a single connectReader method? @sjl-stripe are there other method / parameter name changes we are missing in the React Native layer?

sjl-stripe · 2024-11-13T03:19:07Z

@tim-lin-bbpos you might've already made this change - confirmPaymentIntent, confirmSetupIntent and confirmRefund now return a Cancelable .

tim-lin-bbpos · 2024-12-03T06:07:32Z

@tim-lin-bbpos To match the new native commands can we consolidate all connect* methods to a single connectReader method? @sjl-stripe are there other method / parameter name changes we are missing in the React Native layer?

Done.

@tim-lin-bbpos you might've already made this change - confirmPaymentIntent, confirmSetupIntent and confirmRefund now return a Cancelable .

No, but should we do it in another pr if it's not affect the main feature since it's a bit massive now?
And would you mind to provide more context about the cancelable? like when should we deal with this cancelable?

nazli-stripe · 2024-12-13T21:59:01Z

No, but should we do it in another pr if it's not affect the main feature since it's a bit massive now?
And would you mind to provide more context about the cancelable? like when should we deal with this cancelable?

In v4 we now support cancelling the confirm operations, and should surface a new method like cancelCollectPaymentIntent in the RN SDK. The native SDK will handle whether the cancellation succeeds or not

I'll create a new ticket and we can address this in a new PR. Once we address the merge conflicts I think this PR is good to merge

Apply change

tim-lin-bbpos · 2024-12-16T06:29:42Z

No, but should we do it in another pr if it's not affect the main feature since it's a bit massive now?
And would you mind to provide more context about the cancelable? like when should we deal with this cancelable?

In v4 we now support cancelling the confirm operations, and should surface a new method like cancelCollectPaymentIntent in the RN SDK. The native SDK will handle whether the cancellation succeeds or not

I'll create a new ticket and we can address this in a new PR. Once we address the merge conflicts I think this PR is good to merge

Rebased with main and fix ci too, thanks for follow up.

tim-lin-bbpos self-assigned this Nov 11, 2024

tim-lin-bbpos requested review from billfinn-stripe, bric-stripe, nazli-stripe, ugochukwu-stripe, ebarrenechea-stripe and mshafrir-stripe as code owners November 11, 2024 08:57

sjl-stripe reviewed Nov 11, 2024

View reviewed changes

sjl-stripe reviewed Nov 12, 2024

View reviewed changes

nazli-stripe requested a review from xiaoshen-stripe November 12, 2024 03:15

tim-lin-bbpos force-pushed the bbpos/sdk4.0.0 branch 2 times, most recently from 1ad049c to 5a2c56f Compare November 14, 2024 07:27

tim-lin-bbpos force-pushed the bbpos/sdk4.0.0 branch from 5a2c56f to 35d99e2 Compare December 3, 2024 06:00

tim-lin-bbpos added 10 commits December 16, 2024 13:23

Apply android sdk 4.0.0 change

0129d24

Apply ios sdk 4.0.0 change

47e1136

Apply change

Refine and fix test

be312b6

Fix listener issue

4a030e2

Fix comment

2aa9453

Expose DisconnectReason

abba49e

Align naming from storedAt to storedAtMs

0469d79

Update listener structure

df7954a

refine bridge interface

96f8dc9

Add parameter

06c757a

tim-lin-bbpos added 2 commits December 16, 2024 13:24

Update ui for allowRedisplay

9f0e9f1

Add default value

c3a085c

tim-lin-bbpos force-pushed the bbpos/sdk4.0.0 branch from 469ec41 to cff636f Compare December 16, 2024 05:30

Add missing error string

d6c44f2

tim-lin-bbpos force-pushed the bbpos/sdk4.0.0 branch from cff636f to d6c44f2 Compare December 16, 2024 05:44

Fix ios ci

90ef859

tim-lin-bbpos force-pushed the bbpos/sdk4.0.0 branch from fc04c0e to 90ef859 Compare December 16, 2024 06:02

tim-lin-bbpos changed the title ~~[WIP] Bump Terminal SDK to 4.0.0~~ Bump Terminal SDK to 4.0.0 Dec 16, 2024

nazli-stripe approved these changes Dec 16, 2024

View reviewed changes

nazli-stripe approved these changes Dec 17, 2024

View reviewed changes

nazli-stripe merged commit 534ec6a into main Dec 17, 2024
3 checks passed

nazli-stripe deleted the bbpos/sdk4.0.0 branch December 17, 2024 00:10

	putString("storedAt", offlineDetails.storedAtMs.toString())
	putString("storedAtMs", offlineDetails.storedAtMs.toString())

		val autoReconnectOnUnexpectedDisconnect = if (discoveryMethod == DiscoveryMethod.BLUETOOTH_SCAN \|\| discoveryMethod == DiscoveryMethod.USB \|\| discoveryMethod == DiscoveryMethod.TAP_TO_PAY) {
		getBoolean(params, "autoReconnectOnUnexpectedDisconnect")

	listener: ReaderDisconnectListener? = null
	connectionConfiguration: RNConnectionConfiguration? = null

		AllowRedisplay.ALWAYS,
		// customerConsentCollected,//TODO: seems removed

Bump Terminal SDK to 4.0.0 #847

Bump Terminal SDK to 4.0.0 #847

Conversation

tim-lin-bbpos commented Nov 11, 2024 • edited Loading

Summary

Motivation

Testing

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjl-stripe Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazli-stripe commented Nov 12, 2024

sjl-stripe commented Nov 13, 2024

tim-lin-bbpos commented Dec 3, 2024 • edited Loading

nazli-stripe commented Dec 13, 2024

tim-lin-bbpos commented Dec 16, 2024

tim-lin-bbpos commented Nov 11, 2024 •

edited

Loading

sjl-stripe Nov 11, 2024 •

edited

Loading

tim-lin-bbpos commented Dec 3, 2024 •

edited

Loading