Skip to content
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

Open PayPal App URL if present in BT GW response #1234

Merged
merged 18 commits into from
Mar 26, 2024

Conversation

scannillo
Copy link
Contributor

@scannillo scannillo commented Mar 23, 2024

DTBTSDK-3612

Summary of changes

  • Move /v1/paypal_hermes/create_payment_resource & /v1/paypal_hermes/setup_billing_agreement URL response parsing into new file BTPayPalApprovalURLParser
    • Remove manual query param parsing logic in favor of Apple's built-in URLComponents
  • Add logic path for opening PayPal app if paypalAppApprovalUrl response param present from gateway

Checklist

  • Added a changelog entry

Authors

@scannillo

@scannillo scannillo changed the title [DO NOT REVIEW] Open PayPal App URL if present in BT GW hermes response Mar 23, 2024
@scannillo scannillo changed the title Open PayPal App URL if present in BT GW hermes response Open PayPal App URL if present in BT GW response Mar 23, 2024
@scannillo scannillo marked this pull request as ready for review March 25, 2024 10:03
@scannillo scannillo requested a review from a team as a code owner March 25, 2024 10:04
Sources/BraintreePayPal/BTPayPalApprovalURLParser.swift Outdated Show resolved Hide resolved
var urlComponents = URLComponents(url: paypalAppRedirectUrl, resolvingAgainstBaseURL: true)
urlComponents?.queryItems = [
URLQueryItem(name: "source", value: "braintree_sdk"),
URLQueryItem(name: "switch_initiated_time", value: String(UInt64(Date().timeIntervalSince1970 * 1000)))
Copy link
Contributor

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 based on the discussion on confluence around adding additional precision?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated! Also using Int instead of UInt64, which I think we only needed when we supported 32 bit devices way back when

Ran some local tests to confirm the rounding looks good:

  • 1711390872156.703 --> 1711390872157
  • 1711390872661.359 --> 1711390872661
  • 1711390873618.973 --> 1711390873619

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also want to make that change in main for our analytics timestamps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTBTSDK-3703 - ticket to fix on main

Sources/BraintreePayPal/BTPayPalClient.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalClient.swift Outdated Show resolved Hide resolved
UnitTests/BraintreePayPalTests/BTPayPalClient_Tests.swift Outdated Show resolved Hide resolved
switch redirectType {
case .webBrowser(let url), .payPalApp(let url):
let url = URLComponents(url: url, resolvingAgainstBaseURL: true)
if let token = url?.queryItems?.first(where: { $0.name == "token" || $0.name == "ba_token" })?.value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question, is there ever an instance where we'd get both of these params back for the app switch flow but only actually care about 1 of them? Asking because if so we may always want to attempt to parse the ba_token first then if not there move on to the token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! We should prioritize the ba_token over the token if both are available. Since this change also needs to be made on main (+ tests added for the priority), I was thinking it could be a separate PR targeting main first, and then handling the merge conflicts when updating this branch. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I am fine if we want to target that on main instead of here. The merge conflict may be a bit weird since it'll likely be in the section we removed in this PR, but the tests would help us remember.

Comment on lines +719 to +723
if let urlTimestamp = urlComponents?.queryItems?[1].value {
XCTAssertNotNil(Int(urlTimestamp))
} else {
XCTFail("Expected integer value for query param `switch_initiated_time`")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if let urlTimestamp = urlComponents?.queryItems?[1].value {
XCTAssertNotNil(Int(urlTimestamp))
} else {
XCTFail("Expected integer value for query param `switch_initiated_time`")
}
XCTAssertNotNil(Int(urlComponents?.queryItems?[1].value ?? ''))

🧶 : Take or leave to avoid if statement in the test.

#endif

/// The type of PayPal authentication flow to occur
enum PayPalRedirectType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: Would it make sense for this enum to be in it's own file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to leave in here since it's so small, but down to move if folks feel strongly!

@@ -4,7 +4,8 @@ import BraintreeCore
@UIApplicationMain class AppDelegate: UIResponder, UIApplicationDelegate {

private let returnURLScheme = "com.braintreepayments.Demo.payments"
private let universalLinkURL = "https://braintree-ios-demo.fly.dev/braintree-payments"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: Is this the domain that hosts the apple-app-site-association file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Our Demo app now has an HTTPS URL registered for it 🎊 - https://braintree-ios-demo.fly.dev/.well-known/apple-app-site-association

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that's cool!

@scannillo scannillo merged commit e749527 into paypal-app-switch-feature Mar 26, 2024
7 checks passed
@scannillo scannillo deleted the parse-paypalAppApprovalUrl branch March 26, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants