From d3fec18471d98437a7a3f4f671f838470011dcc7 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:08:49 +0800 Subject: [PATCH 1/5] fix(spp_change_request_v2): let the registrant prefill through the freeze on an empty repaired detail _ensure_detail() may bind a new detail row to a submitted request (19.0.3.1.10), but the registrant prefill that follows is a write to the mapped fields, and _assert_content_editable refused it whenever the request type carries field mappings, which every shipped Edit Individual / Edit Group type does through spp_cr_types_base. The module's own test type had no mappings, so the repair passed in isolation and failed on every real deployment. The freeze now accepts exactly one write past submission: the registrant's current values onto a detail that holds no proposed content yet. It is recognised by shape, not caller, so RPC cannot claim it; a detail already carrying a proposal stays frozen even for the registrant's own value, and any other value is refused on an empty detail too. The frozen-detail tests give the test types the shipped mappings so module CI exercises this shape, and cover Edit Group. Refs #443 --- spp_change_request_v2/__manifest__.py | 2 +- .../models/change_request_detail_base.py | 41 ++++++++- spp_change_request_v2/readme/HISTORY.md | 4 + .../tests/test_frozen_detail_binding.py | 91 ++++++++++++++++++- 4 files changed, 133 insertions(+), 5 deletions(-) diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index b09a537a..80f9dd34 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.16", + "version": "19.0.3.1.17", "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/change_request_detail_base.py b/spp_change_request_v2/models/change_request_detail_base.py index 6546f162..b3863d29 100644 --- a/spp_change_request_v2/models/change_request_detail_base.py +++ b/spp_change_request_v2/models/change_request_detail_base.py @@ -99,19 +99,56 @@ def _protected_content_fields(self, change_request): protected |= {m.source_field for m in cr_type.apply_mapping_ids if m.source_field} return protected + def _is_prefill_of_empty_detail(self, vals, protected): + """Whether ``vals`` only copies the registrant's current values onto a + detail that holds no proposed content yet. + + ``_ensure_detail()`` repairs a submitted request that lost its detail + row by creating one and prefilling it from the registrant. That write + reaches the freeze like any other, yet it proposes nothing: every value + equals what the registrant already holds, so applying it changes + nothing — whereas leaving the row empty would, on approval, clear every + mapped field. The write is recognised by its shape, not by its caller, + so an RPC client cannot claim it: the detail must hold no protected + content at all, and each protected value written must be the + registrant's current value for that prefill mapping. + """ + self.ensure_one() + registrant = self.registrant_id + if not registrant: + return False + if any(normalize_frozen_value(self[f]) for f in protected if f in self._fields): + return False + mapping = self._get_prefill_mapping() + for field_name in protected: + if field_name not in vals or field_name not in self._fields: + continue + registrant_field = mapping.get(field_name) + if not registrant_field: + return False + current = getattr(registrant, registrant_field, False) + if normalize_frozen_value(vals[field_name]) != normalize_frozen_value(current): + return False + return True + def _assert_content_editable(self, vals): """Reject edits to proposed-change fields once the CR is submitted. Mirrors the view-level readonly (approval_state not in draft/revision) at the server so it cannot be bypassed via RPC. Editing requires resetting - the CR to draft, which re-routes the approval. + the CR to draft, which re-routes the approval. The one write accepted + past submission is the registrant prefill of a detail that holds nothing + yet (see ``_is_prefill_of_empty_detail``). """ for rec in self: change_request = rec.change_request_id state = change_request.approval_state if not change_request or state in ("draft", "revision") or not state: continue - for field_name in rec._protected_content_fields(change_request): + protected = rec._protected_content_fields(change_request) + if rec._is_prefill_of_empty_detail(vals, protected): + continue + for field_name in protected: if field_name not in vals or field_name not in rec._fields: continue # Normalize both sides (recordset -> id, None -> False) so an diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 4b8652f3..b8253b0a 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.17 + +- fix(change_request): a submitted request that lost its detail row can be opened again on a real deployment. 19.0.3.1.10 let `_ensure_detail()` bind a new detail past submission, but the registrant prefill that follows is a write to the mapped fields, and the detail-level freeze refused it whenever the request type carries field mappings — which every shipped Edit Individual / Edit Group type does through `spp_cr_types_base`. The module's own test type had no mappings, so the repair passed in isolation and failed everywhere else. The freeze now lets through exactly one write past submission: the registrant's current values onto a detail that holds no proposed content yet. Recognised by shape, not caller, so it cannot be claimed over RPC: a detail already carrying a proposal stays frozen even for the registrant's own value (writing it back would turn an approved "clear this field" into a no-op), and a value other than the registrant's is refused on an empty detail too. Leaving the repaired row empty was never an option: approving it would clear every mapped field. The frozen-detail tests now give the test types the shipped mappings so this shape is exercised in module CI (#443) + ### 19.0.3.1.16 - fix(change_request): an Edit Individual change request can be opened again on a registrant that already holds a future date of birth. `spp.change.request.create` prefills the detail from the registrant and that prefill is a write, so the guard added in 19.0.3.1.15 refused the copied value and the request could not be created at all — closing the very path field staff use to correct the date. A birthdate the guard would refuse is now dropped from the prefill mapping rather than offered, so the field arrives empty and a valid date has to be entered. The rule itself lives in one place (`_is_future_birthdate` on the mixin), so what prefill declines to offer and what the constraint refuses cannot drift apart. diff --git a/spp_change_request_v2/tests/test_frozen_detail_binding.py b/spp_change_request_v2/tests/test_frozen_detail_binding.py index 1b6da6a3..625dbd35 100644 --- a/spp_change_request_v2/tests/test_frozen_detail_binding.py +++ b/spp_change_request_v2/tests/test_frozen_detail_binding.py @@ -19,6 +19,33 @@ from .common import CRTestCase, get_or_create_cr_type +# The field mappings spp_cr_types_base ships for the two field_mapping types. +# The module's own test database has no spp_cr_types_base, so without these the +# test types carry no mappings, the detail-level freeze protects nothing but +# ``field_to_modify``, and the repair path passes here while failing on every +# real deployment. +EDIT_INDIVIDUAL_MAPPINGS = [ + ("given_name", "given_name"), + ("family_name", "family_name"), + ("birthdate", "birthdate"), + ("gender_id", "gender_id"), + ("phone", "phone"), + ("email", "email"), + ("address_line1", "street"), + ("address_line2", "street2"), + ("city", "city"), + ("postal_code", "zip"), +] +EDIT_GROUP_MAPPINGS = [ + ("group_name", "name"), + ("phone", "phone"), + ("email", "email"), + ("address_line1", "street"), + ("address_line2", "street2"), + ("city", "city"), + ("postal_code", "zip"), +] + @tagged("post_install", "-at_install") class TestFrozenDetailBinding(CRTestCase): @@ -26,9 +53,23 @@ class TestFrozenDetailBinding(CRTestCase): def setUpClass(cls): super().setUpClass() cls.edit_type = get_or_create_cr_type(cls.env, "edit_individual") + cls.edit_group_type = get_or_create_cr_type(cls.env, "edit_group") + cls._ensure_mappings(cls.edit_type, EDIT_INDIVIDUAL_MAPPINGS) + cls._ensure_mappings(cls.edit_group_type, EDIT_GROUP_MAPPINGS) - def _submitted_cr_without_detail(self): - cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) + @classmethod + def _ensure_mappings(cls, cr_type, pairs): + """Give a test-created type the mappings its shipped counterpart has.""" + if cr_type.apply_mapping_ids: + return + cls.env["spp.change.request.type.mapping"].create( + [{"type_id": cr_type.id, "source_field": source, "target_field": target} for source, target in pairs] + ) + + def _submitted_cr_without_detail(self, cr_type=None, registrant=None): + cr_type = cr_type or self.edit_type + registrant = registrant or self.test_individual + cr = self.CR.create({"request_type_id": cr_type.id, "registrant_id": registrant.id}) cr.get_detail() # materialise, then unbind while still in draft cr.write({"detail_res_id": False}) cr.sudo().write({"approval_state": "pending"}) @@ -44,12 +85,58 @@ def test_ensure_detail_can_bind_after_submit(self): self.assertTrue(detail, "_ensure_detail must be able to repair a submitted CR") self.assertTrue(cr.detail_res_id) self.assertEqual(detail.change_request_id, cr) + # The repaired detail proposes what the registrant already holds, so an + # approval applies nothing rather than clearing every mapped field. + self.assertEqual(detail.given_name, self.test_individual.given_name) + self.assertEqual(detail.family_name, self.test_individual.family_name) def test_get_detail_works_after_repair(self): cr = self._submitted_cr_without_detail() cr._ensure_detail() self.assertTrue(cr.get_detail()) + def test_edit_group_can_be_repaired_after_submit(self): + """Edit Group has fully overlapping prefill and apply mappings too.""" + cr = self._submitted_cr_without_detail(self.edit_group_type, self.test_group) + detail = cr._ensure_detail() + self.assertTrue(detail) + self.assertEqual(detail.group_name, self.test_group.name) + + # ------------------------------------------------------------------ + # Only the registrant prefill of an empty detail gets past the freeze + # ------------------------------------------------------------------ + + def _submitted_cr_with_fresh_detail(self): + """A submitted request whose detail row exists but holds nothing yet.""" + cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) + detail = cr.get_detail() + detail.write({source: False for source, _target in EDIT_INDIVIDUAL_MAPPINGS}) + cr.sudo().write({"approval_state": "pending"}) + return cr, detail + + def test_prefill_of_empty_detail_is_accepted_after_submit(self): + _cr, detail = self._submitted_cr_with_fresh_detail() + detail.prefill_from_registrant() + self.assertEqual(detail.given_name, self.test_individual.given_name) + + def test_other_value_on_empty_detail_is_refused_after_submit(self): + """Emptiness alone is not a licence: the value must be the registrant's.""" + _cr, detail = self._submitted_cr_with_fresh_detail() + with self.assertRaises(UserError): + detail.write({"given_name": "Someone Else"}) + + def test_prefill_shaped_write_on_detail_with_content_is_refused(self): + """A detail that already carries a proposal is frozen even for the + registrant's own value: writing it back would turn an approved + "clear this field" into a no-op.""" + cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) + detail = cr.get_detail() + detail.write({"family_name": False}) # in draft: propose clearing the family name + self.assertTrue(detail.given_name, "precondition: the detail still holds other proposed content") + cr.sudo().write({"approval_state": "pending"}) + with self.assertRaises(UserError): + detail.write({"family_name": self.test_individual.family_name}) + # ------------------------------------------------------------------ # Substitution must still be refused # ------------------------------------------------------------------ From 911797c09ac3ebb2b38a0259762ebfd3800badf5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:25:29 +0800 Subject: [PATCH 2/5] fix(spp_change_request_v2): prefill the repaired detail inside create() and keep the freeze absolute Review round: the write-path carve-out validated the no-op claim against the prefill mapping while apply uses the apply mapping (target_field, transform), and its precondition (a detail with no protected content) is exactly the shape of an approved clear-every- field proposal, so it left a one-write slot on such details. _ensure_detail now passes _prefill_values() into the detail's create(), so the repair performs no post-submit write at all and _assert_content_editable keeps no exemption. _prefill_values() returns ids for record values, since create() does not accept recordsets where write() did. Tests assert the repaired detail arrives populated and that every post-submit write to a mapped field, the registrant's own value included, stays refused. Refs #443 --- .../models/change_request.py | 15 ++-- .../models/change_request_detail_base.py | 82 +++++++------------ spp_change_request_v2/readme/HISTORY.md | 2 +- .../tests/test_frozen_detail_binding.py | 46 +++++++---- 4 files changed, 72 insertions(+), 73 deletions(-) diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 81d9e481..9eb069ae 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -855,13 +855,18 @@ def _ensure_detail(self): # Use sudo() for creation - users don't need create permission # Detail records are always created by the system automatically - detail = detail_model.sudo().create({cr_field: self.id}) # nosemgrep: odoo-sudo-without-context + vals = {cr_field: self.id} + # Pre-fill from the registrant in the same create() rather than by a + # later write(): this also runs to repair a request that was already + # submitted, and the detail-level freeze refuses every post-submit + # write to a mapped field. The prefill copies what the registrant + # already holds, so it proposes nothing, while a repaired row left + # empty would, on approval, clear every mapped field. + if hasattr(detail_model, "_prefill_values"): + vals.update(detail_model.sudo().new(vals)._prefill_values()) # nosemgrep: odoo-sudo-without-context + detail = detail_model.sudo().create(vals) # nosemgrep: odoo-sudo-without-context self.detail_res_id = detail.id - # Pre-fill detail from registrant if the detail model supports it - if hasattr(detail, "prefill_from_registrant"): - detail.prefill_from_registrant() - return self.get_detail() # ══════════════════════════════════════════════════════════════════════════ diff --git a/spp_change_request_v2/models/change_request_detail_base.py b/spp_change_request_v2/models/change_request_detail_base.py index b3863d29..e18685bc 100644 --- a/spp_change_request_v2/models/change_request_detail_base.py +++ b/spp_change_request_v2/models/change_request_detail_base.py @@ -99,56 +99,22 @@ def _protected_content_fields(self, change_request): protected |= {m.source_field for m in cr_type.apply_mapping_ids if m.source_field} return protected - def _is_prefill_of_empty_detail(self, vals, protected): - """Whether ``vals`` only copies the registrant's current values onto a - detail that holds no proposed content yet. - - ``_ensure_detail()`` repairs a submitted request that lost its detail - row by creating one and prefilling it from the registrant. That write - reaches the freeze like any other, yet it proposes nothing: every value - equals what the registrant already holds, so applying it changes - nothing — whereas leaving the row empty would, on approval, clear every - mapped field. The write is recognised by its shape, not by its caller, - so an RPC client cannot claim it: the detail must hold no protected - content at all, and each protected value written must be the - registrant's current value for that prefill mapping. - """ - self.ensure_one() - registrant = self.registrant_id - if not registrant: - return False - if any(normalize_frozen_value(self[f]) for f in protected if f in self._fields): - return False - mapping = self._get_prefill_mapping() - for field_name in protected: - if field_name not in vals or field_name not in self._fields: - continue - registrant_field = mapping.get(field_name) - if not registrant_field: - return False - current = getattr(registrant, registrant_field, False) - if normalize_frozen_value(vals[field_name]) != normalize_frozen_value(current): - return False - return True - def _assert_content_editable(self, vals): """Reject edits to proposed-change fields once the CR is submitted. Mirrors the view-level readonly (approval_state not in draft/revision) at the server so it cannot be bypassed via RPC. Editing requires resetting - the CR to draft, which re-routes the approval. The one write accepted - past submission is the registrant prefill of a detail that holds nothing - yet (see ``_is_prefill_of_empty_detail``). + the CR to draft, which re-routes the approval. There is no exemption: + the registrant prefill of a repaired detail happens inside ``create()`` + (``_ensure_detail`` passes ``_prefill_values()`` to it), so it never + reaches this guard. """ for rec in self: change_request = rec.change_request_id state = change_request.approval_state if not change_request or state in ("draft", "revision") or not state: continue - protected = rec._protected_content_fields(change_request) - if rec._is_prefill_of_empty_detail(vals, protected): - continue - for field_name in protected: + for field_name in rec._protected_content_fields(change_request): if field_name not in vals or field_name not in rec._fields: continue # Normalize both sides (recordset -> id, None -> False) so an @@ -311,27 +277,37 @@ def _get_prefill_mapping(self): """ return {} - def prefill_from_registrant(self): - """Pre-fill detail fields from registrant. - - This method updates the current record with values from the registrant - based on the mapping defined in _get_prefill_mapping(). + def _prefill_values(self): + """The registrant's current values for the fields in _get_prefill_mapping(). - Override _get_prefill_mapping() in detail models to enable prefilling. + Works on a ``new()`` record as well as a stored one, so ``_ensure_detail`` + can pass the result straight into ``create()``; only truthy registrant + values are included. """ self.ensure_one() if not self.registrant_id: - return - - mapping = self._get_prefill_mapping() - if not mapping: - return + return {} values = {} - for detail_field, registrant_field in mapping.items(): + for detail_field, registrant_field in self._get_prefill_mapping().items(): registrant_value = getattr(self.registrant_id, registrant_field, False) - if registrant_value: - values[detail_field] = registrant_value + if not registrant_value: + continue + if isinstance(registrant_value, models.BaseModel): + # create() takes an id where write() also accepts a recordset. + registrant_value = registrant_value.id + values[detail_field] = registrant_value + return values + + def prefill_from_registrant(self): + """Pre-fill detail fields from registrant. + + This method updates the current record with values from the registrant + based on the mapping defined in _get_prefill_mapping(). It is a write, + so on a submitted request the detail-level freeze applies to it. + Override _get_prefill_mapping() in detail models to enable prefilling. + """ + values = self._prefill_values() if values: self.write(values) diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index b8253b0a..f0ee7a87 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.3.1.17 -- fix(change_request): a submitted request that lost its detail row can be opened again on a real deployment. 19.0.3.1.10 let `_ensure_detail()` bind a new detail past submission, but the registrant prefill that follows is a write to the mapped fields, and the detail-level freeze refused it whenever the request type carries field mappings — which every shipped Edit Individual / Edit Group type does through `spp_cr_types_base`. The module's own test type had no mappings, so the repair passed in isolation and failed everywhere else. The freeze now lets through exactly one write past submission: the registrant's current values onto a detail that holds no proposed content yet. Recognised by shape, not caller, so it cannot be claimed over RPC: a detail already carrying a proposal stays frozen even for the registrant's own value (writing it back would turn an approved "clear this field" into a no-op), and a value other than the registrant's is refused on an empty detail too. Leaving the repaired row empty was never an option: approving it would clear every mapped field. The frozen-detail tests now give the test types the shipped mappings so this shape is exercised in module CI (#443) +- fix(change_request): a submitted request that lost its detail row can be opened again on a real deployment. 19.0.3.1.10 let `_ensure_detail()` bind a new detail past submission, but it then prefilled the row from the registrant with a write, and the detail-level freeze refused that write whenever the request type carries field mappings — which every shipped Edit Individual / Edit Group type does through `spp_cr_types_base`. The module's own test type had no mappings, so the repair passed in isolation and failed everywhere else. The prefill now goes into the detail's `create()` (`_prefill_values()` feeds it), so the repair performs no post-submit write at all and the freeze keeps no exemption: a repaired row arrives already carrying what the registrant holds — leaving it empty was never an option, approving that would clear every mapped field — and every later write to a mapped field on a submitted request, the registrant's own value included, stays refused. The frozen-detail tests give the test types the shipped mappings so this shape is exercised in module CI (#443) ### 19.0.3.1.16 diff --git a/spp_change_request_v2/tests/test_frozen_detail_binding.py b/spp_change_request_v2/tests/test_frozen_detail_binding.py index 625dbd35..84e97ff1 100644 --- a/spp_change_request_v2/tests/test_frozen_detail_binding.py +++ b/spp_change_request_v2/tests/test_frozen_detail_binding.py @@ -103,40 +103,58 @@ def test_edit_group_can_be_repaired_after_submit(self): self.assertEqual(detail.group_name, self.test_group.name) # ------------------------------------------------------------------ - # Only the registrant prefill of an empty detail gets past the freeze + # The freeze stays absolute: the repair prefills inside create(), never by write() # ------------------------------------------------------------------ - def _submitted_cr_with_fresh_detail(self): - """A submitted request whose detail row exists but holds nothing yet.""" + def _submitted_cr_with_empty_detail(self): + """A submitted request whose detail proposes clearing every mapped field. + + This is also the shape a repaired row would have if it were created + empty, which is why the repair must prefill at creation: no write to a + mapped field is accepted past submission, whatever value it carries. + """ cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) detail = cr.get_detail() detail.write({source: False for source, _target in EDIT_INDIVIDUAL_MAPPINGS}) cr.sudo().write({"approval_state": "pending"}) return cr, detail - def test_prefill_of_empty_detail_is_accepted_after_submit(self): - _cr, detail = self._submitted_cr_with_fresh_detail() - detail.prefill_from_registrant() - self.assertEqual(detail.given_name, self.test_individual.given_name) + def test_prefill_write_is_refused_after_submit(self): + """Even the registrant's own values cannot be written onto an empty + submitted detail: that would turn an approved "clear these fields" into + a no-op. The repair path does not need this write (see create-time prefill).""" + _cr, detail = self._submitted_cr_with_empty_detail() + with self.assertRaises(UserError): + detail.prefill_from_registrant() + self.assertFalse(detail.given_name) def test_other_value_on_empty_detail_is_refused_after_submit(self): - """Emptiness alone is not a licence: the value must be the registrant's.""" - _cr, detail = self._submitted_cr_with_fresh_detail() + _cr, detail = self._submitted_cr_with_empty_detail() with self.assertRaises(UserError): detail.write({"given_name": "Someone Else"}) - def test_prefill_shaped_write_on_detail_with_content_is_refused(self): - """A detail that already carries a proposal is frozen even for the - registrant's own value: writing it back would turn an approved - "clear this field" into a no-op.""" + def test_registrants_own_value_on_detail_with_content_is_refused(self): + """A single cleared field cannot be restored to the registrant's value either.""" cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) detail = cr.get_detail() detail.write({"family_name": False}) # in draft: propose clearing the family name - self.assertTrue(detail.given_name, "precondition: the detail still holds other proposed content") cr.sudo().write({"approval_state": "pending"}) with self.assertRaises(UserError): detail.write({"family_name": self.test_individual.family_name}) + def test_repaired_detail_is_prefilled_without_a_write(self): + """The repair creates the detail already populated, so a later approval + applies nothing rather than clearing every mapped field.""" + cr = self._submitted_cr_without_detail() + detail = cr._ensure_detail() + expected = { + source: getattr(self.test_individual, target) + for source, target in EDIT_INDIVIDUAL_MAPPINGS + if getattr(self.test_individual, target) + } + for field_name, value in expected.items(): + self.assertEqual(detail[field_name], value, field_name) + # ------------------------------------------------------------------ # Substitution must still be refused # ------------------------------------------------------------------ From 5b39a7133c7c0dab9dcd3b5615e7617a4530fbcb Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:39:38 +0800 Subject: [PATCH 3/5] docs(spp_change_request_v2): regenerate README from fragments (CI output) --- spp_change_request_v2/README.rst | 21 ++++++ .../static/description/index.html | 72 ++++++++++++------- 2 files changed, 68 insertions(+), 25 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 29781591..b90c9407 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,27 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.1.17 +~~~~~~~~~~~ + +- fix(change_request): a submitted request that lost its detail row can + be opened again on a real deployment. 19.0.3.1.10 let + ``_ensure_detail()`` bind a new detail past submission, but it then + prefilled the row from the registrant with a write, and the + detail-level freeze refused that write whenever the request type + carries field mappings — which every shipped Edit Individual / Edit + Group type does through ``spp_cr_types_base``. The module's own test + type had no mappings, so the repair passed in isolation and failed + everywhere else. The prefill now goes into the detail's ``create()`` + (``_prefill_values()`` feeds it), so the repair performs no + post-submit write at all and the freeze keeps no exemption: a repaired + row arrives already carrying what the registrant holds — leaving it + empty was never an option, approving that would clear every mapped + field — and every later write to a mapped field on a submitted + request, the registrant's own value included, stays refused. The + frozen-detail tests give the test types the shipped mappings so this + shape is exercised in module CI (#443) + 19.0.3.1.16 ~~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index 90a55209..cb95e02e 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,28 @@

