feat(quotes): quotes module CRUD with server-computed totals (PMS-672) #453

Merged

longjacksonle merged 4 commits from feat/PMS-672-quotes-module-crud into main

2026-07-21 19:33:36 +02:00

longjacksonle commented

2026-07-21 19:30:07 +02:00

Owner

What

PMS-672, phase 2 of the PMS-670 Quotes epic. Adds a real quotes module: CRUD over the quote and its line items, with totals computed server-side.

Before this, the only code touching quotes was modules::approvals, which existence-checks the parent row so /quotes/{id}/approvals can hang off it. The entity had no CRUD of its own.

Routes

GET|POST /quotes · GET|PUT|DELETE /quotes/{id} · POST /quotes/{id}/lines · PUT|DELETE /quotes/{id}/lines/{line_id}

Design notes worth reviewing

Totals are never taken from the caller. CreateQuoteRequest has no subtotal or total field at all, and recompute_totals is the single place they are derived. Every mutating path funnels through it, including a tax_amount-only edit, which moves total without touching a line.

Two freeze rules, not one. is_frozen covers sent onwards plus cancelled: no staff write at all. allows_content_edit is deliberately narrower and confines content changes to draft and rejected. A quote in submitted or approved still has to advance through the workflow, but letting its figures move underneath the approver looking at them would make the approval meaningless.

Staff cannot forge the client's decision. is_staff_settable excludes every status owned by another actor or route: sent, accepted / declined, expired, and converted.

Cross-tenant guards on the FKs. assert_company_in_tenant / assert_contact_in_tenant reject foreign ids. The foreign keys alone are not enough, because FK checks bypass RLS, so a foreign company_id would otherwise produce a quote silently linked across tenants. Same guard and reason as assert_payment_term_in_tenant (PMS-333).

DELETE cancels, it does not delete. A quote that reached a customer is a commercial record, and the approvals rows referencing it (target='quote') would be orphaned by a hard delete.

RBAC is borrowed, not invented. The billing module gate plus RequireFinance, the same pair invoices, contracts, and rate-cards use. /api/v1/quotes is added to the RBAC coverage matrix.

Verify

9 integration tests plus 5 unit tests: numbering, computed totals, per-line recompute (including a negative discount line), cross-quote line 404, forged totals ignored on create and update, sent 409ing on all four mutations, content-freeze-at-submitted with rejected reopening, all five forged statuses 409, and full cross-tenant isolation (6 routes 404, list total 0, foreign company 400). The pre-existing /quotes/{id}/approvals surface still works against an API-created quote.

Two things worth knowing for anyone writing similar tests: the cross-tenant test enables the billing module for the second tenant deliberately, because otherwise the module gate 404s before the tenant check is reached and the test would pass vacuously; and PMS-138 binds login to (tenant_id, email) with a default-tenant fallback, so common::login cannot reach a non-default tenant and the test needs its own login helper.

Full suite and just check clean. One pre-existing unrelated failure: tests/readiness.rs::ready_returns_ok_when_db_reachable_and_infisical_unconfigured fails locally because the dev .env supplies Infisical vars while that profile is not running. Confirmed identical on a clean main tree with these changes stashed; CI does not set those vars.

Follow-ups

PMS-673 (send + portal accept/decline) is already open on top of this branch. Then PMS-674 (convert to Project) and PMS-675 (SPA).

## What PMS-672, phase 2 of the PMS-670 Quotes epic. Adds a real `quotes` module: CRUD over the quote and its line items, with totals computed server-side. Before this, the only code touching `quotes` was `modules::approvals`, which existence-checks the parent row so `/quotes/{id}/approvals` can hang off it. The entity had no CRUD of its own. ## Routes `GET|POST /quotes` · `GET|PUT|DELETE /quotes/{id}` · `POST /quotes/{id}/lines` · `PUT|DELETE /quotes/{id}/lines/{line_id}` ## Design notes worth reviewing **Totals are never taken from the caller.** `CreateQuoteRequest` has no `subtotal` or `total` field at all, and `recompute_totals` is the single place they are derived. Every mutating path funnels through it, including a `tax_amount`-only edit, which moves `total` without touching a line. **Two freeze rules, not one.** `is_frozen` covers `sent` onwards plus `cancelled`: no staff write at all. `allows_content_edit` is deliberately narrower and confines content changes to `draft` and `rejected`. A quote in `submitted` or `approved` still has to advance through the workflow, but letting its figures move underneath the approver looking at them would make the approval meaningless. **Staff cannot forge the client's decision.** `is_staff_settable` excludes every status owned by another actor or route: `sent`, `accepted` / `declined`, `expired`, and `converted`. **Cross-tenant guards on the FKs.** `assert_company_in_tenant` / `assert_contact_in_tenant` reject foreign ids. The foreign keys alone are not enough, because FK checks bypass RLS, so a foreign `company_id` would otherwise produce a quote silently linked across tenants. Same guard and reason as `assert_payment_term_in_tenant` (PMS-333). **`DELETE` cancels, it does not delete.** A quote that reached a customer is a commercial record, and the approvals rows referencing it (`target='quote'`) would be orphaned by a hard delete. **RBAC is borrowed, not invented.** The `billing` module gate plus `RequireFinance`, the same pair `invoices`, `contracts`, and `rate-cards` use. `/api/v1/quotes` is added to the RBAC coverage matrix. ## Verify 9 integration tests plus 5 unit tests: numbering, computed totals, per-line recompute (including a negative discount line), cross-quote line 404, forged totals ignored on create and update, `sent` 409ing on all four mutations, content-freeze-at-`submitted` with `rejected` reopening, all five forged statuses 409, and full cross-tenant isolation (6 routes 404, list total 0, foreign company 400). The pre-existing `/quotes/{id}/approvals` surface still works against an API-created quote. Two things worth knowing for anyone writing similar tests: the cross-tenant test enables the `billing` module for the second tenant deliberately, because otherwise the module gate 404s before the tenant check is reached and the test would pass vacuously; and PMS-138 binds login to `(tenant_id, email)` with a default-tenant fallback, so `common::login` cannot reach a non-default tenant and the test needs its own login helper. Full suite and `just check` clean. One pre-existing unrelated failure: `tests/readiness.rs::ready_returns_ok_when_db_reachable_and_infisical_unconfigured` fails locally because the dev `.env` supplies Infisical vars while that profile is not running. Confirmed identical on a clean `main` tree with these changes stashed; CI does not set those vars. ## Follow-ups PMS-673 (send + portal accept/decline) is already open on top of this branch. Then PMS-674 (convert to Project) and PMS-675 (SPA).

