[18.0][ADD] sale_order_add_consumed_components#3761
Conversation
rousseldenis
left a comment
There was a problem hiding this comment.
@bosd Could you check tests ?
|
A question about that:
If you bill the finished product, where is the risk of underbilling ??? |
e1b7b1d to
e1d79b0
Compare
In our use case, We are a contract manufacturer. The end products will be packaged. The packages that will be used are defined as Components on the bom. Without this module. The client will not be charged for the added "Packagings". Additional context: The "End products" are stored at the contract manufactureres facility. We want to trigger the Invoicing before we deliver the end products to the customer. As they can be held in storage for a long time. |
3010738 to
910938a
Compare
Done. Tests of this module are ok now. The 18 branch needs fixing see: #3667 |
🟢 now!! |
alexey-pelykh
left a comment
There was a problem hiding this comment.
Review: REQUEST_CHANGES
Thanks for the contribution -- the use case is clear and valuable. However, I found several issues that should be addressed before this can be merged.
Critical Findings
| # | File | Finding | Severity |
|---|---|---|---|
| 1 | models/mrp_production.py |
Fragile SO lookup via origin string match. origin can contain comma-separated references (e.g. "SO001, SO002"). Using search([("name", "=", mo.origin)]) will silently fail when multiple sources exist. Consider traversing procurement_group_id.sale_id or move_dest_ids to reliably find the linked SO. |
Critical |
| 2 | models/mrp_production.py |
_compute_mrp_production_ids is placed on mrp.production but operates as if it is on sale.order. The method iterates for sale in self: where self is mrp.production, and writes to sale.mrp_production_ids / sale.mrp_production_count which are sale.order fields. This method will either error at runtime or do nothing. It should be on a sale.order inheritance class if needed. |
Critical |
| 3 | models/mrp_production.py |
button_mark_done fires sync prematurely. button_mark_done() can return a wizard action (e.g. immediate production, backorder). The override calls action_add_consumed_components_to_sale() unconditionally after super(), meaning components may be added even if the user cancels the wizard or the MO is not yet truly done. The sync should happen only after the MO reaches done state. |
Critical |
Moderate Findings
| # | File | Finding | Severity |
|---|---|---|---|
| 4 | models/mrp_production.py |
Direct qty_delivered writes on existing SO lines without setting qty_delivered_method="manual". For newly created lines the method is set, but for existing lines found in so_line_map, qty_delivered is incremented directly. Odoo may recompute this field and overwrite the value. |
Moderate |
| 5 | models/mrp_production.py |
No UoM conversion. move.quantity uses the stock move UoM which may differ from the product's sale UoM or the SO line's UoM. Quantities should be converted before adding to the SO line. |
Moderate |
| 6 | models/mrp_production.py |
so_line_map assumes one SO line per product. If the SO has multiple lines for the same product (different descriptions, discounts, etc.), only the first is picked, and quantity is silently added to a potentially wrong line. |
Moderate |
| 7 | models/sale_order_line.py |
super() called with filtered subset. super(SaleOrderLine, lines)._action_launch_stock_rule(...) changes self for the entire MRO chain, which can break if other modules override this method expecting the full recordset. Consider filtering inside the method body instead. |
Moderate |
| 8 | demo/sale_demo.xml |
Reference to auto-generated product template XML ID (product_product_finished_product_template) -- verify this works in Odoo 18 as auto-ID patterns may have changed. Also, type="consu" may not be valid in Odoo 18 (replaced by different product type handling). |
Moderate |
| 9 | tests/test_sale_order_component_sync.py |
Tests call action_add_consumed_components_to_sale() directly instead of going through button_mark_done(). At least one test should exercise the full integration path. |
Moderate |
Minor / Style Findings
| # | File | Finding | Severity |
|---|---|---|---|
| 10 | models/mrp_production.py:22 |
f-string in logger call. OCA/Odoo style requires _logger.info("...", arg) (lazy evaluation), not f-strings. |
Minor |
| 11 | models/mrp_production.py |
Commented-out code (# 'qty_delivered': quantity, # 'route_id': False, # so_line.product_uom_qty = quantity). Remove before merge. |
Minor |
| 12 | models/sale_order_line.py |
Missing copyright/license header. OCA requires AGPL-3 headers on all Python files. | Minor |
| 13 | tests/test_sale_order_component_sync.py:166 |
try/except with self.fail() anti-pattern. Just let the exception propagate -- the test will fail automatically with a clearer traceback. |
Minor |
| 14 | readme/ROADMAP.md |
Empty file (just a newline). Either add content or remove it. | Minor |
| for mo in self: | ||
| if not mo.origin: | ||
| continue | ||
|
|
There was a problem hiding this comment.
The origin field can contain comma-separated SO references (e.g. "SO001, SO002"). This search will silently fail in that case.
Consider using procurement_group_id.sale_id or traversing move_dest_ids to reliably resolve the linked Sale Order.
| # Fix the domain so the smartbutton on the SO | ||
| # will not show the individual components | ||
| @api.depends("procurement_group_id") | ||
| def _compute_mrp_production_ids(self): |
There was a problem hiding this comment.
This method is placed on mrp.production but iterates for sale in self: and writes to sale.mrp_production_ids / sale.mrp_production_count, which are sale.order fields.
This will either error at runtime or be a no-op. If the intent is to customize the SO smart button, this override should be on a sale.order class instead.
| ) | ||
|
|
||
| def button_mark_done(self): | ||
| res = super().button_mark_done() |
There was a problem hiding this comment.
button_mark_done() can return a wizard action (immediate production, backorder confirmation). This override fires the component sync unconditionally after super(), even if the MO is not yet truly in done state.
Consider checking mo.state == 'done' before calling the sync, or hook into _post_mark_done() / a state-change method instead.
| if product.id in so_line_map: | ||
| # Update existing sale order line quantity | ||
| so_line = so_line_map[product.id] | ||
| so_line.product_uom_qty += quantity |
There was a problem hiding this comment.
For existing SO lines, qty_delivered is incremented directly without ensuring qty_delivered_method = 'manual'. Odoo may recompute this field and overwrite your value.
Also, no UoM conversion is performed -- move.quantity uses the stock move's UoM which may differ from the SO line's UoM.
| sale_order = self.env["sale.order"].search( | ||
| [("name", "=", mo.origin)], limit=1 | ||
| ) | ||
| if not sale_order: |
There was a problem hiding this comment.
OCA style: use _logger.info("No Sale Order found with origin %s", mo.origin) instead of f-strings in logger calls (lazy evaluation).
|
|
||
| def _action_launch_stock_rule(self, previous_product_uom_qty=False): | ||
| lines = self.filtered(lambda ln: not ln.is_mrp_component_line) | ||
| return super(SaleOrderLine, lines)._action_launch_stock_rule( |
There was a problem hiding this comment.
Calling super(SaleOrderLine, lines) with a filtered subset changes self for the entire MRO chain. Other modules overriding _action_launch_stock_rule may break because they won't see the full recordset.
Consider overriding to filter within the method body, or use a different hook to prevent stock rule launches for component lines.
| @@ -0,0 +1,13 @@ | |||
| from odoo import fields, models | |||
There was a problem hiding this comment.
Missing copyright/license header. OCA requires AGPL-3 headers on all Python source files.
| move.quantity = 1.0 | ||
|
|
||
| self.mo.action_add_consumed_components_to_sale() | ||
| return self.mo |
There was a problem hiding this comment.
All tests call action_add_consumed_components_to_sale() directly, bypassing button_mark_done(). Please add at least one test that exercises the full flow through button_mark_done() to verify the integration trigger works correctly.
Add consumed components from Manufacturing Orders to the originating Sales Order as invoiceable lines. Use case: contract manufacturing where the customer provides raw material, and the customer is charged for packagings/components consumed during production.
a426dfa to
0cf0b7f
Compare
|
🟢 Done! Attended to review comments. Squashed all the commits. Added unit tests. |
…line Use get_product_multiline_description_sale() instead of display_name when creating component lines on the SO. This includes the product's sale description (description_sale field) in the partner's language, matching the behavior of manually adding a product to an SO line.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the rework, @bosd. Re-reviewed against the current diff and my earlier findings are all addressed:
- SO lookup is robust now:
_find_origin_sale_order()tries the procurement group, then downstreammove_dest_ids.sale_line_id, then a comma-splitname in [...]fallback. - The stray compute that iterated
mrp.productionas if it weresale.orderis gone. button_mark_doneonly syncs for MOs that actually reachedstate == "done"aftersuper().- Existing lines are written with
qty_delivered_method="manual", and UoM is converted via_get_move_qty_in_product_uom. so_line_mapis scoped tois_mrp_component_linelines only, so it won't clobber the finished-product line.
The new test suite is great — test_button_mark_done_integration, test_uom_conversion, test_multiple_mos_aggregate_quantity, plus the idempotency and origin-resolution tests exercise exactly the paths in question. CI is green.
LGTM.
Co-Reviewed-By: Claude Opus 4.8 noreply@anthropic.com
|
@rousseldenis Cam you re-review? |
Business Need In many manufacturing workflows, products are sold to
customers via Sales Orders and subsequently manufactured through
Manufacturing Orders (MOs). Often, these manufactured products require
various components that are consumed during production and tracked via
Bills of Materials (BoMs).
However, in certain business scenarios, it's necessary not only to
manufacture a finished product but also to invoice the customer for the
individual components used in production, especially when the components
themselves are valuable, consumable, or customer-specific.
Odoo, by default, does not automatically add consumed components to the
related Sale Order. This leads to:
Module Purpose This module addresses the above gap by automatically
adding all
sale_ok=Truecomponents that were actually consumed in aManufacturing Order to the linked Sale Order. It ensures:
Compatibility with multiple MOs linked to the same SO (incrementing
quantities when the same component is used more than once).
Real word use case: This module is very usefull in a subcontractee
scenario. E.g. the customer is the owner of the component and
endproduct. But the subcontractee is providing operations over the
product and adding components. These components need to be invoiced
separately to the customer. In this scenario, the raw material supplied
by the customer is not saleble. (He already owns it and gives it in
consinee to the subcontractee.) The added components should be set up as
salebale products.
With this module installed and the scenario where consumption of the
components are added to a mo. Upon "mark done" of the MO the consumed
components are added to the sale order. Making them invoicable to the
customer.