-
Notifications
You must be signed in to change notification settings - Fork 131
[WOOMOB-302][Mobile Payments] Update Stripe's SDK to 4.3.1 #13970
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
00ee94e
ed46d95
0cb2988
9ccb8d3
f635d44
1f0b7c5
08e9dcc
d85302a
abe45c8
4701f39
677907e
d6c714b
94c0390
d1039c4
52fe95f
d43087b
5f04514
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package com.woocommerce.android.cardreader.internal.connection | ||
|
||
import com.stripe.stripeterminal.external.callable.Cancelable | ||
import com.stripe.stripeterminal.external.callable.TapToPayReaderListener | ||
import com.stripe.stripeterminal.external.models.DisconnectReason | ||
import com.stripe.stripeterminal.external.models.Reader | ||
import com.woocommerce.android.cardreader.LogWrapper | ||
import com.woocommerce.android.cardreader.internal.LOG_TAG | ||
|
||
class TapToPayReaderListenerImpl( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we can just log status changes. Probably we need to propagate this to the user and offer a correct explanation with further steps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kidinov Thanks for the comment! Before, we didn't even have this listener, so we didn't propagate the events to the user. As this PR issue is just to update the SDK without modifying behavior, what do you think if we leave it as it is on this PR and create an issue enhancement for propagating these events to the users and letting them to handle them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @toupper 👋
Oh, I see. We didn't have
Yep, updating to a major version often brings changes in behavior, beyond just the API modifications, indicating a need for adaptation on our side too. However, I’m uncertain in this case since it's unclear how the app will behave during the reconnection process and the duration it may require. Previously, we would display an error message when it was "not connected" and that was the end of it. Now, it will attempt to reconnect, but we will ignore the process and not present any information to the user. It feels like if we don't want to actually handle the reconnection, we may need to disable it to keep the same behavior like now, and have a separate issue to handle this as you proposed
|
||
private val logWrapper: LogWrapper | ||
) : TapToPayReaderListener { | ||
override fun onDisconnect(reason: DisconnectReason) { | ||
logWrapper.d(LOG_TAG, "onDisconnect") | ||
} | ||
|
||
override fun onReaderReconnectFailed(reader: Reader) { | ||
logWrapper.d(LOG_TAG, "onReaderReconnectFailed") | ||
} | ||
|
||
override fun onReaderReconnectStarted( | ||
reader: Reader, | ||
cancelReconnect: Cancelable, | ||
reason: DisconnectReason | ||
) { | ||
logWrapper.d(LOG_TAG, "onReaderReconnectStarted") | ||
} | ||
|
||
override fun onReaderReconnectSucceeded(reader: Reader) { | ||
logWrapper.d(LOG_TAG, "onReaderReconnectSucceeded") | ||
} | ||
} |
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.
I believe we shouldn't just log it. We may need to handle it properly and share it with the user