From fe5e762b0052425c92e5bac242b3c1187d333944 Mon Sep 17 00:00:00 2001 From: emjay0921 Date: Mon, 10 Aug 2026 11:36:37 +0800 Subject: [PATCH] fix(registry): let a removed ID type be used again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing an ID through a change request keeps the record and marks it Invalid rather than deleting it. Two separate checks counted that dead row, so once an ID had been removed the registrant was left with an Invalid ID and no way to add a valid one of the same type — which defeats the point of the Remove action. Both had to change; fixing either alone still leaves the user stuck. UNIQUE(partner_id, id_type_id) on spp.registry.id becomes a partial unique index over rows WHERE status IS DISTINCT FROM 'invalid'. A table constraint cannot express the condition. IS DISTINCT FROM rather than != is what keeps a NULL status blocking: those are IDs added straight through the registry, which are live records, not removed ones. The change request add path searched for any ID of the type regardless of status and refused on the strength of it. It now looks for a live one, so the Remove-then-Add sequence the ticket describes completes. Uniqueness is asserted before the write rather than through @api.constrains. Constraints run on flush, by which point the INSERT has already hit the index and the user sees a raw psycopg UniqueViolation instead of a sentence naming the ID type. The index stays as the race-safe guarantee. Covers the spec exactly: a live ID still reserves its type, an ID with no status still reserves its type, successive removals leave several Invalid rows behind without blocking, and flipping an Invalid row back to valid while a live one exists is refused. Note for upgrades: init() drops the old constraint explicitly rather than relying on the ORM noticing it is no longer declared, so an existing database needs spp_registry upgraded, not merely restarted. OP#1136 --- spp_change_request_v2/strategies/update_id.py | 7 +- .../tests/test_update_id_strategy.py | 53 ++++++++ spp_registry/models/reg_id.py | 78 ++++++++++- spp_registry/tests/test_reg_id.py | 125 ++++++++++++++++++ 4 files changed, 258 insertions(+), 5 deletions(-) diff --git a/spp_change_request_v2/strategies/update_id.py b/spp_change_request_v2/strategies/update_id.py index 30f615420..ca8694daf 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 1c888e4ef..1654d1f9d 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/models/reg_id.py b/spp_registry/models/reg_id.py index d7bdb64a4..6a705b75a 100644 --- a/spp_registry/models/reg_id.py +++ b/spp_registry/models/reg_id.py @@ -80,10 +80,80 @@ 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. + _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: diff --git a/spp_registry/tests/test_reg_id.py b/spp_registry/tests/test_reg_id.py index 161b5bcd6..85b47cf1a 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):