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.

Sign up for GitHub

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

Jump to bottom

Merchant Passed Email addition to Native Checkout #1118

Merged
merged 12 commits into from
Nov 16, 2023
Merged

Merchant Passed Email addition to Native Checkout #1118

merged 12 commits into from
Nov 16, 2023

Conversation

mariolopez-pypl
Copy link
Contributor

@mariolopez-pypl mariolopez-pypl commented Oct 24, 2023 •
edited
Loading

Thank you for your contribution to Braintree.

Before submitting this PR, note we cannot accept language translation PRs. We support the same languages that are supported by PayPal, and have a dedicated localization team to provide the translations. If there is an error in a specific translation, you may open an issue and we will escalate it to the localization team.

Summary of changes

Allow merchant to pass a user's email to use PayPalCheckout's feature to more quickly authenticate the user.

Checklist

  • Added a changelog entry

@mariolopez-pypl mariolopez-pypl requested a review from a team as a code owner October 24, 2023 00:10
jaxdesmarais
jaxdesmarais reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

Does this require a MXO release to be merged in? If so we should add a do not merge tag. Have we also confirmed this works as expected for the standard PayPal web flow since it's also being added to the BTPayPalVaultRequest? Additionally we will want to add tests for this new parameter.

Sources/BraintreePayPalNativeCheckout/BTPayPalNativeCheckoutRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPalNativeCheckout/BTPayPalNativeCheckoutClient.swift Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalVaultRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalVaultRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPalNativeCheckout/BTPayPalNativeVaultRequest.swift Outdated Show resolved Hide resolved
@mariolopez-pypl
Copy link
Contributor Author

In regards to your adding tests comments, should we add new tests in BTPayPalNativeCheckoutClient_Tests if so how do you think we should test this in there?

scannillo
scannillo reviewed Oct 25, 2023
View reviewed changes
Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift Outdated
@@ -82,6 +82,9 @@ import BraintreeCore
/// Optional: If set to `true`, this enables the Checkout with Vault flow, where the customer will be prompted to consent to a billing agreement during checkout. Defaults to `false`.
public var requestBillingAgreement: Bool

/// Optional: User email to initiate a quicker authentication flow in cases where the user has a PayPal Account with the same email.
public var userAuthenticationEmail: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Take it or leave it comment - I would call these just userEmail

@scannillo scannillo mentioned this pull request Oct 25, 2023
Closed
1 task
scannillo
scannillo reviewed Oct 25, 2023
View reviewed changes
Sources/BraintreePayPal/BTPayPalCheckoutRequest.swift Outdated
@@ -82,6 +82,9 @@ import BraintreeCore
/// Optional: If set to `true`, this enables the Checkout with Vault flow, where the customer will be prompted to consent to a billing agreement during checkout. Defaults to `false`.
public var requestBillingAgreement: Bool

/// Optional: User email to initiate a quicker authentication flow in cases where the user has a PayPal Account with the same email.
Copy link
Contributor

Choose a reason for hiding this comment

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

Linking this comment from the Android PR to align docstrings across platforms. I think the one written here is a bit more descriptive!

@scannillo
Copy link
Contributor

In regards to your adding tests comments, should we add new tests in BTPayPalNativeCheckoutClient_Tests if so how do you think we should test this in there?

We have a big TODO in our test file for that class, because testing MXO requires a hefty amount of mocking/protocol wrapping since all of the classes are final. Maybe in the future this is something we could work with your team on trying to simplify from the PPCP/BT side!

For this PR, you can ignore adding tests to BTPayPalNativeCheckoutClient_Tests since the testing infrastructure isn't set up yet.

@jaxdesmarais
Copy link
Contributor

➕ 1 to what Sammy said about Client tests - I mainly meant adding Request tests which it looks like were added

@mariolopez-pypl
Copy link
Contributor Author

Great, right now we are still in the process of conducting a new releases on the MXO side, I'll update here once those changes are released.

jaxdesmarais
jaxdesmarais reviewed Nov 15, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalVaultRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPalNativeCheckout/BTPayPalNativeVaultRequest.swift Outdated Show resolved Hide resolved
jaxdesmarais
jaxdesmarais approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

🚀

@scannillo scannillo merged commit 29c97d3 into braintree:main Nov 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scannillo scannillo scannillo approved these changes

@jaxdesmarais jaxdesmarais jaxdesmarais approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mariolopez-pypl @scannillo @jaxdesmarais