feat(bom): multi-level BOMs with recursive requirement explosion - #211
Merged
Conversation
A BOM line could only reference a purchased material, so products built from in-house subassemblies had no way to express their real structure and MRP stopped at the first level. Adds make-or-buy to the item master — materials.is_manufactured plus producing_process_template_id — so a line pointing at a manufactured material expands into that material's own BOM, to any depth. bom_items is untouched, which is what keeps existing single-level BOMs and API clients working: a BOM with no manufactured components explodes to exactly the numbers the flat calculation produced (asserted). Quantities and scrap compound down the tree (qty_per_unit x parent requirement x (1 + scrap%)), and a component reached by several paths is summed once. NetRequirementsService now plans against exploded leaves rather than level one. Circular references are rejected at both entry points — a BOM line that would make a template a component of itself, and an item master pointing its producing template at a template that already consumes it — each as a 422 with a translated message, plus a depth backstop so pre-existing bad data cannot hang a request. New App\Services\Material\BomExplosionService: tree() for the nested structure, leafRequirements() for accumulated leaves. Out of scope here (follow-ups): UI tree view, REST endpoints for the hierarchy, work-order snapshotting of the structure, cross-level genealogy, and parent / child work orders. Refs #178 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbLa6GeJZACX3tFCE7iNJq
|
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:
📝 WalkthroughWalkthroughAdds hierarchical BOM support through manufactured-material metadata, recursive explosion with compounded quantities and scrap, cycle prevention, leaf-based MRP requirements, backward-compatible defaults, and feature coverage for nested structures and validation failures. ChangesHierarchical BOM support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessTemplate
participant BomExplosionService
participant Material
participant NetRequirementsService
ProcessTemplate->>BomExplosionService: expand BOM tree
BomExplosionService->>Material: resolve manufactured components
BomExplosionService-->>NetRequirementsService: aggregated leaf requirements
NetRequirementsService-->>ProcessTemplate: planned leaf material demand
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Collaborator
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/Models/Material.php (1)
21-45: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRegister the new manufacturing columns on
materialsinShapeRegistry.php.
Backend\App\Sync\ShapeRegistrymapsmaterialsonly through existing columns;is_manufacturedandproducing_process_template_idare missing from itscolumnslist, so synced clients won’t be able to read/write them. Add both columns there, or remove/ignore the PR guidance ifmaterialsis not a shape-synced entity.🤖 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/Models/Material.php` around lines 21 - 45, Update Backend\App\Sync\ShapeRegistry’s materials column mapping to include both is_manufactured and producing_process_template_id, ensuring shape-synced clients can read and write the new manufacturing fields. If materials is intentionally not shape-synced, remove or ignore the related PR guidance instead.Source: Coding guidelines
🤖 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/Requests/Api/V1/StoreMaterialRequest.php`:
- Around line 25-26: Enforce the make-or-buy invariant in the material
validation rules: a non-null producing_process_template_id must require an
effective is_manufactured=true value, or normalize/reject the relationship when
manufacturing is false or absent. Apply this to creation in
backend/app/Http/Requests/Api/V1/StoreMaterialRequest.php lines 25-26 and
backend/app/Http/Requests/Web/Admin/StoreMaterialRequest.php lines 47-48; for
partial updates, use the existing material state in
backend/app/Http/Requests/Api/V1/UpdateMaterialRequest.php lines 26-27 and
backend/app/Http/Requests/Web/Admin/UpdateMaterialRequest.php lines 51-52.
In `@backend/app/Models/Material.php`:
- Around line 139-156: Update Material::isExplodable() to return true only when
the material is manufactured, has a producing_process_template_id, and
producingTemplate resolves to a non-deleted ProcessTemplate; add regression
coverage confirming materials referencing soft-deleted templates are treated as
non-explodable and do not reach explode(ProcessTemplate $template) with null.
---
Outside diff comments:
In `@backend/app/Models/Material.php`:
- Around line 21-45: Update Backend\App\Sync\ShapeRegistry’s materials column
mapping to include both is_manufactured and producing_process_template_id,
ensuring shape-synced clients can read and write the new manufacturing fields.
If materials is intentionally not shape-synced, remove or ignore the related PR
guidance instead.
🪄 Autofix (Beta)
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: 4c05bcce-ad20-4ec9-abb7-6df0a474d566
📒 Files selected for processing (14)
CHANGELOG.mdbackend/app/Http/Requests/Api/V1/StoreMaterialRequest.phpbackend/app/Http/Requests/Api/V1/UpdateMaterialRequest.phpbackend/app/Http/Requests/Web/Admin/StoreMaterialRequest.phpbackend/app/Http/Requests/Web/Admin/UpdateMaterialRequest.phpbackend/app/Models/Material.phpbackend/app/Services/Material/BomExplosionService.phpbackend/app/Services/Material/BomService.phpbackend/app/Services/Material/NetRequirementsService.phpbackend/database/factories/MaterialFactory.phpbackend/database/migrations/2026_07_26_100000_add_manufacturing_to_materials_table.phpbackend/lang/en.jsonbackend/lang/pl.jsonbackend/tests/Feature/Material/HierarchicalBomTest.php
- isExplodable() only checked the FK, but process templates soft-delete and nullOnDelete doesn't fire on a soft delete — so a soft-deleted producing template left the id set while the relation resolved to null, and explosion descended into a null template (TypeError). It now requires the relation to resolve; such a material falls back to a leaf. Reproduced with a failing test first, which is in this commit. - Added is_manufactured and producing_process_template_id to the materials Electric shape, per the project rule that a new column on a synced table must be allowlisted or the UI never sees it. Guarded by a test. Not applied as suggested: CodeRabbit wanted the four material Form Requests to reject a producing template when is_manufactured is false. That combination is meaningful — a make-or-buy item can be bought for now while its routing stays on file — and explosion requires both halves, so neither on its own yields an inconsistent state. Documented the intent in the migration and locked it with a test instead of adding validation that would break the dual-source case. Refs #178 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YbLa6GeJZACX3tFCE7iNJq
Collaborator
Author
|
@jakub-przepiora ready to review |
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
2 participants
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.
Summary
A BOM line could only reference a purchased material, so products built from in-house subassemblies couldn't express
their real structure and MRP stopped at the first level.
Adds make-or-buy to the item master (
materials.is_manufactured+producing_process_template_id): a line pointingat a manufactured material expands into that material's own BOM, to any depth.
bom_itemsis untouched, so existingsingle-level BOMs and API clients keep working.
Quantities and scrap compound down the tree; a component reached by several paths is summed once. MRP now plans
against exploded leaves. Circular references are rejected at both entry points (BOM line and item master) as a 422,
with a depth backstop.
New
App\Services\Material\BomExplosionService—tree()andleafRequirements().Type of change
Related issue
Refs #178 — does not close it. Lands the model, recursion and safety rails. Deferred: UI tree, REST endpoints,
work-order snapshotting, cross-level genealogy, parent/child work orders.
Testing
14 new tests in
tests/Feature/Material/HierarchicalBomTest.php: three levels created and exploded, leaf quantitiesthrough the hierarchy, scrap compounding, shared components summed once, five acyclic levels, direct + indirect cycles
rejected, item-master mirror rejected, and a backward-compat check that a single-level BOM gives numbers identical to
calculateRequirements. Migration rollback and query cost verified separately.php artisan testpasses — 1984 tests, 0 failures (viaphp -d memory_limit=2G vendor/bin/phpunit; theartisan testwrapper OOMs locally at the default 128M limit, unrelated to this change)Checklist
.envsecrets committeddown()verified$fillableupdated if new model columns addedcomposer auditclean — 37 pre-existing advisories; no dependencies added or changed hereNotes for reviewers
deliberately out of scope (noted in code).
savinghook so every write path is covered without duplicating itacross four Form Requests.
materialswas not added to the Electric shape — fine with no UI, but required when the tree view lands.Summary by CodeRabbit
New Features
Bug Fixes