Feat/channel aware stripe #2058

martijnvdbrug · 2023-02-28T10:12:52Z

This PR implements config for Stripe per channel: A user can set ApiKey and webhooks per channel via the Admin UI.

It would be nice to encrypt the ApiKey based on an env var, before storing it in the DB, but I can't find a way to hook into the saving of handler args.

The PR also includes a Stripe dev-server to test out changes during development:

E2e tests are still missing the webhook mocking, but that's not part of this PR. I've manually verified payment flow with the dev-server and local tunnel:

(admin UI looks nice!)

martijnvdbrug · 2023-03-03T09:38:41Z

packages/admin-ui/src/lib/core/src/common/introspection-result.ts

-        ],
-        UpdatePromotionResult: ['MissingConditionsError', 'Promotion'],
-    },
+      export interface PossibleTypesResultData {


I regenerated types. How to prettify everything?

There should be a pre-commit hook that applies Prettier to all changed files. Is that not working?

Nope, yarn husky install gives me error Command "husky" not found.. I see the dependency. But this is something for later. I will try to manually lint or prettier

NVM, it is working now, not sure why. I did get an Out Of Memory so I had to change the command to NODE_OPTIONS=\"--max-old-space-size=8096\" lint-staged

michaelbromley · 2023-03-06T11:59:57Z

Thanks for this @martijnvdbrug! I'll review it soon when I get some time to dedicate to this.

In the mean time I'll invite @vrosa to see if he has any feedback, as the original author of the Stripe integration.

michaelbromley · 2023-03-06T16:19:28Z

By the way, I just pushed a lot of changes to the major branch, including all the TypeORM breaking changes. I'm afraid you'll need to do a bit of extra work to integrate these changes.

I made some notes on the main changes in the TypeORM APIs:

Ref: https://typeorm.io/changelog#030httpsgithubcomtypeormtypeormpull8616-2022-03-17

TypeORM upgrade requires Node.JS version 14+
changing where null to use IsNull()
changing return types to null rather than undefined
change findOne(id) to findOne({ where: { id } })
change where: { order: myOrder } to where: { order: { id: myOrder.id }}
change findInIds(ids) to find({ where: { id: In(ids) } })

vrosa · 2023-03-12T22:19:43Z

Thanks for this @martijnvdbrug! I'll review it soon when I get some time to dedicate to this.

In the mean time I'll invite @vrosa to see if he has any feedback, as the original author of the Stripe integration.

Thanks for the heads up, I'll have a closer look later this week if that's ok.

martijnvdbrug · 2023-03-13T15:26:24Z

@michaelbromley Not sure why the generated types changed in core. Looks like formatting. How can I make sure formatting is up to date?

If I run yarn format . in the root of the project it changes a lot of files that are not part of this PR. Also, didn't formatting used to be automatic on commit or push?

martijnvdbrug · 2023-03-23T18:38:33Z

@michaelbromley Is the major development ready? I can't build the major branch locally, which results in not being able to build the payments-plugin, because of type mismatches (DocumentNode, probably because of version mismatches)

michaelbromley · 2023-03-24T07:45:14Z

@martijnvdbrug You're right, it was in a broken state after a recent merge from master. Should be working now hopefully!

martijnvdbrug · 2023-03-24T09:18:30Z

package.json

@@ -87,7 +87,7 @@
    "hooks": {
      "commit-msg": "commitlint -e $HUSKY_GIT_PARAMS",
      "post-commit": "git update-index --again",
-      "pre-commit": "lint-staged",
+      "pre-commit": "NODE_OPTIONS=\"--max-old-space-size=8096\" lint-staged",


Not sure if this is an Ubuntu specific issue, but the OOM also happens in my own repo's with eslint, and this is how I fixed it. https://stackoverflow.com/questions/53230823/fatal-error-ineffective-mark-compacts-near-heap-limit-allocation-failed-javas

martijnvdbrug · 2023-03-24T09:34:41Z

Strange, e2e tests here are also failing with `Error: Cannot find module '@ardatan/aggregate-error'
Require stack:

/home/martijn/git/vendure/node_modules/@nestjs/graphql/node_modules/@graphql-tools/utils/index.cjs.js`
Is it caching node_modules somehow? Yarn.lock is not part of this PR

michaelbromley · 2023-03-24T11:16:20Z

Not sure why, maybe caching? Anyway, I found a few more issues with e2e tests and just pushed some changes to major.

vrosa · 2023-03-24T23:24:00Z

packages/payments-plugin/src/stripe/stripe.controller.ts

        const paymentIntent = event.data.object as Stripe.PaymentIntent;
+        const { metadata: { channelToken, orderCode, orderId } = {} } = paymentIntent;


Destructuring here is going to throw an error in case paymentIntent is null or undefined. These new lines should be moved to after the if (!paymentIntent) check, since retrieving the order depends on a valid payment intent in the first place:

const paymentIntent = event.data.object as Stripe.PaymentIntent; if (!paymentIntent) { Logger.error(noPaymentIntentErrorMessage, loggerCtx); response.status(HttpStatus.BAD_REQUEST).send(noPaymentIntentErrorMessage); return; } const { metadata: { channelToken, orderCode, orderId } = {} } = paymentIntent; // more code

Good point. fixed

vrosa · 2023-03-24T23:27:51Z

packages/payments-plugin/src/stripe/stripe.controller.ts

-        const order = await this.orderService.findOneByCode(ctx, orderCode);
-        if (!order) {
-            throw Error(`Unable to find order ${orderCode}, unable to settle payment ${paymentIntent.id}!`);
+        try {


I reckon it makes the most sense to have this check right after an order is retrieved to ensure we're processing an actual message from Stripe before proceeding any further:

if (!order) { throw Error(`Unable to find order ${orderCode}, unable to settle payment ${paymentIntent.id}!`); } try { event = this.stripeService.constructEventFromPayload(request.rawBody, signature); } // more code

Also valid feedback. Fixed

vrosa · 2023-03-24T23:56:44Z

packages/payments-plugin/src/stripe/stripe.handler.ts

@@ -50,39 +72,47 @@ export const stripePaymentMethodHandler = new PaymentMethodHandler({
    },

    async createRefund(ctx, input, amount, order, payment, args): Promise<CreateRefundResult> {
-        const result = await stripeService.createRefund(payment.transactionId, amount);


Out of interest, why did you remove createRefund from StripeService and moved the code here? The idea was to encapsulate interactions with the Stripe object inside the service so the handler doesn't need to know about the Stripe API, e.g. stripe.refunds.create.

Why I did it: simplicity, the function was just proxying to the stripe client.

I have changed it back, encapsulation the stripe client inside the service is also a good point.

vrosa · 2023-03-25T00:22:03Z

packages/payments-plugin/src/stripe/stripe-client.ts

+export class VendureStripeClient extends Stripe {
+
+    constructor(
+        public apiKey: string,


It's a minor thing but from what I could tell apiKey doesn't need to be public as it's not referenced anywhere unlike webhookSecret here.

You're right, it can be private.

martijnvdbrug · 2023-03-29T07:36:26Z

@vrosa @michaelbromley thanks for the thorough feedback on this PR. I've incorporated all feedback and updated with the latest Major branch again.

(Still the same errors in e2e, that seem unrelated to these changes)

yasserlens · 2023-04-14T02:54:43Z

I'd very much appreciate having an update on where this is at the moment - I eagerly need it to launch a new store on the same vendure instance that already serves another store. Thank you for your work on this! 🙏

michaelbromley · 2023-04-19T13:04:13Z

Hi everyone. Apologies for the wait - I had quite a lot on my plate preparing the v2 beta release.

@martijnvdbrug thanks for this incredible effort and also @vrosa for your thorough review.

This is ready to merge, but if would be good in Martijn can provide a paragraph on the breaking change that I can use in the merge commit, which will subsequently appear in the changelog.

Also if you feel it would be helpful, you can provide an expanded explanation which I can then put in the migration guide for v1->v2.

martijnvdbrug · 2023-04-21T11:10:01Z

@michaelbromley Breaking change docs:

Updating Stripe from `v1.x` to `v2`

The Stripe plugin has been made channel aware. This means your api key and webhook secret are now stored in the database, per channel, instead of environment variables.

To migrate to v2 of the Stripe plugin from @vendure/payments you need to:

Remove the apiKey and webhookSigningSecret from the plugin initialization in vendure-config.ts:

-StripePlugin.init({
-    apiKey: process.env.YOUR_STRIPE_SECRET_KEY,
-    webhookSigningSecret: process.env.YOUR_STRIPE_WEBHOOK_SIGNING_SECRET,
-    storeCustomersInStripe: true,
-}),
+StripePlugin.init({
+    storeCustomersInStripe: true,
+ }),

Start the server and login as administrator.
For each channel that you'd like to use Stripe payments, you need to create a payment method with payment handler Stripe payment and the apiKey and webhookSigningSecret belonging to that channel's Stripe account.

michaelbromley · 2023-04-21T11:15:34Z

Thanks @martijnvdbrug!

This will be in 2.0.0-beta.2, expected next week 🥳

martijnvdbrug added 3 commits February 28, 2023 11:10

feat(payment-plugin): channel aware stripe methods WIP

00339a2

feat(payment-plugin): untracked files

892bc04

Merge branch 'major' into feat/channel-aware-stripe

f32588a

martijnvdbrug changed the base branch from master to major March 3, 2023 07:17

martijnvdbrug added 2 commits March 3, 2023 08:25

feat: wip

285d1ed

feat(payment-plugin): channel aware Stripe handler

e99fb87

martijnvdbrug commented Mar 3, 2023

View reviewed changes

feat(payment-plugin): fixme comment

813ec44

martijnvdbrug marked this pull request as ready for review March 3, 2023 09:42

martijnvdbrug closed this Mar 8, 2023

martijnvdbrug reopened this Mar 8, 2023

martijnvdbrug added 2 commits March 13, 2023 16:20

feat(payments-plugin): mreged major

dffd44c

feat(payments-plugin): reverted type fix

7f4e148

martijnvdbrug requested a review from michaelbromley March 13, 2023 15:26

martijnvdbrug added 4 commits March 13, 2023 16:30

feat(payments-plugin): docs

6fa4a42

feat(payments-plugin): docs

4e5239e

feat(payments-plugin): less nesting

e3c0af4

feat(payment-plugin): merged major

d37e58b

martijnvdbrug added 3 commits March 24, 2023 09:34

Merge branch 'major' into feat/channel-aware-stripe

56a8cf3

feat(payment-plugin): Updated with major branch

6c0914b

feat(payment-plugin): Restored generated files

916ee8f

martijnvdbrug commented Mar 24, 2023

View reviewed changes

vrosa reviewed Mar 25, 2023

View reviewed changes

martijnvdbrug added 3 commits March 29, 2023 09:04

Merge branch 'major' into feat/channel-aware-stripe

617cdc5

feat(payment-plugin): Pr fixes

25d8b21

feat(payment-plugin): Pr fix

7d82dd5

martijnvdbrug requested review from vrosa and michaelbromley and removed request for michaelbromley and vrosa March 29, 2023 07:36

michaelbromley added 2 commits April 19, 2023 14:22

chore(job-queue-plugin): Fix lint issues

730501d

Merge branch 'major' into feat/channel-aware-stripe

61dbf75

michaelbromley merged commit 3b88702 into vendure-ecommerce:major Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/channel aware stripe #2058

Feat/channel aware stripe #2058

martijnvdbrug commented Feb 28, 2023 •

edited

Loading

martijnvdbrug Mar 3, 2023

michaelbromley Mar 6, 2023

martijnvdbrug Mar 24, 2023

martijnvdbrug Mar 24, 2023

michaelbromley commented Mar 6, 2023

michaelbromley commented Mar 6, 2023

vrosa commented Mar 12, 2023

martijnvdbrug commented Mar 13, 2023

martijnvdbrug commented Mar 23, 2023

michaelbromley commented Mar 24, 2023

martijnvdbrug Mar 24, 2023

martijnvdbrug commented Mar 24, 2023

michaelbromley commented Mar 24, 2023

vrosa Mar 24, 2023

martijnvdbrug Mar 29, 2023

vrosa Mar 24, 2023

martijnvdbrug Mar 29, 2023

vrosa Mar 24, 2023

martijnvdbrug Mar 29, 2023

vrosa Mar 25, 2023

martijnvdbrug Mar 29, 2023

martijnvdbrug commented Mar 29, 2023 •

edited

Loading

yasserlens commented Apr 14, 2023

michaelbromley commented Apr 19, 2023

martijnvdbrug commented Apr 21, 2023

michaelbromley commented Apr 21, 2023

		const paymentIntent = event.data.object as Stripe.PaymentIntent;
		const { metadata: { channelToken, orderCode, orderId } = {} } = paymentIntent;

Feat/channel aware stripe #2058

Feat/channel aware stripe #2058

Conversation

martijnvdbrug commented Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbromley commented Mar 6, 2023

michaelbromley commented Mar 6, 2023

vrosa commented Mar 12, 2023

martijnvdbrug commented Mar 13, 2023

martijnvdbrug commented Mar 23, 2023

michaelbromley commented Mar 24, 2023

Choose a reason for hiding this comment

martijnvdbrug commented Mar 24, 2023

michaelbromley commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvdbrug commented Mar 29, 2023 • edited Loading

yasserlens commented Apr 14, 2023

michaelbromley commented Apr 19, 2023

martijnvdbrug commented Apr 21, 2023

Updating Stripe from v1.x to v2

michaelbromley commented Apr 21, 2023

martijnvdbrug commented Feb 28, 2023 •

edited

Loading

martijnvdbrug commented Mar 29, 2023 •

edited

Loading

Updating Stripe from `v1.x` to `v2`