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

feat: implement auto webhook creation #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thesayyn
Copy link

What kind of change does this PR introduce?

feature

What is the current behavior?

Before these changes, the webhook was created manually which is error-prone and time-consuming.

#4

What is the new behavior?

Webhooks are automatically created and updated if there is any change on the stripe side.

Additional context

Add any other context or screenshots.

@kiwicopple
Copy link
Member

I think you might be the first external contributor here @thesayyn, thanks !

I'll try review this in the next couple of days, but from first skim this looks amazing

@@ -6,10 +6,14 @@ type configType = {
SCHEMA: string
STRIPE_SECRET_KEY: string
STRIPE_WEBHOOK_SECRET: string
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this has been removed from .env.sample

Copy link
Author

@thesayyn thesayyn Jun 22, 2021

Choose a reason for hiding this comment

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

Tests were failing with or without it so I wasn’t sure whether this should be removed.

I could remove it if I could get the tests to pass.

Copy link
Author

Choose a reason for hiding this comment

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

Also:
Users could leave empty the STRIPE_WEBHOOK_URL property and fill this one to opt-out from the automatic creation of the hook.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe i should clarify this in the documentation

Copy link
Member

Choose a reason for hiding this comment

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

The tests pass on my local, but the need a bit of set up:

  • create a database on Supabase
  • grab the Database URL from the settings
  • fill out the .env file:
NODE_ENV=production
STRIPE_SECRET_KEY=sk_test_XXXX
STRIPE_WEBHOOK_SECRET=whsec_XXXX
SCHEMA=stripe
DATABASE_URL=postgres://postgres:XXXX@db.XXX.supabase.co:5432/postgres?sslmode=disable&search_path=stripe
  • Run dbmate up (will create the stripe tables)
  • Run npm run test

I guess I should add a local postgres with docker to make this process a bit simpler

Copy link
Author

Choose a reason for hiding this comment

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

Yes that would make more sense

Copy link
Member

Choose a reason for hiding this comment

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

OK so now that I read through the PR, is there a way that we can mock Stripe so we don't need an account? Previously I was just testing again local copies of Stripe events, and now it looks like it will need a valid STRIPE_SECRET (correct me if I'm wrong)

Copy link
Author

Choose a reason for hiding this comment

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

yes, we can mock stripe if we do not use it directly. we could create helper functions that do the thing we want and we can have a mock version of them instead of a mocked stripe library which is way too complex.

Copy link
Author

Choose a reason for hiding this comment

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

There are ways we could avoid auto-creation of webhook by not supplying the webhook URL. hence the creation will be skipped.

Copy link
Author

Choose a reason for hiding this comment

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

@kiwicopple What do you think?

@thesayyn
Copy link
Author

I think you might be the first external contributor here @thesayyn, thanks !

I'll try review this in the next couple of days, but from first skim this looks amazing

That’s awesome. I just wanted to have fun with this. 🤓

@ak4zh
Copy link

ak4zh commented Aug 9, 2023

Any updates on this?

@rokk4
Copy link

rokk4 commented Aug 21, 2023

That would be amazing! Any updates on that? @thesayyn @kiwicopple Is it just blocked by review?

@kiwicopple
Copy link
Member

@kevcodez - perhaps you can jump on this one when you get a chance

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.

None yet

4 participants