feat(invoices): add deposit refund entries with negative claim adjustments

[steilerDev]

Summary

• Adds an entryType (deposit | refund) to invoice deposits so refunds can be recorded alongside deposit installments; immutable after creation.
• Refunds are entered as positive amounts and treated as negative contributions when computing the invoice's final payment amount — only paid/claimed refunds reduce it, pending refunds do not.
• Frontend gains a Deposit/Refund entry-type choice on the deposit form, negative-styled refund rows with a Badge, and refund-aware totals on the invoice list and detail pages.

Fixes #1876

Test plan

• Unit tests pass (95%+ coverage)
• Integration tests pass
• E2E tests pass (invoice deposits + invoices list flows)
• CI Quality Gates pass (typecheck, tests, build, audit)

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com
Co-Authored-By: Claude backend-developer (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude frontend-developer (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude translator (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) noreply@anthropic.com

[steilerDev]

[ux-designer] Design review of PR #1880 against the visual spec posted on issue #1876.

Verified against spec

Token adherence — clean. Every new declaration in InvoiceDepositsSection.module.css (.refund, .entryTypeSelector + children, .amountCell, .amountNegative) uses var(--token-name); no hardcoded hex/px/rgba anywhere in the diff. .refund uses --color-status-blocked-bg/-text, .amountNegative uses --color-danger-text-on-light — exactly as specced.

Entry-type radio group — matches spec verbatim: plain <input type="radio"> + role="group" + aria-label, no segmented-control invented, .entryTypeSelector CSS is a byte-for-byte port of AutoItemizePage.module.css .modeSelector. Radios are correctly disabled in edit mode (isMutating || isEdit) rather than hidden — I checked invoiceDepositService.ts's update path and entry_type is indeed immutable after creation (never in the update payload, and the migration has no update-path mutation), so this satisfies the "if entryType becomes immutable, disable rather than hide" branch of the spec correctly.

Refund badge + negative amount (table row & mobile card) — both DepositRow and DepositCard render the Badge (value="refund") only when entryType === 'refund', immediately followed by the amount span with .amountNegative, and formatCurrency(-deposit.amount) supplies the literal minus sign — two non-color channels as required, no reliance on red text alone. Status badges (Pending/Paid/Claimed) are reused verbatim with no relabeling — confirmed no "Refunded" status was added anywhere.

Accessibility — the OverflowMenu triggerAriaLabel fallback I flagged in the spec (deposit.description ?? 'deposit') is fixed correctly in both DepositRow and DepositCard: deposit.description ?? t(...entryTypeLabels.${deposit.entryType}) — a refund row with no description now announces "Refund" instead of the hardcoded "deposit". role="group" + aria-label present on the entry-type selector as specced.

Dark mode — --color-danger-text-on-light and --color-status-blocked-bg/-text both have correct dark overrides in tokens.css (lines 688, 730–731), same pairing already shipping for blocked/inactive states elsewhere. No new dark-mode risk.

Responsive/touch targets — no new breakpoint logic; .entryTypeSelector inline row with flex-wrap: wrap safety net as specced; radio <label> wraps input+text giving an effective hit target well over 44×44px. Table/card switch at 767px unchanged.

Effective Amount displays — finalPaymentAmount/.finalPaymentAmount keep their existing neutral token treatment (not recolored red), as specced — a computed remainder isn't an error state.

Adjudicated deviations — validated on their merits

1. .refund badge class lives in InvoiceDepositsSection.module.css instead of Badge.module.css. Checked Badge.tsx: it concatenates styles.badge (base, from Badge.module.css) with variant.className (caller-supplied) — the component is agnostic to which module the class comes from. More importantly, .statusPending/.statusPaid/.statusClaimed/.statusQuotation in this same file are already local classes, not in Badge.module.css — so .refund living alongside them isn't actually a deviation from an established pattern, it's consistent with the pre-existing local-badge-class convention this file already uses for every other variant. No objection.

2. New hidden-by-default "Effective Amount" column (effectiveAmount, bound to finalPaymentAmount) instead of touching "Remaining Amount". Confirmed remainingAmount still calls the untouched, itemization-based calculateRemaining() — genuinely unrelated data. effectiveAmount is defaultVisible: false with render: (inv) => formatCurrency(inv.finalPaymentAmount), same column-visibility convention already used by remainingAmount and notes. Both EN/DE i18n keys present (Effective Amount / Effektivbetrag). This avoids the regression the deviation was meant to avoid while staying visually consistent with the existing hidden-column pattern — correct call.

Findings

None — no hardcoded values, no dark-mode gaps, no accessibility regressions, no component-reuse violations. Implementation matches the spec closely enough that this was a fast review.

Verdict: APPROVED

[steilerDev]

[security-engineer] Security review of PR #1880 (story #1876, deposit refunds).

Scope reviewed: server/src/routes/invoiceDeposits.ts, server/src/services/invoiceDepositService.ts, server/src/services/shared/{money,depositAggregateUtils}.ts, server/src/errors/AppError.ts, migration 0040_invoice_deposit_entry_type.sql, shared/src/types/invoice.ts, and the downstream aggregate consumers (budgetServiceFactory.ts, budgetOverviewService.ts, budgetSourceService.ts, budgetBreakdownService.ts, invoiceService.ts).

1. entryType enum / injection
createDepositSchema restricts entryType to enum: ['deposit', 'refund'] at the AJV layer, and migration 0040 backs it with a DB-level CHECK(entry_type IN ('deposit','refund')). All queries go through Drizzle's parameterized query builder or tagged sql templates with bound params — no string concatenation of request data into SQL anywhere in the diff. No injection vector.

2. Immutability via removeAdditional
updateDepositSchema has no entryType property, and UpdateDepositRequest (shared/src/types/invoice.ts) doesn't include the field at the type level either — so this isn't just a schema-level strip, updateDeposit() never reads data.entryType even if it were present. Fastify's default removeAdditional: true silently drops an entryType sent in a PATCH body rather than 400ing (confirmed by the PR's own test invoiceDeposits.test.ts:750 "Scenario 5"). This is the established, previously-reviewed pattern for this codebase (see invoices.test.ts, invoiceBudgetLines.test.ts) — no new concern. Silently ignoring an unknown/immutable field here has no security impact: it's not a permission or type field that could be used to smuggle a privilege change, just a domain enum that the service layer independently refuses to honor.

3. Sum-invariant / money handling
exceedsAmount() rounds both sides to whole cents (Math.round(amount * 100)) before comparing, correctly avoiding IEEE-754 summation noise (ADR/issue #1806) without weakening the cap — a sum can't sneak past the invoice total via float drift. The read-check-write (SELECT SUM(...) then INSERT/UPDATE) is wrapped in a single db.transaction() callback using the same tx handle for both statements. Since better-sqlite3 is synchronous and Node is single-threaded, there's no way for interleaved JS to run between the check and the write within that callback — no TOCTOU window. This matches the transaction-safety pattern already verified elsewhere in this codebase.

4. Negative-fraction aggregation (refund abuse potential)
Confirmed the refund invariant is intentionally scoped independently: SUM(refund-type entries) ≤ invoice.amount, not ≤ SUM(paid deposit-type entries) (documented explicitly in wiki/API-Contract.md:3615 and covered by tests, e.g. depositAggregateUtils.test.ts "a claimed refund alone reduces (goes negative in) the claimed slice"). Practical effect: an authenticated user (admin or member — deposits have no requireRole gate, consistent with the rest of the invoice domain) can create a refund entry with no corresponding prior deposit and drive it to claimed, producing a negative contribution in a per-status aggregate slice (e.g. a "claimed" cost-breakdown filter) for that invoice/budget line. I verified this cannot push the invoice-level total below zero (Math.max(0, ...) in computeFinalPaymentAmount, and the fraction math in splitByDeposits caps combined positive contribution at exactly 1 before the refund fraction is subtracted), so this is not an integrity bypass of the invoice total — but it can surface a confusing/misleading negative dollar figure in a filtered UI view (e.g. the payment-status cost-breakdown filter added in PR #1786) for an invoice that never actually received a matching deposit.

This is a data-integrity/business-logic nuance rather than a classic OWASP vulnerability — no auth bypass, no cross-user data exposure, and all users in this single-tenant household app already hold equivalent trust to invoice/budget financial data. I'm flagging it Informational, not blocking: consider (in a follow-up, not this PR) validating SUM(refund) ≤ SUM(received deposit-type entries) if the intent is for refunds to always net against real money that came in, rather than against the invoice's face value.

5. Error responses
New error classes (RefundExceedsInvoiceError, DepositsExceedInvoiceTotalError, InvalidDepositStatusTransitionError, InvalidDepositDateForStatusError) only echo domain data the requesting user already has access to (invoice total, current sums, requested amount, headroom) — no stack traces, SQL fragments, or file paths in details. No leakage.

6. Misc

• No new npm dependencies (the package.json diff is just a formatting change to an existing override block from PR fix(deps): scope js-yaml override so gray-matter keeps patched 3.x #1854).
• No dangerouslySetInnerHTML/innerHTML/eval in the new/modified client code (InvoiceDepositsSection.tsx, InvoicesPage.tsx).
• GET/PATCH/DELETE all correctly scope deposit lookups to (invoiceId, depositId) via assertDepositBelongsToInvoice, preventing a mismatched invoiceId/id pair in the URL from resolving to an unrelated deposit.

No critical or high findings. One informational note above (item 4) for future consideration — does not block merge.

Verdict: APPROVED

[steilerDev]

[product-architect] Architecture review of PR #1880 (story #1876 — deposit refunds with negative claim adjustments).

Scope reviewed: migration 0040, Drizzle schema + shared types, the refund-aware aggregation design in depositAggregateUtils, every consumer SQL projection, error-code conventions, wiki accuracy (API-Contract.md / Schema.md), and test coverage adequacy. I did not re-review UI styling or E2E specifics.

Verified

Migration 0040 — correct. I ran the DDL against better-sqlite3 (3.53.1) directly to confirm rather than trust the pattern: ALTER TABLE ... ADD COLUMN entry_type TEXT NOT NULL DEFAULT 'deposit' CHECK(entry_type IN ('deposit','refund')) succeeds, backfills existing rows to 'deposit', and the CHECK is enforced on subsequent inserts. The documented ROLLBACK: ALTER TABLE invoice_deposits DROP COLUMN entry_type also succeeds despite the inline CHECK — so the rollback note is accurate (this differs from 0033, which conservatively documented a table rebuild; both are fine). The additive/defaulted design makes the migration a genuine no-op for all existing data.

Consumer SQL projection coverage — complete. I grepped exhaustively for every site that joins invoice_deposits. All of them now project entry_type AS deposit_entry_type: budgetBreakdownService (both WI and HI queries), budgetOverviewService, budgetSourceService (all four: claimed, unclaimed, discretionary remainder, discretionary no-source), budgetServiceFactory (resolveRelationsBatch, getInvoiceAggregates), and invoiceService.listAllInvoices. No stragglers. The ?? 'deposit' coalesce in both splitByDeposits and computeFinalPaymentAmounts is a sensible belt-and-braces default for the nullable LEFT JOIN column.

Type consistency. entryType is on InvoiceDeposit and CreateDepositRequest, and correctly absent from UpdateDepositRequest — the type system, the PATCH JSON schema, and the service (which only ever reads existing.entryType) all agree on immutability. InvoiceDepositEntryType is exported from shared/src/index.ts. snake_case DB / camelCase TS split is respected throughout.

Error-code conventions. REFUND_EXCEEDS_INVOICE is added to the ErrorCode union, AppError (400, consistent with the sibling DEPOSITS_EXCEED_INVOICE_TOTAL), both errors.json locales, and both wiki error tables. Splitting the invariant into two independently-capped sums with two distinct codes is the right call — a shared code would have made the availableHeadroom detail ambiguous.

No added query cost. computeFinalPaymentAmounts reuses the existing summaryRawRows from listAllInvoices; the list endpoint's finalPaymentAmount became correct without a second query. Good.

Test coverage is proportionate and well-targeted. What I specifically wanted to see is present: a zero-refund regression/identity test at every consumer boundary (depositAggregateUtils, budgetServiceFactory, budgetSourceService, budgetOverviewService, budgetBreakdownService, invoiceService), plus explicit coverage of the residual-fraction exclusion (a refund does not affect the residual contribution under the parent invoice status) and the deliberate invariant break (Σ totalAmount can be less than invoice.amount). That is exactly the shape of testing this change needed.

Wiki updated in the same PR. Both API-Contract.md and Schema.md are updated, including the removeAdditional: true silent-strip note on the PATCH endpoint. That note is accurate and matches established project behaviour (same convention already documented for invoiceAutoItemize and invoiceBudgetLines). Documenting a 200-with-strip rather than implying a 400 is the honest description of what the API actually does.

Findings (medium/low — none blocking)

1. (Medium) Two different "a refund is realized" thresholds coexist, and only one is documented.
computeFinalPaymentAmount counts a refund only when its status is paid/claimed. But splitByDeposits assigns every refund a negative fraction regardless of status, so a pending refund nets negatively into the pending status slice of budget aggregation. Concretely, for a 1000 pending invoice with a single pending refund of 200: the budget pending slice reads 800, while finalPaymentAmount reads 1000.

Both behaviours are individually defensible (splitByDeposits is internally symmetric — a pending deposit is +X in pending, a pending refund is −X — while finalPaymentAmount is deliberately cash-realized). But the wiki's "Refund Semantics" block states only "A pending refund has not yet returned money, so it does not reduce the final payment amount", which reads as a global rule and will mislead the next person to touch budget aggregation. Please extend that block to state the aggregation-side rule explicitly.

2. (Medium) The documented finalPaymentAmount formula omits the Math.max(0, ...) clamp, which is now materially reachable.
API-Contract.md documents invoice.amount - Σ(deposit-type) - Σ(refund-type where paid|claimed) with no clamp, but the implementation clamps at 0. Pre-refund this was near-unreachable (deposits were capped at the invoice total). With independent refund capping it is now easy to reach: 1000 invoice, 1000 in paid deposits, 500 claimed refund → true net is −500 (the vendor owes you 500), displayed as 0. Either document the clamp, or raise whether a net-credit invoice should surface as negative — I'd lean toward the latter being the more useful behaviour, but it is out of scope here.

3. (Medium/Low) Refund headroom is independent of what has actually been paid.
The invariant is Σ refunds ≤ invoice.amount, unrelated to deposit or invoice payment state. A full 1000 refund can therefore be recorded against a wholly unpaid, pending invoice. If that is intentional (keeping the two caps symmetric and simple, and trusting a <5-user self-hosted instance not to enter nonsense), fine — but it should be a stated decision rather than an emergent one. A one-line note in the Schema.md invariants would settle it.

4. (Low) actualCost is deliberately not reduced by refunds — assert this in the wiki, not only in tests.
computeDepositAwareAggregates keeps actualCost at the full itemized amount while netting actualCostPaid / actualCostClaimed. The test a claimed refund reduces actualCostClaimed and actualCostPaid, actualCost unaffected pins it, which is good, but the wiki says nothing. Since this makes actualCostPaid < actualCost on a fully-settled invoice — which reads as "not fully paid" to anyone scanning the budget view — it deserves an explicit line in the Refund Semantics block.

Findings 1, 2 and 4 are all the same fix: one consolidated paragraph in the Schema.md "Refund Semantics" section (plus the clamp in the API-Contract.md formula) covering aggregation-side behaviour, the clamp, and actualCost. Suitable for refinement rather than blocking this PR — the code is right, the documentation is merely thinner than the behaviour.

No API contract violations, no schema/type inconsistencies, no missed consumer projections, no naming or error-shape deviations, and no business logic that belongs elsewhere.

VERDICT: APPROVED

[steilerDev]

[product-owner] Product Owner review of PR #1880 against story #1876 — Deposit refunds with negative claim adjustments.

Acceptance Criteria Coverage

#	Acceptance Criterion	Verdict	Evidence
1	Entry type Deposit/Refund selectable; refunds stored positive, treated negative in aggregation	✅ Met	Radio group in AddEditDepositModal; entryType on CreateDepositRequest; amount > 0 still enforced at DB level; negation is display/aggregation-time only. Tests: InvoiceDepositsSection.test.tsx entry-type suite, invoiceDepositsApi.test.ts, route Scenarios 1–2, E2E Scenario 9
2	Refund renders as a negative row, reusing the deposit status lifecycle and labels	✅ Met	Refund Badge + .amountNegative on the amount span in both table row and mobile card; the Pending/Paid/Claimed status Badge is untouched — no relabeling, no 4th status. Tests: refund row rendering (table + card), E2E Scenario 12 (pending → paid → claimed → revert uses identical menu items and badges), dark-mode E2E
3	€10,000 invoice with a paid €1,500 refund shows €8,500 on list and detail	✅ Met (see Deviation 1)	Detail: finalPaymentRow now refund-aware server-side. List: new Effective Amount column bound to finalPaymentAmount. Tests: invoiceService Scenario 6 (detail + list), route Scenarios 6/7/8, E2E Scenario 9 asserts 2,000 → 1,500 after mark-paid, InvoicesPage effectiveAmount suite, E2E invoices.spec.ts
4	Refund reduces per-source allocations consistently across all consumers	✅ Met	deposit_entry_type wired into every deposit-joining query: budgetSourceService (claimed, unclaimed, discretionary ×2), budgetOverviewService, budgetBreakdownService (WI + HI), budgetServiceFactory (resolveRelationsBatch, getInvoiceAggregates), invoiceService.listAllInvoices. Each consumer has a dedicated refund test and a paired zero-refund regression test
5	Refunds €9,000 + new €2,000 on a €10,000 invoice → REFUND_EXCEEDS_INVOICE, no entry created	✅ Met	Sum invariant is now scoped per entry type; distinct RefundExceedsInvoiceError. Route Scenario 3 asserts 400, availableHeadroom: 1000, and that no row was inserted; service test + E2E Scenario 10 (inline headroom message, modal stays open)
6	Valid refund within the cap is accepted	✅ Met	Scenario 3 boundary case (refund exactly equal to total succeeds, next one fails) and Scenario 4 (deposit and refund caps are independent — €9,000 of each on a €10,000 invoice both succeed)
7	splitByDeposits gives a refund a negative fraction in its status slice	✅ Met	Scenario 9 plus four supporting cases; refunds are correctly excluded from residualFraction, including when a refund alone would exceed the invoice total
8	Already-claimed-then-refunded portion appears as a negative contribution in its slice	✅ Met	Scenario 10: claimed deposit fully offset by an equal claimed refund nets to 0; claimed refund alone goes negative; partial offset reduces without flipping. This is the input the negative claim-report line will consume downstream
9	No regressions for deposit-only invoices	✅ Met	Migration defaults existing rows to 'deposit'; explicit "regression: zero refunds" tests in depositAggregateUtils (×3), invoiceDepositService, invoiceService, budgetSourceService, budgetOverviewService, budgetBreakdownService, and budgetServiceFactory (×2), several asserting byte-identical output to the pre-#1876 formula

9 of 9 acceptance criteria met.

Adjudicated Deviations — validated, not re-litigated

1. Effective amount on the invoice list via a new hidden-by-default "Effective Amount" column. Accepted, and I think it was the right call. The existing "Remaining Amount" column answers a different question (invoice amount − Σ itemized budget lines); silently rebinding it to finalPaymentAmount would have changed the meaning of a column users already read, for a value that is not itemization at all. A separate, explicitly labelled column preserves both semantics. Hidden-by-default keeps the default table unchanged for the overwhelming majority of invoices that have no refunds. Binding verified correct (inv.finalPaymentAmount), label present in both locales (Effective Amount / Effektivbetrag), covered by unit and E2E tests including one asserting the two columns show distinct values for the same invoice.

2. entryType immutability on PATCH enforced by AJV stripping (200, not 400). Accepted. updateDepositSchema omits entryType and sets additionalProperties: false; Fastify's default removeAdditional: true strips it before validation. This matches the repo-wide convention for unknown PATCH fields, so a bespoke 400 here would be the inconsistent choice. The user-facing path is never silent: the edit modal renders both radios disabled with the current type checked (per the UX spec's explicit preference over hiding the field), and the edit payload excludes entryType entirely. Scenario 5b correctly confirms a body of only entryType is a 200 no-op rather than tripping minProperties. The behaviour is documented verbatim in the wiki API Contract and Schema pages.

Wiki accuracy: checked. The submodule bump (123d83a) documents the entry type, the refund-aware finalPaymentAmount formula, both per-type sum invariants, the REFUND_EXCEEDS_INVOICE code, the entry_type column, and both deviations above. No divergence from the implementation found.

Glossary Decision — APPROVED

"Refund": { "de": { "singular": "Rückerstattung", "plural": "Rückerstattungen" } }

Approved. "Refund" qualifies as a domain term rather than generic UI copy: it is a first-class entry type on the deposit entity and surfaces as a badge label, a radio option, and an error string. It sits directly alongside the existing Deposit → Abschlagszahlung entry, so pinning it prevents the two from drifting apart in future translations. Rückerstattung is the correct financial term and is properly distinct from Gutschrift (credit note), which is an accounting document and a different concept — picking it avoids a real ambiguity. Plural form is correct. Confirmed the German UI strings already use the glossary term consistently (entryTypeLabels.refund, refundAmountHint, refundExceedsTotal, REFUND_EXCEEDS_INVOICE).

Scope

Clean. No undocumented functionality. The package.json change is a Prettier reformat of an existing overrides block; the wiki bump and agent-memory updates are expected artifacts.

Non-blocking follow-ups (do not fix in this PR)

1. Label divergence for the same value. finalPaymentAmount is labelled "Effective Amount" in the list and "Final payment" in the detail section. Worth aligning during the Bank Report Wizard stories, when refunds become part of a user workflow rather than a data model.
2. "Effective Amount" is net of deposits too. With deposits present the figure is amount − deposits − received refunds, not "gross minus refunds". Accurate and documented, but the bare column header could be read as the latter. A tooltip or help text is a cheap addition in a later polish story.
3. Column is not sortable while "Remaining Amount" is. Understandable since finalPaymentAmount is computed rather than stored — flagging only so it is a conscious gap.
4. Process note for the orchestrator: at the time of this review no agent reviews (architecture, security, UX, QA) had been posted on the PR. Quality Gates is green. My approval covers product acceptance only and does not substitute for the required security review.

---

VERDICT: APPROVED

[github-actions]

🎉 This PR is included in version 2.13.0-beta.26 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