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

Convert Delegation Pattern to Completion Handler #293

Merged
merged 15 commits into from
Oct 29, 2024

Conversation

KunJeongPark
Copy link
Collaborator

@KunJeongPark KunJeongPark commented Oct 22, 2024

Summary of changes

  • convert from returning results via delegate functions to merchants to returning results via completion handlers.
  • provide async await version of public functions

Checklist

  • Added a changelog entry

Authors

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

@KunJeongPark KunJeongPark marked this pull request as ready for review October 23, 2024 20:00
@KunJeongPark KunJeongPark requested a review from a team as a code owner October 23, 2024 20:00
if let error {
self.setUpdateSetupTokenFailureResult(vaultError: error)
} else if let result {
self.approveResultSuccessResult(
Copy link
Collaborator

Choose a reason for hiding this comment

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

qq: should this method be named approveOrderSuccessResult()?

Copy link
Collaborator Author

@KunJeongPark KunJeongPark Oct 23, 2024

Choose a reason for hiding this comment

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

That's a good call. There are many changes I want to make on the demo app, but I stuck to ones relevant to the conversion.
Like needing the loading progressView where I forgot them, to see if there were delays introduced in PayPal vault.

@sshropshire
Copy link
Collaborator

I do see how completion handlers could be useful though. We do have some time in the beta to figure out the best strategy. Ultimately offering both sounds like a viable option to me.

@KunJeongPark
Copy link
Collaborator Author

KunJeongPark commented Oct 24, 2024

I do see how completion handlers could be useful though. We do have some time in the beta to figure out the best strategy. Ultimately offering both sounds like a viable option to me.

  • yes, with both PRs, there will be async/await and completion handler versions of the public functions.
    It's just a matter of which set is wrapped.

await paypalVaultViewModel.vault(setupTokenID: setupTokenID)
ZStack {
Button("Vault PayPal") {
Task {
Copy link
Collaborator

Choose a reason for hiding this comment

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

random q: would it be cleaner if we created the Task within the paypalVaultViewModel.vault() method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Button actions are synchronous contexts, so I need to create a Task block there to call an async/await function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense what you are saying, make the views look cleaner. Let me add that to demo app refactor ticket.

CHANGELOG.md Outdated
* Remove `vault(vaultRequest:)` method that uses delegate callbacks
* Add `approveOrder(request:completion:(CardResult?, Error?) -> Void)` to `CardClient`
* Add `vault(request:completion:(CardVaultResult?, Error?) -> Void)` to `CardClient`
* Add `approveOrder(request:) async throws -> Cardresult`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Add `approveOrder(request:) async throws -> Cardresult`
* Add `approveOrder(request:) async throws -> CardResult`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! Nice catch!

@KunJeongPark KunJeongPark merged commit 6030da7 into v2_beta Oct 29, 2024
5 checks passed
@KunJeongPark KunJeongPark deleted the delegation-to-completion-handler branch October 29, 2024 20:14
KunJeongPark added a commit that referenced this pull request Nov 19, 2024
* Remove MXO (#291)

* Remove MXO

* remove Package.resolved file containing incompatible pinstorage version

* CHANGELOG entry

* Jax feedback and docStrings correction

* Remove PatchOrder for MXO, unused files in demo app

* Remove more classes used exclusively for MXO

* Convert Delegation Pattern to Completion Handler (#293)

* PayPal remove delgation pattern for completion handler function for checkout

* lint error for paypal start

* completion and analytics into notify functions

* paypal vault completion, unit tests

* remove PayPal delegates, references

* Card vault to completion, unit tests

* Card approve to completion, unit tests

* docstrings for approve completion param

* wrap paypal functions in async await

* async await wrapper for CardClient functions

* remove CardDelegate and references

* fix error in notifyCheckoutFailure

* Make error names and messages payPal caps consistent

* changelog entires

* Steven PR feedback: typo in Chagelog

* Simplify Cancel Errors (#296)

* Simplify cancel errors

* CHANGELOG for the cancel errors

* Steven PR feedback: change back CardClientError.canceled to .threeDSecureCanceled

* CHANGELOG update

* Cancel error handling (#297)

* http performRequest returns NetworkingClientErrors

* CardClient helper function for threeDSCancel, demo app cancel, demo app minor fixes

* PayPalClient cancel helper functions and demo app changes

* Steven PR feedback: move static helper functions to error enums

* Rename CardClientError -> CardError, PayPalWebCheckoutError -> PayPalError

* Steven PR feedback: return CoreSDKError in merchant completion handler

* CHANGELOG and analytics typo and fix wrong code in graphql error

* v2 migration guide (#295)

* v2 migration guide

* just cocoapods or SPM

* fix typos

* minor spacing changes

* Update with simplified cancel errors

* Steven PR feedback - diff to render green/red

* include removal of delegate methods in delete block

* update with cardClient threeDSecureCanceled error

* change to threeDSecureCanceled in migration steps

* add comment highlighting cancellation errors

* typo fix

* clarify separating cancel cases in errors

* revert cancel handling instructions

* add changes for cancellation helper methods

* fix typo in PayPalError.isCheckoutCanceled

* Steven PR feedback

* Make PayPalError and CardError Code enum consistent (#298)

* Changelog: move additive changes from breaking section (#299)

* Changelog: move additive changes from breaking section

* remove duplicate line for PayPal cancel errors
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.

3 participants