Repository navigation
feat(store): billing account delete hides the account's token transactions with the row - #1999
rohilsurana wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Summary
Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Coverage Report for CI Build 39228452466Coverage increased (+0.06%) to 55.524%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
4f3a2c0 to
88f48ee
Compare
be61aad to
49f5fe5
Compare
88f48ee to
55510df
Compare
49f5fe5 to
170adbe
Compare
55510df to
5a6a5bc
Compare
170adbe to
c2b82b8
Compare
c2b82b8 to
e625872
Compare
5a6a5bc to
0286c5f
Compare
| } | ||
|
|
||
| func (s *Service) logTokensForfeited(ctx context.Context, customer Customer, forfeit ForfeitedTokens) { | ||
| if err := audit.NewLogger(ctx, customer.OrgID).LogWithAttrs(audit.BillingTokensForfeitedEvent, audit.Target{ |
There was a problem hiding this comment.
this is not audit records, can we add audit records or is this event audit log intended?
There was a problem hiding this comment.
Moved to an audit record. The forfeited and purchased amounts are now event metadata on the billing_customer.deleted record, written in the delete transaction. The old log event is gone, and org delete no longer writes its own.
…tions and reports the forfeit
…t calls the billing provider
5d7f7fc to
7f9267e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: raystack/frontier/.coderabbit.yaml
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e8cc6bc4-0fe1-4ef3-ac62-1754a045a56d
📒 Files selected for processing: 18
billing/customer/errors.gobilling/customer/mocks/repository.gobilling/customer/service.gobilling/customer/service_test.gocmd/serve.gocore/audit/audit.gocore/deleter/delete_notice.gocore/deleter/delete_notice_test.gocore/deleter/mocks/audit_record_service.gocore/deleter/mocks/credit_service.gocore/deleter/service.gocore/deleter/service_test.godocs/content/docs/reference/webhook.mdxinternal/api/v1beta1connect/billing_errors.gointernal/api/v1beta1connect/billing_errors_test.gointernal/store/postgres/billing_customer_repository.gointernal/store/postgres/billing_customer_repository_pg_test.goweb/sdk/admin/utils/webhook-events.ts
💤 Files with no reviewable changes: 4
- docs/content/docs/reference/webhook.mdx
- core/deleter/mocks/credit_service.go
- web/sdk/admin/utils/webhook-events.ts
- core/audit/audit.go
| records, err := d.auditRecordService.List(ctx, utils.NewRQLQuery("", 0, forfeitRecordsLimit, []rql.Filter{ | ||
| {Name: "event", Operator: "eq", Value: auditrecord.BillingCustomerDeletedEvent.String()}, | ||
| {Name: "org_id", Operator: "eq", Value: orgID}, | ||
| }, []rql.Sort{{Name: "occurred_at", Order: "desc"}}, nil)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Confirm that standalone billing-account deletion reaches customer.Service.Delete.
rg -nP -C4 'DeleteBillingAccount|customerService\.Delete\(' --type=go -g '!**/mocks/**'Repository: raystack/frontier
Length of output: 4760
🏁 Script executed:
set -u
printf '%s\n' '--- core/deleter/delete_notice.go ---'
nl -ba core/deleter/delete_notice.go | sed -n '1,230p'
printf '%s\n' '--- billing/customer/service.go delete declarations/callers ---'
rg -n -C8 -F -- 'func (s *Service) Delete' billing internal core --glob '*.go' || true
printf '%s\n' '--- billing customer repository delete implementation ---'
rg -n -C16 -F -- 'func (r *BillingCustomerRepository) Delete' internal/store billing --glob '*.go' || true
printf '%s\n' '--- delete notice and customer delete call sites ---'
rg -n -C6 -e 'recoverForfeitFromAudit' -e 'customerService\.Delete\(' -e 'Delete\(ctx,.*customer' core billing internal --glob '*.go' || true
printf '%s\n' '--- relevant delete/audit tests ---'
rg -n -C6 -e 'forfeited_tokens' -e 'BillingCustomerDeletedEvent' -e 'recoverForfeitFromAudit' core billing internal --glob '*_test.go' || trueRepository: raystack/frontier
Length of output: 37442
🏁 Script executed:
set -u
printf '%s\n' '--- customer service Delete ---'
nl -ba billing/customer/service.go | sed -n '255,325p'
printf '%s\n' '--- repository Delete implementations ---'
rg -n -C12 --glob '*.go' -F -- 'func (r *BillingCustomerRepository) Delete' internal/store billing || true
rg -n -C12 --glob '*.go' -F -- 'BillingCustomerDeletedEvent' internal/store billing || true
printf '%s\n' '--- organization deletion flow ---'
nl -ba core/deleter/service.go | sed -n '250,315p'
nl -ba core/deleter/service.go | sed -n '430,495p'
printf '%s\n' '--- delete notice tests ---'
nl -ba core/deleter/delete_notice_test.go | sed -n '1,115p'Repository: raystack/frontier
Length of output: 20190
🏁 Script executed:
set -u
printf '%s\n' '--- billing customer List and repository visibility ---'
rg -n -C10 --glob '*.go' -F -- 'func (r *BillingCustomerRepository) List' internal/store/postgres
rg -n -C8 --glob '*.go' -F -- 'func (r *BillingCustomerRepository) GetByID' internal/store/postgres
printf '%s\n' '--- complete repository Delete block ---'
nl -ba internal/store/postgres/billing_customer_repository.go | sed -n '500,625p'Repository: raystack/frontier
Length of output: 5331
Scope forfeiture recovery to organization teardown attempts.
recoverForfeitFromAudit filters only by billing_customer.deleted and organization ID. Standalone DeleteBillingAccount calls the same customer deletion method used by organization teardown, and each deletion writes forfeited and purchased token metadata. A later organization deletion can therefore include a prior standalone deletion in its notice and settlement. The recovery runs on every organization deletion, not only retries.
Add an explicit organization-teardown marker to the deletion audit record and filter recovery by that marker. Do not use the organization ID alone as the scope.
There was a problem hiding this comment.
@rohilsurana This is real: DeleteBillingAccount writes the same forfeit metadata on billing_customer.deleted, so a later org delete adds it to the owner notice. Can we instead mark the records the org teardown writes, and only recover those?
| hideTransactionsQuery := `WITH hidden AS ( | ||
| UPDATE billing_transactions SET deleted_at = now() | ||
| WHERE account_id = $1 AND deleted_at IS NULL | ||
| RETURNING type, source, amount | ||
| ) | ||
| SELECT coalesce(sum(CASE WHEN type = $2 THEN amount ELSE -amount END), 0), | ||
| coalesce(sum(CASE WHEN source = $3 THEN (CASE WHEN type = $2 THEN amount ELSE -amount END) ELSE 0 END), 0) | ||
| FROM hidden` | ||
| hideTransactionsParams := []any{id, credit.CreditType, credit.SourceSystemBuyEvent} | ||
|
|
||
| var customerModel Customer | ||
| var forfeitedTokens, purchasedTokens int64 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'func .*CreateEntry|func .*CreateTransaction|Insert.*Transaction|insert into billing_transactions|Insert\("billing_transactions"|billing_transactions.*INSERT' billing internal | head -120
sed -n '105,130p' internal/store/postgres/migrations/20231012175641_create_billing_tables.up.sql
sed -n '500,595p' internal/store/postgres/billing_customer_repository.goRepository: raystack/frontier
Length of output: 6128
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- transaction repository ---'
sed -n '1,280p' internal/store/postgres/billing_transactions_repository.go
printf '%s\n' '--- concrete CreateEntry callers ---'
rg -n -F --glob '*.go' -e '.CreateEntry(' -e 'CreateEntry(' billing internal | head -240
printf '%s\n' '--- credit service and repository declarations ---'
rg -n -F --glob '*.go' -e 'type TransactionRepository' -e 'func (.*Credit' -e 'Credit(' billing internal | head -240
printf '%s\n' '--- focused tests ---'
sed -n '1,240p' internal/store/postgres/billing_transactions_repository_pg_test.go
printf '%s\n' '--- billing transaction migrations ---'
rg -n -F --glob '*.sql' -e 'billing_transactions' -e 'ALTER TABLE billing_transactions' internal/store/postgres/migrations | head -240Repository: raystack/frontier
Length of output: 23712
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- CreateEntry dependencies ---'
rg -n -F --glob 'internal/store/postgres/billing_transactions_repository.go' -e 'func (r BillingTransactionRepository) getBalanceInTx' -e 'func (r BillingTransactionRepository) GetBalance' -e 'customerRepo.GetByID' -e 'fromLive(TABLE_BILLING_TRANSACTIONS)' internal/store/postgres/billing_transactions_repository.go
rg -n -F --glob 'internal/store/postgres/billing_customer_repository.go' -e 'func (r BillingCustomerRepository) GetByID' -e 'func (r BillingCustomerRepository) Delete' internal/store/postgres/billing_customer_repository.go
printf '%s\n' '--- relevant transaction and customer blocks ---'
sed -n '100,245p' internal/store/postgres/billing_transactions_repository.go
sed -n '70,135p' internal/store/postgres/billing_customer_repository.go
sed -n '485,585p' internal/store/postgres/billing_customer_repository.go
printf '%s\n' '--- credit service interface and callers ---'
sed -n '1,175p' billing/credit/service.go
rg -n -F --glob '*.go' -e 'NewCreditService' -e 'credit.Service' -e 'Credit(ctx' -e 'Debit(ctx' billing internal cmd | head -240
printf '%s\n' '--- all billing transaction constraints in migrations ---'
rg -n -i --glob '*.sql' -e 'foreign key' -e 'references billing_customers' -e 'billing_transactions' internal/store/postgres/migrationsRepository: raystack/frontier
Length of output: 21189
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- production Add callers and wiring ---'
rg -n -F --glob '*.go' -g '!**/*_test.go' -e '.CreditService.Add(' -e '.creditService.Add(' -e 'CreditService.Add(' -e 'creditService.Add(' -e 'credit.Service' -e 'NewService(' billing internal cmd | head -260
printf '%s\n' '--- exact customer lookup and SQL helper blocks ---'
sed -n '175,235p' internal/store/postgres/billing_customer_repository.go
rg -n -F --glob 'internal/store/postgres/*.go' -e 'func fromLive' -e 'func softDelete' -e 'func (.*WithTxn' -e 'WithTxn(ctx' internal/store/postgres pkg/db
sed -n '1,120p' internal/store/postgres/query.go 2>/dev/null || true
sed -n '1,220p' pkg/db/client.go 2>/dev/null || trueRepository: raystack/frontier
Length of output: 14266
Lock the live customer in the credit transaction.
CreateEntry checks the target customer before it starts its transaction. credit.Service.Add then inserts the customer credit without reading or locking that customer inside the transaction.
If the pre-check succeeds and BillingCustomerRepository.Delete commits before the insert transaction starts, createTransactionEntry can insert a new live transaction for the soft-deleted account. The deletion hides only transactions that exist when its update runs. This leaves a live transaction outside the deletion totals.
Add a transaction-scoped live-customer check with FOR UPDATE before the balance check and transaction inserts. Return customer.ErrNotFound when the row is already soft-deleted.
There was a problem hiding this comment.
@rohilsurana This is the gap the description lists. Can we close it in a follow-up with a FOR KEY SHARE read of the live customer inside CreateEntry's transaction? It only has to clash with the delete's FOR UPDATE.
| type CreditService interface { | ||
| GetBalance(ctx context.Context, accountID string) (int64, error) | ||
| List(ctx context.Context, flt credit.Filter) ([]credit.Transaction, error) | ||
| DeleteByAccountID(ctx context.Context, accountID string) error |
There was a problem hiding this comment.
credit.Service.DeleteByAccountID has no callers after this change. Can we remove it, along with the repository method?
|
The description still says the forfeit goes to |
This branch was successfully deployed
3 participants
Summary
Deleting a billing account now hides the account's token transactions. The delete also tells the caller how many tokens were forfeited.
An account that owes tokens cannot be deleted. The API answers with failed precondition.
When tokens are forfeited, an
app.billing.tokens.forfeitedaudit record is written.This PR is stacked on #1983.
Changes
HasLiveRecordsrepository method. A refused delete no longer leaves a deleted customer at Stripe.ErrHasTokenDebterror to failed precondition.billing/creditandcore/deleterare not changed.Technical Details
account_idis the customer id, never the system account.system.buy, kept between zero and the forfeited amount.Test Plan
go build ./...go veton the touched packagesgo test ./internal/store/postgres/ ./billing/... ./core/deleter/... ./internal/api/v1beta1connect/golangci-lint runon the touched packages, 0 issuesgofmton the changed filesSQL Safety