spouliot
91ed19c2b1
ApplyGiftCertificate posts DR 2500 Gift Certificate Liability / CR AR, but the AR recompute only subtracted payments and credit-memo applications — so the redemption's 2500 debit was recomputed while its AR credit was not, leaving the Trial Balance out of balance by the total gift-certificate amount redeemed and overstating AR on the Balance Sheet. Subtract GC redemptions from AR in both recompute engines: - FinancialReportService: Balance Sheet (gcRedeemedBs) and Trial Balance (gcRedeemedTb) - LedgerService: AR section (dated rows) and ComputePriorBalanceAsync (prior balance) AR Aging was already correct (uses BalanceDue, which includes GiftCertificateRedeemed). Adds a LedgerService regression test. Build clean; 292 unit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
194 lines
16 KiB
Markdown
194 lines
16 KiB
Markdown
# 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.
|
||
|
||
---
|
||
|
||
### O3 — Write-path account lookups omitted explicit `CompanyId` — **RESOLVED**
|
||
- Added `CompanyId` to every **GL posting-path** account lookup that determines where money posts:
|
||
- `InvoicesController` — all account-resolver helpers (`GetCheckingAccountIdAsync`,
|
||
`GetCustomerDepositsAccountIdAsync`, `GetSalesReturnsAccountIdAsync`, `GetCustomerCreditsAccountIdAsync`,
|
||
`GetArAccountIdAsync`, `GetBadDebtAccountIdAsync`, `ResolveSalesTaxAccountIdAsync`,
|
||
`GetSalesDiscountAccountIdAsync`, `GetGcLiabilityAccountIdAsync`) plus the bank-account and write-off
|
||
expense dropdowns (scoped via `_tenantContext`).
|
||
- `CreditMemosController` — Create/Apply/Void GL lookups (scoped via the in-scope `customer`/`invoice`/`memo`).
|
||
- `GiftCertificatesController` — Create, BulkCreate, Void GL lookups + `GetGcLiabilityAccountIdAsync`.
|
||
- `BillsController` — AP/expense account resolution that pre-fills `APAccountId` (Create + CreateFromPO).
|
||
- `DepositsController` and `JournalEntriesController.SalesTaxPayment` already scoped correctly.
|
||
|
||
### O4 — Sales-tax remittance could over-remit (drive 2200 negative) — **RESOLVED**
|
||
- `JournalEntriesController.SalesTaxPayment` (POST) now rejects any amount exceeding `taxAcct.CurrentBalance`
|
||
(0.005 rounding tolerance), so a typo can no longer push Sales Tax Payable into an abnormal debit balance.
|
||
|
||
Verification of O3+O4: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests` → **291 passed**.
|
||
|
||
### O5 — Gift Certificate Liability 2500 missing for new tenants / mislabeled on default company — **RESOLVED**
|
||
- **Root cause (same shape as O1):** 2500 is resolved by number as the GC liability
|
||
(`GiftCertificatesController`). The `AccountingGapsPhase2` migration seeded it for tenants existing at
|
||
deploy, but (a) the per-tenant seeder `SeedDataService.Accounts.cs` never created a 2500, so tenants
|
||
onboarded afterward had **no GC liability account** and GC GL postings silently no-op'd; and (b) the
|
||
default-company seeder `SeedData.cs` created 2500 as **"Long-Term Loan"**, so that company's GC
|
||
obligations were mislabeled (and the migration's `NOT EXISTS` guard skipped it).
|
||
- **Fix:**
|
||
- `SeedDataService.Accounts.cs` now seeds **2500 "Gift Certificate Liability" (IsSystem)**.
|
||
- `SeedData.cs` now seeds 2500 as GC liability and moves the long-term loan to **2900**.
|
||
- `EnsureSystemAccountsAsync` self-heals: renames any 2500 still named "Long-Term Loan" → "Gift
|
||
Certificate Liability" (preserving user renames) and ensures a 2500 exists.
|
||
- Migration `20260620002950_FixGiftCertificateLiabilityAccount`: moves long-term loan to 2900 where a
|
||
2500="Long-Term Loan" exists and no 2900 is present; relabels the mislabeled 2500; safety-net inserts a
|
||
2500 for any company lacking one. Non-destructive (no Id/number/balance changes); Down is best-effort.
|
||
- Verified on the dev DB: existing 2500 GC-liability rows preserved; no spurious accounts added; build
|
||
clean; migration applied; **291 unit tests pass**.
|
||
|
||
---
|
||
|
||
## Read-path account lookup sweep (Tier 1–3) — **RESOLVED**
|
||
Completed the app-wide defense-in-depth pass: every `Accounts` lookup in a controller now carries an
|
||
explicit `CompanyId` predicate, matching the standing rule in CLAUDE.md. ~19 lookups across 12 controllers:
|
||
- **Tier 1 (write-path validation):** `AccountsController` duplicate account-number check on Create/Edit.
|
||
- **Tier 2 (dropdowns/lists):** `AccountsController` (Index/year-end/parent dropdown), `BankReconciliations`,
|
||
`Bills` (bank list + receipt-scan + suggest), `Budgets`, `CatalogItems`, `Expenses`, `FixedAssets`,
|
||
`Inventory`, `JournalEntries` (chart dropdown), `Vendors`.
|
||
- **Tier 3 (`accountIds.Contains` display maps):** `JournalEntries` Details, `Reports` budget vs actual,
|
||
`VendorCredits` Details — scoped via the in-scope entity's `CompanyId` for uniformity.
|
||
`companyId` source per controller: `_tenantContext.GetCurrentCompanyId()` where available, else the
|
||
in-scope entity's `CompanyId`, else `_userManager` current user. Build clean; 291 unit tests pass.
|
||
|
||
---
|
||
|
||
## Whole-system re-audit (2026-06-19) — NEW findings, OPEN
|
||
|
||
Root cause shared by all three: the GL is kept two ways — **direct postings** via
|
||
`AccountBalanceService.Debit/CreditAsync` (authoritative `Account.CurrentBalance`) and **recomputes**
|
||
via `LedgerService` (drives `RecalculateAllAsync`) and `FinancialReportService` (drives the reports).
|
||
Every posting type must be mirrored in *both* recompute engines or balances diverge. Several postings
|
||
are **not** mirrored, so: (a) "Recalculate Balances" corrupts those accounts, (b) the Balance
|
||
Reconciliation report shows false drift, and in one case (O7) the **Trial Balance does not balance**.
|
||
O2 (Sales Discounts 4950) was the first instance found and fixed; these are the rest.
|
||
|
||
### O6 — Inventory consumption COGS not in either recompute — **MEDIUM**
|
||
- `JobsController:3019` and `InventoryController:1855` post **DR COGS / CR Inventory** when an inventory
|
||
item with both `CogsAccountId` and `InventoryAccountId` is consumed on a job. Neither `LedgerService`
|
||
nor `FinancialReportService` reads this (no JobUsage/InventoryTransaction COGS section).
|
||
- **Impact:** COGS understated (profit overstated) and Inventory asset overstated on P&L/Balance Sheet
|
||
relative to the posted `CurrentBalance`; a recalc wipes the effect; reconciliation flags drift. TB still
|
||
*balances* (both sides omitted). Only active for items with both account mappings set.
|
||
- **Fix direction:** add a COGS/Inventory section to both recompute engines driven by `InventoryTransaction`
|
||
consumption rows (needs the posted cost captured on the transaction, e.g. `TotalCost`), or — better —
|
||
post these via real `JournalEntry` lines so the JE recompute path already covers them.
|
||
|
||
### O7 — Gift-certificate redemptions credit AR but AR recompute omitted it — **RESOLVED**
|
||
- `InvoicesController.ApplyGiftCertificate:3137` posts **DR 2500 GC Liability / CR AR** (no Payment row,
|
||
no `AmountPaid` change — only `invoice.GiftCertificateRedeemed`). The **2500 debit IS** recomputed
|
||
(GC redemptions), but the **AR credit is NOT**: AR recompute = `sum(Total) − Payments − CreditMemoApplications`
|
||
in both `FinancialReportService` (BS line 358 / TB line 1129) and `LedgerService` (AR section).
|
||
- **Impact:** because only one side of the entry is recomputed, the **Trial Balance is out of balance by the
|
||
total GC redeemed**; Balance Sheet AR is overstated; recalc corrupts AR. (AR **Aging** is correct — it uses
|
||
`BalanceDue`, which includes `GiftCertificateRedeemed`.) Active for any company that redeems GCs on invoices.
|
||
- **Fix applied:** GC redemptions now subtracted from AR in both recompute engines — `FinancialReportService`
|
||
Balance Sheet (`gcRedeemedBs`) and Trial Balance (`gcRedeemedTb`), and `LedgerService` AR section + prior
|
||
balance. Mirrors the `cmApplied` treatment. Regression test
|
||
`GetAccountLedgerAsync_AR_GiftCertificateRedemption_CreditsAccountsReceivable`. Build clean; 292 tests pass.
|
||
|
||
### O8 — Written-off invoices misstated in AR recompute — **MEDIUM**
|
||
- `InvoicesController.WriteOff:1689` posts **DR Bad Debt / CR AR** for the balance due and marks the invoice
|
||
`WrittenOff`. In the recompute, written-off invoices' `Total` is still counted as an AR **debit**
|
||
(`arDebits` only excludes Draft/Voided), while `FinancialReportService` **excludes** their payments from AR
|
||
credits (Status != WrittenOff filter) and neither engine models the write-off CR — so a written-off invoice
|
||
shows its full `Total` as open AR on recompute. `LedgerService` and `FinancialReportService` also disagree
|
||
(Ledger does not apply the WrittenOff payment filter).
|
||
- **Impact:** AR overstated by written-off balances on recompute/reports; recalc corrupts AR; the two engines
|
||
disagree. Active when invoices are written off.
|
||
- **Fix direction:** treat `WrittenOff` consistently — exclude written-off invoices' `Total` from `arDebits`
|
||
(or model the write-off CR) and align the two engines.
|
||
|
||
**Architectural note:** these keep recurring because reports re-derive balances from source documents in
|
||
parallel with the live postings. The durable fix is to make **every** financial event post `JournalEntry`
|
||
lines and drive all reports/recompute from those lines alone (single source of truth), retiring the
|
||
per-document re-derivation. Worth considering before the ledger grows further.
|
||
|
||
## Status
|
||
Findings **O1–O5, O7, + the read-path sweep are resolved** on `dev`. **O6 and O8 remain OPEN** — both need the
|
||
JournalEntry approach (post a balanced JE at consumption/write-off time) plus a one-time historical backfill,
|
||
because neither event leaves a reliably recompute-able marker (O6: no flag distinguishing COGS-posting
|
||
reductions, and posted cost uses AverageCost while the transaction stores UnitCost; O8: the write-off's
|
||
bad-debt account and amount aren't stored as a ledger record, so mirroring only the AR side would re-break the
|
||
trial balance).
|
||
Original audit numbering #1–3/#5/#6/#8 remains unrecoverable (see top). Nothing merged to `master` yet.
|