fix(registry): let a removed ID type be used again

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
 ~~~~~~~~~~
 

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",

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.

spp_change_request_v2/static/description/index.html

@@ -1339,6 +1339,15 @@ <h2>Changelog</h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.3.1.2</h1>
+<ul class="simple">
+<li>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)</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.3.1.1</h1>
 <ul class="simple">
 <li>fix(change_request): enforce the <tt class="docutils literal">(cr_type_id, reason)</tt> uniqueness
@@ -1352,7 +1361,7 @@ <h1>19.0.3.1.1</h1>
 applied) so the constraint applies cleanly on upgrade.</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.3.1.0</h1>
 <ul class="simple">
 <li>revert(change_request): restore the create-a-new-individual <strong>Add
@@ -1370,7 +1379,7 @@ <h1>19.0.3.1.0</h1>
 <strong>not</strong> restored here; reinstate separately if needed.</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.3.0.0</h1>
 <ul class="simple">
 <li>feat(change_request): redesign the group/membership CR flows (#242) —
@@ -1392,7 +1401,7 @@ <h1>19.0.3.0.0</h1>
 must adapt (see #1133).</li>
 </ul>
 </div>
-<div class="section" id="section-4">
+<div class="section" id="section-5">
 <h1>19.0.2.0.8</h1>
 <ul class="simple">
 <li>fix(views): disable inline creation of CR document types on the Change
@@ -1403,7 +1412,7 @@ <h1>19.0.2.0.8</h1>
 Documents” modal (missing Name field) that blocked saving (#1125)</li>
 </ul>
 </div>
-<div class="section" id="section-5">
+<div class="section" id="section-6">
 <h1>19.0.2.0.7</h1>
 <ul class="simple">
 <li>fix(security): align CR Requestor / CR Local Validator / CR HQ
@@ -1415,7 +1424,7 @@ <h1>19.0.2.0.7</h1>
 dependencies.</li>
 </ul>
 </div>
-<div class="section" id="section-6">
+<div class="section" id="section-7">
 <h1>19.0.2.0.6</h1>
 <ul class="simple">
 <li>fix(views): route post-submit CRs (pending / approved / applied /
@@ -1430,7 +1439,7 @@ <h1>19.0.2.0.6</h1>
 list so row-click goes through the stage router.</li>
 </ul>
 </div>
-<div class="section" id="section-7">
+<div class="section" id="section-8">
 <h1>19.0.2.0.5</h1>
 <ul class="simple">
 <li>fix(security): add a global <tt class="docutils literal">ir.rule</tt> on <tt class="docutils literal">spp.change.request</tt> that
@@ -1443,27 +1452,27 @@ <h1>19.0.2.0.5</h1>
 roles).</li>
 </ul>
 </div>
-<div class="section" id="section-8">
+<div class="section" id="section-9">
 <h1>19.0.2.0.3</h1>
 <ul class="simple">
 <li>fix: add HTML escaping to all computed Html fields with
 <tt class="docutils literal">sanitize=False</tt> to prevent stored XSS (#50)</li>
 </ul>
 </div>
-<div class="section" id="section-9">
+<div class="section" id="section-10">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix: fix batch approval wizard line deletion (#130)</li>
 </ul>
 </div>
-<div class="section" id="section-10">
+<div class="section" id="section-11">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: skip field types before getattr and isolate detail prefetch
 (#129)</li>
 </ul>
 </div>
-<div class="section" id="section-11">
+<div class="section" id="section-12">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

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,
         )

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."""
 

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
 ~~~~~~~~~~
 

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",

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")

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:

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