From 468f888b64cd9111bb556dd146c84af8e2a07f71 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Tue, 18 Aug 2026 13:34:36 +0800 Subject: [PATCH 1/5] feat(spp_programs): one Add dialog for program configuration, and keep it per program MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- spp_programs/__manifest__.py | 1 + spp_programs/models/constants.py | 53 +++ spp_programs/models/program_manager_ui.py | 124 +++--- spp_programs/models/programs.py | 96 ++++- spp_programs/security/ir.model.access.csv | 3 + spp_programs/tests/__init__.py | 1 + .../tests/test_manager_setup_wizard.py | 271 +++++++++++++ .../views/program_config_cards_view.xml | 359 ++++++++++++------ spp_programs/wizard/__init__.py | 1 + spp_programs/wizard/manager_setup_wizard.py | 196 ++++++++++ spp_programs/wizard/manager_setup_wizard.xml | 59 +++ 11 files changed, 992 insertions(+), 172 deletions(-) create mode 100644 spp_programs/tests/test_manager_setup_wizard.py create mode 100644 spp_programs/wizard/manager_setup_wizard.py create mode 100644 spp_programs/wizard/manager_setup_wizard.xml diff --git a/spp_programs/__manifest__.py b/spp_programs/__manifest__.py index 8dac1cba2..c3029d903 100644 --- a/spp_programs/__manifest__.py +++ b/spp_programs/__manifest__.py @@ -114,6 +114,7 @@ "wizard/enrollment_wizard_views.xml", "wizard/exit_membership_wizard.xml", "wizard/prepare_entitlement_confirm_wizard.xml", + "wizard/manager_setup_wizard.xml", ], "assets": { "web.assets_backend": [ diff --git a/spp_programs/models/constants.py b/spp_programs/models/constants.py index efb0e14d3..f8c0bf23e 100644 --- a/spp_programs/models/constants.py +++ b/spp_programs/models/constants.py @@ -40,3 +40,56 @@ "spp.compliance.manager": "spp.compliance.manager.default", }, } + +# The cards on a program's Configuration tab (OP#1172). Each names the field on +# spp.program, the wrapper model behind it, and the wording the Add dialog uses. +# The keys match MANAGER_TYPE_INFO's "category" so the two can be read together: +# this map says where a category lives, MANAGER_TYPE_INFO describes the methods +# inside it. +# +# The concrete methods themselves are deliberately absent. They come from the +# wrapper's `_selection_manager_ref_id()`, which is what other modules extend +# when they add one — spp_program_geofence adds an eligibility method that way, +# and a hard-coded list here would never see it. +MANAGER_CATEGORIES = { + "eligibility": { + "field": "eligibility_manager_ids", + "wrapper": "spp.eligibility.manager", + "label": "Eligibility Method", + }, + "entitlement": { + "field": "entitlement_manager_ids", + "wrapper": "spp.program.entitlement.manager", + "label": "Entitlement Type", + }, + "cycle": { + "field": "cycle_manager_ids", + "wrapper": "spp.cycle.manager", + "label": "Cycle Schedule", + }, + "compliance": { + "field": "compliance_manager_ids", + "wrapper": "spp.compliance.manager", + "label": "Compliance Criteria", + }, + "payment": { + "field": "payment_manager_ids", + "wrapper": "spp.program.payment.manager", + "label": "Payment Method", + }, + "deduplication": { + "field": "deduplication_manager_ids", + "wrapper": "spp.deduplication.manager", + "label": "Deduplication Method", + }, + "notification": { + "field": "notification_manager_ids", + "wrapper": "spp.program.notification.manager", + "label": "Notification Channel", + }, + "program": { + "field": "program_manager_ids", + "wrapper": "spp.program.manager", + "label": "Program Manager", + }, +} diff --git a/spp_programs/models/program_manager_ui.py b/spp_programs/models/program_manager_ui.py index b44621553..f05c82936 100644 --- a/spp_programs/models/program_manager_ui.py +++ b/spp_programs/models/program_manager_ui.py @@ -7,6 +7,9 @@ """ from odoo import _, api, fields, models +from odoo.exceptions import UserError + +from .constants import MANAGER_CATEGORIES def _format_recurrence(duration, rrule_type): @@ -241,6 +244,12 @@ class ProgramManagerUI(models.Model): payment_manager_display = fields.Char(compute="_compute_banner_layout_helpers") payment_manager_detail = fields.Text(compute="_compute_banner_layout_helpers") + # OP#1172: Notifications became a card like the rest, so it needs the same + # single-vs-multi helpers the other cards use. + notification_manager_count = fields.Integer(compute="_compute_banner_layout_helpers") + notification_manager_display = fields.Char(compute="_compute_banner_layout_helpers") + notification_manager_detail = fields.Text(compute="_compute_banner_layout_helpers") + @api.depends("eligibility_manager_ids", "eligibility_manager_ids.manager_ref_id") def _compute_eligibility_summary(self): for rec in self: @@ -403,6 +412,8 @@ def _compute_compliance_summary(self): "compliance_manager_ids.manager_ref_id", "payment_manager_ids", "payment_manager_ids.manager_ref_id", + "notification_manager_ids", + "notification_manager_ids.manager_ref_id", ) def _compute_banner_layout_helpers(self): """Populate the `_manager_count / _display / _detail` fields @@ -413,6 +424,7 @@ def _compute_banner_layout_helpers(self): ("cycle_manager_ids", "cycle"), ("compliance_manager_ids", "compliance"), ("payment_manager_ids", "payment"), + ("notification_manager_ids", "notification"), ) for rec in self: for field_name, prefix in banners: @@ -624,86 +636,68 @@ def action_configure_compliance(self): return self._open_manager_setup_wizard("compliance") return False - def action_add_compliance_manager(self): - """Open the default compliance manager form in create mode. - - The program form's compliance banner shows a `+ Add` zero-state - button when no compliance manager is configured. We open the - concrete model (`spp.compliance.manager.default`) in create mode - with `default_program_id` and `_spp_wrapper_model` in context. - Saving the dialog runs the source-mixin's `create()` override, - which auto-creates the wrapper (see source_mixin.py). Dismissing - the dialog with `X` leaves nothing in the DB — that's the whole - point of #953. + def action_add_manager(self): + """Open the Add dialog for one Configuration card (OP#1172). + + One action serves every card: the button passes its category in the + context, so adding an eligibility method and adding a payment method + are the same gesture instead of one bespoke action per section. + + The methods on offer come from the wrapper, so a category whose module + is not installed says so rather than opening a dialog with an empty + list — notifications have no channel at all until a bridge module such + as SMS is installed. """ self.ensure_one() - if not self.can_edit_configuration: + if not self.can_edit_configuration or self.state == "ended": return False - if self.compliance_manager_ids: - return self.action_configure_compliance() - Concrete = self.env["spp.compliance.manager.default"] + category = self.env.context.get("manager_category") + info = MANAGER_CATEGORIES.get(category) + if not info: + raise UserError(_("Unknown configuration category %s.") % category) + wizard = self.env["spp.manager.setup.wizard"] + methods = wizard._methods_for_category(category) + if not methods: + raise UserError( + _("No %s is available. Install a module that provides one, then add it here.") % info["label"].lower() + ) return { "type": "ir.actions.act_window", - "name": _("Compliance Criteria"), - "res_model": Concrete._name, + "name": _("Add a %s") % info["label"], + "res_model": wizard._name, "view_mode": "form", - "views": [(Concrete.get_manager_view_id(), "form")], + "views": [(False, "form")], "target": "new", "context": { "default_program_id": self.id, - # The mixin's create() will create the wrapper and rely - # on its `program_id` inverse to populate the program's - # One2many `compliance_manager_ids` automatically — no - # m2m write needed. - "_spp_wrapper_model": "spp.compliance.manager", + "default_category": category, + "default_method": methods[0][0], + "default_name": methods[0][1], }, } - def action_add_payment_manager(self): - """Open the default payment manager form in create mode. - - Mirrors `action_add_compliance_manager`. The concrete model's - `create()` override auto-creates the default batch tag if the - form was saved with `create_batch=True` and no tag selected — - so we don't have to pre-create it here (which would orphan the - tag if the user dismisses the dialog). The source-mixin's - `create()` override creates the wrapper, then writes it into - the program's `payment_manager_ids` Many2many because that - field doesn't auto-resolve via the wrapper's `program_id` - inverse. See #953. + def action_add_compliance_manager(self): + """Compliance's Add button, kept for callers that predate OP#1172. + + Compliance opened its concrete form directly (#952) and payment did the + same (#953), while the other cards had no Add at all. Every card now + goes through one dialog, so all this does is name the category. """ - self.ensure_one() - if not self.can_edit_configuration: - return False - if self.payment_manager_ids: - return self.action_configure_payment() - Concrete = self.env["spp.program.payment.manager.default"] - return { - "type": "ir.actions.act_window", - "name": _("Payment Processing"), - "res_model": Concrete._name, - "view_mode": "form", - "views": [(Concrete.get_manager_view_id(), "form")], - "target": "new", - "context": { - "default_program_id": self.id, - "_spp_wrapper_model": "spp.program.payment.manager", - "_spp_program_m2m_field": "payment_manager_ids", - }, - } + return self.with_context(manager_category="compliance").action_add_manager() + + def action_add_payment_manager(self): + """Payment's Add button, kept for callers that predate OP#1172.""" + return self.with_context(manager_category="payment").action_add_manager() def _open_manager_setup_wizard(self, manager_type): - """Open wizard to set up a new manager of the specified type.""" - return { - "type": "ir.actions.client", - "tag": "display_notification", - "params": { - "title": _("Setup Required"), - "message": _("Please add a %s manager first using the list below.") % manager_type, - "sticky": False, - "type": "warning", - }, - } + """Point a caller at the Add dialog for this category (OP#1172). + + This used to pop a warning telling the user to "add a manager using the + list below" — the inline list with the Reference field, which is the + control this ticket removes. The categories it is called with are the + MANAGER_CATEGORIES keys, so it can now open the real thing. + """ + return self.with_context(manager_category=manager_type).action_add_manager() def get_manager_type_options(self, category): """Get available manager type options for a category.""" diff --git a/spp_programs/models/programs.py b/spp_programs/models/programs.py index 5841fb332..e3e44de92 100644 --- a/spp_programs/models/programs.py +++ b/spp_programs/models/programs.py @@ -2,7 +2,7 @@ import logging from odoo import _, api, fields, models -from odoo.exceptions import UserError +from odoo.exceptions import UserError, ValidationError from . import constants @@ -203,6 +203,77 @@ def _check_unique_program_name(self): if existing: raise UserError(_("A program with this name already exists. Program names must be unique.")) + # ------------------------------------------------------------------ + # configuration isolation (OP#1172) + # ------------------------------------------------------------------ + + @staticmethod + def _configuration_fields(): + """The Configuration tab's fields, in one place for the rules below.""" + return [info["field"] for info in constants.MANAGER_CATEGORIES.values()] + + def _check_configuration_is_own(self, field, wrappers): + """Refuse configuration that belongs to another program. + + Every manager names the program it was created for and runs against + that program, so linking one into a second program does not configure + the second — it only makes the form lie about what will happen. The + Configuration tab no longer offers a picker that can do this; this + covers the API, data imports and duplicated programs. + + Only what is being linked now is checked. A database that already holds + a cross-program link from the old picker stays loadable, and the row's + ✕ can still take it off. + """ + self.ensure_one() + for wrapper in wrappers: + concrete = wrapper.manager_ref_id + owner = wrapper.program_id or ( + concrete.program_id if concrete and "program_id" in concrete._fields else False + ) + if owner and owner != self: + raise ValidationError( + _( + "%(method)s belongs to the program %(owner)s, so it cannot be used by " + "%(program)s as well. Each program's configuration is its own — add a " + "method to this program instead." + ) + % { + "method": wrapper.display_name or self.env[wrapper._name]._description, + "owner": owner.display_name, + "program": self.display_name, + } + ) + + def copy(self, default=None): + """Duplicate the program with its own copy of the configuration. + + These fields are mostly Many2many, so a plain copy would link the + source's managers into the duplicate — the sharing this ticket removes. + Each method is copied instead: the duplicate starts configured the same + way and owns what it runs. + """ + default = dict(default or {}) + fields_to_copy = [field for field in self._configuration_fields() if field in self._fields] + for field in fields_to_copy: + default.setdefault(field, False) + new_programs = super().copy(default) + for source, new_program in zip(self, new_programs, strict=False): + for field in fields_to_copy: + context = { + "_spp_wrapper_model": source._fields[field].comodel_name, + "default_program_id": new_program.id, + } + if source._fields[field].type == "many2many": + # A Many2many does not resolve from the wrapper's program_id, + # so the copy has to be linked explicitly — see the source mixin. + context["_spp_program_m2m_field"] = field + for wrapper in source[field]: + concrete = wrapper.manager_ref_id + if concrete and concrete.exists(): + concrete.with_context(**context).copy({"program_id": new_program.id}) + return new_programs + @api.depends("program_membership_ids") def _compute_has_members(self): if self.env.context.get("skip_program_statistics"): @@ -276,6 +347,13 @@ def _compute_can_edit_configuration(self): @api.model def create(self, vals): res = super().create(vals) + # Everything linked at creation is being linked now, so all of it is + # checked. Reading `vals` instead would miss it: base create() is + # model_create_multi, so what arrives here is a list of dicts. + for record in res: + for field in record._configuration_fields(): + if record[field]: + record._check_configuration_is_own(field, record[field]) if self.env.context.get("skip_default_managers"): return res if self.env.context.get("create_default_managers"): @@ -285,6 +363,22 @@ def create(self, vals): res.update({man: [(4, man_ids[man])]}) return res + def write(self, vals): + """Refuse configuration linked in from another program (OP#1172). + + Only the links this write adds are checked, so a database that already + holds a cross-program link stays editable and the link can be removed. + """ + touched = [field for field in self._configuration_fields() if field in vals] + before = {(rec.id, field): set(rec[field].ids) for rec in self for field in touched} + result = super().write(vals) + for rec in self: + for field in touched: + added = set(rec[field].ids) - before[(rec.id, field)] + if added: + rec._check_configuration_is_own(field, rec[field].browse(sorted(added))) + return result + @api.model def create_default_managers(self, program_id): ret_vals = {} diff --git a/spp_programs/security/ir.model.access.csv b/spp_programs/security/ir.model.access.csv index baced8b9a..5c6c93a5d 100644 --- a/spp_programs/security/ir.model.access.csv +++ b/spp_programs/security/ir.model.access.csv @@ -404,3 +404,6 @@ access_spp_prepare_entitlement_confirm_wizard_validator,Prepare Entitlement Conf access_spp_program_membership_exit_wizard_officer,Program Membership Exit Wizard Officer Access,spp_programs.model_spp_program_membership_exit_wizard,spp_programs.group_programs_officer,1,1,1,0 access_spp_program_membership_exit_wizard_manager,Program Membership Exit Wizard Manager Access,spp_programs.model_spp_program_membership_exit_wizard,spp_programs.group_programs_manager,1,1,1,1 access_spp_program_membership_exit_wizard_admin,Program Membership Exit Wizard Admin Access,spp_programs.model_spp_program_membership_exit_wizard,spp_security.group_spp_admin,1,1,1,1 +access_spp_manager_setup_wizard_manager,Manager Setup Wizard Manager Access,spp_programs.model_spp_manager_setup_wizard,group_programs_manager,1,1,1,1 +access_spp_manager_setup_wizard_validator,Manager Setup Wizard Validator Access,spp_programs.model_spp_manager_setup_wizard,group_programs_validator,1,1,1,0 +access_spp_manager_setup_wizard_admin,Manager Setup Wizard Admin Access,spp_programs.model_spp_manager_setup_wizard,spp_security.group_spp_admin,1,1,1,1 diff --git a/spp_programs/tests/__init__.py b/spp_programs/tests/__init__.py index 15dc1cbe6..a08b13f2a 100644 --- a/spp_programs/tests/__init__.py +++ b/spp_programs/tests/__init__.py @@ -43,3 +43,4 @@ from . import test_cycle_null_entitlement_approval from . import test_approve_entitlements_program_isolation from . import test_payment_batch_payment_ids +from . import test_manager_setup_wizard diff --git a/spp_programs/tests/test_manager_setup_wizard.py b/spp_programs/tests/test_manager_setup_wizard.py new file mode 100644 index 000000000..45b4205cc --- /dev/null +++ b/spp_programs/tests/test_manager_setup_wizard.py @@ -0,0 +1,271 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""OP#1172: one way to configure a program, and it stays that program's own. + +Every card on the Configuration tab used to be filled through an inline list +with a `manager_ref_id` Reference field. Both halves of that control offered +other programs' managers, so one program could be configured with another's +while the manager went on running against the program it was created for. + +These tests cover the replacement — one Add dialog for every card — and the +isolation rules that hold whether the configuration arrives from the form, the +API, or a duplicated program. +""" + +from lxml import etree + +from odoo.exceptions import UserError, ValidationError +from odoo.tests import TransactionCase, tagged + +from ..models.constants import MANAGER_CATEGORIES + +# The cards this branch converted. Deduplication is deliberately absent: its +# card is being converted under OP#1171 and lands separately. +CONVERTED = ["eligibility", "entitlement", "cycle", "compliance", "payment", "notification"] + + +@tagged("post_install", "-at_install") +class TestManagerSetupWizard(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.program = cls.env["spp.program"].create({"name": "Manager Setup Wizard [TEST]"}) + cls.wizard_model = cls.env["spp.manager.setup.wizard"] + + def _add(self, category, method=None, name="A method", program=None): + """Add a method the way the dialog does.""" + methods = self.wizard_model._methods_for_category(category) + wizard = self.wizard_model.with_context(default_category=category).create( + { + "program_id": (program or self.program).id, + "category": category, + "method": method or methods[0][0], + "name": name, + } + ) + wizard.action_create_manager() + return wizard + + def _configured(self, category, program=None): + return (program or self.program)[MANAGER_CATEGORIES[category]["field"]] + + # ------------------------------------------------------------------ + # the dialog + # ------------------------------------------------------------------ + + def test_every_card_can_be_configured_through_the_dialog(self): + """One dialog, every category — including the One2many one. + + Compliance resolves from the wrapper's program_id while the rest are + Many2many and need an explicit link, which is the step that used to be + forgotten: the manager gets created and the program never picks it up. + """ + for category in CONVERTED: + methods = self.wizard_model._methods_for_category(category) + if not methods: + continue + with self.subTest(category=category): + program = self.env["spp.program"].create({"name": f"Dialog {category} [TEST]"}) + self._add(category, name=f"My {category}", program=program) + + configured = self._configured(category, program) + self.assertEqual(len(configured), 1, f"{category} should be wired to the program") + self.assertEqual(configured.manager_ref_id.name, f"My {category}") + self.assertEqual(configured.manager_ref_id.program_id, program) + + def test_the_methods_come_from_the_wrapper_not_a_list_here(self): + """So a module that adds a method is offered without editing the wizard.""" + wrapper = self.env[MANAGER_CATEGORIES["eligibility"]["wrapper"]] + offered = dict(self.wizard_model._methods_for_category("eligibility")) + + self.assertTrue(offered, "eligibility should offer at least one method") + for model, _label in wrapper._selection_manager_ref_id(): + if model in self.env: + self.assertIn(model, offered, f"{model} is registered on the wrapper but not offered") + + def test_an_unknown_category_offers_nothing(self): + self.assertEqual(self.wizard_model._methods_for_category("not-a-category"), []) + + def test_the_name_is_suggested_from_the_method(self): + methods = self.wizard_model._methods_for_category("eligibility") + if len(methods) < 2: + self.skipTest("needs a category with more than one method") + wizard = self.wizard_model.with_context(default_category="eligibility").new( + {"program_id": self.program.id, "category": "eligibility", "method": methods[0][0]} + ) + wizard._onchange_method_suggests_a_name() + self.assertEqual(wizard.name, methods[0][1]) + + # A name the user typed is left alone. + wizard.name = "Our own wording" + wizard.method = methods[1][0] + wizard._onchange_method_suggests_a_name() + self.assertEqual(wizard.name, "Our own wording") + + def test_the_same_method_cannot_be_added_twice(self): + program = self.env["spp.program"].create({"name": "Twice [TEST]"}) + self._add("eligibility", name="First", program=program) + + with self.assertRaises(UserError): + self._add("eligibility", name="Second", program=program) + + def test_a_method_can_be_added_again_after_it_is_removed(self): + """The ✕ on a Many2many row removes the relation, not the record. + + The leftover kept its program_id, so the duplicate check used to refuse + a method the card no longer showed (OP#1171). + """ + program = self.env["spp.program"].create({"name": "Re-add [TEST]"}) + self._add("eligibility", name="First", program=program) + removed = program.eligibility_manager_ids + program.write({"eligibility_manager_ids": [(3, removed.id)]}) + + self._add("eligibility", name="Second", program=program) + + self.assertEqual(len(program.eligibility_manager_ids), 1, "the method should be back") + self.assertFalse(removed.exists(), "the removed method should not linger") + + def test_a_category_with_no_method_says_so(self): + """Notifications have no channel until a bridge module is installed.""" + empty = [c for c in CONVERTED if not self.wizard_model._methods_for_category(c)] + if not empty: + self.skipTest("every category has a method installed") + with self.assertRaises(UserError): + self.program.with_context(manager_category=empty[0]).action_add_manager() + + def test_add_is_refused_on_an_ended_program(self): + program = self.env["spp.program"].create({"name": "Ended [TEST]", "state": "ended"}) + self.assertFalse(program.with_context(manager_category="eligibility").action_add_manager()) + + def test_the_dead_end_helper_now_opens_the_dialog(self): + """It used to pop "add a manager using the list below" — that list is gone.""" + action = self.program._open_manager_setup_wizard("eligibility") + + self.assertEqual(action.get("type"), "ir.actions.act_window") + self.assertEqual(action.get("res_model"), "spp.manager.setup.wizard") + + # ------------------------------------------------------------------ + # isolation + # ------------------------------------------------------------------ + + def test_another_programs_method_cannot_be_linked_in(self): + owner = self.env["spp.program"].create({"name": "Owner [TEST]"}) + self._add("eligibility", name="Owner's rule", program=owner) + borrower = self.env["spp.program"].create({"name": "Borrower [TEST]"}) + + with self.assertRaises(ValidationError): + borrower.write({"eligibility_manager_ids": [(4, owner.eligibility_manager_ids.id)]}) + + def test_another_programs_method_cannot_be_linked_at_creation(self): + owner = self.env["spp.program"].create({"name": "Owner At Create [TEST]"}) + self._add("eligibility", name="Owner's rule", program=owner) + + with self.assertRaises(ValidationError): + self.env["spp.program"].create( + { + "name": "Borrower At Create [TEST]", + "eligibility_manager_ids": [(4, owner.eligibility_manager_ids.id)], + } + ) + + def test_a_database_that_already_shares_one_stays_editable(self): + """Only what a write adds is checked. + + Rejecting everything already linked would trap a database polluted by + the old picker: the ✕ is itself a write, and with two foreign methods + linked, removing one would be refused because of the other. + """ + owner = self.env["spp.program"].create({"name": "Legacy Owner [TEST]"}) + self._add("eligibility", name="First", program=owner) + self._add("cycle", name="Second", program=owner) + polluted = self.env["spp.program"].create({"name": "Legacy Borrower [TEST]"}) + for field_name, wrapper in ( + ("eligibility_manager_ids", owner.eligibility_manager_ids), + ("cycle_manager_ids", owner.cycle_manager_ids), + ): + field = self.env["spp.program"]._fields[field_name] + self.env.cr.execute( + f"INSERT INTO {field.relation} ({field.column1}, {field.column2}) VALUES (%s, %s)", + (polluted.id, wrapper.id), + ) + polluted.invalidate_recordset() + + # Taking one off is a write on a field that still holds the other. + polluted.write({"eligibility_manager_ids": [(3, owner.eligibility_manager_ids.id)]}) + + self.assertFalse(polluted.eligibility_manager_ids) + self.assertEqual(polluted.cycle_manager_ids, owner.cycle_manager_ids) + + def test_duplicating_a_program_copies_its_configuration(self): + """A plain copy would link the source's methods into the duplicate.""" + source = self.env["spp.program"].create({"name": "Source [TEST]"}) + self._add("eligibility", name="Source rule", program=source) + + duplicate = source.copy({"name": "Duplicate [TEST]"}) + + self.assertTrue(duplicate.eligibility_manager_ids, "the duplicate should be configured too") + self.assertNotEqual( + duplicate.eligibility_manager_ids, + source.eligibility_manager_ids, + "the duplicate must not share the source's method", + ) + self.assertEqual(duplicate.eligibility_manager_ids.manager_ref_id.program_id, duplicate) + self.assertEqual(source.eligibility_manager_ids.manager_ref_id.name, "Source rule") + + # ------------------------------------------------------------------ + # the cards + # ------------------------------------------------------------------ + + def _arch(self): + return etree.fromstring(self.env.ref("spp_programs.view_program_form_config_cards").arch) + + def test_every_card_offers_add(self): + arch = self._arch() + for category in CONVERTED: + with self.subTest(category=category): + buttons = arch.xpath(f"//button[@name='action_add_manager'][contains(@context, \"'{category}'\")]") + self.assertTrue(buttons, f"the {category} card needs an Add button") + + def test_no_card_edits_the_reference_field_inline(self): + """The Reference field is what offered other programs' managers.""" + arch = self._arch() + for category in CONVERTED: + field = MANAGER_CATEGORIES[category]["field"] + with self.subTest(category=category): + self.assertFalse( + arch.xpath(f"//field[@name='{field}']//field[@name='manager_ref_id']"), + f"{category} should list methods, not edit their Reference", + ) + + def test_no_card_offers_add_a_line(self): + """Denied through 'link' as well as 'create'. + + These fields are Many2many, and for those the list renderer reads + `"link" in activeActions ? link : create`, so create="0" on the list + was never consulted and the row it left opened a picker listing every + program's managers. + """ + arch = self._arch() + for category in CONVERTED: + field_name = MANAGER_CATEGORIES[category]["field"] + with self.subTest(category=category): + field = arch.xpath(f"//field[@name='{field_name}']")[0] + options = field.get("options") or "" + self.assertIn("'link'", options, "the link row is what a Many2many shows") + self.assertIn("'create'", options, "create must be denied too") + self.assertNotIn("'unlink'", options, "removing a method must stay possible") + self.assertEqual(field.xpath("./list")[0].get("create"), "0") + + def test_edit_is_only_offered_when_there_is_one_method(self): + """One button cannot sensibly open two, and it used to open the first.""" + arch = self._arch() + for category in CONVERTED: + count = f"{category}_manager_count" + with self.subTest(category=category): + edit = arch.xpath(f"//button[@name='action_configure_{category}'][contains(@class,'btn-primary')]")[0] + self.assertIn(count, edit.get("invisible") or "") + + def test_notifications_is_a_card_like_the_rest(self): + """It was the last section still rendered as a bare group.""" + headings = [h.strip() for h in self._arch().xpath("//div[contains(@class, 'card-header')]//h5/text()")] + + self.assertIn("Notifications", headings, f"found {headings}") diff --git a/spp_programs/views/program_config_cards_view.xml b/spp_programs/views/program_config_cards_view.xml index c11f3bfd3..fe05b97fd 100644 --- a/spp_programs/views/program_config_cards_view.xml +++ b/spp_programs/views/program_config_cards_view.xml @@ -20,6 +20,25 @@ Replaces the technical manager configuration with intuitive sections. string="Configuration" groups="spp_security.group_spp_admin,spp_programs.group_programs_manager,spp_programs.group_programs_validator" > + @@ -47,24 +66,46 @@ Replaces the technical manager configuration with intuitive sections. > Configured + +
+
+ + No eligibility method configured — click Add above to choose who qualifies for this program. +
- + - +
+
+ + No entitlement type configured — click Add above to choose what beneficiaries receive. +
- + - +
+
+ + No schedule configured — click Add above to choose how often this program runs. +
- + - - + +
@@ -405,24 +491,23 @@ Replaces the technical manager configuration with intuitive sections. placeholder="No compliance rule configured yet — click Edit above." /> - + - + @@ -516,24 +608,23 @@ Replaces the technical manager configuration with intuitive sections. placeholder="No payment processing configured yet — click Edit above." /> - + - + + + + + +
+ + + + + +
+ + No outgoing mail server is configured. Ask your administrator to set one up under + Settings → Technical → Email → Outgoing Mail Servers before enabling email + notifications. +
+
+ + No notification channel configured. It is optional — click Add above to message beneficiaries. +
+ + - - + +
+ - - + + + + + +
- Send SMS or other notifications to beneficiaries. -
- - -
- - No outgoing mail server is configured. Ask your administrator to set one up under - Settings → Technical → Email → Outgoing Mail Servers before enabling email - notifications. + Check for duplicate beneficiaries by phone, ID, etc.
diff --git a/spp_programs/wizard/__init__.py b/spp_programs/wizard/__init__.py index 333f0b72a..d84c205ef 100644 --- a/spp_programs/wizard/__init__.py +++ b/spp_programs/wizard/__init__.py @@ -15,3 +15,4 @@ from . import enrollment_wizard from . import exit_membership_wizard from . import prepare_entitlement_confirm_wizard +from . import manager_setup_wizard diff --git a/spp_programs/wizard/manager_setup_wizard.py b/spp_programs/wizard/manager_setup_wizard.py new file mode 100644 index 000000000..193696c51 --- /dev/null +++ b/spp_programs/wizard/manager_setup_wizard.py @@ -0,0 +1,196 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""One dialog for adding any program configuration method (OP#1172). + +Every card on a program's Configuration tab used to be filled in the same way: +an inline list with a `manager_ref_id` Reference field and an "Add a line" row. +That control asks the user to pick a *model* and then find or create a record +of it, and both halves of it leak — the Reference picker and, on the Many2many +cards, the link dialog behind "Add a line" both list managers belonging to +other programs, which silently wires another program's configuration into this +one. + +This wizard asks the two questions that actually matter — which method, and +what to call it — and creates a record that belongs to this program only. The +methods on offer come from the wrapper's ``_selection_manager_ref_id()``, so a +module that adds a method (spp_program_geofence adds an eligibility one) shows +up here without touching this file. +""" + +from odoo import _, api, fields, models +from odoo.exceptions import UserError + +from ..models.constants import MANAGER_CATEGORIES +from ..models.program_manager_ui import MANAGER_TYPE_INFO + + +class ManagerSetupWizard(models.TransientModel): + _name = "spp.manager.setup.wizard" + _description = "Add a Configuration Method" + + program_id = fields.Many2one( + "spp.program", + required=True, + readonly=True, + ) + category = fields.Selection( + selection=[(key, info["label"]) for key, info in MANAGER_CATEGORIES.items()], + required=True, + readonly=True, + help="Which card on the Configuration tab this method belongs to.", + ) + method = fields.Selection( + selection="_selection_method", + string="Method", + required=True, + help="How this part of the program is handled. Each method can be added once per program.", + ) + method_description = fields.Char(compute="_compute_method_description") + # Drives whether the Method question is asked at all: a category with one + # method has nothing to choose, and a radio list of one is just noise. + method_count = fields.Integer(compute="_compute_method_count") + name = fields.Char( + string="Name", + required=True, + help="Shown on the program's configuration page.", + ) + + # ------------------------------------------------------------------ + # the methods on offer + # ------------------------------------------------------------------ + + @api.model + def _methods_for_category(self, category): + """The concrete manager models a category can offer, as selection pairs. + + Read from the wrapper rather than from a list here so that methods + added by other modules are included, and so that a method whose module + has been uninstalled drops out instead of raising when it is picked. + MANAGER_TYPE_INFO only supplies nicer wording where it has some. + """ + info = MANAGER_CATEGORIES.get(category) + if not info or info["wrapper"] not in self.env: + return [] + methods = [] + for model, label in self.env[info["wrapper"]]._selection_manager_ref_id(): + if model in self.env: + methods.append((model, MANAGER_TYPE_INFO.get(model, {}).get("name") or label)) + return methods + + @api.model + def _selection_method(self): + """Selection values for the Method field. + + A Selection cannot depend on another field's value, so the category + comes from the context the Add button opens this dialog with. + """ + return self._methods_for_category(self.env.context.get("default_category")) + + @api.depends("method") + def _compute_method_description(self): + for wizard in self: + wizard.method_description = MANAGER_TYPE_INFO.get(wizard.method, {}).get("description", "") + + @api.depends("category") + def _compute_method_count(self): + for wizard in self: + wizard.method_count = len(self._methods_for_category(wizard.category)) + + @api.onchange("method") + def _onchange_method_suggests_a_name(self): + """Pre-fill the name from the method, so naming is one keystroke. + + Only while the user has not typed their own, and only replacing a + suggestion we made ourselves. + """ + labels = dict(self._methods_for_category(self.category)) + if not self.name or self.name in set(labels.values()): + self.name = labels.get(self.method, "") + + # ------------------------------------------------------------------ + # creating the method + # ------------------------------------------------------------------ + + def _sweep_removed_methods(self): + """Delete the methods the card no longer shows. + + Most of these program fields are Many2many, so the ✕ on a row removes + the *relation* and leaves the manager behind with its ``program_id`` + still pointing here. Those leftovers never run — a program is + configured through its own field, not through the managers' + ``program_id`` — but they used to make the duplicate check below refuse + a method the card no longer showed (OP#1171). + + Only managers that no program links are swept: on a Many2many, one this + program created but another program links is that program's method now, + not garbage. + """ + self.ensure_one() + field = MANAGER_CATEGORIES[self.category]["field"] + wrapper = MANAGER_CATEGORIES[self.category]["wrapper"] + removed = self.env[wrapper].search([("program_id", "=", self.program_id.id)]) - self.program_id[field] + if not removed: + return + linked = self.env["spp.program"].search([(field, "in", removed.ids)]) + for leftover in removed - linked[field]: + # The concrete record owns the wrapper: spp.manager.source.mixin's + # unlink() takes the wrapper with it. manager_ref_id is a Reference, + # so it carries no foreign key and can outlive what it points at — + # unlinking that blind would raise MissingError on the Add button. + concrete = leftover.manager_ref_id + ((concrete and concrete.exists()) or leftover).unlink() + + def action_create_manager(self): + """Create the concrete manager; the wrapper follows automatically. + + ``spp.manager.source.mixin.create`` builds the wrapper when it sees + ``_spp_wrapper_model`` in the context, so this creates one record and + gets both — and dismissing the dialog leaves nothing behind (#953). + + ``_spp_program_m2m_field`` matters as much as the wrapper model on the + Many2many cards: unlike a One2many they do not resolve from the + wrapper's ``program_id``, so without it the manager is created and the + program never picks it up — the card keeps saying nothing is configured + and the method never runs. + """ + self.ensure_one() + self._sweep_removed_methods() + + field = MANAGER_CATEGORIES[self.category]["field"] + configured = self.program_id[field].filtered( + lambda wrapper: wrapper.manager_ref_id and wrapper.manager_ref_id._name == self.method + ) + if configured: + raise UserError( + _("This program already has a %(method)s %(category)s.") + % { + "method": dict(self._methods_for_category(self.category)).get(self.method, self.method), + "category": MANAGER_CATEGORIES[self.category]["label"].lower(), + } + ) + + context = { + "default_program_id": self.program_id.id, + "_spp_wrapper_model": MANAGER_CATEGORIES[self.category]["wrapper"], + } + if self.env["spp.program"]._fields[field].type == "many2many": + context["_spp_program_m2m_field"] = field + concrete = ( + self.env[self.method] + .with_context(**context) + .create( + { + "name": self.name, + "program_id": self.program_id.id, + } + ) + ) + wrapper = self.env[MANAGER_CATEGORIES[self.category]["wrapper"]].search( + [("manager_ref_id", "=", f"{concrete._name},{concrete.id}")], + limit=1, + ) + if wrapper: + # Land on the method's own form rather than back on the card with + # something unconfigured and a cog to discover. Eligibility filters, + # entitlement amounts and compliance criteria all live there. + return wrapper.open_manager_form(title=MANAGER_CATEGORIES[self.category]["label"]) + return {"type": "ir.actions.act_window_close"} diff --git a/spp_programs/wizard/manager_setup_wizard.xml b/spp_programs/wizard/manager_setup_wizard.xml new file mode 100644 index 000000000..8acf617a7 --- /dev/null +++ b/spp_programs/wizard/manager_setup_wizard.xml @@ -0,0 +1,59 @@ + + + + + spp.manager.setup.wizard.form + spp.manager.setup.wizard + +
+ + + + + + +
+ + +
+ + +
+
+ + +
+ From 0be86cccbccfc2282dff7413f15e4aa32b73803c Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Mon, 24 Aug 2026 11:39:15 +0800 Subject: [PATCH 2/5] fix(spp_programs): say why a program takes only one entitlement method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- spp_programs/models/constants.py | 9 +++++ .../tests/test_manager_setup_wizard.py | 37 +++++++++++++++++++ spp_programs/wizard/manager_setup_wizard.py | 30 +++++++++++---- 3 files changed, 68 insertions(+), 8 deletions(-) diff --git a/spp_programs/models/constants.py b/spp_programs/models/constants.py index f8c0bf23e..16ba700d1 100644 --- a/spp_programs/models/constants.py +++ b/spp_programs/models/constants.py @@ -61,6 +61,15 @@ "field": "entitlement_manager_ids", "wrapper": "spp.program.entitlement.manager", "label": "Entitlement Type", + # One per program, and not by choice of this dialog: spp.program's + # 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. QA asked for several of the same kind (OP#1172 round 1); that + # needs the entitlement engine to iterate managers first, so the dialog + # says what the program can actually do rather than accepting a second + # method the cycle would then choke on. + "single_manager": True, }, "cycle": { "field": "cycle_manager_ids", diff --git a/spp_programs/tests/test_manager_setup_wizard.py b/spp_programs/tests/test_manager_setup_wizard.py index 45b4205cc..a843cb4be 100644 --- a/spp_programs/tests/test_manager_setup_wizard.py +++ b/spp_programs/tests/test_manager_setup_wizard.py @@ -143,6 +143,43 @@ def test_the_dead_end_helper_now_opens_the_dialog(self): self.assertEqual(action.get("type"), "ir.actions.act_window") self.assertEqual(action.get("res_model"), "spp.manager.setup.wizard") + # ------------------------------------------------------------------ + # entitlements: one per program, for now (OP#1172 round 1) + # ------------------------------------------------------------------ + + def test_a_second_entitlement_method_is_refused_with_the_real_reason(self): + """QA asked for several cash entitlements; the engine allows one. + + 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. Accepting a second here would create a program every cycle + operation then failed on, so the dialog refuses and says why. + + This test pins today's limit rather than blessing it: when the engine + learns to iterate entitlement managers, this is the test that changes. + """ + program = self.env["spp.program"].create({"name": "One Entitlement [TEST]"}) + self._add("entitlement", method="spp.program.entitlement.manager.cash", name="First cash", program=program) + + with self.assertRaises(UserError) as cm: + self._add("entitlement", method="spp.program.entitlement.manager.cash", name="Second cash", program=program) + self.assertIn("supports one", str(cm.exception)) + + with self.assertRaises(UserError): + self._add("entitlement", method="spp.program.entitlement.manager.inkind", name="Goods", program=program) + + self.assertEqual(len(program.entitlement_manager_ids), 1, "the program keeps the method it had") + + def test_the_engine_still_reaches_for_exactly_one_entitlement_manager(self): + """Guards the reason above: if this stops being true, revisit the limit.""" + program = self.env["spp.program"].create({"name": "Engine Assumption [TEST]"}) + self._add("entitlement", method="spp.program.entitlement.manager.cash", name="Cash", program=program) + + self.assertTrue(program.get_manager(program.MANAGER_ENTITLEMENT)) + with self.assertRaises(NotImplementedError): + program.get_managers(program.MANAGER_ENTITLEMENT) + # ------------------------------------------------------------------ # isolation # ------------------------------------------------------------------ diff --git a/spp_programs/wizard/manager_setup_wizard.py b/spp_programs/wizard/manager_setup_wizard.py index 193696c51..9bb209dbc 100644 --- a/spp_programs/wizard/manager_setup_wizard.py +++ b/spp_programs/wizard/manager_setup_wizard.py @@ -155,19 +155,33 @@ def action_create_manager(self): self.ensure_one() self._sweep_removed_methods() - field = MANAGER_CATEGORIES[self.category]["field"] - configured = self.program_id[field].filtered( - lambda wrapper: wrapper.manager_ref_id and wrapper.manager_ref_id._name == self.method - ) - if configured: + info = MANAGER_CATEGORIES[self.category] + field = info["field"] + labels = dict(self._methods_for_category(self.category)) + configured = self.program_id[field].filtered(lambda wrapper: wrapper.manager_ref_id) + + if info.get("single_manager") and configured: + # Say what the limit actually is. This is not a duplicate rule: a + # program supports one entitlement method whatever its kind, because + # the cycle machinery reaches for exactly one (OP#1172 round 1). raise UserError( - _("This program already has a %(method)s %(category)s.") + _( + "%(program)s already has a %(category)s: %(existing)s. A program supports one " + "for now — change that one, or remove it before adding another." + ) % { - "method": dict(self._methods_for_category(self.category)).get(self.method, self.method), - "category": MANAGER_CATEGORIES[self.category]["label"].lower(), + "program": self.program_id.display_name, + "category": info["label"].lower(), + "existing": ", ".join(configured.mapped("display_name")), } ) + if configured.filtered(lambda wrapper: wrapper.manager_ref_id._name == self.method): + raise UserError( + _("This program already has a %(method)s %(category)s.") + % {"method": labels.get(self.method, self.method), "category": info["label"].lower()} + ) + context = { "default_program_id": self.program_id.id, "_spp_wrapper_model": MANAGER_CATEGORIES[self.category]["wrapper"], From 6c8b91c9fa4c45774c28ed64abb5ea992f62245e Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Mon, 24 Aug 2026 11:48:49 +0800 Subject: [PATCH 3/5] fix(spp_programs): let a cash entitlement line have a base amount again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/test_entitlement_amount_cel.py | 60 +++++++++++++++++++ .../cel/entitlement_amount_cel_views.xml | 38 ++++++++++-- 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/spp_programs/tests/test_entitlement_amount_cel.py b/spp_programs/tests/test_entitlement_amount_cel.py index b665f8546..f105d0f81 100644 --- a/spp_programs/tests/test_entitlement_amount_cel.py +++ b/spp_programs/tests/test_entitlement_amount_cel.py @@ -1,6 +1,8 @@ import logging from datetime import timedelta +from lxml import etree + from odoo import fields from odoo.exceptions import UserError, ValidationError from odoo.tests import TransactionCase, tagged @@ -369,3 +371,61 @@ def test_18_security_safe_field_access(self): result = item._calculate_cel_amount(self.beneficiary1) self.assertEqual(result, 100.0) + + +@tagged("post_install", "-at_install") +class TestEntitlementAmountCELForm(TransactionCase): + """OP#1172 round 1: what the amount item form must offer. + + QA could not add a cash entitlement line: the form had no field for the base + amount, and a formula built from the widget's symbol browser failed to + compile. Both came from this view rather than from the entitlement logic. + """ + + def _item_form(self): + arch = etree.fromstring( + self.env["spp.program.entitlement.manager.cash"].get_view( + self.env.ref("spp_programs.view_entitlement_manager_cash_form").id, "form" + )["arch"] + ) + forms = arch.xpath("//field[@name='entitlement_item_ids']/form") + self.assertTrue(forms, "the items list should still open a form") + return forms[0] + + def test_the_base_amount_stays_on_the_form(self): + """Formulas are documented to build on it, so it must be settable. + + The CEL view used to hide it outright, which left base_amount + permanently unset and every documented "base_amount * ..." formula + unusable. + """ + amounts = [f for f in self._item_form().iter("field") if f.get("name") == "amount"] + + self.assertTrue(amounts, "the item form should offer the base amount") + self.assertNotEqual(amounts[0].get("invisible"), "1", "hiding it is what QA reported") + + def test_the_formula_field_does_not_advertise_the_wrong_symbols(self): + """The evaluator receives `me` and `base_amount`, not the entitlements profile. + + _validate_cel_expression and _calculate_cel_amount build their own + context, so a symbol browser listing spp.entitlement's fields offers + names that never arrive — `r.birthdate` compiles against + spp.entitlement and fails. + """ + expressions = [f for f in self._item_form().iter("field") if f.get("name") == "amount_cel_expression"] + + self.assertTrue(expressions, "the formula field should be on the form") + self.assertEqual(expressions[0].get("show_symbol_browser"), "false") + self.assertIsNone(expressions[0].get("cel_profile"), "no profile matches this evaluator yet") + + def test_the_documented_vocabulary_actually_validates(self): + """A plain number and a base_amount formula both pass the model's own check.""" + manager = self.env["spp.program.entitlement.manager.cash"].create( + {"name": "CEL Form [TEST]", "program_id": self.env["spp.program"].create({"name": "CEL Form P [TEST]"}).id} + ) + for expression in ("500", "base_amount * 1.1", "me.household_size * 100"): + with self.subTest(expression=expression): + item = self.env["spp.program.entitlement.manager.cash.item"].create( + {"entitlement_id": manager.id, "amount": 100.0, "amount_cel_expression": expression} + ) + item._validate_cel_expression() diff --git a/spp_programs/views/cel/entitlement_amount_cel_views.xml b/spp_programs/views/cel/entitlement_amount_cel_views.xml index cada7d545..85efc3051 100644 --- a/spp_programs/views/cel/entitlement_amount_cel_views.xml +++ b/spp_programs/views/cel/entitlement_amount_cel_views.xml @@ -34,13 +34,25 @@ - + - 1 + Base Amount 0 + Optional. Formulas can build on this value as base_amount. @@ -49,14 +61,30 @@ position="after" > + Date: Tue, 25 Aug 2026 11:41:28 +0800 Subject: [PATCH 4/5] fix(spp_programs): stop requiring a formula on the entitlement amount item MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/test_entitlement_amount_cel.py | 54 ++++++++++++++++++- .../cel/entitlement_amount_cel_views.xml | 17 ++++-- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/spp_programs/tests/test_entitlement_amount_cel.py b/spp_programs/tests/test_entitlement_amount_cel.py index f105d0f81..d796403f6 100644 --- a/spp_programs/tests/test_entitlement_amount_cel.py +++ b/spp_programs/tests/test_entitlement_amount_cel.py @@ -164,7 +164,15 @@ def test_06_cel_validation_syntax_error(self): ) def test_07_cel_calculation_empty_formula(self): - """Test that calculating with empty formula raises error.""" + """Calculating from an empty formula raises. + + Nothing reaches this with an empty expression: the manager that + actually runs (CashEntitlementManagerCEL.prepare_entitlements in + entitlement_condition_cel.py) tests amount_cel_expression first, and + the preview compute returns early. This pins the guard, not a path + users can take -- an empty formula is handled long before here, see + test_07b. + """ item = self.env["spp.program.entitlement.manager.cash.item"].create( { "entitlement_id": self.entitlement_manager.id, @@ -174,6 +182,35 @@ def test_07_cel_calculation_empty_formula(self): with self.assertRaises(UserError): item._calculate_cel_amount(self.beneficiary1) + def test_07b_prepare_entitlements_without_a_formula(self): + """A typed Base Amount with no formula must still pay out. + + QA round 2: "CEL expression field should be optional FOR NOW. just to + let user define the amount." The form stopped requiring a formula; this + pins the behaviour behind that, so the field cannot quietly become + required again without a failure here. + """ + self.env["spp.program.entitlement.manager.cash.item"].create( + { + "entitlement_id": self.entitlement_manager.id, + "amount": 425.0, + "amount_cel_expression": False, + } + ) + + self.entitlement_manager.prepare_entitlements(self.cycle, self.membership1 | self.membership2) + + entitlements = self.env["spp.entitlement"].search( + [ + ("cycle_id", "=", self.cycle.id), + ("partner_id", "in", [self.beneficiary1.id, self.beneficiary2.id]), + ] + ) + + self.assertEqual(len(entitlements), 2, "a formula-less item must still pay every beneficiary") + for ent in entitlements: + self.assertEqual(ent.initial_amount, 425.0) + def test_08_cel_non_numeric_result(self): """Test that non-numeric results raise an error.""" item = self.env["spp.program.entitlement.manager.cash.item"].create( @@ -404,6 +441,21 @@ def test_the_base_amount_stays_on_the_form(self): self.assertTrue(amounts, "the item form should offer the base amount") self.assertNotEqual(amounts[0].get("invisible"), "1", "hiding it is what QA reported") + def test_the_formula_is_not_required(self): + """A fixed sum should not oblige anyone to write "500" as a formula. + + QA round 2: "CEL expression field should be optional FOR NOW. just to + let user define the amount." + """ + formulas = [f for f in self._item_form().iter("field") if f.get("name") == "amount_cel_expression"] + + self.assertTrue(formulas, "the item form should still offer the formula") + self.assertNotEqual( + formulas[0].get("required"), + "1", + "requiring a formula is what QA reported in round 2", + ) + def test_the_formula_field_does_not_advertise_the_wrong_symbols(self): """The evaluator receives `me` and `base_amount`, not the entitlements profile. diff --git a/spp_programs/views/cel/entitlement_amount_cel_views.xml b/spp_programs/views/cel/entitlement_amount_cel_views.xml index 85efc3051..dd3e693f9 100644 --- a/spp_programs/views/cel/entitlement_amount_cel_views.xml +++ b/spp_programs/views/cel/entitlement_amount_cel_views.xml @@ -42,7 +42,8 @@ unconditionally — as this view used to — left no way to set the value those formulas multiply, which is what QA reported as "I don't have a field to define the base amount" (OP#1172 - round 1). Not required: a formula may compute the whole amount. + round 1). Either field may stand alone: a formula may compute the + whole amount, or the Base Amount may be paid with no formula. --> Date: Fri, 28 Aug 2026 15:50:59 +0800 Subject: [PATCH 5/5] fix(spp_programs): declare every category the engine caps at one manager 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. --- spp_programs/models/constants.py | 21 ++++++ .../tests/test_manager_setup_wizard.py | 26 ++++++++ .../views/program_config_cards_view.xml | 66 ++++++++++++------- 3 files changed, 89 insertions(+), 24 deletions(-) diff --git a/spp_programs/models/constants.py b/spp_programs/models/constants.py index 1be1cd9bf..1d40303ec 100644 --- a/spp_programs/models/constants.py +++ b/spp_programs/models/constants.py @@ -87,6 +87,13 @@ "field": "cycle_manager_ids", "wrapper": "spp.cycle.manager", "label": "Cycle Schedule", + # Capped at one by spp.program's check_managers_limit, same as + # entitlement. Unreachable through the dialog while this category has a + # single concrete method in-repo -- the already-configured check fires + # first -- but a module registering a second one (as spp_program_geofence + # does for eligibility) would otherwise get the constraint's after-the-fact + # wording, which is the experience this dialog exists to remove. + "single_manager": True, }, "compliance": { "field": "compliance_manager_ids", @@ -97,6 +104,13 @@ "field": "payment_manager_ids", "wrapper": "spp.program.payment.manager", "label": "Payment Method", + # Capped at one by spp.program's check_managers_limit, same as + # entitlement. Unreachable through the dialog while this category has a + # single concrete method in-repo -- the already-configured check fires + # first -- but a module registering a second one (as spp_program_geofence + # does for eligibility) would otherwise get the constraint's after-the-fact + # wording, which is the experience this dialog exists to remove. + "single_manager": True, }, "deduplication": { "field": "deduplication_manager_ids", @@ -112,5 +126,12 @@ "field": "program_manager_ids", "wrapper": "spp.program.manager", "label": "Program Manager", + # Capped at one by spp.program's check_managers_limit, same as + # entitlement. Unreachable through the dialog while this category has a + # single concrete method in-repo -- the already-configured check fires + # first -- but a module registering a second one (as spp_program_geofence + # does for eligibility) would otherwise get the constraint's after-the-fact + # wording, which is the experience this dialog exists to remove. + "single_manager": True, }, } diff --git a/spp_programs/tests/test_manager_setup_wizard.py b/spp_programs/tests/test_manager_setup_wizard.py index a843cb4be..52a84d095 100644 --- a/spp_programs/tests/test_manager_setup_wizard.py +++ b/spp_programs/tests/test_manager_setup_wizard.py @@ -306,3 +306,29 @@ def test_notifications_is_a_card_like_the_rest(self): headings = [h.strip() for h in self._arch().xpath("//div[contains(@class, 'card-header')]//h5/text()")] self.assertIn("Notifications", headings, f"found {headings}") + + def test_every_capped_category_is_declared_capped(self): + """MANAGER_CATEGORIES must agree with what the engine enforces. + + check_managers_limit refuses a second entitlement, cycle, payment or + program manager. A category the engine caps but the dialog does not + know about accepts the second method and lets the constraint refuse it + afterwards, with wording this dialog exists to replace. Unreachable + while each of those categories has one concrete method in-repo, so this + pins the agreement rather than a behaviour anyone can trigger today. + """ + import inspect + + from odoo.addons.spp_programs.models import constants + + source = inspect.getsource(type(self.env["spp.program"]).check_managers_limit) + capped_by_engine = { + key for key, info in constants.MANAGER_CATEGORIES.items() if f"len(record.{info['field']}) > 1" in source + } + declared = {key for key, info in constants.MANAGER_CATEGORIES.items() if info.get("single_manager")} + + self.assertEqual( + declared, + capped_by_engine, + "single_manager and check_managers_limit disagree about which categories take only one manager", + ) diff --git a/spp_programs/views/program_config_cards_view.xml b/spp_programs/views/program_config_cards_view.xml index 0029be405..bb91e74b3 100644 --- a/spp_programs/views/program_config_cards_view.xml +++ b/spp_programs/views/program_config_cards_view.xml @@ -67,10 +67,13 @@ Replaces the technical manager configuration with intuitive sections. Configured