diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 4b687bcd..dc463b5b 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -853,6 +853,14 @@ Before declaring a new CR type complete: Changelog ========= +19.0.3.1.2 +~~~~~~~~~~ + +- fix(change_request_v2): adding an ID now looks for a live one of that + type rather than any row at all, so an ID that was removed through a + change request no longer blocks adding a replacement of the same type + (#1136) + 19.0.3.1.1 ~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index cd24a9e2..181d8fec 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.1", + "version": "19.0.3.1.2", "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/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index a87a4655..e6296543 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.2 + +- fix(change_request_v2): adding an ID now looks for a live one of that type rather than any row at all, so an ID that was removed through a change request no longer blocks adding a replacement of the same type (#1136) + ### 19.0.3.1.1 - fix(change_request): enforce the `(cr_type_id, reason)` uniqueness of per-reason Required-Documents rules with `models.Constraint` (#394). The rule was previously declared via the legacy `_sql_constraints` attribute, which Odoo 19 ignores — the constraint was never created, so duplicate rules for the same reason could be saved silently since 19.0.3.0.0 and one WARNING line was logged on every registry load. A pre-migration removes duplicate rules (the lowest-id rule per pair is kept, matching which rule the runtime applied) so the constraint applies cleanly on upgrade. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index d5ddbc6e..1587cb85 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1339,6 +1339,15 @@

Changelog

+

19.0.3.1.2

+ +
+

19.0.3.1.1

-
+

19.0.3.1.0

-
+

19.0.3.0.0

  • feat(change_request): redesign the group/membership CR flows (#242) — @@ -1392,7 +1401,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 @@ -1403,7 +1412,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 @@ -1415,7 +1424,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1430,7 +1439,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 @@ -1443,27 +1452,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/strategies/update_id.py b/spp_change_request_v2/strategies/update_id.py index 30f61542..ca8694da 100644 --- a/spp_change_request_v2/strategies/update_id.py +++ b/spp_change_request_v2/strategies/update_id.py @@ -41,11 +41,16 @@ def _apply_add(self, registrant, detail, change_request): if not detail.id_value: raise UserError(_("ID value is required.")) - # Check if ID type already exists for this registrant + # Check if a *live* ID of this type already exists for this registrant. + # Removing an ID through a change request marks it Invalid rather than + # deleting it, so an unscoped search counted those dead rows and left + # the type permanently unusable — the same defect as the uniqueness + # index on spp.registry.id (OP#1136). existing = self.env["spp.registry.id"].search( [ ("partner_id", "=", registrant.id), ("id_type_id", "=", detail.id_type_id.id), + ("status", "!=", "invalid"), ], limit=1, ) diff --git a/spp_change_request_v2/tests/test_update_id_strategy.py b/spp_change_request_v2/tests/test_update_id_strategy.py index 1c888e4e..1654d1f9 100644 --- a/spp_change_request_v2/tests/test_update_id_strategy.py +++ b/spp_change_request_v2/tests/test_update_id_strategy.py @@ -186,6 +186,59 @@ def test_remove_id(self): self.assertTrue(cr.is_applied) self.assertEqual(id_to_remove.status, "invalid") + def test_readd_same_type_after_removal(self): + """OP#1136: removing an ID must free its type for a replacement. + + The reported bug end to end — a removed ID is kept and marked Invalid, + and both the duplicate check here and the uniqueness rule on + spp.registry.id counted that dead row, so the registrant was left with + an Invalid ID and no way to add a valid one of the same type. + """ + original = self.id_model.create( + { + "partner_id": self.individual.id, + "id_type_id": self.passport_type.id, + "value": "PP-ORIGINAL", + "status": "valid", + } + ) + + removal = self.cr_model.create({"request_type_id": self.cr_type.id, "registrant_id": self.individual.id}) + removal.get_detail().write( + { + "operation": "remove", + "existing_id_record_id": original.id, + "id_type_id": self.passport_type.id, + } + ) + removal.approval_state = "approved" + removal.action_apply() + self.assertEqual(original.status, "invalid") + + # The replacement, through the same change-request route. + replacement = self.cr_model.create({"request_type_id": self.cr_type.id, "registrant_id": self.individual.id}) + replacement.get_detail().write( + { + "operation": "add", + "id_type_id": self.passport_type.id, + "id_value": "PP-REPLACEMENT", + } + ) + replacement.approval_state = "approved" + replacement.action_apply() + + self.assertTrue(replacement.is_applied) + live = self.id_model.search( + [ + ("partner_id", "=", self.individual.id), + ("id_type_id", "=", self.passport_type.id), + ("status", "!=", "invalid"), + ] + ) + self.assertEqual(len(live), 1, "exactly one live ID of that type should remain") + self.assertEqual(live.value, "PP-REPLACEMENT") + self.assertEqual(original.status, "invalid", "the removed ID stays on file as Invalid") + def test_update_without_existing_id_fails(self): """Test update operation requires existing ID.""" diff --git a/spp_registry/README.rst b/spp_registry/README.rst index a9369ecd..b4a09d08 100644 --- a/spp_registry/README.rst +++ b/spp_registry/README.rst @@ -139,6 +139,17 @@ Dependencies Changelog ========= +19.0.2.2.0 +~~~~~~~~~~ + +- fix(registry): let an ID type be used again after its ID was removed. + Removing an ID through a change request keeps the row and marks it + Invalid, and the old uniqueness rule counted those dead rows — so the + registrant was left with an Invalid ID and no way to add a valid one + of the same type. Uniqueness now applies to live IDs only, and is + refused before the write so the message names the ID type rather than + surfacing a database error (#1136) + 19.0.2.1.4 ~~~~~~~~~~ diff --git a/spp_registry/__manifest__.py b/spp_registry/__manifest__.py index 4b2a0948..f75d8539 100644 --- a/spp_registry/__manifest__.py +++ b/spp_registry/__manifest__.py @@ -3,7 +3,7 @@ { "name": "OpenSPP Registry", "category": "OpenSPP/Core", - "version": "19.0.2.1.4", + "version": "19.0.2.2.0", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_registry/migrations/19.0.2.2.0/pre-migration.py b/spp_registry/migrations/19.0.2.2.0/pre-migration.py new file mode 100644 index 00000000..bc35408d --- /dev/null +++ b/spp_registry/migrations/19.0.2.2.0/pre-migration.py @@ -0,0 +1,33 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Drop the unconditional ID-type constraint this version replaces (OP#1136). + +``spp.registry.id`` used to carry ``UNIQUE(partner_id, id_type_id)``. Removing +an ID through a change request keeps the row and marks it Invalid, so that +constraint counted dead rows: once an ID had been removed, that type could never +be used again for the registrant. It is replaced by a partial unique index that +ignores invalid rows, declared on the model as ``models.UniqueIndex``. + +The replacement is created by the framework, but the old constraint has to be +dropped here: Odoo only adds and updates the constraints a model declares, and +never removes one that has simply stopped being declared. Left in place it would +keep refusing exactly the Remove-then-Add sequence this version fixes. + +Runs pre-migration so the constraint is gone before the ORM reconciles the +table, which is also when the new index is created. +""" + +import logging + +_logger = logging.getLogger(__name__) + + +def migrate(cr, version): + if not version: + return + + # Written as a literal rather than composed: nothing here is dynamic, and a + # composed SQL string is the pitfall the injection check exists to catch. + # Odoo names a table constraint "{table}_{attribute with the leading + # underscore removed}", so `_unique_partner_id_type` became this. + cr.execute("ALTER TABLE spp_registry_id DROP CONSTRAINT IF EXISTS spp_registry_id_unique_partner_id_type") + _logger.info("Dropped the unconditional ID-type constraint; live-only uniqueness is now a partial index") diff --git a/spp_registry/models/reg_id.py b/spp_registry/models/reg_id.py index d7bdb64a..59af9a21 100644 --- a/spp_registry/models/reg_id.py +++ b/spp_registry/models/reg_id.py @@ -80,10 +80,76 @@ class SPPRegistrantID(models.Model): help="Raw response or notes from verification", ) - _unique_partner_id_type = models.Constraint( - "UNIQUE(partner_id, id_type_id)", - "A registrant cannot have duplicate ID types", - ) + # OP#1136: uniqueness applies to *live* IDs only. Removing an ID through a + # change request marks it Invalid rather than deleting it, and a plain + # UNIQUE(partner_id, id_type_id) counted those dead rows — so once an ID had + # been removed, that type could never be used again for that registrant. + # + # Enforced as a partial unique index rather than a table constraint, since + # the rule needs a WHERE clause. Note IS DISTINCT FROM, not != : a NULL + # status means an ID added straight through the registry, which is live and + # must still reserve its type. + # Declared rather than built with raw SQL in init(): models.UniqueIndex + # takes a WHERE clause, so the framework creates, drops and recreates the + # index as this definition changes, and there is no SQL string for the + # injection check to flag (OP#1136 review). The old unconditional + # constraint is dropped by migrations/19.0.2.2.0/pre-migration.py, since + # Odoo does not remove constraints that simply stop being declared. + _unique_active_id_type = models.UniqueIndex("(partner_id, id_type_id) WHERE status IS DISTINCT FROM 'invalid'") + + def _assert_id_type_free(self, partner_id, id_type_id, status, exclude_id=None): + """Raise unless this registrant has no live ID of that type. + + Checked ahead of the write rather than through ``@api.constrains``: + constraints run on flush, by which point the INSERT has already hit the + partial index and the user gets a raw database error instead of a + sentence. The index remains the race-safe guarantee; this is what makes + the refusal readable (OP#1136). + + ``status`` of ``invalid`` is a removed ID and never conflicts. A NULL + status is an ID added straight through the registry — live, and it does. + """ + if status == "invalid" or not partner_id or not id_type_id: + return + domain = [ + ("partner_id", "=", partner_id), + ("id_type_id", "=", id_type_id), + ("status", "!=", "invalid"), + ] + if exclude_id: + domain.append(("id", "!=", exclude_id)) + # Runs sudo because the rule is about the registrant's data, not the + # acting user's visibility: a clashing ID the user cannot read must + # still block the write, otherwise the partial index refuses the INSERT + # afterwards with a raw database error — the outcome this check exists to + # replace. Only the clashing row's type and registrant name are used, in + # the message the user already knows they are editing. + clash = self.sudo().search(domain, limit=1) # nosemgrep: odoo-sudo-without-context + if clash: + raise ValidationError( + _( + "%(registrant)s already has a valid %(id_type)s. Update the existing one, or remove it first.", + registrant=clash.partner_id.display_name, + id_type=clash.id_type_id.display_name, + ) + ) + + @api.model_create_multi + def create(self, vals_list): + for vals in vals_list: + self._assert_id_type_free(vals.get("partner_id"), vals.get("id_type_id"), vals.get("status")) + return super().create(vals_list) + + def write(self, vals): + if {"partner_id", "id_type_id", "status"} & set(vals): + for rec in self: + self._assert_id_type_free( + vals.get("partner_id", rec.partner_id.id), + vals.get("id_type_id", rec.id_type_id.id), + vals.get("status", rec.status), + exclude_id=rec.id, + ) + return super().write(vals) def _compute_available_id_type_ids(self): for rec in self: diff --git a/spp_registry/readme/HISTORY.md b/spp_registry/readme/HISTORY.md index 3eb56157..2dde423f 100644 --- a/spp_registry/readme/HISTORY.md +++ b/spp_registry/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.2.0 + +- fix(registry): let an ID type be used again after its ID was removed. Removing an ID through a change request keeps the row and marks it Invalid, and the old uniqueness rule counted those dead rows — so the registrant was left with an Invalid ID and no way to add a valid one of the same type. Uniqueness now applies to live IDs only, and is refused before the write so the message names the ID type rather than surfacing a database error (#1136) + ### 19.0.2.1.4 - fix(registry): remove the dead `@api.constrains("age")` `_check_age_is_integer` guard. `age` is a non-stored compute derived from `birthdate`, so the constraint never fired and only emitted the registry-load warning `@constrains parameter 'age' is not writeable`. Computed `age` values are unchanged; stale i18n entries for the removed message are dropped diff --git a/spp_registry/static/description/index.html b/spp_registry/static/description/index.html index 0e678633..26a37f53 100644 --- a/spp_registry/static/description/index.html +++ b/spp_registry/static/description/index.html @@ -518,6 +518,18 @@

    Changelog

+

19.0.2.2.0

+
    +
  • fix(registry): let an ID type be used again after its ID was removed. +Removing an ID through a change request keeps the row and marks it +Invalid, and the old uniqueness rule counted those dead rows — so the +registrant was left with an Invalid ID and no way to add a valid one +of the same type. Uniqueness now applies to live IDs only, and is +refused before the write so the message names the ID type rather than +surfacing a database error (#1136)
    • +
+
+

19.0.2.1.4

  • fix(registry): remove the dead @api.constrains("age") @@ -529,7 +541,7 @@

    19.0.2.1.4

    dropped
-
+

19.0.2.1.3

  • fix(registry): show an ID Status column on the group form @@ -540,7 +552,7 @@

    19.0.2.1.3

    (#1110)
-
+

19.0.2.1.1

  • fix(views): add reusable x2many_no_padding JS widget that @@ -550,7 +562,7 @@

    19.0.2.1.1

    don’t bloat the layout (#943).
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_registry/tests/test_reg_id.py b/spp_registry/tests/test_reg_id.py index 161b5bcd..85b47cf1 100644 --- a/spp_registry/tests/test_reg_id.py +++ b/spp_registry/tests/test_reg_id.py @@ -317,6 +317,131 @@ def test_different_partners_same_type_allowed(self): ) self.assertTrue(rec.id) + # ── OP#1136: an Invalid ID must not reserve its type forever ── + + def test_new_id_allowed_when_existing_one_is_invalid(self): + """The reported bug. + + Removing an ID through a change request marks it Invalid rather than + deleting it. The uniqueness rule counted that dead row, so the type + could never be used again for that registrant. + """ + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "removed-via-cr", + "status": "invalid", + } + ) + + replacement = self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "the-new-one", + "status": "valid", + } + ) + + self.env.flush_all() + self.assertTrue(replacement.id) + + def test_second_valid_id_of_same_type_still_rejected(self): + """A live ID still reserves its type.""" + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "the-live-one", + "status": "valid", + } + ) + + with self.assertRaises(ValidationError): + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "a-second-one", + "status": "valid", + } + ) + self.env.flush_all() + + def test_id_with_no_status_still_reserves_its_type(self): + """IDs added straight through the registry carry no status. + + Those are live records, not removed ones, so they must keep blocking a + duplicate — otherwise the fix would open a hole for every ID that was + never touched by a change request. + """ + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "added-in-the-registry", + } + ) + + with self.assertRaises(ValidationError): + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "a-duplicate", + } + ) + self.env.flush_all() + + def test_two_invalid_ids_of_the_same_type_are_tolerated(self): + """Successive removals leave more than one dead row behind.""" + for value in ("first-removed", "second-removed"): + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": value, + "status": "invalid", + } + ) + self.env.flush_all() + + replacement = self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "current", + "status": "valid", + } + ) + self.env.flush_all() + self.assertTrue(replacement.id) + + def test_reviving_an_invalid_id_when_a_valid_one_exists_is_rejected(self): + """Flipping a dead row back to valid must not create two live ones.""" + dead = self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "removed", + "status": "invalid", + } + ) + self.RegId.create( + { + "partner_id": self.individual_a.id, + "id_type_id": self.id_type_national.id, + "value": "live", + "status": "valid", + } + ) + self.env.flush_all() + + with self.assertRaises(ValidationError): + dead.write({"status": "valid"}) + self.env.flush_all() + @tagged("post_install", "-at_install") class TestNameSearch(RegIdCommon):