Add deep link support as a fallback to app links #1223
Conversation
| * will be returned. | ||
| */ | ||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| class GetReturnLinkTypeUseCase(private val merchantRepository: MerchantRepository) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you expand on what you envision for adding a domain layer?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ok this looks good to me 👍 - @saperi22 @jaxdesmarais any thoughts on this pattern?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
| appLinkReturnUrl: Uri | ||
| ) : this(BraintreeClient(context, authorization, null, appLinkReturnUrl)) | ||
| appLinkReturnUrl: Uri, | ||
| returnUrlScheme: String? = null |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call, let me update the kdoc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| } | ||
|
|
||
| GetReturnLinkTypeUseCase.ReturnLinkType.DEEP_LINK -> { | ||
| returnUrlScheme(merchantRepository.returnUrlScheme) |
There was a problem hiding this comment.
Do we need to check that returnUrlScheme isn't null?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We're actually defaulting the returnUrlScheme to the merchant's package name appended with .braintree, if a null value is passed in.
So the returnUrlScheme in MerchantRepository is non null. I believe this was the existing deep link behavior.
| val returnLinkType = getReturnLinkTypeUseCase() | ||
| val navigationLink = when (returnLinkType) { | ||
| GetReturnLinkTypeUseCase.ReturnLinkType.APP_LINK -> merchantRepository.appLinkReturnUri | ||
| GetReturnLinkTypeUseCase.ReturnLinkType.DEEP_LINK -> merchantRepository.returnUrlScheme |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yep I agree, we'd want to throw an error to prevent a broken flow. I'll update this.
There was a problem hiding this comment.
Similar to the comment above, we're defaulting the returnUrlScheme to the merchant's package name. Did we want to change this behavior?
|
QQ: would it be possible to use the already sent in app link url for deeplink return url? |
The original deep link implementation only set the scheme in the intent filter: While the app link approach requires http/https as the scheme: So I don't think we'll be able to reuse the app link URI for the deep link fallback. |
tdchow
changed the title
Summary of changes
returnUrlSchemeproperty to thePayPalClientconstructor, which is used for deep linking back to the merchant app if the user has unchecked "Open supported links".GetReturnLinkTypeUseCasethat returns which link type should be sent to the PayPal request and payment URLChecklist
Authors