fix(spp_drims): stop backorder dispatches bypassing the DRIMS request - #390
fix(spp_drims): stop backorder dispatches bypassing the DRIMS request#390emjay0921 wants to merge 7 commits into
Conversation
Validating a dispatch short of its demand and choosing "Create Backorder" left the request reading as fully dispatched, told no one, and attributed the parent shipment's per-shipment facts to goods still in the warehouse. Odoo builds a backorder with picking.copy(), so every field left at the default copy=True was inherited. Mark the per-shipment facts copy=False: beneficiary count, departure/arrival, the pod_* block, transport and driver details, discrepancy notes and drims_return_id. This also stops the Duplicate action producing a dispatch that claims a delivery, and it makes the beneficiary guard in button_validate() fire on the backorder instead of being pre-satisfied by the inherited value — which had let one 100-unit distribution to 500 people report 1000 beneficiaries served on spp.hazard.incident.drims_beneficiaries_served. Announce the backorder on the request: an internal note plus a to-do activity for the coordinators of the destination area, resolved by mirroring rule_request_coordinator_scope so it reaches exactly those permitted to see the request. Reopen the request at Ready for Dispatch while a backorder is pending, and re-advance to Dispatched once nothing is outstanding. Because action_create_dispatch counts a quantity as dispatched when it is committed to a picking rather than when it ships, that running total is now rebuilt from the moves that still stand whenever moves are validated or cancelled. A single reconciliation covers a cancelled backorder, a cancelled dispatch, and declining Create Backorder — the last of which cancels no move at all, it just drops the excess demand, so a cancellation hook alone would have missed it. Odoo already carries drims_request_id and drims_request_line_id onto the backorder and its split move, so the request link and per-line attribution were correct and are covered by a regression test. OP#1087
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #390 +/- ##
==========================================
- Coverage 72.66% 72.38% -0.28%
==========================================
Files 329 1010 +681
Lines 24298 60913 +36615
==========================================
+ Hits 17655 44091 +26436
- Misses 6643 16822 +10179
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Cancelling or declining a backorder stopped releasing the quantity after PR #396 merged. OP#1079 turned the request line's quantity_dispatched into a stored compute over per-warehouse allocation rows, and _reconcile_quantity_dispatched still assigned to the line. A second dispatch then refused with "Nothing left to dispatch on this request". Rebuild the allocation rows instead. Every dispatch move carries the allocation it draws from - action_create_dispatch is the only thing that creates them and always stamps drims_allocation_id, and a backorder copies the link - so splitting the reconciled total per allocation is unambiguous. Nothing was wrong when this ticket passed QA; the ground moved underneath it after merge. The tests were weaker than they looked. Writing a stored compute does persist, until something retriggers it, so only one of the three release tests failed - the other two asserted the line and passed on ordering luck while the allocation underneath was left at the full dispatched quantity. That is the figure _drims_available_quantity subtracts, so the released stock was never actually freed for re-allocation. All three now assert the allocation rows, and all three fail without the fix. Also records why each sudo() in this ticket's code is deliberate. Those findings are what has been failing pre-commit on PR #390 since it opened.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
This is the best-engineered PR of the current batch, and the PR body's rigor held up under verification — the measured reproductions, the "verified rather than assumed" items, and the OP#1079 subtlety are all real.
What I checked against the Odoo 19 source and this repo:
- Both hook signatures match exactly:
stock.move._action_done(cancel_backorder=False)andstock.picking._create_backorder(backorder_moves=None). drims_allocation_idpre-exists onstock.move, is stamped byaction_create_dispatch(request.py:843), and is kept distinct on move merges — so rebuilding the dispatched totals onto the allocation rows stands on solid ground. Catching that a write to the line's stored compute is silently discarded (and fixing it at the allocation level, with_assert_releasedpinning it in tests) is exactly right.button_validate'sresult is Truevs wizard-dict distinction is correct, and the pending-backorder check prevents state flip-flop during the short validation itself.- The reopen→re-dispatch loop closes:
allocatedis precisely whataction_create_dispatchrequires, and the cancel test proves the remaining balance re-dispatches. - The tests drive the real
stock.backorder.confirmationwizard (process()/process_cancel_backorder()), including the decline path that no cancellation hook could see, and the no-coordinator path.
1. Blocker — version bump
spp_drims stays at 19.0.3.0.0. This is the convention ask, not a breakage one: the change is pure Python plus field-attribute changes (copy=False is registry-level), so nothing malfunctions without -u — but the module ships in 2026.08 and same-version changes give upgraders no signal (#441). With the bump added I'm happy to approve.
One practical wrinkle you already flagged on OP#1087: with #391/#392/#393/#433 (plus #416/#414) all touching spp_drims, the concrete version number is a merge-time decision — suggest each PR takes the next patch version and prepends its HISTORY entry when it rebases for merge, in whatever order Edwin sets for the queue.
2. Non-blocking suggestions
- Re-advance hook placement:
_sync_state_after_dispatch_donefires only frombutton_validate. A backorder validated through another path (API, barcode flow, a direct_action_done) reconciles quantities via the move hook but never re-advances the request — it staysallocatedwith everything shipped. Hooking picking_action_doneinstead of (or in addition to)button_validatecloses that; your pending-backorder check already guards against premature advance. - Audit trail: the sudo'd
message_postmakes the note's author OdooBot, and neither the note nor the activity names who validated short. Worth including the acting user (captured before thesudo()) in the message body — in a humanitarian-accountability context, "who shipped short" is part of the record this PR otherwise builds so carefully. - Tiny:
_get_state_by_codeduplicates the search inside the existing_set_state_by_code— the setter could reuse the getter.
…ok, named audit trail Version bump to 19.0.3.0.1 with its changelog entry. Nothing malfunctions without an upgrade here — the change is Python plus registry-level field attributes — but the module ships in a release, and a same-version change gives upgraders no signal. The re-advance moves from button_validate to picking._action_done. It used to fire only for the web client's Validate button, so a backorder released through the API, the barcode flow or a direct _action_done reconciled its quantities through the move hook and then left the request sitting at "allocated" with everything already shipped. _action_done is the point every path goes through, and running after super() means any backorder has already been split off, which is what the sync inspects before advancing. The backorder note now names who validated short. It is posted through sudo, so OdooBot authors it; in a humanitarian-accountability trail "who shipped short" is part of the record. _set_state_by_code reuses _get_state_by_code instead of repeating its search. The Semgrep pragmas move onto the line each finding anchors to, matching the form used elsewhere in the repo. The rationale comments above each sudo already explained why it is needed; the placement is what the scanner reads. Two tests added: a backorder validated outside the web client advances the request, and the note names the acting user.
Moving the pragma onto the matched line dropped odoo-sudo-without-context, which the local semgrep hook then flagged. Both rules match that expression, so both ids belong on it.
|
Thanks — the blocker and all three suggestions are done, plus the Semgrep comments. Pushed as 1. Version bump — done. 2. Re-advance hook — moved to There is a test for it that deliberately bypasses 3. Audit trail — the note now names the validating user. Captured from 4. On the eight Semgrep comments: each of those Worth recording how that went, because it is a small lesson in trusting the tool over the reasoning: doing so initially introduced a finding. The
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — all four items addressed, and the hook relocation made the code simpler as well as more correct.
Verified on a08c1890 / a8f7d4e6:
- Bump to
19.0.3.0.1with its changelog entry, honestly framed as signal-only for this change — which matches the original assessment. - The re-advance now lives in
_action_done, aftersuper(), by which point any backorder has already been split off — which is exactly what_sync_state_after_dispatch_doneinspects before advancing. Every path funnels through it (web client, API, barcode, direct call), and the fragileresult is Truewizard-dict distinction inbutton_validateis gone entirely. The new test validating a backorder outside the web client pins the path that used to strand requests atallocated. - The note names who shipped short, with a test asserting the acting user's display name in the body. Since
sudo()hasn't switchedenv.usersince Odoo 13, the name captured is genuinely the officer — and carrying it in the body keeps the record robust regardless of how the message author renders. _set_state_by_codereuses_get_state_by_code, and the semgrep pragmas sit on their anchor lines with the rationale comments intact. CI fully green.
One merge-time line, same family as the rest of the queue: spp_drims now has four approved-or-approaching PRs carrying four different bump numbers (#414 → 4.0.0, #416 → 3.1.0, #433 → 3.0.4, this one → 3.0.1). All correct individually; each renumbers mechanically at its merge-time rebase in the order Edwin sets, and only the migration-carrying one (#416) has a directory to move with it.
Merge stays with Edwin per the usual flow.
3 participants
Why is this change needed?
Validating a DRIMS dispatch short of its demand and choosing Create Backorder processed the remainder entirely outside the request workflow. Reproduced on a dev instance before changing anything:
message_postappeared nowhere in the module).picking.copy(), so every field left at the defaultcopy=Truewas inherited. Three consequences, all measured:beneficiary_countwas inherited andhazard_incident.py:341sums it over every done dispatch. Validating the 10-unit backorder moveddrims_beneficiaries_served1500 → 2000 — one 100-unit distribution to 500 people reporting 1000 served.button_validate()returnedTruewith no prompt, because the inherited value already satisfied it. Nobody ever confirmed where the remaining units went.date_departedanddriver_name— a departure timestamp predating its own existence, on goods still in the warehouse.In a humanitarian context these are false coverage figures and undocumented aid movements, which is what the ticket flags as the real risk.
Not broken, and verified rather than assumed:
waybill_numberis alreadycopy=Falseand regenerates, anddrims_request_id/drims_request_line_idalready carry onto the backorder and its split move — so the request link and per-line attribution were correct. Both are now covered by a regression test instead of being "fixed".How was the change implemented?
copy=Falseon the per-shipment facts (stock_picking.py) — beneficiary count, departure/arrival, thepod_*block, transport and driver details, discrepancy notes,drims_return_id. Declarative, so it fixes every copy path rather than just backorders, including the Duplicate action producing a dispatch that claims a delivery. It also makes the existing beneficiary guard fire on the backorder.Coordinator notification (
request.py) —_create_backorder()routes DRIMS dispatch backorders to their request, which posts an internal note and schedules a to-do activity. Coordinators are resolved by mirroringrule_request_coordinator_scope(security/rules.xml), matching users whosedrims_area_idscover the destination area or any ancestor, so the notification reaches exactly those permitted to see the request. Runssudo()because a warehouse officer may sit outside the request's area scope and this is system bookkeeping, not a user edit.Request state — reopens at
allocated(Ready for Dispatch) while a backorder is pending, and re-advances todispatchedonce nothing is outstanding.Quantity reconciliation (
request_line.py,stock_move.py) —action_create_dispatchcounts a quantity as dispatched when it is committed to a picking rather than when it ships, so that running total is rebuilt from the moves that still stand on_action_done/_action_cancel. One reconciliation covers a cancelled backorder, a cancelled dispatch, and declining Create Backorder. That last path cancels no move at all — Odoo just drops the excess demand — so a cancellation hook alone would have missed it.quantity_dispatchedkeeps its existing "committed to a picking" meaning, so OP#1033's partial-dispatch behaviour is untouched.New unit tests
spp_drims/tests/test_dispatch_backorder.py— 12 tests:dispatchedwhile the backorder is pending, and returns todispatchedonce it ships.Unit tests executed by the author
Full module suite, run after merging the current
19.0:All 12 new tests confirmed executing by name in the log rather than inferred from the total. OP#1033's existing partial-dispatch assertions still pass.
./spp lint(ruff + ruff-format) passes on all six files.Also verified in the running app, not only in tests: the backorder shows
beneficiary_count=0, no departure, no driver and its own waybill while keeping request/type/incident/area; the request reopens toallocated; the coordinator receives both the note and an assigned activity; the guard fires; the incident counts 540; and on the cancel pathtotal_dispatcheddrops to 90 with Create Dispatch succeeding for the remaining 10.How to test manually
Requires a DRIMS warehouse with stock, and a user in DRIMS District Coordinator whose DRIMS Areas include the request's destination area.
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 against the same module. The entry for this change would befix(spp_drims): stop backorder dispatches bypassing the DRIMS requestat19.0.2.0.1.Two adjacent gaps found while investigating, both left alone as out of scope:
quantity_deliveredis never written by any production code in the module — only by tests. Sototal_deliveredandfulfillment_pctare permanently 0, andalert.py:276-282's "quantity needed" always reports the full requested amount. That is the delivered side, which OP#1088's Confirm Delivery popup owns.🤖 Generated with Claude Code