-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: paddle payment provider #486
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: main
Are you sure you want to change the base?
Conversation
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.
Hey @jedpattersonpaddle, great PR.
Thanks for kick-starting this for us. Means a lot.
I didn't manage to go over the whole PR yet, just the paymentProcessor.ts
and paddle/webhook.ts
changes for now. I will go over the rest tomorrow.
It's mostly some typescript stuff and naming changes.
One thing we would kindly request of you, is if you could drop all of the changes not related to adding paddle functionality to open-saas
.
This is mostly referring to changes to paymentProcessor.ts
file.
I left a bigger comment there as to why.
I see that a lot of changes I requested are our fault, mostly that we have left payment/stripe
folder a bit messy. So if you copied our patterns you got the messy part too. Sorry about that.
However, we are currently in the process of improving that older Stripe code:
#493
Thanks a lot for this.
Means a lot to have someone with Paddle domain knowledge.
*/ | ||
// export const paymentProcessor: PaymentProcessor = lemonSqueezyPaymentProcessor; | ||
export const paymentProcessor: PaymentProcessor = stripePaymentProcessor; | ||
function getPaymentProcessor(): PaymentProcessor { |
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 wouldn't go this way about it.
The great thing about the previous approach is that payment providers aren't connected to each other.
You would also usually only want a single payment provider.
After you select your provider, the code and packages for rest of the payment providers is "dead code".
You're free to uninstall the dependencies and to remove that code from your code base.
This way you're not tied to the code that you application will never use.
If you ever change your mind about the provider, the simplest way to change it is to initialize a new open-saas
project and just copy the desired template's files (+ dependencies install).
In this approach you are forced to keep all of the payment providers, because your code depends on all of them inside of this function.
At least that was our reasoning for keeping the previous approach.
You are free to discuss this with us, but I would open up a new issue about it and do it in separate PR.
We want this PR to be focused solely on adding Paddle
.
We usually refer to this blog post when making our PRs:
https://mtlynch.io/code-review-love/#5-narrowly-scope-changes
}; | ||
|
||
async function handleSubscriptionCreated(eventData: EventEntity, prismaUserDelegate: PrismaClient['user']) { | ||
if (eventData.eventType !== EventName.SubscriptionCreated) return; |
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.
Instead of if (eventData.eventType !== EventName.SubscriptionCreated) return;
check, I would do:
async function handleSubscriptionCreated(
paddleEvent: SubscriptionCreatedEvent,
prismaUserDelegate: PrismaClient['user']
): Promise<void> {
// code
}
This will make sure nobody passes in the wrong event in the first place.
}` | ||
); | ||
break; | ||
default: |
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 is good place to:
default:
assertUnreachable(subscription.scheduledChange.action);
Since we exhausted all of the options, we won't get any errors.
subscriptionStatus = getSubscriptionStatusFromPaddleStatus(subscription.status, subscription.id); | ||
} | ||
|
||
const user = await updateUserPaddlePaymentDetails( |
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.
We don't need the user
.
await updateUserPaddlePaymentDetails(
console.log(`Subscription ${subscription.id} updated for customer ${subscription.customerId}`); | ||
} | ||
|
||
async function handleSubscriptionCanceled(eventData: EventEntity, prismaUserDelegate: PrismaClient['user']) { |
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.
Same as above:
async function handleSubscriptionCanceled(
paddleEvent: SubscriptionCanceledEvent,
prismaUserDelegate: PrismaClient['user']
): Promise<void> {
{ | ||
paddleCustomerId: eventData.data.customerId, | ||
// @ts-ignore userId is not typed in customData | ||
userId: eventData.data.customData?.userId as string, |
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.
Same as above:
userId: paddleEventCustomDataSchema.parse(paddleEvent.data.customData).userId,
} | ||
} | ||
|
||
function getSubscriptionStatusFromPaddleStatus(status: string, subscriptionId: string): SubscriptionStatus { |
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.
We can make this a bit more type safe, especially if we import the same-named SubscriptionStatus
from Paddle.
Also passing in subscriptionId: string
just for logging feels bad. I would avoid modifying a mapping function just for that.
import {
type SubscriptionStatus as PaddleSubscriptionStatus,
} from '@paddle/paddle-node-sdk';
// ...
function getSubscriptionStatusFromPaddleSubscriptionStatus(
paddleSubscriptionStatus: PaddleSubscriptionStatus
): SubscriptionStatus {
switch (paddleSubscriptionStatus) {
case 'active':
return SubscriptionStatus.Active;
case 'past_due':
return SubscriptionStatus.PastDue;
case 'canceled':
return SubscriptionStatus.Deleted;
case 'paused':
// Treating paused as past due for now
return SubscriptionStatus.PastDue;
default:
return SubscriptionStatus.PastDue; // Safe fallback
}
}
I think that code showcases well that we lack some important SubscriptionStatus
states in open-saas
.
I'll open up an issue about it later. But that is for a separate PR.
One question.
Do you think we should add trialing
as active
?
Then we cover all of the possible paddle statuses?
function getSubscriptionStatusFromPaddleSubscriptionStatus(
paddleSubscriptionStatus: PaddleSubscriptionStatus
): SubscriptionStatus {
switch (paddleSubscriptionStatus) {
case 'active':
return SubscriptionStatus.Active;
case 'past_due':
return SubscriptionStatus.PastDue;
case 'canceled':
return SubscriptionStatus.Deleted;
case 'paused':
// Treating paused as past due for now
return SubscriptionStatus.PastDue;
case 'trialing':
return SubscriptionStatus.Active;
default:
assertUnreachable(paddleSubscriptionStatus);
}
}
I'll start reviewing the rest of the code now. |
Thanks for all the detailed comments! I'll go through these today and hopefully have something releasable done soon. Great work with OpenSaaS, I was really impressed when playing around with it! |
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.
Hey finished reviewing the rest of the code.
Great work on this.
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 Paddle sandbox account.
One thing I have question about is, we don't handle anything non-subscription related in paddle/webhook.ts
?
How do we handle one-time payments? Do you have to still implement that path?
userId, | ||
userEmail, | ||
paymentPlan, | ||
prismaUserDelegate, |
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.
No need for prismaUserDelegate
if we don't use it.
createCheckoutSession: async ({ userId, userEmail, paymentPlan }: CreateCheckoutSessionArgs) => {
const session = await createPaddleCheckoutSession({
priceId: paymentPlan.getPaymentProcessorPlanId(),
customerEmail: userEmail,
userId,
});
return { session };
},
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.
Nevermind, it seems we will need it after all, we should use the prismaUserDelegate
instead of prisma.user
.
|
||
const customers = await customerCollection.next(); | ||
|
||
if (!customers) { |
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.
We really want to check if there are no customers with that customerEmail
here.
I assume customers customerCollection.next()
always returns an array, sometimes empty sometimes with customers (according to types).
If it always returns an array !customers
is always false.
So we always go to the second branch and try to fetch the customer, even if the array could be empty.
We should instead check the .length
of the array.
if (customers.length === 0) {}
email: customerEmail, | ||
}); | ||
|
||
await prisma.user.update({ |
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.
Okay we have a function called createPaddleCheckoutSession
but in reality it:
- fetches Paddle customers
- if none exist we create a Paddle customer
- we update the
User
entity - we create a checkout session
We should split this function up.
I would create two separate functions:
ensurePaddleCustomer
createPaddleCheckoutSession
And then move "we update the User
entity" part to paddlePaymentProcessor.createCheckoutSession
in paddle/paymentProcessor.ts
To reiterate:
We first call ensurePaddleCustomer
which:
- fetches Paddle customers
- if none exist we create a Paddle customer
- returns the found/created Paddle customer
Then we update the User
entity with prismaUserDelegate
inside of paddlePaymentProcessor.createCheckoutSession
.
Finally createPaddleCheckoutSession
should:
- create a checkout session
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.
Something akin to this in paddlePaymentProcessor.createCheckoutSession
:
const customer = await ensurePaddleCustomer({
customerEmail: userEmail,
});
await prismaUserDelegate.update({
where: {
id: userId,
},
data: {
paymentProcessorUserId: customer.id,
},
});
const checkoutSession = await createPaddleCheckoutSession({
userId,
customerId: customer.id,
priceId: paymentPlan.getPaymentProcessorPlanId(),
});
}); | ||
} | ||
|
||
if (!customer) throw new Error('Could not create customer'); |
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.
Is this necessary?
I would assume paddle.customers.create
would throw its own error if it failed?
return null; | ||
} | ||
|
||
try { |
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.
Just a question.
Why not return: https://developer.paddle.com/concepts/customer-portal#customer-portal
We want to give people to both mange their subscriptions, change payment methods and view past invoices?
I don't know too much about paddle.
Is there differences from what you're doing here and that?
I see that paddle.customerPortalSessions
exists.
}); | ||
} | ||
|
||
if (!user) { |
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 don't think we need to check for user.
Prisma will throw error here if no users are found to be updated:
return await prismaUserDelegate.update({
where: {
id: user.id,
},
data: updateData,
});
And User.paymentProcessorUserId
should exists as soon as the user created a single checkout session.
It's impossible to call updateUserPaddlePaymentDetails
if he didn't create a checkout session and bought something, so we don't have to check for it.
throw new Error(`User not found for Paddle customer ID: ${paddleCustomerId}`); | ||
} | ||
|
||
const updateData: any = { |
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.
Instead of doing any
you may find something like this more useful: https://github.com/wasp-lang/open-saas/pull/493/files#diff-167a62d0761d8215c87779c0a1a97a2b1e38db5f2be5180a8d644c4f4f80fb57
We can split updateUserPaddlePaymentDetails
into two different sub-functions:
updateUserPaddleOneTimePaymentDetails
updateUserPaddleSubscriptionDetails
Also also same for the function arguments type:
export async function updateUserPaddlePaymentDetails(
paymentDetails: UpdateUserPaddleOneTimePaymentDetails | UpdateUserPaddleSubscriptionDetails,
prismaUserDelegate: PrismaClient['user']
) {
// ...
}
interface UpdateUserPaddleOneTimePaymentDetails {
customerId: Customer['id'];
datePaid: Date;
numOfCreditsPurchased: number;
}
interface UpdateUserPaddleSubscriptionDetails {
customerId: Customer['id'];
subscriptionStatus: SubscriptionStatus;
paymentPlanId?: PaymentPlanId;
datePaid?: Date;
}
In that way we can keep everything type-safe.
Description
This PR adds support for Paddle as a payment provider. Users will need to set which provider they want to use in the .env file, but this is clearly documented in the README
Contributor Checklist