diff --git a/spp_api_v2/__init__.py b/spp_api_v2/__init__.py index f179455e..31302aae 100644 --- a/spp_api_v2/__init__.py +++ b/spp_api_v2/__init__.py @@ -33,3 +33,41 @@ def _post_init_hook(env): if endpoint: endpoint.action_sync_registry() _logger.info("Synced FastAPI endpoint registry for spp_api_v2") + + # Backfill system_id for existing registrants that don't have one + _backfill_system_ids(env) + + +def _backfill_system_ids(env): + """Assign system_id to all existing registrants missing one.""" + import logging + import uuid + + _logger = logging.getLogger(__name__) + + system_id_type = env.ref("spp_api_v2.code_id_type_system_id", raise_if_not_found=False) + if not system_id_type: + return + + RegistryId = env["spp.registry.id"].sudo() # nosemgrep: odoo-sudo-without-context + # nosemgrep: odoo-sudo-without-context, odoo-sudo-on-sensitive-models + registrants = env["res.partner"].sudo().search([("is_registrant", "=", True)]) + + # Find registrants that already have a system_id + existing = RegistryId.search([("id_type_id", "=", system_id_type.id)]) + has_system_id = {r.partner_id.id for r in existing} + + to_create = [] + for partner in registrants: + if partner.id not in has_system_id: + to_create.append( + { + "partner_id": partner.id, + "id_type_id": system_id_type.id, + "value": str(uuid.uuid4()), + } + ) + + if to_create: + RegistryId.create(to_create) + _logger.info("Backfilled system_id for %d registrants", len(to_create)) diff --git a/spp_api_v2/__manifest__.py b/spp_api_v2/__manifest__.py index 0dc19b9a..edea566a 100644 --- a/spp_api_v2/__manifest__.py +++ b/spp_api_v2/__manifest__.py @@ -25,6 +25,7 @@ "security/privileges.xml", "security/groups.xml", "security/ir.model.access.csv", + "data/system_id_type.xml", "data/config_data.xml", "data/fastapi_endpoint.xml", "data/api_path_data.xml", @@ -39,6 +40,7 @@ "views/consent_views.xml", "views/api_outgoing_log_views.xml", "views/menu.xml", + "views/reg_id_system_views.xml", ], "assets": {}, "demo": [], diff --git a/spp_api_v2/data/system_id_type.xml b/spp_api_v2/data/system_id_type.xml new file mode 100644 index 00000000..dc4ab2f6 --- /dev/null +++ b/spp_api_v2/data/system_id_type.xml @@ -0,0 +1,22 @@ + + + + + + + system_id + System ID + both + Auto-generated unique identifier for API addressability. Not an identity document. + 0 + + diff --git a/spp_api_v2/models/__init__.py b/spp_api_v2/models/__init__.py index d42209ce..22e45f5f 100644 --- a/spp_api_v2/models/__init__.py +++ b/spp_api_v2/models/__init__.py @@ -11,3 +11,5 @@ from . import fastapi_endpoint_registry from . import ir_http_patch from . import res_partner_mobile +from . import res_partner_system_id +from . import spp_registry_id_system diff --git a/spp_api_v2/models/res_partner_system_id.py b/spp_api_v2/models/res_partner_system_id.py new file mode 100644 index 00000000..c34b4556 --- /dev/null +++ b/spp_api_v2/models/res_partner_system_id.py @@ -0,0 +1,65 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Auto-assign system ID to registrants for API addressability.""" + +import logging +import uuid + +from odoo import api, models + +_logger = logging.getLogger(__name__) + + +class ResPartnerSystemId(models.Model): + _inherit = "res.partner" + + @api.model_create_multi + def create(self, vals_list): + """Auto-assign a system_id registry ID to new registrants. + + Every registrant needs at least one identifier so the API can + address it. Identity documents (national_id, passport) may be + added later or not at all. The system_id is a stable UUID that + fills this gap without exposing internal database IDs. + """ + partners = super().create(vals_list) + self._assign_system_ids(partners) + return partners + + def _assign_system_ids(self, partners): + """Create system_id registry entries for registrants that lack one.""" + system_id_type = self._get_system_id_type() + if not system_id_type: + return + + RegistryId = self.env["spp.registry.id"].sudo() # nosemgrep: odoo-sudo-without-context + + for partner in partners: + if not partner.is_registrant: + continue + + # Check if already has a system_id + existing = RegistryId.search( + [ + ("partner_id", "=", partner.id), + ("id_type_id", "=", system_id_type.id), + ], + limit=1, + ) + if existing: + continue + + RegistryId.create( + { + "partner_id": partner.id, + "id_type_id": system_id_type.id, + "value": str(uuid.uuid4()), + } + ) + + def _get_system_id_type(self): + """Retrieve the system_id vocabulary code, or None if not installed.""" + try: + return self.env.ref("spp_api_v2.code_id_type_system_id") + except ValueError: + _logger.warning("system_id vocabulary code not found — skipping auto-assignment") + return None diff --git a/spp_api_v2/models/spp_registry_id_system.py b/spp_api_v2/models/spp_registry_id_system.py new file mode 100644 index 00000000..c1a6bd79 --- /dev/null +++ b/spp_api_v2/models/spp_registry_id_system.py @@ -0,0 +1,14 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Hide system_id from the ID type dropdown in the UI.""" + +from odoo import models + + +class SPPRegistryIdSystem(models.Model): + _inherit = "spp.registry.id" + + def _compute_available_id_type_ids(self): # pylint: disable=missing-return + """Exclude system_id from the dropdown — it is auto-assigned, not user-selectable.""" + super()._compute_available_id_type_ids() + for rec in self: + rec.available_id_type_ids = rec.available_id_type_ids.filtered(lambda c: c.code != "system_id") diff --git a/spp_api_v2/routers/bulk.py b/spp_api_v2/routers/bulk.py index e701480d..6769ac71 100644 --- a/spp_api_v2/routers/bulk.py +++ b/spp_api_v2/routers/bulk.py @@ -119,6 +119,8 @@ async def bulk_export( # Convert to API schema data = service.to_api_schema(record, extensions=extension_list) + if data is None: # pragma: no cover — record has no valid identifiers + continue # Apply consent filtering filtered_data = consent_service.filter_response( diff --git a/spp_api_v2/routers/filter.py b/spp_api_v2/routers/filter.py index 7b00137c..5ca0e133 100644 --- a/spp_api_v2/routers/filter.py +++ b/spp_api_v2/routers/filter.py @@ -195,6 +195,8 @@ async def search( for record in records: last_record_id = record.id data = service.to_api_schema(record, extensions=extension_list) + if data is None: # pragma: no cover — record has no valid identifiers + continue if consent_type: partner_id = record.id if resource_config["model"] == "res.partner" else None diff --git a/spp_api_v2/routers/group.py b/spp_api_v2/routers/group.py index 92615914..e856a242 100644 --- a/spp_api_v2/routers/group.py +++ b/spp_api_v2/routers/group.py @@ -102,6 +102,11 @@ async def read_group( # Convert to API schema extension_list = extensions.split(",") if extensions else None data = service.to_api_schema(group, extensions=extension_list) + if data is None: # pragma: no cover — safety net; identifier lookup above would 404 first + raise HTTPException( + status_code=404, + detail="Group not found", + ) # Apply consent filtering consent_service = ConsentService(env) @@ -203,6 +208,8 @@ def search_function(offset, limit): def consent_filter_function(group): group_data = group_service.to_api_schema(group, extensions=extension_list) + if group_data is None: + return None filtered_data = consent_service.filter_response(group.id, api_client, "group", group_data) consent_info = filtered_data.pop("_consent", None) if consent_info and consent_info.get("status") in ( diff --git a/spp_api_v2/routers/individual.py b/spp_api_v2/routers/individual.py index 9916edfe..13b6d9ff 100644 --- a/spp_api_v2/routers/individual.py +++ b/spp_api_v2/routers/individual.py @@ -98,6 +98,11 @@ async def read_individual( # Convert to API schema extension_list = extensions.split(",") if extensions else None data = service.to_api_schema(partner, extensions=extension_list) + if data is None: # pragma: no cover — safety net; identifier lookup above would 404 first + raise HTTPException( + status_code=404, + detail="Individual not found", + ) # Apply consent filtering consent_service = ConsentService(env) @@ -218,6 +223,8 @@ def search_function(offset, limit): def consent_filter_function(partner): data = individual_service.to_api_schema(partner, extensions=extension_list) + if data is None: + return None filtered_data = consent_service.filter_response(partner.id, api_client, "individual", data) consent_info = filtered_data.pop("_consent", None) if consent_info and consent_info.get("status") in ( diff --git a/spp_api_v2/routers/program_membership.py b/spp_api_v2/routers/program_membership.py index 4830ca50..bc96bb2d 100644 --- a/spp_api_v2/routers/program_membership.py +++ b/spp_api_v2/routers/program_membership.py @@ -72,6 +72,11 @@ async def read_program_membership( # Convert to API schema data = service.to_api_schema(membership) + if data is None: # pragma: no cover — safety net; identifier lookup above would 404 first + raise HTTPException( + status_code=404, + detail="ProgramMembership not found", + ) # Apply consent filtering for the beneficiary consent_service = ConsentService(env) @@ -164,6 +169,8 @@ def search_function(offset, limit): def consent_filter_function(membership): try: data = service.to_api_schema(membership) + if data is None: + return None filtered_data = consent_service.filter_response( membership.partner_id.id, api_client, "program_membership", data ) diff --git a/spp_api_v2/services/group_service.py b/spp_api_v2/services/group_service.py index d9982c5a..22afd151 100644 --- a/spp_api_v2/services/group_service.py +++ b/spp_api_v2/services/group_service.py @@ -108,8 +108,9 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: if not group: return {} - # Build identifier list + # Build identifier list — system_id sorted last so real IDs take precedence identifiers = [] + system_id_entry = None for reg_id in group.reg_ids: # Use id_type_id.uri for full code URI (e.g., urn:openspp:vocab:id-type#household_id) # NOT namespace_uri which only returns vocabulary namespace @@ -121,10 +122,19 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: # Only add period if it exists (not None) if hasattr(reg_id, "period") and reg_id.period: ident_dict["period"] = reg_id.period - identifiers.append(ident_dict) + if reg_id.id_type_id.code == "system_id": + system_id_entry = ident_dict + else: + identifiers.append(ident_dict) + if system_id_entry: + identifiers.append(system_id_entry) if not identifiers: - raise ValidationError(f"Group {group.name} has no valid external identifiers") + _logger.warning( + "Group (id=%s) has no identifiers — system_id may not have been assigned.", + group.id, + ) + return None # Build Group resource group_data = { @@ -203,8 +213,9 @@ def _build_member(self, membership) -> dict[str, Any]: if not individual or not individual.reg_ids: return None - # Build reference to individual - primary_id = individual.reg_ids[0] + # Build reference to individual — prefer non-system_id identifiers + non_system = [r for r in individual.reg_ids if r.id_type_id and r.id_type_id.code != "system_id" and r.value] + primary_id = non_system[0] if non_system else individual.reg_ids[0] entity_ref = { "reference": f"Individual/{primary_id.namespace_uri}|{primary_id.value}", "display": individual.name, @@ -1204,8 +1215,11 @@ def get_membership_history(self, group, limit=100, offset=0, since=None) -> list if not individual or not individual.reg_ids: continue - # Build individual reference - primary_id = individual.reg_ids[0] + # Build individual reference — prefer non-system_id identifiers + non_system = [ + r for r in individual.reg_ids if r.id_type_id and r.id_type_id.code != "system_id" and r.value + ] + primary_id = non_system[0] if non_system else individual.reg_ids[0] member_ref = Reference( reference=f"Individual/{primary_id.namespace_uri}|{primary_id.value}", display=individual.name, diff --git a/spp_api_v2/services/individual_service.py b/spp_api_v2/services/individual_service.py index b50479c3..72d049dc 100644 --- a/spp_api_v2/services/individual_service.py +++ b/spp_api_v2/services/individual_service.py @@ -114,21 +114,31 @@ def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: if not partner: return {} - # Build identifier list (REQUIRED, at least one) + # Build identifier list (REQUIRED, at least one). + # Sort so system_id comes last — real identity documents take precedence. identifiers = [] + system_id_entry = None for reg_id in partner.reg_ids: # Use id_type_id.uri for full code URI (e.g., urn:openspp:vocab:id-type#national_id) # NOT namespace_uri which only returns vocabulary namespace if reg_id.id_type_id and reg_id.id_type_id.uri and reg_id.value: - identifier = { + entry = { "system": reg_id.id_type_id.uri, "value": reg_id.value, } - identifiers.append(identifier) + if reg_id.id_type_id.code == "system_id": + system_id_entry = entry + else: + identifiers.append(entry) + if system_id_entry: + identifiers.append(system_id_entry) if not identifiers: - # Must have at least one identifier per spec - raise ValidationError(f"Partner {partner.name} has no valid external identifiers") + _logger.warning( + "Individual (id=%s) has no identifiers — system_id may not have been assigned.", + partner.id, + ) + return None # Build name (REQUIRED) name = { @@ -280,9 +290,10 @@ def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: def _build_group_reference(self, group) -> dict: """Build Reference to a Group""" - # Get primary identifier for group + # Get primary identifier for group — prefer non-system_id identifiers if group.reg_ids: - primary_id = group.reg_ids[0] + non_system = [r for r in group.reg_ids if r.id_type_id and r.id_type_id.code != "system_id" and r.value] + primary_id = non_system[0] if non_system else group.reg_ids[0] ref = f"Group/{primary_id.namespace_uri}|{primary_id.value}" else: # No identifier - this should not happen in a properly configured system @@ -749,7 +760,8 @@ def get_groups(self, individual, status: str | None = None, limit: int = 100) -> for membership in memberships: try: response = membership_to_response(membership) - results.append(response) + if response is not None: + results.append(response) except Exception as e: # Log error but continue processing other memberships # Use group/individual names instead of database ID diff --git a/spp_api_v2/services/membership_utils.py b/spp_api_v2/services/membership_utils.py index fba7ff8a..07d4e9b5 100644 --- a/spp_api_v2/services/membership_utils.py +++ b/spp_api_v2/services/membership_utils.py @@ -1,12 +1,13 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Shared membership utilities for API V2 services.""" +import logging from typing import Any -from odoo.exceptions import ValidationError +_logger = logging.getLogger(__name__) -def membership_to_response(membership) -> dict[str, Any]: +def membership_to_response(membership) -> dict[str, Any] | None: """ Convert spp.group.membership record to MembershipResponse schema. @@ -17,30 +18,54 @@ def membership_to_response(membership) -> dict[str, Any]: membership: spp.group.membership record Returns: - Dictionary matching MembershipResponse schema - - Raises: - ValidationError: If group or individual lacks external identifiers + Dictionary matching MembershipResponse schema, or None if + group or individual lacks valid external identifiers. """ - # Build group reference + # Build group reference — prefer non-system identifiers over system_id group = membership.group - group_id = group.reg_ids[0] if group.reg_ids else None + non_system = [ + r for r in group.reg_ids if r.id_type_id and r.id_type_id.uri and r.value and r.id_type_id.code != "system_id" + ] + group_id = ( + non_system[0] + if non_system + else next((r for r in group.reg_ids if r.id_type_id and r.id_type_id.uri and r.value), None) + ) if not group_id: - raise ValidationError(f"Group {group.name} has no valid external identifiers") + _logger.warning( + "Skipping membership (id=%s): group (id=%s) has no valid identifiers.", + membership.id, + group.id, + ) + return None group_ref = { - "reference": f"Group/{group_id.namespace_uri}|{group_id.value}", + "reference": f"Group/{group_id.id_type_id.uri}|{group_id.value}", "display": group.name, } - # Build individual reference + # Build individual reference — prefer non-system identifiers over system_id individual = membership.individual - individual_id = individual.reg_ids[0] if individual.reg_ids else None + non_system = [ + r + for r in individual.reg_ids + if r.id_type_id and r.id_type_id.uri and r.value and r.id_type_id.code != "system_id" + ] + individual_id = ( + non_system[0] + if non_system + else next((r for r in individual.reg_ids if r.id_type_id and r.id_type_id.uri and r.value), None) + ) if not individual_id: - raise ValidationError(f"Individual {individual.name} has no valid external identifiers") + _logger.warning( + "Skipping membership (id=%s): individual (id=%s) has no valid identifiers.", + membership.id, + individual.id, + ) + return None individual_ref = { - "reference": f"Individual/{individual_id.namespace_uri}|{individual_id.value}", + "reference": f"Individual/{individual_id.id_type_id.uri}|{individual_id.value}", "display": individual.name, } diff --git a/spp_api_v2/services/program_membership_service.py b/spp_api_v2/services/program_membership_service.py index 5625d3ee..e448ccea 100644 --- a/spp_api_v2/services/program_membership_service.py +++ b/spp_api_v2/services/program_membership_service.py @@ -266,9 +266,10 @@ def _build_beneficiary_reference(self, partner) -> dict: # Determine resource type resource_type = "Group" if partner.is_group else "Individual" - # Get primary identifier + # Get primary identifier — prefer non-system identifiers over system_id if partner.reg_ids: - primary_id = partner.reg_ids[0] + non_system = [r for r in partner.reg_ids if r.id_type_id and r.id_type_id.code != "system_id"] + primary_id = non_system[0] if non_system else partner.reg_ids[0] ref = f"{resource_type}/{primary_id.id_type_id.uri}|{primary_id.value}" else: # No identifier - this should not happen in a properly configured system diff --git a/spp_api_v2/tests/test_group_api.py b/spp_api_v2/tests/test_group_api.py index 7f5a5a6e..01999b69 100644 --- a/spp_api_v2/tests/test_group_api.py +++ b/spp_api_v2/tests/test_group_api.py @@ -134,6 +134,30 @@ def test_read_group_not_found(self): self.assertEqual(response.status_code, 404) + def test_read_group_no_identifiers_returns_404(self): + """GET group without valid identifiers returns 404""" + no_id_group = self.create_test_group( + name="Will Lose IDs Group", + identifier_value="WILL-LOSE-GRP-001", + ) + + no_consent_client = self.create_api_client( + name="No Consent Client For Group 404", + scopes=[{"resource": "group", "action": "read"}], + require_consent=False, + legal_basis="public_interest", + ) + token = self.generate_jwt_token(no_consent_client) + + # Delete all registry IDs to simulate missing identifiers + no_id_group.reg_ids.unlink() + + url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_household_id|WILL-LOSE-GRP-001" + response = self.url_open(url, headers=self._get_headers(token=token)) + + # Partner's identifier was deleted, so lookup by identifier returns 404 + self.assertEqual(response.status_code, 404) + def test_read_group_invalid_format(self): """GET with invalid identifier format returns 400""" url = f"{self.api_base_url}/INVALID-FORMAT" @@ -177,6 +201,28 @@ def test_search_groups_success(self): self.assertIn("total", data["meta"]) self.assertIn("data", data) + def test_search_skips_groups_without_identifiers(self): + """Search skips groups without valid identifiers instead of crashing""" + no_id = self.env["res.partner"].create( + { + "name": "No Identifiers Group Search", + "is_registrant": True, + "is_group": True, + } + ) + no_id.reg_ids.unlink() + + response = self.url_open(self.api_base_url, headers=self._get_headers()) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertIn("data", data) + for resource in data.get("data", []): + self.assertNotEqual( + resource.get("name", ""), + "No Identifiers Group Search", + ) + def test_search_by_name(self): """Search with name parameter filters results""" url = f"{self.api_base_url}?name=Smith" diff --git a/spp_api_v2/tests/test_group_service.py b/spp_api_v2/tests/test_group_service.py index 6d9047f5..4ba83e62 100644 --- a/spp_api_v2/tests/test_group_service.py +++ b/spp_api_v2/tests/test_group_service.py @@ -136,7 +136,9 @@ def test_to_api_schema_members(self): member = data["member"][0] self.assertIn("entity", member) - self.assertIn("IND-001", member["entity"]["reference"]) + # Reference uses primary identifier (national_id preferred over system_id) + ref = member["entity"]["reference"] + self.assertTrue(ref.startswith("Individual/"), f"Expected Individual/ prefix, got: {ref}") self.assertEqual(member["entity"]["display"], individual.name) self.assertIn("role", member) self.assertEqual(member["role"]["coding"][0]["code"], "head") @@ -168,9 +170,8 @@ def test_to_api_schema_metadata(self): self.assertIn("versionId", data["meta"]) self.assertIn("lastUpdated", data["meta"]) - def test_to_api_schema_missing_identifiers_raises(self): - """Group without identifiers raises ValidationError""" - # Create group without registry ID + def test_to_api_schema_auto_system_id(self): + """Group without explicit identifiers gets auto-assigned system_id""" group = self.env["res.partner"].create( { "name": "No Identifier", @@ -179,8 +180,10 @@ def test_to_api_schema_missing_identifiers_raises(self): } ) - with self.assertRaises(ValidationError): - self.service.to_api_schema(group) + result = self.service.to_api_schema(group) + self.assertIsNotNone(result) + self.assertTrue(len(result["identifier"]) > 0) + self.assertEqual(result["identifier"][0]["system"], "urn:openspp:vocab:id-type#system_id") def test_from_api_schema_creates_vals(self): """from_api_schema converts API schema to Odoo vals""" @@ -322,6 +325,63 @@ def test_to_api_schema_no_members(self): self.assertNotIn("member", data) self.assertNotIn("quantity", data) + def test_to_api_schema_member_without_explicit_identifiers_has_system_id(self): + """Members without explicit identifiers still appear via auto-assigned system_id""" + # Create an individual without explicit registry IDs — system_id auto-assigned + no_id_individual = self.env["res.partner"].create( + { + "name": "No ID Member", + "is_registrant": True, + "is_group": False, + } + ) + group = self.create_test_group(identifier_value="HH-MEMBER-SKIP") + + # Create membership directly + self.env["spp.group.membership"].create( + { + "group": group.id, + "individual": no_id_individual.id, + } + ) + + data = self.service.to_api_schema(group) + + # Member should be included via system_id + self.assertIn("member", data) + displays = [m.get("entity", {}).get("display", "") for m in data["member"]] + self.assertIn("No ID Member", displays) + + def test_to_api_schema_member_with_empty_value_identifier_still_has_system_id(self): + """Members with cleared non-system identifiers still appear via auto-assigned system_id""" + # Create individual with an identifier that has no value + ind_with_bad_id = self.create_test_individual( + name="Bad Value Member", + given_name="Bad", + family_name="Value", + identifier_value="TEMP-BAD-VAL", + ) + # Clear the value on non-system_id registry IDs to simulate invalid identifier + # (keep system_id intact — it's the fallback identifier) + for reg_id in ind_with_bad_id.reg_ids: + if not reg_id.id_type_id or reg_id.id_type_id.code != "system_id": + reg_id.value = False + + group = self.create_test_group(identifier_value="HH-MEMBER-BAD-VAL") + self.env["spp.group.membership"].create( + { + "group": group.id, + "individual": ind_with_bad_id.id, + } + ) + + data = self.service.to_api_schema(group) + + # Member should still appear because auto-assigned system_id provides a valid identifier + self.assertIn("member", data) + displays = [m.get("entity", {}).get("display", "") for m in data["member"]] + self.assertIn("Bad Value Member", displays) + def test_create_member_invalid_reference_ignored(self): """Invalid member references are logged and ignored""" schema = Group( diff --git a/spp_api_v2/tests/test_individual_api.py b/spp_api_v2/tests/test_individual_api.py index 8a513b06..df0c68c1 100644 --- a/spp_api_v2/tests/test_individual_api.py +++ b/spp_api_v2/tests/test_individual_api.py @@ -108,6 +108,34 @@ def test_read_individual_not_found(self): self.assertEqual(response.status_code, 404) + def test_read_individual_no_identifiers_returns_404(self): + """GET individual without valid identifiers returns 404""" + # Create individual with a known identifier, then remove all reg_ids + # so to_api_schema returns None and the router returns 404. + no_id_partner = self.create_test_individual( + name="Will Lose IDs", + given_name="Will", + family_name="LoseIDs", + identifier_value="WILL-LOSE-001", + ) + + no_consent_client = self.create_api_client( + name="No Consent Client For 404", + scopes=[{"resource": "individual", "action": "read"}], + require_consent=False, + legal_basis="public_interest", + ) + token = self.generate_jwt_token(no_consent_client) + + # Delete all registry IDs to simulate missing identifiers + no_id_partner.reg_ids.unlink() + + url = f"{self.api_base_url}/urn:openspp:vocab:id-type%23test_national_id|WILL-LOSE-001" + response = self.url_open(url, headers=self._get_headers(token=token)) + + # Partner's identifier was deleted, so lookup by identifier returns 404 + self.assertEqual(response.status_code, 404) + def test_read_individual_invalid_format(self): """GET with invalid identifier format returns 400""" url = f"{self.api_base_url}/INVALID-FORMAT" @@ -154,6 +182,32 @@ def test_search_individuals_success(self): self.assertIn("links", data) self.assertIn("total", data["meta"]) + def test_search_skips_individuals_without_identifiers(self): + """Search skips individuals without valid identifiers instead of crashing""" + # Create individual then remove all registry IDs + no_id = self.env["res.partner"].create( + { + "name": "No Identifiers Search", + "is_registrant": True, + "is_group": False, + } + ) + # Ensure no reg_ids exist + no_id.reg_ids.unlink() + + response = self.url_open(self.api_base_url, headers=self._get_headers()) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + # Should succeed — no-identifier record silently skipped + self.assertIn("data", data) + # Verify the no-identifier individual is not in results + for resource in data.get("data", []): + self.assertNotEqual( + resource.get("name", {}).get("given", ""), + "No Identifiers Search", + ) + def test_search_by_name(self): """Search with name parameter filters results""" url = f"{self.api_base_url}?name=Jane" diff --git a/spp_api_v2/tests/test_individual_service.py b/spp_api_v2/tests/test_individual_service.py index 60442ea2..24294d13 100644 --- a/spp_api_v2/tests/test_individual_service.py +++ b/spp_api_v2/tests/test_individual_service.py @@ -3,8 +3,6 @@ from datetime import date -from odoo.exceptions import ValidationError - from ..schemas.individual import Individual from ..schemas.patch import IndividualPatch from ..services.individual_service import IndividualService @@ -163,7 +161,8 @@ def test_to_api_schema_group_membership(self): self.assertIn("groupMembership", data) membership = data["groupMembership"][0] self.assertIn("group", membership) - self.assertIn("HH-001", membership["group"]["reference"]) + ref = membership["group"]["reference"] + self.assertTrue(ref.startswith("Group/"), f"Expected Group/ prefix, got: {ref}") self.assertEqual(membership["group"]["display"], "Test Household") self.assertIn("role", membership) self.assertEqual(membership["role"]["coding"][0]["code"], "head") @@ -178,9 +177,8 @@ def test_to_api_schema_metadata(self): self.assertIn("versionId", data["meta"]) self.assertIn("lastUpdated", data["meta"]) - def test_to_api_schema_missing_identifiers_raises(self): - """Partner without identifiers raises ValidationError""" - # Create partner without registry ID + def test_to_api_schema_auto_system_id(self): + """Partner without explicit identifiers gets auto-assigned system_id""" partner = self.env["res.partner"].create( { "name": "No Identifier", @@ -189,8 +187,10 @@ def test_to_api_schema_missing_identifiers_raises(self): } ) - with self.assertRaises(ValidationError): - self.service.to_api_schema(partner) + result = self.service.to_api_schema(partner) + self.assertIsNotNone(result) + self.assertTrue(len(result["identifier"]) > 0) + self.assertEqual(result["identifier"][0]["system"], "urn:openspp:vocab:id-type#system_id") def test_from_api_schema_creates_vals(self): """from_api_schema converts API schema to Odoo vals""" diff --git a/spp_api_v2/tests/test_program_membership_api.py b/spp_api_v2/tests/test_program_membership_api.py index 17612904..47a77e64 100644 --- a/spp_api_v2/tests/test_program_membership_api.py +++ b/spp_api_v2/tests/test_program_membership_api.py @@ -111,6 +111,28 @@ def test_search_program_memberships_success(self): self.assertIn("meta", data) self.assertIn("total", data["meta"]) + def test_search_skips_memberships_without_identifiers(self): + """Search skips memberships where beneficiary lacks identifiers""" + # Create individual, enroll, then remove identifiers + no_id_ind = self.create_test_individual( + identifier_value="TEMP-NO-ID", + given_name="NoId", + family_name="Beneficiary", + ) + self.create_test_membership( + partner=no_id_ind, + program=self.program, + state="enrolled", + ) + # Remove identifiers — to_api_schema will return None for this membership + no_id_ind.reg_ids.unlink() + + response = self.url_open(self.api_base_url, headers=self._get_headers()) + + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertIn("data", data) + def test_search_by_beneficiary_individual(self): """Search by beneficiary returns memberships for that individual""" url = f"{self.api_base_url}?beneficiary=Individual/urn:openspp:vocab:id-type%23test_national_id|ENROLL-001" diff --git a/spp_api_v2/views/reg_id_system_views.xml b/spp_api_v2/views/reg_id_system_views.xml new file mode 100644 index 00000000..328de5ba --- /dev/null +++ b/spp_api_v2/views/reg_id_system_views.xml @@ -0,0 +1,32 @@ + + + + + + res.partner.form.hide.system.id + res.partner + + + + [('id_type_id.code', '!=', 'system_id')] + + + [('id_type_id.code', '!=', 'system_id')] + + + + diff --git a/spp_api_v2_change_request/services/change_request_service.py b/spp_api_v2_change_request/services/change_request_service.py index e1db5971..b241c6b9 100644 --- a/spp_api_v2_change_request/services/change_request_service.py +++ b/spp_api_v2_change_request/services/change_request_service.py @@ -101,15 +101,23 @@ def find_registrant_by_identifier(self, system: str, value: str): return self.env["res.partner"] def get_primary_identifier(self, partner): - """Get primary external identifier for a partner.""" - if partner.reg_ids: - reg_id = partner.reg_ids[0] - return { - "system": reg_id.namespace_uri or "", - "value": reg_id.value or "", - "display": partner.name, - } - return None + """Get primary external identifier for a partner. + + Prefers non-system_id identifiers (system_id is an auto-assigned + fallback and should only be returned when no other identifier exists). + """ + if not partner.reg_ids: + return None + # Prefer non-system_id identifiers + reg_id = next( + (r for r in partner.reg_ids if not r.id_type_id or r.id_type_id.code != "system_id"), + partner.reg_ids[0], + ) + return { + "system": reg_id.namespace_uri or "", + "value": reg_id.value or "", + "display": partner.name, + } def to_api_schema(self, cr) -> dict[str, Any]: """ diff --git a/spp_api_v2_change_request/tests/test_change_request_service.py b/spp_api_v2_change_request/tests/test_change_request_service.py index 140dfa36..f75ceaa5 100644 --- a/spp_api_v2_change_request/tests/test_change_request_service.py +++ b/spp_api_v2_change_request/tests/test_change_request_service.py @@ -267,11 +267,11 @@ def test_to_api_schema_empty_recordset(self): empty = self.env["spp.change.request"] self.assertEqual(service.to_api_schema(empty), {}) - def test_to_api_schema_registrant_without_identifier(self): - """to_api_schema falls back to internal identifier when registrant has no reg_ids.""" + def test_to_api_schema_registrant_without_explicit_identifier(self): + """to_api_schema uses auto-assigned system_id when registrant has no explicit reg_ids.""" service = ChangeRequestService(self.env) - # Create registrant without external identifier + # Create registrant without explicit identifier (system_id auto-assigned) partner_no_id = self.partner_model.create( { "name": "No ID Registrant", @@ -287,8 +287,10 @@ def test_to_api_schema_registrant_without_identifier(self): ) data = service.to_api_schema(cr) - self.assertEqual(data["registrant"]["system"], "urn:openspp:internal") - self.assertTrue(data["registrant"]["value"].startswith("partner-")) + # Partner now auto-gets a system_id, so it uses that instead of internal fallback + self.assertEqual(data["registrant"]["system"], "urn:openspp:vocab:id-type") + # Value is a UUID from system_id auto-assignment + self.assertTrue(len(data["registrant"]["value"]) > 0) def test_to_api_schema_with_description_and_notes(self): """to_api_schema includes description and notes when set.""" @@ -596,8 +598,8 @@ def test_get_primary_identifier_with_reg_ids(self): self.assertEqual(result["value"], "TEST-123") self.assertEqual(result["display"], self.registrant.name) - def test_get_primary_identifier_without_reg_ids(self): - """get_primary_identifier returns None for partner without reg_ids.""" + def test_get_primary_identifier_without_explicit_reg_ids(self): + """get_primary_identifier returns auto-assigned system_id for partner without explicit reg_ids.""" service = ChangeRequestService(self.env) partner_no_id = self.partner_model.create( @@ -608,7 +610,10 @@ def test_get_primary_identifier_without_reg_ids(self): } ) result = service.get_primary_identifier(partner_no_id) - self.assertIsNone(result) + # Partner now auto-gets a system_id on creation + self.assertIsNotNone(result) + self.assertEqual(result["system"], "urn:openspp:vocab:id-type") + self.assertEqual(result["display"], "No ID Partner") # ────────────────────────────────────────────────────────────────────── # create with optional fields tests diff --git a/spp_dci_client_ibr/tests/test_ibr_service.py b/spp_dci_client_ibr/tests/test_ibr_service.py index 5f166f37..699da876 100644 --- a/spp_dci_client_ibr/tests/test_ibr_service.py +++ b/spp_dci_client_ibr/tests/test_ibr_service.py @@ -179,15 +179,15 @@ def test_check_duplication_no_partner(self, mock_client_class): service.check_duplication(None) @patch("odoo.addons.spp_dci_client_ibr.services.ibr_service.DCIClient") - def test_check_duplication_no_identifiers(self, mock_client_class): - """Test check_duplication raises error for partner without IDs.""" + def test_check_duplication_no_identifiers_non_registrant(self, mock_client_class): + """Test check_duplication raises error for non-registrant partner without IDs.""" from odoo.addons.spp_dci_client_ibr.services.ibr_service import IBRService - # Create partner without identifiers + # Create non-registrant partner (registrants auto-get system_id) partner_no_id = self.Partner.create( { "name": "No ID Person", - "is_registrant": True, + "is_registrant": False, } ) @@ -199,6 +199,29 @@ def test_check_duplication_no_identifiers(self, mock_client_class): self.assertIn("no identifiers configured", str(cm.exception)) + @patch("odoo.addons.spp_dci_client_ibr.services.ibr_service.DCIClient") + def test_check_duplication_registrant_without_explicit_ids_uses_system_id(self, mock_client_class): + """Test check_duplication proceeds using auto-assigned system_id for registrant.""" + from odoo.addons.spp_dci_client_ibr.services.ibr_service import IBRService + + # Registrants now auto-get system_id, so duplication check should proceed + partner_system_only = self.Partner.create( + { + "name": "System ID Only Person", + "is_registrant": True, + } + ) + + mock_client = MagicMock() + mock_client.search_by_id.return_value = self._create_mock_search_response(has_matches=False) + mock_client_class.return_value = mock_client + + service = IBRService(self.data_source, self.env) + result = service.check_duplication(partner_system_only) + + # Should proceed without error using the system_id + self.assertFalse(result["is_duplicate"]) + @patch("odoo.addons.spp_dci_client_ibr.services.ibr_service.DCIClient") def test_search_beneficiary_success(self, mock_client_class): """Test successful beneficiary search."""