# Moon ERP — Accounting Frontend Audit

Audit of `/home/moonui/public_html/moon-erp/src/app/` covering 25+ accounting feature components, 20+ services, 20 models, NgRx store slices. Severity: CRITICAL / HIGH / MEDIUM / LOW.

Methodology: read the most critical feature components in full (journal-entries, ledger, year-end-closing, bank-reconciliations, fixed-assets, receipt-vouchers, expenses, opening-balances, checks-issued, transfers, petty-cash), corresponding services, NgRx slices, route definitions, shared `data-table`, and print configs.

---

## CRITICAL Findings

### C1. Journal Entry form has NO currency / exchange_rate inputs (multi-currency drift)

File: `src/app/features/journal-entries/journal-entries.component.ts` lines 179-187, .html lines 178-233.

Model `JournalEntryLine` (`core/models/journal-entry.model.ts:1-28`) and `CreateJournalEntry` (`:54-68`) both expose `currency_id` and `exchange_rate` on each line. The line builder `createLine()` (lines 179-187) does NOT include either control. The HTML form has no column for currency or rate. The save payload (lines 314-326) drops them.

Consequence: For any non-base-currency account, the entry posts at face value with no FX context. This is silently wrong — multi-currency JEs cannot be created via the UI at all. If backend forces a default rate, history is wrong; if it rejects, users hit 422 with no UI explanation.

### C2. Floating-point comparison for "balanced" tolerance is too loose / inconsistent

- `JournalEntriesComponent.isBalanced` (line 205-207): `Math.abs(totalDebit - totalCredit) < 0.001`.
- `OpeningBalancesComponent.isBalanced` (line 93-95): `< 0.01`.

A 0.001 tolerance allows the user to save a JE off by 0.0009 — but the backend (decimal(12,3) per CLAUDE.md) tracks 3 decimals. Inconsistent thresholds between JE and opening-balances mean an opening balance accepted on UI (tolerance 0.01) may be rejected by backend or, worse, posted off-by-cent into retained earnings forever.

### C3. Date submission via `toISOString().split('T')[0]` causes one-day shift east of UTC

Files using this pattern: `recurring-entries.component.ts:198-199`, `bank-reconciliations.component.ts:134-136`, `fixed-assets.component.ts:271,293-294`, `opening-balances.component.ts:306`, payment-vouchers (likely mirror of receipt).

`toISOString()` converts local-midnight `Date` to UTC. For Saudi Arabia (UTC+3), users entering 2026-01-01 produce `2025-12-31` after toISOString. This posts **fiscal-year-end entries into the wrong fiscal year** — material accounting error.

Note: `journal-entries.component.ts:385-390` correctly uses local Y/M/D formatting via `formatDate()`. The pattern is mixed. Standardize on the local-only helper; ban `toISOString`.

### C4. Transfers form does not prevent from_account == to_account

File: `src/app/features/transfers/transfers.component.ts:66-73`.

Validators only require both fields. No cross-field validator ensuring `from_account_id !== to_account_id`. A user can transfer money to the same account — backend may accept and double-post or reject 422. No same-account guard.

### C5. No client-side guard against future-dated entries / closed period entries

JE form, receipt/payment vouchers, expenses, transfers, fixed-assets — none block submission when `date` falls in a closed fiscal period or a fiscal year where `is_closed=true`. Year-end-closing UI shows closed status but does not propagate to entry forms. Result: 422 from backend or draft entries that cannot be posted.

### C6. Journal Entry validation drift: amount = 0 lines, missing line description, double-zero rows

`createLine()` (line 179-187): `debit` and `credit` have NO validators. A line with `debit=0, credit=0` is accepted into the payload (lines 319-325). "isBalanced" check accepts a JE with two zero rows as balanced.

Also, account-line allows `debit > 0 AND credit > 0` simultaneously on the same row — accounting principle requires one or the other per line. Need line-level validator: exactly one of (debit, credit) must be > 0.

---

## HIGH Findings

### H1. `watchSave` pattern causes false-success / cross-action contamination

File: `journal-entries.component.ts:346-361` (replicated in `opening-balances.component.ts:216-227`, `checks-issued.component.ts:258-272`, `transfers.component.ts:119-131`).

