search

braintree / braintree_android

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

Add returnUrlScheme to feature clients #1056

Closed tdchow closed 1 week ago

tdchow commented 2 weeks ago

Summary of changes

Note - I was unable to find a good way to unit test these changes without refactoring how dependency injection currently works. If anyone has other ideas on how to test the returnUrlScheme field, please let me know!

Checklist

Authors

List GitHub usernames for everyone who contributed to this pull request.

sshropshire commented 1 week ago

Note - I was unable to find a good way to unit test these changes without refactoring how dependency injection currently works. If anyone has other ideas on how to test the returnUrlScheme field, please let me know!

This is a great callout. In v5 it seems like BraintreeClient may no longer be needed. Its counterpart in v3 is BraintreeFragment.java, but since we refactored v4 for increased modularity we shouldn't need this class. Also PPCP Android has no concept of a core client.

I'd personally like to remove BraintreeClient–doing so would resolve the DI ambiguity we see here.

KunJeongPark commented 1 week ago

Why do we need this? Is this an android specific feature? For app switching in iOS, I think we hard code the base url components

tdchow commented 1 week ago

Note - I was unable to find a good way to unit test these changes without refactoring how dependency injection currently works. If anyone has other ideas on how to test the returnUrlScheme field, please let me know!

This is a great callout. In v5 it seems like BraintreeClient may no longer be needed. Its counterpart in v3 is BraintreeFragment.java, but since we refactored v4 for increased modularity we shouldn't need this class. Also PPCP Android has no concept of a core client.

I'd personally like to remove BraintreeClient–doing so would resolve the DI ambiguity we see here.

I agree! I feel like we can refactor out BraintreeClient from the feature clients. Luckily it won't be a breaking change and we can hopefully tackle this once v5 is released.

tdchow commented 1 week ago

Why do we need this? Is this an android specific feature? For app switching in iOS, I think we hard code the base url components

This is a specific change for v5. When merging main into the v5 branch the returnUrlScheme param was set on the BraintreeClient, which is no longer implemented by the merchant.

sshropshire commented 1 week ago

Why do we need this? Is this an android specific feature? For app switching in iOS, I think we hard code the base url components

Yes it's Android specific. It allows merchants to explicitly declare a deep link scheme.

One common use case for a merchant providing their own custom url scheme is when their application ID contains underscores. The default AndroidManifest.xml placeholder we recommend would produce an invalid scheme in this case, since URL schemes cannot contain underscores.