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

Add BTPayPalEditRequest and Tokenize for Edit FI #1358

Merged
merged 9 commits into from
Jul 11, 2024
Merged

Add BTPayPalEditRequest and Tokenize for Edit FI #1358

merged 9 commits into from
Jul 11, 2024

Conversation

KunJeongPark
Copy link
Contributor

@KunJeongPark KunJeongPark commented Jul 8, 2024 •
edited
Loading

Summary of changes

  • Create PayPalEditRequest that takes in a String token
  • Add tokenize function that takes in PayPalEditRequest and returns BTPayPalAccountNonce or Error

Checklist

  • Added a changelog entry

Authors

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

@KunJeongPark KunJeongPark changed the title Add BTPayPalEditRequest Add BTPayPalEditRequest and Tokenize for Edit FI Jul 8, 2024
@KunJeongPark KunJeongPark marked this pull request as ready for review July 8, 2024 20:04
@KunJeongPark KunJeongPark requested a review from a team as a code owner July 8, 2024 20:04
jaxdesmarais
jaxdesmarais reviewed Jul 9, 2024
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.

I think we can at least mock out a few tests and in the implementation just add a TODO to implement in a future PR. For the BTPayPalEditRequest we should be able to already add that test.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalClient.swift Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalEditRequest.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalEditRequest.swift Outdated Show resolved Hide resolved
@KunJeongPark
Copy link
Contributor Author

KunJeongPark commented Jul 9, 2024 •
edited
Loading

I think we can at least mock out a few tests and in the implementation just add a TODO to implement in a future PR. For the BTPayPalEditRequest we should be able to already add that test.

I'm working on the test function shells and one for BTPayPalEditRequest now.

scannillo
scannillo reviewed Jul 10, 2024
View reviewed changes
Sources/BraintreePayPal/BTPayPalClient.swift
/// - request: A `BTPayPalEditRequest`
/// - completion: This completion will be invoked exactly once when tokenization is complete or an error occurs.
/// - Warning: This feature is currently in beta and may change or be removed in future releases.
public func tokenize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to Jax's comment re: only supporting new features in Swift, what do folks think about only supporting new features in the async-await method syntax? We could hold on the completion style syntax and add it later based on merchant feedback? Is now a good time to trial this?

Would love folks thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

People I've talked to are working with async-await syntax in their code bases, but that's just a small sampling. I'm sure there are a lot of companies with legacy repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a great point - i wonder how many merchants are transitioning to or already using the async/await style over the completion style syntax. imo i like that having both signatures gives merchants the choice to choose what works best for them but i also feel the asyn/await reads more clearly

Copy link
Contributor

@scannillo scannillo Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed! I like the brevity of the async-await style lately too

I think V7 will be a good chance to yank the completion syntax 👀

scannillo
scannillo reviewed Jul 11, 2024
View reviewed changes
CHANGELOG.md Outdated
* BraintreePayPal
* Add PayPal edit funding instrument flow (BETA)
* Add `BTPayPalEditRequest` for edit FI flow
* Add `tokenizetokenize(_:completion:)` method that takes in a `BTPayPalEditRequest`
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
* Add `tokenizetokenize(_:completion:)` method that takes in a `BTPayPalEditRequest`
* Add `BTPayPalClient.tokenize(_:completion:)` method that takes in a `BTPayPalEditRequest`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thank you

agedd
agedd reviewed Jul 11, 2024
View reviewed changes
CHANGELOG.md Outdated
@@ -1,5 +1,12 @@
# Braintree iOS SDK Release Notes

## unreleased
* BraintreePayPal
* Add PayPal edit funding instrument flow (BETA)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (take it or leave it)

Suggested change
* Add PayPal edit funding instrument flow (BETA)
* Add PayPal Edit Funding Instrument Flow (BETA)

agedd
agedd approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@agedd agedd left a comment

Choose a reason for hiding this comment

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

lgtm! 🚀 just left a few small comments

@KunJeongPark KunJeongPark merged commit 9c4a62e into edit-fi-feature Jul 11, 2024
6 of 7 checks passed
@KunJeongPark KunJeongPark deleted the edit-fi-request-tokenize branch July 11, 2024 21:02
jaxdesmarais
jaxdesmarais reviewed Jul 11, 2024
View reviewed changes
Braintree.xcodeproj/project.pbxproj
@@ -1380,6 +1384,7 @@
BE6BC22B2BA9C67600C3E321 /* BTPayPalVaultBaseRequest.swift */,
BE349110294B77E100D2CF68 /* BTPayPalVaultRequest.swift */,
62A659A32B98CB23008DFD67 /* PrivacyInfo.xcprivacy */,
3B5CD9AF2C3C3FCC002EE8F7 /* BTPayPalEditRequest.swift */,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure these files are alphabetized?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaxdesmarais jaxdesmarais jaxdesmarais left review comments

@agedd agedd agedd approved these changes

@scannillo scannillo scannillo 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.
4 participants
@KunJeongPark @jaxdesmarais @scannillo @agedd