Component subscribes to `selectJournalEntriesSaving` AFTER dispatch. NgRx dispatch is synchronous, so first emission is `true`. But if any concurrent JE action sets saving=false between them, `watchSave` resolves with a stale error/success. There is also no de-duplication — clicking "Post" twice opens two `watchSave` listeners both racing on the next saving=false flip; the second resolution applies the wrong success message.

Additionally `watchSave` reads `selectJournalEntriesError` with `take(1)` — if a previous action left `error` non-null and the current action succeeded without resetting `error`, success path shows an error toast.

Recommended: drop the pattern. Use ofType(success/failure) effect stream, or surface toasts from effects.

### H2. Subscriptions leak in every component (no takeUntilDestroyed / unsubscribe)

Out of 19 audited components, **zero** use `takeUntilDestroyed`, `takeUntil(destroy$)`, or consistent async pipe. Sample inline subscriptions never unsubscribed:

- `journal-entries.component.ts:151-158` — `accounts$.subscribe` and `fiscalYears$.subscribe` in ngOnInit.
- `receipt-vouchers.component.ts:121,125,131,135,139,143,149,159` — 8 unmanaged.
- `expenses.component.ts:107,112,117,121,125,130` — 6 unmanaged.
- `opening-balances.component.ts:107,191,217` — store selector subscriptions never closed.
- `bank-reconciliations.component.ts:78`, `fixed-assets.component.ts:101`, `checks-issued.component.ts:103,108` — leaks.

Long-running ERP sessions accumulate retained closures. More dangerously, when forms rebuild and old subscriptions remain alive, mutations on old form state race with new state.

Recommended: convert to `selectSignal` / `inject(DestroyRef)` + `takeUntilDestroyed` (Angular 21 idiomatic).

### H3. No client-side permission gating on action buttons

Every audited feature component. None inject `PermissionService` or check `hasPermission` on Approve / Post / Reverse / Cancel / Delete. Route is permission-guarded; mid-page actions are not.

Examples:
- `journal-entries.component.html:49-96`: Approve, Post, Reverse, Cancel, Delete rendered for any user with route access. A view-only user clicks "Post" → 403.
- `receipt-vouchers.component.ts:209,272,293`: delete/cancel/print exposed without permission check.
- `fixed-assets.component.ts:206,223,239,262`: delete, dispose, sell, runDepreciation exposed without check.
- `year-end-closing.component.ts:113,145`: execute / reopen always shown; reopen is destructive.

Per `PermissionService` (lines 1-50), `hasPermission(prefix)` is available. Bind `*ngIf="permissionService.hasPermission('accounting.journal-entries.approve')"` per action.

### H4. List staleness after row actions

Some flows update local items signal optimistically; others reload; others do neither.

- `journal-entries.component.ts:reloadCurrentPage()` reloads — fine.
- `bank-reconciliations.component.ts:166` — opens detail via getById; parallel `loadData()` is fire-and-forget — list and detail may diverge.
- `expenses.component.ts:onSave` mutates items optimistically with API response (line 195-199) — but `expense_category` (joined) and `journal_entry` may be missing if backend serializer didn't eager-load.
- `receipt-vouchers.component.ts:245-249,283`: optimistic local update — partner relation may be stale.
- `fixed-assets.component.ts:dispose/sell/runDepreciation` call `loadData()` — but during the half-second wait, UI shows pre-depreciation values then jumps.
- `opening-balances.component.ts:onSave` does NOT reload the list — `watchResult` shows success toast but no `loadPage()` call. New OB appears only on next page change.

### H5. Number input validation gaps

InputNumber `mode="decimal"` and 2 decimals — good. But:

- `journal-entries.component.html:201,204`: debit/credit `[min]="0"`. No max. No required. Comma vs dot: with Arabic locale toggle, PrimeNG InputNumber locale may produce dotless input where backend expects `.`.
- `payment-vouchers.component.ts:103` / `receipt-vouchers.component.ts:103`: `Validators.required, Validators.min(0.01)` — no max; a 100M-SAR voucher submits silently.
- `fixed-assets.component.ts:134-135`: `purchase_cost: [0, [required, min(0)]]` — allows 0-cost asset; depreciation will produce 0 per period (silent). `salvage_value` has no validators — can exceed `purchase_cost` giving negative depreciable base.
- `checks-issued.component.ts:93,95`: amount `min(0.01)` good, but no validator ensuring `due_date >= issue_date`. A check dated yesterday with due date last month is silently accepted.

