fix(spp_drims): show only reachable states on the dispatch status bar - #392
Conversation
action_create_dispatch confirms a request dispatch the moment it creates it, so a dispatch never sits in Draft. Both Draft and Waiting were still drawn as greyed-out steps on the form, inherited from stock.picking. Core renders two status bars for stock.picking, split on picking_type_code. A dispatch is outgoing, so it picks up the non-incoming one with draft,confirmed,assigned,done. Narrow that bar to exclude dispatches and add a dispatch-only bar showing assigned,done, rather than editing the shared statusbar_visible — which would have dropped Draft from every non-incoming transfer in the database. The three bars' conditions are mutually exclusive, so exactly one renders per picking. Waiting is hidden as a *future* step only, and deliberately so: contrary to the ticket's rationale it is genuinely reachable. DRIMS allocation writes quantity_allocated on the request line and creates no Odoo reservation, so two requests can allocate the same units and whichever dispatches second has nothing to reserve and lands in confirmed. That is safe here because the statusbar widget always renders the current value even when it is excluded from statusbar_visible (getAllItems filters on `value === currentValue || visibleSelection.includes(value)`), so a dispatch short of stock still shows Waiting to warehouse staff. Both facts are covered by tests so neither gets "simplified" later on a false assumption, and the mutual exclusivity test evaluates the real invisible expressions per picking kind rather than matching strings. OP#1086
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #392 +/- ##
==========================================
- Coverage 72.81% 72.54% -0.27%
==========================================
Files 448 1012 +564
Lines 31448 61220 +29772
==========================================
+ Hits 22899 44414 +21515
- Misses 8549 16806 +8257
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ch-statusbar # Conflicts: # spp_drims/views/stock_picking_views.xml
OP#1079 removed spp.drims.request.source_warehouse_id and made the request line's quantity_allocated a stored compute over allocation rows, so these fixtures no longer built a request at all.
…ating test_waiting_state_is_reachable_for_a_dispatch built its Waiting dispatch by allocating the same stock to two requests. Since PR #396 (OP#1079) that is refused - _drims_available_quantity subtracts pending allocations, so stock cannot be promised twice - and the test errored on its own setup. The ticket's reasoning still holds. Allocation records per-warehouse rows and reserves nothing, so it cannot guarantee the stock is still there when the dispatch is created: an inventory adjustment or transfer in between leaves the dispatch with nothing to reserve and it lands in confirmed. That is why Waiting is hidden only as a future step, never as the current value. Rebuilt around that instead. No production code changes.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
This is approve-quality work held back by one mechanical item. Everything load-bearing verified against the Odoo 19 source, all claims true verbatim:
- Core renders exactly the two statusbars described (
stock/views/stock_picking_views.xml:130-131), and your replacementinvisiblerestates core's condition exactly before adding the dispatch exclusion — the same wholesale-replacement discipline #433 used, applied correctly again. - Both widget citations check out:
statusbar_field.js:285keeps the current value even when excluded fromvisibleSelection— the fact that makes hiding Waiting safe — and:372confirmsstatusbar_visiblegenuinely is read from the field tag, correctly distinguished from #391's x2manyoptionstrap. Knowing which attributes Odoo reads from where is becoming this queue's quiet superpower. - The three bar conditions are mutually exclusive, and the test proves it by evaluating the real
invisibleexpressions against four picking shapes rather than string-matching attributes.
The best part is the epistemics: you checked the ticket's rationale before acting on it, found it wrong (allocation reserves nothing — "stock is already reserved from the allocation step" is not the case), reproduced the consequence on a dev instance, and then pinned both premises with tests so neither gets "simplified" later. The race test's docstring even refines the story correctly: since OP#1079 allocation won't double-promise, but nothing holds the stock, so the shelf can empty between allocation and dispatch and Waiting is genuinely reachable.
1. Blocker — version bump
The reviewer note carries the pre-ruling "post-merge" convention; per Edwin's since-settled ruling (see #391), the bump + HISTORY land in the PR, number reconciled at merge time in queue order. With it added, this is an approve.
2. Follow-up — the fact you uncovered deserves its own ticket
"Allocation creates no Odoo reservation" is a real design question, not just a rationale correction: should _allocate_stock_fifo reserve stock so an allocation actually holds it, or is reserve-at-dispatch the intended model (accepting that adjustments/transfers can starve a dispatch)? No existing OP ticket covers it (searched); filing one so the question gets decided deliberately rather than rediscovered by the next person who trusts the ticket's original rationale.
Adds the version bump the review asked for — 19.0.4.0.3, above the 19.0.4.0.2 #393 just landed — with its changelog entry. stock_picking_views.xml needed rebuilding rather than merging: #433, #391 and this branch all insert xpath blocks into the same region, and git's split of the conflict cut through an <xpath> element, so a mechanical resolution produced malformed XML. Took 19.0's file and re-applied this branch's own insertion — the OP#1086 comment plus the two statusbar xpaths — after the DRIMS button block. Verified as a pure insertion: no removals against 19.0, and the file parses. tests/__init__ takes every test module the branches registered.
The review's one blocker: 19.0.4.0.3, above the 19.0.4.0.2 #393 landed, with its changelog entry and the regenerated README.
|
Thanks — the bump is in. Pushed as 1. Version bump — On the merge itself, since it was not mechanical: Also of note: the README regeneration hook kept failing locally because pypandoc downloads pandoc into the repo and unpacks it with
2. The allocation-reservation question — agreed it is a real design question rather than a rationale correction, and it is recorded on OP#1086 so it does not get rediscovered. I have not opened a ticket for it: we are deliberately holding off on new follow-up tickets at the moment. If you would rather it be filed now, say so and I will, or file it yourself and I will link it from the ticket. |
Second drift back-merge (#392, #436, #438 landed on 19.0). Conflicts confined to spp_programs metadata: 19.0's #436 took 19.0.2.2.1, which the batch chain had assigned to #336 (with #353 at .2 carrying a migration). Resolution renumbers the batch chain one notch up: manifest -> 19.0.2.2.3, HISTORY ladder 2.2.3 = #353, 2.2.2 = #336, 2.2.1 = #436, and migrations/19.0.2.2.2/ renamed to 19.0.2.2.3/ so the dir matches the version claiming #353's change in the merged changelog (either name runs for real upgrade paths; only test DBs ever saw it as 2.2.2). README.rst/index.html regenerated via the pinned oca-gen hook. program_manager.py auto-merged; batch 2 (lock helpers) and #436 (protected-state filtering) touch disjoint functions.
2 participants
Why is this change needed?
action_create_dispatchconfirms a request dispatch the moment it creates it, so a dispatch never sits in Draft. Both Draft and Waiting were still drawn as greyed-out steps on the form, inherited fromstock.picking— visual clutter, and two states warehouse staff can never reach.How was the change implemented?
Core renders two status bars for
stock.picking, split onpicking_type_code:statusbar_visibledraft,assigned,donedraft,confirmed,assigned,doneA dispatch is outgoing, so it picks up the second one. Rather than editing that shared
statusbar_visible— which would have dropped Draft from every non-incoming transfer in the database — this narrows it to exclude dispatches and adds a third, dispatch-only bar:draft,assigned,done— untoucheddraft,confirmed,assigned,done— unchangedassigned,doneThe three conditions are mutually exclusive, so exactly one bar renders per picking. There is a test that evaluates the real
invisibleexpressions against four picking shapes to pin that, rather than pattern-matching the attribute strings.A correction to the ticket's rationale
The ticket states Waiting is unreachable because "stock is already reserved from the allocation step". That is not the case, and it was worth checking before hiding the state.
_allocate_stock_fifoonly writesquantity_allocatedon the request line; it creates no Odoo reservation. So two requests can allocate the same units before either dispatches, and the second finds nothing to reserve. Reproduced on a dev instance:Hiding Waiting is nevertheless safe, for a reason specific to the widget:
getAllItems()filters the selection withvalue === currentValue || visibleSelection.includes(value)(statusbar_field.js), so an excluded state still renders when it is the current one. Waiting therefore disappears only as a greyed-out future step — a dispatch genuinely short of stock still displays Waiting and stays visible to warehouse staff. That is what makes the AC implementable as written.Both facts are covered by tests (
test_dispatch_is_confirmed_on_creation_never_draft,test_waiting_state_is_reachable_for_a_dispatch) so neither gets "simplified" later on a false assumption.Worth noting for whoever reviews: unlike x2many
create/delete,statusbar_visiblegenuinely is read from the field tag (statusbar_field.js:372,visibleSelection: attrs.statusbar_visible?...), which is why core sets it that way.New unit tests
spp_drims/tests/test_dispatch_statusbar.py— 5 tests:assigned,done, correctly conditioned.Unit tests executed by the author
Full module suite:
All 5 new tests confirmed executing by name in the log.
./spp lint(ruff, ruff-format, prettier) passes on all three files.Also verified the combined arch on a fresh database: three status bars, with the conditions above.
How to test manually
demo-scripts/ds-openspp2-op1086-spp_drims-20260803-01.pybuilds one picking per case on a fresh DB, including a dispatch genuinely stuck in Waiting (via the real allocation race, not a contrived write).Related links
Notes for the reviewer
readme/HISTORY.mdand the manifest version are deliberately untouched — per our convention those land on19.0after merge, to avoid conflicts between concurrent PRs on the same module.This branch is independent of #390 (OP#1087) and #391 (OP#1057). It only touches the
<header>region ofstock_picking_views.xml, whereas #391 touches the Operations field in the same file, so they occupy different hunks and should merge in either order.🤖 Generated with Claude Code