# CoreERP Audit Report
**Date:** 2026-06-29  
**Audited By:** Claude Code (AI Audit — read-only inspection)  
**Scope:** Phase 4 Sales Engine audit  
**Status:** No files modified during this audit

---

## A. Project Environment

| Property | Value |
|---|---|
| **Laravel Version** | v13.9.0 |
| **PHP Version** | 8.4.4 |
| **Auth System** | Laravel Breeze (email/password) |
| **Admin Template** | AdminLTE v3.16.0 (jeroennoten/laravel-adminlte) |
| **Permission Package** | spatie/laravel-permission v7.4.1 |

**Key installed packages:**

| Package | Purpose |
|---|---|
| `laravel/framework` v13.9.0 | Core framework |
| `jeroennoten/laravel-adminlte` v3.16.0 | Admin UI shell |
| `spatie/laravel-permission` v7.4.1 | Role/permission management |
| `laravel/breeze` v2.4 (dev) | Auth scaffolding |
| `laravel/tinker` | REPL |
| `laravel/pint` | Code style |

---

## B. Route Audit

### Route Structure

The entire ERP is wrapped inside one middleware group:

```
Route::middleware(['auth', 'active.user', 'branch.user', 'audit'])
```

This correctly enforces:
- `auth` — must be logged in
- `active.user` — account not deactivated
- `branch.user` — user must have a branch assigned
- `audit` — POST/PUT/PATCH/DELETE requests are logged

### Route Groups Summary

| Group | Middleware | Status |
|---|---|---|
| Dashboard | auth, active.user, branch.user, audit | OK |
| Branches, Users, Roles | + permission:*.view | OK |
| Customers, Suppliers | + permission:*.view | OK |
| Warehouses, Units, Categories, Items, Aliases | + permission:stock.view | OK |
| Stock adjustments/transfers | + permission:stock.adjust/transfer | OK |
| Quotations | + permission:sales.view | OK |
| **Proformas** | **NO permission middleware** | **CRITICAL BUG** |
| AJAX routes (items, customers, suppliers, prices, alias) | auth, active.user, branch.user, audit | OK (inside group) |

### Critical Route Finding — Proformas Missing Permission Middleware

In `routes/web.php` lines 135–137:
```php
// Route::resource('proformas', ProformaInvoiceController::class)
//     ->middleware('permission:sales.view');

Route::resource('proformas', ProformaInvoiceController::class);
```

The permission middleware for proformas was **commented out** and the uncommented version has **no permission middleware at all**. Any authenticated user with a branch can access proforma routes.

### Specific Routes Check

| Route | Name | Status |
|---|---|---|
| GET /ajax/customers/search | ajax.customers.search | OK |
| GET /ajax/items/search | ajax.items.search | OK |
| GET /ajax/prices/lookup | ajax.prices.lookup | OK |
| POST /ajax/customers/store | ajax.customers.store | OK |
| POST /ajax/item-aliases/store | ajax.item-aliases.store | OK |
| GET /ajax/main-items/search | ajax.main-items.search | OK |
| GET /ajax/suppliers/search | ajax.suppliers.search | OK |
| POST /quotations/{id}/approve | quotations.approve | OK |
| POST /quotations/{id}/cancel | quotations.cancel | OK |
| POST /quotations/{id}/convert-to-proforma | quotations.convert-to-proforma | OK |
| POST /proformas/{id}/approve | proformas.approve | OK |
| POST /proformas/{id}/cancel | proformas.cancel | OK |

### Route Naming Bug in View

`resources/views/quotations/show.blade.php` line 57:
```blade
action="{{ route('quotations.convert.proforma', $quotation) }}"
```

The actual route name is `quotations.convert-to-proforma` (with hyphen, not dot). This will throw a **RouteNotFoundException** at runtime when the Convert to Proforma button is clicked.

---

## C. Database and Migration Audit

### Migration Order (chronological)

| Order | Migration | Table | Notes |
|---|---|---|---|
| 1 | 0001_01_01_000000 | users | Laravel default |
| 2 | 0001_01_01_000001 | cache | Laravel default |
| 3 | 0001_01_01_000002 | jobs | Laravel default |
| 4 | 2026_05_19_150029 | branches | OK |
| 5 | 2026_05_19_150122 | users.branch_id | OK |
| 6 | 2026_05_19_150157 | audit_logs | OK |
| 7 | 2026_05_19_150600 | permission tables | OK |
| 8 | 2026_05_29_094722 | customers | Base table — without ownership fields |
| 9 | 2026_05_29_105208 | suppliers | OK |
| 10 | 2026_05_30_104111 | warehouses | OK |
| 11 | 2026_05_30_105032 | units | OK |
| 12 | 2026_05_30_110046 | item_categories | OK |
| 13 | 2026_05_30_111327 | items + item_aliases | Same timestamp — risky ordering |
| 14 | 2026_05_30_124152 | stock_movements | OK |
| 15 | 2026_06_01_102714 | warehouse_item_balances | OK |
| 16 | 2026_06_02_103807 | quotations | OK |
| 17 | 2026_06_02_103811 | quotation_items | OK |
| 18 | 2026_06_02_105225 | customers.document_prefix | Add-on migration |
| 19 | 2026_06_02_105403 | customer_document_sequences | Legacy/orphaned table |
| 20 | 2026_06_02_123621 | customers.created_by etc | Add-on migration |
| 21 | 2026_06_02_142812 | proforma_invoices | Base table |
| 22 | 2026_06_02_142816 | proforma_invoice_items | OK |
| 23 | 2026_06_03_114343 | customer_item_prices | OK |
| 24 | 2026_06_24_132023 | document_sequences | New global sequence table |
| 25 | 2026_06_25_150609 | proforma_invoices approval fields | Add-on migration |

### Migration Issues Found

