Skip to content

feat(store): org invoices search skips soft-deleted rows - #1951

Open
AmanGIT07 wants to merge 2 commits into
mainfrom
soft-delete-org-invoices-reads
Open

AmanGIT07 wants to merge 2 commits into
mainfrom
soft-delete-org-invoices-reads

Conversation

@AmanGIT07

Copy link
Copy Markdown
Contributor

Seventh of the org view repositories. Follows #1947 and #1949.

Summary

The organization invoices search leaves out soft-deleted invoices, billing customers, and organizations. The change covers both of the repository's query paths, the listing and the group-by count.

Changes

  • Both queries start from fromLive(billing_invoices) and add live on the joined billing customers.
  • Both gain an inner join on organizations with live on it, so a soft-deleted organization reports nothing.
  • New docker-backed suite org_invoices_repository_pg_test.go: seeds a soft-deleted invoice, one under a soft-deleted billing customer, and one under a soft-deleted organization, then checks the listing and the group counts.

Technical Details

This repository builds two queries. buildBaseQuery returns the rows and prepareGroupByQuery returns the per-state counts shown above them. Filtering only the first would have left the summary counting rows the list no longer shows.

All three tables have carried deleted_at since they were created.

The join is on the organizations primary key, so it matches at most one row and cannot duplicate invoices.

Frontier does not set deleted_at on these tables yet, so the search returns the same rows as before.

Test Plan

  • go test -run 'TestOrgInvoices' ./internal/store/postgres/ passes (unit tests and the new suite)
  • The new suite fails on main, including the group counts
  • golangci-lint run ./internal/store/postgres/... reports no issues
  • End to end against a local server through FrontierService/SearchOrganizationInvoices. Two orgs were created through the public RPCs; billing customers and invoices were inserted by SQL because the sandbox has no billing provider configured. deleted_at was then set by SQL. The same scenarios ran against main on the same database, with only the binary swapped.
# Scenario This branch main
1 Baseline before any soft delete: three invoices, counted two paid and one open PASS PASS
2 The soft-deleted invoice is not listed PASS FAIL
3 An invoice of a soft-deleted billing customer is not listed PASS FAIL
4 Only the live invoice is listed PASS FAIL
5 The group counts match the listing PASS FAIL
6 The soft-deleted organization reports no invoices PASS FAIL
7 An org id nobody owns is refused by authorization PASS PASS
8 The live organization still answers PASS PASS

On main the counts came back as two paid and one open while the list showed three rows, two of which should have been hidden.

SQL Safety

  • Values flow through ? placeholders, goqu.Ex{}, or goqu.Record{} — never fmt.Sprintf or + building a query that gets executed.
  • ToSQL() callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Never query, _, err := ….
  • No ? placeholders inside single-quoted SQL literals in goqu.L.
  • No new //nolint:forbidigo or // #nosec G20x annotations.

Both the listing and the group-by count read invoices and billing
customers through the live-row helpers, and join organizations so a
soft-deleted org reports nothing.
…-deleted rows

Seeds a soft-deleted invoice, one under a soft-deleted billing customer,
and one under a soft-deleted organization, then checks the listing and
the group counts.
@vercel

vercel Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
frontier Ready Ready Preview Sep 25, 2026 8:41am UTC

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: raystack/frontier/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: aa860714-d59d-4094-8d71-db8cceb3b378

📥 Commits

Reviewing files that changed from the base of the PR and between 1a8f1dd and 06a753d.

📒 Files selected for processing (3)
  • internal/store/postgres/org_invoices_repository.go
  • internal/store/postgres/org_invoices_repository_pg_test.go
  • internal/store/postgres/org_invoices_repository_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Organization invoice search now excludes invoices, customers, and organizations that are no longer active.
    • Grouped invoice counts now reflect active records only.
    • Organizations that are no longer active no longer appear to have invoices in search results.

Walkthrough

The organization invoice queries now exclude soft-deleted invoices, customers, and organizations. Updated query expectations and PostgreSQL integration tests cover invoice search and grouped counts.

Changes

Organization invoice queries

Layer / File(s) Summary
Apply live-row filtering to invoice queries
internal/store/postgres/org_invoices_repository.go, internal/store/postgres/org_invoices_repository_test.go, internal/store/postgres/org_invoices_repository_pg_test.go
The base and grouped queries join the matching organization and exclude soft-deleted invoices, customers, and organizations. Unit and PostgreSQL tests cover search results and grouped counts.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 06a75

The invoice listing and grouped counts are intended to exclude soft-deleted records. No actionable issue is established; the PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 06a75

The change narrows invoice search results to live invoices belonging to live customers and organizations. The existing organization access check and organization filter remain in place. No new exposure was identified, although security coverage is incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed read behavior is limited to invoice results and grouped counts for an authorized organization; the query changes narrow visibility rather than adding a route or widening its organization predicate.

Trust Boundaries and Controls

  • observed — The caller supplies an organization ID, but the existing RPC authorization checks that organization before the handler invokes the repository. The apparent public-entrypoint changes in the test files are test definitions, not new request handlers.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 36114276913

Coverage increased (+0.2%) to 52.281%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: 24 of 24 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 41172
Covered Lines: 21525
Line Coverage: 52.28%
Coverage Strength: 17.02 hits per line

💛 - Coveralls

This branch was successfully deployed

1 active deployment
Preview — 06a753d5 Deployed Sep 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants