# WP1 report — `PUT /production/boms/{bom}` persists components and operations

**Status:** DONE
**Repo/branch:** `/home/moonui2/moon-erp-be` on `hazemdev2` — committed, **not pushed, not merged**
**Commit:** `513cc5697` — `fix(production): persist BOM components and operations on PUT /boms/{bom}`
**Migration:** none · **FE change:** none

---

## What was wrong (confirmed, not re-derived)

`UpdateBomRequest` declared neither `components` nor `operations`, so
`$request->validated()` in `BomController::update()` had already dropped both arrays before
`$bom->update(...)` ran. The FE sends the whole document to this endpoint (`bom.service.ts` has no
sync method at all), so every component/operation edit reported success and wrote nothing.

`show()` additionally did not eager-load `components.substitutes`, so the editor never received the
substitutes; because `update()`/`syncComponents()` replace components wholesale, the first save after
opening a BOM erased every substitute.

## What changed

| File | Change |
|---|---|
| `Modules/Production/app/Http/Requests/Concerns/BomLineRules.php` | **new.** `bomComponentRules($presence)` / `bomOperationRules($presence)` — the single source of truth for the line rules. `$presence` (`'required'` for the sync endpoints, `'sometimes'` for the document update) is the only per-caller difference; the per-item rules are identical by construction, copied verbatim from the previous `syncComponents`/`syncOperations` blocks so nothing about them changed semantically. |
| `Modules/Production/app/Http/Requests/UpdateBomRequest.php` | `use BomLineRules;`; `rules()` now returns the header rules merged with both line rule sets at `'sometimes'`. |
| `Modules/Production/app/Http/Controllers/BomController.php` | `use BomLineRules;`. `syncComponents()`/`syncOperations()` now call `$request->validate(self::bomComponentRules('required'))` / `self::bomOperationRules('required')` — the two inline rule copies are gone. `update()` rewritten (below). `show()` now eager-loads `components.substitutes.product`. |
| `Modules/Production/tests/Feature/ProductionBomUpdateLinesTest.php` | **new**, 6 cases. |
| `docs/moonstack/CHANGELOG.md` | bilingual `[Unreleased]` bullet (user-facing bug fix). |

### `update()` shape

```
$data = $request->validated();
$replaceComponents = array_key_exists('components', $data);   // ABSENT ≠ empty
$replaceOperations = array_key_exists('operations', $data);
… unset both from $data …
DB::transaction:
    $bom->update($data);                     // header
    if ($replaceComponents) { substitutes()->delete() per component; components()->delete(); saveComponents(); }
    if ($replaceOperations) { operations()->delete(); saveOperations(); }
```

Deliberate details:

- **Key presence, not emptiness.** An absent `components` key leaves the lines untouched; an explicit
  `components: []` clears them. This survives Laravel's `excludeUnvalidatedArrayKeys` behaviour:
  with items present the leaf paths rebuild the key, with `[]` no wildcard rules expand so the
  `components` key itself is kept in `validated()`.
- **`validated()` also strips read-only noise.** The FE round-trips `id` / `bom_id` /
  `product_name` / `unit_name` back from `GET`; none of them are in the rules, so they never reach
  `saveComponents()`.
- **One transaction.** `saveComponents()` calls `assertComponentUnitsMatchBase()`, which throws a
  `ValidationException` *after* the header row is already updated. Only the transaction undoes that —
  covered by a dedicated test.
- **Same deletion order as `syncComponents()`**: substitutes first, then components.
- **No new defaults/coercions.** `supply_source` and `customer_qty_per_unit` pass through exactly as
  the sync path handles them; a blank `supply_source` is simply not sent by the FE and is never
  turned into `own`.

## Red → green (criterion 1)

New file: `Modules/Production/tests/Feature/ProductionBomUpdateLinesTest.php`.
Helpers are all prefixed `bomUpdateLines*` (`bomUpdateLinesFixture`, `bomUpdateLinesProduct`,
`bomUpdateLinesSeedBom`) — verified unique across the suite, no redeclare risk.

### BEFORE the fix — `pest …/ProductionBomUpdateLinesTest.php` (exit 2)

```
  ⨯ it persists edited components and operations through PUT /boms/{id…  13.42s
  ⨯ it keeps nested substitutes through a GET then PUT round trip         8.41s
  ✓ it leaves existing lines untouched when the payload omits component…  7.00s
  ⨯ it returns 422 and saves no header when a component fails request v…  6.69s
  ⨯ it rolls the header back when the component unit guard rejects the…   9.59s
  ⨯ it stores supply_source and customer_qty_per_unit exactly as sent     6.41s

  Tests:    5 failed, 1 passed (15 assertions)
  Duration: 51.56s
```

Representative failure messages from that run (they prove the arrays were discarded):

```
FAILED … it persists edited components and operations through PUT /boms/{id}
  Failed asserting that two arrays are identical.   (components still the seeded pair)

FAILED … it returns 422 and saves no header when a component fails request validation
  Expected response status code [422] but received 200.        ← quantity:0 was never validated
  at ProductionBomUpdateLinesTest.php:209

FAILED … it rolls the header back when the component unit guard rejects the payload mid-save
  Expected response status code [422] but received 200.        ← the unit guard never ran
  at ProductionBomUpdateLinesTest.php:234

FAILED … it stores supply_source and customer_qty_per_unit exactly as sent
  Failed asserting that two strings are identical.
  -'split'
  +'factory'                                                   ← nothing was written
  at ProductionBomUpdateLinesTest.php:265
```

