-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feature(payments-plugin): PayPal two-step flow implementation #3099
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
base: minor
Are you sure you want to change the base?
feature(payments-plugin): PayPal two-step flow implementation #3099
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure if most of the maintainers use WebStorm, but this makes it way easier to execute a specific test file in VSCode.
Also had to make some changes to the get-package-dir to make it work not only for the e2e within the payments-plugin.
| }); | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log('Auth Token: ', shopClient.getAuthToken()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This token is printed, so we can enter it in the Web UI of the paypal plugin. I would be open to setting this automatically in the Vendure REST Endpoint, but I wasn't quite sure how.
| } | ||
|
|
||
| // @ts-ignore | ||
| function html(strings, ...values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well. I know we shouldn't integrate code only for syntax highlighting, but as this isn't code of the library itself and only exists for testing purposes, one could argue that it's still okay.
Especially as it doesn't require additional dependencies.
packages/payments-plugin/src/paypal/paypal-authorization.service.ts
Outdated
Show resolved
Hide resolved
packages/payments-plugin/src/paypal/paypal-authorization.service.ts
Outdated
Show resolved
Hide resolved
|
I still have to implement the idempotency headers supported by the PayPal Api, even though it already works without them. This could also be done is a future PR. |
| if (!sessionOrder) { | ||
| throw new IllegalOperationError('Session has no active order'); | ||
| } | ||
| if (sessionOrder.state !== 'ArrangingPayment') { | ||
| throw new IllegalOperationError('Order must be in arranging payment state'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these could be internationalized with existing error message for example there are:
"NO_ACTIVE_ORDER_ERROR": "There is no active Order associated with the current session",
and
"ORDER_MODIFICATION_ERROR": "Order contents may only be modified when in the \"AddingItems\" state",
"ORDER_PAYMENT_STATE_ERROR": "A Payment may only be added when Order is in \"ArrangingPayment\" state",
"ORDER_STATE_TRANSITION_ERROR": "Cannot transition Order from \"{ fromState }\" to \"{ toState }\"",
Might be good to take a second pass over all thrown errors and see what can be internationalized and reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good feeback, thank you :) Will definetly do that!
a46fc50 to
3674cc2
Compare
3674cc2 to
cda6f72
Compare
|
Renamed the commit messages to fit other scopes and rebased on minor branch. |
058e1e3 to
dc6eaeb
Compare
…nt for assertions
94099ff to
05251c8
Compare
|
|
|
||
| return (await response.json()) as PayPalOrderInformation; | ||
| } catch (error: any) { | ||
| const message = 'error.paypal-authorization-details-failed'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though SonarQube already mentioned this code is often duplicated, I want to make sure that errors are correctly handled this way in Vendure. Maybe you guys have some feedback on this :)
|
I've added a small documentation on how to get started using this plugin. Reviews are welcome right now :) |
|
@michaelbromley @sonntag-philipp Hey guys, is this pr outdated? This plugin would be a great complement to Braintree which is not available in many countries. |
|
Hi @sonntag-philipp - apologies for the long time with no response on this. We'd be interested in getting this merged for the next minor release, v3.4. The conflicts look rather minor, would you have a bit of time to get that resolved? If not, let me know and one of us can take that on. |
|
@michaelbromley Oh, I thought the PR was "dismissed" (or something like that) so I moved the PayPal package to my local workspace (without relevant changes) :D I don't have much time until at least Monday, so I could fix the conflicts until Tuesday next week. If you need it more early for the 3.4 minor release, feel free to work on that :) |
|
@sonntag-philipp there's no rush on this. We're looking around the end of June for the 3.4 release. No, the PR was not dismissed - we just dropped the ball on the communication front with all the things we have going on. |
|
Sorry guys, I have to meet a deadline on another project. This PR has to wait some more time. Just the merge would not be the problem, but I think I do not I will be able to respond somewhat fast on issues that occur when this plugin is used. At least until the end of july I will not be able to spend much time on this. |
| }, | ||
| compatibility: '^3.0.0', | ||
| adminApiExtensions: { | ||
| schema: shopSchemaExtensions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the shop and admin schema swapped here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true! good catch
|
@sonntag-philipp I would really like to get this PR merged as it is a great improvement. Do you know if you can spend some more time to finish this? Please let me know. |
|
Any news on this topic? It would be really good to see a Paypal Integration in Vendure. |


Description
This PR introduces a PayPal payments plugin based on the PayPal Orders Api with a 2 step flow as specified in the Vendure documentation.
The main purpose of this plugin is to offer the user a solution where PayPal can be used directly without a step in between.
Documentation will be added by me before this plugin is merged. Just want to make sure I don't have to change it after review feedback of the implementation.
Breaking changes
I am not able to determine a breaking change. The implemented features represent new functionality and should have no effect on existing implementations.
Overview
Screenshots
Checklist
📌 Always:
👍 Most of the time: