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

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

anamarn · 2025-01-09T13:55:56Z

New modifications

Due that some integrations tests requires IS_BILLING_ENABLED set to true, be mindful of the required environment variables that needs to be defined in this casse (look at EnvironmentService).

The migration separation implemented on 8772, implies that the billing tables are only created when billing is enabled. Thus I recommend as a first approach to run the integration tests with the flag 'with database reset'.

Always verify that the server is running correctly before running the tests.

Common tests (self-hosting instance)
Set IS_BILLING_ENABLED to false

 npx nx run twenty-server:test:integration:with-db-reset

Common+ Billing tests (cloud instance)
Set IS_BILLING_ENABLED to true
Add the other env variables required (you can also look at the server ci to know who they are)

npx nx run twenty-server:test:integration:with-db-reser test/integration/billing/suites/billing-controller.integration-spec.ts

You can ommit the file path if you want to run all tests

charlesBochet · 2025-01-09T14:32:21Z

.vscode/settings.json

@@ -49,4 +49,6 @@
  "files.associations": {
    ".cursorrules": "markdown"
  },
+  "jestrunner.codeLensSelector": "**/*.{test,spec,integration-spec}.{js,jsx,ts,tsx}"


what is it?

github-actions · 2025-01-09T17:31:41Z

	Fails
🚫	`node` failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-e909703a.json

Generated by 🚫 dangerJS against 2ef8786

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

charlesBochet added -PR: wip -PR: awaiting author and removed -PR: awaiting review labels Jan 8, 2025

anamarn added 3 commits January 9, 2025 14:09

separate into multiple stripe service refactor billing integration tests

789234e

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

2ab2deb

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

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

93d7901

anamarn changed the title ~~DRAFT: Add Integration and unit tests on Billing~~ Add Integration and unit tests on Billing Jan 9, 2025

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

charlesBochet reviewed Jan 9, 2025

View reviewed changes

charlesBochet approved these changes Jan 9, 2025

View reviewed changes

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

2ef8786

charlesBochet merged commit c39af5f into main Jan 9, 2025
20 checks passed

charlesBochet deleted the feat/add-unit-integration-test-billing branch January 9, 2025 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Integration and unit tests on Billing #9317

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

anamarn commented Jan 9, 2025

charlesBochet Jan 9, 2025

github-actions bot commented Jan 9, 2025

		@@ -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) {

Add Integration and unit tests on Billing #9317

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

anamarn commented Jan 9, 2025

charlesBochet Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

Log

Weiko Jan 3, 2025 •

edited

Loading