The one green case before the fix is the header-only test — it passed trivially, because doing
nothing is the correct behaviour there. It is kept because it is the regression guard for the new
"absent key = don't touch" rule.

### AFTER the fix — same command (exit 0)

```
   PASS  Modules\Production\tests\Feature\ProductionBomUpdateLinesTest
  ✓ it persists edited components and operations through PUT /boms/{id…  12.93s
  ✓ it keeps nested substitutes through a GET then PUT round trip         8.20s
  ✓ it leaves existing lines untouched when the payload omits component…  6.88s
  ✓ it returns 422 and saves no header when a component fails request v…  7.39s
  ✓ it rolls the header back when the component unit guard rejects the…   6.21s
  ✓ it stores supply_source and customer_qty_per_unit exactly as sent     6.51s

  Tests:    6 passed (36 assertions)
  Duration: 48.16s
```

## Acceptance criteria

| # | Criterion | Evidence |
|---|---|---|
| 1 | Red-before-green | Both runs pasted above: 5 failed/1 passed → 6 passed. |
| 2 | Components + operations change through `PUT /boms/{id}` | Test 1: one component edited (qty 2→7, waste 5), one removed, one added; the single operation replaced by two — asserted against the DB, not the response. |
| 3 | Substitutes survive `GET` → resend verbatim → `PUT` | Test 2: `GET /boms/{id}` (now returns them, thanks to the `show()` fix), the returned `components` array is posted back untouched, `MfgBomSubstitute::count() === 1` with product/priority/ratio intact. |
| 4 | Header-only `PUT` leaves lines alone | Test 3: asserts the same component **row ids**, so it also catches a delete-and-recreate. |
| 5 | Invalid `components[]` → 422 and no header write | Two tests: request-level (`quantity: 0` → `422` + `components.0.quantity` error) and controller-level (unit ≠ product base unit → the guard fires *after* the header update; header and both original components verified unchanged — this is the one only the transaction can pass). |
| 6 | Existing sync endpoints unchanged | `ProductionBomV2Test` (substitute sync + re-sync clears) and `ProductionBomApiTest` both **PASS** in the post-fix module run. Their rules are now literally the same array the tests always exercised. |
| 7 | `supply_source` / `customer_qty_per_unit` round-trip | Test 6 pins `factory` → stored `factory` (never `own`) and `split` + `0.25` → stored verbatim. |

## Scoped suite

`pest Modules/Production`, post-fix, full module:

```
Tests:    10 failed, 622 passed (3624 assertions)
Duration: 1443.04s
```

All 10 failures are **pre-existing and unrelated** to BOMs:

| Test file | Failures | Verified pre-existing how |
|---|---|---|
| `ConsignmentFoundationTest` | 1 (`settle-buy under DIRECT grn_mode …`) | Same failure, same line, in the pre-fix run started before any edit. |
| `CostAiAnomalyVarianceTest` | 3 (`F1 variance: …`) | Same — captured pre-fix. |
| `ProductionVarianceTest` | 6 (`BE-V: …`, all `record-output` returning 422) | Re-ran that file alone with my two source edits stashed (`git stash push -- …`): **identical 6 failed / 8 passed**. Stash popped, tree restored. |

There is no `../baseline.md` in the plan folder — the numbers above are the effective baseline for the
Production module: **622 passed / 10 pre-existing failures, zero new failures.**

## Housekeeping

- `./vendor/bin/pint` on the four touched PHP files → `{"result":"pass"}`.
- `chown moonui2:moonui2` on every edited file (re-applied after the `git stash pop`, which had
  rewritten two of them as root).
- `bash local-deploy.sh` run after the edits — autoload rebuilt, caches cleared, "no new migrations".
- Committed on `hazemdev2` as `513cc5697`. **Not pushed, not merged.** The earlier unpushed commits
  were left untouched.
- No `migrate:fresh` / `migrate:refresh` / `db:wipe` was run; the suite uses in-memory SQLite
  (`phpunit.xml`), `moonui2_dev_be` was never touched.
- Bilingual CHANGELOG bullet added under `[Unreleased]` (this is a user-facing fix, and the repo hook
  enforces it before any future push).

## Notes / concerns (nothing blocking)

1. **`StoreBomRequest` still holds a third copy of the line rules.** It was out of this WP's stated
   scope, and its copy differs deliberately (company-scoped `Rule::exists` on `product_id`/`unit_id`,
   `required_with:components` instead of `required`), so folding it into `BomLineRules` would have
   meant changing either its rules or the sync path's. Worth a follow-up: the *stricter* Store
   variant is the correct one.
2. **Which leads to a real, pre-existing hole this WP did not widen or narrow:** the sync rules
   (`components.*.product_id => exists:products,id`) are **not company-scoped**, so a component can
   reference another tenant's product. `PUT /boms/{id}` now inherits exactly the same rule set, so
   the hole is not new — it was always reachable through `PUT /boms/{id}/components` — but it is now
   reachable from the main save button too. Tightening it to
   `Rule::exists('products','id')->where('company_id', …)` in `BomLineRules` would fix both callers
   in one line. Deliberately not done here: it changes sync-endpoint behaviour, which criterion 6
   asked to keep identical. **Recommend a small follow-up WP.**
3. **No FE change was needed** and none was made. Verified in `boms.component.ts`: the payload it
   already builds validates cleanly against the new rules (`supply_source` is always one of the four
   enum values via `normaliseBomSupplySource`, `customer_qty_per_unit` is zeroed off `split`,
   optional fields are `undefined` rather than `''`), and the component form group already reads
   `data.substitutes`, so the `show()` fix alone makes the substitute round trip work end to end.