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..97a40b4e 100644 --- a/spp_api_v2/services/group_service.py +++ b/spp_api_v2/services/group_service.py @@ -124,7 +124,13 @@ def to_api_schema(self, group, extensions=None) -> dict[str, Any]: identifiers.append(ident_dict) if not identifiers: - raise ValidationError(f"Group {group.name} has no valid external identifiers") + _logger.warning( + "Skipping group (id=%s): no valid external identifiers. Created by uid=%s on %s.", + group.id, + group.create_uid.id if group.create_uid else "unknown", + group.create_date, + ) + return None # Build Group resource group_data = { diff --git a/spp_api_v2/services/individual_service.py b/spp_api_v2/services/individual_service.py index b50479c3..7317f57a 100644 --- a/spp_api_v2/services/individual_service.py +++ b/spp_api_v2/services/individual_service.py @@ -127,8 +127,13 @@ def to_api_schema(self, partner, extensions=None) -> dict[str, Any]: identifiers.append(identifier) if not identifiers: - # Must have at least one identifier per spec - raise ValidationError(f"Partner {partner.name} has no valid external identifiers") + _logger.warning( + "Skipping individual (id=%s): no valid external identifiers. Created by uid=%s on %s.", + partner.id, + partner.create_uid.id if partner.create_uid else "unknown", + partner.create_date, + ) + return None # Build name (REQUIRED) name = { @@ -749,7 +754,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..968cd2ad 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,16 +18,19 @@ 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 group = membership.group - group_id = group.reg_ids[0] if group.reg_ids else None + group_id = next((r for r in group.reg_ids if r.namespace_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 external identifiers.", + membership.id, + group.id, + ) + return None group_ref = { "reference": f"Group/{group_id.namespace_uri}|{group_id.value}", @@ -35,9 +39,14 @@ def membership_to_response(membership) -> dict[str, Any]: # Build individual reference individual = membership.individual - individual_id = individual.reg_ids[0] if individual.reg_ids else None + individual_id = next((r for r in individual.reg_ids if r.namespace_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 external identifiers.", + membership.id, + individual.id, + ) + return None individual_ref = { "reference": f"Individual/{individual_id.namespace_uri}|{individual_id.value}", 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..e22c3f8b 100644 --- a/spp_api_v2/tests/test_group_service.py +++ b/spp_api_v2/tests/test_group_service.py @@ -179,8 +179,8 @@ 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.assertIsNone(result) def test_from_api_schema_creates_vals(self): """from_api_schema converts API schema to Odoo vals""" @@ -322,6 +322,67 @@ def test_to_api_schema_no_members(self): self.assertNotIn("member", data) self.assertNotIn("quantity", data) + def test_to_api_schema_member_without_identifiers_skipped(self): + """Members without valid identifiers are skipped from member list""" + # Create an individual without registry IDs + 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 (bypassing the API which would validate) + self.env["spp.group.membership"].create( + { + "group": group.id, + "individual": no_id_individual.id, + } + ) + + data = self.service.to_api_schema(group) + + # Member without identifiers should be skipped + if "member" in data: + for member in data["member"]: + self.assertNotIn( + "No ID Member", + member.get("entity", {}).get("display", ""), + ) + + def test_to_api_schema_member_with_empty_value_identifier_skipped(self): + """Members with reg_ids lacking value are skipped from member list""" + # 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 the registry ID to simulate invalid identifier + for reg_id in ind_with_bad_id.reg_ids: + 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 with empty-value identifier should be skipped + if "member" in data: + for member in data["member"]: + self.assertNotIn( + "Bad Value Member", + member.get("entity", {}).get("display", ""), + ) + 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..8b088440 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 @@ -189,8 +187,8 @@ 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.assertIsNone(result) 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"