Cleanup App Switch #1381

jaxdesmarais · 2024-08-05T15:16:29Z

Summary of changes

Move LinkType to enum
Remove logic to pass linkType into Approval URL parser
Move app installed check into Vault Request

Checklist

~~[ ] Added a changelog entry~~

Authors

@jaxdesmarais

Sources/BraintreeCore/Analytics/LinkType.swift

Co-authored-by: scannillo <[email protected]>

scannillo · 2024-08-05T15:28:53Z

Sources/BraintreePayPal/BTPayPalVaultRequest.swift

@@ -19,6 +19,9 @@ import BraintreeCore
    /// - Warning: This property is currently in beta and may change or be removed in future releases.
    var enablePayPalAppSwitch: Bool = false

+    /// exposed for mocking in tests
+    var application: URLOpener = UIApplication.shared


IMO it's a little strange for VaultRequest to do the app installed inspection. It adds logic into our "model" type

Agree, but it also felt odd that we were overriding a property in the client to ensure we don't pass the app switch params if the app is not installed here:

braintree_ios/Sources/BraintreePayPal/BTPayPalClient.swift

Lines 336 to 338 in 6a375f2

if !self.payPalAppInstalled {

(request as? BTPayPalVaultRequest)?.enablePayPalAppSwitch = false

}

Do you know where else this logic could live that would make more sense? We could potentially pass it as a property when we call parameters() similar to universalLink. Curious to hear your thoughts.

IMO the way it exists now is the highest separation of concerns (business logic in PayPalClient, model only in VaultRequest)

Would we be okay passing it through here instead? I don't think changing a param in the model is the best separation of concern either. This came up when @richherrera was looking through the implementation for Android and this was a point of confusion.

braintree_ios/Sources/BraintreePayPal/BTPayPalVaultRequest.swift

Line 49 in 6a375f2

