fix(spp_drims): name constants after codes the vocabulary actually ships - #438
Conversation
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
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #438 +/- ##
==========================================
- Coverage 74.76% 72.60% -2.17%
==========================================
Files 509 421 -88
Lines 35008 30221 -4787
==========================================
- Hits 26175 21943 -4232
+ Misses 8833 8278 -555
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
This goes well beyond a constants cleanup, and the extra distance is where the value is: three real user-facing defects were hiding behind the dead names — the urgent-request SLA silently answering 24 hours instead of 8, list decorations and priority badges that never fired, and a search filter that always returned nothing. "A wrong answer rather than an error" is exactly the failure mode worth spending a PR on.
What I verified:
- The design principle is right and made durable. Naming every constant after the code it holds removes the remap-to-nearest-concept trap, and
CODE_NAMESPACES+ the test walker means a typo or a renamed vocabulary code fails the build instead of silently never matching. The nine tests cover the resolver walk, the naming mirror, canonical coverage, the regression pin on the four previously-broken constants, SLA keying, and both view guards. - The config-parameter rename is safe on upgrade:
config_defaults.xmlis already<odoo noupdate="1">, so customized values are preserved by design; the old.high/.lowkeys were dead config nothing ever read (before this PR the code asked for.urgent, which didn't exist, and fell to the hardcoded default), so no data migration is needed; and the newdefault_sla_hours_urgentxml_id gets created on upgrade even inside the noupdate block. - Scoping is correct:
spp.drims.alert.priorityis a genuine Selection field with its own values and untouched views — leaving it alone was right. - One pleasing synergy:
STATE_DELIVEREDandSTATE_FULFILLEDboth now resolve against the shipped vocabulary — which is consistent with OP#1174's open question about what distinguishes them. The constants are ready for whichever answer that ticket lands on.
1. Blocker — version bump
Per the standing ruling: bump + HISTORY in the PR, number at merge-time rebase. It's functionally meaningful here, not just signalling — the view fixes (decorations, badge, filter) and the parameter rename are data, so without -u spp_drims users keep the broken badges and the dead filter this PR exists to fix. Joins the spp_drims merge-time renumbering queue.
With the bump, this is an approve.
2. Housekeeping, on our side
OP#1175 (filed 2026-08-19 from #391's reviewer note) turns out to duplicate OP#1165, which has existed since 2026-08-03 — that one's on the reviewer, not you. #1175 is being marked as a duplicate pointing here.
Adds the version bump the review asked for — 19.0.4.0.4 — with its changelog entry. The bump matters here rather than being bookkeeping: the view fixes and the parameter rename are data, so without an upgrade users keep the broken badge and the dead search filter this branch exists to fix. constants.py conflicted where #414 added NON_ACCEPT_DISPOSITIONS and DONATION_STATE_DRAFT at the same point this branch reworded the donation-states heading. Both kept.
|
Thanks — bump added and merged. Pushed as 1. Version bump — On the merge:
2. OP#1175 — noted, thanks for catching the duplicate and marking it. Nothing needed on this side. |
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?
Four constants in
spp_drims/models/constants.pyheld values that exist in no shipped vocabulary:PRIORITY_LOW/MEDIUM/HIGH("low"/"medium"/"high", where the data shipsroutine/urgent/critical) andDRIMS_TYPE_TRANSFER("transfer", where the code isinternal_transfer). Nothing failed, because a search for a non-existent code returns an empty set rather than raising, so any comparison against one simply never matched. All four were unused, which is why it went unnoticed — while the file still read as the reference for valid codes (OP#1165).The same wrong values were also hardcoded as bare strings in the places that mattered, which is where users actually felt it:
highandmedium, so those highlights never appeared.get_approval_sla_hourswas keyed bycritical,high,routine,low. 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.How was the change implemented?
PRIORITY_ROUTINE/URGENT/CRITICALandDRIMS_TYPE_INTERNAL_TRANSFER, so a name cannot describe something the data does not have.cancelled,fulfilled,critical_shortage,quality_issue,routine,urgent,internal_transfer) andVOCAB_*constants for the ten vocabularies that had none, pod-statuses among them.spp.drims.alert.priorityis a different field — a Selection oflow/medium/high/critical— and its views are correct. Left alone.New unit tests
spp_drims/tests/test_constants.py— the durable part.CODE_NAMESPACESrecords which vocabulary each group of code constants draws from, and the test walks it to assert every constant resolves to a realspp.vocabulary.code, so a constant naming a code the data lacks fails here rather than silently matching nothing.Unit tests executed by the author
Full
spp_drimssuite on this branch after merging19.0in: 272 tests, 0 failed, 0 errors.How to test manually
Related links