fix(spp_studio): remove or rewrite pack items with non-evaluable CEL expressions - #444
fix(spp_studio): remove or rewrite pack items with non-evaluable CEL expressions#444gonzalesedwin1123 wants to merge 3 commits into
Conversation
…expressions 24 of the 106 shipped spp.studio.pack.item records carried filter expressions referencing registrant fields or studio variables that exist in no module, so the logic they install could never translate or evaluate, and they fail spp_cel_load_testing's strict studio-validation suite (gating #432). Rewrite the 4 items where a meaningful sub-expression survives (OVC Child Age Eligibility, OVC Vulnerable Household Check, GMI Residency Requirement, Public Works Poverty Status Check); remove the other 20, which have no working near-equivalent. Every kept expression was verified through the resolver+translator path the validation test uses. Pack data is noupdate, so a migration applies the same removals and rewrites to existing databases. Bump to 19.0.2.0.2. Fixes #431
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #444 +/- ##
==========================================
+ Coverage 74.76% 74.81% +0.05%
==========================================
Files 509 565 +56
Lines 35008 39497 +4489
==========================================
+ Hits 26172 29551 +3379
- Misses 8836 9946 +1110
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…variable Cross-module sweep found that core spp_registry already defines the in_institutional_care field the item filters on - it only lacked a variable definition. Add the in_institutional_care standard variable over that field and keep the shipped expression, instead of removing the item. standard_variables.xml is not noupdate, so existing databases receive the variable through the regular upgrade data load; removals in the 19.0.2.0.2 migration drop from 20 to 19.
emjay0921
left a comment
There was a problem hiding this comment.
One change requested. The mechanics of this PR are clean and I verified them item by item — notes in a separate comment, along with a root-cause finding that is bigger than this PR.
The GMI rewrite is the only fail-open change in the set, and it reads like the others
Three of the four rewrites drop OR-branches, which makes the predicate stricter — fewer registrants qualify. That is the safe direction for a shipped eligibility template. The GMI one drops an AND-conjunct:
is_legal_resident && residency_months >= 12 → residency_months >= 12
That makes the rule more permissive: it now passes registrants the policy author wrote it to exclude. The new description — "Minimum residency duration requirement (12 months)." — drops any mention of legal residency, so nothing downstream records that the requirement was there and was removed. The PR body lists it in the same table as the three narrowing rewrites, and the note that "descriptions updated where the semantics narrowed" does not cover this case.
I checked whether the conjunct could be rescued the way Institutional Residence was: it cannot. There is no is_legal_resident field on any model (ir_model_fields on a dev DB, and a repo-wide field index), so the two consistent options are the same ones applied to the other 19 items:
- Remove the item, like the 19 whose load-bearing terms have nothing behind them. This is the option I would take — a template that silently grants eligibility is worse than no template.
- Keep item and expression as shipped, accepting that it does not evaluate until the field exists, and track it under the variable-catalogue issue (#446).
If there is a reason to prefer the narrowed-but-permissive form, that is a policy call rather than a data-hygiene one, and it should be stated explicitly in the PR body and the HISTORY entry so it is not read as an equivalent rewrite.
Everything else I found is non-blocking.
|
Non-blocking findings, plus the verification I did on the mechanics. The root cause survives this fix — filed as #446The 19 removed pack items were consumers of the defect, not the defect. The practical consequence is that Studio's variable picker still offers these to no-code users, so the first filter anyone builds on one reproduces #431 from the UI. Worth keeping in mind when #431 is closed: the pack items are fixed, the class is not. It also means the framing in the PR body is inverted for these six. It says the items reference "studio variables that exist in no module" and that "every load-bearing identifier is backed by nothing" — the variables exist, their fields do not. That is worth being precise about because it is the exact mirror of the Removed items leave their installed artifacts behind
The migration overwrites local edits without checking
MinorThe new Verified
One carry-overNeither this PR's CI nor #432's runs the assert that would actually validate this fix — |
Fixes #431.
Problem
24 of the 106
spp.studio.pack.itemrecords shipped inspp_studio/data/packs/carry filterexpressions that cannot work on any install: they reference registrant fields or studio variables
that exist in no module, so the logic they install can never translate or evaluate. They also fail
spp_cel_load_testing's strict studio-validation test(
test_all_pack_cel_expressions_translate), which gates the merge of #432 and would fail theweekly
ci-fullrun once that migration lands.Approach
Per-item rewrite where a real near-equivalent survives, remove the rest — no placeholder
variables were invented, and no test was weakened. Every candidate rewrite was validated
empirically through the exact resolve+translate path the test uses (odoo shell against an
installed DB), because translation stops at the first bad identifier — terms after it in the
original expressions had never actually been validated.
Fixed properly (1)
Exclusion Criteria · Institutional Residence Exclusion keeps its shipped expression
(
!in_institutional_care): the item only lacked a variable definition — corespp_registryalready defines the
in_institutional_carefield on individuals. This PR adds the missingin_institutional_carestandard variable over that field.standard_variables.xmlis notnoupdate, so existing databases get the variable through the regular upgrade data load.Rewritten (4)
age < child_age_limit || (age < 25 && is_in_education)age < child_age_limitis_elderly_headed || is_child_headed || is_skipped_generation || has_chronically_ill_headis_elderly_headed || is_child_headedis_legal_resident && residency_months >= 12residency_months >= 12per_capita_income <= poverty_threshold || is_food_insecureper_capita_income <= poverty_thresholdDescriptions updated where the semantics narrowed.
Removed (19)
No meaningful sub-expression survives — every load-bearing identifier is backed by nothing:
!has_mother && !has_fatherfallback expands tomembers.*, which doesn't exist in individual context — no rescue)is_single_parentalso fails validation; the single surviving term isn't a near-equivalent)Housing Quality, Pension Receipt, Duplicate Program, Income Tax Payer, Bank Balance,
Livestock Ownership (10 items; the pack keeps Land Ownership and Institutional Residence)
Migration
Pack data is
noupdate="1", so existing databases keep the broken records after the XML change.migrations/19.0.2.0.2/post-migration.pyapplies the same 19 removals and 4 rewrites directly(the restored Institutional Residence item needs no migration handling — only its variable, which
arrives via the regular data load).
Version
19.0.2.0.1 → 19.0.2.0.2+ HISTORY fragment.Verification
All local (Docker), with
spp_cel_load_testingtaken from PR #432 @419752d6since the strict test lives there:test_all_pack_cel_expressions_translatefails with exactly the 24 items from spp_studio: shipped logic packs contain filter expressions referencing nonexistent fields/variables (24 items) #431 (14 translation errors + 10 unresolved-variable items).0 failed, 0 error(s) of 6 tests;Translated 30/25 CEL expressions (0 errors, 0 with unresolved variables).origin/19.0), then upgraded with this branch: migration logsremoved 19 and rewrote 4 shipped pack items, item count 106 → 87, thein_institutional_carevariable is created by the data load, rewritten/restored expressions verified in the DB, and the strict suite passes on the upgraded DB (0 failed of 6;Translated 32/26, 0 errors, 0 unresolved).Passed: 225 | Failed: 0 | Errors: 0.Out of scope / follow-ups
is_orphan,owns_car,land_hectares,residency_years,is_single_parent, …). They are the samedefect one level down but are not exercised by the strict test unless a pack item references
them; needs its own issue.
Disability Status Verification maps cleanly onto
spp_disability_registry(
has_disability,disability_severity_id, assessments), and livestock/land exclusions ontospp_farmer_registry/spp_land_record. Since pack items are plain data records, those modulescan ship their own items into the shipped packs, defined against their own fields — the right
long-term home for domain templates. Needs its own issue.
comparisons through unvalidated (
r.land_hectares <= 2translates even though the field existsnowhere) — latent quirk worth a look.
required_modulessupport for "template" packs (issue spp_studio: shipped logic packs contain filter expressions referencing nonexistent fields/variables (24 items) #431 Option 3) if aspirational packs arewanted as a product feature.
Reviewer caveat
Neither this PR's CI nor #432's exercises the strict test (per-module jobs don't co-install
spp_studioandspp_cel_load_testing). Only a manual ci-full run after both merge does.The local runs documented above are the substitute evidence.