feat(spp_drims): collect proof of delivery in a popup off the dispatch - #393
Conversation
The Confirm Departure and Confirm Delivery buttons sat in their own column beside Distribution Details, far enough from Departure & Arrival to read as unrelated. Each now sits on the row of the field it fills. Confirm Delivery stays hidden until departure is recorded, with a hint in its place. The ticket asked for it greyed out, which an Odoo form cannot express: ViewButton's `disabled` is a component prop and is never wired to the arch, so `invisible` is the only declarative option. The ordering is enforced on the model as well, in action_open_delivery_confirmation and action_confirm_pod, so no path can log an arrival for goods that never left. Confirming delivery now opens spp.drims.delivery.confirmation.wizard instead of expecting the officer to have typed the receiver's details into the form and then pressing a button that refused if they had not. The wizard collects receiver, delivery status, signature, photos, GPS and notes, writes them back to the POD block, and locks that block once confirmed so the delivery record is not casually edited afterwards. It also records what actually arrived per line. Nothing in the module wrote spp.drims.request.line.quantity_delivered before this, so total_delivered and fulfillment_pct sat at 0 however much had been delivered, and spp.drims.alert kept reporting the full requested quantity as still needed. Quantities default to everything dispatched, are capped at it, and accumulate so a request filled by several dispatches totals correctly rather than overwriting. Lines are populated in both default_get and create. default_get alone only covers callers that pass the picking through the context; passing picking_id in the values — the obvious way from a script or over RPC — produced a wizard with no lines that silently recorded nothing. A test pins that path. test_pod_confirmation asserted the old behaviour of confirming delivery with no departure recorded, and is updated for the new ordering. OP#1088
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #393 +/- ##
==========================================
- Coverage 72.72% 72.54% -0.19%
==========================================
Files 447 1012 +565
Lines 31339 61220 +29881
==========================================
+ Hits 22792 44411 +21619
- Misses 8547 16809 +8262
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Confirming a delivery from the popup failed with "Expected singleton: uom.uom()". The wizard line held its move, request line, product, unit and dispatched quantity as plain readonly fields, and the web client does not send readonly fields back when it saves, so a confirmation arrived carrying nothing but quantity_delivered. The empty unit turned uom_id.compare into a bare singleton error. The crash was the fortunate part. The request line came back empty too, so had the check been more forgiving the confirmation would have recorded no delivered quantities at all and left fulfillment reading 0 - the very number this wizard exists to write. Derive the line from its move instead: move_id is required and writable, and the request line, product, unit and dispatched quantity are related off it, so the move is the only value that has to survive the round trip. The quantity check also falls back to float_compare, so a missing unit can never produce a raw singleton error again. Move the departure and delivery buttons left as well, by hiding each date while it is still empty. An empty datetime widget was pushing the button across the row. The field and its button are mutually exclusive: an unset date means there is an action to take, a set one means it is done. Round 1 passed every server-side test and still shipped broken, because the tests built the wizard themselves with all values supplied. The new test sends the payload the browser actually sends, and a second one guards move_id against going readonly again.
PR #396 landed on 19.0 after this branch last merged up. OP#1079 removed spp.drims.request.source_warehouse_id and made the request line's quantity_allocated a stored compute over allocation rows, so the fixture no longer built a request at all and took the whole delivery-confirmation class with it.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Same standard as #390 and #391 — the rigor is appreciated and it verified out. What I checked:
- The delivered-total arithmetic is sound: per-line cap at the move's dispatched quantity, once-per-picking via the
is_pod_confirmedguard, and accumulation onto the request line — so the total can't exceed what was dispatched, across any number of dispatches. Andquantity_deliveredis a plain writable Float, so this isn't the stored-compute trap #390 fixed on the dispatch side. - The round-2
move_idderivation is the correct fix — the web client not sending readonly fields back is exactly the kind of thing that silently zeroes a feature, and deriving product/uom/request-line from the move closes it structurally. - Both line-population paths covered (
default_getfor the context path,createfor the values path), with the values-path bug pinned by a test. Finding it via fixtures and documenting it honestly is the right craft. - The GPS constraint rescuing
_compute_pod_gps_point's silent drop, the model-side ordering guards, and deferring thedelivered/fulfilledstate-machine question to its own ticket are all right.
1. Blocker — version bump (policy settled)
As ruled on #391: bump + HISTORY land in the PR, number assigned at merge-time rebase in Edwin's queue order. Please add them when this rebases for merge.
2. Blocker — the wizard on a departed-but-unvalidated dispatch records nothing, then locks
action_open_delivery_confirmation checks type / departed / not-confirmed — but not state == 'done' — and _prepare_line_commands only reads done moves. The gap sits exactly between two of your own PRs' flow assumptions: #390's documented flow (and its tests) confirm departure before validating, while this PR's tests always validate first. An officer following #390's order who clicks Confirm Delivery before Validate gets a zero-line wizard; confirming writes the POD block, sets is_pod_confirmed, records no delivered quantities — and the already-confirmed guard then prevents ever recording them. Fulfillment stays 0 with no recovery path short of manual field surgery.
Two small fixes, ideally both: require state == 'done' in action_open_delivery_confirmation ("Validate the transfer before confirming delivery"), and refuse action_confirm when line_ids is empty. A test pinning the departed-but-unvalidated path would keep the two flows honest against each other.
3. Nit — photo provenance
pod_photo_ids uploads through many2many_binary on the wizard form, so the attachments carry res_model = spp.drims.delivery.confirmation.wizard. They survive transient vacuum, but the provenance is wrong and access flows through transient-model semantics. Re-pointing res_model/res_id to the picking in action_confirm keeps the evidence record clean.
Both follow-up tickets you've named across these PRs (the request state-machine design question, and #391's dead constants) are being filed on OpenProject so they don't evaporate.
…alidated Version bump with its changelog entry, per the settled convention. The wizard's lines come from done moves, so on a departed-but-unvalidated dispatch it opened empty — and confirming an empty wizard wrote the proof of delivery, set is_pod_confirmed and recorded no delivered quantities, after which the already-confirmed guard prevented ever recording them. Fulfilment stayed at zero with no way back short of editing fields by hand. The trap is reachable by following OP#1087's documented order, which confirms departure before validating. Two guards rather than one: the opener now requires state 'done', and action_confirm refuses when there are no lines, so the direct API cannot burn the one-shot flag either. Delivery photos upload through the wizard form, so they were filed against the transient model. They are re-pointed to the picking on confirm, which is where the evidence belongs and whose access rules should govern it.
|
Thanks — both blockers and the nit are done. Pushed as 1. Version bump — 2. The departed-but-unvalidated trap — fixed, and it was worse than a poor error message. You traced it exactly: an officer following #390's documented order gets a zero-line wizard, and confirming it writes the POD block, burns the one-shot
Three tests: the departed-but-unvalidated path through the button, the empty-lines path straight at the wizard (asserting the flag is not burned), and the photo provenance below. The first one is the one that keeps the two PRs' flow assumptions honest against each other — it builds a picking in exactly the state #390's flow produces. 3. Photo provenance — re-pointed.
On the two follow-up tickets: the dead constants are already filed and fixed as OP#1165 / PR #438. The request state-machine design question is still unfiled — happy for you to raise it, or I will. |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — both blockers and the nit are fixed, each with its own pinning test.
Verified on f3fede6a:
- The zero-line lock is closed with belt and braces:
action_open_delivery_confirmationnow requiresstate == 'done'(with the comment naming the exact trap — reachable by following #390's documented depart-then-validate order), andaction_confirmindependently refuses an empty wizard so a direct API construction can't burn the one-shotis_pod_confirmedflag either. The two new tests pin both layers, and the no-lines test asserts the flag is not burned — which is the harm that mattered. - Photo provenance re-pointed: attachments move to
res_model = stock.picking/ the dispatch's id on confirm, with a test asserting both. Evidence now lives where its access rules should govern it. - Bump to
19.0.3.0.3with its HISTORY entry, slotting into the queue's number line (#390 → .1, #391 → .2, this → .3, #433 → .4, #416 → 3.1.0, #414 → 4.0.0) — reconciled at merge time per the settled convention.
CI fully green. The follow-ups born from this PR remain tracked: OP#1174 (the delivered/fulfilled state-machine question) stands, and #433 remains the rebase-heavy neighbour whichever of the two lands second, since both rework the same DRIMS-tab block. Merge stays with Edwin per the usual flow.
Renumbered to 19.0.4.0.2, above the 19.0.4.0.1 #391 just landed. The manifest conflicted twice — the version and the data-file list, where each branch had registered its own wizard view; both views are kept. tests/__init__ and wizard/__init__ likewise take every module each side added. stock_picking_views.xml merged cleanly despite the DRIMS-tab rename #433 made, and the delivery-confirmation button still sits inside the renamed tab.
Drift back-merge per the staging plan (19.0 advanced 11 commits since the batch-2 cut: #303, #322, #323, #390, #391, #393, #411, #412, #414, #416, #433). Conflicts were confined to spp_farmer_registry_demo metadata: batch 2's #336 demo fix claimed version 19.0.2.1.2, which 19.0 has since consumed (#412=.2, #322=.3, #323=.4). Resolution re-bumps the batch-2 farmer-demo change to 19.0.2.1.5 with its HISTORY entry moved on top; README.rst/index.html regenerated via the pinned oca-gen hook. The change ships no migration directory, so the collision was metadata-only. The generator code change itself auto-merged cleanly.
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.
…#392) * fix(spp_drims): show only reachable states on the dispatch status bar 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 * test(spp_drims): follow the per-warehouse allocation model 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. * test(drims): produce Waiting by losing the stock, not by double-allocating 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. * fix(spp_drims): bump version for the dispatch status bar change 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. * docs(spp_drims): regenerate README for the 19.0.4.0.3 entry
2 participants
Why is this change needed?
Three problems on the dispatch form, all in the ticket:
action_confirm_podthen refused with "Please enter the receiver's name" if you had not — the button told you what you should have done rather than letting you do it.How was the change implemented?
Layout — each button now sits on the row of the field it fills, via
<div class="o_row">inside the Departure & Arrival group.Ordering — Confirm Delivery is hidden until
date_departedis set, with a muted "Confirm departure first" hint in its place.The ticket asked for the button greyed out, which an Odoo form cannot express:
ViewButton'sdisabledis a component prop andform_compiler.jsnever wires it to the arch, soinvisibleis the only declarative option. Flagged rather than silently substituted. The ordering is also enforced on the model — inaction_open_delivery_confirmationand inaction_confirm_pod— so no path can log an arrival for goods that never left, whatever the UI does.The popup — new
spp.drims.delivery.confirmation.wizard: receiver name/title/ID and delivery status up front, then tabs for Delivered Items, Evidence (signature, GPS, photos) and Notes & Discrepancies. On confirm it writes the existing POD block on the picking, which then becomesreadonly="is_pod_confirmed"so the delivery record is not casually edited afterwards.Delivered quantities — the popup also records what actually arrived per line. This is the part worth a reviewer's attention: nothing in the module wrote
spp.drims.request.line.quantity_deliveredbefore this, sototal_deliveredandfulfillment_pctsat at 0 however much had been delivered, andspp.drims.alertkept reporting the full requested quantity as still needed. Both #390 and #391 flagged this as belonging here. Quantities default to everything dispatched, are capped at it, and accumulate so a request filled by several dispatches totals correctly instead of overwriting.A bug the tests missed and the fixtures caught
Lines are populated in both
default_getandcreate.default_getalone only covers callers that pass the picking through the context — which is what the UI does, so the suite was green — but passingpicking_idin thecreate()values, the obvious way from a script or over RPC, produced a wizard with no lines that silently recorded nothing.create()only routes missing fields throughdefault_get, so the early return fired. Found by running the demo script rather than by the tests; there is now a test pinning that exact path.New unit tests
spp_drims/tests/test_delivery_confirmation_wizard.py— 15 tests:fulfillment_pctgoes 0 → 100; a short delivery records only what arrived; quantities accumulate across two dispatches._compute_pod_gps_point; departure cannot be re-recorded after delivery.Unit tests executed by the author
Full module suite:
All 15 new tests confirmed executing by name in the log.
./spp lint(ruff, ruff-format, prettier) passes. Module upgrades cleanly; both wizard models registered with 8 ACL rows across manager / officer / warehouse staff / coordinator, mirroring the create-return wizard's grants.Verified in the running app on a fresh database via
demo-scripts/ds-openspp2-op1088-spp_drims-20260804-01.py(7/7, exits non-zero on regression), and the rendering was checked by hand: buttons sit inline with their fields, the popup's quantity column is editable, the POD block populates and locks, and the request's Fulfillment % reads 100.How to test manually
The script above leaves three dispatches, one per state.
Related links
Notes for the reviewer
Behaviour change:
action_confirm_podnow refuses when departure has not been recorded.test_pod_confirmationasserted the old behaviour and is updated for the new ordering. Anything calling that method directly needs departure first. The method is kept for programmatic callers; the form routes throughaction_open_delivery_confirmation.Deliberately out of scope: the request state machine is still unwired. A fully delivered request stays at
dispatchedeven now that Fulfillment reads 100%, because nothing sets thedeliveredorfulfilledstates — and the request-states vocabulary contains both with no documented distinction between them. That is a design decision rather than an oversight, so it wants its own ticket instead of being folded in here.readme/HISTORY.mdand the manifest version are untouched — per our convention those land on19.0after merge, to avoid conflicts between concurrent PRs on the same module.Overlap with the other open DRIMS PRs: this touches the DRIMS tab of
stock_picking_views.xmlandaction_confirm_pod/action_confirm_departureinstock_picking.py. #391 (OP#1057) touches the Operations field andbutton_validate; #392 (OP#1086) touches the header statusbars; #390 (OP#1087) touches field definitions andbutton_validate. Different hunks throughout, so they should merge in any order, but this is the largest of the four and may want a rebase check if it lands last.🤖 Generated with Claude Code