public override func parameters(with configuration: BTConfiguration, universalLink: URL? = nil) -> [String: Any] {

I also agree with Sammy. It's correct—after reviewing the iOS implementation, I didn’t understand why the model had "enablePayPalAppSwitch" set to false, no matter if I configured it as true. I also agree with you, Jax, that modifying this method isn't ideal. I think we could leave it as it is.

I understand the concern now - currently we're changing the merchant-facing VaultRequest. enablePayPalAppSwitch setting without them knowing, which could be confusing. Jax I like the idea of injecting into the parameters() method more than having a reference to UIApplication in the VaultRequest!

Yeah exactly - sorry I realize I should have done a better job of explaining the intent of the change initially. Pushed the change here: 893d736

richherrera

LGTM!!

richherrera · 2024-08-06T16:11:04Z

Sources/BraintreePayPal/BTPayPalApprovalURLParser.swift

@@ -45,8 +45,8 @@ struct BTPayPalApprovalURLParser {
        return nil
    }

-    init?(body: BTJSON, linkType: String?) {


…cation from VaultRequest

KunJeongPark · 2024-08-07T21:42:48Z

Sources/BraintreePayPal/BTPayPalApprovalURLParser.swift

-    init?(body: BTJSON, linkType: String?) {
-        if linkType == "universal", let payPalAppRedirectURL = body["agreementSetup"]["paypalAppApprovalUrl"].asURL() {
+    init?(body: BTJSON) {
+        if let payPalAppRedirectURL = body["agreementSetup"]["paypalAppApprovalUrl"].asURL() {


For EditFI work, we are not using payPalAppApprovalUrl because app switching is not in scope yet for that work stream. But if the gateway returns this value as well as approvalURL in generate_edit_fi_url response, using this Parser we would get payPalAppRedirectURL instead of the approvalURL. For right now for the draft on a branch (editFI-gateway-endpoints), I just forced the web Url fetching by passing in linkType as nil (or could have passed in "deepLink" ). I guess I can use something else to get the webURL.

Is the gateway returning a payPalAppApprovalUrl for the edit FI endpoints? Because they should not be and we should raise that if so. This was only ever added because when we were working off of stage we always got a payPalAppApprovalUrl regardless of eligibility and was just never cleaned up. See old convo here: #1271 (comment). I tested the experience in this PR and no longer see this behavior with the app uninstalled.

The Gateway specs on Confluence show: Response - agreementSetup {tokenId, paypalAppApprovalUrl, approvalUrl}
as response for edit FI url endpoint. Maybe the doc is not updated or maybe it's supposed to return just approval url. I will check with you and Gateway team tomorrow.

Okay, then removing the link type will not impact that flow and we should not need to pass in a link type, it will always fail this first check for a payPalAppApprovalUrl and move on to the next block which includes the web URL.

I've posted the question on the collaboration channel so we can clarify with them.

Fwiw we also have to opt in to the app switch flow for the Gateway to return that property, so I would expect it only exists to future proof this feature. We need to pass them everything here to say the merchant opted into app switch which we won't be doing for Edit FI currently:

braintree_ios/Sources/BraintreePayPal/BTPayPalVaultRequest.swift

Lines 56 to 64 in 38297e4

if enablePayPalAppSwitch, let universalLink {

let appSwitchParameters: [String: Any] = [

"launch_paypal_app": enablePayPalAppSwitch,

"os_version": UIDevice.current.systemVersion,

"os_type": UIDevice.current.systemName,

"merchant_app_return_url": universalLink.absoluteString

]

return baseParameters.merging(appSwitchParameters) { $1 }

}

The gateway only returns a payPalAppApprovalUrl if those properties are passed AND the merchant is eligible on the gateway side.

Great! thank you for the clarification. I do want to make sure that this is true for editFI endpoint as well since there are no details given in the specs. I want to clarify that payPalAppApprovalUrl will return a nil value or not exist at all for generate_edit_fi_url endpoint if these parameters are not passed. And these parameters are not mentioned for generate_edi_fi_url endpoint.

What you are saying makes sense though.

For sure! Will be good to clarify but shouldn't block this PR. If the gateway does something weird we can make the necessary updates in the Edit FI branch.

jaxdesmarais added 5 commits August 2, 2024 15:07

Merge branch 'main' into cleanup-app-switch

b479e9b

update app install check

407e758

add LinkType enum; update linkType logic

d2dce53

cleanup unneeded logic

7d21b2c

update logic; update tests

fbd98a8

jaxdesmarais requested a review from a team as a code owner August 5, 2024 15:16

jaxdesmarais added 2 commits August 5, 2024 10:20

remove redundant enum type

a3ce334

move constant first in conditional

231e0b0

scannillo reviewed Aug 5, 2024

View reviewed changes

Sources/BraintreeCore/Analytics/LinkType.swift Show resolved Hide resolved

Update Sources/BraintreeCore/Analytics/LinkType.swift

e4f4484

Co-authored-by: scannillo <[email protected]>

scannillo reviewed Aug 5, 2024

View reviewed changes

richherrera approved these changes Aug 6, 2024

View reviewed changes

pass isPayPalAppInstalled into parameters; update tests; remove appli…

893d736

…cation from VaultRequest

KunJeongPark reviewed Aug 7, 2024

View reviewed changes

KunJeongPark approved these changes Aug 8, 2024

View reviewed changes

jaxdesmarais merged commit cc7317f into main Aug 9, 2024
7 of 8 checks passed

jaxdesmarais deleted the cleanup-app-switch branch August 9, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup App Switch #1381

Cleanup App Switch #1381

jaxdesmarais commented Aug 5, 2024

scannillo Aug 5, 2024

jaxdesmarais Aug 5, 2024

scannillo Aug 5, 2024

jaxdesmarais Aug 5, 2024

richherrera Aug 6, 2024

scannillo Aug 7, 2024

jaxdesmarais Aug 7, 2024

richherrera left a comment

richherrera Aug 6, 2024

KunJeongPark Aug 7, 2024 •

edited

Loading

jaxdesmarais Aug 7, 2024

KunJeongPark Aug 7, 2024 •

edited

Loading

jaxdesmarais Aug 7, 2024

KunJeongPark Aug 8, 2024

jaxdesmarais Aug 8, 2024

KunJeongPark Aug 8, 2024 •

edited

Loading

jaxdesmarais Aug 8, 2024

	if !self.payPalAppInstalled {
	(request as? BTPayPalVaultRequest)?.enablePayPalAppSwitch = false
	}

	if enablePayPalAppSwitch, let universalLink {
	let appSwitchParameters: [String: Any] = [
	"launch_paypal_app": enablePayPalAppSwitch,
	"os_version": UIDevice.current.systemVersion,
	"os_type": UIDevice.current.systemName,
	"merchant_app_return_url": universalLink.absoluteString
	]
	return baseParameters.merging(appSwitchParameters) { $1 }
	}

Cleanup App Switch #1381

Cleanup App Switch #1381

Conversation

jaxdesmarais commented Aug 5, 2024

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

richherrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunJeongPark Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunJeongPark Aug 7, 2024 • edited Loading

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

KunJeongPark Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunJeongPark Aug 7, 2024 •

edited

Loading

KunJeongPark Aug 7, 2024 •

edited

Loading

KunJeongPark Aug 8, 2024 •

edited

Loading