Polar integration #461
Polar integration #461
Conversation
- improve encapsulation and separation of concerns - simplify stats calculation
- add JSDoc comments for PaymentProcessor methods - improve JSDoc comments for exported object
- Enable processor selection by environment variable or specified override for testing scenarios - Refactor env var validation
- Add type-safety to webhook handler - Apply current Wasp env var validation
|
@vincanger Still a WIP, but as I've gotten to the point where I can create orders and subscriptions, I thought I should share the changes I've made that affect other parts of the codebase for discussion. Configurable provider selectionThe current codebase requires the developer implementing the template to modify the codebase to select which payment provider to implement. I understand the reasoning behind this decision, but it also makes it impossible to implement e2e tests for multiple providers without modifying the codebase. To address this, I implemented a new Updated schema validationOpenSaas currently uses a custom validation function to ensure the required env vars are set, but with the introduction of Zod validation, this seems redundant and so I implemented Zod-based validation for this provider, which could also be applied to the existing platforms, if desired. Refactored stats jobThe stats job previously contained functions for both Stripe and LemonSqueezy, which felt like a bit of code smell as it violates the open/closed principle, so I refactored that code to make the revenue calculation a function of the PaymentProcessor interface to be implemented by each integration. |
There was a problem hiding this comment.
Did a quick review. I see out moduleResolution is causing import issues for the polar SDK. Hopefully we can get that sorted out quick. Everything looks to be on the right track though! The main thing I'd like to change at the moment is to remove comments that are redundant as many of them just repeat what's discernible from the function name.
- Fix order status checking - Remove redundant subscription status mapping type and custom status values - Remove redundant JSDoc comments
There was a problem hiding this comment.
Hey @Genyus,
went over the PR.
Great work on this.
I'm sorry that we are asking you to remove so much from PR, but we want to focus on getting only Polar working here first.
Also I would appreciate it if you could reduce the amount of unnecessary jsdocs/comments. Didn't want to point it out everywhere since there is a lot. Do take care to use it only when we need it.
For now I've only went through the code, didn't run anything yet.
After you implement changes I would like to actually test everything out with Polar sandbox account. I would recommend you do the same.
template/app/.env.server.example
Outdated
| @@ -3,6 +3,9 @@ | |||
| # If you use `wasp start db` then you DO NOT need to add a DATABASE_URL env variable here. | |||
| # DATABASE_URL= | |||
|
|
|||
| # Supports Stripe, LemonSqueezy, Polar | |||
| PAYMENT_PROCESSOR_ID=Stripe | |||
There was a problem hiding this comment.
Hey, it's great to see that you're trying to improve open-saas further above the Polar integration, but I wouldn't do this as part of this PR.
Paddle integration PR had the same problem here:
https://github.com/wasp-lang/open-saas/pull/486/files#r2266908329
Doing in this PR will distract us from the main point (Polar), and will prolong the process to get the feature we want.
I've explained in the Paddle PR (linked above) why we don't believe this approach is right for us.
I would kindly ask you to remove non-Polar parts of the PR.
Thanks for the effort.
There was a problem hiding this comment.
@FranjoMindek Thanks for the feedback. Happy to proceed, but one of the main reasons I adopted this approach was because I wanted to add e2e tests for Polar and the current architecture makes it impossible to run tests for more than one provider without modifying the source. Do you have any suggestions for how that limitation could be addressed?
There was a problem hiding this comment.
Hey @Genyus.
Truthfully, e2e-tests are in a bad state and outdated.
We will want to improve them but that is a separate issue.
However we did start opening up issues about them:
#469
I wouldn't be concerned with improving them in this PR.
I think it's okay for now to require people to change the app to test different payment providers in e2e-tests.
What we want of e2e-tests is that we have separate package.json scripts to run for different providers.
So if we have the correct payment provider in app and run the correct script, it works.
So I would rename the current scripts to be more explicit about stripe and then add scripts for Polar.
This is kinda harder to do fully correct because we would have to change a lot of script names.
I would most likely suggest to do minimal changes for now, and we will refactor all of the names then in a separate PR dedicated to e2e-tests.
So just change the script name for running Stripe e2e tests, and add yours to be similarly named.
"local:e2e:stripe:start": "npm run local:e2e:cleanup-stripe && npm run local:e2e:start-stripe && npm run local:e2e:playwright:ui && npm run local:e2e:cleanup-stripe",
"local:e2e:polar:start": "...",
},There was a problem hiding this comment.
One thing in code part of e2e-tests you would need to change is to make all "Stripe specific" functions be changed with "payment provider abstract" ones.
Now to know which one to run, you could do the environment variable trick in e2e-tests, but we would set them through npm scripts instead.
e.g. "local:e2e:polar:start": "PAYMENT_PROVIDER=polar ..."
Then those abstract functions would use PAYMENT_PROVIDER to know what to do under the hood.
There was a problem hiding this comment.
There is probably a better way to do this (but it would require too much changes).
This will allows us to do this PR reasonably fast and without larger changes, and later on we can improve it.
There was a problem hiding this comment.
OK, understood. So I had already created a separate e2e-tests branch to work on these specifically and I use a PAYMENT_PROVIDER_ID env var to handle provider-specific scripts, as you suggested. But I'll leave this alone until I've completed the requested changes to the application code.
There was a problem hiding this comment.
Okay, btw ping me here and/or on discord when you are ready for re-review.
- Remove payment processors and types - Restore hard-coded payment processor references - Remove validation schemas
- Remove unnecessary try/catch blocks - Remove excessive JSDoc comments
- Remove env var and rely solely on polar.customerSessions.create call
- Refactor sandbox mode selection logic
There was a problem hiding this comment.
Hey Genyus, glad we reduce the PR to affect only Polar.
I went over it besides webook.ts individual handle functions for events.
I skipped them over for now because they might change from other comments.
One thing we are also missing here is documentation update to opensaas-sh/blog.
We should add it once we are happy with Polar integration.
| await handleOrderCompleted(data, prismaUserDelegate); | ||
|
|
||
| break; | ||
| case 'subscription.revoked': |
There was a problem hiding this comment.
I would be careful here that we don't react to events twice.
e.g. if subscription.revoked also triggers subscription.updated we are reacting twice to it -> bad.
Only add subscription events besides subscription.updated if they happen without updated event.
Can you confirm this for me?
There was a problem hiding this comment.
Unfortunately, the Polar API emits two updated events for each action, e.g.
subscription.updatedsubscription.canceledsubscription.updated
When applying an actual change to the subscription, we only receive the two subscription.update events , and the payload for each event is identical besides the type property.
The docs claim that the data.status property should reflect the status of the subscription, suggesting we should be able to simply listen to the subscription.updated event alone, but that hasn't been the case in my testing — the status value is always active, regardless of the actual subscription state.
I've reached out in the Polar community to see if there's an explanation/resolution for this unexpected behaviour.
There was a problem hiding this comment.
I've played around a bit with this.
Unfortunately, the Polar API emits two updated events for each action
As you've said, all subscription events chains start with subscription.updated, then that event chain fires, then end with subscription.updated. Why it sends subscription.updated twice, I'm not sure.
e.g.
cancelling at period end:
subscription.updated -> subscription.canceled -> subscription.updated
cancelling immediately (through Polar dashbaord):
subscription.updated -> subscription.canceled -> subscription.revoked -> subscription.updated
And as you've said all of them have the same body, only the type is different.
From what I understood subscription.updated is not a unique event, it just fires together with every other event. So If we omit subscription.updated and react to everything else we should be fine.
That means we either:
- Only use
subscription.updatedand handle all branching there - Use every
subscriptionevent besidessubscription.updatedandsubscription.created(since we use card payments we don't need this), and handle things individually
Downsides of 1):
- Since
subscription.updatedis fired twice per some action, we write the same thing to our entity twice doing unnecessary work. Not a real problem since subscription updates are idempotent. - We depend on API not changing that our branching logic works. E.g.
cancel_at_period_endexisting. There is no API versioning on Polar yet like on Stripe.
Downsides of 2):
- All of the events inside of some action "event chain" have the same body. It feels weird to react to
subscription.canceledbutdata.ended_atanddata.statusare notnullbecause this "canceled" is part of "revoke" action. In other words, we must react according toevent.typeinstead ofevent.data.
The docs claim that the data.status property should reflect the status of the subscription, suggesting we should be able to simply listen to the subscription.updated event alone, but that hasn't been the case in my testing — the status value is always active, regardless of the actual subscription state.
That is because you've most likely only made the subscription "cancel at period end" instead of "immediately".
Subscription to be cancelled is still active. When cancelled immediately (or when period end comes) it properly gets the canceled status with subscription.revoked event.
We can see that the statuses are incomplete, incomplete_expired, trialing, active, past_due, canceled, unpaid. So there is no status for something "to be canceled".
From above you can try to attempt either 1) or 2) that is up to you.
There was a problem hiding this comment.
So If we omit subscription.updated and react to everything else we should be fine.
We can't actually omit subscription.updated, because if you change your subscription level in the Polar dashboard (e.g 'Hobby' -> 'Pro'), then this is the only event that gets fired. As such, I think Option 1 is the way to go.
Since subscription.updated is fired twice per some action, we write the same thing to our entity twice doing unnecessary work. Not a real problem since subscription updates are idempotent.
I'm still corresponding with Polar support, but I'm pretty sure this is a bug and hoping we'll get a resolution at some point.
So there is no status for something "to be canceled".
Yep, I figured this out after some further review of the docs
There was a problem hiding this comment.
We can't actually omit subscription.updated, because if you change your subscription level in the Polar dashboard (e.g 'Hobby' -> 'Pro'), then this is the only event that gets fired. As such, I think Option 1 is the way to go.
Good catch there. Didn't try out the subscription change yet.
Let's go with 1) as you've said.
There was a problem hiding this comment.
I refactored webhook.ts, please review when you're able
There was a problem hiding this comment.
Hey, still didn't do webhook.ts file since I presume we are waiting for their response.
In the meantime I noticed I missed a single comment you made because it was resolved. So I responded now.
I also have a question around createPolarCheckoutSession and ensurePolarCustomer.
- Query and update users by Polar customer ID instead of Wasp user ID - Remove single-function file
- Consolidate all client behaviour into one file
There was a problem hiding this comment.
Mostly webhook.ts logic, with one question to polarClient.ts.
In my opinion SubscriptionAction machinery seems too over-engineered without proper look into polar states/actions. I would avoid such approach. It's really had to model such state machines right.
e.g. deleted state doesn't exist in polar but we handle it
I think we can simplify a lot by merging handleSubscriptionUpdated, applySubscriptionStateChange and removing the SubscriptionAction concept.
Then if we see some abstractions we can make them.
Currently these just add on to the complexity instead of solving it.
| } | ||
| } | ||
|
|
||
| function getSubscriptionAction(context: SubscriptionActionContext): SubscriptionAction { |
There was a problem hiding this comment.
After reviewing a bit this whole getSubscriptionAction function and SubscriptionAction actions seem weird.
I'd rather much avoid this approach because it's hard to model all possible actions and edge cases.
I'd rather start with base values inside of handleSubscriptionUpdated and update them according to subscription data. Seems much more safe.
| throw new Error(`Unknown Polar product ID: ${polarProductId}`); | ||
| } | ||
|
|
||
| async function updateUserPaymentDetails( |
There was a problem hiding this comment.
Also not sure why we mode this here out of checkoutUtil.ts
I would for now follow the pattern we have.
If we want to break the pattern we have to update it everywhere.
There was a problem hiding this comment.
Sorry, I'm slightly lost on this one. What's been moved out of checkoutUtils.ts, exactly? I checked all the referenced functions and I'm not sure what you're referring to...
- Remove ts-ignore statements
- Remove redundant type - Relocate middleware function - Rename functions - Replace string references with enum values - Remove redundant error handling
- Simplify handling logic - Remove unused types - Replace metadata parsing with built-in Polar response property checking
Description
Adds support for Polar payments platform. Closes #441
Related to wasp-lang/wasp#3034 as we need to remove
Stitchesinternally in order to be able to use the correctmoduleResolutionthat Polar's SDK depends on in our tsconfig.Contributor Checklist