braintree / braintree_android

Braintree SDK for Android
https://developer.paypal.com/braintree/docs/start/hello-client/android/v4
MIT License
405 stars 232 forks source link

Users getting UserCanceledException on tokenizePayPalAccount #557

Open mikkoville opened 2 years ago

mikkoville commented 2 years ago

General information

Issue description

Re opening issue: https://github.com/braintree/braintree_android/issues/409 since seems it has not been fixed in 4.10.1.

We do not have instructions how to reproduce or have not been able to reproduce yet locally.

We have multiple reports of our users complaining they cannot link PayPal due to it just reporting "Cancelled" when they try to link it.

This is what the users report:

  1. User presses our "Link PayPal" button where we start new activity based on your example that calls payPalClient.tokenizePayPalAccount(this, request)
  2. User sees the following screen: Screenshot 2022-05-27 at 16 29 57 3: User is redirected to our app due to message "Cancelled". We are listening for: override fun onPayPalFailure(error: Exception) in our activity and when the error is UserCanceledException we exit the activity and show "Cancelled" message to users what they are reporting.

Here is our PayPalActivity code:

class BraintreeActivity : AppCompatActivity(), PayPalListener {

    private lateinit var braintreeClient: BraintreeClient
    private lateinit var payPalClient: PayPalClient

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        val payPalToken = intent.extras?.getString(KEY_PAYPAL_TOKEN)
        if (payPalToken != null) {
            braintreeClient = BraintreeClient(this, payPalToken)
            payPalClient = PayPalClient(this, braintreeClient)
        } else {
            setErrorResult(exception)
        }
        payPalClient.setListener(this)
        tokenizePayPalAccountVault()
    }

    override fun onNewIntent(intent: Intent?) {
        super.onNewIntent(intent)
        // required if your activity's launch mode is "singleTop", "singleTask", or "singleInstance"
        setIntent(intent)
    }

    override fun onPayPalSuccess(payPalAccountNonce: PayPalAccountNonce) {
        setSuccessResult(payPalAccountNonce.string, payPalAccountNonce.email)
    }

    override fun onPayPalFailure(error: Exception) {
        when (error) {
            is UserCanceledException -> setErrorResult(error, cancelled = true)
            else -> setErrorResult(error)
        }
    }

    private fun tokenizePayPalAccountVault() {
        val request = PayPalVaultRequest()
        request.billingAgreementDescription = "Company name"
        payPalClient.tokenizePayPalAccount(this, request)
    }

    private fun setSuccessResult(payPalAccountNonce: String, userEmail: String?) {
        val intent = Intent().apply {
            putExtra(KEY_RESULT_SUCCESS_NONCE, payPalAccountNonce)
            putExtra(KEY_RESULT_SUCCESS_EMAIL, userEmail)
        }
        goBack(RESULT_OK, intent)
    }

    private fun setErrorResult(error: Exception?, cancelled: Boolean = false) {
        val intent = Intent().apply {
            // We have our own localized message for cancelled
            if (!cancelled) putExtra(KEY_RESULT_ERROR, error?.message)
            putExtra(KEY_RESULT_CANCELLED, cancelled)
        }
        goBack(RESULT_CANCELED, intent)
    }

    private fun goBack(resultCode: Int, intent: Intent) {
        setResult(resultCode, intent)
        finish()
    }

    companion object {

        private const val KEY_PAYPAL_TOKEN = "paypal_client_token"
        private const val KEY_RESULT_SUCCESS_NONCE = "result_success_nonce"
        private const val KEY_RESULT_SUCCESS_EMAIL = "result_success_email"
        private const val KEY_RESULT_ERROR = "result_error"
        private const val KEY_RESULT_CANCELLED = "user_cancelled"

        fun toSuccess(extras: Bundle): BraintreeSuccess {
            val nonce = extras.getString(KEY_RESULT_SUCCESS_NONCE)
                ?: error("Should be called only on Activity.RESULT_OK")
            val userEmail = extras.getString(KEY_RESULT_SUCCESS_EMAIL)
            return BraintreeSuccess(nonce, userEmail)
        }

        fun toException(extras: Bundle): PaymentException {
            val errorMsg = extras.getString(KEY_RESULT_ERROR)
            val cancelled = extras.getBoolean(KEY_RESULT_CANCELLED, false)
            return if (cancelled) {
                PaymentException(msg = errorMsg, cancelled = true)
            } else {
                PaymentException(msg = errorMsg)
            }
        }

        fun start(
            activity: Activity,
            requestCode: Int,
            payPalClientToken: String,
        ) {
            val intent = Intent(activity, BraintreeActivity::class.java)
            intent.putExtra(KEY_PAYPAL_TOKEN, payPalClientToken)
            activity.startActivityForResult(intent, requestCode)
            activity.overridePendingTransition(android.R.anim.fade_in, android.R.anim.fade_out)
        }
    }

}

