[QL] Consolidate Return URL Logic #1271
Conversation
| /// This class interprets URLs received from the PayPal app via app switch returns. | ||
| /// | ||
| /// PayPal app switch authorization requests should result in success or user-initiated cancelation. These states are communicated in the url. | ||
| struct BTPayPalReturnURL { |
There was a problem hiding this comment.
This file was renamed but is showing additional diff. Adding logic for the web flow in the init as well as adding isValidURLAction plus action methods here are the actual changes.
| /// If we are using the deeplink/ASWeb based PayPal flow we want to check that the host and path matches | ||
| /// the static callbackURLHostAndPath. For the universal link flow we do not care about this check. | ||
| if hostAndPath != BTPayPalRequest.callbackURLHostAndPath && linkType == "deeplink" { |
There was a problem hiding this comment.
@agedd I know you mentioned this logic for this specific if block being confusing before. I added the docstrings as well as passing/checking the linkType here as deeplink instead of passing in the request and doing some weird stuff with that. Please let me know if this makes it clearer what this check is doing!
There was a problem hiding this comment.
this looks great! @jaxdesmarais imo definitely a lot more clearer to read :)
| import XCTest | ||
| @testable import BraintreePayPal | ||
|
|
||
| final class BTPayPalReturnURL_Tests: XCTestCase { |
There was a problem hiding this comment.
Most of this test class is not new, was just renamed. The testInitWithSchemeURL are the only new tests since we are passing both scheme based URLs and Universal Link URLs into the init now.
| notifyFailure(with: BTPayPalError.invalidURLAction, completion: completion) | ||
| return | ||
| } | ||
|
|
||
| guard let response = responseDictionary(from: url) else { |
There was a problem hiding this comment.
The responseDictionary method was a bit odd because it was both building the dictionary to pass into the post parameters as well as doing a check that the URL did not contain cancel. The only part that used the return URL was the cancel check so I broke this out to build the dictionary here which made more sense.
| enum BTPayPalReturnURLState { | ||
| case unknownPath | ||
| case succeeded | ||
| case canceled |
There was a problem hiding this comment.
small nit:
| case canceled | |
| case cancelled |
There was a problem hiding this comment.
Our SDKs always use the 1 L canceled/cancelation
| init?(body: BTJSON) { | ||
| if let payPalAppRedirectURL = body["agreementSetup"]["paypalAppApprovalUrl"].asURL() { | ||
| init?(body: BTJSON, linkType: String?) { | ||
| if linkType == "universal", let payPalAppRedirectURL = body["agreementSetup"]["paypalAppApprovalUrl"].asURL() { |
There was a problem hiding this comment.
Nit for another time - maybe another indicator we should move this to an enum
There was a problem hiding this comment.
Actually, I don't think the BTPayPalApprovalURLParser class should care if the merchant has opted into universal links or not. It's only determining which URL type it got back from the GW response.
The flow logic for falling back to the web flow already is taken care of in BTPayPalClient.
There was a problem hiding this comment.
As of Thursday, we were still getting the paypalAppApprovalUrl from the Gateway regardless of the BTPayPalClient. This may have been because we were working with a mock and an update may have been deployed to just allow our client side handling. Worth digging into but I was still getting this URL with the PPBeta app uninstalled.
Summary of changes
BTPayPalAppSwitchReturnURLtoBTPayPalReturnURLsince this is now used for both app switch and web based flowsBTPayPalAppSwitchReturnURL_TeststoBTPayPalReturnURL_TestsChecklist
[ ] Added a changelog entryAuthors