Fix Customer Deposits account mislabel and Sales Discounts recalc (audit O1, O2)

O1: account 2300 has always been used by the deposit GL code as the Customer
Deposits liability (resolved by number), but it was seeded/named "Payroll
Liabilities" for tenants the AccountingDepositsGL migration's NOT EXISTS guard
skipped — so the liability was mislabeled on the balance sheet. Rename 2300 to
"Customer Deposits" (IsSystem) and move payroll to a new 2400 account:
  - both seed paths (SeedDataService.Accounts, SeedData)
  - EnsureSystemAccountsAsync self-heal (renames only where still default-named,
    preserving user renames; ensures 2400 exists)
  - migration RenameDepositsAccountAddPayroll for existing tenants
Account number 2300 is unchanged, so the deposit posting code needs no changes.

O2: LedgerService never recomputed 4950 Sales Discounts, so "Recalculate
Balances" wiped it to JE-only and the Balance Reconciliation report showed false
drift. Add a 4950 section to GetAccountLedgerAsync and ComputePriorBalanceAsync
that reproduces the actual postings (invoice discounts DR + credit-memo issuance
DR, less the unapplied remainder of voided memos CR), matching AccountBalanceService.

Adds a LedgerService regression test for 4950. Documents both fixes plus the
remaining open findings (O3, O4) in docs/ACCOUNTING_AUDIT.md so the audit is no
longer lost. Build clean; 291 unit tests pass; migration applied.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-06-19 19:37:57 -04:00
parent 9532812b9f
commit 1005be0c9e
8 changed files with 11763 additions and 5 deletions
+108
View File
@@ -0,0 +1,108 @@
# Accounting System Audit
> Living record of the accounting/GL audit and its remediation status.
> **Keep this file updated** whenever an accounting finding is opened or closed — the
> original audit was lost once because it was never written down. Don't let that happen again.
Last reconciled against code: **2026-06-19** (branch `dev`, commits through `9532812`).
Verification at that point: `dotnet build` clean (0 errors); `dotnet test tests/PowderCoating.UnitTests`**290 passed, 0 failed**.
---
## How the GL is modeled (context for the findings)
Balances are computed two different ways, and they must agree:
1. **At posting time** — controllers call `AccountBalanceService.DebitAsync/CreditAsync`, which
mutate the denormalized `Account.CurrentBalance`.
2. **At report time**`FinancialReportService` (Trial Balance, Balance Sheet, P&L, AR/AP aging,
statements) and `LedgerService` (per-account ledger + `RecalculateBalances`) recompute balances
from source documents.
`LedgerService` also backs the **Balance Reconciliation report** (the detective control added in
`c2cd19e`), which compares stored `CurrentBalance` vs the ledger-recomputed balance. Any account
whose recompute path is incomplete will (a) be silently corrupted by a "Recalculate Balances" run
and (b) produce misleading output in the reconciliation report.
---
## Resolved findings (this audit batch)
| # | Finding | Fix | Commit | Verified |
|---|---------|-----|--------|----------|
| **#4** | `Sales Tax Payable` (2200) was credited on every invoice but never relieved — the liability grew forever (no remittance flow). | `JournalEntries/SalesTaxPayment` GET+POST: DR 2200 / CR bank, balanced, in a transaction, honors the period lock and validates amount + bank-account tenancy. | `0c921ba` | Posting reviewed `JournalEntriesController:235-296`. Reporting needs no change (posted JE lines already flow through TB/BS/ledger). |
| **#7** | Most report queries relied on the global tenant filter, which is **bypassed for SuperAdmin** → cross-company leakage on P&L / Balance Sheet / Trial Balance / aging / statements. | Explicit `CompanyId == companyId` predicate added to every query in `FinancialReportService`. | `9532812` | Confirmed: every query in `FinancialReportService.cs` carries an explicit `CompanyId` predicate. |
| **#9a** | A cash refund posted **DR AR** (control up) while the customer subledger went down — opposite directions → guaranteed AR drift. | Refund now **reverses the sale**: DR Sales Returns (4960) + DR Sales Tax Payable (tax portion) / CR bank; AR untouched. Split centralized in `Core/Accounting/RefundAllocation.Split`. | `2a82a1d` | Posting (`InvoicesController.IssueRefund`/`CancelRefund`) and both recompute paths (`LedgerService` §5b, `FinancialReportService` TB/BS/P&L) all use `RefundAllocation.Split` — they agree by construction. |
| **#9b** | Store-credit refunds & credit memos posted nothing to the GL on issue (only a `CreditMemo` + `Customer.CreditBalance`) → outstanding store credit invisible on the balance sheet. | New **2350 Customer Credits** liability. Issue: DR 4950 / CR 2350. Apply: DR 2350 / CR AR. Void remainder: DR 2350 / CR 4950. Updated in all 8 posting sites + TB/BS/P&L + ledger §12b. | `9ce3612` | Posting sites confirmed (`CreditMemosController` 180-185/262-270/314-320; `InvoicesController` store-credit path). Reporting `cmContraRevenue`/`cmIssuedNonVoided` math reviewed. |
| — | No detective control to catch denormalized-balance drift. | **Balance Reconciliation report** (`Reports/Reconciliation`): per-account stored vs recomputed, plus AR/AP control-vs-subledger. | `c2cd19e` | Reviewed `FinancialReportService.GetBalanceReconciliationAsync`. NOTE: its usefulness is limited by O2 below. |
### Original numbering gap
The commits reference findings **#4, #7, #9a, #9b** — implying a list of at least #1#9. The original
audit document was never persisted and is unrecoverable. Findings **#1, #2, #3, #5, #6, #8** cannot be
matched to commits and their status is **unknown**. If the original list resurfaces, reconcile it here.
---
## Resolved findings (2026-06-19 remediation)
### O1 — Account 2300 mislabeled "Payroll Liabilities" while used as Customer Deposits — **RESOLVED**
- **Root cause refined:** there is no payroll feature posting to 2300; the deposit GL code has always used
2300 (resolved by number) as the Customer Deposits liability. Migration `AccountingDepositsGL` already
renamed it to "Customer Deposits" for tenants that lacked a 2300, but its `NOT EXISTS` guard **skipped**
pre-existing tenants, leaving them mislabeled. The two seed files still created new tenants with the
wrong name.
- **Fix (chosen approach: rename 2300, move payroll to a new 2400):**
- Seed files now create **2300 = "Customer Deposits" (IsSystem)** and a separate **2400 = "Payroll
Liabilities"** — `SeedDataService.Accounts.cs`, `SeedData.cs`.
- `EnsureSystemAccountsAsync` self-heals: renames any 2300 still named "Payroll Liabilities" → "Customer
Deposits" (preserving user renames) and ensures 2400 exists.
- Migration `20260619233108_RenameDepositsAccountAddPayroll` does the same for existing tenants (rename
only where Name is still the default; insert 2400 where missing). Down is best-effort (soft-deletes
only empty 2400s; does not re-introduce the mislabel).
- Account **number 2300 is unchanged**, so the deposit posting code (`DepositsController`,
`InvoicesController`) needed no changes — its lookups are by `"2300"`.
### O2 — `LedgerService` never recomputed 4950 Sales Discounts → recalc corrupted it — **RESOLVED**
- **Fix:** added a 4950 section to both `LedgerService.GetAccountLedgerAsync` and `ComputePriorBalanceAsync`
that reproduces the *actual postings* so a balance recalc matches the stored balance:
invoice discounts (DR at invoice date) + credit-memo issuance (DR full amount at issue) the unapplied
remainder of voided memos (CR at void). This mirrors what `AccountBalanceService` posts at
`InvoicesController:849/1108/1629` and `CreditMemosController`, so the Balance Reconciliation report no
longer shows false drift on 4950.
- **Note / micro-discrepancy:** the ledger uses `memo.AmountApplied` for the voided remainder (matching the
true posting), whereas `FinancialReportService` TB/BS derive the voided portion from applications against
non-voided invoices. These differ only in the rare case of a voided memo applied to a since-voided
invoice. Left as-is (report-side nuance, not O2's target); documented here so it isn't mistaken for a bug.
- Regression test: `LedgerServiceTests.GetAccountLedgerAsync_SalesDiscounts4950_IncludesInvoiceDiscountsAndCreditMemoContraRevenue`.
Verification of O1+O2: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests`**291 passed**;
migration applied to the dev database successfully.
---
## Open findings (deferred — to do after O1/O2 per user)
### O3 — Write-path account lookups omit explicit `CompanyId` (defense-in-depth gap) — **LOW-MEDIUM**
- Finding #7 added explicit `CompanyId` predicates to the **read** path, but several **posting** lookups
still rely on the global filter alone, e.g. `CreditMemosController` (182-184, 268, 317-318) and
`InvoicesController` refund/credit account lookups use `FirstOrDefaultAsync(a => a.AccountNumber == "2350" && a.IsActive)`
with no `CompanyId`. `JournalEntriesController.SalesTaxPayment` (246) does it correctly.
- **Impact:** low in practice (these run under a non-SuperAdmin company context where the global filter
applies), but it violates the repo's standing rule and would post to the wrong tenant's account if ever
executed in a SuperAdmin/multi-company context.
- **Fix direction:** add `a.CompanyId == companyId` to every account lookup on the posting path.
### O4 — Sales-tax remittance can over-remit (drive 2200 negative) — **LOW**
- `SalesTaxPayment` validates amount > 0 but does **not** cap the payment at the outstanding 2200 balance.
- **Impact:** a typo can push Sales Tax Payable negative (a debit-balance liability), which is an
abnormal balance that then surfaces oddly on the balance sheet.
- **Fix direction:** show the current 2200 balance (already shown on the GET form) and reject/warn when
`amount` exceeds it.
---
## Suggested priority order
1. **O1** (mislabeled/commingled liability — real balance-sheet correctness issue)
2. **O2** (recalc corruption + reconciliation report reliability)
3. **O3** (defense-in-depth on write path)
4. **O4** (input guard)