[QL] Update `BTAppContextSwitcher.sharedInstance.universalLink` to URL Type #1232

jaxdesmarais · 2024-03-22T13:57:14Z

Summary of changes

Replace BTAppContextSwitcher.sharedInstance.universalLink with init: BTPayPalVaultRequest(userAuthenticationEmail:enablePayPalAppSwitch:universalLink:offerCredit:)
OG convo here: [QL] Add additional POST params to /setup_billing_agreement API call #1228 (comment)

Checklist

~~[ ] Added a changelog entry~~

Authors

@jaxdesmarais

…to URL type

scannillo · 2024-03-25T10:08:51Z

Sources/BraintreePayPal/BTPayPalVaultRequest.swift

@@ -38,12 +38,12 @@ import BraintreeCore
            baseParameters["payer_email"] = userAuthenticationEmail
        }

-        if enablePayPalAppSwitch {
+        if enablePayPalAppSwitch, let universalLinkURL = BTAppContextSwitcher.sharedInstance.universalLink {


Do you think we should return an error if they try to opt into enablePayPalAppSwitch but don't have a universal link set?

🤔 that's a good question - I think it depends on the merchant experience we want. If this is not set we would get nil back as the paypalAppApprovalUrl so would just continue with the ASWeb based flow, but nothing would blow up. On the other side of things I can see a merchant ``not wanting the ASWeb flow if they have set enablePayPalAppSwitch in which case we should add a check on the client that if true this value is also set. Which way are you leaning?

I lean toward returning an error if they don't have the universal link set.

Actually brings me to another thought - we don't need them to pass us the URL in their app delegate, do we? We shouldn't need it that early. We only need it at the time of making the GQ hermes POST request. We could enforce this at the BTPayPalRequest init level. We can add an additional init specific to this flow that takes in everything we need: email, flag, link

Yeah, that is a great question - I was just rolling with the design we had outlined. Technically I don't think the returnURLScheme for the Venmo flow needs to be set in BTAppContextSwitcher either. It's also not used until the tokenize method is called. Do you know historically why those were set in that manner instead of on the clients?

I am down to move this into a new init - I think that makes sense in this PR. We would also need to make a very fast follow docs PR since that was merged in today to be released Wednesday. I think if we make it required in the init we won't need to throw an error from the request or do a check in the client and throw the error there.

If that sounds good to you I can make the change here and get up a docs update as well?

@scannillo this update is reflected in the PR - if we are good with this change, I will open a new docs PR and ping the docs team as well as update our HLD. In BTPayPalVaultRequest I added a new init with the required properties and made universalLink an internal var on the class so it can only be set on the init - this will enforce it as required without us needing to throw an error in the init for this flow.

Do you know historically why those were set in that manner instead of on the clients?

Unfortunately I don't know why it was that way

No worries, it's a great question in any case! I'm going to add a NEXT_MAJOR_VERSION note to returnURLScheme so we can move that to the clients in the future. 🚀

jaxdesmarais · 2024-03-26T15:23:50Z

Sources/BraintreePayPal/BTPayPalVaultRequest.swift

    /// Optional: Used to determine if the customer will use the PayPal app switch flow.
    /// Defaults to `false`.
    /// - Note: This property is currently in beta and may change or be removed in future releases.
-    public var enablePayPalAppSwitch: Bool
+    var enablePayPalAppSwitch: Bool = false


Should we consider defaulting this to true? Do we even need this property at all or can we assume if the universal link is passed a merchant is opting into the flow?

hmm yea that's a good point ~ imo i feel it's safe to remove this property since we can assume whether or not to launch the app switch flow based on if universalLink is passed in

I think we are going to leave this as is for now to make it apparent to merchants that they are opting into this flow with both the bool and a universalLink. With just the universalLink it may make merchants think we are using that on the existing web flow (which we are not)

agedd · 2024-03-26T16:39:04Z

Sources/BraintreePayPal/BTPayPalVaultRequest.swift

+    /// - Parameters:
+    ///   - userAuthenticationEmail: Required: User email to initiate a quicker authentication flow in cases where the user has a PayPal Account with the same email.
+    ///   - enablePayPalAppSwitch: Required: Used to determine if the customer will use the PayPal app switch flow.
+    ///   This property is currently in beta and may change or be removed in future releases.


nit(optional): can we move this line after all the parameter comments

I'm actually going to move it with the main note: 631b3e3. Here is what a merchant will see:

UnitTests/BraintreePayPalTests/BTPayPalRequest_Tests.swift

warmkesselj · 2024-03-26T17:00:55Z

Demo/Application/Features/PayPalWebCheckoutViewController.swift

@@ -87,7 +87,12 @@ class PayPalWebCheckoutViewController: PaymentButtonBaseViewController {

    @objc func universalLinkFlow(_ sender: UIButton) {
        // TODO: implement in a future PR - used here so we don't have to remove lazy instantiation
-        let request = BTPayPalVaultRequest(enablePayPalAppSwitch: true)
+        // TODO: replace URL with https://braintree-ios-demo.fly.dev/braintree-payments


Curious why we can't use: "https://braintree-ios-demo.fly.dev/braintree-payments"

The gateway has not yet allowlisted our URL for this flow, so we are using a URL that is included in their allowlist currently.

CHANGELOG.md

…raintree_ios into universalLink-to-url

switch BTAppContextSwitcher.sharedInstance.universalLink from String …

12257fb

…to URL type

jaxdesmarais requested a review from a team as a code owner March 22, 2024 13:57

scannillo reviewed Mar 25, 2024

View reviewed changes

jaxdesmarais added 4 commits March 25, 2024 19:55

move universalLink into init

cdab32e

update unit test and docstrings

cf2d05e

Merge branch 'paypal-app-switch-feature' into universalLink-to-url

41ce86f

add CHANGELOG; update tests; general cleanup

1cd4a44

jaxdesmarais commented Mar 26, 2024

View reviewed changes

agedd reviewed Mar 26, 2024

View reviewed changes

UnitTests/BraintreePayPalTests/BTPayPalRequest_Tests.swift Outdated Show resolved Hide resolved

warmkesselj reviewed Mar 26, 2024

View reviewed changes

jaxdesmarais added 3 commits March 26, 2024 12:17

PR feedback: update docstrings for init

631b3e3

update test and init

89fef87

Merge branch 'paypal-app-switch-feature' into universalLink-to-url

1020df2

scannillo approved these changes Mar 26, 2024

View reviewed changes

richherrera approved these changes Mar 26, 2024

View reviewed changes

jaxdesmarais added 2 commits March 26, 2024 12:51

add returnURLScheme NEXT_MAJOR_VERSION note

84bb2c4

I cannot spell

24ac06a

jaxdesmarais commented Mar 26, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jaxdesmarais added 3 commits March 26, 2024 13:20

Update CHANGELOG.md

91dccbc

update test with required field

3ddf538

Merge branch 'universalLink-to-url' of https://github.com/braintree/b…

4852b46

…raintree_ios into universalLink-to-url

jaxdesmarais merged commit 6720c73 into paypal-app-switch-feature Mar 26, 2024
5 of 7 checks passed

jaxdesmarais deleted the universalLink-to-url branch March 26, 2024 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QL] Update `BTAppContextSwitcher.sharedInstance.universalLink` to URL Type #1232

[QL] Update `BTAppContextSwitcher.sharedInstance.universalLink` to URL Type #1232

jaxdesmarais commented Mar 22, 2024 •

edited

Loading

scannillo Mar 25, 2024

jaxdesmarais Mar 25, 2024

scannillo Mar 25, 2024

jaxdesmarais Mar 26, 2024

jaxdesmarais Mar 26, 2024

scannillo Mar 26, 2024

jaxdesmarais Mar 26, 2024

jaxdesmarais Mar 26, 2024

agedd Mar 26, 2024

jaxdesmarais Mar 26, 2024

agedd Mar 26, 2024

jaxdesmarais Mar 26, 2024

warmkesselj Mar 26, 2024

jaxdesmarais Mar 26, 2024

[QL] Update BTAppContextSwitcher.sharedInstance.universalLink to URL Type #1232

[QL] Update BTAppContextSwitcher.sharedInstance.universalLink to URL Type #1232

Conversation

jaxdesmarais commented Mar 22, 2024 • edited Loading

Summary of changes

Checklist

Authors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[QL] Update `BTAppContextSwitcher.sharedInstance.universalLink` to URL Type #1232

[QL] Update `BTAppContextSwitcher.sharedInstance.universalLink` to URL Type #1232

jaxdesmarais commented Mar 22, 2024 •

edited

Loading