longjacksonle added 4 commits

2026-07-21 19:30:07 +02:00

feat(quotes): quote + line DTOs and the lifecycle status model (PMS-672) 5955c86793

Phase 2 of the PMS-670 Quotes epic. Adds the DTO layer over the schema PMS-671 landed: `QuoteStatus` and `QuoteLineType` mirroring their CHECK constraints, the request/response shapes, and the filter.

Three predicates on `QuoteStatus` carry the rules the rest of the module enforces, kept here so they are unit-testable and stated once. `is_frozen` covers the issued and terminal states (`sent` onwards, plus `cancelled`) where no staff write is legitimate at all. `allows_content_edit` is deliberately narrower than the inverse: a quote in `submitted` or `approved` still has to advance through the workflow, but its figures must not move underneath the approver looking at them, so content edits are confined to `draft` and `rejected`. `is_staff_settable` excludes every status owned by another actor or route (`sent`, the client's `accepted`/`declined`, derived `expired`, and `converted`), so a staff user cannot forge a client's acceptance with a plain header update.

`CreateQuoteRequest` deliberately has no `subtotal` or `total` field. Totals are derived from the lines server-side, so accepting one would only invite the caller to believe it mattered.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

feat(quotes): quotes service with server-computed totals (PMS-672) 2bcc11723e

CRUD over quotes and their line items. Mirrors `BillingService` throughout, since invoices are the same parent/child money shape: gapless per-tenant numbering via a row-locked `quote_sequences` bump inside the caller's transaction, `begin_with_tenant` on every method so RLS scopes the statements, and the same audit-write-in-the-same-transaction posture.

`recompute_totals` is the single place totals are derived, and every mutating path funnels through it, so a stored total always equals the sum of the quote's lines plus tax. It runs even when only `tax_amount` changed, because that moves `total` without touching a line.

`assert_company_in_tenant` and `assert_contact_in_tenant` reject ids belonging to another tenant. The foreign keys alone are not enough: FK checks bypass RLS, so a caller passing a foreign company id would otherwise get a quote that silently links across tenants. Same guard and same reason as `assert_payment_term_in_tenant` (PMS-333).

`DELETE` is modelled as a `cancelled` transition rather than a row delete. A quote that reached a customer is a commercial record, and the approvals rows referencing it (`target='quote'`) would be orphaned by a hard delete.

Unit tests pin the three status predicates and the string round-trips.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

feat(quotes): mount the quotes routes under /api/v1 (PMS-672) b630b68f40

Routes for the quote and its lines, merged rather than nested so the URLs stay flat and sit alongside the `/quotes/{id}/approvals` paths that `modules::approvals` already owns.

RBAC mirrors the other money-bearing surfaces exactly rather than inventing a policy: the `billing` module gate plus `RequireFinance`, the same pair `invoices`, `contracts`, and `rate-cards` use. PMS-350 established that financial surfaces are finance-gated on reads as well as writes, and a quote is a priced commercial document, so it belongs in that set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

test(quotes): integration coverage for CRUD, totals, freezing, tenancy (PMS-672)

E2E / Playwright against staging (pull_request) Successful in 40s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 1m53s

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

Integration / integration tests (pull_request) Successful in 5m57s

Details

9196a2bf80

Pins the guarantees the ticket calls out: CRUD round-trip with per-tenant number allocation, totals recomputed on every line mutation, a caller-supplied total never persisted, `sent` quotes rejecting every edit with 409, content freezing at `submitted` while the status still advances, staff unable to forge the client's decision, and quotes invisible across tenants on every route including the list.

The cross-tenant test enables the `billing` module for the second tenant on purpose: without it the module gate 404s before the tenant check is reached and the test would pass vacuously. It also needs its own login helper, because PMS-138 binds the login lookup to `(tenant_id, email)` and falls back to the default tenant, so `common::login` can only ever reach default-tenant users.

Also adds `/api/v1/quotes` to the RBAC coverage matrix next to the other financial surfaces, and asserts the pre-existing approvals surface still works against a quote created through the new API rather than a raw INSERT.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>