feat(store): billing reads skip soft-deleted rows - #1962
rohilsurana wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughBilling customer, invoice, and transaction queries now exclude soft-deleted records in specified lookups, lists, searches, and balance calculations. PostgreSQL tests cover these filters and their effects on returned records and spending checks. A TODO comment documents an invoice and customer deletion-order constraint. ChangesBilling repository reads
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to Billing reads and balances exclude soft-deleted records as intended, with tests covering filtering and spending checks. No actionable merge-blocking issue is identified; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes narrow billing visibility without expanding tenant access or administrative privileges. Balance reads and spending checks use the same filtering rule. No newly exploitable security issue was identified, but operational database writers and concurrent deletion behavior remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Coverage Report for CI Build 36717147649Coverage increased (+0.8%) to 53.743%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Summary
The billing customer, transaction and invoice repositories read rows without ever checking
deleted_at. They now skip deleted rows, using the sameliveandfromLivehelpers as the rest of the store.Nothing writes
deleted_atto these three tables yet, so every read returns the same rows as before. This is groundwork so that soft-deleting billing rows on org delete is safe to turn on later.Changes
deleted_at: the reads by id, the lists, and the three balance sums.CreateEntryand the balance shown to the user in agreement, since both read through the same helpers.Technical Details
The plain invoice
Listreads one table and is left that way, so an invoice under a deleted customer still shows up there. That matches every other single-table list in the store, and the admin search is where the customer and the org get checked. A test pins both, so the difference is deliberate rather than something I missed.The update paths are untouched. So are the hard deletes; moving those to soft deletes is separate work.
That separate work has an ordering problem worth flagging now. Once the cascade soft-deletes, an invoice can only be marked deleted after its customer is, and the loop deletes the customer last. The comment sits on the line that will break.
No migration. All three tables already have the column.
Test Plan
go test -run 'TestBillingCustomerRepository|TestBillingTransactionRepository|TestBillingInvoiceRepository|TestSearchInvalidUUID' ./internal/store/postgres/passesmaingolangci-lint run ./internal/store/postgres/... ./core/deleter/...reports no issuesEach repository gets a postgres-backed test that seeds a live row and a deleted one, and I watched each fail before adding the filter. The balances came back 135, 135, 100 and 50 against the expected 95, 95, 70 and 30, which is every deleted row still being counted. The admin search returned four invoices before the change and one after. Reverting each repository file on its own was also checked, and every new test fails when its own change is taken away.
The eight pinned SQL strings in the invoice unit test were regenerated. The params are unchanged, since
IS NULLbinds nothing.SQL Safety (if your PR touches
*_repository.goorgoqu.*)?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L(usemake_interval(hours => ?)-style functions instead).//nolint:forbidigoor// #nosec G20xannotation has a one-line justification on the same line that a reviewer can verify.The added predicates come from the existing
live()helper and bind no values. The onlyfmt.Sprintfin the diff buildsTRUNCATEin the new test teardowns from package table constants, the same as the other suites in this package. No new lint or gosec annotations.