add sync customer command and drop subscription customer constraint #9131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds a command to sync Stripe customer data with the BillingCustomer table and modifies database constraints to facilitate this synchronization.
- Added
BillingSyncCustomerDataCommandin/billing/commands/billing-sync-customer-data.command.tsto sync Stripe customer data for active workspaces - Removed foreign key constraint between
billingSubscriptionandbillingCustomertables via migration1734532875877-dropSubscriptionCustomerConstraint.ts - Added
getCustomerData()inStripeServicelacks error handling for empty subscription results and array bounds checking - Potential data integrity risk from removing foreign key constraints without compensating application-level checks
- Command assumes Stripe API availability and lacks comprehensive error recovery strategy
5 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE "core"."billingSubscription" DROP CONSTRAINT "FK_9120b7586c3471463480b58d20a"`, |
There was a problem hiding this comment.
logic: dropping this constraint removes referential integrity - ensure application code properly handles subscription cleanup when customers are deleted
Comment on lines
82
to
86
| { | ||
| nullable: false, | ||
| onDelete: 'CASCADE', | ||
| createForeignKeyConstraints: false, | ||
| }, |
There was a problem hiding this comment.
logic: disabling foreign key constraints removes database-level data integrity checks - ensure application code properly validates customer existence before subscription operations
Comment on lines
72
to
79
| if (!options.dryRun && !billingCustomer) { | ||
| const customerData = | ||
| await this.stripeService.getCustomerData(workspaceId); | ||
|
|
||
| await this.billingCustomerRepository.upsert(customerData, { | ||
| conflictPaths: ['workspaceId'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
logic: No validation that customerData exists or is valid before upserting. Could create empty/invalid records if Stripe call fails silently.
Comment on lines
+81
to
+85
| if (options.verbose) { | ||
| this.logger.log( | ||
| chalk.yellow(`Added ${workspaceId} to billingCustomer table`), | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: Log message indicates addition but this runs even for failed operations. Move inside the if block after successful upsert.
Comment on lines
+45
to
+51
| } catch (error) { | ||
| this.logger.log( | ||
| chalk.red( | ||
| `Running command on workspace ${workspaceId} failed with error: ${error}, ${error.stack}`, | ||
| ), | ||
| ); | ||
| continue; |
There was a problem hiding this comment.
style: Error is logged but not tracked/reported. Consider adding error metrics or alerting for failed syncs.
| query: `metadata['workspaceId']:'${workspaceId}'`, | ||
| limit: 1, | ||
| }); | ||
| const stripeCustomerId = String(subscription.data[0].customer); |
There was a problem hiding this comment.
logic: direct array access without length check could throw error - validate subscription.data.length > 0 first
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE "core"."billingSubscription" DROP CONSTRAINT "FK_9120b7586c3471463480b58d20a"`, |
There was a problem hiding this comment.
this should be removed from the original migration, otherwise we will apply it first before removing it and end up in the same issue while applying it
charlesBochet
approved these changes
Dec 18, 2024
charlesBochet
approved these changes
Dec 19, 2024
Log |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR:
Solves (https://github.com/twentyhq/private-issues/issues/212)
Add command to sync customer data from stripe to BillingCustomerTable for all active workspaces. Drop foreign key contraint on billingCustomer in BillingSubscription (in order to not break the DB).
In order to test:
Run the command:
npx nx run twenty-server:command billing:sync-customer-dataTake into consideration
Due that all the previous subscriptions in Stripe have the workspaceId in their metadata, we use that information as source of true for the data sync
Things to do: