fix(spp_programs): stop Enroll Eligible undoing a deliberate pause - #436
Conversation
Pausing a membership is a program officer's explicit decision, undone only by Resume. Enroll Eligible re-evaluated paused members along with everyone else and wrote them back to enrolled, silently reversing that decision. The concept was already in the code — a comment noting that duplicated and exited "should only be changed through their own workflows" — but paused was not in the set. Make it explicit as constants.PROTECTED_MEMBERSHIP_STATES and apply it everywhere re-running eligibility decides a membership's state. Three paths, not one. The reported symptom is the enrol branch of _enroll_eligible_registrants, but its disenrol sweep had the same gap and wrote a paused member the eligibility manager did not return to not_eligible — destroying the pause just as thoroughly. The per-membership enroll_eligible_registrants and verify_eligibility on spp.program.membership were a third: their buttons are hidden unless the record is draft, but the methods are public and reachable over RPC, and the ticket's point is that a pause should be trustworthy. The async branch, taken for programs above MIN_ROW_JOB_QUEUE, dispatches into the same _enroll_eligible_registrants, so both branches are covered. Program-level verify_eligibility passes ["enrolled", "not_eligible"] and never sees paused. Nothing outside spp_programs overrides this logic, so SP-MIS and Farmer Registry pick the fix up from the shared code as the ticket expects. Tests were written first and confirmed failing against the unfixed code — four assertions of 'enrolled' != 'paused' plus an error where Resume had nothing paused left to resume — then passing after. Deliberately untouched: deduplication still flags a paused member as duplicated. A duplicate is a duplicate regardless of pause, and that is a different action from the one reported here, though it does mean dedup protects exited and not_eligible while leaving paused open. OP#1117
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #436 +/- ##
==========================================
- Coverage 74.77% 73.15% -1.63%
==========================================
Files 509 480 -29
Lines 35022 32181 -2841
==========================================
- Hits 26189 23541 -2648
+ Misses 8833 8640 -193
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.
The "three paths, not one" analysis is what makes this PR good, and it survived an independent sweep. What I verified:
- The three-paths claim is complete. I went looking for a fourth writer and the candidates all clear:
cycle.py's compliance filter writesstate = "enrolled"onspp.cycle.membership— a different model, no effect on the program-membership pause; every external writer of"enrolled"is a demo-data seeder; and the API v2 programs endpoint doesn't write membership state at all. Nothing outsidespp_programsoverrides the fixed logic, as claimed. - Finding the disenrol-sweep second bug (paused →
not_eligible) beyond the reported symptom is exactly the kind of review-resistant fix this deserves — as is treating the per-membership methods as live paths because they're RPC-reachable regardless of hidden buttons. - The constant carries its reasoning in a docstring, the async branch funnels into the same fixed method, and the tests cover all three paths plus the no-collateral case (a draft neighbour still enrolls while the paused member is skipped) and resume-still-works — written failing-first.
1. Blocker — version bump
Per the ruling (see #391): bump + HISTORY in the PR, number assigned at merge-time rebase. spp_programs sits at 19.0.2.2.0 and shipped in 2026.08. Pure-Python fix, so this is the convention ask, not a breakage one — with it added, this is an approve.
2. Non-blocking — the dedup asymmetry you flagged deserves a ticket
You noted it yourself: deduplication still flags a paused member as duplicated, while it protects exited and not_eligible. "A duplicate is a duplicate" is a defensible position — but now that PROTECTED_MEMBERSHIP_STATES codifies which states are workflow-owned, dedup honouring a different, overlapping set is a design inconsistency that should be a recorded decision rather than an accident. Filed as a follow-up on OpenProject so it gets decided once, deliberately.
The review's one blocker: 19.0.2.2.1 with its changelog entry. Pure Python, so nothing breaks without an upgrade — but a same-version change gives upgraders no signal.
Note the file had no 19.0.2.2.0 entry: the manifest was bumped to that version without one, so the changelog's head was 19.0.2.1.3. This adds 19.0.2.2.1 only — reconstructing what 2.2.0 contained is not something to guess at here.
|
Thanks — bump added and merged as 1. Version bump — One thing worth flagging while I was there:
2. The dedup asymmetry — thanks for filing it. Agreed on the framing: it was defensible as "a duplicate is a duplicate", but once |
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?
Pausing a membership is a program officer's explicit decision, meant to be undone only by Resume. Enroll Eligible re-evaluated paused members along with everyone else and wrote them back to
enrolled, reversing that decision with no warning (OP#1117). Low frequency, high trust impact: an officer who pauses a beneficiary expects it to stick.The concept was already half-present in the code — a comment noting that
duplicatedandexited"should only be changed through their own workflows" — butpausedwas not in the set.How was the change implemented?
constants.PROTECTED_MEMBERSHIP_STATESand applied everywhere re-running eligibility decides a membership's state._enroll_eligible_registrants, but its disenrol sweep had the same gap and would write a paused member the eligibility manager did not return tonot_eligible— destroying the pause just as thoroughly. The per-membershipenroll_eligible_registrantsandverify_eligibilityonspp.program.membershipwere the third: their buttons are hidden unless the record is draft, but the methods are public and reachable over RPC, and the point of the ticket is that a pause should be trustworthy.MIN_ROW_JOB_QUEUE, dispatches into the same_enroll_eligible_registrants, so both branches are covered. Program-levelverify_eligibilitypasses["enrolled", "not_eligible"]and never sees paused.spp_programsoverrides this logic, so SP-MIS and Farmer Registry pick the fix up from the shared code, as the ticket expects.Deliberately untouched: deduplication still flags a paused member as duplicated. A duplicate is a duplicate regardless of pause, and that is a different action from the one reported here — though it does mean dedup protects
exitedandnot_eligiblewhile leavingpausedopen.New unit tests
spp_programs/tests/test_program_enrollment.py— four assertions that a paused membership is still paused after Enroll Eligible (enrol path, disenrol sweep, and the two per-membership methods), plus one that Resume still has something to resume. Written first and confirmed failing against the unfixed code, then passing.Unit tests executed by the author
Full
spp_programssuite on this branch after merging19.0in: 684 tests, 0 failed, 0 errors.How to test manually
not_eligible.Related links