class BraintreeSuccess(val payPalNonce: String, val userEmail: String?)

And here is our manifest with our Main activity + PayPal activity config:

       <activity
           android:name=".activities.MainActivity"
           android:exported="true"
           android:label="App name"
           android:launchMode="singleTask"
           android:resizeableActivity="false"
           android:screenOrientation="portrait"
           android:theme="@style/ExMainTheme"
           android:windowSoftInputMode="stateHidden|adjustResize">
       </activity>

       <activity
            android:name=".braintree.BraintreeActivity"
            android:exported="true"
            android:label="app name"
            android:launchMode="singleTask"
            android:resizeableActivity="false"
            android:screenOrientation="portrait"
            android:theme="@style/ExMainTheme"
            android:windowSoftInputMode="stateHidden|adjustResize"
            tools:ignore="UnusedAttribute">

            <intent-filter>
                <action android:name="android.intent.action.VIEW" />
                <data android:scheme="${applicationId}.braintree" />

                <category android:name="android.intent.category.DEFAULT" />
                <category android:name="android.intent.category.BROWSABLE" />
            </intent-filter>

       </activity>
hollabaq86 commented 2 years ago

👋 @mikkoville thanks for reaching out, I don't expect this to change the behavior, but need to ask: can you update to the latest version, 4.11.0?

