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

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/models/reg_id.py

@@ -80,10 +80,80 @@
         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.
+    _UNIQUE_ACTIVE_INDEX = "spp_registry_id_active_id_type_uniq"
+
+    def init(self):
+        super().init()
+        # Drop the unconditional constraint this replaces. Odoo removes
+        # constraints it no longer finds declared, but an explicit drop keeps
+        # upgrades of existing databases predictable.
+        self.env.cr.execute(
+            "ALTER TABLE spp_registry_id DROP CONSTRAINT IF EXISTS spp_registry_id_unique_partner_id_type"
+        )
+        self.env.cr.execute(
+            f"""
+            CREATE UNIQUE INDEX IF NOT EXISTS {self._UNIQUE_ACTIVE_INDEX}
+                ON spp_registry_id (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))
+        clash = self.sudo().search(domain, limit=1)
+        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/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):