Venmo - Support App Link Returns

[scannillo]

Epic: DTMOBILES-927
Jira: DTMOBILES-1163

Summary of changes

• Enable merchants to pass a appLinkReturnUri to VenmoClient
• Deprecate returnUrlScheme integration pattern

⚠️ Pending RCIP approval from Venmo side before merge & release.

Checklist

• Added a changelog entry
• Relevant test coverage

Authors

@scannillo

[scannillo]

Without the (String)null cast, the compiler will error with:

VenmoClient is ambiguous. venmoClient = new VenmoClient(requireContext(), super.getAuthStringArg(), null);
Both constructor VenmoClient(Context,String,Uri) in VenmoClient and constructor VenmoClient(Context,String,String) in VenmoClient match

Is this technically breaking for Java merchants? I tried a few things, but landed on this being the best I could come up with. I like the idea of deprecating the constructor that we want merchants to move away from.

I want to get some of our Android experts opinions here on the best public API for this change.

[tdchow]

I would say that this is a breaking change for Java integrations due to requiring the casting of the null argument.

Unfortunately, I'm not aware of an elegant solution for this. One potential solution to the breaking change is to reorder the parameters in the new constructor that takes in the app link. But I think this would go against best practices on parameter ordering.

Another option would be to have a creator function, createVenmoClientWithAppLink(). But this would deviate from all the other clients.

[saperi22]

I would say that this is a breaking change for Java integrations due to requiring the casting of the null argument.

I see your point, but the signatures are different. If someone were sending a null, they now have to take an action, but from a method signature standpoint, we are adding a new one, not removing one. We could argue both ways 🤷.
To not complicate things and move on, we could just document this change in the CHANGELOG.md and move on. It is hard to miss as it is caught by the compiler, and it is easy to fix.
If it were a run time issue, I'd have an issue, but I feel we shouldn't hold on to this as this is a well known limitation of java-kotlin interop. Thoughts?

[scannillo]

That's a great point. We either do it as the PR is written and live with the interop issue, or we have to do something slightly wonky to avoid it (like changing param order or a different integration pattern/method like Tim suggested).

I'm OK with leaving it as-is, but will defer to the teams opinion.

If someone were sending a null, they now have to take an action

We could also default returnUrlScheme: String? = Null to ease things a little, so they can remove the param entirely.

[saperi22]

I'm also ok with leaving this as-is.
Also, +1 to adding the default arg., that would make this discussion redundant I guess.

[tdchow]

Leaving it as is sounds good! I'm also in favor of setting the default value to null. If we do that, we'll need to make sure to add the @JvmOverloads annotation to generate the different constructors for Java.

[scannillo]

Thank you both for the feedback. I updated this PR to make returnUrlScheme nullable + add the JvmOverloads