[FIX] stock_product_pack: don't skip non detailed packs on dont_create_move - #262
[FIX] stock_product_pack: don't skip non detailed packs on dont_create_move#262gal-adhoc wants to merge 1 commit into
Conversation
|
Hi @victoralmau, @pedrobaeza, |
OCA-git-bot
added
mod:product_pack
d2a357e to
24ff7f5
Compare
OCA-git-bot
removed
mod:product_pack
24ff7f5 to
b907b4e
Compare
dc05e39 to
61c8a19
Compare
Sadiq-OSI
left a comment
Sadiq-OSI
left a comment
There was a problem hiding this comment.
The procurement filtering itself looks correct, and the focused module tests pass.
I found two delivered-quantity edge cases that should be fixed before merging:
- Changing the ordered quantity after confirmation does not recompute
qty_delivered. - A draft quotation can be marked delivered when
dont_create_moveis enabled.
Please add regression coverage for both scenarios.
| # qty_delivered below, so it must be invalidated/recomputed whenever a | ||
| # child's own moves change (e.g. a return), not just when the pack | ||
| # line's own moves do. | ||
| @api.depends( |
There was a problem hiding this comment.
Please add product_uom_qty to this @api.depends list and add a regression test for editing the quantity after confirmation.
qty_delivered is assigned from product_uom_qty below, but changing a confirmed line from 2 units to 5 currently leaves qty_delivered at 2 because the stored compute is not invalidated. With invoicing based on delivered quantities, this can produce an incorrect invoice quantity.
There was a problem hiding this comment.
Fixed — added product_uom_qty to the @api.depends list. Added a regression test (test_dont_create_move_qty_change_after_confirm) that confirms a line, then bumps product_uom_qty from 1 to 5, and asserts qty_delivered follows it. Verified it fails without the depends fix.
| # former delivered upon confirmation, as dont_create_move's field | ||
| # help text documents. | ||
| for line in self.filtered( | ||
| lambda x: x.qty_delivered_method == "stock_move" |
There was a problem hiding this comment.
Please restrict this assignment to confirmed sale lines, for example by requiring x.state == "sale", and add a draft-order regression test.
The current condition applies in every state. If dont_create_move is enabled for a pack already used on a draft quotation, the dependency recompute changes qty_delivered from 0 to the full ordered quantity before confirmation. That contradicts the field help, which says the pack is delivered upon sale confirmation.
There was a problem hiding this comment.
Fixed — added x.state == "sale" to the filter. Added a regression test (test_dont_create_move_draft_quotation_not_delivered) that adds the line while draft, then toggles dont_create_move on (triggering the dependency recompute), and asserts qty_delivered stays 0 in both cases. Verified it fails without the state check.
61c8a19 to
9539278
Compare
…e_move stock.rule.run() only removed the parent pack procurement (so it doesn't create its own stock.move) when pack_type was 'detailed'. Non detailed packs with 'dont_create_move' active kept generating a stock.move for the parent product in the delivery, defeating the purpose of the flag. The exclusion now only depends on pack_ok + dont_create_move, so it applies regardless of the pack display type. Also avoids mutating the procurements list while iterating over it. Non detailed packs don't expand into separate component order lines (only detailed ones do), so once its own stock.move is skipped there's nothing else left to deliver it. sale_stock_product_pack's _compute_qty_delivered is extended to consider such a line delivered upon confirmation, matching dont_create_move's own field help text. Additionally, _compute_qty_delivered derives a detailed pack line's own qty_delivered from its components, but only depended on the pack line's own (untouched) stock moves. Returning a pack's components never invalidated the cached value on the parent line, leaving it stale (e.g. still showing the full quantity as delivered after a component-level return). Add explicit depends mirroring sale_stock's own on qty_delivered, one hop further through the components' moves.
9539278 to
a1b3f29
Compare
3 participants
Summary
stock.rule.run()only removed the parent pack's procurement (so it doesn't create its ownstock.move) whenpack_type == "detailed". A non detailed pack withdont_create_moveactive kept generating astock.movefor the parent product in the delivery, defeating the purpose of the flag.Additionally,
_compute_qty_deliveredderives a pack line's ownqty_deliveredfrom its components, but only depended on the pack line's own (untouched) stock moves -- returning a pack's components never invalidated the cached value on the parent line, leaving it stale.Change
stock_product_pack/models/stock_rule.py: the exclusion now depends only onpack_ok+dont_create_move, applying to bothdetailedandnon_detailedpacks. Also avoids mutating theprocurementslist while iterating over it (the previousfor ... procurements.remove(...)pattern could skip elements).sale_stock_product_pack/models/sale_order.py:stock.moveis skipped there's nothing else left to deliver it andqty_deliveredwould stay stuck at 0 forever._compute_qty_deliveredis extended to consider such a line delivered upon confirmation, matchingdont_create_move's own field help text ("will be set as delivered upon sale confirmation").@api.dependson the components' own stock moves, so a detailed pack'sqty_deliveredis correctly recomputed when its components are returned (previously stayed stale, e.g. still showing the full quantity delivered after a component-level return).Companion fix on the Adhoc side (same root cause, for a downstream
quantity_returnedfield instead ofqty_delivered): ingadhoc/sale#1766Test plan
test_dont_create_move_non_detailed_pack: confirms a sale order with a non detailed pack (dont_create_move=True), asserts the delivery has nostock.movefor the pack product, and thatqty_deliveredequals the ordered quantity.test_qty_delivered_pack_after_component_return: sells 2 packs (detailed), delivers, returns exactly 1 pack's worth of both components proportionally, and assertsqty_delivereddrops accordingly -- without any manual/forced recompute.product_pack/sale_product_pack/stock_product_pack/sale_stock_product_packtest suites pass againstoca/product-pack:19.0(28 tests, 0 failures).