From 9d19703d66c22c7fa5c12fab28ed3af90fd8fc23 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Mon, 3 Aug 2026 16:32:52 +0800 Subject: [PATCH 1/3] fix(spp_drims): only let a dispatch ship what its request approved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dispatch is generated from an approved request, but its Operations tab stayed editable in Ready state, so two things could be smuggled past the approval workflow. Both were reproduced on a dev instance: each validated successfully with no warning. "Add a Product" attached a move no request line asked for, and 50 units of a never-requested item shipped as done. Separately, using the padlock to unlock the picking made Demand editable again, so an approved line went from 100 to 150 and shipped 150 against a request for 100 with 100 allocated. The request line was left reading dispatched=150 against requested=100, and the unapproved item's unit cost was booked into the incident's drims_distributed_value as distributed relief value. Add _check_drims_dispatch_matches_request, called at the top of button_validate: every live move has to trace back to a line of this request, and nothing may ship beyond that line's allocated quantity, counting what earlier dispatches already shipped for it. Allocation is itself capped at the requested quantity by _allocate_stock_fifo, so comparing against quantity_allocated transitively enforces the approved amount rather than introducing a second notion of it. The check is keyed on drims_request_line_id, not Odoo's `additional` flag. `additional` is only set when a line is added through the form, so a move created over RPC or by an import leaves it False and would slip past a check based on it; there is a test asserting that precondition. On the form, lock the line-up through the Operations field's `options`: no "Add a Product" row and no row delete for a request dispatch. These have to be `options` entries rather than bare create/delete attributes on the field — the x2many passes its options dict through as crudOptions and evaluates each entry as a domain against the parent record, whereas bare attributes on a field tag are ignored. Quantity is deliberately left editable, since entering less than Demand is how a partial dispatch and its backorder are produced; making the list readonly wholesale would break that. OP#1057 --- spp_drims/models/stock_picking.py | 89 ++++++- spp_drims/tests/__init__.py | 1 + spp_drims/tests/test_dispatch_line_lock.py | 269 +++++++++++++++++++++ spp_drims/views/stock_picking_views.xml | 33 +++ 4 files changed, 390 insertions(+), 2 deletions(-) create mode 100644 spp_drims/tests/test_dispatch_line_lock.py diff --git a/spp_drims/models/stock_picking.py b/spp_drims/models/stock_picking.py index 703c4403f..bcaf68283 100644 --- a/spp_drims/models/stock_picking.py +++ b/spp_drims/models/stock_picking.py @@ -266,13 +266,98 @@ def action_view_drims_return(self): "res_id": self.drims_return_id.id, } + def _check_drims_dispatch_matches_request(self): + """Refuse to dispatch anything the request did not approve (OP#1057). + + A dispatch is generated from an approved request, but the Operations tab + stays editable in Ready state, so two things could still be smuggled past + the approval workflow: + + 1. **Extra products.** "Add a Product" attaches a move that no request + line asked for. Note this is keyed on ``drims_request_line_id`` rather + than Odoo's ``additional`` flag: ``additional`` is only set when a line + is added through the form, so a move created over RPC or by an import + has ``additional = False`` and would slip past a check based on it. + + 2. **Inflated quantities.** Unlocking the picking makes Demand editable + again, so an approved line can be raised above what was allocated. + Allocation is itself capped at the requested quantity by + ``_allocate_stock_fifo``, so comparing against ``quantity_allocated`` + transitively enforces the approved amount. + + Raises: + UserError: naming the offending products, if either check fails. + """ + for picking in self: + if picking.drims_type != "request_dispatch" or not picking.drims_request_id: + continue + + live_moves = picking.move_ids.filtered(lambda m: m.state != "cancel") + live_move_ids = set(live_moves.ids) + approved_line_ids = set(picking.drims_request_id.line_ids.ids) + + # 1. Every move has to trace back to a line of *this* request. + unapproved_products = sorted( + {m.product_id.display_name for m in live_moves if m.drims_request_line_id.id not in approved_line_ids} + ) + if unapproved_products: + raise UserError( + _( + "Dispatch %(picking)s contains items that are not part of " + "request %(request)s: %(products)s.\n\n" + "A dispatch may only ship what the request had approved and " + "allocated. Remove these lines, or raise a new request for " + "them and have it approved.", + picking=picking.name, + request=picking.drims_request_id.reference, + products=", ".join(unapproved_products), + ) + ) + + # 2. Nothing may ship beyond what the request line had allocated, + # counting what earlier dispatches already shipped for that line. + over_dispatched = [] + for line in live_moves.drims_request_line_id: + line_moves = self.env["stock.move"].search( + [ + ("drims_request_line_id", "=", line.id), + ("state", "!=", "cancel"), + ] + ) + already_shipped = sum(m.quantity for m in line_moves if m.state == "done") + about_to_ship = sum(m.quantity for m in line_moves if m.id in live_move_ids) + if line.uom_id.compare(already_shipped + about_to_ship, line.quantity_allocated) > 0: + over_dispatched.append( + _( + "%(product)s: dispatching %(total)s but only %(allocated)s is allocated", + product=line.product_id.display_name, + total=already_shipped + about_to_ship, + allocated=line.quantity_allocated, + ) + ) + if over_dispatched: + raise UserError( + _( + "Dispatch %(picking)s would ship more than request " + "%(request)s allocated:\n\n%(details)s\n\n" + "Reduce the quantities, or allocate more stock to the " + "request first.", + picking=picking.name, + request=picking.drims_request_id.reference, + details="\n".join(over_dispatched), + ) + ) + def button_validate(self): """Override button_validate to enforce beneficiary validation and invalidate cache. When a request_dispatch picking is validated, this: - 1. Validates that beneficiary tracking fields are filled (beneficiary_count, beneficiary_area_id) - 2. Invalidates the cached KPI values to ensure dashboard shows current data + 1. Refuses items or quantities the request never approved (OP#1057) + 2. Validates that beneficiary tracking fields are filled (beneficiary_count, beneficiary_area_id) + 3. Invalidates the cached KPI values to ensure dashboard shows current data """ + self._check_drims_dispatch_matches_request() + # Validate beneficiary tracking for DRIMS dispatches for picking in self: if picking.drims_type == "request_dispatch": diff --git a/spp_drims/tests/__init__.py b/spp_drims/tests/__init__.py index bb8d0dc9a..36520de6d 100644 --- a/spp_drims/tests/__init__.py +++ b/spp_drims/tests/__init__.py @@ -5,6 +5,7 @@ from . import test_allocation_preview_wizard from . import test_approval from . import test_coordination +from . import test_dispatch_line_lock from . import test_donation from . import test_incident from . import test_personnel diff --git a/spp_drims/tests/test_dispatch_line_lock.py b/spp_drims/tests/test_dispatch_line_lock.py new file mode 100644 index 000000000..553114999 --- /dev/null +++ b/spp_drims/tests/test_dispatch_line_lock.py @@ -0,0 +1,269 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +from datetime import date, timedelta + +from odoo.exceptions import UserError +from odoo.tests import tagged + +from .common import DrimsTestCommon + + +@tagged("post_install", "-at_install") +class TestDrimsDispatchLineLock(DrimsTestCommon): + """OP#1057: a dispatch may only ship what its request approved. + + The Operations tab stays editable while a dispatch is Ready, so products + could be added and quantities inflated past the approval workflow. These + tests cover the model-side guard, which is what protects RPC and imports — + the view attributes only stop the UI inviting the mistake. + """ + + def setUp(self): + super().setUp() + self.future_date = date.today() + timedelta(days=30) + self.rogue_product = self.env["product.product"].create( + { + "name": "Unapproved Item", + "type": "consu", + "is_storable": True, + "standard_price": 999.0, + } + ) + + # ------------------------------------------------------------------ + # helpers + # ------------------------------------------------------------------ + + def _stock_up(self, product, quantity): + self.env["stock.quant"].create( + { + "product_id": product.id, + "location_id": self.warehouse.lot_stock_id.id, + "quantity": quantity, + } + ) + + def _dispatch_for(self, requested=100, allocated=100): + """An allocated request plus its confirmed dispatch, ready to validate.""" + request = self.env["spp.drims.request"].create( + { + "incident_id": self.incident.id, + "destination_area_id": self.area.id, + "date_needed": self.future_date, + "source_warehouse_id": self.warehouse.id, + "line_ids": [ + ( + 0, + 0, + { + "product_id": self.product.id, + "quantity_requested": requested, + "uom_id": self.product.uom_id.id, + }, + ) + ], + } + ) + request.action_submit() + request.action_approve() + request.line_ids[0].quantity_allocated = allocated + request.state_id = self.env["spp.vocabulary.code"].search( + [ + ("vocabulary_id.namespace_uri", "=", "urn:openspp:vocab:drims:request-states"), + ("code", "=", "allocated"), + ], + limit=1, + ) + request.action_create_dispatch() + picking = request.picking_ids + picking.write({"beneficiary_count": 500, "beneficiary_area_id": self.area.id}) + return request, picking + + def _pick_everything(self, picking): + for move in picking.move_ids: + move.quantity = move.product_uom_qty + picking.move_ids.picked = True + + # ------------------------------------------------------------------ + # extra products + # ------------------------------------------------------------------ + + def test_added_product_blocks_validation(self): + """A product the request never asked for must stop the dispatch.""" + self._stock_up(self.product, 100) + self._stock_up(self.rogue_product, 100) + _request, picking = self._dispatch_for() + + self.env["stock.move"].create( + { + "picking_id": picking.id, + "product_id": self.rogue_product.id, + "product_uom_qty": 50.0, + "location_id": picking.location_id.id, + "location_dest_id": picking.location_dest_id.id, + } + ) + self._pick_everything(picking) + + with self.assertRaises(UserError) as cm: + picking.button_validate() + self.assertIn("Unapproved Item", str(cm.exception)) + self.assertNotEqual(picking.state, "done") + + def test_guard_is_not_keyed_on_the_additional_flag(self): + """The check must catch moves Odoo never flagged as ``additional``. + + ``additional`` is only set when a line is added through the form, so a + move created over RPC or by an import leaves it False. Keying the guard on + it would leave exactly that hole. + """ + self._stock_up(self.product, 100) + self._stock_up(self.rogue_product, 100) + _request, picking = self._dispatch_for() + + rogue_move = self.env["stock.move"].create( + { + "picking_id": picking.id, + "product_id": self.rogue_product.id, + "product_uom_qty": 50.0, + "location_id": picking.location_id.id, + "location_dest_id": picking.location_dest_id.id, + } + ) + self.assertFalse(rogue_move.additional, "precondition: RPC-created moves are not 'additional'") + self._pick_everything(picking) + + with self.assertRaises(UserError): + picking.button_validate() + + def test_move_from_another_request_blocks_validation(self): + """A move linked to a different request's line is still not approved here. + + The message names the dispatch's own request — the one whose approval was + bypassed — rather than the request the line was borrowed from. + """ + self._stock_up(self.product, 300) + request_a, picking_a = self._dispatch_for() + _request_b, picking_b = self._dispatch_for() + + # Smuggle B's move onto A's dispatch. + picking_b.move_ids[0].picking_id = picking_a.id + self._pick_everything(picking_a) + + with self.assertRaises(UserError) as cm: + picking_a.button_validate() + message = str(cm.exception) + self.assertIn(request_a.reference, message) + self.assertIn(self.product.display_name, message) + self.assertNotEqual(picking_a.state, "done") + + # ------------------------------------------------------------------ + # inflated quantities + # ------------------------------------------------------------------ + + def test_demand_raised_above_allocation_blocks_validation(self): + """Unlocking and raising Demand must not ship more than was allocated.""" + self._stock_up(self.product, 300) + _request, picking = self._dispatch_for(requested=100, allocated=100) + + picking.is_locked = False + move = picking.move_ids[0] + self.assertTrue(move.is_initial_demand_editable, "precondition: unlocking frees Demand") + move.product_uom_qty = 150.0 + self._pick_everything(picking) + + with self.assertRaises(UserError) as cm: + picking.button_validate() + self.assertIn("more than", str(cm.exception)) + self.assertNotEqual(picking.state, "done") + + def test_picked_quantity_above_allocation_blocks_validation(self): + """Over-picking beyond the allocation is refused even with Demand intact.""" + self._stock_up(self.product, 300) + _request, picking = self._dispatch_for(requested=100, allocated=100) + + move = picking.move_ids[0] + move.quantity = 130.0 + move.picked = True + + with self.assertRaises(UserError): + picking.button_validate() + + # ------------------------------------------------------------------ + # legitimate flows must be untouched + # ------------------------------------------------------------------ + + def test_full_dispatch_validates(self): + """The ordinary case still works.""" + self._stock_up(self.product, 100) + _request, picking = self._dispatch_for() + self._pick_everything(picking) + + picking.button_validate() + + self.assertEqual(picking.state, "done") + + def test_partial_dispatch_validates(self): + """Shipping less than Demand must stay allowed. + + This is how a partial dispatch and its backorder are produced (OP#1087), + so the guard must not treat under-picking as a violation. + """ + self._stock_up(self.product, 100) + _request, picking = self._dispatch_for() + move = picking.move_ids[0] + move.quantity = 90.0 + move.picked = True + + # No UserError from the DRIMS guard; Odoo asks about the backorder. + action = picking.button_validate() + self.assertIsInstance(action, dict) + self.assertEqual(action["res_model"], "stock.backorder.confirmation") + + def test_second_dispatch_after_top_up_validates(self): + """The cumulative check must not false-positive across dispatches.""" + self._stock_up(self.product, 5000) + request, first = self._dispatch_for(requested=5000, allocated=2000) + self._pick_everything(first) + first.button_validate() + self.assertEqual(first.state, "done") + + # Allocate the rest and dispatch again. + request.line_ids[0].quantity_allocated = 5000 + request.action_create_dispatch() + second = request.picking_ids - first + second.write({"beneficiary_count": 200, "beneficiary_area_id": self.area.id}) + self._pick_everything(second) + + second.button_validate() + + self.assertEqual(second.state, "done") + self.assertEqual(request.line_ids[0].quantity_dispatched, 5000) + + def test_non_drims_picking_is_unaffected(self): + """The guard must only apply to DRIMS request dispatches.""" + self._stock_up(self.rogue_product, 50) + picking = self.env["stock.picking"].create( + { + "picking_type_id": self.warehouse.out_type_id.id, + "location_id": self.warehouse.lot_stock_id.id, + "location_dest_id": self.env.ref("stock.stock_location_customers").id, + "move_ids": [ + ( + 0, + 0, + { + "product_id": self.rogue_product.id, + "product_uom_qty": 10.0, + "location_id": self.warehouse.lot_stock_id.id, + "location_dest_id": self.env.ref("stock.stock_location_customers").id, + }, + ) + ], + } + ) + picking.action_confirm() + self._pick_everything(picking) + + picking.button_validate() + + self.assertEqual(picking.state, "done") diff --git a/spp_drims/views/stock_picking_views.xml b/spp_drims/views/stock_picking_views.xml index 8a0ea785a..3bb1d82b9 100644 --- a/spp_drims/views/stock_picking_views.xml +++ b/spp_drims/views/stock_picking_views.xml @@ -117,6 +117,39 @@ state != 'done' or drims_type_id + + + {'create': [('drims_type', '!=', 'request_dispatch')], 'delete': [('drims_type', '!=', 'request_dispatch')]} + + From 7e7c34124fd9e765042f3799237042d98550da23 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Tue, 11 Aug 2026 10:53:17 +0800 Subject: [PATCH 2/3] 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. --- spp_drims/tests/test_dispatch_line_lock.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/spp_drims/tests/test_dispatch_line_lock.py b/spp_drims/tests/test_dispatch_line_lock.py index 553114999..8fd3b6d3c 100644 --- a/spp_drims/tests/test_dispatch_line_lock.py +++ b/spp_drims/tests/test_dispatch_line_lock.py @@ -42,6 +42,21 @@ def _stock_up(self, product, quantity): } ) + def _allocate(self, line, quantity, warehouse=None): + """Record a per-warehouse allocation for ``line``. + + OP#1079 replaced the writable ``quantity_allocated`` on the request line + with a stored compute over ``allocation_ids``, so allocation has to be + expressed as a row against a warehouse. + """ + return self.env["spp.drims.request.allocation"].create( + { + "request_line_id": line.id, + "warehouse_id": (warehouse or self.warehouse).id, + "quantity_allocated": quantity, + } + ) + def _dispatch_for(self, requested=100, allocated=100): """An allocated request plus its confirmed dispatch, ready to validate.""" request = self.env["spp.drims.request"].create( @@ -49,7 +64,6 @@ def _dispatch_for(self, requested=100, allocated=100): "incident_id": self.incident.id, "destination_area_id": self.area.id, "date_needed": self.future_date, - "source_warehouse_id": self.warehouse.id, "line_ids": [ ( 0, @@ -65,7 +79,7 @@ def _dispatch_for(self, requested=100, allocated=100): ) request.action_submit() request.action_approve() - request.line_ids[0].quantity_allocated = allocated + self._allocate(request.line_ids[0], allocated) request.state_id = self.env["spp.vocabulary.code"].search( [ ("vocabulary_id.namespace_uri", "=", "urn:openspp:vocab:drims:request-states"), @@ -228,7 +242,7 @@ def test_second_dispatch_after_top_up_validates(self): self.assertEqual(first.state, "done") # Allocate the rest and dispatch again. - request.line_ids[0].quantity_allocated = 5000 + request.line_ids[0].allocation_ids[0].quantity_allocated = 5000 request.action_create_dispatch() second = request.picking_ids - first second.write({"beneficiary_count": 200, "beneficiary_area_id": self.area.id}) From 149168504d13e57db01a65752af0e03206b391cb Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Wed, 19 Aug 2026 11:55:35 +0800 Subject: [PATCH 3/3] =?UTF-8?q?fix(spp=5Fdrims):=20address=20the=20dispatc?= =?UTF-8?q?h=20line-lock=20review=20=E2=80=94=20version=20bump=20and=20an?= =?UTF-8?q?=20arch=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Version bump to 19.0.3.0.2 with its changelog entry, following the convention Edwin settled: bumps and HISTORY entries land in the PR, with the concrete number reconciled at merge time in queue order. An arch test now pins the options dict on the Operations move_ids field. The first attempt at this guard used bare create/delete attributes on the field tag, which Odoo ignores there — QA caught it, nothing failed. A regression would restore the Add a line and trash affordances just as silently, so the test reads the options off the combined arch and asserts both keys are denied and scoped to request dispatches. --- spp_drims/README.rst | 9 +++++++++ spp_drims/__manifest__.py | 2 +- spp_drims/readme/HISTORY.md | 4 ++++ spp_drims/static/description/index.html | 12 +++++++++++- spp_drims/tests/test_dispatch_line_lock.py | 21 +++++++++++++++++++++ 5 files changed, 46 insertions(+), 2 deletions(-) diff --git a/spp_drims/README.rst b/spp_drims/README.rst index 35393c233..fb3e9068a 100644 --- a/spp_drims/README.rst +++ b/spp_drims/README.rst @@ -179,6 +179,15 @@ Dependencies Changelog ========= +19.0.3.0.2 +~~~~~~~~~~ + +- fix(drims): only let a dispatch ship what its request approved. + Products cannot be added to a request dispatch and quantities cannot + be raised past what was allocated — enforced on the model, so imports + and API callers are covered too, with the Operations tab's Add a line + and delete affordances hidden to match (#1057) + 19.0.3.0.0 ~~~~~~~~~~ diff --git a/spp_drims/__manifest__.py b/spp_drims/__manifest__.py index e775182ab..6b55a42a4 100644 --- a/spp_drims/__manifest__.py +++ b/spp_drims/__manifest__.py @@ -5,7 +5,7 @@ "and distribution tracking. Links to hazard incidents with multi-tier " "approval workflows and warehouse operations.", "category": "OpenSPP/Inventory", - "version": "19.0.3.0.0", + "version": "19.0.3.0.2", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_drims/readme/HISTORY.md b/spp_drims/readme/HISTORY.md index 41a4b4ea2..cf42ae8b9 100644 --- a/spp_drims/readme/HISTORY.md +++ b/spp_drims/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.3.0.2 + +- fix(drims): only let a dispatch ship what its request approved. Products cannot be added to a request dispatch and quantities cannot be raised past what was allocated — enforced on the model, so imports and API callers are covered too, with the Operations tab's Add a line and delete affordances hidden to match (#1057) + ### 19.0.3.0.0 - feat(drims): allocate stock per source warehouse. The Allocate Stock wizard now auto-splits each requested line across the DRIMS warehouses that hold stock (e.g. 70 → 50 @ WH1 + 20 @ WH2) with editable rows; the split is captured on a new per-warehouse allocation record, shown on the request's Allocations tab and summarised in a "Source Warehouse(s)" column on the Requests list; dispatch creates one picking per source warehouse. The single "Source Warehouse" field on the request has been removed — the warehouse(s) are chosen in the wizard. The allocation wizard distinguishes no-stock, stock-shortfall and deliberate partial-allocation cases with clear messages, and the request line's Fulfillment % tracks allocated ÷ requested so the bar reflects allocation progress (#1079) diff --git a/spp_drims/static/description/index.html b/spp_drims/static/description/index.html index f64fb3c62..e680bac3f 100644 --- a/spp_drims/static/description/index.html +++ b/spp_drims/static/description/index.html @@ -565,6 +565,16 @@

Changelog

+

19.0.3.0.2

+
    +
  • fix(drims): only let a dispatch ship what its request approved. +Products cannot be added to a request dispatch and quantities cannot +be raised past what was allocated — enforced on the model, so imports +and API callers are covered too, with the Operations tab’s Add a line +and delete affordances hidden to match (#1057)
    • +
+
+

19.0.3.0.0

  • feat(drims): allocate stock per source warehouse. The Allocate Stock @@ -584,7 +594,7 @@

    19.0.3.0.0

    destination-type selector (#1075)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_drims/tests/test_dispatch_line_lock.py b/spp_drims/tests/test_dispatch_line_lock.py index 8fd3b6d3c..efbcee3a0 100644 --- a/spp_drims/tests/test_dispatch_line_lock.py +++ b/spp_drims/tests/test_dispatch_line_lock.py @@ -1,6 +1,8 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from datetime import date, timedelta +from lxml import etree + from odoo.exceptions import UserError from odoo.tests import tagged @@ -281,3 +283,22 @@ def test_non_drims_picking_is_unaffected(self): picking.button_validate() self.assertEqual(picking.state, "done") + + def test_the_operations_field_denies_create_and_delete_on_a_dispatch(self): + """Pin the options dict, because the first attempt at this was silent. + + Bare create/delete attributes on the field tag are ignored — QA caught + that — so the guard rides on `options`, whose entries are domains + evaluated against the picking. A regression here would restore the Add + a line and trash affordances with nothing failing to say so + (OP#1057 review). + """ + view = self.env.ref("stock.view_picking_form") + arch = etree.fromstring(self.env["stock.picking"].get_view(view.id, "form")["arch"]) + moves = arch.xpath("//page[@name='operations']/field[@name='move_ids']") + + self.assertTrue(moves, "the Operations page should still carry move_ids") + options = moves[0].get("options") or "" + for action in ("create", "delete"): + self.assertIn(f"'{action}'", options, f"{action} must be denied through options") + self.assertIn("request_dispatch", options, "the denial is scoped to DRIMS dispatches")