From 5f46319d1d4eb0268904d9f83bc0d0dc1efeda90 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Wed, 5 Aug 2026 12:02:01 +0800 Subject: [PATCH 1/2] fix(spp_drims): correct constants that named codes the vocabulary lacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four constants in constants.py held values present in no shipped vocabulary: PRIORITY_LOW/MEDIUM/HIGH ("low"/"medium"/"high", where the data has routine/urgent/critical) and DRIMS_TYPE_TRANSFER ("transfer", where the code is internal_transfer). Nothing failed, because a search for a non-existent code returns an empty set rather than raising, so any comparison against one would simply never match. All four were unused, which is why it had gone unnoticed; the file still read as the reference for valid codes. Rather than remap them to the nearest concept — "urgent" is not a medium priority — each code constant is now named after the code it holds, so the name cannot describe something the data does not have. That gives PRIORITY_ROUTINE/URGENT/CRITICAL and DRIMS_TYPE_INTERNAL_TRANSFER. Also completes the file, since a partly declared group is what sends a caller back to hardcoding the string: adds the seven missing codes (cancelled, fulfilled, critical_shortage, quality_issue, routine, urgent, internal_transfer) and VOCAB_* constants for the ten vocabularies that had none, pod-statuses among them. The durable part is the new test. CODE_NAMESPACES records which vocabulary each group of code constants draws from, and test_constants.py walks it to assert every constant resolves to a real spp.vocabulary.code, that every namespace exists and builds on VOCAB_BASE, and that each name mirrors its value. Each assertion was checked by reintroducing the defect it guards against and confirming it fails. Completeness is asserted only over canonical codes. Covering every code broke as soon as spp_drims_sl was installed, since it legitimately layers life_threatening onto priority-levels with is_local set; spp_drims cannot owe a constant for what a country module adds. A non-local addition still fails, which is intended. OP#1165 --- spp_drims/models/constants.py | 59 ++++++++-- spp_drims/tests/__init__.py | 1 + spp_drims/tests/test_constants.py | 176 ++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 12 deletions(-) create mode 100644 spp_drims/tests/test_constants.py diff --git a/spp_drims/models/constants.py b/spp_drims/models/constants.py index c9033e598..8d6237405 100644 --- a/spp_drims/models/constants.py +++ b/spp_drims/models/constants.py @@ -2,27 +2,48 @@ """ DRIMS Constants -This module defines vocabulary namespace URIs and other constants used across -the DRIMS module. Using constants ensures consistency and makes URIs easier -to maintain. +Vocabulary namespace URIs and the code values within them, used across the DRIMS +module so that neither has to be written as a bare string. + +**Every code constant is named after the code it holds.** Four constants used to +be named for a concept that had no matching code in the shipped vocabulary data +(``PRIORITY_LOW``/``MEDIUM``/``HIGH`` and ``DRIMS_TYPE_TRANSFER``), so anything +comparing against them silently never matched — a ``search`` for a non-existent +code returns an empty set rather than raising. Mirroring the code in the name +removes the guesswork (OP#1165). + +``CODE_NAMESPACES`` at the bottom records which vocabulary each group of code +constants belongs to. ``spp_drims/tests/test_constants.py`` walks it and asserts +every constant resolves to a real ``spp.vocabulary.code``, so a typo or a renamed +code fails the build instead of quietly never matching. Add new groups there. """ # Vocabulary Namespace Base URI VOCAB_BASE = "urn:openspp:vocab:drims" # Vocabulary Namespace URIs +VOCAB_AGENCY_TYPES = f"{VOCAB_BASE}:agency-types" VOCAB_ALERT_TYPES = f"{VOCAB_BASE}:alert-types" -VOCAB_DONOR_TYPES = f"{VOCAB_BASE}:donor-types" +VOCAB_COORDINATION_MODES = f"{VOCAB_BASE}:coordination-modes" +VOCAB_DISTRIBUTION_TYPES = f"{VOCAB_BASE}:distribution-types" VOCAB_DONATION_STATES = f"{VOCAB_BASE}:donation-states" +VOCAB_DONOR_TYPES = f"{VOCAB_BASE}:donor-types" VOCAB_DRIMS_TYPES = f"{VOCAB_BASE}:drims-types" +VOCAB_HAZARD_TYPES = f"{VOCAB_BASE}:hazard-types" +VOCAB_ITEM_CATEGORIES = f"{VOCAB_BASE}:item-categories" +VOCAB_ITEM_CONDITIONS = f"{VOCAB_BASE}:item-conditions" +VOCAB_ITEM_DISPOSITIONS = f"{VOCAB_BASE}:item-dispositions" +VOCAB_ORGANIZATION_ROLES = f"{VOCAB_BASE}:organization-roles" +VOCAB_PERSONNEL_ROLES = f"{VOCAB_BASE}:personnel-roles" +VOCAB_POD_STATUSES = f"{VOCAB_BASE}:pod-statuses" VOCAB_PRIORITY_LEVELS = f"{VOCAB_BASE}:priority-levels" VOCAB_REQUEST_STATES = f"{VOCAB_BASE}:request-states" VOCAB_RESTRICTIONS = f"{VOCAB_BASE}:restrictions" +VOCAB_RETURN_CONDITIONS = f"{VOCAB_BASE}:return-conditions" +VOCAB_RETURN_REASONS = f"{VOCAB_BASE}:return-reasons" VOCAB_TRANSPORT_MODES = f"{VOCAB_BASE}:transport-modes" -VOCAB_ITEM_CONDITIONS = f"{VOCAB_BASE}:item-conditions" -VOCAB_ITEM_DISPOSITIONS = f"{VOCAB_BASE}:item-dispositions" -# Common state codes +# Request state codes (spp.drims.request) STATE_DRAFT = "draft" STATE_SUBMITTED = "submitted" STATE_APPROVED = "approved" @@ -30,8 +51,10 @@ STATE_ALLOCATED = "allocated" STATE_DISPATCHED = "dispatched" STATE_DELIVERED = "delivered" +STATE_FULFILLED = "fulfilled" +STATE_CANCELLED = "cancelled" -# Donation state codes +# Donation state codes (spp.drims.donation) DONATION_STATE_ANNOUNCED = "announced" DONATION_STATE_RECEIVED = "received" DONATION_STATE_INSPECTED = "inspected" @@ -42,7 +65,7 @@ # DRIMS type codes (for stock.picking classification) DRIMS_TYPE_DONATION_RECEIPT = "donation_receipt" DRIMS_TYPE_REQUEST_DISPATCH = "request_dispatch" -DRIMS_TYPE_TRANSFER = "transfer" +DRIMS_TYPE_INTERNAL_TRANSFER = "internal_transfer" DRIMS_TYPE_RETURN = "return" # Alert type codes @@ -50,9 +73,21 @@ ALERT_EXPIRY = "expiry" ALERT_SLA_BREACH = "sla_breach" ALERT_SLA_WARNING = "sla_warning" +ALERT_CRITICAL_SHORTAGE = "critical_shortage" +ALERT_QUALITY_ISSUE = "quality_issue" # Priority level codes -PRIORITY_LOW = "low" -PRIORITY_MEDIUM = "medium" -PRIORITY_HIGH = "high" +PRIORITY_ROUTINE = "routine" +PRIORITY_URGENT = "urgent" PRIORITY_CRITICAL = "critical" + +#: Which vocabulary each group of code constants draws from, keyed by the name +#: prefix. Walked by ``test_constants.py`` to prove every constant resolves. +#: Keep this in step when adding a group, or the new group goes unchecked. +CODE_NAMESPACES = { + "STATE_": VOCAB_REQUEST_STATES, + "DONATION_STATE_": VOCAB_DONATION_STATES, + "DRIMS_TYPE_": VOCAB_DRIMS_TYPES, + "ALERT_": VOCAB_ALERT_TYPES, + "PRIORITY_": VOCAB_PRIORITY_LEVELS, +} diff --git a/spp_drims/tests/__init__.py b/spp_drims/tests/__init__.py index bb8d0dc9a..3052e8cca 100644 --- a/spp_drims/tests/__init__.py +++ b/spp_drims/tests/__init__.py @@ -1,6 +1,7 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import common from . import test_activity_feed +from . import test_constants from . import test_alert from . import test_allocation_preview_wizard from . import test_approval diff --git a/spp_drims/tests/test_constants.py b/spp_drims/tests/test_constants.py new file mode 100644 index 000000000..61d75ffa2 --- /dev/null +++ b/spp_drims/tests/test_constants.py @@ -0,0 +1,176 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""OP#1165: prove every constant in ``constants.py`` resolves to real data. + +Four constants used to hold values that existed in no vocabulary +(``PRIORITY_LOW``/``MEDIUM``/``HIGH``, ``DRIMS_TYPE_TRANSFER``). Nothing failed, +because a ``search`` for a non-existent code returns an empty set rather than +raising — so a comparison against one would simply never match. These tests turn +that silent drift into a build failure. +""" + +from odoo.tests import tagged + +from ..models import constants +from .common import DrimsTestCommon + + +@tagged("post_install", "-at_install") +class TestDrimsConstants(DrimsTestCommon): + """Every namespace and code constant must exist in the shipped data.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.vocabulary = cls.env["spp.vocabulary"] + + # ------------------------------------------------------------------ + # helpers + # ------------------------------------------------------------------ + + @staticmethod + def _constants_with_prefix(prefix): + """Every public ``prefix``-named string constant, as {name: value}.""" + return { + name: value + for name, value in vars(constants).items() + if name.startswith(prefix) and isinstance(value, str) and not name.startswith("_") + } + + def _codes_in(self, namespace_uri): + """Every code in the vocabulary, including locally added ones.""" + return set( + self.env["spp.vocabulary.code"].search([("vocabulary_id.namespace_uri", "=", namespace_uri)]).mapped("code") + ) + + def _canonical_codes_in(self, namespace_uri): + """Only the codes this vocabulary ships with, excluding local overlays. + + ``is_local`` marks codes layered on by a country module or an admin — + ``spp_drims_sl``, for instance, adds ``life_threatening`` to + priority-levels. Extending a vocabulary that way is the intended design, + so those codes must not be held against ``spp_drims``'s own constants. + """ + return set( + self.env["spp.vocabulary.code"] + .search( + [ + ("vocabulary_id.namespace_uri", "=", namespace_uri), + ("is_local", "=", False), + ] + ) + .mapped("code") + ) + + # ------------------------------------------------------------------ + # namespaces + # ------------------------------------------------------------------ + + def test_every_vocab_namespace_exists(self): + """Each VOCAB_* constant must name a vocabulary that is actually installed.""" + namespaces = self._constants_with_prefix("VOCAB_") + namespaces.pop("VOCAB_BASE", None) + self.assertTrue(namespaces, "no VOCAB_* constants found — has the module moved?") + + missing = { + name: uri + for name, uri in namespaces.items() + if not self.vocabulary.search_count([("namespace_uri", "=", uri)]) + } + self.assertFalse( + missing, + "VOCAB_* constants naming vocabularies that do not exist: " + + ", ".join(f"{n} = {u!r}" for n, u in sorted(missing.items())), + ) + + def test_vocab_namespaces_use_the_base_uri(self): + """A namespace typed out in full would drift from VOCAB_BASE unnoticed.""" + namespaces = self._constants_with_prefix("VOCAB_") + namespaces.pop("VOCAB_BASE", None) + for name, uri in sorted(namespaces.items()): + self.assertTrue( + uri.startswith(f"{constants.VOCAB_BASE}:"), + f"{name} = {uri!r} does not build on VOCAB_BASE", + ) + + # ------------------------------------------------------------------ + # codes + # ------------------------------------------------------------------ + + def test_every_code_constant_resolves(self): + """The regression this ticket is about. + + Reported all at once rather than failing on the first, so a rename in the + vocabulary data shows the full list of constants to update. + """ + self.assertTrue(constants.CODE_NAMESPACES, "CODE_NAMESPACES is empty") + + failures = [] + for prefix, namespace_uri in sorted(constants.CODE_NAMESPACES.items()): + available = self._codes_in(namespace_uri) + self.assertTrue( + available, + f"vocabulary {namespace_uri} has no codes — cannot verify {prefix}* constants", + ) + for name, value in sorted(self._constants_with_prefix(prefix).items()): + if value not in available: + failures.append( + f"{name} = {value!r} is not a code in {namespace_uri} " + f"(available: {', '.join(sorted(available))})" + ) + self.assertFalse(failures, "constants that resolve to nothing:\n " + "\n ".join(failures)) + + def test_code_constants_are_named_after_their_code(self): + """Keeps the file honest: the name states the value. + + ``PRIORITY_HIGH = "high"`` looked right and was wrong, because the + vocabulary calls that level ``urgent``. Deriving the expected name from the + value stops a concept-name being invented for a code that does not exist. + """ + mismatches = [] + for prefix in sorted(constants.CODE_NAMESPACES): + for name, value in sorted(self._constants_with_prefix(prefix).items()): + expected = f"{prefix}{value.upper()}" + if name != expected: + mismatches.append(f"{name} = {value!r} — expected the name {expected}") + self.assertFalse( + mismatches, + "code constants whose name does not mirror their value:\n " + "\n ".join(mismatches), + ) + + def test_code_groups_cover_their_canonical_vocabulary(self): + """Every code the vocabulary ships with should have a constant. + + A partially declared group is what sent an earlier caller back to + hardcoding ``'urgent'``, which is how the bad values survived unnoticed. + + Deliberately limited to canonical codes. Asserting over *all* codes broke + as soon as ``spp_drims_sl`` was installed, because it legitimately adds + ``life_threatening`` to priority-levels — and ``spp_drims`` cannot be held + responsible for declaring constants for codes a country module layers on. + A non-local addition to one of these vocabularies would still fail here, + which is intended: that genuinely does leave these constants incomplete. + """ + gaps = [] + for prefix, namespace_uri in sorted(constants.CODE_NAMESPACES.items()): + declared = set(self._constants_with_prefix(prefix).values()) + for code in sorted(self._canonical_codes_in(namespace_uri) - declared): + gaps.append(f"{namespace_uri} code {code!r} has no {prefix}* constant") + self.assertFalse(gaps, "vocabulary codes with no constant:\n " + "\n ".join(gaps)) + + # ------------------------------------------------------------------ + # the specific values that were wrong + # ------------------------------------------------------------------ + + def test_the_previously_broken_constants_are_right_now(self): + """Pins the four values from OP#1165 against a silent re-introduction.""" + self.assertEqual(constants.PRIORITY_ROUTINE, "routine") + self.assertEqual(constants.PRIORITY_URGENT, "urgent") + self.assertEqual(constants.PRIORITY_CRITICAL, "critical") + self.assertEqual(constants.DRIMS_TYPE_INTERNAL_TRANSFER, "internal_transfer") + + # The old names described levels the vocabulary does not have. + for gone in ("PRIORITY_LOW", "PRIORITY_MEDIUM", "PRIORITY_HIGH", "DRIMS_TYPE_TRANSFER"): + self.assertFalse( + hasattr(constants, gone), + f"{gone} is back; it names a code that does not exist in the vocabulary", + ) From 71bd99d425b58142ba27f78bbb7c6a6df076fa9b Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Fri, 14 Aug 2026 12:15:27 +0800 Subject: [PATCH 2/2] fix(spp_drims): point priority consumers at codes the vocabulary has OP#1165 corrected constants.py but left the places that used the same wrong values as bare strings. The priority-levels vocabulary ships routine, urgent and critical; the views and the SLA lookup still named high, medium and low. - The request list decorated rows and the priority badge on 'high' and 'medium', so those highlights never appeared. - The search panel offered a "High Priority" filter whose domain matched no record, so it always returned nothing. - get_approval_sla_hours was keyed by 'critical', 'high', 'routine', 'low', and the shipped parameters matched. An urgent request found no entry and fell back to the routine default of 24 hours instead of 8 - a wrong answer rather than an error, which is the failure mode this ticket is about. spp.drims.alert.priority is a different field, a Selection of low/medium/high/critical, and its views are correct. Left alone. Three guards added: the SLA hours resolve per shipped priority code, and the list decorations and search filters only name codes that exist. The two view guards fail if the old values come back. --- spp_drims/data/config_defaults.xml | 9 +--- spp_drims/models/request.py | 2 +- spp_drims/models/res_config_settings.py | 9 +++- spp_drims/tests/test_constants.py | 63 +++++++++++++++++++++++++ spp_drims/views/request_views.xml | 12 ++--- 5 files changed, 79 insertions(+), 16 deletions(-) diff --git a/spp_drims/data/config_defaults.xml b/spp_drims/data/config_defaults.xml index f355d013e..dc4baab55 100644 --- a/spp_drims/data/config_defaults.xml +++ b/spp_drims/data/config_defaults.xml @@ -12,8 +12,8 @@ 4 - - drims.sla.hours.high + + drims.sla.hours.urgent 8 @@ -22,11 +22,6 @@ 24 - - drims.sla.hours.low - 48 - - drims.sla.warning_threshold_pct 75 diff --git a/spp_drims/models/request.py b/spp_drims/models/request.py index 05725f586..0d92fe217 100644 --- a/spp_drims/models/request.py +++ b/spp_drims/models/request.py @@ -310,7 +310,7 @@ def _compute_sla_status(self): SLA thresholds are configurable via Settings > DRIMS Configuration (requires spp_studio_drims module) or System Parameters: - - drims.sla.hours.critical/high/routine/low + - drims.sla.hours.critical/urgent/routine - drims.sla.warning_threshold_pct """ now = fields.Datetime.now() diff --git a/spp_drims/models/res_config_settings.py b/spp_drims/models/res_config_settings.py index ba1233e47..5aa2644d7 100644 --- a/spp_drims/models/res_config_settings.py +++ b/spp_drims/models/res_config_settings.py @@ -37,14 +37,19 @@ def get_approval_sla_hours(self, priority_code): """Get approval SLA hours for a given priority code. Args: - priority_code: One of 'critical', 'high', 'routine', 'low' + priority_code: a code from the DRIMS priority-levels vocabulary — + 'critical', 'urgent' or 'routine' Returns: int: Hours allowed to approve requests of this priority """ # nosemgrep: odoo-sudo-without-context — standard Odoo pattern for system parameter access ICP = self.env["ir.config_parameter"].sudo() - defaults = {"critical": 4, "high": 8, "routine": 24, "low": 48} + # Keyed by the codes the priority-levels vocabulary actually ships. + # This used to name 'high' and 'low', which no priority has, so an + # urgent request fell through to the routine default of 24 hours + # instead of 8 and nothing said so (OP#1165). + defaults = {"critical": 4, "urgent": 8, "routine": 24} param_key = f"drims.sla.hours.{priority_code}" return int(ICP.get_param(param_key, defaults.get(priority_code, 24))) diff --git a/spp_drims/tests/test_constants.py b/spp_drims/tests/test_constants.py index 61d75ffa2..81ba77a9a 100644 --- a/spp_drims/tests/test_constants.py +++ b/spp_drims/tests/test_constants.py @@ -8,6 +8,10 @@ that silent drift into a build failure. """ +import re + +from lxml import etree + from odoo.tests import tagged from ..models import constants @@ -174,3 +178,62 @@ def test_the_previously_broken_constants_are_right_now(self): hasattr(constants, gone), f"{gone} is back; it names a code that does not exist in the vocabulary", ) + + # ------------------------------------------------------------------ + # the same mistake, in the consumers rather than the constants + # ------------------------------------------------------------------ + + def test_sla_hours_are_keyed_by_real_priority_codes(self): + """Every shipped priority must get its own approval SLA. + + The lookup used to be keyed by 'high' and 'low', which no priority has, + so an urgent request silently fell through to the routine default of 24 + hours instead of 8 — a wrong answer rather than an error, which is the + whole failure mode OP#1165 is about. + """ + settings = self.env["res.config.settings"] + codes = self._codes_in(constants.VOCAB_PRIORITY_LEVELS) + + hours = {code: settings.get_approval_sla_hours(code) for code in codes} + self.assertEqual( + hours, + {"routine": 24, "urgent": 8, "critical": 4}, + f"a priority is not getting its own SLA: {hours}", + ) + + # Urgent must not silently share the routine fallback. + self.assertNotEqual( + hours["urgent"], + hours["routine"], + "urgent is falling through to the routine default", + ) + + def test_priority_views_reference_codes_that_exist(self): + """Decorations and filters keyed on a priority code must name a real one. + + A decoration or a search filter naming a code that does not exist is + accepted silently and simply never matches — the filter returns nothing + and the highlight never appears. + """ + codes = set(self._codes_in(constants.VOCAB_PRIORITY_LEVELS)) + arch = etree.fromstring(self.env.ref("spp_drims.spp_drims_request_list").arch) + referenced = set(re.findall(r"priority_id\.code\s*==\s*'([a-z_]+)'", etree.tostring(arch, encoding="unicode"))) + + self.assertTrue(referenced, "no priority decorations found — has the view changed?") + self.assertFalse( + referenced - codes, + f"these decorations name priority codes that do not exist: {sorted(referenced - codes)}", + ) + + def test_priority_search_filters_reference_codes_that_exist(self): + search = etree.fromstring(self.env.ref("spp_drims.spp_drims_request_search").arch) + codes = set(self._codes_in(constants.VOCAB_PRIORITY_LEVELS)) + referenced = set( + re.findall(r"'priority_id\.code',\s*'=',\s*'([a-z_]+)'", etree.tostring(search, encoding="unicode")) + ) + + self.assertTrue(referenced, "no priority filters found — has the search view changed?") + self.assertFalse( + referenced - codes, + f"these filters name priority codes that do not exist: {sorted(referenced - codes)}", + ) diff --git a/spp_drims/views/request_views.xml b/spp_drims/views/request_views.xml index 32b999080..16836130c 100644 --- a/spp_drims/views/request_views.xml +++ b/spp_drims/views/request_views.xml @@ -9,7 +9,7 @@