From 17d84034d80ed2d34cbec3106e1cd0b4b6e6d891 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Fri, 7 Aug 2026 15:38:09 +0800 Subject: [PATCH] fix(spp_programs): stop Enroll Eligible undoing a deliberate pause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- spp_programs/models/constants.py | 12 ++ .../models/managers/program_manager.py | 18 ++- spp_programs/models/program_membership.py | 9 +- spp_programs/tests/test_program_enrollment.py | 105 ++++++++++++++++++ 4 files changed, 138 insertions(+), 6 deletions(-) diff --git a/spp_programs/models/constants.py b/spp_programs/models/constants.py index efb0e14d3..10c442b6f 100644 --- a/spp_programs/models/constants.py +++ b/spp_programs/models/constants.py @@ -7,6 +7,18 @@ STATE_ENDED = "ended" STATE_CANCELLED = "cancelled" +#: Membership states that only their own workflow may move a member out of. +#: Re-running eligibility — "Enroll Eligible" / "Verify Eligibility" — must step +#: over these rather than re-deciding them: +#: +#: - ``duplicated`` is resolved by deduplication +#: - ``exited`` is a closed record, reopened only by re-enrolling deliberately +#: - ``paused`` is a program officer's explicit decision, undone only by Resume +#: +#: ``paused`` was missing here, so Enroll Eligible silently resumed paused +#: members and, on the other branch, demoted them to not_eligible (OP#1117). +PROTECTED_MEMBERSHIP_STATES = ("duplicated", "exited", "paused") + MANAGER_ELIGIBILITY = 1 MANAGER_CYCLE = 2 MANAGER_PROGRAM = 3 diff --git a/spp_programs/models/managers/program_manager.py b/spp_programs/models/managers/program_manager.py index 7622e5f0b..21f71337c 100644 --- a/spp_programs/models/managers/program_manager.py +++ b/spp_programs/models/managers/program_manager.py @@ -7,6 +7,7 @@ from odoo.addons.job_worker.delay import group +from .. import constants from ..programs import SPPProgram from .pagination_utils import compute_id_ranges @@ -262,10 +263,13 @@ def _enroll_eligible_registrants(self, states, offset=0, limit=None, min_id=None for el in eligibility_managers: members = el.enroll_eligible_registrants(members) # enroll the one not already enrolled: - # Exclude members that are duplicated or exited — those states - # should only be changed through their own workflows. + # Exclude members in a state only its own workflow may leave — see + # PROTECTED_MEMBERSHIP_STATES. Notably `paused`: a program officer + # paused that member deliberately, and only Resume may undo it (OP#1117). _logger.debug("members filtered: %s", members) - not_enrolled = members.filtered(lambda m: m.state not in ("enrolled", "duplicated", "exited")) + not_enrolled = members.filtered( + lambda m: m.state != "enrolled" and m.state not in constants.PROTECTED_MEMBERSHIP_STATES + ) _logger.debug("not_enrolled: %s", not_enrolled) # Run pre-enrollment hooks (e.g., scoring eligibility checks). @@ -321,9 +325,15 @@ def _enroll_eligible_registrants(self, states, offset=0, limit=None, min_id=None for member in enrollable: program._post_enrollment_hook(member.partner_id) # dis-enroll the one not eligible anymore: + # Same protected states apply on the way down. A paused member the + # eligibility manager did not return was being swept into not_eligible, + # which destroys the pause just as thoroughly as re-enrolling it would + # (OP#1117) — that is a second, separate path to the same bug. enrolled_members_ids = members.ids members_to_remove = member_before.filtered( - lambda m: m.state not in ("not_eligible", "duplicated", "exited") and m.id not in enrolled_members_ids + lambda m: m.state != "not_eligible" + and m.state not in constants.PROTECTED_MEMBERSHIP_STATES + and m.id not in enrolled_members_ids ) # _logger.debug("members_to_remove: %s", members_to_remove) members_to_remove.write( diff --git a/spp_programs/models/program_membership.py b/spp_programs/models/program_membership.py index d4c5d76f2..0b3c5a532 100644 --- a/spp_programs/models/program_membership.py +++ b/spp_programs/models/program_membership.py @@ -280,7 +280,10 @@ def verify_eligibility(self): member = self for em in eligibility_managers: member = em.enroll_eligible_registrants(member) - if len(member) == 0: + if len(member) == 0 and self.state not in constants.PROTECTED_MEMBERSHIP_STATES: + # Leave duplicated / exited / paused alone: each is owned by its own + # workflow, and demoting a paused member to not_eligible would undo a + # deliberate pause just as surely as re-enrolling it (OP#1117). self.state = "not_eligible" return @@ -293,7 +296,9 @@ def enroll_eligible_registrants(self): member = em.enroll_eligible_registrants(member) if len(member) > 0: - if self.state in ("duplicated", "exited"): + if self.state in constants.PROTECTED_MEMBERSHIP_STATES: + # Includes paused: resuming is the Resume button's job, not + # something re-running eligibility may decide (OP#1117). message = _( "Cannot enroll: beneficiary is currently %s.", dict(self._fields["state"].selection).get(self.state, self.state), diff --git a/spp_programs/tests/test_program_enrollment.py b/spp_programs/tests/test_program_enrollment.py index f4a288441..59e8b5b1c 100644 --- a/spp_programs/tests/test_program_enrollment.py +++ b/spp_programs/tests/test_program_enrollment.py @@ -94,6 +94,111 @@ def test_enrollment_skips_exited(self): membership.invalidate_recordset() self.assertEqual(membership.state, "exited") + def test_enrollment_skips_paused(self): + """OP#1117: enrollment does not change paused state to enrolled. + + A pause is a deliberate decision by a program officer and may only be + undone through Resume, so re-running eligibility must step over it — + the same treatment duplicated and exited already get above. + """ + group = self._create_group("Paused Group") + membership = self._enroll(group, "paused") + + self.pm_default._enroll_eligible_registrants(["paused"]) + + membership.invalidate_recordset() + self.assertEqual(membership.state, "paused") + + def test_enroll_eligible_button_leaves_paused_alone(self): + """OP#1117 as reported: via the program's Enroll Eligible button. + + The button passes no state, so every membership is considered — which + is how a paused one was being swept back into enrolled. + """ + group = self._create_group("Paused Via Button") + membership = self._enroll(group, "enrolled") + membership.action_pause() + self.assertEqual(membership.state, "paused", "precondition: membership is paused") + + self.program.enroll_eligible_registrants() + + membership.invalidate_recordset() + self.assertEqual( + membership.state, + "paused", + "Enroll Eligible re-enrolled a paused membership, undoing the pause", + ) + + def test_paused_is_not_demoted_to_not_eligible(self): + """OP#1117, second path: the disenrollment sweep must skip paused too. + + A paused member the eligibility manager does not return was being + written to not_eligible, which destroys the pause just as thoroughly as + re-enrolling it. + """ + group = self._create_group("Paused Ineligible") + membership = self._enroll(group, "paused") + + # Empty state list -> the manager returns nothing, so every member is a + # demotion candidate. + self.pm_default._enroll_eligible_registrants(["paused"]) + + membership.invalidate_recordset() + self.assertEqual(membership.state, "paused") + + def test_paused_skip_does_not_block_other_members(self): + """Guard the fix: skipping paused must not skip everyone else.""" + draft_group = self._create_group("Draft Alongside Paused") + draft = self._enroll(draft_group, "draft") + paused_group = self._create_group("Paused Alongside Draft") + paused = self._enroll(paused_group, "paused") + + self.pm_default._enroll_eligible_registrants(["draft", "paused"]) + + draft.invalidate_recordset() + paused.invalidate_recordset() + self.assertEqual(draft.state, "enrolled", "a draft member should still be enrolled") + self.assertEqual(paused.state, "paused") + + def test_membership_level_enroll_refuses_a_paused_member(self): + """OP#1117, third path: the per-membership Enroll button. + + Its button is hidden unless the membership is draft, but the method is + public and reachable over RPC or from a server action, so it is guarded + rather than left to the view. + """ + group = self._create_group("Paused Single Enroll") + membership = self._enroll(group, "enrolled") + membership.action_pause() + + membership.enroll_eligible_registrants() + + membership.invalidate_recordset() + self.assertEqual(membership.state, "paused") + + def test_membership_level_verify_does_not_demote_a_paused_member(self): + """OP#1117: per-membership Verify must not push paused to not_eligible.""" + group = self._create_group("Paused Single Verify") + membership = self._enroll(group, "enrolled") + membership.action_pause() + + membership.verify_eligibility() + + membership.invalidate_recordset() + self.assertEqual(membership.state, "paused") + + def test_resume_remains_the_only_way_back(self): + """Pause is undone deliberately, through Resume.""" + group = self._create_group("Resumable Group") + membership = self._enroll(group, "enrolled") + membership.action_pause() + self.program.enroll_eligible_registrants() + + membership.invalidate_recordset() + membership.action_resume() + + self.assertEqual(membership.state, "enrolled") + def test_enrollment_enrolls_draft(self): """Enrollment changes draft state to enrolled.""" group = self._create_group("Draft Group")