feat(materials): partial consumption, returns to stock & reclassification (#99) - #238
Merged
jakub-przepiora merged 2 commits intoAug 12, 2026
Conversation
…tion (#99) Reconcile a work order's pulled materials against what actually happened on the floor, every change booked to the stock_movements ledger. Built on the allocation/consumption path (MaterialAllocationService + StockMovementService). - Partial consumption: wire the existing recordConsumption() — bookkeeping only (stock left at allocation; consumeForBatch returns the leftover at completion). - Return to stock: new MaterialAllocationService::returnQuantity() books TYPE_RETURN, releases the reservation, restores picked lots (new LotPickingService::returnPartialForAllocation) and shrinks allocated_qty so the completion reconciler never double-returns. - Reclassification (class + status): new MaterialReclassificationService — reclassifyClass() moves a quantity between materials as a correlated pair of TYPE_RECLASSIFY movements; reclassifyStatus() reuses MaterialHoldService for released/quarantine and scraps a rejected lot's remaining quantity from stock. New TYPE_RECLASSIFY / SOURCE_RECLASSIFICATION constants and an append-only material_reclassifications audit table correlating the legs. - API v1: /material-allocations/{id}/consume & /return (production users), /material-reclassifications/class & /material-lots/{id}/reclassify-status (role:Supervisor|Admin). Web: a Materials reconciliation panel on the admin work-order page with consume/return/reclassify modals. All validation via Form Requests; WorkOrder::allocations() relation added. - i18n en/pl parity (+20 keys). Tests: partial consumption, return incl. the double-return guard, reclassification class + status, admin web actions. Backend 2318 passed; vitest 25; build OK; Pint clean.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds partial material consumption, allocation returns, class and lot-status reclassification, audit records, API and web routes, authorization checks, admin UI workflows, translations, and feature tests. ChangesMaterial reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant WorkOrderPage
participant WorkOrderManagementController
participant MaterialAllocationService
participant MaterialReclassificationService
Admin->>WorkOrderPage: Open work order
WorkOrderPage->>WorkOrderManagementController: Load allocations and permissions
Admin->>WorkOrderPage: Submit reconciliation action
WorkOrderPage->>WorkOrderManagementController: Post consumption, return, or reclassification
WorkOrderManagementController->>MaterialAllocationService: Process allocation change
WorkOrderManagementController->>MaterialReclassificationService: Process material change
WorkOrderManagementController-->>WorkOrderPage: Redirect with result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php (2)
166-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the allocation eager-load into the existing
load()call.Line 135 already calls
$workOrder->load(...). Line 168 issues a secondload(). Add the allocation relations to the first call to keep the loading in one place.♻️ Proposed change
- $workOrder->load(['customer', 'line', 'productType', 'batches.steps', 'issues.issueType', 'issues.reportedBy']); + $workOrder->load([ + 'customer', 'line', 'productType', 'batches.steps', 'issues.issueType', 'issues.reportedBy', + // Materials reconciliation (`#99`). + 'allocations.material', 'allocations.lotPicks.lot', + ]);Then remove the
load()on line 168.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php` around lines 166 - 185, Merge ['allocations.material', 'allocations.lotPicks.lot'] into the existing $workOrder->load(...) call in the controller method, then remove the later load() invocation immediately before the allocations mapping. Keep the allocation transformation unchanged.
272-274: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCap or defer the
materialsprop.
Material::where('is_active', true)->orderBy('code')->get(...)loads the complete active material catalogue into every work-order page render for every Supervisor and Admin. The list is only needed inside the reclassify modal. On a plant with thousands of materials this inflates every page response.Consider a searchable lookup endpoint, or restrict the list to the materials of the same material type as the order's allocations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php` around lines 272 - 274, Update the materials prop in WorkOrderManagementController so work-order pages do not load the entire active material catalogue; defer it to a searchable lookup endpoint or restrict the query to materials matching the order’s allocation material type, while preserving availability for the reclassify modal.backend/resources/js/Pages/admin/work-orders/Show.jsx (1)
274-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd dialog semantics and an Escape handler to
ModalFrame.The frame has no
role="dialog", noaria-modal, no focus trap, and no Escape key handler. Screen-reader users receive no dialog announcement. Keyboard users cannot dismiss the modal with Escape.The Cancel button remains keyboard-reachable, so the task is still completable. The existing
DoneModalshares this shape, so a shared fix would improve both.♿ Proposed minimum improvement
function ModalFrame({ title, children }) { + useEffect(() => { + const onKey = (e) => { if (e.key === 'Escape') onClose?.(); }; + document.addEventListener('keydown', onKey); + return () => document.removeEventListener('keydown', onKey); + }, [onClose]); + return ( <div className="fixed inset-0 z-50 flex items-center justify-center bg-black/50"> - <div className="bg-om-card rounded-om-sm shadow-xl p-6 w-full max-w-md mx-4"> - <h3 className="text-lg font-bold text-om-ink mb-4">{title}</h3> + <div role="dialog" aria-modal="true" aria-label={title} + className="bg-om-card rounded-om-sm shadow-xl p-6 w-full max-w-md mx-4"> + <h3 className="text-lg font-bold text-om-ink mb-4">{title}</h3>Pass
onCloseintoModalFramefrom each caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/resources/js/Pages/admin/work-orders/Show.jsx` around lines 274 - 283, Update ModalFrame to accept an onClose callback, add dialog semantics with role="dialog" and aria-modal="true", and register an Escape-key handler that invokes onClose. Pass the appropriate close handler from every ModalFrame caller, including DoneModal, while preserving the existing Cancel-button behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php`:
- Around line 320-334: Update the reclassify method to enforce the nested
work-order scope before calling
MaterialReclassificationService::reclassifyClass: verify that the requested
source material is allocated to the supplied WorkOrder and reject the request
when it is not. Keep the existing source, target, lot lookup, and
reclassification flow unchanged for valid allocations, and use the existing
allocation relationship or model symbol rather than trusting source_material_id
alone.
- Around line 284-286: Update RecordConsumptionRequest::authorize() and
ReturnAllocationRequest::authorize() to explicitly deny authorization when the
allocation’s batch relationship is null, before dereferencing batch->workOrder.
Preserve the existing authorization checks for available batches and ensure
soft-deleted or otherwise unavailable batches return false rather than causing
an exception.
In `@backend/app/Services/Material/LotPickingService.php`:
- Around line 229-266: Add a lot-tracking test covering
returnPartialForAllocation with an allocation whose picks span multiple
MaterialLot records. Assert the returned quantity is restored across lots in
reverse pick order, a consumed lot with available quantity is reopened, and pick
rows are reduced or deleted when fully returned.
In `@backend/app/Services/Material/MaterialAllocationService.php`:
- Around line 403-445: Move the transaction boundary in the return flow before
validation, and inside it reload the allocation with lockForUpdate() to obtain
the authoritative row. Perform the status, quantity, and returnable checks
against this locked allocation, then use the same row for material lookup, stock
movement, reservation release, lot return, and the updates in
returnPartialForAllocation.
In `@backend/app/Services/Material/MaterialReclassificationService.php`:
- Around line 116-159: Reload the route-bound lot inside the DB::transaction
closure using a row lock, and move the `$fromStatus` assignment into that
closure after the locked reload. Ensure the rejection flow in the transaction
uses this locked, current `MaterialLot` instance when calculating `$remaining`
and recording scrap, preventing concurrent requests from processing the same
available quantity twice.
In
`@backend/database/migrations/2026_08_12_100000_create_material_reclassifications_table.php`:
- Around line 26-27: Update the source_material_id foreign-key definition in the
migration creating the material reclassifications table to use
restrictOnDelete() instead of cascadeOnDelete(), preserving audit rows when
source materials are deleted while leaving target_material_id behavior
unchanged.
In `@backend/lang/en.json`:
- Around line 5192-5212: Add the missing “Work order :code created.” translation
key with its English value to both language JSON files, keeping the key sets
synchronized and preserving valid JSON formatting.
In `@backend/resources/js/Pages/admin/work-orders/Show.jsx`:
- Around line 307-312: Add a processing state to ReturnModal, ConsumeModal, and
ReclassifyModal; guard each submit handler so it cannot post while a request is
in flight, set processing before calling router.post, and clear it via the
request completion callback. Pass processing to each modal’s ModalActions
disabled prop so all submit buttons remain disabled until the request completes.
In `@backend/routes/web.php`:
- Around line 429-432: Align the authorization for the consume and return routes
with the stock-mutating reclassify route by adding the `role:Supervisor|Admin`
middleware to both `work-orders.allocations.consume` and
`work-orders.allocations.return`. Keep their existing handlers and route
structure unchanged.
In `@backend/tests/Feature/Api/V1/RecordConsumptionTest.php`:
- Around line 87-96: Update the consumption flow exercised by
test_zero_consumption_falls_back_to_planned_at_completion so recordConsumption
preserves the distinction between an explicit consumed_qty of 0 and no
consumption record. Use an explicit recorded marker or nullable unrecorded
value, and make consumeForBatch return the full allocation only for an
explicitly recorded zero while retaining the existing behavior for genuinely
unrecorded consumption.
- Around line 115-119: Add authenticated unauthorized-user coverage to
RecordConsumptionTest::test_guest_cannot_record_consumption by creating a
logged-in user without the required reconciliation permission or role and
asserting consume is denied; likewise update ReturnAllocationTest at
backend/tests/Feature/Api/V1/ReturnAllocationTest.php lines 140-144 with an
authenticated unauthorized-user test for return, while preserving the existing
guest and Admin authorization tests.
In `@backend/tests/Feature/Web/Admin/WorkOrderMaterialReconciliationTest.php`:
- Around line 66-122: Add tests in WorkOrderMaterialReconciliationTest covering
the missing mandatory cases: an Operator must receive 403 when posting to
reclassify; reclassifying with identical source_material_id and
target_material_id must produce a validation error; returning more than the
allocation’s returnable quantity must redirect with an error flash and leave
material stock unchanged; and exercise the non-allocated status guard through
the return action if the fixture setup permits.
---
Nitpick comments:
In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php`:
- Around line 166-185: Merge ['allocations.material',
'allocations.lotPicks.lot'] into the existing $workOrder->load(...) call in the
controller method, then remove the later load() invocation immediately before
the allocations mapping. Keep the allocation transformation unchanged.
- Around line 272-274: Update the materials prop in
WorkOrderManagementController so work-order pages do not load the entire active
material catalogue; defer it to a searchable lookup endpoint or restrict the
query to materials matching the order’s allocation material type, while
preserving availability for the reclassify modal.
In `@backend/resources/js/Pages/admin/work-orders/Show.jsx`:
- Around line 274-283: Update ModalFrame to accept an onClose callback, add
dialog semantics with role="dialog" and aria-modal="true", and register an
Escape-key handler that invokes onClose. Pass the appropriate close handler from
every ModalFrame caller, including DoneModal, while preserving the existing
Cancel-button behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b9195aa-ac4c-4b77-8297-4b3557c79331
📒 Files selected for processing (25)
CHANGELOG.mdbackend/app/Http/Controllers/Api/V1/MaterialAllocationController.phpbackend/app/Http/Controllers/Api/V1/MaterialReclassificationController.phpbackend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.phpbackend/app/Http/Requests/Api/V1/ReclassifyClassRequest.phpbackend/app/Http/Requests/Api/V1/ReclassifyLotStatusRequest.phpbackend/app/Http/Requests/Api/V1/RecordConsumptionRequest.phpbackend/app/Http/Requests/Api/V1/ReturnAllocationRequest.phpbackend/app/Models/MaterialReclassification.phpbackend/app/Models/StockMovement.phpbackend/app/Models/WorkOrder.phpbackend/app/Services/Material/LotPickingService.phpbackend/app/Services/Material/MaterialAllocationService.phpbackend/app/Services/Material/MaterialReclassificationService.phpbackend/database/migrations/2026_08_12_100000_create_material_reclassifications_table.phpbackend/lang/en.jsonbackend/lang/pl.jsonbackend/resources/js/Pages/admin/work-orders/Show.jsxbackend/routes/api.phpbackend/routes/web.phpbackend/tests/Feature/Api/V1/ReclassifyClassTest.phpbackend/tests/Feature/Api/V1/ReclassifyLotStatusTest.phpbackend/tests/Feature/Api/V1/RecordConsumptionTest.phpbackend/tests/Feature/Api/V1/ReturnAllocationTest.phpbackend/tests/Feature/Web/Admin/WorkOrderMaterialReconciliationTest.php
Comment on lines
+284
to
+286
| public function recordConsumption(RecordConsumptionRequest $request, WorkOrder $workOrder, MaterialAllocation $allocation, MaterialAllocationService $allocations) | ||
| { | ||
| $this->assertAllocationBelongs($workOrder, $allocation); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check nullability of material_allocations.batch_id and soft-delete usage on Batch.
set -euo pipefail
fd -t f . backend/database/migrations --exec rg -n -C3 'material_allocations' {} \; | head -80
echo '--- MaterialAllocation model ---'
fd -t f 'MaterialAllocation.php' backend/app/Models --exec cat -n {} \;
echo '--- Batch soft deletes ---'
fd -t f 'Batch.php' backend/app/Models --exec rg -n 'SoftDelete|batch_id|workOrder' {} \;
echo '--- Request authorize methods ---'
fd -t f 'RecordConsumptionRequest.php|ReturnAllocationRequest.php' backend/app/Http/Requests --exec cat -n {} \;Repository: Mes-Open/OpenMes
Length of output: 3726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Relevant migration definitions ---'
rg -n -C5 'batch_id|CREATE TABLE.*material_allocations|Schema::create\(' backend/database/migrations -g '*.php' | head -240
echo '--- Model files ---'
fd -t f -i 'MaterialAllocation.php|Batch.php' backend/app/Models -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}
echo '--- Request files ---'
fd -t f -i 'RecordConsumptionRequest.php|ReturnAllocationRequest.php' backend/app/Http/Requests -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}
echo '--- Controller authorization flow ---'
sed -n '260,320p' backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php
rg -n -C4 'assertAllocationBelongs|recordConsumption|returnAllocation' backend/app/Http/Controllers backend/app/Http/Requests -g '*.php'Repository: Mes-Open/OpenMes
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Files that define or alter material_allocations.batch_id ---'
rg -l -F 'batch_id' backend/database/migrations -g '*.php' | sort
echo '--- Exact batch_id definitions ---'
rg -n -C8 -F "foreignId('batch_id')" backend/database/migrations -g '*.php'
echo '--- Soft-delete implementation ---'
fd -t f -i 'SoftDeletesWithAudit.php' backend/app -x cat -n {}
echo '--- Request imports and route bindings ---'
sed -n '1,45p' backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php
rg -n -C3 'recordConsumption|returnAllocation' backend/routes backend/app -g '*.php' | head -160
echo '--- Belongs-to assertion ---'
sed -n '338,352p' backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.phpRepository: Mes-Open/OpenMes
Length of output: 39499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
migration = Path("backend/database/migrations/2026_05_09_100002_create_material_allocations_table.php").read_text()
batch = Path("backend/app/Models/Batch.php").read_text()
soft_deletes = Path("backend/app/Models/Concerns/SoftDeletesWithAudit.php").read_text()
requests = [
Path("backend/app/Http/Requests/Api/V1/RecordConsumptionRequest.php").read_text(),
Path("backend/app/Http/Requests/Api/V1/ReturnAllocationRequest.php").read_text(),
]
controller = Path("backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php").read_text()
assert "foreignId('batch_id')->constrained()->cascadeOnDelete()" in migration
assert "use SoftDeletesWithAudit;" in batch
assert "use SoftDeletes;" in soft_deletes
assert all("$allocation->batch->workOrder" in source for source in requests)
assert "$allocation->batch?->work_order_id" in controller
print("material_allocations.batch_id is declared non-nullable")
print("Batch uses SoftDeletesWithAudit, which uses SoftDeletes")
print("Both request authorize methods dereference batch without a null check")
print("The web controller checks the batch relationship with null-safe access")
PYRepository: Mes-Open/OpenMes
Length of output: 406
Guard missing batches in both authorization methods.
material_allocations.batch_id is non-nullable, but Batch uses SoftDeletesWithAudit. A soft-deleted batch is hidden from $allocation->batch, so both RecordConsumptionRequest::authorize() and ReturnAllocationRequest::authorize() dereference workOrder on null and return 500 before the controller check. Add an explicit batch-null guard and deny authorization when the batch is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php`
around lines 284 - 286, Update RecordConsumptionRequest::authorize() and
ReturnAllocationRequest::authorize() to explicitly deny authorization when the
allocation’s batch relationship is null, before dereferencing batch->workOrder.
Preserve the existing authorization checks for available batches and ensure
soft-deleted or otherwise unavailable batches return false rather than causing
an exception.
Comment on lines
+320
to
+334
| public function reclassify(ReclassifyClassRequest $request, WorkOrder $workOrder, MaterialReclassificationService $reclassifications) | ||
| { | ||
| try { | ||
| $source = Material::findOrFail($request->validated('source_material_id')); | ||
| $target = Material::findOrFail($request->validated('target_material_id')); | ||
| $lot = $request->validated('source_lot_id') ? MaterialLot::findOrFail($request->validated('source_lot_id')) : null; | ||
|
|
||
| $reclassifications->reclassifyClass( | ||
| $source, | ||
| $target, | ||
| (float) $request->validated('qty'), | ||
| $request->user(), | ||
| $lot, | ||
| $request->validated('reason'), | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope reclassify to the work order, or drop the route parameter.
The action receives $workOrder and never uses it (PHPMD flags the unused parameter). source_material_id comes straight from the request body and is validated only by exists:materials,id. A Supervisor on the page for work order A can therefore reclassify any material in the system through work order A's URL. The nested route implies a scope that the action does not enforce.
Add a check that the source material is allocated to this work order. That matches the panel's UI, where the source is always the allocation's material.
🔒 Proposed scoping check
public function reclassify(ReclassifyClassRequest $request, WorkOrder $workOrder, MaterialReclassificationService $reclassifications)
{
try {
$source = Material::findOrFail($request->validated('source_material_id'));
+
+ // The panel always reclassifies one of this order's pulled materials —
+ // enforce that, so the nested route is a real scope and not decoration.
+ if (! $workOrder->allocations()->where('material_id', $source->id)->exists()) {
+ abort(404);
+ }
+
$target = Material::findOrFail($request->validated('target_material_id'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function reclassify(ReclassifyClassRequest $request, WorkOrder $workOrder, MaterialReclassificationService $reclassifications) | |
| { | |
| try { | |
| $source = Material::findOrFail($request->validated('source_material_id')); | |
| $target = Material::findOrFail($request->validated('target_material_id')); | |
| $lot = $request->validated('source_lot_id') ? MaterialLot::findOrFail($request->validated('source_lot_id')) : null; | |
| $reclassifications->reclassifyClass( | |
| $source, | |
| $target, | |
| (float) $request->validated('qty'), | |
| $request->user(), | |
| $lot, | |
| $request->validated('reason'), | |
| ); | |
| public function reclassify(ReclassifyClassRequest $request, WorkOrder $workOrder, MaterialReclassificationService $reclassifications) | |
| { | |
| try { | |
| $source = Material::findOrFail($request->validated('source_material_id')); | |
| // The panel always reclassifies one of this order's pulled materials — | |
| // enforce that, so the nested route is a real scope and not decoration. | |
| if (! $workOrder->allocations()->where('material_id', $source->id)->exists()) { | |
| abort(404); | |
| } | |
| $target = Material::findOrFail($request->validated('target_material_id')); | |
| $lot = $request->validated('source_lot_id') ? MaterialLot::findOrFail($request->validated('source_lot_id')) : null; | |
| $reclassifications->reclassifyClass( | |
| $source, | |
| $target, | |
| (float) $request->validated('qty'), | |
| $request->user(), | |
| $lot, | |
| $request->validated('reason'), | |
| ); |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 320-320: Avoid unused parameters such as '$workOrder'. (undefined)
(UnusedFormalParameter)
🪛 PHPStan (2.2.7)
[error] 323-323: Call to an undefined static method App\Models\Material::findOrFail().
(staticMethod.notFound)
[error] 324-324: Call to an undefined static method App\Models\Material::findOrFail().
(staticMethod.notFound)
[error] 325-325: Call to an undefined static method App\Models\MaterialLot::findOrFail().
(staticMethod.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/Http/Controllers/Web/Admin/WorkOrderManagementController.php`
around lines 320 - 334, Update the reclassify method to enforce the nested
work-order scope before calling
MaterialReclassificationService::reclassifyClass: verify that the requested
source material is allocated to the supplied WorkOrder and reject the request
when it is not. Keep the existing source, target, lot lookup, and
reclassification flow unchanged for valid allocations, and use the existing
allocation relationship or model symbol rather than trusting source_material_id
alone.
Source: Linters/SAST tools
Comment on lines
+229
to
+266
| public function returnPartialForAllocation(MaterialAllocation $allocation, float $qty): void | ||
| { | ||
| if ($qty <= 0 || ! $this->isLotTrackingEnabled()) { | ||
| return; | ||
| } | ||
|
|
||
| DB::transaction(function () use ($allocation, $qty) { | ||
| $picks = $allocation->lotPicks()->with('lot')->lockForUpdate()->get()->reverse(); | ||
| $remaining = $qty; | ||
|
|
||
| foreach ($picks as $pick) { | ||
| if ($remaining <= self::EPSILON) { | ||
| break; | ||
| } | ||
| $take = min($remaining, (float) $pick->picked_qty); | ||
| if ($take <= 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if ($pick->lot) { | ||
| $pick->lot->increment('quantity_available', $take); | ||
| if ($pick->lot->status === MaterialLot::STATUS_CONSUMED && (float) $pick->lot->fresh()->quantity_available > 0) { | ||
| $pick->lot->update(['status' => MaterialLot::STATUS_RELEASED]); | ||
| } | ||
| \App\Sync\CollectionBroadcaster::flush($pick->lot); // increment bypasses model events | ||
| } | ||
|
|
||
| $newPicked = (float) $pick->picked_qty - $take; | ||
| if ($newPicked <= self::EPSILON) { | ||
| $pick->delete(); | ||
| } else { | ||
| $pick->update(['picked_qty' => $newPicked]); | ||
| } | ||
|
|
||
| $remaining -= $take; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add coverage for partial lot returns.
No supplied return test creates MaterialLot or AllocationLotPick records. The tests do not verify lot quantity restoration, reopening a consumed lot, or reducing and deleting pick rows.
Add a lot-tracking test for a partial return across more than one picked lot. As per coding guidelines, “Tests are mandatory for new endpoints/business logic” and must cover “domain edge cases.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/Services/Material/LotPickingService.php` around lines 229 - 266,
Add a lot-tracking test covering returnPartialForAllocation with an allocation
whose picks span multiple MaterialLot records. Assert the returned quantity is
restored across lots in reverse pick order, a consumed lot with available
quantity is reopened, and pick rows are reduced or deleted when fully returned.
Source: Coding guidelines
Comment on lines
+403
to
+445
| if ($allocation->status !== MaterialAllocation::STATUS_ALLOCATED) { | ||
| throw new \DomainException('Can only return material from an `allocated` allocation.'); | ||
| } | ||
| if ($qty <= 0) { | ||
| throw new \InvalidArgumentException('Return quantity must be positive.'); | ||
| } | ||
|
|
||
| $returnable = (float) $allocation->allocated_qty | ||
| - (float) $allocation->consumed_qty | ||
| - (float) $allocation->scrap_qty; | ||
| if ($qty > $returnable + 1e-9) { | ||
| throw new \InvalidArgumentException('Return quantity exceeds the unconsumed allocated quantity.'); | ||
| } | ||
|
|
||
| return DB::transaction(function () use ($allocation, $qty, $user, $reason) { | ||
| $material = $allocation->material; | ||
|
|
||
| if (! $material) { | ||
| throw new \DomainException('Allocation has no associated material.'); | ||
| } | ||
|
|
||
| $this->stockMovements->record( | ||
| $material, | ||
| StockMovement::TYPE_RETURN, | ||
| $qty, | ||
| user: $user, | ||
| sourceType: StockMovement::SOURCE_BATCH, | ||
| sourceId: $allocation->batch_id, | ||
| reason: $reason ?? 'Batch #'.$allocation->batch_id.' — unused material returned to stock', | ||
| ); | ||
|
|
||
| $this->releaseReservation($material, $qty); | ||
|
|
||
| // Lot tracking: hand the returned quantity back to the picked lots. | ||
| $this->lotPicking->returnPartialForAllocation($allocation, $qty); | ||
|
|
||
| $allocation->update([ | ||
| // Shrink the allocation so consumeForBatch's leftover calc excludes what we just returned. | ||
| 'allocated_qty' => (float) $allocation->allocated_qty - $qty, | ||
| 'returned_qty' => (float) $allocation->returned_qty + $qty, | ||
| ]); | ||
|
|
||
| return $allocation->fresh(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Lock the allocation before validation.
Two concurrent returns can both validate the same returnable quantity. Both requests can then create return movements and release reservations. This can overstate stock and make reserved_quantity negative.
Start the transaction before the status and quantity checks. Reload MaterialAllocation with lockForUpdate() inside that transaction. Use the locked row for validation and updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/Services/Material/MaterialAllocationService.php` around lines 403
- 445, Move the transaction boundary in the return flow before validation, and
inside it reload the allocation with lockForUpdate() to obtain the authoritative
row. Perform the status, quantity, and returnable checks against this locked
allocation, then use the same row for material lookup, stock movement,
reservation release, lot return, and the updates in returnPartialForAllocation.
Comment on lines
+116
to
+159
| $fromStatus = $lot->status; | ||
|
|
||
| return DB::transaction(function () use ($lot, $toStatus, $by, $reason, $fromStatus) { | ||
| // Create the audit row first so the reject scrap movement can point at it. | ||
| $record = MaterialReclassification::create([ | ||
| 'type' => MaterialReclassification::TYPE_STATUS, | ||
| 'source_material_id' => $lot->material_id, | ||
| 'source_lot_id' => $lot->id, | ||
| 'from_status' => $fromStatus, | ||
| 'to_status' => $toStatus, | ||
| 'reason' => $reason, | ||
| 'performed_by' => $by->id, | ||
| 'performed_at' => now(), | ||
| ]); | ||
|
|
||
| switch ($toStatus) { | ||
| case MaterialLot::STATUS_QUARANTINE: | ||
| $this->holds->hold($lot, $reason ?? '', $by); | ||
| break; | ||
|
|
||
| case MaterialLot::STATUS_RELEASED: | ||
| $this->holds->release($lot, $by); | ||
| break; | ||
|
|
||
| case MaterialLot::STATUS_REJECTED: | ||
| if (in_array($lot->status, [MaterialLot::STATUS_CONSUMED, MaterialLot::STATUS_REJECTED], true)) { | ||
| throw new \DomainException("Cannot reject a {$lot->status} lot."); | ||
| } | ||
| $remaining = (float) $lot->quantity_available; | ||
| if ($remaining > 0 && $lot->material) { | ||
| $this->stockMovements->record( | ||
| $lot->material, | ||
| StockMovement::TYPE_SCRAP, | ||
| -$remaining, | ||
| user: $by, | ||
| sourceType: StockMovement::SOURCE_RECLASSIFICATION, | ||
| sourceId: $record->id, | ||
| reason: $reason ?? 'Lot '.$lot->lot_number.' rejected — scrapped from stock', | ||
| ); | ||
| } | ||
| $lot->update([ | ||
| 'status' => MaterialLot::STATUS_REJECTED, | ||
| 'quantity_available' => 0, | ||
| ]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Lock and reload the lot inside the transaction.
Line 116 reads the route-bound lot before the transaction. Two rejection requests can both read STATUS_RELEASED and the same quantity_available. Each request can then record a scrap movement, which decreases stock twice.
Lock and reload MaterialLot before setting $fromStatus and calculating $remaining.
Proposed fix
- $fromStatus = $lot->status;
-
- return DB::transaction(function () use ($lot, $toStatus, $by, $reason, $fromStatus) {
+ return DB::transaction(function () use ($lot, $toStatus, $by, $reason) {
+ $lot = MaterialLot::query()
+ ->lockForUpdate()
+ ->findOrFail($lot->getKey());
+ $fromStatus = $lot->status;
+
// Create the audit row first so the reject scrap movement can point at it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $fromStatus = $lot->status; | |
| return DB::transaction(function () use ($lot, $toStatus, $by, $reason, $fromStatus) { | |
| // Create the audit row first so the reject scrap movement can point at it. | |
| $record = MaterialReclassification::create([ | |
| 'type' => MaterialReclassification::TYPE_STATUS, | |
| 'source_material_id' => $lot->material_id, | |
| 'source_lot_id' => $lot->id, | |
| 'from_status' => $fromStatus, | |
| 'to_status' => $toStatus, | |
| 'reason' => $reason, | |
| 'performed_by' => $by->id, | |
| 'performed_at' => now(), | |
| ]); | |
| switch ($toStatus) { | |
| case MaterialLot::STATUS_QUARANTINE: | |
| $this->holds->hold($lot, $reason ?? '', $by); | |
| break; | |
| case MaterialLot::STATUS_RELEASED: | |
| $this->holds->release($lot, $by); | |
| break; | |
| case MaterialLot::STATUS_REJECTED: | |
| if (in_array($lot->status, [MaterialLot::STATUS_CONSUMED, MaterialLot::STATUS_REJECTED], true)) { | |
| throw new \DomainException("Cannot reject a {$lot->status} lot."); | |
| } | |
| $remaining = (float) $lot->quantity_available; | |
| if ($remaining > 0 && $lot->material) { | |
| $this->stockMovements->record( | |
| $lot->material, | |
| StockMovement::TYPE_SCRAP, | |
| -$remaining, | |
| user: $by, | |
| sourceType: StockMovement::SOURCE_RECLASSIFICATION, | |
| sourceId: $record->id, | |
| reason: $reason ?? 'Lot '.$lot->lot_number.' rejected — scrapped from stock', | |
| ); | |
| } | |
| $lot->update([ | |
| 'status' => MaterialLot::STATUS_REJECTED, | |
| 'quantity_available' => 0, | |
| ]); | |
| return DB::transaction(function () use ($lot, $toStatus, $by, $reason) { | |
| $lot = MaterialLot::query() | |
| ->lockForUpdate() | |
| ->findOrFail($lot->getKey()); | |
| $fromStatus = $lot->status; | |
| // Create the audit row first so the reject scrap movement can point at it. | |
| $record = MaterialReclassification::create([ | |
| 'type' => MaterialReclassification::TYPE_STATUS, | |
| 'source_material_id' => $lot->material_id, | |
| 'source_lot_id' => $lot->id, | |
| 'from_status' => $fromStatus, | |
| 'to_status' => $toStatus, | |
| 'reason' => $reason, | |
| 'performed_by' => $by->id, | |
| 'performed_at' => now(), | |
| ]); | |
| switch ($toStatus) { | |
| case MaterialLot::STATUS_QUARANTINE: | |
| $this->holds->hold($lot, $reason ?? '', $by); | |
| break; | |
| case MaterialLot::STATUS_RELEASED: | |
| $this->holds->release($lot, $by); | |
| break; | |
| case MaterialLot::STATUS_REJECTED: | |
| if (in_array($lot->status, [MaterialLot::STATUS_CONSUMED, MaterialLot::STATUS_REJECTED], true)) { | |
| throw new \DomainException("Cannot reject a {$lot->status} lot."); | |
| } | |
| $remaining = (float) $lot->quantity_available; | |
| if ($remaining > 0 && $lot->material) { | |
| $this->stockMovements->record( | |
| $lot->material, | |
| StockMovement::TYPE_SCRAP, | |
| -$remaining, | |
| user: $by, | |
| sourceType: StockMovement::SOURCE_RECLASSIFICATION, | |
| sourceId: $record->id, | |
| reason: $reason ?? 'Lot '.$lot->lot_number.' rejected — scrapped from stock', | |
| ); | |
| } | |
| $lot->update([ | |
| 'status' => MaterialLot::STATUS_REJECTED, | |
| 'quantity_available' => 0, | |
| ]); |
🧰 Tools
🪛 PHPStan (2.2.7)
[error] 120-120: Call to an undefined static method App\Models\MaterialReclassification::create().
(staticMethod.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/Services/Material/MaterialReclassificationService.php` around
lines 116 - 159, Reload the route-bound lot inside the DB::transaction closure
using a row lock, and move the `$fromStatus` assignment into that closure after
the locked reload. Ensure the rejection flow in the transaction uses this
locked, current `MaterialLot` instance when calculating `$remaining` and
recording scrap, preventing concurrent requests from processing the same
available quantity twice.
Comment on lines
+307
to
+312
| function submit(e) { | ||
| e.preventDefault(); | ||
| router.post(`/admin/work-orders/${workOrder.id}/allocations/${alloc.id}/consume`, | ||
| { consumed_qty: consumed, scrap_qty: scrap || 0 }, | ||
| { preserveScroll: true, onSuccess: onClose }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Block re-submission while a request is in flight.
All three submit handlers call router.post with no in-flight guard. The submit button stays enabled until the response arrives. A double click posts twice.
Each endpoint appends a non-idempotent row to the stock_movements ledger. A duplicate return therefore restores the quantity twice and releases the reservation twice.
Track a processing flag and pass it to ModalActions, which already accepts disabled.
🛡️ Proposed fix (shown for `ReturnModal`; apply the same pattern to `ConsumeModal` and `ReclassifyModal`)
function ReturnModal({ workOrder, alloc, onClose }) {
const returnable = Math.max(0, alloc.allocated_qty - alloc.consumed_qty - alloc.scrap_qty);
const [qty, setQty] = useState('');
const [reason, setReason] = useState('');
+ const [processing, setProcessing] = useState(false);
function submit(e) {
e.preventDefault();
+ if (processing) return;
+ setProcessing(true);
router.post(`/admin/work-orders/${workOrder.id}/allocations/${alloc.id}/return`,
{ qty, reason },
- { preserveScroll: true, onSuccess: onClose });
+ { preserveScroll: true, onSuccess: onClose, onFinish: () => setProcessing(false) });
}- <ModalActions onClose={onClose} submitLabel={__('Return to stock')} />
+ <ModalActions onClose={onClose} submitLabel={__('Return to stock')} disabled={processing} />Also applies to: 341-346, 375-380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/resources/js/Pages/admin/work-orders/Show.jsx` around lines 307 -
312, Add a processing state to ReturnModal, ConsumeModal, and ReclassifyModal;
guard each submit handler so it cannot post while a request is in flight, set
processing before calling router.post, and clear it via the request completion
callback. Pass processing to each modal’s ModalActions disabled prop so all
submit buttons remain disabled until the request completes.
Comment on lines
+429
to
+432
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/consume', [AdminWorkOrderController::class, 'recordConsumption'])->name('work-orders.allocations.consume'); | ||
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/return', [AdminWorkOrderController::class, 'returnAllocation'])->name('work-orders.allocations.return'); | ||
| Route::post('/work-orders/{workOrder}/reclassify', [AdminWorkOrderController::class, 'reclassify']) | ||
| ->middleware('role:Supervisor|Admin')->name('work-orders.reclassify'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Align the authorization strength of the consume and return routes with reclassify.
The reclassify route requires role:Supervisor|Admin. The consume and return routes carry no role middleware. Their only authorization is tab.access plus the Form Request check can('view', $allocation->batch->workOrder). Both actions move stock: returnQuantity records a TYPE_RETURN stock movement, releases the reservation, and shrinks the allocation.
A "view" ability is a read permission. Using it to authorize a write to the stock ledger means any role with the work-orders tab and view access can adjust inventory.
Either add role middleware to these two routes, or change the Form Requests to check a write ability such as update.
🔒 Proposed route-level fix
- Route::post('/work-orders/{workOrder}/allocations/{allocation}/consume', [AdminWorkOrderController::class, 'recordConsumption'])->name('work-orders.allocations.consume');
- Route::post('/work-orders/{workOrder}/allocations/{allocation}/return', [AdminWorkOrderController::class, 'returnAllocation'])->name('work-orders.allocations.return');
+ Route::post('/work-orders/{workOrder}/allocations/{allocation}/consume', [AdminWorkOrderController::class, 'recordConsumption'])
+ ->middleware('role:Supervisor|Admin')->name('work-orders.allocations.consume');
+ Route::post('/work-orders/{workOrder}/allocations/{allocation}/return', [AdminWorkOrderController::class, 'returnAllocation'])
+ ->middleware('role:Supervisor|Admin')->name('work-orders.allocations.return');Note: if operators are meant to declare consumption, keep the route open but confirm the intent explicitly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/consume', [AdminWorkOrderController::class, 'recordConsumption'])->name('work-orders.allocations.consume'); | |
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/return', [AdminWorkOrderController::class, 'returnAllocation'])->name('work-orders.allocations.return'); | |
| Route::post('/work-orders/{workOrder}/reclassify', [AdminWorkOrderController::class, 'reclassify']) | |
| ->middleware('role:Supervisor|Admin')->name('work-orders.reclassify'); | |
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/consume', [AdminWorkOrderController::class, 'recordConsumption']) | |
| ->middleware('role:Supervisor|Admin')->name('work-orders.allocations.consume'); | |
| Route::post('/work-orders/{workOrder}/allocations/{allocation}/return', [AdminWorkOrderController::class, 'returnAllocation']) | |
| ->middleware('role:Supervisor|Admin')->name('work-orders.allocations.return'); | |
| Route::post('/work-orders/{workOrder}/reclassify', [AdminWorkOrderController::class, 'reclassify']) | |
| ->middleware('role:Supervisor|Admin')->name('work-orders.reclassify'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/web.php` around lines 429 - 432, Align the authorization for
the consume and return routes with the stock-mutating reclassify route by adding
the `role:Supervisor|Admin` middleware to both `work-orders.allocations.consume`
and `work-orders.allocations.return`. Keep their existing handlers and route
structure unchanged.
Comment on lines
+87
to
+96
| public function test_zero_consumption_falls_back_to_planned_at_completion(): void | ||
| { | ||
| $this->withHeader('Authorization', 'Bearer '.$this->token($this->admin())) | ||
| ->postJson("/api/v1/material-allocations/{$this->allocation->id}/consume", ['consumed_qty' => 0]) | ||
| ->assertOk(); | ||
|
|
||
| // consumed_qty 0 → consumeForBatch treats it as fully planned (no leftover returned). | ||
| app(MaterialAllocationService::class)->consumeForBatch($this->batch); | ||
| $this->assertEqualsWithDelta(400.0, (float) $this->material->fresh()->stock_quantity, 0.0001); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Distinguish explicit zero consumption from no consumption record.
recordConsumption(..., 0) persists consumed_qty as zero. consumeForBatch() then treats zero as unrecorded and consumes the full allocation. This test therefore accepts inventory loss when an operator records zero actual usage.
Store an explicit recorded marker, or make unrecorded consumed_qty nullable. Return the full allocation when the recorded quantity is zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Feature/Api/V1/RecordConsumptionTest.php` around lines 87 - 96,
Update the consumption flow exercised by
test_zero_consumption_falls_back_to_planned_at_completion so recordConsumption
preserves the distinction between an explicit consumed_qty of 0 and no
consumption record. Use an explicit recorded marker or nullable unrecorded
value, and make consumeForBatch return the full allocation only for an
explicitly recorded zero while retaining the existing behavior for genuinely
unrecorded consumption.
Comment on lines
+115
to
+119
| public function test_guest_cannot_record_consumption(): void | ||
| { | ||
| $this->postJson("/api/v1/material-allocations/{$this->allocation->id}/consume", ['consumed_qty' => 10]) | ||
| ->assertUnauthorized(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Test authenticated users without reconciliation authorization.
Both endpoint test classes verify guest rejection and Admin success. Neither verifies that an authenticated user without the required work-order permission or role receives a denial.
backend/tests/Feature/Api/V1/RecordConsumptionTest.php#L115-L119: add an authenticated unauthorized-user test forconsume.backend/tests/Feature/Api/V1/ReturnAllocationTest.php#L140-L144: add an authenticated unauthorized-user test forreturn.
As per coding guidelines, “Tests are mandatory for new endpoints/business logic” and must cover “authorization (guest + wrong role).”
📍 Affects 2 files
backend/tests/Feature/Api/V1/RecordConsumptionTest.php#L115-L119(this comment)backend/tests/Feature/Api/V1/ReturnAllocationTest.php#L140-L144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Feature/Api/V1/RecordConsumptionTest.php` around lines 115 -
119, Add authenticated unauthorized-user coverage to
RecordConsumptionTest::test_guest_cannot_record_consumption by creating a
logged-in user without the required reconciliation permission or role and
asserting consume is denied; likewise update ReturnAllocationTest at
backend/tests/Feature/Api/V1/ReturnAllocationTest.php lines 140-144 with an
authenticated unauthorized-user test for return, while preserving the existing
guest and Admin authorization tests.
Source: Coding guidelines
Comment on lines
+66
to
+122
| public function test_admin_records_consumption(): void | ||
| { | ||
| $this->actingAs($this->admin()) | ||
| ->post($this->base()."/allocations/{$this->allocation->id}/consume", ['consumed_qty' => 70]) | ||
| ->assertRedirect() | ||
| ->assertSessionHas('success'); | ||
|
|
||
| $this->assertEqualsWithDelta(70.0, (float) $this->allocation->fresh()->consumed_qty, 0.0001); | ||
| } | ||
|
|
||
| public function test_admin_returns_unused_material(): void | ||
| { | ||
| $this->actingAs($this->admin()) | ||
| ->post($this->base()."/allocations/{$this->allocation->id}/return", ['qty' => 25]) | ||
| ->assertRedirect() | ||
| ->assertSessionHas('success'); | ||
|
|
||
| $this->assertEqualsWithDelta(425.0, (float) $this->material->fresh()->stock_quantity, 0.0001); | ||
| $this->assertEqualsWithDelta(75.0, (float) $this->allocation->fresh()->allocated_qty, 0.0001); | ||
| } | ||
|
|
||
| public function test_admin_reclassifies_to_another_class(): void | ||
| { | ||
| $target = Material::create([ | ||
| 'code' => 'M2', 'name' => 'Material 2', | ||
| 'material_type_id' => MaterialType::create(['code' => 'ALT', 'name' => 'Alt'])->id, | ||
| 'unit_of_measure' => 'kg', 'stock_quantity' => 0, | ||
| ]); | ||
|
|
||
| $this->actingAs($this->admin()) | ||
| ->post($this->base().'/reclassify', [ | ||
| 'source_material_id' => $this->material->id, | ||
| 'target_material_id' => $target->id, | ||
| 'qty' => 30, | ||
| ]) | ||
| ->assertRedirect() | ||
| ->assertSessionHas('success'); | ||
|
|
||
| // Source already at 400 after allocation; reclassify moves 30 more out. | ||
| $this->assertEqualsWithDelta(370.0, (float) $this->material->fresh()->stock_quantity, 0.0001); | ||
| $this->assertEqualsWithDelta(30.0, (float) $target->fresh()->stock_quantity, 0.0001); | ||
| } | ||
|
|
||
| public function test_guest_cannot_reconcile(): void | ||
| { | ||
| $this->post($this->base()."/allocations/{$this->allocation->id}/return", ['qty' => 25]) | ||
| ->assertRedirect('/login'); | ||
| } | ||
|
|
||
| public function test_allocation_from_another_work_order_is_404(): void | ||
| { | ||
| $otherWo = WorkOrder::factory()->create(['process_snapshot' => ['bom' => []]]); | ||
|
|
||
| $this->actingAs($this->admin()) | ||
| ->post("/admin/work-orders/{$otherWo->id}/allocations/{$this->allocation->id}/return", ['qty' => 10]) | ||
| ->assertNotFound(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the missing mandatory test categories.
The file covers the happy paths and the guest case. Three mandated categories are absent:
- Wrong role. No test asserts that a non-Supervisor and non-Admin user is refused on
/reclassify. Therole:Supervisor|Adminmiddleware on that route is therefore untested. Removing it would not fail the suite. - Validation 422. No test submits an invalid payload, for example a missing
qtyor atarget_material_idequal tosource_material_id. - Domain edge cases.
MaterialAllocationService::returnQuantityrejects a quantity above the returnable amount and rejects an allocation that is not inallocatedstatus. Neither guard is exercised through the web action, so the error flash path in the controller is untested.
This follows the guideline that tests are mandatory for new endpoints and business logic, covering happy path, validation 422, authorization (guest + wrong role), and domain edge cases.
💚 Proposed additional tests
public function test_operator_cannot_reclassify(): void
{
$operator = tap(User::factory()->create(), fn ($u) => $u->assignRole('Operator'));
$this->actingAs($operator)
->post($this->base().'/reclassify', [
'source_material_id' => $this->material->id,
'target_material_id' => $this->material->id + 1,
'qty' => 5,
])
->assertForbidden();
}
public function test_reclassify_rejects_identical_source_and_target(): void
{
$this->actingAs($this->admin())
->post($this->base().'/reclassify', [
'source_material_id' => $this->material->id,
'target_material_id' => $this->material->id,
'qty' => 5,
])
->assertSessionHasErrors('target_material_id');
}
public function test_return_above_the_returnable_quantity_is_refused(): void
{
$this->actingAs($this->admin())
->post($this->base()."/allocations/{$this->allocation->id}/return", ['qty' => 5000])
->assertRedirect()
->assertSessionHas('error');
// Stock is untouched: 500 less the 100 allocated.
$this->assertEqualsWithDelta(400.0, (float) $this->material->fresh()->stock_quantity, 0.0001);
}🧰 Tools
🪛 PHPStan (2.2.7)
[error] 89-89: Call to an undefined static method App\Models\Material::create().
(staticMethod.notFound)
[error] 91-91: Call to an undefined static method App\Models\MaterialType::create().
(staticMethod.notFound)
[error] 117-117: Call to an undefined static method App\Models\WorkOrder::factory().
(staticMethod.notFound)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/Feature/Web/Admin/WorkOrderMaterialReconciliationTest.php`
around lines 66 - 122, Add tests in WorkOrderMaterialReconciliationTest covering
the missing mandatory cases: an Operator must receive 403 when posting to
reclassify; reclassifying with identical source_material_id and
target_material_id must produce a validation error; returning more than the
allocation’s returnable quantity must redirect with an error flash and leave
material stock unchanged; and exercise the non-allocated status guard through
the return action if the fixture setup permits.
Sources: Coding guidelines, Learnings
- Concurrency: returnQuantity() and reclassifyStatus() now lock + re-read their row (lockForUpdate) inside the transaction before validation, so concurrent returns/rejects can't both pass the same check and over-move stock. - Recorded-zero semantics: new material_allocations.consumption_recorded flag. recordConsumption() sets it; consumeForBatch() honours a declared zero (return everything) instead of falling back to consuming the full allocation. Only a genuinely unrecorded allocation still falls back to the planned quantity. - Authorization: consume/return web routes moved under role:Supervisor|Admin (they move stock); reclassify now enforces the nested work-order scope (source material must be allocated to the order); RecordConsumption/ReturnAllocation requests guard a soft-deleted batch before dereferencing the work order. - Audit integrity: material_reclassifications.source_material_id uses restrictOnDelete so audit rows survive a material hard-delete. - UI: consume/return/reclassify modals guard against double-submit (processing flag → disabled button, cleared onFinish). - i18n: add the missing 'Work order :code created.' key to en + pl (parity kept). - Tests: partial multi-lot return (restore newest-first, reopen consumed lot, reduce/delete picks); authenticated-unauthorized (operator) for consume & return; web wrong-role, 422 identical source/target, and over-returnable error flash; recorded-zero vs unrecorded consumption. Backend 2325 passed; vitest 25; build OK; Pint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References #99.
Lets the shop floor reconcile a work order's pulled materials against what actually happened — over-issue, leftovers, regrading — with every change booked to the
stock_movementsledger. Built on the allocation/consumption path (MaterialAllocationService+StockMovementService), single-location stock. All additive; existing batch-completion consumption is unchanged.What & why
MaterialAllocationService::recordConsumption(). Bookkeeping only: stock already left at allocation time, andconsumeForBatchreturns the over-issued leftover at completion.returnQuantity(): booksTYPE_RETURN, releases the reservation, restores the picked lots (newLotPickingService::returnPartialForAllocation()), and shrinksallocated_qtyso the completion reconciler never returns the same quantity twice (the double-return guard).MaterialReclassificationService:TYPE_RECLASSIFYmovements (source −, target +).MaterialHoldService; reject scraps the lot's remaining quantity out of stock (TYPE_SCRAP) and zeroes it.TYPE_RECLASSIFY/SOURCE_RECLASSIFICATIONconstants and an append-onlymaterial_reclassificationsaudit table correlating the legs (a class move has two movements on two materials; a quarantine has none — the ledger alone can't represent either).Surfaces
POST /material-allocations/{id}/consume&/return(production users, authorized via the work-order policy);POST /material-reclassifications/class&POST /material-lots/{id}/reclassify-status(role:Supervisor|Admin).WorkOrder::allocations()relation added; i18n en/pl parity (+20 keys).Tests
Partial consumption (persist, no spurious delta, completion returns leftover, 422, auth); return incl. the double-return guard (consume 50 → return 50 → complete returns nothing more, reserved lands at baseline); reclassification class (paired movements sharing the audit-row id, source-lot decrement, auth) and status (quarantine w/o delta, reject scraps, illegal transition, auth); admin web actions.
Verification: backend
php artisan test— 2318 passed; vitest — 25;npm run build— OK; Pint clean on changed files.Scope note
This PR ships the admin work-order panel + full API (usable by any client). An operator-screen surface for declaring consumption/returns at the point of work is a planned fast-follow — the services and endpoints it needs are already in place here.
Summary by CodeRabbit