-
Notifications
You must be signed in to change notification settings - Fork 295
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
[QL] Conform BTPayPalClient
to BTAppContextSwitchClient
Protocol
#1218
Changes from 8 commits
c6dddfa
8bfb2bf
afe1923
f69edbb
666324a
3fd897b
62b57bc
bb8a1f7
ef40241
9efbff2
49879e4
adfb1df
14a35df
429533e
a6aabdd
4b39541
2d53f54
d4e6e3e
ff61d88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,12 @@ import BraintreeDataCollector | |
/// Exposed for testing, the ASWebAuthenticationSession instance used for the PayPal flow | ||
var webAuthenticationSession: BTWebAuthenticationSession | ||
|
||
// MARK: - Static Properties | ||
|
||
/// This static instance of `BTPayPalClient` is used during the app switch process. | ||
/// We require a static reference of the client to call `handleReturnURL` and return to the app. | ||
static var payPalClient: BTPayPalClient? = nil | ||
|
||
// MARK: - Private Properties | ||
|
||
/// Indicates if the user returned back to the merchant app from the `BTWebAuthenticationSession` | ||
|
@@ -221,6 +227,12 @@ import BraintreeDataCollector | |
performSwitchRequest(appSwitchURL: url, paymentType: paymentType, completion: completion) | ||
} | ||
|
||
// MARK: - App Switch Methods | ||
|
||
func handleOpen(_ url: URL) { | ||
// TODO: implement handling return URL in a follow up PR | ||
} | ||
|
||
// MARK: - Private Methods | ||
|
||
private func tokenize( | ||
|
@@ -431,3 +443,18 @@ import BraintreeDataCollector | |
completion(nil, BTPayPalError.canceled) | ||
} | ||
} | ||
|
||
extension BTPayPalClient: BTAppContextSwitchClient { | ||
/// :nodoc: | ||
@_documentation(visibility: private) | ||
@objc public static func handleReturnURL(_ url: URL) { | ||
payPalClient?.handleOpen(url) | ||
BTPayPalClient.payPalClient = nil | ||
} | ||
|
||
/// :nodoc: | ||
@_documentation(visibility: private) | ||
@objc public static func canHandleReturnURL(_ url: URL) -> Bool { | ||
url.scheme == "https" && (url.path.contains("cancel") || url.path.contains("success")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the current contracts I don't believe we will ever get an error path back. In the future if we do we can consider adding it here. We can also consider adding a check here that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There won't be an error specific scenario in the URL, no. Though we will want to surface the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🤔 how would we both surface the cancel as an error and continue the flow? If we surfaced it in our completion I would expect the execution of that path would also end? Or do we want to diverge from that pattern and latter in the flow return both a nonce and an error?
There is a TODO in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes! Like we do today when we get back There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes since, sorry I read that wrong at first as we want to continue the flow on cancel, but that portion was for success. 🙈 Do we think it'd be helpful to also validate that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question - maybe we can assume that if the switch successfully made it into the merchant app, that the host of the URL is fine. I vote for not doing that check, for now. But don't feel strongly if you have other thoughts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with that too, we can always add it in a future PR if we decide we need additional validation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thought - might be more readable to keep this similar to Venmo which has a separate class ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm down, updated here (I opted to call it |
||
} | ||
} |
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 know this is the naming we use in Venmo, but it's a little confusing to me since without looking through the code callsites you don't know if this method is for opening the Venmo app, or opening the merchant app.
Take it or leave it comment - but we could rename to
handleReturnURL()
or something similarThere 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.
That makes sense to me too: 2d53f54