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

App installed check #1233

Merged
merged 17 commits into from
Apr 4, 2024
Merged

Conversation

stechiu
Copy link
Collaborator

@stechiu stechiu commented Mar 22, 2024

Summary of changes

We want to check if the PayPal app is installed. This branch will be merged into paypal-app-switch-feature. This PR doesn't cover the actual app switching to the PayPal app. To test, change the url scheme to maps://

  • Add appInstalled check if enablePayPalAppSwitch=true, fallback to existing tokenize() flow if not

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@stechiu stechiu changed the base branch from main to paypal-app-switch-feature March 22, 2024 19:00
@@ -83,7 +87,12 @@ import BraintreeDataCollector
_ request: BTPayPalVaultRequest,
completion: @escaping (BTPayPalAccountNonce?, Error?) -> Void
) {
tokenize(request: request, completion: completion)
// Check if PayPal app is installed
if request.enablePayPalAppSwitch == true {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to diverge in the flow just yet. We still need to get the response back from this API call which will either contain the Venice URL, or it won't. Based on if the URL present, we will or won't proceed with app switch flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where we instead want to do the appInstalled check. If that check fails, we don't want to bother encoding these additional parameters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I've updated the PR with the changes

@stechiu stechiu marked this pull request as ready for review March 25, 2024 23:26
@stechiu stechiu requested a review from a team as a code owner March 25, 2024 23:26
@@ -5,22 +5,22 @@ concurrency:
cancel-in-progress: true
jobs:
cocoapods:
name: CocoaPods (Xcode 14.3)
name: CocoaPods (Xcode 15.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the base branch needs to be updated for this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's strange... I'll update the app-installed-check branch with the main and see if that helps

…ree/braintree_ios into app-installed-check

# Conflicts:
#	CHANGELOG.md
#	Sources/BraintreePayPal/BTPayPalClient.swift
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -728,6 +728,27 @@ class BTPayPalClient_Tests: XCTestCase {
}


func testIsiOSAppSwitchAvailable_whenApplicationCanOpenPayPalInAppURL_returnsTrue() {
Copy link
Contributor

@scannillo scannillo Apr 1, 2024

Choose a reason for hiding this comment

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

Instead of asserting that the function isPayPalAppInstalled returns some boolean, we rather want our unit tests to assert on the component's public API (in this case BTPayPalClient). Implementation details (such as if the app installed check lives in a method or not) should be able to change without breaking our unit tests.

So this test should assert on the end result behavior, which is that the API POST request to /v1/paypal_hermes/create_payment_resource does include the app switch params if the app is installed, and does not include the app switch params if the app is not installed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I've made changes to test the endpoints

stechiu and others added 12 commits April 2, 2024 11:14
* fix urlString comparison to behave as expected
* remove unneeded cannedErrorResponse causing tokenize to return before expected
…aintree_ios into app-installed-check

# Conflicts:
#	UnitTests/BraintreePayPalTests/BTPayPalClient_Tests.swift
# Conflicts:
#	Sources/BraintreePayPal/BTPayPalClient.swift
…aintree_ios into app-installed-check

# Conflicts:
#	Sources/BraintreePayPal/BTPayPalClient.swift
self.payPalAppInstalled = self.isPayPalAppInstalled()

if !self.payPalAppInstalled {
(request as? BTPayPalVaultRequest)?.enablePayPalAppSwitch = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@jaxdesmarais jaxdesmarais merged commit 75a084f into paypal-app-switch-feature Apr 4, 2024
5 of 7 checks passed
@jaxdesmarais jaxdesmarais deleted the app-installed-check branch April 4, 2024 16:14
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.

5 participants