Changelog

+

19.0.3.1.17

+
    +
  • fix(change_request): a submitted request that lost its detail row can +be opened again on a real deployment. 19.0.3.1.10 let +_ensure_detail() bind a new detail past submission, but it then +prefilled the row from the registrant with a write, and the +detail-level freeze refused that write whenever the request type +carries field mappings — which every shipped Edit Individual / Edit +Group type does through spp_cr_types_base. The module’s own test +type had no mappings, so the repair passed in isolation and failed +everywhere else. The prefill now goes into the detail’s create() +(_prefill_values() feeds it), so the repair performs no +post-submit write at all and the freeze keeps no exemption: a repaired +row arrives already carrying what the registrant holds — leaving it +empty was never an option, approving that would clear every mapped +field — and every later write to a mapped field on a submitted +request, the registrant’s own value included, stays refused. The +frozen-detail tests give the test types the shipped mappings so this +shape is exercised in module CI (#443)
    • +
+
+

19.0.3.1.16

  • fix(change_request): an Edit Individual change request can be opened @@ -1358,7 +1380,7 @@

    19.0.3.1.16

    which of several lines to fix.
-
+

19.0.3.1.15

  • fix(change_request): refuse a date of birth in the future while the @@ -1374,7 +1396,7 @@

    19.0.3.1.15

    recorded earlier that local day (#362)
-
+

19.0.3.1.14

  • fix(change_request): group-scope conflict rules work again. @@ -1391,7 +1413,7 @@

    19.0.3.1.14

    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 @@ -1408,7 +1430,7 @@

    19.0.3.1.13

    routing key is still not applied.
-
+

19.0.3.1.12

  • fix(change_request): auto-apply-on-approve runs through the public @@ -1423,7 +1445,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 @@ -1462,7 +1484,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 @@ -1502,7 +1524,7 @@

    19.0.3.1.10

    configured mapping.
-
+

19.0.3.1.9

  • fix(security): duplicate detection now scores the fields both change @@ -1519,7 +1541,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 @@ -1537,7 +1559,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 @@ -1552,7 +1574,7 @@

    19.0.3.1.7

    endpoint.
-
+

19.0.3.1.6

  • fix(security): derive conflict and duplicate detection from the change @@ -1566,7 +1588,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 @@ -1578,7 +1600,7 @@

    19.0.3.1.5

    are noupdate.
-
+

19.0.3.1.4

  • fix(security): add ownership and area record rules to every concrete @@ -1595,7 +1617,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 @@ -1608,7 +1630,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 @@ -1617,7 +1639,7 @@

    19.0.3.1.2

    (#1136)
-
+

19.0.3.1.1

  • fix(change_request): enforce the (cr_type_id, reason) uniqueness @@ -1631,7 +1653,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 @@ -1649,7 +1671,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) — @@ -1671,7 +1693,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 @@ -1682,7 +1704,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 @@ -1694,7 +1716,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1709,7 +1731,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 @@ -1722,27 +1744,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
    • From 440596dd81063743ff32c8514cb37641d55fe0bb Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 13:46:01 +0800 Subject: [PATCH 4/5] fix(spp_change_request_v2): refuse a post-submit repair that would propose a change Second review round. The rebuilt row applies nothing only as far as the prefill covers the apply mapping; a value the prefill declines to offer (a legacy future date of birth, or a mapping added later for a field the prefill does not know) would be applied as clearing that field, and the frozen row could not be corrected. A field-mapping request repaired after submission is now checked with the strategy's preview and refused with an explicit message when anything would change; a post-submit repair is logged. Tests apply a repaired request end to end and cover the future-birthdate refusal, the draft-time drop, a Many2one prefill value, and pin the test mappings against the shipped ones. Refs #443 --- .../models/change_request.py | 41 +++++++++++- spp_change_request_v2/readme/HISTORY.md | 2 +- .../tests/test_birthdate_mixin.py | 10 +-- .../tests/test_frozen_detail_binding.py | 63 ++++++++++++++++++- 4 files changed, 106 insertions(+), 10 deletions(-) diff --git a/spp_change_request_v2/models/change_request.py b/spp_change_request_v2/models/change_request.py index 9eb069ae..f8966837 100644 --- a/spp_change_request_v2/models/change_request.py +++ b/spp_change_request_v2/models/change_request.py @@ -862,13 +862,50 @@ def _ensure_detail(self): # write to a mapped field. The prefill copies what the registrant # already holds, so it proposes nothing, while a repaired row left # empty would, on approval, clear every mapped field. + prefill = {} if hasattr(detail_model, "_prefill_values"): - vals.update(detail_model.sudo().new(vals)._prefill_values()) # nosemgrep: odoo-sudo-without-context - detail = detail_model.sudo().create(vals) # nosemgrep: odoo-sudo-without-context + # nosemgrep: odoo-sudo-without-context + prefill = detail_model.sudo().new(vals)._prefill_values() + # nosemgrep: odoo-sudo-without-context + detail = detail_model.sudo().create({**vals, **prefill}) self.detail_res_id = detail.id + if self.approval_state and self.approval_state not in ("draft", "revision"): + if self.request_type_id.apply_strategy == "field_mapping": + self._assert_reconstructed_detail_proposes_nothing() + _logger.warning( + "Change request %s: detail record reconstructed after submission", + self.name, + ) return self.get_detail() + def _assert_reconstructed_detail_proposes_nothing(self): + """A field-mapping detail rebuilt for a submitted request must not carry a proposal. + + The rebuilt row holds the registrant's current values, so approving + it applies nothing. That holds only as far as the prefill covers the + apply mapping: a value the prefill declines to offer (a future date of + birth, or a mapping added later for a field the prefill does not know) + would be applied as "clear this field", because the field-mapping + strategy writes empty values on purpose. Nobody can correct the row + afterwards, since it is frozen, so the request is refused instead and + has to be reset to draft. + """ + self.ensure_one() + sudo_rec = self.sudo() # nosemgrep: odoo-sudo-without-context + changes = dict(sudo_rec.request_type_id.get_apply_strategy().preview(sudo_rec) or {}) + changes.pop("_action", None) + changes.pop("_message", None) + if changes: + raise UserError( + _( + "The details of this submitted change request were lost and cannot be " + "reconstructed without proposing a change to: %(fields)s. " + "Reset the request to draft and enter the details again.", + fields=", ".join(sorted(changes)), + ) + ) + # ══════════════════════════════════════════════════════════════════════════ # APPROVAL ACTIONS # ══════════════════════════════════════════════════════════════════════════ diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index f0ee7a87..f41e393e 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.3.1.17 -- fix(change_request): a submitted request that lost its detail row can be opened again on a real deployment. 19.0.3.1.10 let `_ensure_detail()` bind a new detail past submission, but it then prefilled the row from the registrant with a write, and the detail-level freeze refused that write whenever the request type carries field mappings — which every shipped Edit Individual / Edit Group type does through `spp_cr_types_base`. The module's own test type had no mappings, so the repair passed in isolation and failed everywhere else. The prefill now goes into the detail's `create()` (`_prefill_values()` feeds it), so the repair performs no post-submit write at all and the freeze keeps no exemption: a repaired row arrives already carrying what the registrant holds — leaving it empty was never an option, approving that would clear every mapped field — and every later write to a mapped field on a submitted request, the registrant's own value included, stays refused. The frozen-detail tests give the test types the shipped mappings so this shape is exercised in module CI (#443) +- fix(change_request): a submitted request that lost its detail row can be opened again on a real deployment. 19.0.3.1.10 let `_ensure_detail()` bind a new detail past submission, but it then prefilled the row from the registrant with a write, and the detail-level freeze refused that write whenever the request type carries field mappings — which every shipped Edit Individual / Edit Group type does through `spp_cr_types_base`. The module's own test type had no mappings, so the repair passed in isolation and failed everywhere else. The prefill now goes into the detail's `create()` (`_prefill_values()` feeds it), so the repair performs no post-submit write at all and the freeze keeps no exemption: a repaired row arrives already carrying what the registrant holds — leaving it empty was never an option, approving that would clear every mapped field — and every later write to a mapped field on a submitted request, the registrant's own value included, stays refused. Where the prefill cannot cover a mapped field — a legacy registrant holding a future date of birth, which the prefill declines to offer — the rebuilt row would propose clearing it, so a post-submit repair of a field-mapping request is refused with an explicit message instead, and the request has to be reset to draft; a post-submit repair is also logged. The frozen-detail tests give the test types the shipped mappings so this shape is exercised in module CI, and apply a repaired request end to end to prove the registrant is untouched (#443) ### 19.0.3.1.16 diff --git a/spp_change_request_v2/tests/test_birthdate_mixin.py b/spp_change_request_v2/tests/test_birthdate_mixin.py index 89b4d872..4b155188 100644 --- a/spp_change_request_v2/tests/test_birthdate_mixin.py +++ b/spp_change_request_v2/tests/test_birthdate_mixin.py @@ -64,11 +64,11 @@ def test_add_member_future_birthdate_rejected(self): def test_edit_individual_prefill_skips_future_birthdate(self): """A change request can still be opened on a registrant holding one. - ``spp.change.request.create`` prefills the detail from the registrant, - and that prefill is a write: offering the birthdate back would raise - the mixin's constraint while the request is being created, so the - request could not be opened at all — closing the very path field staff - use to correct the date. + ``spp.change.request.create`` prefills the detail from the registrant + (inside the detail's own ``create()``, where the mixin's constraint + runs too): offering the birthdate back would raise while the request + is being created, so the request could not be opened at all — closing + the very path field staff use to correct the date. """ subject = self.Partner.create( { diff --git a/spp_change_request_v2/tests/test_frozen_detail_binding.py b/spp_change_request_v2/tests/test_frozen_detail_binding.py index 84e97ff1..15a392d8 100644 --- a/spp_change_request_v2/tests/test_frozen_detail_binding.py +++ b/spp_change_request_v2/tests/test_frozen_detail_binding.py @@ -14,6 +14,9 @@ request, so it cannot be used to attach someone else's detail. """ +from datetime import timedelta + +from odoo import fields from odoo.exceptions import UserError from odoo.tests import tagged @@ -59,13 +62,29 @@ def setUpClass(cls): @classmethod def _ensure_mappings(cls, cr_type, pairs): - """Give a test-created type the mappings its shipped counterpart has.""" + """Give a test-created type the mappings its shipped counterpart has. + + When the shipped type is present (a full stack), the constants must + match it exactly, so a drift in ``spp_cr_types_base`` shows up here + instead of silently narrowing what these tests clear and assert. + """ if cr_type.apply_mapping_ids: + shipped = {(m.source_field, m.target_field) for m in cr_type.apply_mapping_ids} + if shipped != set(pairs): + raise AssertionError(f"{cr_type.code}: shipped mappings drifted from the test constants") return cls.env["spp.change.request.type.mapping"].create( [{"type_id": cr_type.id, "source_field": source, "target_field": target} for source, target in pairs] ) + def _plant_future_birthdate(self, registrant): + """Store a future date of birth the way a legacy record holds one: + the registry constraint refuses it on write, so go under the ORM.""" + future = fields.Date.context_today(registrant) + timedelta(days=30) + self.env.cr.execute("UPDATE res_partner SET birthdate = %s WHERE id = %s", (future, registrant.id)) + registrant.invalidate_recordset(["birthdate"]) + return future + def _submitted_cr_without_detail(self, cr_type=None, registrant=None): cr_type = cr_type or self.edit_type registrant = registrant or self.test_individual @@ -124,7 +143,7 @@ def test_prefill_write_is_refused_after_submit(self): submitted detail: that would turn an approved "clear these fields" into a no-op. The repair path does not need this write (see create-time prefill).""" _cr, detail = self._submitted_cr_with_empty_detail() - with self.assertRaises(UserError): + with self.assertRaisesRegex(UserError, "already been submitted for approval"): detail.prefill_from_registrant() self.assertFalse(detail.given_name) @@ -145,6 +164,10 @@ def test_registrants_own_value_on_detail_with_content_is_refused(self): def test_repaired_detail_is_prefilled_without_a_write(self): """The repair creates the detail already populated, so a later approval applies nothing rather than clearing every mapped field.""" + gender = self.env["spp.vocabulary.code"].search([("namespace_uri", "ilike", "gender")], limit=1) + if gender: + # A Many2one value: create() takes an id where write() also took a recordset. + self.test_individual.write({"gender_id": gender.id}) cr = self._submitted_cr_without_detail() detail = cr._ensure_detail() expected = { @@ -152,9 +175,45 @@ def test_repaired_detail_is_prefilled_without_a_write(self): for source, target in EDIT_INDIVIDUAL_MAPPINGS if getattr(self.test_individual, target) } + self.assertGreaterEqual(len(expected), 4, "fixture must hold enough values for this to assert anything") + if gender: + self.assertEqual(detail.gender_id, gender) for field_name, value in expected.items(): self.assertEqual(detail[field_name], value, field_name) + def test_approving_a_repaired_request_changes_nothing(self): + """End to end: apply the repaired request and the registrant is untouched.""" + before = {target: getattr(self.test_individual, target) for _source, target in EDIT_INDIVIDUAL_MAPPINGS} + cr = self._submitted_cr_without_detail() + cr._ensure_detail() + cr.sudo().write({"approval_state": "approved"}) + + cr.sudo().request_type_id.get_apply_strategy().apply(cr.sudo()) + + after = {target: getattr(self.test_individual, target) for _source, target in EDIT_INDIVIDUAL_MAPPINGS} + self.assertEqual(after, before) + + def test_repair_refused_when_it_would_propose_a_change(self): + """A value the prefill declines to offer would be applied as "clear this + field" — the field-mapping strategy writes empties on purpose. A legacy + registrant holding a future date of birth is the shipped case: the + prefill drops it, so the rebuilt row would clear the DOB on approval. + The repair refuses instead, and the request keeps no detail row.""" + future = self._plant_future_birthdate(self.test_individual) + cr = self._submitted_cr_without_detail() + with self.assertRaisesRegex(UserError, "cannot be reconstructed without proposing a change"): + cr._ensure_detail() + self.assertEqual(self.test_individual.birthdate, future, "the registrant is untouched") + + def test_repair_in_draft_still_drops_a_future_birthdate(self): + """Before submission the row is editable, so the birthdate is simply left + empty for the user to correct — the 19.0.3.1.16 behaviour.""" + self._plant_future_birthdate(self.test_individual) + cr = self.CR.create({"request_type_id": self.edit_type.id, "registrant_id": self.test_individual.id}) + detail = cr.get_detail() + self.assertFalse(detail.birthdate) + self.assertEqual(detail.given_name, self.test_individual.given_name) + # ------------------------------------------------------------------ # Substitution must still be refused # ------------------------------------------------------------------ From d90820cbf1e83986a284230c3eed8ba7d4bd5bad Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 21 Sep 2026 14:11:43 +0800 Subject: [PATCH 5/5] docs(spp_change_request_v2): regenerate README from fragments (CI output) --- spp_change_request_v2/README.rst | 12 +++++++++--- spp_change_request_v2/static/description/index.html | 12 +++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index b90c9407..ecec4a7b 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -870,9 +870,15 @@ Changelog row arrives already carrying what the registrant holds — leaving it empty was never an option, approving that would clear every mapped field — and every later write to a mapped field on a submitted - request, the registrant's own value included, stays refused. The - frozen-detail tests give the test types the shipped mappings so this - shape is exercised in module CI (#443) + request, the registrant's own value included, stays refused. Where the + prefill cannot cover a mapped field — a legacy registrant holding a + future date of birth, which the prefill declines to offer — the + rebuilt row would propose clearing it, so a post-submit repair of a + field-mapping request is refused with an explicit message instead, and + the request has to be reset to draft; a post-submit repair is also + logged. The frozen-detail tests give the test types the shipped + mappings so this shape is exercised in module CI, and apply a repaired + request end to end to prove the registrant is untouched (#443) 19.0.3.1.16 ~~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index cb95e02e..002e0c2f 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1355,9 +1355,15 @@

    19.0.3.1.17

    row arrives already carrying what the registrant holds — leaving it empty was never an option, approving that would clear every mapped field — and every later write to a mapped field on a submitted -request, the registrant’s own value included, stays refused. The -frozen-detail tests give the test types the shipped mappings so this -shape is exercised in module CI (#443) +request, the registrant’s own value included, stays refused. Where the +prefill cannot cover a mapped field — a legacy registrant holding a +future date of birth, which the prefill declines to offer — the +rebuilt row would propose clearing it, so a post-submit repair of a +field-mapping request is refused with an explicit message instead, and +the request has to be reset to draft; a post-submit repair is also +logged. The frozen-detail tests give the test types the shipped +mappings so this shape is exercised in module CI, and apply a repaired +request end to end to prove the registrant is untouched (#443)