feat(spp_programs): unify the Program configuration into one set of cards - #472
Merged
Conversation
…p it per program Every card on the Configuration tab was filled through an inline list with a manager_ref_id Reference field. That control asks for a model and then a record of it, and both halves offered other programs' managers — the Reference picker directly, and on the Many2many cards the link dialog behind "Add a line". A manager picked that way keeps running against the program it was created for, so the second program's form showed configuration that was never going to apply to it. Adding now goes through one dialog for every card: which method, what to call it, then the method's own form to configure it. The methods on offer come from the wrapper's _selection_manager_ref_id(), so a module that registers one is included without editing the wizard, and a category with none says so instead of opening an empty list. Eligibility, Entitlement, Cycle, Compliance, Payment and Notifications now share one shape: a badge, an Add button, a row per method with its own cog, an empty state, and an Edit button only when there is exactly one method — it used to open the first of several silently. Notifications was the last section still rendered as a bare group and is now a card like the rest. The rows deny both 'create' and 'link'; for a Many2many the renderer reads the second, which is why create="0" alone never suppressed the row. 'unlink' is untouched, so the x still removes a method. Isolation no longer depends on the form: create and write refuse a manager owned by another program, and duplicating a program copies its methods instead of linking the original's. Only what a write adds is checked, so a database that already holds a cross-program link stays editable and the link can be removed. Duplicate Detection is deliberately untouched here — its card is being converted under OP#1171 on another branch. The isolation rules still cover it.
QA's round 1 finding was that a second cash entitlement could not be added, and that the card should allow several of one kind while refusing the other kind. The first half is real; the cause is not this dialog. spp.program.check_managers_limit refuses a second entitlement manager, and the cycle machinery reaches for exactly one: get_manager() calls ensure_one(), and get_managers() raises NotImplementedError for this kind — six call sites in cycle.py depend on that. Lifting the constraint alone would produce programs that every cycle operation then failed on, so several-per-program needs the engine to iterate managers first, along with decisions about how amounts and approval definitions combine. I tried allowing repeats here and the tests caught it immediately, which is the useful part: the wizard now states the actual limit — "A program supports one for now" and which method is already configured — instead of a duplicate-style message that implied a second one of a different kind would be accepted. Two tests pin this: the second method is refused whichever kind it is, and the engine assumption itself (get_manager works, get_managers raises) is asserted, so if that changes the limit gets revisited rather than silently outliving its reason. The kind restriction QA describes is moot while the limit is one, and lands with the engine work.
QA could not add a cash entitlement line (OP#1172 round 1). The cause was in this CEL view, not in the configuration cards: - it hid the item's `amount` field outright, although the evaluator passes that value into formulas as `base_amount` and the field's own help documents formulas built on it. With no way to set it, "base_amount * ..." could never work, which is what "I don't have a field to define the base amount" meant. The field is back, labelled Base Amount, optional, since a formula may compute the whole thing. - it pointed the formula widget's symbol browser at the "entitlements" CEL profile, whose current record is spp.entitlement with the beneficiary hanging off it as `registrant`. This field is never evaluated in that profile: _validate_cel_expression and _calculate_cel_amount build a context of `me` (the beneficiary) and `base_amount`. So the browser advertised names the evaluator never receives, and a formula taken from it — `r.birthdate` — failed with "Invalid field spp.entitlement.birthdate", the compilation error in QA's screenshot. The browser is off and the placeholder now shows the real vocabulary. Three tests pin it: the base amount stays on the form, the formula field advertises no mismatched profile, and the documented vocabulary — a plain number, a base_amount formula, and a `me.` formula — passes the model's own validator. Aligning the two properly means an entitlement-amount CEL profile whose current record is the beneficiary; until that exists, offering no symbol list beats offering the wrong one.
… item A fixed sum is a normal entitlement, and the item form obliged the user to express it as a CEL formula before the line could be saved. QA asked for the field to be optional "FOR NOW, just to let user define the amount", with the wider rework of this dialog going to its own ticket. Only the view attribute was in the way. The manager that actually runs for cash — CashEntitlementManagerCEL.prepare_entitlements in entitlement_condition_cel.py — already evaluates a formula only when there is one and pays the Base Amount otherwise, so nothing behind the form needed changing. Tests pin both ends: the form no longer marks the field required, and a formula-less item with a Base Amount still pays every beneficiary, so the requirement cannot return unnoticed from either direction.
Resolves the overlap with #445, which promoted Duplicate Detection to a card while this branch promoted Notifications. Both cards are kept; the old "Optional Features" group that held each of them beforehand is gone, along with the closing tags it left behind. Two collisions the text merge could not see: spp.program had ended up with two write() methods in one class -- the configuration-isolation guard from this branch and the operation-lock guard from 19.0. Python keeps the later definition, so the isolation guard was silently gone and its test failed. Folded into one method, with a note that anything else hooking writes belongs there too. #445's sweep test built its fixture by linking one program's manager into another, which is precisely what this branch forbids. That shape can still exist in databases predating the rule, and sparing it is what the test is for, so it now writes the relation row directly instead of going through write(). Also drops the manager setup wizard's validator ACL row, matching the decision made for the deduplication wizard in #445: the Add button that opens it is admin/manager only, so the row granted a path validators cannot reach. spp_programs takes 19.0.2.4.0. 765 tests, 0 failures.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #472 +/- ##
==========================================
+ Coverage 76.07% 76.37% +0.30%
==========================================
Files 661 682 +21
Lines 44022 45036 +1014
==========================================
+ Hits 33488 34398 +910
- Misses 10534 10638 +104
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
approved these changes
Aug 28, 2026
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
Member
There was a problem hiding this comment.
Review — approved
Reviewed at 7bee7bc (19.0 merged in, spp_programs 19.0.2.4.0). CI fully green including codecov patch; QA passed on round 3 in OP#1172. I read the full diff and traced every contract the new code leans on. Verdict: approve, with one non-blocking suggestion below.
What I verified
- The wizard's create path is sound end-to-end.
action_create_managercreates the concrete with_spp_wrapper_model+default_program_idin context;spp.manager.source.mixin.createbuilds the wrapper and, for Many2many cards, links it via_spp_program_m2m_field— and that program-sidewrite()passes the new isolation guard because the wrapper'sprogram_idis this program. Dismissing the dialog leaves nothing behind, preserving #953's property. - The two-
write()fold is correct.programs.pynow has exactly onewrite(); it calls_assert_operation_lock_writable(vals)first (the #337 guard is intact, and its NOTE comment survives below) and then runs the isolation check on added links only. The before/after ID-set diff handles(4,…),(6,…)and creation-time links alike, and the "polluted legacy DB stays editable" test pins the escape hatch from both directions. - The empty-formula claim is true. The model-level constraint (
_check_cel_expression) only validates a non-empty expression, and theprepare_entitlementsthat actually dispatches for cash isentitlement_condition_cel.py's (it loads afterentitlement_amount_cel.pyincel/__init__.py, so it wins the registry MRO) — line 382 only evaluates a formula when one is set, else paysrec.amount * multiplier.test_07bproves the dispatch through a realprepare_entitlementsrun, and the docstring added totest_07correctly reframes the raise as a guard, not a user path. - The symbol-browser diagnosis is right.
_validate_cel_expression/_calculate_cel_amountbuild their ownme/base_amountcontext, so theentitlementsprofile the widget advertised could never match. Turning the browser off and documenting the real vocabulary is the honest fix; the form tests pin all three view properties. - ACL is consistent. The wizard's
ir.model.accessrows (programs manager + admin) match exactly whocan_edit_configurationlets see the Add button — validators get the View path only. copy()isolation correctly zeroes the config fields before the super call (otherwise the O2M/M2M defaults would link or duplicate wrappers), then copies each concrete with the mixin context so the duplicate owns real copies. Dangling references are guarded withexists().- The #445 sweep-test fixture change is faithful to that test's purpose: the cross-program shape it must spare can no longer be built through the ORM, so writing the relation row directly is the right move, and the docstring says why.
- Version chain: base is 2.3.4 (#445/#447 merged), this takes 2.4.0 — a feature bump is right for a new model + view rework. HISTORY.md and README.rst agree.
- No stale callers:
action_add_compliance_manager/action_add_payment_managersurvive as thin shims,notification_manager_summaryhas no remaining view references, and_open_manager_setup_wizard's dead-end notification is gone.
One non-blocking suggestion
MANAGER_CATEGORIES marks only entitlement as single_manager: True, but check_managers_limit also caps cycle, payment, and program managers at one. Today that gap is unreachable through the dialog — each of those categories has exactly one concrete method in-repo, so the same-method check fires first with its decent message. But the moment any module registers a second cycle or payment method (exactly as spp_program_geofence does for eligibility), the dialog will accept it and the user gets the constraint's old wording — "Only one manager can be configured under Cycle Managers. Please delete any new manager(s)…" — after the fact, which is the experience this PR set out to remove. The card comment "a program may legitimately run more than one method" is also not true for those two cards.
Suggest adding single_manager: True to cycle, payment, and program (with the same style of comment pointing at check_managers_limit), so the friendly refusal covers everything the engine actually enforces. Fine as a fast-follow if you'd rather not re-run QA over it.
Minor observations (no action needed)
_sweep_removed_methodsis a second copy of the OP#1171 dedup wizard's sweep. Once the deduplication card migrates onto this wizard, the dedup-specific one can fold in — worth remembering, not worth doing here.- The duplicate check reads
manager_ref_id._nameon wrappers filtered only for truthiness, so a wrapper whose Reference dangles (target hard-deleted) could still block a re-add of that model until the sweep collects it. Pre-existing dirt-tolerance, and the sweep runs first, so the window is tiny.
Review catch: check_managers_limit refuses a second entitlement, cycle, payment or program manager, but only entitlement carried single_manager in MANAGER_CATEGORIES. The dialog would accept a second cycle or payment method and let the constraint refuse it afterwards with "Only one manager can be configured under ... Please delete any new manager(s) before saving" -- the after-the-fact wording this dialog exists to replace. Unreachable today: each of those categories has one concrete method in this repo, so the already-configured check fires first. It stops being unreachable the moment a module registers a second one, which is exactly what spp_program_geofence does for eligibility. A test pins the two against each other by reading the field names out of check_managers_limit, so they cannot drift apart again. Also corrects the six card comments claiming a program may run more than one method of any category. True for eligibility, compliance, deduplication and notification; not for the four the engine caps.
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
2 participants
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?
The Program Configuration tab was configured two different ways. Some categories were cards; the rest were bare editable lists whose only column was a Reference field, so adding one meant pick a model, then find or create a record of it — the underlying plumbing, exposed. QA could not add a cash entitlement at all through that path.
Three separate faults came out of testing it:
500as an expression. The Base Amount field was also hidden outright, even though formulas are documented to build on it asbase_amount— so there was nothing for them to multiply.mefor the beneficiary andbase_amountfor the fixed amount. A formula built from the browser could not compile, which is theInvalid field spp.entitlement.birthdateerror QA hit.Resolves OP#1172.
How was the change implemented?
One shape for every category. Each is now a card: what is configured,
+ Addopening a dialog that asks which method and what to call it, and a cog per row to open it. A sharedspp.manager.setup.wizardbacks all of them, replacing the per-category wiring. Selecting a method the program already has is refused with a message naming it, rather than a duplicate-record error.Configuration belongs to one program. A manager names the program it was created for and runs against it, so linking one into a second program does not configure the second — it only makes the form lie about what will happen. The tab no longer offers a picker that can do this, and
create/writerefuse it for the API, imports and duplicated programs. Only links being added are checked, so a database already holding a cross-program link stays loadable and the row's ✕ still removes it.The entitlement item. The formula is optional and the Base Amount is visible again. The manager that runs for cash already paid the Base Amount when no formula was set, so only the view attribute was in the way. The symbol browser is off until an entitlement-amount CEL profile exists; the placeholder and help name the real vocabulary meanwhile.
New unit tests
test_manager_setup_wizard.py— the dialog per category, the refusal when a method is already configured, and configuration isolation acrosscreate,writeandcopy.test_entitlement_amount_cel.py— the Base Amount stays on the form, the formula is not required, the advertised symbols match what the evaluator receives, and a formula-less item still pays every beneficiary through a real cycle.Unit tests executed by the author
spp_programs— 765 tests, 0 failed, 0 errors, after merging19.0in.How to test manually
+ Addopens a dialog asking for method and name; the cog opens the created record.500and no formula — it saves, and a prepared cycle pays 500.base_amount * 1.1— the preview shows a sample and a cycle pays 550.Related links
Note for review
Merging
19.0in surfaced two collisions git could not:spp.programhad ended up with twowrite()methods in one class — this branch's isolation guard and 19.0's operation-lock guard — where Python keeps only the later one, silently dropping the first. They are folded into a single method. And #445's sweep test built its fixture by linking one program's manager into another, which this branch forbids; it now writes that legacy shape directly, since sparing it is the case the test exists for.