Repository navigation
Fix credit/void/credit-exhaustion bugs; add a real journal (general ledger) - #2
Merged
Merged
Conversation
balance_due had three independent writers computing it three different ways: applyCreditNote() wrote balance_due - credit directly, while recordPayment() and Document::recalculateTotals() both recomputed it as total - amount_paid with no knowledge a credit had ever been applied — so the next payment, or even an unrelated line edit, erased the credit and the invoice could never reach 'paid'. Adds a credits_applied column and a single recalculateBalance() formula that all three writers now go through. The overpayment guard in recordPayment() is updated to account for credits_applied too. Also adds the Document::UNRECOGNISED_SALES_STATUSES constant used by the next commit.
income-statement, income-by-account and income-by-client all excluded sales invoices with status ['draft', 'cancelled'] — but this system never writes 'cancelled', only 'voided' (DocumentService::voidDocument()). So a voided invoice's revenue leaked into these three reports, while trial-balance and balance-sheet (which correctly exclude 'voided') disagreed with them. Replaces the stale literal with Document::UNRECOGNISED_SALES_STATUSES in all three files, and removes a dead 'cancelled' status tab label on the client detail page that advertised a state that can't occur.
LlmCreditExhaustedException extends LlmApiException, and the five
tier-fallback catches in LlmService caught LlmApiException|RuntimeException
— so when credits ran out on the fast model, the exception was swallowed
and the code immediately retried on the strong model, which failed the
same way. Every document processed after credit exhaustion burned two
failed calls instead of stopping.
Adds a narrow LlmUnusableOutputException for "output can't be used, try
a stronger model" (thrown by parseJsonResponse() instead of a bare
RuntimeException), and adds an explicit catch (LlmCreditExhaustedException)
{ throw $e; } ahead of each of the five fallback catches so the credit
sentinel always propagates instead of being caught by LlmApiException.
Merlin never wrote down a posting — every report re-derived debits and credits from document headers, lines, and status strings, each with its own copy-pasted rules. That drift is what produced the voided-invoice revenue bug fixed earlier on this branch, and project memory already records two prior instances of the same class of bug. Adds journal_entries/journal_lines (append-only, never updated or deleted) and JournalService as the single writer: post() rejects any entry whose debits and credits don't balance, is idempotent per (document_id, source), and reverse() unwinds an entry by posting a mirrored one rather than mutating history. Wires real postings into DocumentService at every event that changes what's owed: markAsSent()/voidDocument() (sales revenue, reversed on void), post()/postAutonomously() (purchase expense), the three places a payment Document gets created (bank-statement settlement, manual purchase payment, manual sales payment via BillingService), and applyCreditNote(). Postings are silently skipped — not an error — when a document's receivable/payable account or a line's income/expense account isn't configured yet, mirroring the whereNotNull() guards the old report queries already relied on; VAT-registered totals without a configured tax_liability_account_id still fail loudly, since that's a real Settings gap rather than an in-progress document. Also removes the headroom clamp added earlier to applyCreditNote() (see prior commit on this branch): a credit note's amount is now stored uncapped in credits_applied, matching the original code's behaviour and letting recalculateBalance()'s floor-at-zero do the work — this also keeps the credit-note journal entry's debit/credit sides trivially equal without needing to prorate a partial credit across income accounts. New tests: JournalServiceTest (balance enforcement, idempotency, reversal), LedgerIntegrityTest (a mixed sales/purchase/payment/void/ credit fixture proves total debits equal total credits end to end), and FinancialYearServiceTest (previously uncovered — every report's date window depends on it).
…ournal
trial-balance and balance-sheet each ran six accumulator queries in PHP,
re-deriving debits and credits from document headers/lines with their
own copy-pasted status filters. Both collapse to one grouped query over
journal_lines/journal_entries now — a voided invoice's reversal is just
another journal entry, so it nets to zero automatically with no status
filtering needed in the report at all.
accounts/show's 6-way UNION across document_lines/documents becomes a
single query against journal_lines, with "contra account" resolved via
a correlated subquery: an entry's one other line when there is exactly
one, else NULL ("Various" — unchanged from the old header-row behaviour
for entries spanning multiple accounts, e.g. a sales invoice's income
lines).
Learned along the way that this codebase's 'date' Eloquent casts store
a full Y-m-d H:i:s value (a general Eloquent behaviour, not particular
to journal_entries) — every existing date-column query in this app
already compensates with whereDate() rather than a raw comparison, and
the new journal queries now do the same instead of the plain where()
the fix plan had assumed.
Added descriptions to the journal lines DocumentService posts ("Invoice
total", "Payment received"/"Payment made") so the account register
reads the same as it did before the rewrite, and added a visited-set
guard to AccountBalanceRollup's parent-walk loop — a parent_id cycle
(a reparenting mistake) would otherwise hang every report with no error.
Three existing tests exercised documents that were never actually
transitioned through DocumentService (status set directly via factory,
or a line left without an account_id) — the old report read straight
off document headers regardless, so this went unnoticed. Fixed the
fixtures to go through the real markAsSent()/post() flow instead of
loosening what the journal will post, since an unposted or unallocated
document has no confirmed entry to show.
…eports alone income-statement collapses to one grouped query over journal_lines per account type (income = code 4, expense = code 5), replacing the duplicated $fetchLines() calls that filtered document headers/lines by type and status separately for each of YTD and month. The net has to be debit-minus-credit (or the reverse for expenses), not a one-sided SUM: JournalService::reverse() unwinds a voided invoice by swapping each line's debit and credit rather than negating one side, so a one-sided sum leaves the reversal with no visible effect — caught by a test asserting revenue actually goes back to zero after a void, not just that the report doesn't throw. income-by-account, income-by-client, expenses-by-account and expenses-by-supplier are deliberately left reading document headers/lines directly rather than journal_lines. Each needs data the journal doesn't preserve at that granularity — invoice counts, the outstanding balance_due, or per-line VAT split out from the invoice total — and neither report has the drift risk the migration exists to fix: sales-side status filtering already excludes voided invoices directly (fixed in an earlier commit on this branch), and purchase invoices only ever move forward through posted/partially_paid/paid, so there's no reversal case to get wrong. Forcing these onto the journal would mean either losing that detail or joining both models for no correctness benefit.
The last piece this migration was missing: nothing stopped a posted invoice's lines from being edited after the fact through the normal inline-edit UI, silently leaving the journal entry already posted for the old amounts wrong with no error and no way to notice — the one thing an append-only ledger must not allow. DocumentLine::saving()/deleting() now refuse to touch a line once its document has a non-reversed JournalEntry. Bypassed by saveQuietly(), which the FX-rate-finalisation path in DocumentService::recordPayment() and PaymentNotificationMatcher::applyCorrectedAmount() already use — both only ever touch lines on invoices confirmed not yet posted, so this doesn't change their behaviour, just makes the same restriction apply to a normal save()/delete() too.
CLAUDE.md gets a new "Journal (General Ledger)" section covering
JournalService as the single writer (post()'s balance/idempotency
guarantees, reverse()'s swap-not-negate semantics and why reports must
sum credit-minus-debit rather than one side alone), the write points in
DocumentService, the DocumentLine immutability guard on posted
documents, and which reports read the journal versus documents/lines
directly and why (invoice counts, balance_due, and per-line VAT have no
journal equivalent). Also documents the credits_applied/
recalculateBalance() convention and the Eloquent 'date'-cast quirk
(stores a full datetime, so whereDate() is required) that came up while
building this.
Updates the Pages table (adds the previously-undocumented /accounts/{id}
route, corrects each report's data source) and the Domain Structure/
morph-map counts to include the new Accounting models, services, and
exceptions. README's module list gets the same one-line addition.
The system guide described the pre-journal architecture as current fact — "there is no separate ledger/journal table", balances "computed at query time from seven sources", and the account transaction view's rows marked "header" for anything sourced from a document total rather than a line. All three are now wrong, so the help chatbot (which reads these pages verbatim via docs:sync) would have confidently explained an architecture that no longer exists. - New system-guide/journal.html: what the journal is and why, entry shape, append-only + reversal semantics (and the swap-not-negate gotcha that trips up any report summing only one side), the posted- document immutability guard, the write points, and which reports read the journal versus documents/document_lines directly and why. Added to the Accounting nav group. - reports.html: rewrites the Income Statement/Balance Sheet/Trial Balance sections to describe the actual journal-backed queries, and states plainly why the four by-account/by-client/by-supplier reports deliberately stayed off the journal (invoice counts, balance_due, and per-line VAT have no journal equivalent). - chart-of-accounts.html: rewrites "Account transaction view" — no more header badge; explains the correlated-subquery contra-account resolution (one other line → that account; more than one → "Various"). - document-lifecycle.html, architecture.html: note that posting now writes a real journal entry and makes the invoice's lines immutable; add JournalEntry/JournalLine/JournalService to the module structure, services list, and morph-map count (also corrects that count, which had drifted stale independently of this change). - user-guide/accounts.html, sales-invoices.html: drop the now-false "header" badge line; state that voiding an invoice reverses its revenue out of every financial report. - content-outline.md: adds the journal.html authoring outline entry. Ran php artisan docs:sync to regenerate storage/app/docs/*.md (the chatbot's cache; not committed, per existing convention) and confirmed against the pest suite — HelpChatTest and the full suite both still pass (661, unchanged).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three live money bugs found in an adversarial review, then builds the general ledger those bugs kept resurfacing from.
balance_duehad three independent writers computing it three different ways; a payment or even an unrelated line edit would erase an already-applied credit. Fixed with acredits_appliedcolumn and a singleDocument::recalculateBalance()formula every writer now goes through.['draft', 'cancelled']— a status this app never writes (it writesvoided). Fixed withDocument::UNRECOGNISED_SALES_STATUSES.LlmCreditExhaustedExceptionwas being swallowed by the tier-fallback catch and immediately retried on the strong model. Fixed with a narrowerLlmUnusableOutputExceptionand explicit re-throws.journal_entries/journal_lines(append-only) andJournalServiceas the single writer — balanced-or-reject, idempotent per(document_id, source), reversal by swapping debit/credit rather than mutating history. Wired real postings into every event that changes what's owed, rewrotetrial-balance,balance-sheet,income-statement, and the per-account transaction register onto it, and made posted documents' lines immutable.income-by-account/income-by-client/expenses-by-account/expenses-by-supplierdeliberately stay ondocuments/document_lines— they need invoice counts,balance_due, and per-line VAT the journal doesn't preserve at that granularity, and neither has the reversal-drift risk the journal exists to fix.Full rationale, including two bugs in my own first pass that tests caught before they landed (a one-sided
SUM()that made reversals invisible to the income statement, and a wrong assumption about how Eloquent storesdate-cast columns), is in the commit messages and inCLAUDE.md's new "Journal (General Ledger)" section.Test plan
php artisan test— 661 passing (baseline was 632; +29 new, 0 removed)vendor/bin/pint --dirtyclean on every commitphp artisan migrate:fresh --seedagainst real MariaDB (not just the SQLite test DB)markAsSent()→ journal entry created and balanced →voidDocument()→ entry reversed — all confirmed, then rolled back cleanly/reports/*and/accounts/{id}in a browser against real data (not done in this session — no live data existed to exercise beyond the smoke test above)🤖 Generated with Claude Code
https://claude.ai/code/session_016hMMJ8syuwMUHfRZ8hkFVD