add info to customer table and stripe customer data #9004

anamarn · 2024-12-10T14:10:59Z

Solves (https://github.com/twentyhq/private-issues/issues/194)

TLDR

Updates the billingCustomer table data using stripe webhooks event, also updates the customer's metadata in stripe, in order to contain the workspaceId associated to this customer.

In order to test
Billing:

Set IS_BILLING_ENABLED to true
Add your BILLING_STRIPE_SECRET and BILLING_STRIPE_API_KEY
Add your BILLING_STRIPE_BASE_PLAN_PRODUCT_ID (use the one in testMode > Base Plan)

Authenticate with your account in the stripe CLI
Run the command: stripe listen --forward-to http://localhost:3000/billing/webhooks

Run the twenty workker

Authenticate yourself on the app choose a plan and run the app normally. In stripe and in posgress the customer table data should be added.

Next steps
Learn more about integrations tests and implement some for this PR.

greptile-apps

PR Summary

This PR refactors the billing webhook handling by splitting it into specialized services for customers, subscriptions, and entitlements, while adding functionality to sync customer metadata between Stripe and the local database.

Added new BillingWebhookCustomerService to handle customer lifecycle events and metadata synchronization
Split BillingWebhookService into three focused services (Customer, Subscription, Entitlement) for better separation of concerns
Added transformer utilities in /utils to standardize Stripe event data conversion
Added handling for CUSTOMER_CREATED/UPDATED/DELETED webhook events in billing.controller.ts
Enhanced error handling with new BILLING_CUSTOMER_DELETED exception code

_{💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!}

_{11 file(s) reviewed, 13 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-12-10T14:11:43Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

+      if (customer.deleted) {
+        throw new BillingException(
+          'Stripe customer deleted',
+          BillingExceptionCode.BILLING_CUSTOMER_DELETED,
+        );
+      }


logic: Throwing an exception here will cause the request to fail without sending a response, since there's no try/catch. Should return an error response instead.

greptile-apps · 2024-12-10T14:12:10Z

...wenty-server/src/engine/core-modules/billing/services/billing-webhook-entitlement.service.ts

+import { transformStripeEntitlementEventToEntitlementRepositoryData } from 'src/engine/core-modules/billing/utils/billing-webhook-entitlement-transformer.util';
+@Injectable()
+export class BillingWebhookEntitlementService {
+  protected readonly logger = new Logger(BillingWebhookEntitlementService.name);


style: logger is initialized but never used in the service

greptile-apps · 2024-12-10T14:12:10Z

...wenty-server/src/engine/core-modules/billing/services/billing-webhook-entitlement.service.ts

+    await this.billingEntitlementRepository.upsert(
+      transformStripeEntitlementEventToEntitlementRepositoryData(
+        workspaceId,
+        data,
+      ),
+      {
+        conflictPaths: ['workspaceId', 'key'],
+        skipUpdateIfNoValuesChanged: true,
+      },
+    );


style: consider wrapping this operation in a transaction to ensure data consistency if the upsert fails

greptile-apps · 2024-12-10T14:12:13Z

...enty-server/src/engine/core-modules/billing/services/billing-webhook-subscription.service.ts

+    if (!workspace) {
+      return;
+    }


style: silently returning on missing workspace could mask configuration issues - consider logging a warning

greptile-apps · 2024-12-10T14:12:14Z

...enty-server/src/engine/core-modules/billing/services/billing-webhook-subscription.service.ts

+    const billingSubscription =
+      await this.billingSubscriptionRepository.findOneOrFail({
+        where: { stripeSubscriptionId: data.object.id },
+      });


logic: findOneOrFail could throw an exception if subscription not found - needs try/catch

greptile-apps · 2024-12-10T14:12:17Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    await this.billingCustomerRepository.upsert(
+      {
+        workspaceId: stripeMetadataWorkspaceId,
+        stripeCustomerId: data.object.id,
+      },
+      {
+        conflictPaths: ['workspaceId', 'stripeCustomerId'],
+        skipUpdateIfNoValuesChanged: true,
+      },
+    );


logic: No error handling around the upsert operation. Should wrap in try/catch to handle potential database errors.

greptile-apps · 2024-12-10T14:12:18Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    );
+  }
+
+  async verifyStripeCustomerMetadata(stripeCustomer: Stripe.Customer) {


logic: verifyStripeCustomerMetadata returns undefined in success case and workspaceId in error case - this is counterintuitive and could cause bugs. Consider standardizing the return type.

greptile-apps · 2024-12-10T14:12:51Z

...erver/src/engine/core-modules/billing/utils/billing-webhook-subscription-transformer.util.ts

+    stripeCustomerId: data.object.customer as string,
+    stripeSubscriptionId: data.object.id,
+    status: data.object.status as SubscriptionStatus,
+    interval: data.object.items.data[0].plan.interval,


logic: Accessing items.data[0] without checking array length could cause runtime errors if subscription has no items

greptile-apps · 2024-12-10T14:12:52Z

packages/twenty-server/src/engine/core-modules/billing/stripe/stripe.service.ts

+  async updateCustomerMetadataWorkspaceId(
+    stripeCustomerId: string,
+    workspaceId: string,
+  ) {
+    await this.stripe.customers.update(stripeCustomerId, {
+      metadata: { workspaceId: workspaceId },
+    });
+  }


style: consider handling case where customer update fails - currently no error handling or retries

greptile-apps · 2024-12-10T14:12:52Z

...erver/src/engine/core-modules/billing/utils/billing-webhook-subscription-transformer.util.ts

+) => {
+  return {
+    workspaceId,
+    stripeCustomerId: data.object.customer as string,


logic: Type casting customer as string without validation could be unsafe if customer is not a string ID

charlesBochet · 2024-12-10T16:14:52Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

        workspaceId,
        event.data,
      );
+      const customer = await this.stripeService.getCustomer(
+        event.data.object.customer as string,


why do we need to cast as?

because the event.data.object.customer is a Stripe.Subscription.customer which can be a
string | Stripe.Customer | Stripe.DeletedCustomer.
Thus we force the cast to string if we want to retrieve the customer from stripe using the stripeId (string)

as is a Typescript keyword, it doesn't really perform real casting as I think you're implying here. Agree with Charles that it feels like a code smell. Are you sure it isn't always returning a customer object and then you can access .id?

charlesBochet · 2024-12-10T16:16:57Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    );
+  }
+
+  async verifyStripeCustomerMetadata(stripeCustomer: Stripe.Customer) {


I think this function is not useful and does not do what we expect it to do (it's returning undefined sometimes, sometimes the workspaceId).

We could rename it to: getWorkspaceIdFromBillingCustomer

I feel we actually don't need this method, let's just move this logic to the subscription webhook service

charlesBochet · 2024-12-10T16:17:44Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

        workspaceId,
        event.data,
      );
+      const customer = await this.stripeService.getCustomer(


we should move this logic into billingWebhookSubscriptionService to not pollute the controller which is handling all events (it will end up being very long if we keed adding code here)

charlesBochet · 2024-12-10T16:18:03Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

+          customer,
+        );
+
+      if (stripeMetadataNeedsUpdate) {


this is actually a workspaceId!

charlesBochet · 2024-12-10T16:19:44Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

+        );
+
+      if (updatedStripeWorkspaceId) {
+        await this.stripeService.updateCustomerMetadataWorkspaceId(


I think this should be moved within processStripeCustomerEvent too

charlesBochet · 2024-12-10T16:20:08Z

packages/twenty-server/src/engine/core-modules/billing/billing.exception.ts

@@ -11,4 +11,5 @@ export class BillingException extends CustomException {

 export enum BillingExceptionCode {
  BILLING_CUSTOMER_NOT_FOUND = 'BILLING_CUSTOMER_NOT_FOUND',
+  BILLING_CUSTOMER_DELETED = 'BILLING_CUSTOMER_DELETED',


why is it an issue, we don't want to update deleted customers subscription?

charlesBochet · 2024-12-10T16:20:27Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    });
+
+    if (!billingCustomer && !stripeMetadataWorkspaceId) {
+      return; //do nothing


comment not useful!

charlesBochet · 2024-12-10T16:21:02Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    });
+
+    if (!billingCustomer && !stripeMetadataWorkspaceId) {
+      return; //do nothing


why do nothing btw, is this a normal behavior?

This function was done that if the customer is not registered in our database and there is no metadata we don't give the workspaceId necessary to update the stripe customer. It is the normal behaviour , I will update the logic in order to have clearer coding patterns

charlesBochet · 2024-12-10T16:21:10Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+      return; //do nothing
+    }
+    if (billingCustomer && !stripeMetadataWorkspaceId) {
+      return billingCustomer.workspaceId; //send stripe to update (stripe customer metadata is not yet up to date)


no comments!

charlesBochet · 2024-12-10T16:23:19Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+    private readonly billingCustomerRepository: Repository<BillingCustomer>,
+  ) {}
+
+  async processStripeCustomerEvent(


this function should not return a workspaceId (you need it because you are putting some other logic depending in the controller). If it does we should make the naming clear

(but it does not IMO)

charlesBochet · 2024-12-10T16:23:36Z

...s/twenty-server/src/engine/core-modules/billing/services/billing-webhook-customer.service.ts

+      return; //do nothing
+    }
+    if (billingCustomer && !stripeMetadataWorkspaceId) {
+      return billingCustomer.workspaceId; //send stripe to update (stripe customer metadata is not yet up to date)


charlesBochet · 2024-12-10T16:24:31Z

...enty-server/src/engine/core-modules/billing/services/billing-webhook-subscription.service.ts

+      });
+
+    await this.billingSubscriptionItemRepository.upsert(
+      transformStripeSubscriptionEventToSubscriptionItemRepositoryData(


nice, very clear

charlesBochet · 2024-12-10T16:25:30Z

packages/twenty-server/src/engine/core-modules/billing/stripe/stripe.service.ts

@@ -114,7 +114,10 @@ export class StripeService {
      success_url: successUrl,
      cancel_url: cancelUrl,
    });
-  }
+  } // I prefered to not create a customer with metadat before the checkout, because it would break the tax calculation


understand the comment but why is it in here?

because Felix suggested this method for adding the customer metadata

I think what Charles meant is that in most cases we keep this kind of comment for the PR but don't leave them in code

charlesBochet · 2024-12-10T16:25:59Z

...server/src/engine/core-modules/billing/utils/billing-webhook-entitlement-transformer.util.ts

+
+import { BillingEntitlementKey } from 'src/engine/core-modules/billing/enums/billing-entitlement-key.enum';
+
+export const transformStripeEntitlementEventToEntitlementRepositoryData = (


file name does not meet function name

based on data type, looks the name should reflect the fact this is an UpdatedEvent

charlesBochet · 2024-12-10T16:26:45Z

...erver/src/engine/core-modules/billing/utils/billing-webhook-subscription-transformer.util.ts

+) => {
+  return {
+    workspaceId,
+    stripeCustomerId: data.object.customer as string,


Because
data.object.customer is of type
(property) Stripe.Price.product: string | Stripe.Product | Stripe.DeletedProduct

This is the same method we use to upsert data in BillingSubscriptionItemRepository (it hasn't changed). Do we need to refactorize?

charlesBochet · 2024-12-10T16:26:54Z

...erver/src/engine/core-modules/billing/utils/billing-webhook-subscription-transformer.util.ts

+  };
+};
+
+export const transformStripeSubscriptionEventToSubscriptionItemRepositoryData =


one util, one file!

FelixMalfait · 2024-12-10T15:46:42Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

@@ -61,16 +65,38 @@ export class BillingController {
        return;
      }

-      await this.billingWehbookService.processStripeEvent(
+      await this.billingWebhookSubscriptionService.processStripeSubscriptionEvent(


processStripeEvent? (Can it be something else than a subscriptionEvent if if's in the subscription service already)

FelixMalfait · 2024-12-11T12:57:32Z

packages/twenty-server/src/engine/core-modules/billing/stripe/stripe.service.ts

@@ -114,7 +114,10 @@ export class StripeService {
      success_url: successUrl,
      cancel_url: cancelUrl,
    });
-  }
+  } // I prefered to not create a customer with metadat before the checkout, because it would break the tax calculation


I think what Charles meant is that in most cases we keep this kind of comment for the PR but don't leave them in code

FelixMalfait · 2024-12-11T13:01:23Z

packages/twenty-server/src/engine/core-modules/billing/billing.controller.ts

        workspaceId,
        event.data,
      );
+      const customer = await this.stripeService.getCustomer(
+        event.data.object.customer as string,


as is a Typescript keyword, it doesn't really perform real casting as I think you're implying here. Agree with Charles that it feels like a code smell. Are you sure it isn't always returning a customer object and then you can access .id?

…twentyhq/twenty into feat/real-time-billing-table-population

charlesBochet

very clear! bravo!

github-actions · 2024-12-12T07:01:34Z

Thanks @anamarn for your contribution!
This marks your 13th PR on the repo. You're top 3% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

add info to customer table and stripe customer data

15eda56

anamarn requested a review from FelixMalfait December 10, 2024 14:11

greptile-apps bot reviewed Dec 10, 2024

View reviewed changes

charlesBochet reviewed Dec 10, 2024

View reviewed changes

charlesBochet assigned anamarn Dec 11, 2024

charlesBochet added the -PR: awaiting author label Dec 11, 2024

Rename unrelated file

fbdeab9

FelixMalfait reviewed Dec 11, 2024

View reviewed changes

anamarn added 2 commits December 11, 2024 15:43

add simplified code version and correct comments

7054169

Merge branch 'feat/real-time-billing-table-population' of github.com:…

d5f5df5

…twentyhq/twenty into feat/real-time-billing-table-population

anamarn added -PR: awaiting review and removed -PR: awaiting author labels Dec 11, 2024

fix import in job module

02e404a

charlesBochet approved these changes Dec 12, 2024

View reviewed changes

charlesBochet merged commit bce5be8 into main Dec 12, 2024
19 checks passed

charlesBochet deleted the feat/real-time-billing-table-population branch December 12, 2024 07:00


		import { BillingEntitlementKey } from 'src/engine/core-modules/billing/enums/billing-entitlement-key.enum';

		export const transformStripeEntitlementEventToEntitlementRepositoryData = (

add info to customer table and stripe customer data #9004

add info to customer table and stripe customer data #9004

Conversation

anamarn commented Dec 10, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2024