From 2c0761ec04252c8988cc64063ad8890ac6ea6463 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Fri, 28 Aug 2026 17:13:50 +0700 Subject: [PATCH 1/2] fix(change_request): make group-scope conflict rules resolve household members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _get_group_member_ids traversed spp.group.membership through individual_id and group_id, but the model names its many2ones individual and group. Resolving a household's members therefore raised KeyError/AttributeError instead of returning them: a change request whose type carried an active group-scope conflict rule crashed on creation for any group registrant, and for any individual registrant with a live membership — exactly the registrants the rule exists to check. The one existing test called the method with a member-less individual, the single shape that happened to work. Group-scope resolution is now tested with real memberships in both directions, including that ended memberships are excluded. Fixes #476 --- spp_change_request_v2/README.rst | 48 ++++++++---- spp_change_request_v2/__manifest__.py | 2 +- .../models/conflict_mixin.py | 6 +- spp_change_request_v2/readme/HISTORY.md | 4 + .../static/description/index.html | 65 ++++++++++------ .../tests/test_conflict_detection_extended.py | 78 +++++++++++++++---- 6 files changed, 146 insertions(+), 57 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 754b51a35..4bcc6aa34 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -752,22 +752,22 @@ Methods available for override on detail models (all inherited from Related fields available on all detail models (from ``spp.cr.detail.base``): -+--------------------------+-----------+------------------------------------------------------------+ -| Field | Type | Source | -+==========================+===========+============================================================+ -| ``change_request_id`` | Many2one | Direct link to parent CR | -+--------------------------+-----------+------------------------------------------------------------+ -| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``approval_state`` | Selection | ``change_request_id.approval_state`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``field_to_modify`` | Selection | Dynamic field selector (populated by | -| | | ``_get_field_to_modify_selection``) | -+--------------------------+-----------+------------------------------------------------------------+ ++----------------------------+-----------+------------------------------------------------------------+ +| Field | Type | Source | ++============================+===========+============================================================+ +| ``change_request_id`` | Many2one | Direct link to parent CR | ++----------------------------+-----------+------------------------------------------------------------+ +| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``approval_state`` | Selection | ``change_request_id.approval_state`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``field_to_modify`` | Selection | Dynamic field selector (populated by | +| | | ``_get_field_to_modify_selection``) | ++----------------------------+-----------+------------------------------------------------------------+ CR Type Fields Reference ~~~~~~~~~~~~~~~~~~~~~~~~ @@ -853,6 +853,22 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.1.14 +~~~~~~~~~~~ + +- fix(change_request): group-scope conflict rules work again. + ``_get_group_member_ids`` traversed ``spp.group.membership`` records + through ``individual_id`` and ``group_id``, but that model names its + many2ones ``individual`` and ``group`` — so resolving a household's + members raised ``KeyError``/``AttributeError`` instead of returning + them. A change request whose type carried an active group-scope + conflict rule crashed on creation for any group registrant, and for + any individual registrant with a live membership — exactly the + registrants the rule exists to check. The one existing test called the + method with a member-less individual, the single shape that happened + to work; group-scope detection is now tested with real memberships in + both directions, including that ended memberships are excluded. + 19.0.3.1.13 ~~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 50a685622..a6484a53b 100644 --- a/spp_change_request_v2/__manifest__.py +++ b/spp_change_request_v2/__manifest__.py @@ -1,6 +1,6 @@ { "name": "OpenSPP Change Request V2", - "version": "19.0.3.1.13", + "version": "19.0.3.1.14", "sequence": 50, "category": "OpenSPP", "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention", diff --git a/spp_change_request_v2/models/conflict_mixin.py b/spp_change_request_v2/models/conflict_mixin.py index 3bc622e7f..96b368f40 100644 --- a/spp_change_request_v2/models/conflict_mixin.py +++ b/spp_change_request_v2/models/conflict_mixin.py @@ -279,17 +279,17 @@ def _get_group_member_ids(self): if registrant.is_group: if hasattr(registrant, "group_membership_ids"): member_ids.extend( - registrant.group_membership_ids.filtered(lambda m: not m.ended_date).mapped("individual_id.id") + registrant.group_membership_ids.filtered(lambda m: not m.ended_date).mapped("individual.id") ) # If registrant is an individual, find their groups and group members else: if hasattr(registrant, "individual_membership_ids"): for membership in registrant.individual_membership_ids.filtered(lambda m: not m.ended_date): - group = membership.group_id + group = membership.group member_ids.append(group.id) member_ids.extend( - group.group_membership_ids.filtered(lambda m: not m.ended_date).mapped("individual_id.id") + group.group_membership_ids.filtered(lambda m: not m.ended_date).mapped("individual.id") ) return list(set(member_ids)) diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index f90bf8543..5cf9dd00a 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.3.1.14 + +- fix(change_request): group-scope conflict rules work again. `_get_group_member_ids` traversed `spp.group.membership` records through `individual_id` and `group_id`, but that model names its many2ones `individual` and `group` — so resolving a household's members raised `KeyError`/`AttributeError` instead of returning them. A change request whose type carried an active group-scope conflict rule crashed on creation for any group registrant, and for any individual registrant with a live membership — exactly the registrants the rule exists to check. The one existing test called the method with a member-less individual, the single shape that happened to work; group-scope detection is now tested with real memberships in both directions, including that ended memberships are excluded. + ### 19.0.3.1.13 - fix(change_request): a selectable field on a dynamic-approval type may now be applied through more than one mapping. Apply is narrowed to the field a request was routed and approved on, matched against the mapping's `source_field` — which assumed every selectable value is a physical source field. They need not be: a name may be offered as one choice but stored as separate components, so one selectable value legitimately drives several mappings, and matching on `source_field` alone matched none of them, applying nothing at all. A mapping can now declare the selectable value it serves via `routing_field`, defaulting to `source_field`, so existing configurations are unchanged. Narrowing still holds — a mapping belonging to another routing key is still not applied. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 281deb2db..ad655e4f9 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1160,9 +1160,9 @@

Methods Reference

spp.cr.detail.base):

