-
-
Notifications
You must be signed in to change notification settings - Fork 92
feat: added checkout session #172
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
Conversation
|
We also need this feature, can someone watch this PR? |
|
This feature is something we urgently need on our side as well. |
kevcodez
left a comment
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.
Thanks for the contribution!
Can you please
- Add the webhooks to the README as supported
- Clarify whether line items are present in webhooks
- Add an appropriate webhook test
| ) | ||
|
|
||
| // Upsert line items into separate table | ||
| const lineItemsPromises = checkoutSessions |
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 should be a single upsert and not one upsert per line item ideally
Are line items sent via the webhook by default? Otherwise the checkout session line items would never be there for a regular webhook
|
Thank you for the Review. I inspected a checkout session event and indeed there are no line items, so either I would've to fetch them via the api or ignore them and only sync checkout sessions. What do you think? For my use-case it would be really nice to have the line items directly when syncing |
|
@Niki2k1 yeah makes sense, i think a check session without persisting the line items themselves would not add too much value. I think it would be best to call So
|
|
@Niki2k1 Hi, are you going to refine this PR? |
|
@jackkru69 yes, I will implement the changes and update the pr, but at earliest next week. Can't promise anything though. |
|
I updated the PR and added/changed the following items:
|
feat: handle custom checkout.session tableName feat: added syncTimestamp logic for checkout sessions
|
I merged main and resolved some conflicts and also added the new lastSyncedAt logic |
|
@kevcodez Could you check it, pls? |
|
Hi @Niki2k1 , thanks for your very useful contribution! I have just tried a container image built from your feature branch, with database containing products of backfill by mainline codebase. I also retried from scratch. I use backfill with |
|
@andrey-utkin Awesome! Thank you so much for taking the time to test this. |
|
I guess I am not even sure what this primary key specification means: Indexes/keys end up this way: |
| @@ -0,0 +1,26 @@ | |||
| create table if not exists "stripe"."checkout_session_line_items" ( | |||
| "id" text, | |||
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.
These should be unique and therefore this should have a primary key to make the upsert work
|
Given checkout line item has a unique id, was there a reason not marking this as primary key in the migration? This is leading to the insert failing Also, can you please add an integration test that fully covers this including the upsert of the line items? We should catch an error like this during an integration test and not a manual one ideally |
…primary key on id
|
I replaced the composite key with a primary key just on the id field. |
fix: listLineItems mock fix: lineItems migration and schema to include correct fields
|
I added a test (was a good idea because I found multiple issues like missing fields) I also added |
kevcodez
left a comment
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.
Overall LGTM with the tests, two small things I noticed that would have to be addressed so we can merge this 👍
| return this.stripe.refunds.retrieve(stripeId).then((it) => this.upsertRefunds([it])) | ||
| } else if (stripeId.startsWith('cs_')) { | ||
| return this.stripe.checkout.sessions | ||
| .retrieve(stripeId) |
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.
Missing { expand: ['line_items'] } 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.
The line_items are getting filled in the upsert itself via their own endpoint. They are not available in the /v1/checkout/sessions/:id endpoint.
correct me if I am wrong
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.
Aren't you doing the expansion in here too? https://github.com/supabase/stripe-sync-engine/pull/172/files#diff-95fc8d6564ba293c6ce4a37ea33f73126fab928a0311b74c1a56b5b2d19d0bdeR122
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 will remove it there then as well 😅
was left over from when I started.
I moved over to the fillCheckoutSessionsLineItems which uses the /v1/checkout/sessions/:id/line_items endpoint
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.
Will also remove it here: https://github.com/supabase/stripe-sync-engine/pull/172/files/380d7b6106ac8b77421c7c61533afed6ae2255e4#diff-95fc8d6564ba293c6ce4a37ea33f73126fab928a0311b74c1a56b5b2d19d0bdeR919
is all covered by the fillCheckoutSessionsLineItems inside upsertCheckoutSessions
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.
Alright!
|
|
||
| async fillCheckoutSessionsLineItems(checkoutSessionIds: string[], syncTimestamp?: string) { | ||
| for (const checkoutSessionId of checkoutSessionIds) { | ||
| const lineItemResponses = await this.stripe.checkout.sessions.listLineItems( |
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.
Stripe supports auto-pagination in the node sdk, see https://docs.stripe.com/api/pagination/auto?lang=node
You should likely be using for await to ensure that this iterates over all pages, i.e. if this has 101 items, you wouldn't get the last item
You can use expandEntity - example usage
await this.expandEntity(charges, 'refunds', (id) =>
this.stripe.refunds.list({ charge: id, limit: 100 })
)
This will paginate through all items and replace the property, then you can just do your upsert afterwards
fix: backfill prices before inserting lineItems feat: auto pagination for lineItems
fix(checkoutSessions test): race condition
also tested again with my live data |
|
Nice! Thanks for the contribution - will trigger a release now |
|
Thank you! |
|
🎉 This PR is included in version 0.42.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
This PR adds sync for the checkout sessions (and line items)
Additional context
I created two new migrations (one for the checkout sessions and one for the line items.
I am not yet using this in production, so I am a little bit cautious. My tests worked fine though.