-
Notifications
You must be signed in to change notification settings - Fork 294
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.
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
Allow Merchant to Only Set Universal Links for Venmo #1503
base: main
Are you sure you want to change the base?
Conversation
if universalLinkReturnToggle.isOn { | ||
venmoClient = BTVenmoClient( | ||
apiClient: apiClient, | ||
// swiftlint:disable:next force_unwrapping | ||
universalLink: URL(string: "https://mobile-sdk-demo-site-838cead5d3ab.herokuapp.com/braintree-payments")! | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seemed pretty odd, is there a usecase where we want a merchant to set a return URL for the switch but use universal links only for the return? That seemed a bit odd to me and what was happening. I would expect if a merchant passes a universal link we want to use that for the switch and return. Maybe we should be setting this for a new universalLinkkToggle
instead so we can test both the UL and return URL scheme flows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was added before we supported universal links to the merchant app. Down to consolidate them into 1! I don't know how much value there would be to having 2 separate toggles since technically a merchant can opt into the web-fallback (UL to venmo), but still use schemes back to their app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much value there would be to having 2 separate toggles since technically a merchant can opt into the web-fallback (UL to venmo), but still use schemes back to their app
I don't think this is true, as the logic exists if we get a universal link we use them for the return:
queryParameters["x-success"] = universalLink.appendingPathComponent("success").absoluteString | |
queryParameters["x-error"] = universalLink.appendingPathComponent("error").absoluteString | |
queryParameters["x-cancel"] = universalLink.appendingPathComponent("cancel").absoluteString |
Is there a reason we'd want to still use URL scheme's to return back to the app if a universal link is passed?
} else if let bundleIdentifier = bundle.bundleIdentifier, !returnURLScheme.hasPrefix(bundleIdentifier) { | ||
NSLog( | ||
// swiftlint:disable:next line_length | ||
"%@ Venmo requires [BTAppContextSwitcher setReturnURLScheme:] to be configured to begin with your app's bundle ID (%@). Currently, it is set to (%@)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to update this to Swift-y syntax instead of ObjC? Would you consider it a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I have always wondered how/if the NSLog
s are actually useful to merchants. It's also interesting that we return an appNotAvailable
for a missing return URL scheme and return nothing and instead fail later if the bundle ID doesn't contain the return URL scheme. To me it seems like they should both be handled the same, though not sure what we consider in scope for this PR.
Summary of changes
BTAppContextSwitcher.sharedInstance.returnURLScheme
for Venmo they get an error if only passing a universal link. We should allow merchants to only use universal links without needing to set both that and areturnURLScheme
I tested the following flows on device, please feel free to retest these or call out any use cases I missed:
universalLink
andreturnURLScheme
not set (returns an error)returnURLScheme
set (usesreturnURLScheme
for switch and return)universalLink
set (usesuniversalLink
for switch and return)Checklist
Authors