fix(spp_mis_demo_v2): make the SP-MIS demo back up what its programs claim - #475
Draft
emjay0921 wants to merge 9 commits into
Draft
fix(spp_mis_demo_v2): make the SP-MIS demo back up what its programs claim#475emjay0921 wants to merge 9 commits into
emjay0921 wants to merge 9 commits into
Conversation
var_has_disability resolved source_field is_person_with_disability, and var_has_disabled_member evaluated members.exists(m.is_person_with_disability). That name appears nowhere in the codebase; the field on res.partner is has_disability, related through current_disability_assessment_id. Any program filtering on either variable therefore matched nothing, and did so silently -- there is no error for resolving against a field that is not there, only an empty result. Found while tracing why the demo's Disability Support Grant previewed zero households. standard_variables.xml is not noupdate, so existing databases pick the correction up on upgrade without a migration.
…ms claim Four gaps under OP#958, each one a place where a program form promised something the data did not support. Enrollment vs eligibility (#956). Households were enrolled from a static per-blueprint flag while the form previewed the program's CEL, so a program could show 102 enrolled and 9 matching. Programs with a selective rule now enrol whoever that rule matches. Food Assistance and Emergency Relief Fund stay flag-driven and are documented as such: the first matches every active registrant, the second has no expression, and deciding what a programme targets is not demo-data cleanup. Ages were a year out (#956). _birthdate_from_age computed ref.year - age - 1 with a random birth month, so a member asked for at age N came out at N+1 whenever that month fell before the reference month -- eight times in twelve. This, more than loose blueprint specs, is why age-predicated programs matched a fraction of the households meant for them. Three blueprints flagged for the Conditional Child Grant are also retuned; one of them specified a youngest child aged 2-6 and so could never satisfy a rule asking for a member under 2. Disability was never recorded (#955). Blueprints flag members is_disabled and nothing acted on it. The fix belongs in the volume generator, not _create_individual_member as first supposed -- almost every demo individual is created there. Each flagged member now gets an approved assessment whose answers meet the Washington Group threshold, which is the only way has_disability can become true: it is related to current_disability_assessment_id and cannot be written. Adds a dependency on spp_disability_registry, without which the field does not exist at all, and corrects disabled_count, which filtered on is_person_with_disability -- a field that exists nowhere. Approval workflows were unwired (#957). Neither the cycle nor the entitlement manager carried an approval definition. That is not a soft gap: approving a cycle raises "The cycle approval definition is not specified!" and preparing entitlements raises its equivalent, so the demo could not show either flow. Wired as its own pass, because neither manager-configuration step reaches every program. Manager hygiene (#1017). A compliance manager is only created for a program that has a compliance rule -- an empty one is not harmless, since has_compliance_criteria and the cycle's compliance filter are both derived from its existence, so the UI offered filtering that could never match. Manager repair is per record rather than per list, so a wrapper whose concrete manager was deleted is rebuilt instead of skipped. Orphan wrappers left by archived programs are swept at the start of each run. Measured on a volume run: Disability Support Grant 0 -> 80 of 680 households, Conditional Child Grant 9 -> 53 with enrolled equal to matched. The regression tests assert set membership rather than counts, since a count check passes while enrolling the wrong people.
…olled Verifying a real Load Demo on a scratch database, rather than a test that rolls back, showed the Cash Transfer Program matching 22 households and holding 14 memberships. The volume pass evaluates each program's rule against the households it generates and the story pass enrols its personas from scripts; neither covers a story household that satisfies a program's rule without being scripted into it. A reconciliation pass fills exactly that gap, after both other passes so scripted enrollments -- and the dates, payments and entitlements they carry -- are never pre-empted. It evaluates the rule the way the program form's Preview Beneficiaries does, base domain included, so the two numbers measure the same thing. Also adds the test for the criterion the earlier suite missed. Asserting that no enrollee contradicts the rule is necessary but not sufficient; it says nothing about matching registrants nobody enrolled. The new test compares counts, and against all memberships rather than the enrolled ones -- the generator deliberately moves about a tenth into exited, paused or not-eligible for realism, which would otherwise consume the whole tolerance for reasons unrelated to targeting. On a real Load Demo every program with a targeting rule now matches its membership count exactly, bar the Disability Support Grant at 81 against 80: one scripted story household that does not satisfy the rule, left alone deliberately.
QA measured enrolled counts against matching counts and three of six programs came out at or over the 10% bar: Universal Child Grant 628 vs 696, Elderly Social Pension 188 vs 206. The cause was the generator's own state variety. It moved 10% of memberships into exited, paused or not eligible for realism, which is exactly the whole tolerance OP#956 allows, so whether a program passed came down to chance. The ticket's tolerance was written for "households that have since exited", so the exit rate has to fit inside it rather than consume it. The rates now total 4% (1% not eligible, 1.5% paused, 1.5% exited), which still leaves a few dozen records per program in mixed states. Verified on a real Load Demo rather than a rolled-back test: every program with a selective rule is now between 1.2% and 4.5% on the enrolled basis QA actually reads. The test that missed this compared against all memberships, which is the right basis for judging targeting but not the number anyone sees first. It now also checks the enrolled count, and a companion test asserts the state mix has not been flattened to nothing by the tightening. Also corrects an overstated changelog line: naming the real field in disabled_count did not make that variable usable. It still fails to compile, and aggregate filters are not applied at evaluation, both outside this module.
…ompared members.count() accepts two call styles: a single argument is the predicate with m implicit, two arguments are an explicit loop variable and predicate. The comparison path read args[0] as the loop variable whichever style was used, and substituted a True predicate when there was no args[1]. So members.count(pred) > n counted every member. It failed silently. Every aggregate count variable expands to the single-argument form, so child_count, elderly_count and working_age_count all returned the household size, and any program targeting on one matched every household. The demo's Universal Child Grant matched all 696 households on child_count > 0 and looked like it was working. exists() was never affected, which is why variables built on it kept working and this went unnoticed. The boolean-context branch already distinguishes the two styles; the comparison branch now does the same. Measured on a real Load Demo: child_count > 0 goes from 696 to 467 households, elderly_count > 0 from 696 to 153, and 153 is exactly what the equivalent members.exists(m, ...) has always returned. Not fixed, and now documented where it matters: an aggregate count nested inside arithmetic still loses its predicate. dependency_ratio is (child_count + elderly_count) / max(1, working_age_count), so the Emergency Relief Fund rule that leans on it still matches every household. child_count + 0 > 0 reproduces it in one line. That is a different path and wants its own change. The demo's note about Emergency Relief Fund is corrected too: it does have a rule, contrary to what that comment claimed. It stays enrolled from blueprint flags, now for the real reason.
…discarding it
(child_count + elderly_count) / max(1, working_age_count) >= 1.5 cannot
be expressed as an Odoo domain: the value depends on counting related
records per parent and then doing arithmetic on the results. The
translator resolved the whole left-hand side to the field id, so
dependency_ratio >= 1.5 compiled to ('id', '>=', 1.5) and matched every
record with nothing logged.
Such comparisons now become an ArithmeticCompare plan node, evaluated
per candidate. Each aggregate leaf is resolved once into a
{parent_id: value} map with a grouped read, so the cost is a few queries
rather than one per record, though it is still a scan of the candidate
set and no SQL fast path applies. Supported inside arithmetic: + - * / %,
unary minus, max/min/abs/round, aggregate counts, literals and numeric
fields.
Verified against hand computation rather than only against itself: for
250 demo households, member birthdates were read directly, the ratio
computed in plain Python, and the result compared with what the
expression matched. 250 agree, 0 disagree. dependency_ratio >= 1.5 goes
from 696 households to 221, and the named variable and its written-out
expansion now return the same set.
An expression that still cannot be resolved raises instead of falling
back to id. That fallback is what made this silent, and for an
eligibility rule matching everyone is worse than refusing to compile.
The demo's Emergency Relief Fund rejoins CEL-driven enrollment: its rule
leans on dependency_ratio, so it matched all 696 households before and
selects 239 now. On a real Load Demo six of the seven programmes are
within 3.4% of their matched count; only Food Assistance stays
flag-driven, its rule matching every active registrant by design.
QA returned OP#1017 on this: the manager cards read "Default Cycle Schedule" and "Default Payment" on every program. That was a deliberate choice earlier in the ticket, on the reasoning that these are the names a hand-built program gets too, so demo data would mirror production. Seeing the cards, that argument does not answer what the requirement actually asks for. Production has the same ambiguity; the demo simply makes it visible across seven programs at once, and no manager list or m2o lookup can tell one program's schedule from another's. Concrete managers are now prefixed with the program's initials, the same shorthand the programs are referred to by and the style the requirement gives: DSG - Default Cycle Schedule, UCG - Basic Cash, CTP - CEL Compliance Criteria. Applied where managers are created and where a dangling one is rebuilt, so a repaired manager is named like the rest. Verified on a real Load Demo. Tests assert every demo manager carries its program's prefix, that no two programs end up with the same manager name, and that the abbreviation is the expected one per program.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #475 +/- ##
==========================================
- Coverage 76.26% 74.50% -1.77%
==========================================
Files 662 716 +54
Lines 44225 52197 +7972
==========================================
+ Hits 33729 38890 +5161
- Misses 10496 13307 +2811
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change needed?
Umbrella OP#958 and all four children: OP#955, OP#956, OP#957, OP#1017. Each is a place where the demo's generated state did not support what the program forms showed.
The tickets' own diagnoses were wrong on the two biggest items, so what follows differs from what they describe:
_create_individual_memberas where theis_disabledflag is dropped. Almost every demo individual is created by the volume generator instead, so fixing the named function changed nothing measurable._birthdate_from_age, which was the main reason programs with age conditions matched a fraction of the households meant for them.How was the change implemented?
Enrollment follows each program's rule (OP#956). Households were enrolled from a static per-blueprint flag while the form previewed the program's CEL, so a program could report 102 enrolled and 9 matching. Programs with a selective rule now enrol whoever that rule matches, and a reconciliation pass after the story enrollments picks up anyone matching that nothing enrolled. Food Assistance stays flag-driven: its rule matches every active registrant by design, so enrolling from it would put the whole population on the programme.
Ages were a year out (OP#956).
birth_year = ref.year - age - 1with a random birth month produced someone agedage + 1whenever that month fell before the reference month — eight times in twelve. Verified across 600 draws per age: the old form returned[1, 2]for a requested 1, the new one returns[1].Disability was never recorded (OP#955).
res.partner.has_disabilitycannot be written — it is related tocurrent_disability_assessment_idand only follows an approved assessment — so each flagged member now gets one carrying answers that meet the Washington Group threshold. Adds a dependency onspp_disability_registry, without which the field does not exist at all.disabled_countalso filtered onis_person_with_disability, a name that appears nowhere in the codebase.Approval workflows were unwired (OP#957). Not a soft gap: approving a cycle raised "The cycle approval definition is not specified!" and preparing entitlements raised its equivalent, so the demo could not show either flow. Wired as its own pass, because neither manager-configuration step reaches every program.
Manager hygiene (OP#1017). Managers are named for their program (
DSG - Default Cycle Schedule) so two programs' managers are tellable apart. A compliance manager is only created where there is a rule to enforce — an empty one switches on compliance filtering that can never match. Repair is per record, so a wrapper whose concrete manager was deleted is rebuilt rather than skipped. Orphans from archived programs are swept.Two translator fixes in
spp_cel_domain:members.count(predicate)lost its predicate when compared. Both call styles are valid — one argument is the predicate withmimplicit, two are an explicit loop variable and predicate — but the comparison path read the first argument as the variable either way. Socount(pred) > ncounted every member, silently. Every aggregate count variable (child_count,elderly_count,working_age_count) returned the household size, and any program targeting on one matched every household.id, sodependency_ratio >= 1.5compiled to('id', '>=', 1.5)and matched everything. Such comparisons are now evaluated per candidate, with each aggregate leaf resolved once into a per-parent map. An expression that still cannot be resolved now raises rather than falling back toid— for an eligibility rule, matching everyone silently is worse than refusing to compile.New unit tests
spp_mis_demo_v2/tests/test_demo_data_quality.py— approval wiring, disability seeding, manager naming and hygiene, and enrollment-versus-eligibility on both bases.spp_cel_domain/tests/test_cel_count_predicate.py— the predicate is honoured, both call styles agree, arithmetic over aggregates discriminates, and an unresolvable expression does not match everyone.Unit tests executed by the author
spp_cel_domain— 657 tests, 0 failedspp_mis_demo_v2— 296 tests, 0 failedspp_studio— 225 tests, 0 failedspp_programs— 714 tests, 0 failed (checked for regressions from the translator change)Verified on a real Load Demo rather than only in tests, and the ratio arithmetic checked against hand computation over 250 households: 250 agree, 0 disagree.
Universal Child Grant reading 467 rather than 696 is the translator fix working: it was matching every household on
child_count > 0and only appeared correct.How to test manually
Full reset, install
spp_mis_demo_v2, then Settings → MIS Demo Wizard → Load Demo. For each headline program compare Preview Beneficiaries against Beneficiaries with the state filter cleared. Check that a household with no members aged 60+ does not appear inelderly_count > 0.Related links
Known and deliberate
spp_programs_smsdoes not exist in this repo.CR/2026/00005,00007,00010), created without the detail fields their strategies require. Pre-existing and unrelated; wants its own ticket.