**1. Identical timestamp on items and item_aliases (Migration #13)**  
Both `create_items_table.php` and `create_item_aliases_table.php` share the same timestamp `2026_05_30_111327`. Laravel runs them in alphabetical order. `create_item_aliases_table` (a) runs before `create_items_table` (i). The item_aliases table has a foreign key to items — this will **fail on fresh migrate** because items does not yet exist.

**2. `customer_document_sequences` table is ORPHANED**  
Migration `2026_06_02_105403_create_customer_document_sequences_table.php` creates a `customer_document_sequences` table. A corresponding model `CustomerDocumentSequence` exists. However, `DocumentNumberService::customerDocumentNumber()` now just calls `documentNumber()` using the global `document_sequences` table instead. The customer sequence table and model are dead code and will cause confusion. This also conflicts with the project decision to use a system-wide (global) numbering sequence.

**3. `document_prefix` field on customers is unused in numbering**  
Migration `2026_06_02_105225` adds `document_prefix` to customers. The Customer model and CustomerAjaxController accept/store this field. However, `DocumentNumberService` never reads `document_prefix`. This field has no functional effect on document numbers at present.

**4. Proforma invoice base migration missing approval columns**  
The base migration `2026_06_02_142812` does not include `approved_by`, `approved_at`, `cancelled_by`, `cancelled_at`, `cancellation_reason`. These are added later in `2026_06_25_150609`. The ProformaInvoice model lists all five in `$fillable`. This is safe since the later migration runs after, but the separation is messy for new installations.

**5. `document_sequences` is branch-scoped, not system-wide**  
The new `document_sequences` table has a unique constraint on `(branch_id, document_type)`. `DocumentNumberService::documentNumber()` scopes sequences by `auth()->user()->branch_id`. This means numbering is **per-branch** (e.g. Branch A has QI-000001, Branch B also has QI-000001). This contradicts the project spec which says "system-wide continuous sequence per document type." If a single-branch setup, this works. For multi-branch it will create duplicate numbers across branches.

### Column Coverage Check

| Table | Required Field | Status |
|---|---|---|
| quotations | branch_id, customer_id, salesman_id, quotation_number, status, totals, approved_by | OK |
| quotation_items | branch_id, quotation_id, item_id, item_alias_id, quantities, prices, line_total, salesman_profit | OK |
| proforma_invoices | branch_id, customer_id, quotation_id, salesman_id, proforma_number, status, totals, approval/cancel fields | OK (after migration 25) |
| proforma_invoice_items | All required fields | OK |
| customer_item_prices | branch_id, customer_id, item_alias_id, last_selling_price, source | OK |
| document_sequences | branch_id, document_type, prefix, last_number | OK |
| customers | created_by, normalized_name, normalized_phone | OK (after migration 20) |

---

## D. Model Relationship Audit

### Customer Model
- **Relationships:** `creator()`, `quotations()`, `proformas()` — all correct
- **Missing:** No `$casts` — `is_active` should be cast to boolean, `credit_limit` to decimal
- **Missing:** `customerItemPrices()` relationship not defined (minor, not causing errors yet)
- **`BelongsToBranch` trait:** Applied — branch global scope and auto-assignment active

### Item Model
- **Relationships:** `warehouse()`, `category()`, `unit()`, `aliases()`, `warehouseBalances()` — all correct
- **`BelongsToBranch` trait:** Applied
- **Missing:** `$casts` for `is_active` (boolean), `current_quantity`, `cost_price`, `selling_price` (decimal)
- **Missing:** `stockMovements()` relationship not defined

### ItemAlias Model
- **Relationships:** `item()` — correct
- **Missing:** Reverse relationship on Item exists (`aliases()`), but ItemAlias has no `quotationItems()` or `proformaItems()` back-reference
- **Missing:** `$casts` for `is_active`, `is_default` (boolean), `selling_price` (decimal)

### Quotation Model
- **Fillable:** Complete — includes `approved_by`, `approved_at`
- **Casts:** `quotation_date`, `valid_until` (date), `approved_at` (datetime) — OK
- **Relationships:** `customer()`, `salesman()`, `approver()`, `items()` — all correct
- **Missing:** `proformas()` relationship — useful for checking if quotation was already converted
- **`BelongsToBranch` trait:** Applied

### QuotationItem Model
- **Fillable:** All fields present
- **Relationships:** `quotation()`, `item()`, `itemAlias()` — correct
- **Missing:** `$casts` for `quantity`, `selling_price`, `line_total` etc (decimal)
- **`BelongsToBranch` trait:** Applied

### ProformaInvoice Model
- **Fillable:** All fields including approval/cancel fields — correct
- **Casts:** `proforma_date` (date), `approved_at`, `cancelled_at` (datetime), totals (decimal:2) — good
- **Relationships:** `customer()`, `quotation()`, `salesman()`, `items()`, `approvedBy()`, `cancelledBy()` — correct
- **Missing:** `BelongsToBranch` trait NOT applied — unlike Quotation, this model has no branch scope or auto-assignment from trait
- **Missing:** No scope to auto-scope by branch on queries

### ProformaInvoiceItem Model
- **Fillable:** All fields present
- **Relationships:** `proformaInvoice()`, `item()`, `itemAlias()` — correct
- **`BelongsToBranch` trait:** Applied

### StockMovement Model
- **Fillable:** All fields present
- **Casts:** Only `movement_date` cast to date
- **Relationships:** `item()`, `itemAlias()`, `warehouse()` — correct
- **Missing:** `createdBy()` user relationship
- **`BelongsToBranch` trait:** Applied

### WarehouseItemBalance Model
- **Fillable and relationships:** OK
- **`BelongsToBranch` trait:** Applied

### DocumentSequence Model
- **Fillable:** `branch_id`, `document_type`, `prefix`, `last_number` — OK
- **No `BelongsToBranch` trait** — correct, since DocumentNumberService uses `withoutGlobalScope`-equivalent manual query

### CustomerItemPrice Model
- **Fillable and relationships:** OK
- **Casts:** `last_sold_date` (date) — OK
- **`BelongsToBranch` trait:** Applied

---

## E. Controller and Service Audit

### QuotationController

**Strengths:**
- Role-based ownership check in `authorizeSalesDocument()` and `authorizeCustomerForSales()`
- DB transaction wrapping store/update/convert
- Correct status flow: draft → approved → converted/cancelled
- `saveItems()` private helper cleanly separates line calculation logic
- Customer price memory updated on each save

**Issues:**

1. **Document numbering still calls `customerDocumentNumber()` which calls `documentNumber()` scoped by branch.** The quotation number is thus `QI-XXXXXX` scoped per-branch, not system-wide. The prefix used is `QI` not `QT` as per project spec.

2. **`updateCustomerPriceMemory()` fires on EVERY save including draft.** Per spec, price memory should perhaps only update on approved/posted documents. Currently a draft quotation will overwrite a customer's price history.

3. **No `sales.approve` permission check on `approve()` action.** Any user who owns the quotation (or any admin/director/finance) can approve without checking `sales.approve` permission. The route also lacks a specific permission middleware.

4. **`authorizeSalesDocument()` does not handle `Admin` role** — only `Super Admin`, `Director`, `Finance`. If a user is given an `Admin` role in Spatie (a common business need), they won't have access to all quotations.

5. **Duplicate logic:** `saveItems()` and `recalculateTotals()` exist in both `QuotationController` and `ProformaInvoiceController` as near-identical private methods. Should be extracted to a shared service.

6. **`convertToProforma()` does not check if a proforma already exists for this quotation.** A quotation with status `converted` can theoretically only be converted once (status guard), but if status wasn't updated (e.g. partial failure), re-conversion could occur.

### ProformaInvoiceController

**Strengths:**
- DB transaction on store/update
- Approval and cancellation with audit fields (approved_by, approved_at, cancelled_by, cancelled_at, cancellation_reason)
- `authorizeSalesDocument()` present

**Issues:**

1. **Missing `permission:sales.view` middleware** (confirmed in route audit). Any authenticated user can reach proforma routes.

2. **`approve()` has no role/permission guard.** Any user can approve any proforma they have access to. The `authorizeSalesDocument()` check is not called inside `approve()`. Any user who knows the proforma ID can POST to approve it (though at minimum they'd need a valid session).

3. **`cancel()` also has no `authorizeSalesDocument()` call.** Same issue.

4. **`approve()` allows cancelling an approved proforma, then re-approving a cancelled one.** The check `if (! in_array($proforma->status, ['draft', 'pending_approval']))` approves `cancelled` status too since cancelled is not in that array — this allows cancellation → re-approval without restriction.

5. **`updateCustomerPriceMemory()` fires on proforma draft save** — same issue as QuotationController.

6. **Duplicate `saveItems()` / `recalculateTotals()` methods** — identical to QuotationController.

7. **No check if proforma came from quotation and whether the quotation's items have changed** — items can diverge silently.

### AJAX Controllers

#### CustomerSearchController
- **Branch isolation gap:** Does NOT enforce salesman ownership on search. Salesmen can search and see all branch customers (including other salesmen's customers). The `BelongsToBranch` trait global scope applies since `Customer` uses the trait, so cross-branch data is safe. But within a branch, a salesman sees all customers — spec says salesmen should only see their own customers.
- **No rate limiting** on search endpoint.

#### ItemSearchController  
- **OK** — searches active item aliases within branch scope (via trait)
- Does not filter by branch manually but `BelongsToBranch` on `ItemAlias` handles it

#### PriceLookupController
- **`highest_previous_price` logic is cross-customer and cross-branch.** It looks at `QuotationItem` and `ProformaInvoiceItem` tables with only `item_alias_id` filter — no customer or branch filter. This means the highest price from any branch or any customer is returned, which could leak pricing information across branches.
- **No CSRF needed (GET)** — OK
- **`system_price` not returned.** The proforma form JS calls `lookupPrice()` and expects `response.system_price`, but `PriceLookupController` does not return `system_price`. The `row.find('.system-price').val(response.system_price ?? 0)` line in `proformas/form.blade.php` will always set system_price to 0 on price lookup.

#### CustomerAjaxController
- **Good duplicate detection** using `normalized_name` and `normalized_phone`
- **Uses `withoutGlobalScope('branch')`** then manually filters by `branch_id` — correct
- **No permission check for who can create customers via AJAX.** Any authenticated user (including stock-only users) can POST to `/ajax/customers/store`.

#### ItemAliasAjaxController
- **No check if the main item is `is_active`.** A salesman can create an alias for a deactivated item.
- **No permission check for who can create aliases.** Any authenticated user can create item aliases via AJAX, even users without `stock.create` permission.
- **No check if the item belongs to the salesman's branch** beyond the `BelongsToBranch` trait's global scope on `Item` model, which should handle it correctly via the trait.

### DocumentNumberService

**Current behavior:** `customerDocumentNumber()` is a wrapper that ignores the customer ID and calls `documentNumber()`. This means the customer-specific numbering was intentionally abandoned in favor of global sequences — good.

**Issues:**

1. **Sequences are per-branch, not system-wide.** The unique constraint is `(branch_id, document_type)`. Two branches will each start at QI-000001 independently.

2. **Prefix naming inconsistency.** The service uses `QI` for quotation but the project spec calls for `QT`. It uses `PI` for proforma but the spec says `PF`. This affects all generated document numbers. Currently in the database these are stored as `QI-XXXXXX`.

3. **`documentNumber()` is called inside a DB transaction.** The inner `DB::transaction` in `documentNumber()` is called inside the outer transaction in the controllers. In MySQL, nested transactions are handled via savepoints — this works but is unnecessarily complex. The outer transaction in the controller could simply wrap the sequence increment.

4. **Race condition risk if sequence row does not exist.** If two concurrent requests for the same document type on the same branch arrive simultaneously when no sequence row exists yet, both may try to `create()` the row — this would be blocked by the `unique` constraint and one would throw an exception.

### StockService

- **Transaction-safe** with `lockForUpdate()`
- **Negative stock guard** at warehouse level — good
- **Issue:** No negative stock guard at item level (though warehouse-level check covers the specific warehouse)
- **Issue:** `StockService::moveStock()` is not called by QuotationController or ProformaInvoiceController — stock deduction does not happen on quotation/proforma. This is **correct per spec** (stock deduction should happen on posted invoice/delivery).

---

## F. Blade View Audit

### Quotation Views

**`quotations/create.blade.php`:**
- Form wraps the page with `id="quotationForm"`
- `@include('quotations.form')` is inside the form — the modals are included inside `form.blade.php` via `@include('quotations.partials.sales_modals')`
- **NESTED FORM PROBLEM:** `sales_modals.blade.php` contains `<form id="customerAjaxForm">` and `<form id="aliasAjaxForm">` — these modal forms are **included inside the main quotation `<form>` tag**. HTML does not allow nested forms. This is a structural HTML violation. The browser will likely silently discard the inner forms, causing AJAX submit handlers to have no serializable data.

**`quotations/edit.blade.php`:**
- Same structure — same nested form problem applies to edit as well.

**`proformas/create.blade.php`:**
- **FIXED correctly:** Comment says `{{-- IMPORTANT: Modal forms stay OUTSIDE the main proforma form --}}` and `@include('quotations.partials.sales_modals')` is placed **outside** the main `<form>` tag — correct structure.
- **Inconsistency:** The quotation create/edit still has the nested form bug that proforma correctly avoids.

### JavaScript Issues

**Quotation form (`quotations/form.blade.php`):**

1. **`initializeMainItemSelect()` called unconditionally on document ready** even though `#main_item_id` is inside a modal (the alias modal) — this is fine, the select is initialized once.

2. **Customer AJAX success handler checks `res.customer.text` and `res.customer.id`** — matches `CustomerAjaxController` response format — OK.

3. **No CSRF token in jQuery AJAX calls** — `$(this).serialize()` includes the `@csrf` hidden token from the form, so this is fine.

4. **Quotation form JS does not reinitialize Select2 on existing edit rows.** When the edit page loads, existing items have their `<select>` pre-populated, but `.each(function() { initItemSelect($(this)) })` is called — this is correct.

**Proforma form (`proformas/form.blade.php`):**

1. **`response.success` vs `response.status` inconsistency.** The `CustomerAjaxController::store()` returns `'status' => true`, but the proforma JS checks `if (response.success)`. The quotation JS checks the same way. This means the success branch never executes on proforma customer creation — after creating a customer, the customer is NOT auto-selected in the Select2, and the success alert never shows. The user gets no visible confirmation.

2. **`response.system_price` is expected but never returned by `PriceLookupController`.** The `lookupPrice()` call sets `row.find('.system-price').val(response.system_price ?? 0)` — always zero.

3. **Alias creation AJAX success checks `response.success` but controller returns `response.status`.** Same bug — the success branch never fires.

4. **Tax rate default hardcoded to 18.** New rows default to `value="18"` tax rate. This is reasonable for Tanzania VAT but should not be hardcoded in JS — it should come from a config value.

### Select2 Configuration

| Context | Config | Issue |
|---|---|---|
| Quotation customer | minimumInputLength:1, no theme | No `allowClear`, no theme |
| Proforma customer | theme:bootstrap4, allowClear | OK |
| Quotation item | minimumInputLength:2, no theme | No theme, inconsistent |
| Proforma item | theme:bootstrap4, allowClear | OK |
| Main item (alias modal) | minimumInputLength:2 | `dropdownParent` set correctly in proforma, OK in quotation too |

Quotation Select2 setup is less polished than proforma — lacks `theme:'bootstrap4'` and `allowClear`.

---

## G. Sales Engine Current Status

### What Is Already Working

- ✅ Authentication (Breeze), active user check, branch assignment check
- ✅ Roles/permissions foundation (Spatie), `Super Admin` seeded
- ✅ Branches, Users, Role management CRUD
- ✅ Customers CRUD with ownership tracking (created_by, normalized fields)
- ✅ Suppliers CRUD
- ✅ Warehouses, Units, Item Categories CRUD
- ✅ Items and Item Aliases CRUD
- ✅ Stock Movements, Stock Adjustment, Stock Transfer, Warehouse Balances, Bin Card
- ✅ Quotation CRUD with approval workflow (draft → approved → converted/cancelled)
- ✅ Proforma Invoice CRUD with approval workflow (draft → approved/cancelled)
- ✅ Convert Quotation → Proforma Invoice (logic works, route name in view is wrong)
- ✅ Global document sequence numbering (per-branch) replacing customer-specific sequences
- ✅ Customer last price memory (CustomerItemPrice table)
- ✅ AJAX customer search, item alias search, main item search, price lookup, customer creation, alias creation
- ✅ Audit middleware logging POST/PUT/PATCH/DELETE requests
- ✅ BelongsToBranch trait automatically scoping queries and auto-assigning branch_id

### What Is Partially Working

- ⚠️ **Convert to Proforma button** — logic is correct but route name `quotations.convert.proforma` in the view is wrong (should be `quotations.convert-to-proforma`)
- ⚠️ **Price lookup on proforma** — API works but `system_price` not returned; proforma always shows 0 in system price after lookup
- ⚠️ **Customer creation AJAX on proforma** — creates customer but JS response check is wrong (`response.success` vs `response.status`), so customer is not auto-selected after creation
- ⚠️ **Alias creation AJAX on proforma** — same response key mismatch
- ⚠️ **Quotation modals** — nested form HTML structure causes browser-level form nesting violation; may work in some browsers but is structurally incorrect

### What Is Missing (Not Yet Built)

- ❌ Sales Order (SO) module
- ❌ Delivery Note module
- ❌ Tax Invoice module
- ❌ Receipt module
- ❌ Credit Note / Return module
- ❌ `Director`, `Finance`, `Salesman`, `Storekeeper`, `Purchase Officer` roles not seeded (controllers reference these role names but they don't exist in the seeder)
- ❌ Proforma → Sales Order conversion
- ❌ Salesman ownership filter on customer search (salesmen can see all branch customers)
- ❌ Print/PDF output for quotations and proformas
- ❌ `pending_approval` status flow for proformas (referenced in approve() but no path creates this status)
- ❌ Stock deduction on invoice posting
- ❌ Accounting posting on approved financial documents
- ❌ Quotation expiry auto-cancellation (valid_until is stored but never enforced)

### What Should Be Built Next

See Section J.

---

## H. Security and Data Integrity Audit

### Role/Permission Risks

| Risk | Severity | Detail |
|---|---|---|
| Proforma route has no permission middleware | CRITICAL | Any active authenticated user with a branch can create, edit, approve, and cancel proforma invoices |
| `approve()` and `cancel()` on proforma have no `authorizeSalesDocument()` call | HIGH | Any user can approve or cancel any proforma by guessing/knowing the ID |
| `Director`, `Finance`, `Salesman` roles referenced in code but never created in seeder | HIGH | These role names will never match — `hasAnyRole(['Director', 'Finance'])` always returns false until roles are seeded |
| No `sales.approve` permission check on quotation/proforma approve actions | MEDIUM | Approval available to anyone who can view |
| AJAX alias creation has no permission check | MEDIUM | Any authenticated user can create item aliases — should require stock.create or sales.create |
| AJAX customer creation has no permission check | LOW | Any authenticated user can create customers via AJAX |

### Branch Isolation Risks

| Risk | Severity | Detail |
|---|---|---|
| `DocumentNumberService` sequences are branch-scoped, not global | MEDIUM | Multiple branches will generate duplicate document numbers (QI-000001 per branch) |
| `PriceLookupController` highest_previous_price is cross-branch | MEDIUM | Prices from other branches leak into current branch's price suggestions |
| `ProformaInvoice` model missing `BelongsToBranch` trait | MEDIUM | No automatic branch scoping on proforma queries or auto-assignment |

### Salesman Ownership Risks

| Risk | Severity | Detail |
|---|---|---|
| Customer search returns all branch customers to all users | MEDIUM | Salesmen can search and discover other salesmen's customers |
| Authorization uses role names `Director`/`Finance` that are not seeded | HIGH | Until those roles exist, ALL users are treated as non-admin users; no user can see all records (unintended restriction for Finance/Director users) |

### Mass Assignment Risks

- All models with `$fillable` defined look correct
- No `$guarded = []` found (good)
- `CustomerAjaxController::store()` uses validated data array — OK

### Validation Risks

| Risk | Detail |
|---|---|
| No max value validation on `selling_price` or `quantity` | A user can submit selling_price=99999999 — no upper bound |
| No min:0 on `discount_amount` in quotation store but validation says min:0 | OK |
| `tax_rate` not validated for max (e.g. > 100% is allowed) | Low risk |
| No check that `item_alias_id` belongs to the current branch | A user could submit an item_alias_id from another branch |

### Negative Stock Risks

- `StockService::moveStock()` has a warehouse-level negative stock check — good
- No stock deduction on quotation/proforma — correct per spec
- Risk: When Sales Order/Invoice module is built, the same strict check must be enforced

### Price Override Risks

- Selling price is freely editable by salesman (intentional)
- `system_price` is set from `alias.selling_price` — this represents the floor price (alias default price)
- No minimum price enforcement (selling price below system price is allowed — generates negative `salesman_profit`)
- Audit trail: `salesman_profit` field in items captures over/under pricing

### Audit Trail Gaps

| Gap | Detail |
|---|---|
| Audit middleware skips AJAX requests (`!$request->ajax()`) | AJAX create customer, create alias, price lookup are NOT logged |
| `AuditLogger::log()` service exists but is never called in controllers | No field-level before/after logging on any model |
| Audit middleware logs `new_values` as raw POST data | Sensitive amounts are logged but without labels — not easily queryable |
| No audit on quotation status changes (approve, cancel, convert) | These state transitions are not captured with meaningful context |

---

## I. Immediate Fix List

### Priority 1: Critical — Fix Before Continuing

| # | Fix | File | Detail |
|---|---|---|---|
| C1 | Add `permission:sales.view` middleware to proformas resource route | `routes/web.php` line 135 | Uncomment and restore the middleware |
| C2 | Add `authorizeSalesDocument()` call to `approve()` and `cancel()` in ProformaInvoiceController | `ProformaInvoiceController.php` lines 244, 263 | Without this, any logged-in user can approve/cancel any proforma |
| C3 | Fix route name in quotation show view | `views/quotations/show.blade.php` line 57 | Change `quotations.convert.proforma` → `quotations.convert-to-proforma` |
| C4 | Add `Director`, `Finance`, and `Salesman` roles to the seeder | `PhaseOneSeeder.php` | Without these roles, all `hasAnyRole(['Director','Finance'])` checks silently fail |

### Priority 2: Important — Fix Soon

| # | Fix | File | Detail |
|---|---|---|---|
| I1 | Fix JS response key mismatch on proforma (`response.status` vs `response.success`) | `views/proformas/form.blade.php` lines 586, 621 | Customer and alias modals appear to fail silently |
| I2 | Return `system_price` from `PriceLookupController` | `Ajax/PriceLookupController.php` | Proforma system price is always 0 after lookup |
| I3 | Move modal includes OUTSIDE the main form in quotation create/edit views | `views/quotations/create.blade.php`, `edit.blade.php` | Move `@include('quotations.partials.sales_modals')` outside the `<form>` tag like proforma does |
| I4 | Apply `BelongsToBranch` trait to `ProformaInvoice` model | `Models/ProformaInvoice.php` | No branch auto-scope on proforma queries |
| I5 | Change document number prefix: quotation `QI` → `QT`, proforma `PI` → `PF` | `Services/DocumentNumberService.php` | Align with project spec. Note: existing records will keep old prefixes |
| I6 | Add permission check to `ItemAliasAjaxController::store()` | `Ajax/ItemAliasAjaxController.php` | Should require at minimum `sales.create` or `stock.create` |
| I7 | Fix `PriceLookupController` `highest_previous_price` to filter by branch | `Ajax/PriceLookupController.php` | Add `->whereHas('quotation', fn($q) => $q->where('branch_id', auth()->user()->branch_id))` |
| I8 | Remove or drop `customer_document_sequences` table/model/migration | Cleanup | The table is orphaned; `CustomerDocumentSequence` model is dead code |

### Priority 3: Later Improvements

| # | Improvement | Detail |
|---|---|---|
| L1 | Change document sequences to system-wide (remove branch_id from unique constraint) | To match spec requirement for global continuous sequence |
| L2 | Filter customer AJAX search to salesman-owned customers | Add `when(!auth()->user()->hasAnyRole([...]), fn($q) => $q->where('created_by', auth()->id()))` |
| L3 | Extract `saveItems()` and `recalculateTotals()` to a shared `SalesDocumentService` | Removes duplication between Quotation and Proforma controllers |
| L4 | Add `$casts` to Customer, Item, ItemAlias models | Boolean and decimal casts missing |
| L5 | Add per-action audit logging via `AuditLogger::log()` in controllers | Current audit middleware only logs HTTP method, not model-level changes |
| L6 | Validate that `item_alias_id` belongs to the user's branch on document save | Prevents cross-branch item reference injection |
| L7 | Add `sales.approve` permission check on approve routes | Separate approval permission from view permission |
| L8 | Standardize Select2 config on quotation forms to match proforma (add `theme:'bootstrap4'`, `allowClear`) | UI consistency |
| L9 | Fix identical migration timestamps on items/item_aliases | Rename one migration file to avoid ordering ambiguity |
| L10 | Move hardcoded VAT default (18%) to a config/setting value | `resources/views/proformas/form.blade.php` line 424 |

---

## J. Recommended Next Development Step

**Recommended next step: Fix all Priority 1 (Critical) items first, then build the Sales Order (SO) module.**

The four critical fixes (C1–C4) take under one hour and close a real security hole (proforma open to all users) and a broken workflow (convert-to-proforma button throws an error). Once those are done, the Quotation → Proforma flow is complete and stable.

The next module in the business flow is the **Sales Order**. It inherits the same pattern as Proforma (header + line items, document number, status flow) and serves as the bridge between a commercial proforma and the actual stock/accounting pipeline. Building SO next keeps the sales flow moving forward in the correct sequence and all the hard work done (document numbering, customer ownership, item alias system, price memory) will carry forward cleanly into SO.

---

*Report generated: 2026-06-29. No project files were modified during this audit.*