### H6. Year-End Closing — no warning before execute, weak reopen confirm

`year-end-closing.component.ts:113-117`: `openExecuteDialog` resets form and opens dialog **without first calling `checkReadiness`**. User can fire "Execute Closing" against a year backend will reject because not all entries posted, learns only via 422 toast.

`reopenYear()` (line 145-172) only shows confirm dialog with FY name. Reopening reverses every closing entry — budgets vs actual, retained earnings, opening balances of following year all recompute. Destructive irreversible flow should require typing the FY name to confirm or a dual-approval flow.

### H7. Error toasts swallow validation errors (HTTP 422 detail)

Pattern in every error handler: `detail: err.error?.message || 'Failed'`.

Backend returns 422 with `{ message, errors: { field: [msg] } }`. Frontend NEVER walks `err.error.errors`. With multi-line forms (JE, opening balance, budget), users have no way to know which line broke. On 422 form values are preserved (good), but invalid field highlighting is not implemented.

### H8. Reverse Journal Entry uses today's date silently

`journal-entries.component.ts:283-295`: when reversing, the date is hardcoded `new Date().toISOString().split('T')[0]`. Two problems:
1. Same TZ bug as C3.
2. User is never asked for reversal date. Accounting convention often requires reversal in a specific period (next month's first day, or same date). Hardcoding today posts the reversal into a different fiscal period than intended.

### H9. Trial-balance based "account balance" labels can mislead voucher creation

`receipt-vouchers.component.ts:377-393` and `expenses.component.ts:283-298` fetch `/accounting/reports/trial-balance` and append `(balance)` to each cash/bank option label. This is **GL balance**, not "available cash" — for a bank account with outstanding checks it overstates. Users may rely on this to choose an account, leading to overdrafts. Drop the balance label or compute available = GL balance − outstanding `delivered` checks issued.

### H10. Voucher auto-approve setting fetched after form opens

`receipt-vouchers.component.ts:130-146`, `expenses.component.ts:112-127`: `autoApprove` flag and default accounts are fetched async at component init. If user opens "New" dialog before settings load, dialog uses `autoApprove=false` and no default account. On slow API this is racy and inconsistent.

---

## MEDIUM Findings

### M1. `any` types used in critical paths

Total `any` occurrences in audited files: 39. Notable:
- `journal-entries.component.ts:319` — `formValue.lines.map((line: any) => ...)`.
- `payment-vouchers.component.ts`, `receipt-vouchers.component.ts` — 8 each in settings handlers and event types.
- `expenses.component.ts` — 6 in settings handlers `(res: any)`.
- `opening-balances.component.ts` — 7 mostly in event handlers and the "invoice" form.
- `bank-reconciliations.component.ts:134` — `private fmtDate(d: any)`.

Bypasses type checking on payloads going to backend.

### M2. Account name display inconsistent

`journal-entries.component.html:184,193,196`: `optionLabel="name_ar"`, hardcoded — English-locale users see Arabic names. Component `getAccountName` (line 419-423) switches by lang correctly, but inline templates do not. Across components: account dropdowns variously use `name_ar`, `name_ar || name`, `${code} - ${name_ar || name}`. Inconsistent UX. Should be a shared `displayAccount(account, lang)` pipe.

### M3. Number formatting not locale-aware in Arabic

Every `| number:'1.2-2'` renders Latin digits regardless of language. Arabic-locale users often expect Eastern Arabic numerals. At minimum, use `formatNumber(value, locale)`. No currency symbol prefix — bare numbers; users can't tell SAR from USD without context.

Negative numbers: no special styling (no parentheses, no red color) anywhere. Reversals and asset losses display identically to positives. Audit/compliance issue.

### M4. Bank reconciliation `period_end >= period_start` not validated

`bank-reconciliations.component.ts:87-95`: required validators but no cross-field check. Same for `statement_date` relative to period. Reconciliation with end-date-before-start-date possible.

### M5. Fixed assets edit form does not block changes after capitalization

`fixed-assets.component.ts:189-197`: edit form reuses create form with no field disabling. Once depreciation has run (asset has `depreciation_entries`), changing `purchase_cost`, `useful_life_months`, `depreciation_method`, or `in_service_date` invalidates existing depreciation entries. Backend may reject, but UX should disable these fields when `depreciation_entries.length > 0`.

### M6. Filters in Ledger only client-side from query result

`ledger.component.ts:110-130`: Filters (account, date range, status) are passed to backend. Good. **But** Account Ledger and Trial Balance tabs do NOT filter posted vs draft — they show whatever the backend returns. If backend includes draft entries, reports are wrong. No UI control for "posted only".

Search defaults `perPage: 200` with no pagination — if results exceed 200, user silently gets a truncated set. Component does not detect or surface this.

### M7. Status badges and severity choices

`year-end-closing.component.ts:174-182`: `'closed'`, `'completed'`, `'active'` all map to `'success'` — but an active (open) fiscal year is different from a closed one. Semantics conflated.

`bank-reconciliations.component.ts:255-263` uses `'success'` for both `'completed'` AND `'approved'` — visually ambiguous.

### M8. Print configs don't show all required fields

`receipt-voucher-print.config.ts:81-99`: omits `currency.code` if absent → defaults `'SAR'` hardcoded (line 87). Bilingual print uses `lang === 'ar'` for notes. Missing: cost center, branch, created-by (signature line), tax registration number of partner (required by Saudi VAT regulations for receipts above threshold). No "copy" markings (Original / Copy 1 / Copy 2 / File Copy) — required by Saudi accounting practice and ZATCA-style e-invoicing.

### M9. Subscription chains in `receipt-vouchers.onSave` are nested; saving state is brittle

Lines 229-269: on success → if `autoApprove` → call `approve` inside success callback → on error of approve also call `showSuccess` BEFORE showing error. Net effect: when create succeeds but approve fails, user sees both green and red toasts in sequence, suggesting partial success. Real outcome: voucher created in `draft`. Toast wording should be explicit ("Created but approval failed").

### M10. NgRx slice covers only some accounting features; rest are signal-based

Per accounting CLAUDE.md, vouchers, expenses, revenues, year-end-closing, fixed-assets, bank-reconciliations, allocation-rules don't have NgRx slices and use component-level signals. Cross-component invalidation doesn't happen. Closing a fiscal year updates `fiscalYears` slice but doesn't broadcast to forms in other tabs. Two stale browser tabs may post into a closed year.

### M11. `selectUser().subscribe` for branch filtering happens AFTER initial data load

`receipt-vouchers.component.ts:121-124`: `loadData()` (line 163) runs unconditionally before `selectUser` resolves. For users with branch restrictions, initial load may include items they shouldn't see briefly. Backend should enforce, but UI shouldn't display first.

### M12. Recurring entries / entry templates: no execute confirmation UX detail

`recurring-entry.service.ts:execute(id)` exists per CLAUDE.md. `recurring-entries.component.ts:96-100` shows required `frequency`/`start_date` but no "next_execution_date" preview in UI. (Needs verification — partial read.)

---

## LOW Findings

### L1. Inline magic numbers and hardcoded strings
- `receipt-vouchers.component.ts:331`: `'2000-01-01'` as a beginning-of-time date.
- `journal-entries.component.ts:131,200`: `rows = 25` hardcoded.
- Currency `'SAR'` default in print config (line 87).

### L2. `console.log` not exhaustively audited; recommend Stop hook check.

### L3. Language change does not trigger form rebuild
`journal-entries.component.ts:144` subscribes to `onLangChange` with empty callback. Form template translate-pipes labels (OK) but inline `name_ar` (line 184) is not re-bound. Toggling to English keeps Arabic account names in dropdown.

### L4. Confirm dialogs lack "type to confirm" for destructive irreversible actions
- `delete` JE, `dispose` asset, `reopenYear`, `truncate accounts`, `cancel` posted voucher — all use plain confirmationService dialog. Best practice: require typing entity number for destructive irreversible flows.

### L5. Lazy loading is correct, route data is consistent
Every accounting route has `canActivate: [permissionGuard]` and `data: { permissions: [...] }`. Patterns consistent. Good.

### L6. NgRx slice quality solid
Journal-entries reducer is immutable, uses `@ngrx/entity`, separates `loading`/`saving`/`error`, `loaded` flag for cache. Reused across slices.

### L7. Service contract drift surface area
Spot-checked services match backend per OpenAPI. Did not verify every response shape; recommend zod schema validation at interceptor.

### L8. `account.service` listAll forces `per_page=200`
Standard. Fine. For very large COA (>5000 detail accounts) load is heavy. Consider tree-only endpoint for selects.

### L9. `getAccountName` falls back blank for missing accounts
`journal-entries.component.ts:419-423`: returns empty if id not in accounts list. Soft-deleted account references display blank — should show `#{id} (deleted)`.

---

## Strengths

- **Lazy loading and permission guards** consistently applied to every accounting route (`app.routes.ts`).
- **NgRx slice structure** is uniform (entity adapter, loading/saving/error/loaded flags). Effects use `exhaustMap` correctly to prevent duplicate dispatches.
- **listAll() auto-pagination** compensates for backend per_page=25 cap.
- **JE form balance check is visible** (`journal-entries.component.html:236-258`): totals row colored balanced/unbalanced with exact imbalance amount. Save button disabled when not balanced. Right UX.
- **Status-based action gating in templates**: JE template hides Edit/Approve/Delete for non-draft, Post for non-approved, Reverse for non-posted, Cancel only for draft/approved. Buttons gated — permission gating missing (H3).
- **Reactive forms** consistent across all CRUD dialogs.
- **Source navigation map** (`journal-entries.component.ts:79-104`) lets users click through to originating documents — excellent audit traceability.
- **Server-side pagination** wired correctly for high-volume tables (journal-entries, checks, fixed-assets, opening-balances).
- **Print infrastructure** in `shared/print/` with declarative configs is clean.
- **Bank-account/petty-cash branch filtering** implemented in voucher and transfer flows.

---

## Top Remediation Priorities (ordered)

1. **Fix date timezone bug (C3)** — replace every `toISOString().split('T')[0]` with local `formatDate(date)`. Single sweep.
2. **Add currency_id / exchange_rate to JE line form (C1)** — required to make multi-currency module functional.
3. **Add line-level validators (C6)** — exactly-one-of (debit, credit) > 0; reject zero-only lines; reconcile imbalance tolerance to 0.01 across JE + opening balances.
4. **Same-account transfer guard (C4)** + due-date >= issue-date (H5).
5. **Show 422 field-level errors (H7)** — shared helper mapping `err.error.errors` into form control errors and a summary toast.
6. **Permission gate the action buttons (H3)** — inject PermissionService or template directive `*hasPermission`.
7. **Replace `watchSave` pattern (H1)** — surface success/error from effects via Actions stream (`ofType(createSuccess, createFailure)`) and toast there.
8. **Convert all `.subscribe()` to `takeUntilDestroyed(this.destroyRef)` (H2)**.
9. **Block future-dated entries in closed periods (C5)** — load closed-period boundary once, set max-date on every date picker driving postings.
10. **Year-end closing pre-flight readiness (H6)** — execute dialog shows readiness summary inline before final confirm; "type FY name to reopen" for reopen flow.

---

## Items Needing Verification (not fully audited)

- Payment-vouchers full HTML matches receipt-vouchers (skimmed; sizes identical at 427 lines; likely mirror).
- Allocation-rules, reconciliation-rules, ar-ap, cash-movement components.
- Budgets monthly breakdown form (m1-m12) — checked validators but not full HTML.
- Recurring-entries `next_execution_date` UI.
- Reports component export accuracy (only visible page vs all rows). ExportService not inspected.
- Account list / chart-of-accounts CRUD form.
- AccountService truncate flow safety.
- Tax-rates and fiscal-years form validators full scan.
- Verify voucher service `error: any` does not eat HTTP error bodies.

This audit covers the highest-risk transactional flows (JE, ledger, vouchers, year-end, reconciliations, fixed assets, opening balances, checks/transfers) at full-read depth.