We added a boolean isExplicitCancelation to UserCanceledExceptions which can give us more info on these exceptions (I'm assuming the value for these scenarios will be false).

We'll try to replicate this issue and come back to you with feedback.

mikkoville commented 2 years ago

@hollabaq86 sure I will update and do some testing also.

FYI I have been able to partly break the the PayPal flow myself by setting the Developer options -> Don't keep activities flag. Is the UserCanceledExceptions persisted in the BraintreeSdk? Could it be that our users experienced a process / activity death during the flow and now they are getting this persisted cancelled event every time when we launch the PayPal activity? is thisisExplicitCancelation intention to mitigate these false persisted cancellation events?

sshropshire commented 2 years ago

@mikkoville the PayPal flow is designed to only deliver a "canceled" event once (on first access). After that, the browser switching context remains dormant until the user is able to complete the flow.

The isExplicitCancelation flag is designed to notify merchants that the "cancel" link was pressed in the PayPal window. Conversely, when a user returns to the app without completing the flow, we consider it an implicit cancelation.

The Chrome Custom Tabs API prevents us from knowing when the user has pressed the "X" button, so our design is constrained a bit. We're actively seeking feedback for our upcoming next major version and it would be really helpful to hear ideas on how we can improve this part of the SDK.

mikkoville commented 2 years ago

@sshropshire just for me to understand the issue better

Are you persisting the cancellation issue into the BraintreeSDK somehow? I mean store it to a file or database? The reason I am asking because when I was testing with Developer options -> Don't keep activities flag I got into a loop where every time I start my BraintreeActivity (See code in OP) I got UserCanceledException in the onPayPalFailure block. Only way to get out of this loop was to clear my application storage.

If that is the case that it is persisted I am thinking should it be cleared in some situations like when the PayPalClient is initialised? I mean what could be the valid case where right after it being initialised and the listener set we would get a persisted cancelled event?

Forgive me if my speculations about persistence are wrong 🙂

sshropshire commented 2 years ago

@mikkoville correct yes. We store some metadata in encrypted shared prefs to protect payment flows from being interrupted by process kills.

Looking again at the provided code snippet though, I wonder if calling tokenize in onCreate may be causing a loop here? The browser switch delivery code actually runs immediately after onCreate, since we're using lifecycle observers under the hood.

Once the onCreate method is complete and control is returned back to the Android framework, all newly added lifecycle observers will start be notified that the lifecycleOwner (BraintreeActivity in this case) has now reached the CREATED state.

mikkoville commented 2 years ago

@sshropshire right. So where do you suggest us to call tokenize(). Our case seems to be the standard use of your api. Just launch a separate Activity that will call tokenize() after Braintree and PayPal clients are initialised properly. Would it be onStart()? (probably not since onStart() is called again after onNewIntent() and we would call tokenise again when coming back through the intent-filter)

If you are storing the cancelled event in case of process kill how should we handle that? I mean if the process is killed during the tokenize process and our users try to tokenize again after process death the first event they are getting from the PayPal listener is the Cancelled event. I think we should have a way to flush stored cancelled event by API call or do that automatically when PayPalClient is initialised. Or do you see a valid case where after PayPalClient is constructed and we would still need a cancelled event from the previous instance 🤔

sshropshire commented 2 years ago

@mikkoville the best way in this case would be in response to some user action. I understand there may be edge cases though and if we need to look into providing new functionality we can consider adding some new API functionality to support this use case.

As a workaround, in onCreate calling tokenize from within a launchWhenResumed block may work here. This example shows it being called as early as init.

Once the PayPalClient is instantiated and the listener is set, deferring tokenize until onResume should give the lifecycle observer enough time to respond to the onCreate event being finished.

sshropshire commented 2 years ago

Hey @mikkoville touching base. Any luck with the proposed workaround?

mikkoville commented 2 years ago

@sshropshire yes. Sorry I've been away. We can try to workaround it by racing the event but this feels quite bad to me and Im not sure if it will work for all cases 🤔

I saw in the latest release notes mention of native checkout experience? Maybe this is something that could solve this eventually?

sshropshire commented 2 years ago

@mikkoville definitely yes. It's technically in beta at the moment but we won't be introducing any breaking changes in this current major version. We coordinate directly with the native checkout team internally at PayPal and it has been battle-tested by several pilot merchants.

mikkoville commented 1 year ago

@sshropshire could I get more details on this native checkout thing. I see mentions of it in the latest release but cannot find any docs or samples how to use it. Is it something you plan to release to public even though it is still in beta? Thanks

aligokdemir commented 1 year ago

hello, I'm also getting this UserCancelledException when doing checkout with BrainTree SDK, my implementation is very similar to @mikkoville's approach. but once users get stuck in this error, they just can't get rid of it. will there be any fixes in the future releases for this?

mikkoville commented 1 year ago

Yes. With the current SDK it is possible for users get into this "Cancelled" loop forever until they clear the app data which no user is gonna do. The root issue is that the cancelled event is stored in Braintree sdk and it gets sent always as the first event when listener is attached.

So I am very eager to hear more about the native flow or a way to clear this cancelled event before attaching the listener. We have increasing number of users who cannot use PayPal for this reason.

sshropshire commented 1 year ago

@mikkoville @aligokdemir our Native Checkout module for PayPal is in beta. It uses an activity instead of switching to a browser. This takes Chrome Custom Tabs out of the equation.

There's a limitation with Chrome Custom Tabs that prevents us from knowing if the user pressed the close / 'X' button on the top left of the screen, so when a user brings the merchant app back into the foreground, we assume it to be a cancelation.

Like @hollabaq86 mentioned you could check for the newly added boolean property isExplicitCancelation on UserCanceledException. When this is true, it means the user selected the "Return to [MERCHANT_APP_NAME]" link in the PayPal web flow. You can ignore the exception when this property is false as well since we have no guaranteed way to determine the intent of the user at that point.

mikkoville commented 1 year ago

@sshropshire thanks for the response.

I have tried to use that newly added boolean property but I couldn't use it get rid of the issue where we get a persisted cancelled event from BraintreeClient right after the listener is attached. This happens when users previous try is broken by activity death due to low device resources or similar situation and the cancelled event is not cleared from BraintreeClient.

I am looking forward to trying the native solution but still little bit hesitant to put it into production it being Beta. Do you have any even rough ETAs when it would be considered production ready. Thanks

stsc3000 commented 1 year ago

@mikkoville We are facing the same issue, although from the info we received, the process is not killed in the background. Did you find a workaround? Our Activity setup and initialisation process is very similar as well.

sshropshire commented 1 year ago

This happens when users previous try is broken by activity death due to low device resources or similar situation and the cancelled event is not cleared from BraintreeClient.

So far I have these repro steps:

  1. User launches PayPal flow
  2. User explicitly cancels PayPal flow by pressing "Return to [MERCHANT_APP_NAME]"
  3. Activity is relaunched via deep link (and re-created after a process kill)

Actual Behavior: Activity is cold started and Braintree SDK notifies explicit cancelation with isExplicitCancelation boolean set to true.

For your app, would the ideal expected behavior be to allow the Activity to cold start without sending a UserCanceledException?

mikkoville commented 1 year ago

@sshropshire yes.

I think Braintree SDK should not persist this user cancelled event at all. I think it creates more problems than it solves. I've seen other related issuese on your issue tracker that seem to point to the same issue. e,g https://github.com/braintree/braintree_android/issues/588

I cannot think of a valid use case where we would need a persisted cancelled event. If the activity is re-created can't the user just press the cancel button again or is there some other internal reason for this?

The callback is quite unreliable since we don't know if it emits a valid cancelled event. Also as mentioned in this issue already users can end up in a loop where every time we attach to the callback we get the persisted cancelled event -> user exits the PayPal flow and the event stays there every time user tries again. Only way I was able to exit the loop was to clear the app storage which clears the Braintree SDK storage as well.

The solution would be to not persist the User cancelled event or at least provide a way for us to clear it from the Braintree sdk. This way we could always clear it before attaching the listener.

mikkoville commented 1 year ago

@stsc3000 We have not found a workaround to this yet :( We are also quite unaware why some users end up in this never ending loop. Process kill is just one way I can reproduce it myself in our setup. There might be other ways to end up in this. Only thing I am sure it because of the persisted UserCanceledException that is sent by Braintree SDK. The issue is always solved by clearing the app data which clears the persisted state.

sshropshire commented 1 year ago

@mikkoville thanks for this feedback. Internally, we've discussed removing the CANCELED concept. We're hoping we can replace it with something else in our API that allows merchants to check if a browser switch is in progress.

Also, after further investigation, we did find that browser switch will still launch after the host activity is finished. We have a fix to defensively check if the activity is finished before browser switching. Once it's merged we'll update the core SDK as well.

SyrineITrabelsi commented 1 year ago

@sshropshire Hello ! Running into this issue as well on paypal version 4.15.0. do you have an idea on which version the fix is included? I cannot update the library with this issue.

sshropshire commented 1 year ago

@SyrineITrabelsi @mikkoville we're still deciding on the best path forward for v5 of the SDK. We did fix an issue where browser based flows were launching in activities that were in a "finishing" state. Could you try the latest 4.26.0 version and verify that these steps still trigger unexpected behavior?

SyrineITrabelsi commented 1 year ago

@sshropshire I tried with the latest 4.26.0 All tests passes and also the steps shown trigger the right behaviour.

sshropshire commented 1 year ago

Thanks @SyrineITrabelsi !

sshropshire commented 1 year ago

Hi @mikkoville does this issue still occur in the latest 4.26.1 version as mentioned?

mikkoville commented 1 year ago

@sshropshire unfortunately it does not solve the root issue :(

I can still easily reproduce it by the same steps in the original message:https://github.com/braintree/braintree_android/issues/557#issuecomment-1148307803

So if I turn on Don't keep activites and switch between 2 activities where second one has the PayPalListener from your side. The SDK gets into a state where every time I attach a PayPalListener on the onCreate of my activity I will immediately get a UserCanceledException as a first event from the sdk.

So the root issue of the persisted "user cancelled" event is not solved. I am afraid this will not be solved completely until the event is not persisted at all or the event should be cleared from the sdk.

I am already considering to just stop listening for the UserCanceledException in the callback. This would solve our big issue of users not being able to use PayPal at all but of course it would produce poor UX for users who actually want to cancel the flow since that would be partly broken.

mikkoville commented 1 year ago

@sshropshire and others here hitting the same issue. I have made an workaround for the issue which is very ugly but seems to solve the main issue I can reproduce every time.

How to reproduce the issue I have 2 activities. Let's call them MainActivity & BraintreeActivity.

MainActivity launches BraintreeActivity and BraintreeActivity#onCreate() attaches the PayPalListener to the PayPalClient and after that calls payPalClient.tokenizePayPalAccount()

While I am in the BraintreeActivity I kill the application process or just the chrome process running the PayPal webView. After this every time I enter BraintreeActivity it will receive the UserCancelledException as the first event from the listener. I treat this event as user wanting to exit the flow so I exit the BraintreeActivity. This will now happen forever until I clear the app data which clears also persisted states from Braintree SDK.

So I made an ugly hack to swallow UserCancelledException sent by the listener before I know we have tried to tokenise. Here is an example:

class BraintreeActivity : AppCompatActivity(), PayPalListener {

    private lateinit var braintreeClient: BraintreeClient
    private lateinit var payPalClient: PayPalClient

    private var didTryToTokenize: Boolean = false

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        val payPalToken = intent.extras?.getString(KEY_PAYPAL_TOKEN)
        if (payPalToken != null) {
            braintreeClient = BraintreeClient(this, payPalToken)
            payPalClient = PayPalClient(this, braintreeClient)
        } else {
            setErrorResult(exception)
        }
        payPalClient.setListener(this)
        tokenizePayPalAccountVault()
    }

    override fun onPayPalSuccess(payPalAccountNonce: PayPalAccountNonce) {
        setSuccessResult(payPalAccountNonce.string, payPalAccountNonce.email)
    }

    override fun onPayPalFailure(error: Exception) {
        // Swallow all errors before we try to tokenize
        if (!didTryToTokenize) return
        when (error) {
            is UserCanceledException -> setErrorResult(error, cancelled = true)
            else -> setErrorResult(error)
        }
    }

    private fun tokenizePayPalAccountVault() {
        // We need to add some delay here since we need to make sure false positive
        // UserCancelledExceptions are swallowed before me mark didTryToTokenize = true
        someView.postDelayed(
            {
                val request = PayPalVaultRequest()
                payPalClient.tokenizePayPalAccount(this, request)
                didTryToTokenize = true
            },
            500
        )
    }

    private fun setSuccessResult(payPalAccountNonce: String, userEmail: String?) {
        val intent = Intent().apply {
            putExtra(KEY_RESULT_SUCCESS_NONCE, payPalAccountNonce)
            putExtra(KEY_RESULT_SUCCESS_EMAIL, userEmail)
        }
        goBack(RESULT_OK, intent)
    }

    private fun setErrorResult(error: Exception?, cancelled: Boolean = false) {
        val intent = Intent().apply {
            if (!cancelled) putExtra(KEY_RESULT_ERROR, error?.message)
            putExtra(KEY_RESULT_CANCELLED, cancelled)
        }
        goBack(RESULT_CANCELED, intent)
    }

    private fun goBack(resultCode: Int, intent: Intent) {
        setResult(resultCode, intent)
        finish()
    }

}

Let's see if we still get reports of users not being able to use PayPal with this 🤞

Ideally this should be fixed in the sdk so we would not receive the false positive events 🙏

sshropshire commented 1 year ago

@mikkoville that is interesting and a commendable workaround. One thing we've discussed going forward is removing some of the abstraction and encapsulation for browser and app switching to make the SDK less opinionated.

We're currently migrating our codebase to Kotlin and soon after, we'll be starting work on the next major version of the SDK to improve the overall developer experience. We may consider surfacing some of the internal methods in the SDK to make integration issues like this less common.

aligokdemir commented 1 year ago

still no solutions to this right? we are still having issues on 4.26.1 version of the sdk.

sshropshire commented 1 year ago

@aligokdemir we actually have a PR up that would address this issue by giving merchants more control over browser switching: https://github.com/braintree/braintree_android/pull/730. We don't have an exact timeline yet, there's been some internal re-prioritization of effort and we've been adjusting the roadmap of the SDK to fit our new team structure.

sdoward commented 1 year ago

I am experiencing this issue also. It seems to be a pretty major bug that prevents users being able to donate. It would be nice if the developers could make a bug fix for it

sshropshire commented 1 year ago

@sdoward we've recently learned from other merchants that having more control over browser switching can be helpful for scenarios like this. Can you try using the manual integration pattern we added to see if this helps resolve the issue?

We plan on making this style of the integration the default in our next major version, but for now we have to support both method to avoid introducing a breaking change to the 4.x version of the SDK.

pandelisgreen13 commented 10 months ago

I have the same issue after upgrading from version 3 to 4, manual integration pattern does not work, so I mixed up the manual integration with an old one integration guide and it works without any problem

`

class BraintreePaypalActivity : BaseActivity() {

private lateinit var braintreeClient: BraintreeClient
private lateinit var payPalClient: PayPalClient

private lateinit var binding: ActivityBraintreePaypalBinding

override fun onNewIntent(newIntent: Intent?) {
    super.onNewIntent(newIntent)
    // required if your activity's launch mode is "singleTop", "singleTask", or "singleInstance"
    intent = newIntent
}

override fun onResume() {
    super.onResume()

    val browserSwitchResult = braintreeClient.deliverBrowserSwitchResult(this)
    handlePaypalResult(browserSwitchResult)
}

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    binding = ActivityBraintreePaypalBinding.inflate(layoutInflater)
    setContentView(binding.root)

    onBackPressedDispatcher.addCallback(this, object : OnBackPressedCallback(true) {
        override fun handleOnBackPressed() {

            braintreeClient.clearActiveBrowserSwitchRequests(this@BraintreePaypalActivity) // I had problems with back press, so I clear the singleton
            finish()
        }
    })

    val token = intent?.getStringExtra(TOKEN).orEmpty()
    val amount = intent?.getStringExtra(AMOUNT).orEmpty()

    braintreeClient = BraintreeClient(this, token)
    payPalClient = PayPalClient(braintreeClient)

    braintreeClient.clearActiveBrowserSwitchRequests(this) // proactive I clear the singleton

    myTokenizePayPalAccountWithCheckoutMethod(amount)

}

private fun myTokenizePayPalAccountWithCheckoutMethod(amount: String) {
    val request = PayPalCheckoutRequest(amount)
    request.currencyCode = EURO
    request.intent = PayPalPaymentIntent.AUTHORIZE
    payPalClient.tokenizePayPalAccount(this, request)
}

private fun handlePaypalResult(browserSwitchResult: BrowserSwitchResult?) {
    if (browserSwitchResult != null && browserSwitchResult.requestCode == BraintreeRequestCodes.PAYPAL) {
        payPalClient.onBrowserSwitchResult(browserSwitchResult) { payPalAccountNonce: PayPalAccountNonce?, error: java.lang.Exception? ->
            if (payPalAccountNonce != null) {
                // Send nonce to server
                val nonce: String = payPalAccountNonce.string

            } else {
                // handle error
                if (error is UserCanceledException) {

                } else {

                }
            }
            braintreeClient.clearActiveBrowserSwitchRequests(this)

        }
    }
}

} `

tdchow commented 1 month ago

Hey all! We just released a beta for the next major version, v5.

In this version the interface has been updated to give the additional browser switch flexibility. We'd love for you all to try out the new version and see if the new integration pattern resolves the original browser switch issue.

v5 PayPal Migration Guide: https://github.com/braintree/braintree_android/blob/main/v5_MIGRATION_GUIDE.md#paypal v5 Release: https://github.com/braintree/braintree_android/releases/tag/5.0.0-beta1