-+-+ @@ -1339,6 +1339,23 @@

Changelog

+

19.0.3.1.14

+ +
+

19.0.3.1.13

-
+

19.0.3.1.12

  • fix(change_request): auto-apply-on-approve runs through the public @@ -1370,7 +1387,7 @@

    19.0.3.1.12

    the applying user is still recorded as the approver.
-
+

19.0.3.1.11

  • fix(change_request): field-mapping transform expressions are evaluated @@ -1409,7 +1426,7 @@

    19.0.3.1.11

    the full traceback is logged only at DEBUG.
-
+

19.0.3.1.10

  • fix(security): conflict and duplicate detection now decide whether a @@ -1449,7 +1466,7 @@

    19.0.3.1.10

    configured mapping.
-
+

19.0.3.1.9

  • fix(security): duplicate detection now scores the fields both change @@ -1466,7 +1483,7 @@

    19.0.3.1.9

    requester-writable selected_field_name / field_to_modify.
-
+

19.0.3.1.8

  • fix(security): scope the Create-Group member wizards to the parent @@ -1484,7 +1501,7 @@

    19.0.3.1.8

    access-control entry grants.
-
+

19.0.3.1.7

  • fix(security): require change-request manager rights to apply a change @@ -1499,7 +1516,7 @@

    19.0.3.1.7

    endpoint.
-
+

19.0.3.1.6

  • fix(security): derive conflict and duplicate detection from the change @@ -1513,7 +1530,7 @@

    19.0.3.1.6

    an empty one, so detection cannot silently disable itself.
-
+

19.0.3.1.5

  • fix(security): scope the CR Requestor, Local Validator and HQ @@ -1525,7 +1542,7 @@

    19.0.3.1.5

    are noupdate.
-
+

19.0.3.1.4

  • fix(security): add ownership and area record rules to every concrete @@ -1542,7 +1559,7 @@

    19.0.3.1.4

    unrestricted delete their access-control entries grant.
-
+

19.0.3.1.3

  • fix(security): route and apply the same single field for @@ -1555,7 +1572,7 @@

    19.0.3.1.3

    the routing selector.
-
+

19.0.3.1.2

  • fix(change_request_v2): adding an ID now looks for a live one of that @@ -1564,7 +1581,7 @@

    19.0.3.1.2

    (#1136)
-
+

19.0.3.1.1

  • fix(change_request): enforce the (cr_type_id, reason) uniqueness @@ -1578,7 +1595,7 @@

    19.0.3.1.1

    applied) so the constraint applies cleanly on upgrade.
-
+

19.0.3.1.0

  • revert(change_request): restore the create-a-new-individual Add @@ -1596,7 +1613,7 @@

    19.0.3.1.0

    not restored here; reinstate separately if needed.
-
+

