Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add deep link support as a fallback to app links #1223

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+483 −74
Open

Add deep link support as a fallback to app links #1223

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40af61b
Add deep link support as a fallback to app links
tdchow Nov 22, 2024
739c4c0
Add unit tests and kdoc
tdchow Dec 2, 2024
b65f538
Add a null check for both app link and deep link urls
tdchow Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions BraintreeCore/src/main/java/com/braintreepayments/api/core/GetReturnLinkTypeUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package com.braintreepayments.api.core

import android.content.Intent
import android.content.pm.PackageManager
import androidx.annotation.RestrictTo
import com.braintreepayments.api.core.GetReturnLinkTypeUseCase.ReturnLinkType

/**
* Use case that returns which link type should be used for navigating from App Switch / CCT back into the merchant app.
*
* If a user unchecks the "Open supported links" checkbox in the Android OS settings for the merchant's app. If this
* setting is unchecked, this use case will return [ReturnLinkType.DEEP_LINK], otherwise [ReturnLinkType.APP_LINK]
* will be returned.
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
class GetReturnLinkTypeUseCase(private val merchantRepository: MerchantRepository) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on adding a domain layer / use case pattern to the SDK? I wasn't able to find an existing pattern to share business logic outside of creating additional Client classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on what you envision for adding a domain layer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I see it as a layer that encapsulates business logic that can be reused across the SDK. It's also the layer between the UI and the data layer, where it reads data from the repository and applies some sort of business logic.

https://developer.android.com/topic/architecture/domain-layer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this looks good to me 👍 - @saperi22 @jaxdesmarais any thoughts on this pattern?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this pattern. At some point we should consider documenting the Android and iOS patterns internally since they have diverged quite a bit in recent major versions. Unless this is more of a best practice in Android, in which case probably fine to leave undocumented and just slowly migrate to each languages best practices!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say MVVM is the architecture pattern that Google is pushing since they have entire doc sections on the different layers of MVVM. That said, it wouldn't hurt to have a callout somewhere in the repo of the architecture patterns the SDK is using.


enum class ReturnLinkType { APP_LINK, DEEP_LINK }

operator fun invoke(): ReturnLinkType {
val context = merchantRepository.applicationContext
val intent = Intent(Intent.ACTION_VIEW, merchantRepository.appLinkReturnUri).apply {
addCategory(Intent.CATEGORY_BROWSABLE)
}
val resolvedActivity = context.packageManager.resolveActivity(intent, PackageManager.MATCH_DEFAULT_ONLY)
return if (resolvedActivity?.activityInfo?.packageName == context.packageName) {
ReturnLinkType.APP_LINK
} else {
ReturnLinkType.DEEP_LINK
}
}
}
7 changes: 4 additions & 3 deletions Demo/src/main/java/com/braintreepayments/demo/PayPalFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,10 @@ public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup c
});

payPalClient = new PayPalClient(
requireContext(),
super.getAuthStringArg(),
Uri.parse("https://mobile-sdk-demo-site-838cead5d3ab.herokuapp.com/braintree-payments")
requireContext(),
super.getAuthStringArg(),
Uri.parse("https://mobile-sdk-demo-site-838cead5d3ab.herokuapp.com/braintree-payments"),
null
);
payPalLauncher = new PayPalLauncher();

Expand Down
19 changes: 16 additions & 3 deletions PayPal/src/main/java/com/braintreepayments/api/paypal/PayPalClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.braintreepayments.api.core.BraintreeClient
import com.braintreepayments.api.core.BraintreeException
import com.braintreepayments.api.core.BraintreeRequestCodes
import com.braintreepayments.api.core.Configuration
import com.braintreepayments.api.core.GetReturnLinkTypeUseCase
import com.braintreepayments.api.core.LinkType
import com.braintreepayments.api.core.MerchantRepository
import com.braintreepayments.api.core.UserCanceledException
Expand All @@ -24,6 +25,7 @@ class PayPalClient internal constructor(
private val braintreeClient: BraintreeClient,
private val internalPayPalClient: PayPalInternalClient = PayPalInternalClient(braintreeClient),
private val merchantRepository: MerchantRepository = MerchantRepository.instance,
private val getReturnLinkTypeUseCase: GetReturnLinkTypeUseCase = GetReturnLinkTypeUseCase(merchantRepository)
) {

/**
Expand Down Expand Up @@ -54,8 +56,9 @@ class PayPalClient internal constructor(
constructor(
context: Context,
authorization: String,
appLinkReturnUrl: Uri
) : this(BraintreeClient(context, authorization, null, appLinkReturnUrl))
appLinkReturnUrl: Uri,
returnUrlScheme: String? = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should include doc strings noting that if this parameter exists, it will be used as a fallback when app links are not available - merchants should be aware that they are opting in to not using app links in all cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, let me update the kdoc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to consider any type of okay from security for adding this back? I know we were moving towards removing url schemes across the SDKs for security reasons but wasn't sure what applied with it as an opt in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to run this by security as our other modules are still relying on deep links for navigating back to the merchant app. I also don't think security originally required the switch to app links. @sarahkoop - do you have context on if security was originally involved with the switch to app links?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think since we are still supporting deep links across multiple flows it's fine to add back in - this originated as a merchant request not an internal security mandate

) : this(BraintreeClient(context, authorization, returnUrlScheme, appLinkReturnUrl))

/**
* Starts the PayPal payment flow by creating a [PayPalPaymentAuthRequestParams] to be
Expand Down Expand Up @@ -152,10 +155,20 @@ class PayPalClient internal constructor(

return BrowserSwitchOptions()
.requestCode(BraintreeRequestCodes.PAYPAL.code)
.appLinkUri(merchantRepository.appLinkReturnUri)
.url(Uri.parse(paymentAuthRequest.approvalUrl))
.launchAsNewTask(braintreeClient.launchesBrowserSwitchAsNewTask())
.metadata(metadata)
.apply {
when (getReturnLinkTypeUseCase()) {
GetReturnLinkTypeUseCase.ReturnLinkType.APP_LINK -> {
appLinkUri(merchantRepository.appLinkReturnUri)
}

GetReturnLinkTypeUseCase.ReturnLinkType.DEEP_LINK -> {
returnUrlScheme(merchantRepository.returnUrlScheme)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check that returnUrlScheme isn't null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I think we should check if returnUrlScheme is non-null. However, how should we handle the null case?

If the buyer has the Open supported links unchecked, and the merchant passes a null returnUrlScheme the deep link would fail. Maybe we should throw an error before the app switch/CCT is launched and fail fast?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think fail fast and let the merchant decide if they want to tell the user to turn on that setting or how they want to handle it if they don't want to support deep links

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're actually defaulting the returnUrlScheme to the merchant's package name appended with .braintree, if a null value is passed in.

braintree_android/BraintreeCore/src/main/java/com/braintreepayments/api/core/BraintreeClient.kt

Line 54 in bafeda7

?: "${getAppPackageNameWithoutUnderscores(context.applicationContext)}.braintree",

So the returnUrlScheme in MerchantRepository is non null. I believe this was the existing deep link behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think this assumes that the merchant has configured their manifest to support the .braintree deep link - if they haven't explicitly opted into deep link fallbacks and haven't set up their manifest this would fail right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it would fail if they do not define the intent filter. Hmm I guess having it more explicit would be better for the deep link fallback. I'm thinking we may need to have a new field that stores the deep link fallback URL scheme if the other modules are still relying on the existing URL scheme with this default value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just verified that we're using the existing returnUrlScheme with the default value for the other modules. I'll add a new param to the MerchantRepository for holding the fallback URL scheme.

}
}
}
}

/**
Expand Down
29 changes: 25 additions & 4 deletions PayPal/src/main/java/com/braintreepayments/api/paypal/PayPalInternalClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.braintreepayments.api.core.BraintreeClient
import com.braintreepayments.api.core.BraintreeException
import com.braintreepayments.api.core.Configuration
import com.braintreepayments.api.core.DeviceInspector
import com.braintreepayments.api.core.GetReturnLinkTypeUseCase
import com.braintreepayments.api.core.MerchantRepository
import com.braintreepayments.api.datacollector.DataCollector
import com.braintreepayments.api.datacollector.DataCollectorInternalRequest
Expand All @@ -20,9 +21,8 @@ internal class PayPalInternalClient(
private val apiClient: ApiClient = ApiClient(braintreeClient),
private val deviceInspector: DeviceInspector = DeviceInspector(),
private val merchantRepository: MerchantRepository = MerchantRepository.instance,
private val getReturnLinkTypeUseCase: GetReturnLinkTypeUseCase = GetReturnLinkTypeUseCase(merchantRepository)
) {
private val cancelUrl = "${merchantRepository.appLinkReturnUri}://onetouch/v1/cancel"
private val successUrl = "${merchantRepository.appLinkReturnUri}://onetouch/v1/success"
private val appLink = merchantRepository.appLinkReturnUri?.toString()

fun sendRequest(
Expand Down Expand Up @@ -50,12 +50,28 @@ internal class PayPalInternalClient(
payPalRequest.enablePayPalAppSwitch = isPayPalInstalled(context)
}

val returnLinkType = getReturnLinkTypeUseCase()
val navigationLink = when (returnLinkType) {
GetReturnLinkTypeUseCase.ReturnLinkType.APP_LINK -> merchantRepository.appLinkReturnUri
GetReturnLinkTypeUseCase.ReturnLinkType.DEEP_LINK -> merchantRepository.returnUrlScheme
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here - should we be checking that the merchant has included a returnUrlScheme? If a merchant only wants to support app links and app links are not enabled on the device we should have an error scenario I assume

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I agree, we'd want to throw an error to prevent a broken flow. I'll update this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the comment above, we're defaulting the returnUrlScheme to the merchant's package name. Did we want to change this behavior?

}
val appLinkParam = if (
navigationLink == GetReturnLinkTypeUseCase.ReturnLinkType.APP_LINK && isBillingAgreement
) {
merchantRepository.appLinkReturnUri?.toString()
} else {
null
}

val cancelUrl = "$navigationLink://onetouch/v1/cancel"
val successUrl = "$navigationLink://onetouch/v1/success"

val requestBody = payPalRequest.createRequestBody(
configuration = configuration,
authorization = merchantRepository.authorization,
successUrl = successUrl,
cancelUrl = cancelUrl,
appLink = appLinkReturn
appLink = appLinkParam
) ?: throw JSONException("Error creating requestBody")

sendPost(
Expand Down Expand Up @@ -123,12 +139,17 @@ internal class PayPalInternalClient(
)
}

val returnLink = when (getReturnLinkTypeUseCase()) {
GetReturnLinkTypeUseCase.ReturnLinkType.APP_LINK -> merchantRepository.appLinkReturnUri
GetReturnLinkTypeUseCase.ReturnLinkType.DEEP_LINK -> merchantRepository.returnUrlScheme
}

val paymentAuthRequest = PayPalPaymentAuthRequestParams(
payPalRequest = payPalRequest,
browserSwitchOptions = null,
clientMetadataId = clientMetadataId,
pairingId = pairingId,
successUrl = successUrl
successUrl = "$returnLink://onetouch/v1/success"
)

if (isAppSwitchEnabled(payPalRequest) && isPayPalInstalled(context)) {
Expand Down
Loading