DRAFT: Add Integration and unit tests on Billing #9317

anamarn · 2025-01-02T12:52:58Z

Solves [ https://github.com/twentyhq/private-issues/issues/214 ]

TLDR
Add unit and integration tests to Billing. First approach to run jest integration tests directly from VSCode.

In order to run the unit tests:
Run unit test using the CLI or with the jest extension directly from VSCode.

In order to run the integration tests:
Ensure that your database has the billingTables. If that's not the case, migrate the database with IS_BILLING_ENABLED set to true:
npx nx run twenty-server:test:integration test/integration/billing/suites/billing-controller.integration-spec.ts

Doing:

Unit test on transformSubscriptionEventToSubscriptionItem
More tests cases in billingController integration tests.

greptile-apps

PR Summary

This PR adds comprehensive test coverage for the billing functionality, focusing on Stripe webhook handling and data transformations.

Added extensive unit tests in /packages/twenty-server/src/engine/core-modules/billing/utils/__tests__/ covering all Stripe data transformation utilities with thorough edge cases
Added integration tests in /packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts for webhook handling of product, price, and subscription events
Modified nx.json and .eslintrc.cjs to properly configure integration test files with pattern *.integration-spec.ts
Added mock StripeService in create-app.ts for webhook testing, though signature validation could be more robust
Incomplete test coverage for entitlement handling as noted by commented out test case

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

greptile-apps · 2025-01-02T12:53:38Z

...rc/engine/core-modules/billing/utils/__tests__/is-stripe-valid-product-metadata.util.spec.ts

+  it('should return true if metadata is empty', () => {
+    const metadata: Stripe.Metadata = {};
+
+    expect(isStripeValidProductMetadata(metadata)).toBe(true);
+  });


logic: Empty metadata returning true but missing required keys returning false (line 49-55) seems inconsistent. Should validate this is the intended behavior.

greptile-apps · 2025-01-02T12:53:49Z

.eslintrc.cjs

+      files: [
+        '*.spec.@(ts|tsx|js|jsx)',
+        '*.integration-spec.@(ts|tsx|js|jsx)',
+        '*.test.@(ts|tsx|js|jsx)',
+      ],


greptile-apps · 2025-01-02T12:54:20Z

...sts__/transform-stripe-entitlement-updated-event-to-entitlement-repository-data.util.spec.ts

+    ]);
+  });
+
+  it('should return the SSO key with false value,should only render the values that are listed in BillingEntitlementKeys', () => {


style: Test description contains a comma splice. Consider splitting into two test cases for clarity: one for SSO false value and one for BillingEntitlementKeys filtering

greptile-apps · 2025-01-02T12:54:33Z

...es/billing/utils/__tests__/transform-stripe-meter-data-to-meter-repository-data.util.spec.ts

+      default_aggregation: {
+        formula: 'sum',
+      },
+      event_time_window: null,


style: test should also verify behavior with event_time_window='hour' since that's another valid value

greptile-apps · 2025-01-02T12:54:41Z

...s/billing/utils/__tests__/transform-stripe-price-event-to-price-repository-data.util.spec.ts

+      mockData as any,
+    );


style: avoid using 'any' type casting - should define proper type for mockData based on Stripe.PriceCreatedEvent.Data

greptile-apps · 2025-01-02T12:55:53Z

packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts

+    await client
+      .post('/billing/webhooks')
+      .set('Authorization', `Bearer ${ACCESS_TOKEN}`)
+      .set('stripe-signature', 'correct-signature')


logic: Using 'correct-signature' as a static value doesn't properly test signature verification. Should use Stripe's signing secret to generate valid signatures for test payloads.

greptile-apps · 2025-01-02T12:55:53Z

packages/twenty-server/test/integration/utils/create-app.ts

+      constructEventFromPayload: (signature: string, payload: Buffer) => {
+        if (signature === 'correct-signature') {
+          const body = JSON.parse(payload.toString());
+
+          return {
+            type: body.type,
+            data: body.data,
+          };
+        }
+        throw new Error('Invalid signature');
+      },


style: Mock implementation is overly simplified and may miss edge cases. Consider using Stripe's testing webhooks or a more robust signature verification process.

greptile-apps · 2025-01-02T12:55:53Z

packages/twenty-server/test/integration/utils/create-app.ts

+      verify: (req: any, res, buf) => {
+        req.rawBody = buf;
+      },


style: req is typed as 'any'. Consider creating a proper type extending Express.Request

packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts

greptile-apps · 2025-01-02T12:55:54Z

packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts

+describe('BillingController (integration)', () => {
+  const mockTimestamp = 1672531200;
+
+  //put this in utils?


style: Mock data creation utilities should be moved to a shared test utils file for reuse across test suites

FelixMalfait · 2025-01-02T13:17:28Z

packages/twenty-server/test/integration/utils/create-app.ts

@@ -25,7 +28,21 @@ export const createApp = async (
 ): Promise<NestExpressApplication> => {
  let moduleBuilder: TestingModuleBuilder = Test.createTestingModule({
    imports: [AppModule],
-  });
+  })


as discussed, let's initialize the app with the same parameters as the real app (i.e. cors true and rawbody true)

FelixMalfait · 2025-01-02T13:17:34Z

packages/twenty-server/test/integration/utils/create-app.ts

@@ -35,6 +52,14 @@ export const createApp = async (

  const app = moduleFixture.createNestApplication<NestExpressApplication>();

+  app.use(


Let's remove this

FelixMalfait · 2025-01-02T13:22:31Z

packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts

+const client = request(`http://localhost:${APP_PORT}`);
+
+// TODO: Add more tests
+// THIS TEST ONLY WORKS IF THE BILLING TABLES ARE CREATED, VERIFY IS_BILLING_ENABLED IS TRUE


check ci-server.yaml

FelixMalfait · 2025-01-02T13:25:31Z

packages/twenty-server/test/integration/billing/suites/billing-controller.integration-spec.ts

+      .set('stripe-signature', 'correct-signature')
+      .set('Content-Type', 'application/json')
+      .send(JSON.stringify(payload))
+      .expect(200);


You're not testing much here when just checking the status code, could you check something else everytime you check a 200, for example modify the response so that it provides the id of the object we just inserted/modified, and then in the test you verify that the request returned a uuid as a response

…wentyhq/twenty into feat/add-unit-integration-test-billing

Weiko · 2025-01-03T14:36:20Z

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

@@ -43,6 +43,8 @@ export class BillingController {
    @Req() req: RawBodyRequest<Request>,
    @Res() res: Response,
  ) {
+    let resultBody = {};


Style notes:
You can instead create another method like handleStripeEvent that will return the body to simplify your code

const result = await this.handleStripeEvent(event); return res.status(200).send(result).end();

private async handleStripeEvent(event: WebhookEvent) { switch (event.type) { case WebhookEvent.SETUP_INTENT_SUCCEEDED: return this.billingSubscriptionService.handleUnpaidInvoices(event.data); case WebhookEvent.CUSTOMER_SUBSCRIPTION_CREATED: case WebhookEvent.CUSTOMER_SUBSCRIPTION_UPDATED: case WebhookEvent.CUSTOMER_SUBSCRIPTION_DELETED: ......

Weiko · 2025-01-03T14:37:12Z

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

@@ -54,7 +56,9 @@ export class BillingController {
    );

    if (event.type === WebhookEvent.SETUP_INTENT_SUCCEEDED) {


Can we rename this WebhookEvent to BillingWebhookEvent?

Weiko

LGTM

add some tests in billing

8f0847a

anamarn requested a review from FelixMalfait January 2, 2025 12:53

greptile-apps bot reviewed Jan 2, 2025

View reviewed changes

Merge branch 'main' into feat/add-unit-integration-test-billing

9396cf8

FelixMalfait reviewed Jan 2, 2025

View reviewed changes

anamarn added 2 commits January 2, 2025 17:47

lint and upgrade tests

6bed5fa

Merge branch 'feat/add-unit-integration-test-billing' of github.com:t…

a726630

…wentyhq/twenty into feat/add-unit-integration-test-billing

Weiko reviewed Jan 3, 2025

View reviewed changes

Weiko added the -PR: awaiting author label Jan 3, 2025

update billing controller code style

80d0844

anamarn added -PR: awaiting review and removed -PR: awaiting author labels Jan 6, 2025

Weiko approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'main' into feat/add-unit-integration-test-billing

ab494f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: Add Integration and unit tests on Billing #9317

DRAFT: Add Integration and unit tests on Billing #9317

anamarn commented Jan 2, 2025

greptile-apps bot left a comment

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

greptile-apps bot Jan 2, 2025

FelixMalfait Jan 2, 2025

FelixMalfait Jan 2, 2025

FelixMalfait Jan 2, 2025

FelixMalfait Jan 2, 2025

Weiko Jan 3, 2025 •

edited

Loading

Weiko Jan 3, 2025

Weiko left a comment

		@@ -35,6 +52,14 @@ export const createApp = async (

		const app = moduleFixture.createNestApplication<NestExpressApplication>();

		app.use(

		@@ -54,7 +56,9 @@ export class BillingController {
		);

		if (event.type === WebhookEvent.SETUP_INTENT_SUCCEEDED) {

DRAFT: Add Integration and unit tests on Billing #9317

Are you sure you want to change the base?

DRAFT: Add Integration and unit tests on Billing #9317

Conversation

anamarn commented Jan 2, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 2, 2025

Choose a reason for hiding this comment

FelixMalfait Jan 2, 2025

Choose a reason for hiding this comment

FelixMalfait Jan 2, 2025

Choose a reason for hiding this comment

FelixMalfait Jan 2, 2025

Choose a reason for hiding this comment

FelixMalfait Jan 2, 2025

Choose a reason for hiding this comment

Weiko Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Weiko Jan 3, 2025

Choose a reason for hiding this comment

Weiko left a comment

Choose a reason for hiding this comment

Weiko Jan 3, 2025 •

edited

Loading