19.0.3.0.0

  • feat(change_request): redesign the group/membership CR flows (#242) — @@ -1618,7 +1635,7 @@

    19.0.3.0.0

    must adapt (see #1133).
-
+

19.0.2.0.8

  • fix(views): disable inline creation of CR document types on the Change @@ -1629,7 +1646,7 @@

    19.0.2.0.8

    Documents” modal (missing Name field) that blocked saving (#1125)
-
+

19.0.2.0.7

  • fix(security): align CR Requestor / CR Local Validator / CR HQ @@ -1641,7 +1658,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1656,7 +1673,7 @@

    19.0.2.0.6

    list so row-click goes through the stage router.
-
+

19.0.2.0.5

  • fix(security): add a global ir.rule on spp.change.request that @@ -1669,27 +1686,27 @@

    19.0.2.0.5

    roles).
-
+

19.0.2.0.3

  • fix: add HTML escaping to all computed Html fields with sanitize=False to prevent stored XSS (#50)
-
+

19.0.2.0.2

  • fix: fix batch approval wizard line deletion (#130)
-
+

19.0.2.0.1

  • fix: skip field types before getattr and isolate detail prefetch (#129)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_change_request_v2/tests/test_conflict_detection_extended.py b/spp_change_request_v2/tests/test_conflict_detection_extended.py index 0cd425722..a109e786d 100644 --- a/spp_change_request_v2/tests/test_conflict_detection_extended.py +++ b/spp_change_request_v2/tests/test_conflict_detection_extended.py @@ -10,7 +10,7 @@ - Spec scenarios """ -from odoo import Command +from odoo import Command, fields from odoo.exceptions import UserError, ValidationError from odoo.tests import TransactionCase @@ -493,17 +493,15 @@ def setUpClass(cls): } ) - def test_group_scope_same_household_members(self): - """Test group scope detects conflicts for household members.""" - # Create household with members - self.env["res.partner"].create( + def _create_household(self): + """Create a household with two member individuals and return all three.""" + group = self.env["res.partner"].create( { "name": "Test Household", "is_registrant": True, "is_group": True, } ) - individual1 = self.env["res.partner"].create( { "name": "Member 1", @@ -511,20 +509,33 @@ def test_group_scope_same_household_members(self): "is_group": False, } ) - - self.env["res.partner"].create( + individual2 = self.env["res.partner"].create( { "name": "Member 2", "is_registrant": True, "is_group": False, } ) + self.env["spp.group.membership"].create( + [ + {"group": group.id, "individual": individual1.id}, + {"group": group.id, "individual": individual2.id}, + ] + ) + return group, individual1, individual2 - # Add members to household (if membership model exists) - # This is implementation-dependent - # For now, test the _get_group_member_ids method directly + def test_group_scope_same_household_members(self): + """Test group scope detects conflicts for household members.""" + group, individual1, individual2 = self._create_household() - # Create CR for individual1 + # Create CR for individual2 first, then for individual1: the group-scope + # rule must flag the second CR because both registrants share a household. + self.env["spp.change.request"].create( + { + "request_type_id": self.cr_type.id, + "registrant_id": individual2.id, + } + ) cr1 = self.env["spp.change.request"].create( { "request_type_id": self.cr_type.id, @@ -532,9 +543,50 @@ def test_group_scope_same_household_members(self): } ) - # Test _get_group_member_ids returns expected members + # _get_group_member_ids resolves the household and every co-member member_ids = cr1._get_group_member_ids() self.assertIn(individual1.id, member_ids) + self.assertIn(group.id, member_ids) + self.assertIn(individual2.id, member_ids) + + # The rule's action is "warn", so the second CR is flagged, not blocked + self.assertEqual(cr1.conflict_status, "warning") + + def test_group_scope_group_registrant(self): + """A CR whose registrant is the group itself resolves its members.""" + group, individual1, individual2 = self._create_household() + + cr = self.env["spp.change.request"].create( + { + "request_type_id": self.cr_type.id, + "registrant_id": group.id, + } + ) + + member_ids = cr._get_group_member_ids() + self.assertIn(group.id, member_ids) + self.assertIn(individual1.id, member_ids) + self.assertIn(individual2.id, member_ids) + + def test_group_scope_ended_membership_excluded(self): + """Members whose membership has ended are not conflict candidates.""" + group, individual1, individual2 = self._create_household() + membership2 = self.env["spp.group.membership"].search( + [("group", "=", group.id), ("individual", "=", individual2.id)] + ) + membership2.ended_date = fields.Datetime.now() + + cr = self.env["spp.change.request"].create( + { + "request_type_id": self.cr_type.id, + "registrant_id": individual1.id, + } + ) + + member_ids = cr._get_group_member_ids() + self.assertIn(individual1.id, member_ids) + self.assertIn(group.id, member_ids) + self.assertNotIn(individual2.id, member_ids) class TestDuplicateDetectionAdvanced(TransactionCase): From c2a381720f42f00c2a9419d20900f86c21774ee5 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Fri, 28 Aug 2026 17:24:31 +0700 Subject: [PATCH 2/2] test(change_request): reach the ended membership without re-searching it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _create_household already creates both membership rows, so the ended membership test can set ended_date through the individual's own membership instead of querying the pair back by (group, individual). That drops a query and, more usefully, a second place spelling the membership field names — the very drift this branch repairs. Also name the change request under test `cr`, matching the sibling tests; the `cr1` name dated from a `cr2` that no longer exists. Restore the README table padding and column widths emitted by CI's generator: regenerating locally on Python 3.14 re-renders an unrelated docutils table one column wider than CI's Python 3.11, which failed the pre-commit check. Only the changelog entry this branch owns remains. --- spp_change_request_v2/README.rst | 32 +++++++++---------- .../static/description/index.html | 4 +-- .../tests/test_conflict_detection_extended.py | 11 +++---- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 4bcc6aa34..59e61398f 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -752,22 +752,22 @@ Methods available for override on detail models (all inherited from Related fields available on all detail models (from ``spp.cr.detail.base``): -+----------------------------+-----------+------------------------------------------------------------+ -| Field | Type | Source | -+============================+===========+============================================================+ -| ``change_request_id`` | Many2one | Direct link to parent CR | -+----------------------------+-----------+------------------------------------------------------------+ -| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``approval_state`` | Selection | ``change_request_id.approval_state`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``field_to_modify`` | Selection | Dynamic field selector (populated by | -| | | ``_get_field_to_modify_selection``) | -+----------------------------+-----------+------------------------------------------------------------+ ++--------------------------+-----------+------------------------------------------------------------+ +| Field | Type | Source | ++==========================+===========+============================================================+ +| ``change_request_id`` | Many2one | Direct link to parent CR | ++--------------------------+-----------+------------------------------------------------------------+ +| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``approval_state`` | Selection | ``change_request_id.approval_state`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``field_to_modify`` | Selection | Dynamic field selector (populated by | +| | | ``_get_field_to_modify_selection``) | ++--------------------------+-----------+------------------------------------------------------------+ CR Type Fields Reference ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index ad655e4f9..cd7c7753b 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1160,9 +1160,9 @@

    Methods Reference

    spp.cr.detail.base):

Field
-+-+ diff --git a/spp_change_request_v2/tests/test_conflict_detection_extended.py b/spp_change_request_v2/tests/test_conflict_detection_extended.py index a109e786d..a1e54918c 100644 --- a/spp_change_request_v2/tests/test_conflict_detection_extended.py +++ b/spp_change_request_v2/tests/test_conflict_detection_extended.py @@ -536,7 +536,7 @@ def test_group_scope_same_household_members(self): "registrant_id": individual2.id, } ) - cr1 = self.env["spp.change.request"].create( + cr = self.env["spp.change.request"].create( { "request_type_id": self.cr_type.id, "registrant_id": individual1.id, @@ -544,13 +544,13 @@ def test_group_scope_same_household_members(self): ) # _get_group_member_ids resolves the household and every co-member - member_ids = cr1._get_group_member_ids() + member_ids = cr._get_group_member_ids() self.assertIn(individual1.id, member_ids) self.assertIn(group.id, member_ids) self.assertIn(individual2.id, member_ids) # The rule's action is "warn", so the second CR is flagged, not blocked - self.assertEqual(cr1.conflict_status, "warning") + self.assertEqual(cr.conflict_status, "warning") def test_group_scope_group_registrant(self): """A CR whose registrant is the group itself resolves its members.""" @@ -571,10 +571,7 @@ def test_group_scope_group_registrant(self): def test_group_scope_ended_membership_excluded(self): """Members whose membership has ended are not conflict candidates.""" group, individual1, individual2 = self._create_household() - membership2 = self.env["spp.group.membership"].search( - [("group", "=", group.id), ("individual", "=", individual2.id)] - ) - membership2.ended_date = fields.Datetime.now() + individual2.individual_membership_ids.ended_date = fields.Datetime.now() cr = self.env["spp.change.request"].create( {
Field