-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add billing tables #8772
base: main
Are you sure you want to change the base?
Add billing tables #8772
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.
PR Summary
This PR adds core billing tables and entities inspired by Stripe's data model, establishing the foundation for a comprehensive billing system with customer, product, and price management.
- Added migration
1732640291160-addBillingTables.ts
creating three new tables (billingCustomer
,billingProduct
,billingPrice
) with proper constraints and relationships - Introduced
BillingAvailablePlanKey
enum with 'base-plan' and 'pro-plan' options for subscription tier management - Added billing-related environment variables and validation in
environment-variables.ts
for Stripe integration configuration - Potential data consistency risk in
billing-subscription.entity.ts
having bothstripeCustomerId
field andbillingCustomer
relationship - Added unique constraint
IndexOnWorkspaceIdAndStripeCustomerIdUnique
inbillingCustomer
table to prevent duplicate customer records
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
12 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-server/src/engine/core-modules/billing/entities/billing-customer.entity.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-customer.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-entitlement.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-price.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-price.entity.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-product.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-subscription.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/entities/billing-subscription.entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/billing/enums/billing-available-plan-key.enum.ts
Outdated
Show resolved
Hide resolved
This looks great! I'm starting to think that this will really cluter self-hosted instances with tables they don't need. Could you please make is so that the app works without those tables if IS_BILLING_ENABLED is false? I pushed a small commit to help your get in that direction (now depending on the environment variable value it will run a different set of migrations). |
packages/twenty-server/src/engine/core-modules/billing/entities/billing-price.entity.ts
Outdated
Show resolved
Hide resolved
Could you please look at the Greptile comments and delete them if not relevant (or address them if they are!)? Thanks! |
enum: Object.values(BillingAvailablePlanKey), | ||
nullable: false, | ||
}) | ||
planKey: BillingAvailablePlanKey; |
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.
Maybe you've discussed that with @charlesBochet but I guess this comes from metadata column? I would be tempted to use a jsonb metadata column and then cast at the code level to really stick to Stripe as much as we can
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.
agree, let's align on stripe api
`CREATE TYPE "core"."billingProduct_plankey_enum" AS ENUM('base-plan', 'pro-plan')`, | ||
); | ||
await queryRunner.query( | ||
`CREATE TABLE "core"."billingProduct" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "deletedAt" TIMESTAMP WITH TIME ZONE, "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "active" boolean NOT NULL, "stripeProductId" character varying NOT NULL, "defaultPriceId" character varying NOT NULL, "defaultStripePriceId" character varying NOT NULL, "planKey" "core"."billingProduct_plankey_enum" NOT NULL, CONSTRAINT "PK_8bb3c7be66db8e05476808b0ca7" PRIMARY KEY ("id"))`, |
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 constraints?
`CREATE TABLE "core"."billingCustomer" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "deletedAt" TIMESTAMP WITH TIME ZONE, "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "stripeCustomerId" character varying NOT NULL, "workspaceId" character varying NOT NULL, CONSTRAINT "IndexOnWorkspaceIdAndStripeCustomerIdUnique" UNIQUE ("workspaceId", "stripeCustomerId"), CONSTRAINT "PK_5fffcd69bf722c297a3d5c3f3bc" PRIMARY KEY ("id"))`, | ||
); | ||
await queryRunner.query( | ||
`CREATE TYPE "core"."billingProduct_plankey_enum" AS ENUM('base-plan', 'pro-plan')`, |
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.
planKey (case) but not important
`CREATE TYPE "core"."billingPrice_interval_enum" AS ENUM('day', 'month', 'week', 'year')`, | ||
); | ||
await queryRunner.query( | ||
`CREATE TABLE "core"."billingPrice" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "deletedAt" TIMESTAMP WITH TIME ZONE, "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "stripePriceId" character varying NOT NULL, "active" boolean NOT NULL, "usageType" "core"."billingPrice_usagetype_enum" NOT NULL, "interval" "core"."billingPrice_interval_enum", "productId" character varying NOT NULL, "stripeProductId" character varying NOT NULL, "billingProductId" uuid, CONSTRAINT "IndexOnProductIdAndStripePriceIdUnique" UNIQUE ("productId", "stripePriceId"), CONSTRAINT "PK_13927aef8d4e68e176a61c33d89" PRIMARY KEY ("id"))`, |
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 think that these stripeIds are actually unique so unicity constraints do not have to be composed, it won't help
`ALTER TABLE "core"."billingSubscription" ALTER COLUMN "interval" TYPE text`, | ||
); | ||
} catch (error) { | ||
// Column may have already been altered in UseEnumForSubscriptionStatusInterval1719327438923 |
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 get these comments ?
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.
got 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.
I don't see a better way (we could check table existence before but this looks a bit painful to do)
], | ||
migrations: | ||
process.env.IS_BILLING_ENABLED === 'true' | ||
? [ |
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 think this works but I'm not sure how it will behave between migrate up and down in case we change IS_BILLING_ENABLED value. Let's say it is not meant to be changed :p
`ALTER TABLE "core"."billingPrice" DROP CONSTRAINT "IndexOnProductIdAndStripePriceIdUnique"`, | ||
); | ||
await queryRunner.query( | ||
`ALTER TABLE "core"."billingProduct" DROP COLUMN "defaultPriceId"`, |
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.
let's fix the initial migration instead!
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 for all updates on product, price here
@@ -0,0 +1,4 @@ | |||
export enum BillingAvailablePlanKey { |
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.
BillingPlanKey (not sure what available means here)
@@ -22,7 +22,10 @@ export class BillingService { | |||
return this.environmentService.get('IS_BILLING_ENABLED'); | |||
} | |||
|
|||
async hasWorkspaceActiveSubscriptionOrFreeAccess(workspaceId: string) { | |||
async hasWorkspaceActiveSubscriptionOrFreeAccess( |
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.
function name does not look right anymore
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.
Good one, left a few comments.
Let's make 100% this does not break existing workspace:
- checkout
main
, reset database with IS_BILLING_ENABLED false - checkout
feat/add-billing-meter
, migrate
Same with IS_BILLING_ENABLED true
import { BillingAvailablePlanKey } from 'src/engine/core-modules/billing/enums/billing-available-plan-key.enum'; | ||
|
||
export type BillingProductMetadata = { | ||
planKey: BillingAvailablePlanKey; |
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.
don't we also need to identify if a product is the PlanBaseProduct too (the one bearing the base price)
BillingProductMetadata = {
planKey: BillingAvailablePlanKey;
isBasePlan?: boolean;
}
what do you think @FelixMalfait @anamarn ?
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.
Yes, you are right this will simplify the queries when accessing the billing tables, the structure looks good to me, i'll implement this in my next commit
Beforehand, the name of the branch is not representative of the work that has been done in this PR
TLDR:
Solves https://github.com/twentyhq/private-issues/issues/192
Add 3 tables BillingCustomer, BillingProduct and BillingPrice to core, inspired by the Stripe implementation.
In order to test:
Run the command:
npx nx typeorm -- migration:run -d src/database/typeorm/core/core.datasource.ts
Considerations:
I only put the information we should use right now in the Billing module, for instance columns like meter or agreggation formula where omitted in the creation of the tables.
These columns and other ones who fall on the same spectrum will be added as we need them.
If you want to add more information to the table, I'll leave some utility links down bellow:
Next Steps
Use the Stripe Webhook in order to update the tables accordingly