From 3615c00238a9402da117649d7ca03cf5d597d30b Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 15 Dec 2025 09:48:45 +0000 Subject: [PATCH 01/32] First cut PDS search --- gateway-api/src/gateway_api/pds_search.py | 298 ++++++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 gateway-api/src/gateway_api/pds_search.py diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py new file mode 100644 index 00000000..76b97a25 --- /dev/null +++ b/gateway-api/src/gateway_api/pds_search.py @@ -0,0 +1,298 @@ +import uuid +from dataclasses import dataclass +from datetime import date, datetime, timezone +from typing import TypeAlias, cast + +import requests + +Result: TypeAlias = str | dict[str, "Result"] | list["Result"] + + +@dataclass +class SearchResults: + """ + Represents a single patient search result with only the fields requested: + - Given name(s) + - Family name + - NHS Number + - Current general practitioner ODS code + """ + + given_names: str + family_name: str + nhs_number: str + gp_ods_code: str | None + + +class PdsSearch: + """ + Simple client for PDS FHIR R4 patient search (GET /Patient). + + Usage: + + pds = PdsSearch( + auth_token="YOUR_ACCESS_TOKEN", + end_user_org_ods="A12345", + base_url="https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4", + ) + + result = pds.search_patient( + family="Smith", + given="John", + gender="male", + date_of_birth="1980-05-12", + postcode="SW1A1AA", + ) + + if result: + print(result) + """ + + # Defaults – adjust as needed + SANDBOX_URL = "https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4" + INT_URL = "https://int.api.service.nhs.uk/personal-demographics/FHIR/R4" + PROD_URL = "https://api.service.nhs.uk/personal-demographics/FHIR/R4" + + def __init__( + self, + auth_token: str, + end_user_org_ods: str, + base_url: str = SANDBOX_URL, + *, + nhsd_session_urid: str | None = None, + timeout: int = 10, + ) -> None: + """ + :param auth_token: OAuth2 bearer token (without 'Bearer ' prefix) + :param end_user_org_ods: NHSD End User Organisation ODS code + :param base_url: Base URL for the PDS API (one of SANDBOX_URL / INT_URL / + PROD_URL) + :param nhsd_session_urid: Optional NHSD-Session-URID (for healthcare worker + access) + :param timeout: Default timeout in seconds for HTTP calls + """ + self.auth_token = auth_token + self.end_user_org_ods = end_user_org_ods + self.base_url = base_url.rstrip("/") + self.nhsd_session_urid = nhsd_session_urid + self.timeout = timeout + + def _build_headers( + self, + *, + request_id: str | None = None, + correlation_id: str | None = None, + ) -> dict[str, str]: + """ + Build mandatory and optional headers for a PDS request. + """ + headers = { + "Authorization": f"Bearer {self.auth_token}", + "X-Request-ID": request_id or str(uuid.uuid4()), + "NHSD-End-User-Organisation-ODS": self.end_user_org_ods, + "Accept": "application/fhir+json", + } + + if self.nhsd_session_urid: + headers["NHSD-Session-URID"] = self.nhsd_session_urid + + if correlation_id: + headers["X-Correlation-ID"] = correlation_id + + return headers + + def search_patient( + self, + *, + family: str, + given: str, + gender: str, + date_of_birth: str, + postcode: str | None = None, + email: str | None = None, + phone: str | None = None, + # These options are exposed by the PDS API but we don't need them, + # at least for now + # fuzzy_match: bool = False, + # exact_match: bool = False, + # history: bool = False, + # max_results: Optional[int] = None, + request_id: str | None = None, + correlation_id: str | None = None, + timeout: int | None = None, + ) -> SearchResults | None: + """ + Perform a patient search using GET /Patient on the PDS FHIR R4 API. + + Assumes that the search will return a single matching patient. + + Required: + family Patient family name (surname) + given Patient given name + gender 'male' | 'female' | 'other' | 'unknown' + date_of_birth 'YYYY-MM-DD' (exact match, wrapped as eqYYYY-MM-DD) + + Optional: + postcode Patient postcode (address-postalcode) + email Patient email address + phone Patient phone number + fuzzy_match If True, sets _fuzzy-match=true + exact_match If True, sets _exact-match=true + history If True, sets _history=true + max_results Integer 1–50 + request_id Override X-Request-ID (otherwise auto-generated UUID) + correlation_id Optional X-Correlation-ID + timeout Per-call timeout (defaults to client-level timeout) + + Returns: + A single SearchResults instance if a patient is found, else None. + """ + + headers = self._build_headers( + request_id=request_id, + correlation_id=correlation_id, + ) + + params: dict[str, str] = { + "family": family, + "given": given, + "gender": gender, + # Exact DOB match + "birthdate": f"eq{date_of_birth}", + } + + if postcode: + # Use address-postalcode (address-postcode is deprecated) + params["address-postalcode"] = postcode + + if email: + params["email"] = email + + if phone: + params["phone"] = phone + + # # Optional flags + # if fuzzy_match: + # params["_fuzzy-match"] = "true" + # if exact_match: + # params["_exact-match"] = "true" + # if history: + # params["_history"] = "true" + # if max_results is not None: + # params["_max-results"] = max_results + + url = f"{self.base_url}/Patient" + + response = requests.get( + url, + headers=headers, + params=params, + timeout=timeout or self.timeout, + ) + + try: + response.raise_for_status() + except requests.HTTPError: + # TODO: This should log, or something + return None + + bundle = response.json() + return self._extract_single_search_result(bundle) + + # --------------- internal helpers for result extraction ----------------- + + @staticmethod + def _get_gp_ods_code(general_practitioners: list[dict[str, Result]]) -> str | None: + """ + Extract the current general practitioner ODS code from + Patient.generalPractitioner[].identifier.value, if present. + """ + if len(general_practitioners) == 0: + return None + + gp = find_current_record(general_practitioners) + if gp is None: + return None + + identifier = cast("dict[str, Result]", gp.get("identifier", {})) + ods_code = str(identifier.get("value", None)) + + return ods_code + + def _extract_single_search_result( + self, bundle: dict[str, Result] + ) -> SearchResults | None: + """ + Convert a FHIR Bundle from /Patient search into a single SearchResults + object by using the first entry. Returns None if there are no entries. + """ + entries: list[dict[str, Result]] = cast( + "list[dict[str, Result]]", bundle.get("entry", []) + ) # entries["entry"] is definitely a list + if not entries: + return None + + # Search can return multiple patients, except that for APIs it can only + # return one, so this is fine + entry = entries[0] + patient = cast("dict[str, Result]", entry.get("resource", {})) + + nhs_number = str(patient.get("id", "")).strip() + + # Pretty sure NHS number has to be there + if not nhs_number: + return None + + names = cast("list[dict[str, Result]]", patient.get("name", [])) + name_obj = find_current_record(names) + + if name_obj is None: + return None + + given_names_list = cast("list[str]", name_obj.get("given", [])) + family_name = str(name_obj.get("family", "")) or "" + + given_names_str = " ".join(given_names_list).strip() + + # TODO: What happens if the patient isn't registered with a GP so this is empty? + # Probably not for Alpha + gp_list = cast( + "list[dict[str, Result]]", patient.get("generalPractitioner", []) + ) + gp_ods_code = self._get_gp_ods_code(gp_list) + + return SearchResults( + given_names=given_names_str, + family_name=family_name, + nhs_number=nhs_number, + gp_ods_code=gp_ods_code, + ) + + +def find_current_record( + records: list[dict[str, Result]], today: date | None = None +) -> dict[str, Result] | None: + """ + records: list of dicts, each with period.start and period.end (ISO date strings). + today: optional date override (for testing); defaults to today's date. + Returns: the first dict whose period covers 'today', or None if no match. + """ + if today is None: + # TODO: Do we need to do something about UTC here? Do we need to use local time? + # Don't worry for Alpha + today = datetime.now(timezone.utc).date() + + for record in records: + periods = cast("dict[str, str]", record["period"]) + start_str = periods["start"] + end_str = periods["end"] + + # Parse ISO dates + start = date.fromisoformat(start_str) + end = date.fromisoformat(end_str) + + # Inclusive range check + if start <= today <= end: + return record + + return None From 64a41531ace574e48612c607700785d8e6f3bd04 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 15 Dec 2025 10:01:36 +0000 Subject: [PATCH 02/32] Alias repeated complicated type --- gateway-api/src/gateway_api/pds_search.py | 27 ++++++++++++----------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 76b97a25..afab93d5 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -5,7 +5,10 @@ import requests -Result: TypeAlias = str | dict[str, "Result"] | list["Result"] +ResultStructure: TypeAlias = ( + str | dict[str, "ResultStructure"] | list["ResultStructure"] +) +ResultList: TypeAlias = list[dict[str, ResultStructure]] @dataclass @@ -202,7 +205,7 @@ def search_patient( # --------------- internal helpers for result extraction ----------------- @staticmethod - def _get_gp_ods_code(general_practitioners: list[dict[str, Result]]) -> str | None: + def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: """ Extract the current general practitioner ODS code from Patient.generalPractitioner[].identifier.value, if present. @@ -214,20 +217,20 @@ def _get_gp_ods_code(general_practitioners: list[dict[str, Result]]) -> str | No if gp is None: return None - identifier = cast("dict[str, Result]", gp.get("identifier", {})) + identifier = cast("dict[str, ResultStructure]", gp.get("identifier", {})) ods_code = str(identifier.get("value", None)) return ods_code def _extract_single_search_result( - self, bundle: dict[str, Result] + self, bundle: dict[str, ResultStructure] ) -> SearchResults | None: """ Convert a FHIR Bundle from /Patient search into a single SearchResults object by using the first entry. Returns None if there are no entries. """ - entries: list[dict[str, Result]] = cast( - "list[dict[str, Result]]", bundle.get("entry", []) + entries: ResultList = cast( + "ResultList", bundle.get("entry", []) ) # entries["entry"] is definitely a list if not entries: return None @@ -235,7 +238,7 @@ def _extract_single_search_result( # Search can return multiple patients, except that for APIs it can only # return one, so this is fine entry = entries[0] - patient = cast("dict[str, Result]", entry.get("resource", {})) + patient = cast("dict[str, ResultStructure]", entry.get("resource", {})) nhs_number = str(patient.get("id", "")).strip() @@ -243,7 +246,7 @@ def _extract_single_search_result( if not nhs_number: return None - names = cast("list[dict[str, Result]]", patient.get("name", [])) + names = cast("ResultList", patient.get("name", [])) name_obj = find_current_record(names) if name_obj is None: @@ -256,9 +259,7 @@ def _extract_single_search_result( # TODO: What happens if the patient isn't registered with a GP so this is empty? # Probably not for Alpha - gp_list = cast( - "list[dict[str, Result]]", patient.get("generalPractitioner", []) - ) + gp_list = cast("ResultList", patient.get("generalPractitioner", [])) gp_ods_code = self._get_gp_ods_code(gp_list) return SearchResults( @@ -270,8 +271,8 @@ def _extract_single_search_result( def find_current_record( - records: list[dict[str, Result]], today: date | None = None -) -> dict[str, Result] | None: + records: ResultList, today: date | None = None +) -> dict[str, ResultStructure] | None: """ records: list of dicts, each with period.start and period.end (ISO date strings). today: optional date override (for testing); defaults to today's date. From 8fea4654e16d29d78d05bd8b13d4218fc71a796a Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:57:40 +0000 Subject: [PATCH 03/32] Add search by NHS number --- gateway-api/src/gateway_api/pds_search.py | 50 ++++++++++++++++++++++- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index afab93d5..d8ca2a03 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -104,9 +104,55 @@ def _build_headers( return headers - def search_patient( + def search_patient_by_nhs_number( + self, + nhs_number: int, + request_id: str | None = None, + correlation_id: str | None = None, + timeout: int | None = None, + ) -> SearchResults | None: + """ + Does a PDS search for the patient with the given NHS number and returns + a SearchResults object with the matching details. + + :param nhs_number: NHS number to search PDS for + :type nhs_number: int + :param request_id: If set re-use this request ID (if retrying a request) rather + than generating a new one. + :type request_id: str | None + :param correlation_id: Optional transaction correlation ID across multiple + systems; mirrored back in response. + :type correlation_id: str | None + :param timeout: Description + :type timeout: int | None + :return: Description + :rtype: SearchResults | None + """ + headers = self._build_headers( + request_id=request_id, + correlation_id=correlation_id, + ) + + url = f"{self.base_url}/Patient/{nhs_number}" + + response = requests.get( + url, + headers=headers, + params={}, + timeout=timeout or self.timeout, + ) + + try: + response.raise_for_status() + except requests.HTTPError: + # TODO: This should log, or something + return None + + bundle = response.json() + return self._extract_single_search_result(bundle) + + def search_patient_by_details( self, - *, family: str, given: str, gender: str, From 863a35f922d4655e0b9664bb576c312d20822d9c Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 19 Dec 2025 13:49:36 +0000 Subject: [PATCH 04/32] Remove unnecessary * arguments --- gateway-api/src/gateway_api/pds_search.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index d8ca2a03..a9992c84 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -61,7 +61,6 @@ def __init__( auth_token: str, end_user_org_ods: str, base_url: str = SANDBOX_URL, - *, nhsd_session_urid: str | None = None, timeout: int = 10, ) -> None: @@ -82,7 +81,6 @@ def __init__( def _build_headers( self, - *, request_id: str | None = None, correlation_id: str | None = None, ) -> dict[str, str]: From c3cfe62e72bafcaf2253a296fd89a40dca79c0f7 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 31 Dec 2025 09:57:57 +0000 Subject: [PATCH 05/32] First-cut PDS service stub --- gateway-api/stubs/stub_pds.py | 217 ++++++++++++++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 gateway-api/stubs/stub_pds.py diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stub_pds.py new file mode 100644 index 00000000..40126627 --- /dev/null +++ b/gateway-api/stubs/stub_pds.py @@ -0,0 +1,217 @@ +from __future__ import annotations + +import re +import uuid +from dataclasses import dataclass +from datetime import datetime, timezone +from typing import Any + + +@dataclass(frozen=True) +class StubResponse: + status_code: int + headers: dict[str, str] + json: dict[str, Any] + + +class PdsFhirApiStub: + """ + Minimal in-memory stub for the PDS FHIR API, implementing only GET /Patient/{id}. + + Contract elements modelled from the provided OpenAPI: + - Path: /Patient/{id} + - id is the patient's NHS number (10 digits, must be valid) + - X-Request-ID is mandatory and mirrored back in a response header + - X-Correlation-ID is optional and mirrored back if supplied + - ETag follows W/"" and corresponds to Patient.meta.versionId + + See uploaded OpenAPI for details. + """ + + def __init__(self, *, strict_headers: bool = True) -> None: + # strict_headers=True enforces X-Request-ID presence and UUID format. + self.strict_headers = strict_headers + # Internal store: nhs_number -> (patient_resource, version_id_int) + self._patients: dict[str, tuple[dict[str, Any], int]] = {} + + # Seed a deterministic example matching the spec's id example. + self.upsert_patient( + nhs_number="9000000009", + patient={ + "resourceType": "Patient", + "id": "9000000009", + "meta": { + "versionId": "1", + "lastUpdated": "2020-01-01T00:00:00Z", + }, + "identifier": [ + { + "system": "https://fhir.nhs.uk/Id/nhs-number", + "value": "9000000009", + } + ], + "name": [ + { + "use": "official", + "family": "Smith", + "given": ["Jane"], + } + ], + "gender": "female", + "birthDate": "1970-01-01", + }, + version_id=1, + ) + + # --------------------------- + # Public API for tests + # --------------------------- + + def upsert_patient( + self, nhs_number: str, patient: dict[str, Any], version_id: int = 1 + ) -> None: + """Add/replace a patient in the stub store.""" + patient = dict(patient) # shallow copy + patient.setdefault("resourceType", "Patient") + patient["id"] = nhs_number + patient.setdefault("meta", {}) + patient["meta"]["versionId"] = str(version_id) + patient["meta"].setdefault("lastUpdated", self._now_fhir_instant()) + self._patients[nhs_number] = (patient, version_id) + + def get_patient( + self, + nhs_number: str, + request_id: str | None = None, + correlation_id: str | None = None, + authorization: str | None = None, # noqa F841 # Ignored in stub + role_id: str | None = None, # noqa F841 # Ignored in stub + end_user_org_ods: str | None = None, # noqa F841 # Ignored in stub + ) -> StubResponse: + """ + Implements GET /Patient/{id}. + """ + headers_out: dict[str, str] = {} + + # Header handling (mirroring behavior). + if self.strict_headers: + if not request_id: + return self._bad_request( + "Missing X-Request-ID", + request_id=request_id, + correlation_id=correlation_id, + ) + if not self._is_uuid(request_id): + return self._bad_request( + "Invalid X-Request-ID (must be a UUID)", + request_id=request_id, + correlation_id=correlation_id, + ) + if request_id: + headers_out["X-Request-Id"] = request_id + if correlation_id: + headers_out["X-Correlation-Id"] = correlation_id + + # Path parameter validation: 10 digits and valid NHS number. + if not re.fullmatch( + r"\d{10}", nhs_number or "" + ) or not self._is_valid_nhs_number(nhs_number): + return self._operation_outcome( + status_code=400, + headers=headers_out, + spine_code="INVALID_RESOURCE_ID", + display="Resource Id is invalid", + ) + + # Lookup. + if nhs_number not in self._patients: + return self._operation_outcome( + status_code=404, + headers=headers_out, + spine_code="RESOURCE_NOT_FOUND", + display="Patient not found", + ) + + patient, version_id = self._patients[nhs_number] + + # ETag mirrors the "W/\"\"" shape and aligns to meta.versionId. + headers_out["ETag"] = f'W/"{version_id}"' + return StubResponse(status_code=200, headers=headers_out, json=patient) + + # --------------------------- + # Internal helpers + # --------------------------- + + @staticmethod + def _now_fhir_instant() -> str: + return ( + datetime.now(timezone.utc) + .replace(microsecond=0) + .isoformat() + .replace("+00:00", "Z") + ) + + @staticmethod + def _is_uuid(value: str) -> bool: + try: + uuid.UUID(value) + return True + except Exception: + return False + + @staticmethod + def _is_valid_nhs_number(nhs_number: str) -> bool: + """ + NHS number check-digit validation (mod 11). + Rejects cases where computed check digit is 10. + """ + digits = [int(c) for c in nhs_number] + total = sum(digits[i] * (10 - i) for i in range(9)) # weights 10..2 + remainder = total % 11 + check = 11 - remainder + if check == 11: + check = 0 + if check == 10: + return False + return digits[9] == check + + def _bad_request( + self, message: str, *, request_id: str | None, correlation_id: str | None + ) -> StubResponse: + headers: dict[str, str] = {} + if request_id: + headers["X-Request-Id"] = request_id + if correlation_id: + headers["X-Correlation-Id"] = correlation_id + return self._operation_outcome( + status_code=400, + headers=headers, + spine_code="INVALID_REQUEST", + display=message, + ) + + @staticmethod + def _operation_outcome( + *, status_code: int, headers: dict[str, str], spine_code: str, display: str + ) -> StubResponse: + # Matches the example structure shown in the OpenAPI file. + body = { + "resourceType": "OperationOutcome", + "issue": [ + { + "severity": "error", + "code": "value", + "details": { + "coding": [ + { + "system": "https://fhir.nhs.uk/R4/CodeSystem/Spine-ErrorOrWarningCode", + "version": "1", + "code": spine_code, + "display": display, + } + ] + }, + } + ], + } + return StubResponse(status_code=status_code, headers=dict(headers), json=body) From 7f367157383821399ae78a460e4d0bedb5455048 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 31 Dec 2025 10:10:23 +0000 Subject: [PATCH 06/32] Add init.py --- gateway-api/stubs/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 gateway-api/stubs/__init__.py diff --git a/gateway-api/stubs/__init__.py b/gateway-api/stubs/__init__.py new file mode 100644 index 00000000..e69de29b From 595950dc1ba5a696a2baff4492f6fcea2485e01a Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 31 Dec 2025 11:37:55 +0000 Subject: [PATCH 07/32] Updates to make the stub behave --- gateway-api/stubs/stub_pds.py | 41 +++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stub_pds.py index 40126627..152843df 100644 --- a/gateway-api/stubs/stub_pds.py +++ b/gateway-api/stubs/stub_pds.py @@ -28,7 +28,7 @@ class PdsFhirApiStub: See uploaded OpenAPI for details. """ - def __init__(self, *, strict_headers: bool = True) -> None: + def __init__(self, strict_headers: bool = True) -> None: # strict_headers=True enforces X-Request-ID presence and UUID format. self.strict_headers = strict_headers # Internal store: nhs_number -> (patient_resource, version_id_int) @@ -68,10 +68,23 @@ def __init__(self, *, strict_headers: bool = True) -> None: # --------------------------- def upsert_patient( - self, nhs_number: str, patient: dict[str, Any], version_id: int = 1 + self, nhs_number: str, patient: dict[str, Any] = None, version_id: int = 1 ) -> None: """Add/replace a patient in the stub store.""" - patient = dict(patient) # shallow copy + + try: + nhsnum_match = re.fullmatch(r"(\d{10})", nhs_number) + except TypeError as err: + raise TypeError("NHS Number must be a string") from err + + if not nhsnum_match: + raise ValueError("NHS Number must be exactly 10 digits") + + if not self._is_valid_nhs_number(nhs_number): + raise ValueError("NHS Number is not valid") + + patient = dict(patient) if patient is not None else {} + patient.setdefault("resourceType", "Patient") patient["id"] = nhs_number patient.setdefault("meta", {}) @@ -113,6 +126,7 @@ def get_patient( headers_out["X-Correlation-Id"] = correlation_id # Path parameter validation: 10 digits and valid NHS number. + if not re.fullmatch( r"\d{10}", nhs_number or "" ) or not self._is_valid_nhs_number(nhs_number): @@ -165,15 +179,18 @@ def _is_valid_nhs_number(nhs_number: str) -> bool: NHS number check-digit validation (mod 11). Rejects cases where computed check digit is 10. """ - digits = [int(c) for c in nhs_number] - total = sum(digits[i] * (10 - i) for i in range(9)) # weights 10..2 - remainder = total % 11 - check = 11 - remainder - if check == 11: - check = 0 - if check == 10: - return False - return digits[9] == check + # TODO: The AI did this. Check it's correct but also do we need this validation + # in the stub? In the mean time, just pass everything. + return True + # digits = [int(c) for c in nhs_number] + # total = sum(digits[i] * (10 - i) for i in range(9)) # weights 10..2 + # remainder = total % 11 + # check = 11 - remainder + # if check == 11: + # check = 0 + # if check == 10: + # return False + # return digits[9] == check def _bad_request( self, message: str, *, request_id: str | None, correlation_id: str | None From 06562f5a35d3bc297bbc825b0d3c9ec4b3a9eeb9 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 31 Dec 2025 11:39:13 +0000 Subject: [PATCH 08/32] Minor typing fix --- gateway-api/stubs/stub_pds.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stub_pds.py index 152843df..a4829f7c 100644 --- a/gateway-api/stubs/stub_pds.py +++ b/gateway-api/stubs/stub_pds.py @@ -68,7 +68,10 @@ def __init__(self, strict_headers: bool = True) -> None: # --------------------------- def upsert_patient( - self, nhs_number: str, patient: dict[str, Any] = None, version_id: int = 1 + self, + nhs_number: str, + patient: dict[str, Any] | None = None, + version_id: int = 1, ) -> None: """Add/replace a patient in the stub store.""" From 49934d23dc1b247d144fd3e6785c29333978ca48 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 2 Jan 2026 11:11:47 +0000 Subject: [PATCH 09/32] Updated some comments --- gateway-api/stubs/stub_pds.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stub_pds.py index a4829f7c..fb2307f4 100644 --- a/gateway-api/stubs/stub_pds.py +++ b/gateway-api/stubs/stub_pds.py @@ -18,14 +18,15 @@ class PdsFhirApiStub: """ Minimal in-memory stub for the PDS FHIR API, implementing only GET /Patient/{id}. - Contract elements modelled from the provided OpenAPI: + Contract elements modelled from the OpenAPI spec: - Path: /Patient/{id} - - id is the patient's NHS number (10 digits, must be valid) + - id is the patient's NHS number (10 digits) - X-Request-ID is mandatory and mirrored back in a response header - X-Correlation-ID is optional and mirrored back if supplied - ETag follows W/"" and corresponds to Patient.meta.versionId - See uploaded OpenAPI for details. + OpenAPI spec available from + https://github.com/NHSDigital/personal-demographics-service-api/blob/master/specification/personal-demographics.yaml """ def __init__(self, strict_headers: bool = True) -> None: @@ -73,7 +74,16 @@ def upsert_patient( patient: dict[str, Any] | None = None, version_id: int = 1, ) -> None: - """Add/replace a patient in the stub store.""" + """ + Add/replace a patient in the stub store. + + Arguments + nhs_number: String NHS number, ten digits + patient: Dictionary containing details of existing patient to replace + version_id: Version of patient. Increment this if replacing a patient, at least + if you want to be able to track it + + """ try: nhsnum_match = re.fullmatch(r"(\d{10})", nhs_number) From 5337a7a5b9a635e5f3bf53c0df06c35427b3f992 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 11:30:25 +0000 Subject: [PATCH 10/32] Added Pycharm config directory to git ignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 4b30addd..4a5191c9 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,7 @@ **/__pycache__ **/.ruff_cache **/.mypy_cache +**/.idea # Testing artefacts **/.hypothesis From 5f2587799b6f69f3fdda8585f9896a650411f9cc Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 11:31:06 +0000 Subject: [PATCH 11/32] Update imports. Still need to sort stubs. --- .../src/gateway_api/test_pds_search.py | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 gateway-api/src/gateway_api/test_pds_search.py diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py new file mode 100644 index 00000000..62fb525c --- /dev/null +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -0,0 +1,231 @@ +# tests/test_pds_search.py +from __future__ import annotations + +import re +from dataclasses import dataclass +from datetime import date, datetime, timezone +from typing import Any +from uuid import uuid4 + +import pytest +import requests +from stub_pds import PdsFhirApiStub + +import gateway_api.pds_search as pds_search +from gateway_api.pds_search import PdsSearch, find_current_record + + +@dataclass +class FakeResponse: + status_code: int + headers: dict[str, str] + _json: dict[str, Any] + + def json(self) -> dict[str, Any]: + return self._json + + def raise_for_status(self) -> None: + if self.status_code >= 400: + raise requests.HTTPError(f"{self.status_code} Error") + + +@pytest.fixture +def stub() -> PdsFhirApiStub: + # Strict header validation helps ensure PdsSearch sends X-Request-ID correctly. + return PdsFhirApiStub(strict_headers=True) + + +@pytest.fixture +def route_requests_get( + monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub +) -> dict[str, Any]: + """ + Patch requests.get so that calls made by PdsSearch are routed into the stub. + + Returns a capture dict that records the last request made. + """ + capture: dict[str, Any] = {} + + def _fake_get( + url: str, + headers: dict[str, str] | None = None, + params: Any = None, + timeout: Any = None, + ): + headers = headers or {} + capture["url"] = url + capture["headers"] = dict(headers) + capture["params"] = params + capture["timeout"] = timeout + + # Support only GET /Patient/{id} (as used by search_patient_by_nhs_number). + m = re.match(r"^(?P.+)/Patient/(?P\d+)$", url) + if not m: + raise AssertionError(f"Unexpected URL called by client: {url}") + + nhs_number = m.group("nhs") + + stub_resp = stub.get_patient( + nhs_number=nhs_number, + request_id=headers.get("X-Request-ID"), + correlation_id=headers.get("X-Correlation-ID"), + authorization=headers.get("Authorization"), + end_user_org_ods=headers.get("NHSD-End-User-Organisation-ODS"), + ) + + # pds_search.PdsSearch._extract_single_search_result expects a Bundle-like + # structure: {"entry": [{"resource": {...Patient...}}]} + if stub_resp.status_code == 200: + body = {"entry": [{"resource": stub_resp.json}]} + else: + body = stub_resp.json + + return FakeResponse( + status_code=stub_resp.status_code, headers=stub_resp.headers, _json=body + ) + + monkeypatch.setattr(requests, "get", _fake_get) + return capture + + +def test_search_patient_by_nhs_number_success_defaults( + route_requests_get: dict[str, Any], +) -> None: + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + result = client.search_patient_by_nhs_number(9000000009) + + assert result is not None + assert result.nhs_number == "9000000009" + assert result.family_name == "Smith" + assert result.given_names == "Jane" + assert result.gp_ods_code is None # default seeded patient has no GP in stub + + +def test_search_patient_by_nhs_number_sends_expected_headers( + route_requests_get: dict[str, Any], +) -> None: + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + req_id = str(uuid4()) + corr_id = "corr-123" + + result = client.search_patient_by_nhs_number( + 9000000009, + request_id=req_id, + correlation_id=corr_id, + ) + assert result is not None + + headers = route_requests_get["headers"] + assert headers["Authorization"] == "Bearer test-token" + assert headers["NHSD-End-User-Organisation-ODS"] == "A12345" + assert headers["Accept"] == "application/fhir+json" + assert headers["X-Request-ID"] == req_id + assert headers["X-Correlation-ID"] == corr_id + + +def test_search_patient_by_nhs_number_generates_request_id( + route_requests_get: dict[str, Any], +) -> None: + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + result = client.search_patient_by_nhs_number(9000000009) + assert result is not None + + # Ensure client generated an X-Request-ID (stub validates UUID shape in strict mode) + headers = route_requests_get["headers"] + assert "X-Request-ID" in headers + # A light sanity check; strict validation is performed inside the stub. + assert isinstance(headers["X-Request-ID"], str) + assert len(headers["X-Request-ID"]) >= 32 + + +def test_search_patient_by_nhs_number_not_found_returns_none( + stub: PdsFhirApiStub, + route_requests_get: dict[str, Any], +) -> None: + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + # Not seeded into stub store => 404 => raise_for_status => client returns None. + result = client.search_patient_by_nhs_number(9999999999) + assert result is None + + +def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( + monkeypatch: pytest.MonkeyPatch, + stub: PdsFhirApiStub, + route_requests_get: dict[str, Any], +) -> None: + # Freeze "today" inside pds_search.find_current_record by monkeypatching + # the imported datetime class in the module. + # TODO: Not used, check if can be removed. + # frozen_today = date(2026, 1, 2) + + class FrozenDateTime(datetime): + @classmethod + def now(cls, tz=None): # type: ignore[override] + return datetime(2026, 1, 2, 12, 0, 0, tzinfo=timezone.utc) + + monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) + + stub.upsert_patient( + nhs_number="9000000017", + patient={ + "resourceType": "Patient", + "name": [{"use": "official", "family": "Taylor", "given": ["Ben", "A."]}], + "generalPractitioner": [ + # Not current on 2026-01-02 + { + "period": {"start": "2010-01-01", "end": "2012-01-01"}, + "identifier": {"value": "OLDGP"}, + }, + # Current on 2026-01-02 + { + "period": {"start": "2020-01-01", "end": "2030-01-01"}, + "identifier": {"value": "CURRGP"}, + }, + ], + }, + version_id=1, + ) + + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + result = client.search_patient_by_nhs_number(9000000017) + assert result is not None + assert result.nhs_number == "9000000017" + assert result.family_name == "Taylor" + assert result.given_names == "Ben A." + assert result.gp_ods_code == "CURRGP" + + +def test_find_current_record_with_today_override() -> None: + records = [ + {"period": {"start": "2020-01-01", "end": "2020-12-31"}, "v": "a"}, + {"period": {"start": "2021-01-01", "end": "2021-12-31"}, "v": "b"}, + ] + + assert find_current_record(records, today=date(2020, 6, 1)) == records[0] + assert find_current_record(records, today=date(2021, 6, 1)) == records[1] + assert find_current_record(records, today=date(2019, 6, 1)) is None From 921f24567e9754f4010c8cf58e552cebd79b322d Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 12:13:51 +0000 Subject: [PATCH 12/32] Tests are passing --- gateway-api/src/gateway_api/pds_search.py | 48 +++++++++++++++++-- .../src/gateway_api/test_pds_search.py | 39 ++++++++++++--- gateway-api/stubs/stub_pds.py | 1 + 3 files changed, 76 insertions(+), 12 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index a9992c84..ce205fea 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -291,7 +291,7 @@ def _extract_single_search_result( return None names = cast("ResultList", patient.get("name", [])) - name_obj = find_current_record(names) + name_obj = find_current_name_record(names) if name_obj is None: return None @@ -318,9 +318,16 @@ def find_current_record( records: ResultList, today: date | None = None ) -> dict[str, ResultStructure] | None: """ - records: list of dicts, each with period.start and period.end (ISO date strings). - today: optional date override (for testing); defaults to today's date. - Returns: the first dict whose period covers 'today', or None if no match. + generalPractitioner selection. + + Each generalPractitioner record MUST contain: + - identifier.period.start (ISO date) + - identifier.period.end (ISO date) + + There may be zero records; if no record is current, return None. + Missing keys are treated as errors (KeyError), per contract. + + Returns: the first dict whose identifier.period covers 'today', or None. """ if today is None: # TODO: Do we need to do something about UTC here? Do we need to use local time? @@ -328,7 +335,8 @@ def find_current_record( today = datetime.now(timezone.utc).date() for record in records: - periods = cast("dict[str, str]", record["period"]) + identifier = cast("dict[str, ResultStructure]", record["identifier"]) + periods = cast("dict[str, str]", identifier["period"]) start_str = periods["start"] end_str = periods["end"] @@ -341,3 +349,33 @@ def find_current_record( return record return None + + +def find_current_name_record( + records: ResultList, today: date | None = None +) -> dict[str, ResultStructure] | None: + """ + Patient.name[] selection. + + Each name record MUST contain: + - period.start (ISO date) + - period.end (ISO date) + + Returns the first name record whose period covers 'today', else None. + Missing keys are treated as errors (KeyError), per contract. + """ + if today is None: + today = datetime.now(timezone.utc).date() + + for record in records: + periods = cast("dict[str, str]", record["period"]) + start_str = periods["start"] + end_str = periods["end"] + + start = date.fromisoformat(start_str) + end = date.fromisoformat(end_str) + + if start <= today <= end: + return record + + return None diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 62fb525c..bed58b4e 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -189,17 +189,32 @@ def now(cls, tz=None): # type: ignore[override] nhs_number="9000000017", patient={ "resourceType": "Patient", - "name": [{"use": "official", "family": "Taylor", "given": ["Ben", "A."]}], + "name": [ + { + "use": "official", + "family": "Taylor", + "given": ["Ben", "A."], + "period": {"start": "1900-01-01", "end": "9999-12-31"}, + } + ], "generalPractitioner": [ # Not current on 2026-01-02 { - "period": {"start": "2010-01-01", "end": "2012-01-01"}, - "identifier": {"value": "OLDGP"}, + "id": "1", + "type": "Organization", + "identifier": { + "value": "OLDGP", + "period": {"start": "2010-01-01", "end": "2012-01-01"}, + }, }, # Current on 2026-01-02 { - "period": {"start": "2020-01-01", "end": "2030-01-01"}, - "identifier": {"value": "CURRGP"}, + "id": "2", + "type": "Organization", + "identifier": { + "value": "CURRGP", + "period": {"start": "2020-01-01", "end": "2030-01-01"}, + }, }, ], }, @@ -222,8 +237,18 @@ def now(cls, tz=None): # type: ignore[override] def test_find_current_record_with_today_override() -> None: records = [ - {"period": {"start": "2020-01-01", "end": "2020-12-31"}, "v": "a"}, - {"period": {"start": "2021-01-01", "end": "2021-12-31"}, "v": "b"}, + { + "identifier": { + "value": "a", + "period": {"start": "2020-01-01", "end": "2020-12-31"}, + } + }, + { + "identifier": { + "value": "b", + "period": {"start": "2021-01-01", "end": "2021-12-31"}, + } + }, ] assert find_current_record(records, today=date(2020, 6, 1)) == records[0] diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stub_pds.py index fb2307f4..01253230 100644 --- a/gateway-api/stubs/stub_pds.py +++ b/gateway-api/stubs/stub_pds.py @@ -56,6 +56,7 @@ def __init__(self, strict_headers: bool = True) -> None: "use": "official", "family": "Smith", "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "9999-12-31"}, } ], "gender": "female", From 5f98da02f2ec4c0844bc51bfeda765ce6de646c0 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 16:04:51 +0000 Subject: [PATCH 13/32] Updated to new stubs location --- gateway-api/poetry.lock | 2 +- gateway-api/pyproject.toml | 5 ++--- gateway-api/src/gateway_api/test_pds_search.py | 2 +- gateway-api/stubs/{ => stubs}/__init__.py | 0 gateway-api/stubs/{ => stubs}/stub_pds.py | 0 5 files changed, 4 insertions(+), 5 deletions(-) rename gateway-api/stubs/{ => stubs}/__init__.py (100%) rename gateway-api/stubs/{ => stubs}/stub_pds.py (100%) diff --git a/gateway-api/poetry.lock b/gateway-api/poetry.lock index d125513f..338577d4 100644 --- a/gateway-api/poetry.lock +++ b/gateway-api/poetry.lock @@ -2243,4 +2243,4 @@ propcache = ">=0.2.1" [metadata] lock-version = "2.1" python-versions = ">3.13,<4.0.0" -content-hash = "ec013475c1d46e197243aad2c6ac8eb56957da032015c466694978937b784a61" +content-hash = "67e8839de72625c8f7c4d42aea6ea55afaf9f738aef2267bb4dac2f83a389f8e" diff --git a/gateway-api/pyproject.toml b/gateway-api/pyproject.toml index 3a5cd476..2242551f 100644 --- a/gateway-api/pyproject.toml +++ b/gateway-api/pyproject.toml @@ -7,14 +7,13 @@ authors = [ ] readme = "README.md" requires-python = ">3.13,<4.0.0" -dependencies = [ -] [tool.poetry.dependencies] clinical-data-common = { git = "https://github.com/NHSDigital/clinical-data-common.git", tag = "v0.1.0" } [tool.poetry] -packages = [{include = "gateway_api", from = "src"}] +packages = [{include = "gateway_api", from = "src"}, + {include = "stubs", from = "stubs"}] [tool.coverage.run] relative_files = true diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index bed58b4e..cb668adb 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -9,7 +9,7 @@ import pytest import requests -from stub_pds import PdsFhirApiStub +from stubs.stub_pds import PdsFhirApiStub import gateway_api.pds_search as pds_search from gateway_api.pds_search import PdsSearch, find_current_record diff --git a/gateway-api/stubs/__init__.py b/gateway-api/stubs/stubs/__init__.py similarity index 100% rename from gateway-api/stubs/__init__.py rename to gateway-api/stubs/stubs/__init__.py diff --git a/gateway-api/stubs/stub_pds.py b/gateway-api/stubs/stubs/stub_pds.py similarity index 100% rename from gateway-api/stubs/stub_pds.py rename to gateway-api/stubs/stubs/stub_pds.py From ecbd62e1097e7461aab852643a0623f03868c3eb Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 16:19:42 +0000 Subject: [PATCH 14/32] Add typing flag for stubs dir --- gateway-api/stubs/stubs/py.typed | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 gateway-api/stubs/stubs/py.typed diff --git a/gateway-api/stubs/stubs/py.typed b/gateway-api/stubs/stubs/py.typed new file mode 100644 index 00000000..e69de29b From 1943d1e916f5489c427a8fbbf19e6af7f22c246b Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 16:43:41 +0000 Subject: [PATCH 15/32] Bit of tidying up --- .../src/gateway_api/test_pds_search.py | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index cb668adb..c0afe415 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -3,8 +3,8 @@ import re from dataclasses import dataclass -from datetime import date, datetime, timezone -from typing import Any +from datetime import date, datetime, timezone, tzinfo +from typing import Any, cast from uuid import uuid4 import pytest @@ -12,7 +12,7 @@ from stubs.stub_pds import PdsFhirApiStub import gateway_api.pds_search as pds_search -from gateway_api.pds_search import PdsSearch, find_current_record +from gateway_api.pds_search import PdsSearch, ResultList, find_current_record @dataclass @@ -29,6 +29,14 @@ def raise_for_status(self) -> None: raise requests.HTTPError(f"{self.status_code} Error") +class FrozenDateTime(datetime): + @classmethod + def now(cls, tz: tzinfo | None = None) -> FrozenDateTime: + return cast( + "FrozenDateTime", datetime(2026, 1, 2, 12, 0, 0, tzinfo=timezone.utc) + ) + + @pytest.fixture def stub() -> PdsFhirApiStub: # Strict header validation helps ensure PdsSearch sends X-Request-ID correctly. @@ -51,7 +59,7 @@ def _fake_get( headers: dict[str, str] | None = None, params: Any = None, timeout: Any = None, - ): + ) -> FakeResponse: headers = headers or {} capture["url"] = url capture["headers"] = dict(headers) @@ -173,16 +181,6 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( stub: PdsFhirApiStub, route_requests_get: dict[str, Any], ) -> None: - # Freeze "today" inside pds_search.find_current_record by monkeypatching - # the imported datetime class in the module. - # TODO: Not used, check if can be removed. - # frozen_today = date(2026, 1, 2) - - class FrozenDateTime(datetime): - @classmethod - def now(cls, tz=None): # type: ignore[override] - return datetime(2026, 1, 2, 12, 0, 0, tzinfo=timezone.utc) - monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) stub.upsert_patient( @@ -236,20 +234,23 @@ def now(cls, tz=None): # type: ignore[override] def test_find_current_record_with_today_override() -> None: - records = [ - { - "identifier": { - "value": "a", - "period": {"start": "2020-01-01", "end": "2020-12-31"}, - } - }, - { - "identifier": { - "value": "b", - "period": {"start": "2021-01-01", "end": "2021-12-31"}, - } - }, - ] + records = cast( + "ResultList", + [ + { + "identifier": { + "value": "a", + "period": {"start": "2020-01-01", "end": "2020-12-31"}, + } + }, + { + "identifier": { + "value": "b", + "period": {"start": "2021-01-01", "end": "2021-12-31"}, + } + }, + ], + ) assert find_current_record(records, today=date(2020, 6, 1)) == records[0] assert find_current_record(records, today=date(2021, 6, 1)) == records[1] From 9a92ded61a730b75859343000cd9724b447384bd Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 5 Jan 2026 17:25:43 +0000 Subject: [PATCH 16/32] Add docstrings to tests --- .../src/gateway_api/test_pds_search.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index c0afe415..839bf322 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -99,6 +99,11 @@ def _fake_get( def test_search_patient_by_nhs_number_success_defaults( route_requests_get: dict[str, Any], ) -> None: + """ + Verifies that a successful GET /Patient/{nhs_number} returns a parsed SearchResults + populated from the stubbed seeded patient, and that gp_ods_code is None when no + current generalPractitioner record exists. + """ client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -117,6 +122,11 @@ def test_search_patient_by_nhs_number_success_defaults( def test_search_patient_by_nhs_number_sends_expected_headers( route_requests_get: dict[str, Any], ) -> None: + """ + Verifies that search_patient_by_nhs_number forwards the expected HTTP headers, + including Authorization, end-user ODS header, Accept, and caller-provided + X-Request-ID / X-Correlation-ID values. + """ client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -144,6 +154,11 @@ def test_search_patient_by_nhs_number_sends_expected_headers( def test_search_patient_by_nhs_number_generates_request_id( route_requests_get: dict[str, Any], ) -> None: + """ + Verifies that when no request_id is provided by the caller, PdsSearch generates + an X-Request-ID and includes it in the outbound request (the stub validates the + header format in strict mode). + """ client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -165,6 +180,11 @@ def test_search_patient_by_nhs_number_not_found_returns_none( stub: PdsFhirApiStub, route_requests_get: dict[str, Any], ) -> None: + """ + Verifies that when the downstream PDS endpoint returns 404 (patient not found), + the client handles the HTTP error path and returns None rather than propagating + an exception to the caller. + """ client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -181,6 +201,11 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( stub: PdsFhirApiStub, route_requests_get: dict[str, Any], ) -> None: + """ + Verifies that gp_ods_code is extracted from the *current* generalPractitioner + record, where "current" is determined by identifier.period covering today's + date. Time is frozen via monkeypatch to ensure deterministic selection. + """ monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) stub.upsert_patient( @@ -234,6 +259,11 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( def test_find_current_record_with_today_override() -> None: + """ + Verifies that find_current_record honours the explicit `today` override and + selects the first record whose identifier.period covers that date, returning + None when no record is current for the supplied date. + """ records = cast( "ResultList", [ From c3a179ab6a4706b3854fa95089a0656a1e2eeeed Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Tue, 6 Jan 2026 23:08:32 +0000 Subject: [PATCH 17/32] Raise an error if we get an HTTP error code --- gateway-api/src/gateway_api/pds_search.py | 11 +- .../src/gateway_api/test_pds_search.py | 168 ++++++++++++++---- gateway-api/stubs/stubs/stub_pds.py | 23 +++ 3 files changed, 163 insertions(+), 39 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index ce205fea..ec5ac5fd 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -11,6 +11,10 @@ ResultList: TypeAlias = list[dict[str, ResultStructure]] +class ExternalServiceError(Exception): + pass + + @dataclass class SearchResults: """ @@ -142,8 +146,9 @@ def search_patient_by_nhs_number( try: response.raise_for_status() - except requests.HTTPError: + except requests.HTTPError as err: # TODO: This should log, or something + raise ExternalServiceError("PDS request failed") from err return None bundle = response.json() @@ -239,9 +244,9 @@ def search_patient_by_details( try: response.raise_for_status() - except requests.HTTPError: + except requests.HTTPError as err: # TODO: This should log, or something - return None + raise ExternalServiceError("PDS request failed") from err bundle = response.json() return self._extract_single_search_result(bundle) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 839bf322..5a4d4b75 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -12,7 +12,12 @@ from stubs.stub_pds import PdsFhirApiStub import gateway_api.pds_search as pds_search -from gateway_api.pds_search import PdsSearch, ResultList, find_current_record +from gateway_api.pds_search import ( + ExternalServiceError, + PdsSearch, + ResultList, + find_current_record, +) @dataclass @@ -44,7 +49,7 @@ def stub() -> PdsFhirApiStub: @pytest.fixture -def route_requests_get( +def mock_requests_get( monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub ) -> dict[str, Any]: """ @@ -96,14 +101,51 @@ def _fake_get( return capture -def test_search_patient_by_nhs_number_success_defaults( - route_requests_get: dict[str, Any], +def _insert_basic_patient( + stub: PdsFhirApiStub, + nhs_number: str, + family: str, + given: list[str], + general_practitioner: list[dict[str, Any]] | None = None, ) -> None: """ - Verifies that a successful GET /Patient/{nhs_number} returns a parsed SearchResults - populated from the stubbed seeded patient, and that gp_ods_code is None when no - current generalPractitioner record exists. + Helper to create a patient resource with a current name period, and an explicit + generalPractitioner list (defaulting to empty). """ + stub.upsert_patient( + nhs_number=nhs_number, + patient={ + "resourceType": "Patient", + "name": [ + { + "use": "official", + "family": family, + "given": given, + "period": {"start": "1900-01-01", "end": "9999-12-31"}, + } + ], + "generalPractitioner": general_practitioner or [], + }, + version_id=1, + ) + + +def test_search_patient_by_nhs_number_get_patient_success( + stub: PdsFhirApiStub, + mock_requests_get: dict[str, Any], +) -> None: + """ + GET /Patient/{nhs_number} returns 200 and the client extracts basic demographic + fields. + """ + _insert_basic_patient( + stub=stub, + nhs_number="9000000009", + family="Smith", + given=["Jane"], + general_practitioner=[], + ) + client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -116,17 +158,68 @@ def test_search_patient_by_nhs_number_success_defaults( assert result.nhs_number == "9000000009" assert result.family_name == "Smith" assert result.given_names == "Jane" - assert result.gp_ods_code is None # default seeded patient has no GP in stub + assert result.gp_ods_code is None + + +def test_search_patient_by_nhs_number_no_current_gp_returns_gp_ods_code_none( + monkeypatch: pytest.MonkeyPatch, + stub: PdsFhirApiStub, + mock_requests_get: dict[str, Any], +) -> None: + """ + If generalPractitioner exists but none are current for 'today', gp_ods_code is None + (patient still returned). + """ + monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) + + _insert_basic_patient( + stub=stub, + nhs_number="9000000018", + family="Taylor", + given=["Ben"], + general_practitioner=[ + { + "id": "1", + "type": "Organization", + "identifier": { + "value": "OLDGP", + "period": {"start": "2010-01-01", "end": "2012-01-01"}, + }, + } + ], + ) + + client = PdsSearch( + auth_token="test-token", # noqa S106 # test token hardcoded + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + result = client.search_patient_by_nhs_number(9000000018) + + assert result is not None + assert result.nhs_number == "9000000018" + assert result.family_name == "Taylor" + assert result.given_names == "Ben" + assert result.gp_ods_code is None def test_search_patient_by_nhs_number_sends_expected_headers( - route_requests_get: dict[str, Any], + stub: PdsFhirApiStub, + mock_requests_get: dict[str, Any], ) -> None: """ - Verifies that search_patient_by_nhs_number forwards the expected HTTP headers, - including Authorization, end-user ODS header, Accept, and caller-provided - X-Request-ID / X-Correlation-ID values. + Client sends Authorization, ODS, Accept, X-Request-ID, and X-Correlation-ID headers + to the stubbed GET. """ + _insert_basic_patient( + stub=stub, + nhs_number="9000000009", + family="Smith", + given=["Jane"], + general_practitioner=[], + ) + client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -143,7 +236,7 @@ def test_search_patient_by_nhs_number_sends_expected_headers( ) assert result is not None - headers = route_requests_get["headers"] + headers = mock_requests_get["headers"] assert headers["Authorization"] == "Bearer test-token" assert headers["NHSD-End-User-Organisation-ODS"] == "A12345" assert headers["Accept"] == "application/fhir+json" @@ -152,13 +245,21 @@ def test_search_patient_by_nhs_number_sends_expected_headers( def test_search_patient_by_nhs_number_generates_request_id( - route_requests_get: dict[str, Any], + stub: PdsFhirApiStub, + mock_requests_get: dict[str, Any], ) -> None: """ - Verifies that when no request_id is provided by the caller, PdsSearch generates - an X-Request-ID and includes it in the outbound request (the stub validates the - header format in strict mode). + When request_id is omitted, the client generates an X-Request-ID and sends it + (validated by strict stub). """ + _insert_basic_patient( + stub=stub, + nhs_number="9000000009", + family="Smith", + given=["Jane"], + general_practitioner=[], + ) + client = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", @@ -168,43 +269,39 @@ def test_search_patient_by_nhs_number_generates_request_id( result = client.search_patient_by_nhs_number(9000000009) assert result is not None - # Ensure client generated an X-Request-ID (stub validates UUID shape in strict mode) - headers = route_requests_get["headers"] + headers = mock_requests_get["headers"] assert "X-Request-ID" in headers - # A light sanity check; strict validation is performed inside the stub. assert isinstance(headers["X-Request-ID"], str) assert len(headers["X-Request-ID"]) >= 32 def test_search_patient_by_nhs_number_not_found_returns_none( stub: PdsFhirApiStub, - route_requests_get: dict[str, Any], + mock_requests_get: dict[str, Any], ) -> None: """ - Verifies that when the downstream PDS endpoint returns 404 (patient not found), - the client handles the HTTP error path and returns None rather than propagating - an exception to the caller. + If GET /Patient/{nhs_number} returns 404 from the stub, the client + raises an exception. """ - client = PdsSearch( + + pds = PdsSearch( auth_token="test-token", # noqa S106 # test token hardcoded end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) - # Not seeded into stub store => 404 => raise_for_status => client returns None. - result = client.search_patient_by_nhs_number(9999999999) - assert result is None + with pytest.raises(ExternalServiceError): + pds.search_patient_by_nhs_number(9900000001) def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub, - route_requests_get: dict[str, Any], + mock_requests_get: dict[str, Any], ) -> None: """ - Verifies that gp_ods_code is extracted from the *current* generalPractitioner - record, where "current" is determined by identifier.period covering today's - date. Time is frozen via monkeypatch to ensure deterministic selection. + When exactly one generalPractitioner is current, gp_ods_code is extracted + from identifier.value. """ monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) @@ -236,7 +333,7 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( "type": "Organization", "identifier": { "value": "CURRGP", - "period": {"start": "2020-01-01", "end": "2030-01-01"}, + "period": {"start": "2020-01-01", "end": "9999-01-01"}, }, }, ], @@ -260,9 +357,8 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( def test_find_current_record_with_today_override() -> None: """ - Verifies that find_current_record honours the explicit `today` override and - selects the first record whose identifier.period covers that date, returning - None when no record is current for the supplied date. + find_current_record selects the record whose identifier.period includes the + supplied 'today' date. """ records = cast( "ResultList", diff --git a/gateway-api/stubs/stubs/stub_pds.py b/gateway-api/stubs/stubs/stub_pds.py index 01253230..a5339c01 100644 --- a/gateway-api/stubs/stubs/stub_pds.py +++ b/gateway-api/stubs/stubs/stub_pds.py @@ -6,6 +6,8 @@ from datetime import datetime, timezone from typing import Any +from requests import HTTPError, Response + @dataclass(frozen=True) class StubResponse: @@ -29,6 +31,23 @@ class PdsFhirApiStub: https://github.com/NHSDigital/personal-demographics-service-api/blob/master/specification/personal-demographics.yaml """ + @staticmethod + def _http_404_error(url: str | None = None) -> HTTPError: + resp = Response() + resp.status_code = 404 + if url is None: + resp.url = ( + "https://example.test/personal-demographics/FHIR/R4/Patient/9900000001" + ) + else: + resp.url = url + resp.reason = "Not Found" + return HTTPError( + f"404 Client Error: Not Found for url: {resp.url}", response=resp + ) + + NhsNumberErrors = [(9900000001, _http_404_error())] + def __init__(self, strict_headers: bool = True) -> None: # strict_headers=True enforces X-Request-ID presence and UUID format. self.strict_headers = strict_headers @@ -120,6 +139,10 @@ def get_patient( """ headers_out: dict[str, str] = {} + for nhsnum, err in self.NhsNumberErrors: + if nhs_number == str(nhsnum): + raise err + # Header handling (mirroring behavior). if self.strict_headers: if not request_id: From 23d957b6040ddb36645f2db7c1472dc5485d203a Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Tue, 6 Jan 2026 23:15:41 +0000 Subject: [PATCH 18/32] Remove special handling to make 404 work, tests passing --- gateway-api/stubs/stubs/stub_pds.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/gateway-api/stubs/stubs/stub_pds.py b/gateway-api/stubs/stubs/stub_pds.py index a5339c01..01253230 100644 --- a/gateway-api/stubs/stubs/stub_pds.py +++ b/gateway-api/stubs/stubs/stub_pds.py @@ -6,8 +6,6 @@ from datetime import datetime, timezone from typing import Any -from requests import HTTPError, Response - @dataclass(frozen=True) class StubResponse: @@ -31,23 +29,6 @@ class PdsFhirApiStub: https://github.com/NHSDigital/personal-demographics-service-api/blob/master/specification/personal-demographics.yaml """ - @staticmethod - def _http_404_error(url: str | None = None) -> HTTPError: - resp = Response() - resp.status_code = 404 - if url is None: - resp.url = ( - "https://example.test/personal-demographics/FHIR/R4/Patient/9900000001" - ) - else: - resp.url = url - resp.reason = "Not Found" - return HTTPError( - f"404 Client Error: Not Found for url: {resp.url}", response=resp - ) - - NhsNumberErrors = [(9900000001, _http_404_error())] - def __init__(self, strict_headers: bool = True) -> None: # strict_headers=True enforces X-Request-ID presence and UUID format. self.strict_headers = strict_headers @@ -139,10 +120,6 @@ def get_patient( """ headers_out: dict[str, str] = {} - for nhsnum, err in self.NhsNumberErrors: - if nhs_number == str(nhsnum): - raise err - # Header handling (mirroring behavior). if self.strict_headers: if not request_id: From 5e145bd10f64db0f792f16f22e869ef86fea67e5 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 7 Jan 2026 10:24:29 +0000 Subject: [PATCH 19/32] Update docstrings --- gateway-api/src/gateway_api/pds_search.py | 303 +++++++++++------- .../src/gateway_api/test_pds_search.py | 188 ++++++++--- gateway-api/stubs/stubs/stub_pds.py | 139 ++++++-- 3 files changed, 432 insertions(+), 198 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index ec5ac5fd..29aa57e9 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -1,3 +1,32 @@ +""" +PDS (Personal Demographics Service) FHIR R4 patient lookup client. + +Terminology +----------- + +FHIR elements such as ``Patient.name`` and ``Patient.generalPractitioner`` can contain +multiple records, each valid for a specific time range (a FHIR ``Period``). This module +selects the record that is *current* for a given date. + +Contracts enforced by the helper functions: + +* ``Patient.name[]`` records passed to :func:`find_current_name_record` must contain:: + + record["period"]["start"] + record["period"]["end"] + +* ``Patient.generalPractitioner[]`` records passed to :func:`find_current_record` must + contain:: + + record["identifier"]["period"]["start"] + record["identifier"]["period"]["end"] + +If required keys are missing, a ``KeyError`` is raised intentionally. This is treated as +malformed upstream data (or malformed test fixtures) and should be corrected at source. +""" + +from __future__ import annotations + import uuid from dataclasses import dataclass from datetime import date, datetime, timezone @@ -5,6 +34,7 @@ import requests +# Recursive JSON-like structure typing used for parsed FHIR bodies. ResultStructure: TypeAlias = ( str | dict[str, "ResultStructure"] | list["ResultStructure"] ) @@ -12,17 +42,30 @@ class ExternalServiceError(Exception): - pass + """ + Raised when the downstream PDS request fails. + + This module catches :class:`requests.HTTPError` thrown by + ``response.raise_for_status()`` and re-raises it as ``ExternalServiceError`` so + callers are not coupled to ``requests`` exception types. + """ @dataclass class SearchResults: """ - Represents a single patient search result with only the fields requested: - - Given name(s) - - Family name - - NHS Number - - Current general practitioner ODS code + A single extracted patient record. + + Only a small subset of the PDS Patient fields are currently required by this + gateway. More will be added in later phases. + + :param given_names: Given names from the *current* ``Patient.name`` record, + concatenated with spaces. + :param family_name: Family name from the *current* ``Patient.name`` record. + :param nhs_number: NHS number (``Patient.id``). + :param gp_ods_code: The ODS code of the *current* GP, extracted from + ``Patient.generalPractitioner[].identifier.value`` if a current GP record exists + otherwise ``None``. """ given_names: str @@ -33,9 +76,23 @@ class SearchResults: class PdsSearch: """ - Simple client for PDS FHIR R4 patient search (GET /Patient). + Simple client for PDS FHIR R4 patient retrieval. + + The client currently supports one operation: + + * :meth:`search_patient_by_nhs_number` - calls ``GET /Patient/{nhs_number}`` + + There is another operation implemented for searching by demographics: + + * :meth:`search_patient_by_details` - calls ``GET /Patient`` with query parameters + + ...but this is currently not fully tested. Its implementation may be finalised + in a later phase if it is required. - Usage: + Both methods return a :class:`SearchResults` instance when a patient can be + extracted, otherwise ``None``. + + **Usage example**:: pds = PdsSearch( auth_token="YOUR_ACCESS_TOKEN", @@ -43,19 +100,13 @@ class PdsSearch: base_url="https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4", ) - result = pds.search_patient( - family="Smith", - given="John", - gender="male", - date_of_birth="1980-05-12", - postcode="SW1A1AA", - ) + result = pds.search_patient_by_nhs_number(9000000009) if result: print(result) """ - # Defaults – adjust as needed + # URLs for different PDS environments. Requires authentication to use live. SANDBOX_URL = "https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4" INT_URL = "https://int.api.service.nhs.uk/personal-demographics/FHIR/R4" PROD_URL = "https://api.service.nhs.uk/personal-demographics/FHIR/R4" @@ -69,13 +120,14 @@ def __init__( timeout: int = 10, ) -> None: """ - :param auth_token: OAuth2 bearer token (without 'Bearer ' prefix) - :param end_user_org_ods: NHSD End User Organisation ODS code - :param base_url: Base URL for the PDS API (one of SANDBOX_URL / INT_URL / - PROD_URL) - :param nhsd_session_urid: Optional NHSD-Session-URID (for healthcare worker - access) - :param timeout: Default timeout in seconds for HTTP calls + Create a PDS client. + + :param auth_token: OAuth2 bearer token (without the ``"Bearer "`` prefix). + :param end_user_org_ods: NHSD End User Organisation ODS code. + :param base_url: Base URL for the PDS API (one of :attr:`SANDBOX_URL`, + :attr:`INT_URL`, :attr:`PROD_URL`). Trailing slashes are stripped. + :param nhsd_session_urid: Optional ``NHSD-Session-URID`` header value. + :param timeout: Default timeout in seconds for HTTP calls. """ self.auth_token = auth_token self.end_user_org_ods = end_user_org_ods @@ -90,6 +142,11 @@ def _build_headers( ) -> dict[str, str]: """ Build mandatory and optional headers for a PDS request. + + :param request_id: Optional ``X-Request-ID``. If not supplied a new UUID is + generated. + :param correlation_id: Optional ``X-Correlation-ID`` for cross-system tracing. + :return: Dictionary of HTTP headers for the outbound request. """ headers = { "Authorization": f"Bearer {self.auth_token}", @@ -98,9 +155,12 @@ def _build_headers( "Accept": "application/fhir+json", } + # NHSD-Session-URID is required in some flows; include only if configured. if self.nhsd_session_urid: headers["NHSD-Session-URID"] = self.nhsd_session_urid + # Correlation ID is used to track the same request across multiple systems. + # Can be safely omitted, mirrored back in response if included. if correlation_id: headers["X-Correlation-ID"] = correlation_id @@ -114,21 +174,22 @@ def search_patient_by_nhs_number( timeout: int | None = None, ) -> SearchResults | None: """ - Does a PDS search for the patient with the given NHS number and returns - a SearchResults object with the matching details. - - :param nhs_number: NHS number to search PDS for - :type nhs_number: int - :param request_id: If set re-use this request ID (if retrying a request) rather - than generating a new one. - :type request_id: str | None - :param correlation_id: Optional transaction correlation ID across multiple - systems; mirrored back in response. - :type correlation_id: str | None - :param timeout: Description - :type timeout: int | None - :return: Description - :rtype: SearchResults | None + Retrieve a patient by NHS number. + + Calls ``GET /Patient/{nhs_number}``, expects a Bundle-like JSON response in the + shape used by the unit test stub (``{"entry": [{"resource": }]}``), + then extracts a single :class:`SearchResults`. + + :param nhs_number: NHS number to search for. + :param request_id: Optional request ID to reuse for retries; if not supplied a + UUID is generated. + :param correlation_id: Optional correlation ID for tracing. + :param timeout: Optional per-call timeout in seconds. If not provided, + :attr:`timeout` is used. + :return: A :class:`SearchResults` instance if a patient can be extracted, + otherwise ``None``. + :raises ExternalServiceError: If the HTTP request returns an error status and + ``raise_for_status()`` raises :class:`requests.HTTPError`. """ headers = self._build_headers( request_id=request_id, @@ -145,11 +206,11 @@ def search_patient_by_nhs_number( ) try: + # In production, failures surface here (4xx/5xx -> HTTPError). response.raise_for_status() except requests.HTTPError as err: - # TODO: This should log, or something + # TODO: add logging / structured error details if required. raise ExternalServiceError("PDS request failed") from err - return None bundle = response.json() return self._extract_single_search_result(bundle) @@ -163,43 +224,33 @@ def search_patient_by_details( postcode: str | None = None, email: str | None = None, phone: str | None = None, - # These options are exposed by the PDS API but we don't need them, - # at least for now - # fuzzy_match: bool = False, - # exact_match: bool = False, - # history: bool = False, - # max_results: Optional[int] = None, request_id: str | None = None, correlation_id: str | None = None, timeout: int | None = None, ) -> SearchResults | None: """ - Perform a patient search using GET /Patient on the PDS FHIR R4 API. - - Assumes that the search will return a single matching patient. - - Required: - family Patient family name (surname) - given Patient given name - gender 'male' | 'female' | 'other' | 'unknown' - date_of_birth 'YYYY-MM-DD' (exact match, wrapped as eqYYYY-MM-DD) - - Optional: - postcode Patient postcode (address-postalcode) - email Patient email address - phone Patient phone number - fuzzy_match If True, sets _fuzzy-match=true - exact_match If True, sets _exact-match=true - history If True, sets _history=true - max_results Integer 1–50 - request_id Override X-Request-ID (otherwise auto-generated UUID) - correlation_id Optional X-Correlation-ID - timeout Per-call timeout (defaults to client-level timeout) - - Returns: - A single SearchResults instance if a patient is found, else None. + Search for a patient using demographics. + + Calls ``GET /Patient`` with query parameters and extracts the first matching + patient from the Bundle. + + :param family: Patient family name (surname). + :param given: Patient given name. + :param gender: One of ``'male' | 'female' | 'other' | 'unknown'``. + :param date_of_birth: Date of birth in ``YYYY-MM-DD`` format. The query is sent + as ``birthdate=eqYYYY-MM-DD`` for exact matching. + :param postcode: Optional patient postcode; sent as ``address-postalcode``. + :param email: Optional patient email address. + :param phone: Optional patient phone number. + :param request_id: Optional request ID override. + :param correlation_id: Optional correlation ID. + :param timeout: Optional per-call timeout in seconds. If not provided, + :attr:`timeout` is used. + :return: A :class:`SearchResults` instance if a patient can be extracted, + otherwise ``None``. + :raises ExternalServiceError: If the HTTP request returns an error status and + ``raise_for_status()`` raises :class:`requests.HTTPError`. """ - headers = self._build_headers( request_id=request_id, correlation_id=correlation_id, @@ -209,12 +260,12 @@ def search_patient_by_details( "family": family, "given": given, "gender": gender, - # Exact DOB match + # FHIR search "eq" prefix means exact match. "birthdate": f"eq{date_of_birth}", } if postcode: - # Use address-postalcode (address-postcode is deprecated) + # Use address-postalcode (address-postcode is deprecated). params["address-postalcode"] = postcode if email: @@ -223,16 +274,6 @@ def search_patient_by_details( if phone: params["phone"] = phone - # # Optional flags - # if fuzzy_match: - # params["_fuzzy-match"] = "true" - # if exact_match: - # params["_exact-match"] = "true" - # if history: - # params["_history"] = "true" - # if max_results is not None: - # params["_max-results"] = max_results - url = f"{self.base_url}/Patient" response = requests.get( @@ -245,7 +286,7 @@ def search_patient_by_details( try: response.raise_for_status() except requests.HTTPError as err: - # TODO: This should log, or something + # TODO: add logging / structured error details if required. raise ExternalServiceError("PDS request failed") from err bundle = response.json() @@ -256,8 +297,20 @@ def search_patient_by_details( @staticmethod def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: """ - Extract the current general practitioner ODS code from - Patient.generalPractitioner[].identifier.value, if present. + Extract the current GP ODS code from ``Patient.generalPractitioner``. + + This function implements the business rule: + + * If the list is empty, return ``None``. + * If the list is non-empty and no record is current, return ``None``. + * If exactly one record is current, return its ``identifier.value``. + + In future this may change to return the most recent record if none is current. + + :param general_practitioners: List of ``generalPractitioner`` records from a + Patient resource. + :return: ODS code string if a current record exists, otherwise ``None``. + :raises KeyError: If a record is missing required ``identifier.period`` fields. """ if len(general_practitioners) == 0: return None @@ -269,45 +322,51 @@ def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: identifier = cast("dict[str, ResultStructure]", gp.get("identifier", {})) ods_code = str(identifier.get("value", None)) - return ods_code + # Avoid returning the literal string "None" if identifier.value is absent. + return None if ods_code == "None" else ods_code def _extract_single_search_result( self, bundle: dict[str, ResultStructure] ) -> SearchResults | None: """ - Convert a FHIR Bundle from /Patient search into a single SearchResults - object by using the first entry. Returns None if there are no entries. + Extract a single :class:`SearchResults` from a FHIR Bundle. + + The code assumes that the API returns either zero matches (empty entry list) + or a single match; if multiple entries are present, the first entry is used. + + :param bundle: Parsed JSON body containing at minimum an ``entry`` list, where + the first entry contains a Patient resource under ``resource``. + :return: A populated :class:`SearchResults` if extraction succeeds, otherwise + ``None``. + :raises KeyError: If required fields for current record selection + (period fields) are missing in the evaluated structures. """ - entries: ResultList = cast( - "ResultList", bundle.get("entry", []) - ) # entries["entry"] is definitely a list + entries: ResultList = cast("ResultList", bundle.get("entry", [])) if not entries: return None - # Search can return multiple patients, except that for APIs it can only - # return one, so this is fine + # Use the first patient entry. Search by NHS number is unique. Search by + # demographics for an application is allowed to return max one entry from PDS. + # (search by a human can return more, but presumably we count as an application) + # See MaxResults parameter in the PDS OpenAPI spec. entry = entries[0] patient = cast("dict[str, ResultStructure]", entry.get("resource", {})) nhs_number = str(patient.get("id", "")).strip() - - # Pretty sure NHS number has to be there if not nhs_number: return None + # Select current name record and extract names. names = cast("ResultList", patient.get("name", [])) name_obj = find_current_name_record(names) - if name_obj is None: return None given_names_list = cast("list[str]", name_obj.get("given", [])) family_name = str(name_obj.get("family", "")) or "" - given_names_str = " ".join(given_names_list).strip() - # TODO: What happens if the patient isn't registered with a GP so this is empty? - # Probably not for Alpha + # Extract GP ODS code if a current GP record exists. gp_list = cast("ResultList", patient.get("generalPractitioner", [])) gp_ods_code = self._get_gp_ods_code(gp_list) @@ -323,20 +382,27 @@ def find_current_record( records: ResultList, today: date | None = None ) -> dict[str, ResultStructure] | None: """ - generalPractitioner selection. + Select the current record from a ``generalPractitioner`` list. + + A record is "current" if its ``identifier.period`` covers ``today`` (inclusive): - Each generalPractitioner record MUST contain: - - identifier.period.start (ISO date) - - identifier.period.end (ISO date) + ``start <= today <= end`` - There may be zero records; if no record is current, return None. - Missing keys are treated as errors (KeyError), per contract. + The list may be in any of the following states: - Returns: the first dict whose identifier.period covers 'today', or None. + * empty + * contains one or more records, none current + * contains one or more records, exactly one current + + :param records: List of ``generalPractitioner`` records. + :param today: Optional override date, intended for deterministic tests. + If not supplied, the current UTC date is used. + :return: The first record whose ``identifier.period`` covers ``today``, or ``None`` + if no record is current. + :raises KeyError: If required keys are missing for a record being evaluated. + :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. """ if today is None: - # TODO: Do we need to do something about UTC here? Do we need to use local time? - # Don't worry for Alpha today = datetime.now(timezone.utc).date() for record in records: @@ -345,11 +411,9 @@ def find_current_record( start_str = periods["start"] end_str = periods["end"] - # Parse ISO dates start = date.fromisoformat(start_str) end = date.fromisoformat(end_str) - # Inclusive range check if start <= today <= end: return record @@ -360,14 +424,19 @@ def find_current_name_record( records: ResultList, today: date | None = None ) -> dict[str, ResultStructure] | None: """ - Patient.name[] selection. + Select the current record from a ``Patient.name`` list. + + A record is "current" if its ``period`` covers ``today`` (inclusive): - Each name record MUST contain: - - period.start (ISO date) - - period.end (ISO date) + ``start <= today <= end`` - Returns the first name record whose period covers 'today', else None. - Missing keys are treated as errors (KeyError), per contract. + :param records: List of ``Patient.name`` records. + :param today: Optional override date, intended for deterministic tests. + If not supplied, the current UTC date is used. + :return: The first name record whose ``period`` covers ``today``, or ``None`` if no + record is current. + :raises KeyError: If required keys (``period.start`` / ``period.end``) are missing. + :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. """ if today is None: today = datetime.now(timezone.utc).date() diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 5a4d4b75..fcb53b9a 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -1,9 +1,12 @@ -# tests/test_pds_search.py +""" +Unit tests for :mod:`gateway_api.pds_search`. +""" + from __future__ import annotations import re from dataclasses import dataclass -from datetime import date, datetime, timezone, tzinfo +from datetime import date from typing import Any, cast from uuid import uuid4 @@ -11,7 +14,6 @@ import requests from stubs.stub_pds import PdsFhirApiStub -import gateway_api.pds_search as pds_search from gateway_api.pds_search import ( ExternalServiceError, PdsSearch, @@ -22,28 +24,48 @@ @dataclass class FakeResponse: + """ + Minimal substitute for :class:`requests.Response` used by tests. + + Only the methods accessed by :class:`gateway_api.pds_search.PdsSearch` are + implemented. + + :param status_code: HTTP status code. + :param headers: Response headers. + :param _json: Parsed JSON body returned by :meth:`json`. + """ + status_code: int headers: dict[str, str] _json: dict[str, Any] def json(self) -> dict[str, Any]: + """ + Return the response JSON body. + + :return: Parsed JSON body. + """ return self._json def raise_for_status(self) -> None: - if self.status_code >= 400: - raise requests.HTTPError(f"{self.status_code} Error") + """ + Emulate :meth:`requests.Response.raise_for_status`. - -class FrozenDateTime(datetime): - @classmethod - def now(cls, tz: tzinfo | None = None) -> FrozenDateTime: - return cast( - "FrozenDateTime", datetime(2026, 1, 2, 12, 0, 0, tzinfo=timezone.utc) - ) + :return: ``None``. + :raises requests.HTTPError: If the response status is not 200. + """ + if self.status_code != 200: + raise requests.HTTPError(f"{self.status_code} Error") @pytest.fixture def stub() -> PdsFhirApiStub: + """ + Create a stub backend instance. + + :return: A :class:`stubs.stub_pds.PdsFhirApiStub` with strict header validation + enabled. + """ # Strict header validation helps ensure PdsSearch sends X-Request-ID correctly. return PdsFhirApiStub(strict_headers=True) @@ -53,9 +75,15 @@ def mock_requests_get( monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub ) -> dict[str, Any]: """ - Patch requests.get so that calls made by PdsSearch are routed into the stub. + Patch ``requests.get`` so calls are routed into :meth:`PdsFhirApiStub.get_patient`. + + The fixture returns a "capture" dict recording the most recent request information. + This is used by header-related tests. - Returns a capture dict that records the last request made. + :param monkeypatch: Pytest monkeypatch fixture. + :param stub: Stub backend used to serve GET requests. + :return: A capture dictionary containing the last call details + (url/headers/params/timeout). """ capture: dict[str, Any] = {} @@ -65,19 +93,30 @@ def _fake_get( params: Any = None, timeout: Any = None, ) -> FakeResponse: + """ + Replacement function for :func:`requests.get`. + + :param url: URL passed by the client. + :param headers: Headers passed by the client. + :param params: Query parameters (recorded, not interpreted for + GET /Patient/{id}). + :param timeout: Timeout (recorded). + :return: A :class:`FakeResponse` whose behaviour mimics ``requests.Response``. + """ headers = headers or {} capture["url"] = url capture["headers"] = dict(headers) capture["params"] = params capture["timeout"] = timeout - # Support only GET /Patient/{id} (as used by search_patient_by_nhs_number). + # The client under test is expected to call GET {base_url}/Patient/{id}. m = re.match(r"^(?P.+)/Patient/(?P\d+)$", url) if not m: raise AssertionError(f"Unexpected URL called by client: {url}") nhs_number = m.group("nhs") + # Route the "HTTP" request into the in-memory stub. stub_resp = stub.get_patient( nhs_number=nhs_number, request_id=headers.get("X-Request-ID"), @@ -86,8 +125,7 @@ def _fake_get( end_user_org_ods=headers.get("NHSD-End-User-Organisation-ODS"), ) - # pds_search.PdsSearch._extract_single_search_result expects a Bundle-like - # structure: {"entry": [{"resource": {...Patient...}}]} + # PdsSearch expects a Bundle-like structure on success in these tests. if stub_resp.status_code == 200: body = {"entry": [{"resource": stub_resp.json}]} else: @@ -109,8 +147,15 @@ def _insert_basic_patient( general_practitioner: list[dict[str, Any]] | None = None, ) -> None: """ - Helper to create a patient resource with a current name period, and an explicit - generalPractitioner list (defaulting to empty). + Insert a basic Patient record into the stub. + + :param stub: Stub backend to insert into. + :param nhs_number: NHS number (10-digit string). + :param family: Family name for the Patient.name record. + :param given: Given names for the Patient.name record. + :param general_practitioner: Optional list stored under + ``Patient.generalPractitioner``. + :return: ``None``. """ stub.upsert_patient( nhs_number=nhs_number, @@ -135,8 +180,15 @@ def test_search_patient_by_nhs_number_get_patient_success( mock_requests_get: dict[str, Any], ) -> None: """ - GET /Patient/{nhs_number} returns 200 and the client extracts basic demographic - fields. + Verify ``GET /Patient/{nhs_number}`` returns 200 and demographics are extracted. + + This test explicitly inserts the patient into the stub and asserts that the client + returns a populated :class:`gateway_api.pds_search.SearchResults`. + + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture + (ensures patching is active). + :return: ``None``. """ _insert_basic_patient( stub=stub, @@ -147,7 +199,7 @@ def test_search_patient_by_nhs_number_get_patient_success( ) client = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -162,16 +214,25 @@ def test_search_patient_by_nhs_number_get_patient_success( def test_search_patient_by_nhs_number_no_current_gp_returns_gp_ods_code_none( - monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub, mock_requests_get: dict[str, Any], ) -> None: """ - If generalPractitioner exists but none are current for 'today', gp_ods_code is None - (patient still returned). - """ - monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) + Verify that ``gp_ods_code`` is ``None`` when no GP record is current. + + The generalPractitioner list may be: + * empty + * non-empty with no current record + * non-empty with exactly one current record + This test covers the "non-empty, none current" case by + inserting only a historical GP record. + + :param monkeypatch: Pytest monkeypatch fixture. + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture. + :return: ``None``. + """ _insert_basic_patient( stub=stub, nhs_number="9000000018", @@ -190,7 +251,7 @@ def test_search_patient_by_nhs_number_no_current_gp_returns_gp_ods_code_none( ) client = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -209,8 +270,18 @@ def test_search_patient_by_nhs_number_sends_expected_headers( mock_requests_get: dict[str, Any], ) -> None: """ - Client sends Authorization, ODS, Accept, X-Request-ID, and X-Correlation-ID headers - to the stubbed GET. + Verify that the client sends the expected headers to PDS. + + Asserts that the request contains: + * Authorization header + * NHSD-End-User-Organisation-ODS header + * Accept header + * caller-provided X-Request-ID and X-Correlation-ID headers + + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture capturing outbound + headers. + :return: ``None``. """ _insert_basic_patient( stub=stub, @@ -221,7 +292,7 @@ def test_search_patient_by_nhs_number_sends_expected_headers( ) client = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -249,8 +320,15 @@ def test_search_patient_by_nhs_number_generates_request_id( mock_requests_get: dict[str, Any], ) -> None: """ - When request_id is omitted, the client generates an X-Request-ID and sends it - (validated by strict stub). + Verify that the client generates an X-Request-ID when not provided. + + The stub is in strict mode, so a missing or invalid X-Request-ID would cause a 400. + This test confirms a request ID is present and looks UUID-like. + + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture capturing outbound + headers. + :return: ``None``. """ _insert_basic_patient( stub=stub, @@ -261,7 +339,7 @@ def test_search_patient_by_nhs_number_generates_request_id( ) client = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -275,17 +353,23 @@ def test_search_patient_by_nhs_number_generates_request_id( assert len(headers["X-Request-ID"]) >= 32 -def test_search_patient_by_nhs_number_not_found_returns_none( +def test_search_patient_by_nhs_number_not_found_raises_error( stub: PdsFhirApiStub, mock_requests_get: dict[str, Any], ) -> None: """ - If GET /Patient/{nhs_number} returns 404 from the stub, the client - raises an exception. - """ + Verify that a 404 response results in :class:`ExternalServiceError`. + + The stub returns a 404 OperationOutcome for unknown NHS numbers. The client calls + ``raise_for_status()``, which raises ``requests.HTTPError``; the client wraps that + into :class:`ExternalServiceError`. + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture. + :return: ``None``. + """ pds = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -295,16 +379,21 @@ def test_search_patient_by_nhs_number_not_found_returns_none( def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( - monkeypatch: pytest.MonkeyPatch, stub: PdsFhirApiStub, mock_requests_get: dict[str, Any], ) -> None: """ - When exactly one generalPractitioner is current, gp_ods_code is extracted - from identifier.value. - """ - monkeypatch.setattr(pds_search, "datetime", FrozenDateTime) + Verify that a current GP record is selected and its ODS code returned. + The test inserts a patient with two GP records: + * one historical (not current) + * one current (period covers today) + + :param monkeypatch: Pytest monkeypatch fixture. + :param stub: Stub backend fixture. + :param mock_requests_get: Patched ``requests.get`` fixture. + :return: ``None``. + """ stub.upsert_patient( nhs_number="9000000017", patient={ @@ -318,7 +407,7 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( } ], "generalPractitioner": [ - # Not current on 2026-01-02 + # Old { "id": "1", "type": "Organization", @@ -327,7 +416,7 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( "period": {"start": "2010-01-01", "end": "2012-01-01"}, }, }, - # Current on 2026-01-02 + # Current { "id": "2", "type": "Organization", @@ -342,7 +431,7 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( ) client = PdsSearch( - auth_token="test-token", # noqa S106 # test token hardcoded + auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) @@ -357,8 +446,9 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( def test_find_current_record_with_today_override() -> None: """ - find_current_record selects the record whose identifier.period includes the - supplied 'today' date. + Verify that ``find_current_record`` honours an explicit ``today`` value. + + :return: ``None``. """ records = cast( "ResultList", diff --git a/gateway-api/stubs/stubs/stub_pds.py b/gateway-api/stubs/stubs/stub_pds.py index 01253230..f02a0110 100644 --- a/gateway-api/stubs/stubs/stub_pds.py +++ b/gateway-api/stubs/stubs/stub_pds.py @@ -1,3 +1,9 @@ +""" +In-memory PDS FHIR R4 API stub. + +The stub does **not** implement the full PDS API surface, nor full FHIR validation. +""" + from __future__ import annotations import re @@ -9,6 +15,14 @@ @dataclass(frozen=True) class StubResponse: + """ + Minimal response object returned by :class:`PdsFhirApiStub`. + + :param status_code: HTTP-like status code for the response. + :param headers: HTTP-like response headers. + :param json: Parsed JSON response body. + """ + status_code: int headers: dict[str, str] json: dict[str, Any] @@ -16,26 +30,34 @@ class StubResponse: class PdsFhirApiStub: """ - Minimal in-memory stub for the PDS FHIR API, implementing only GET /Patient/{id}. + Minimal in-memory stub for the PDS FHIR API, implementing only ``GET /Patient/{id}`` + + Contract elements modelled from the PDS OpenAPI spec: - Contract elements modelled from the OpenAPI spec: - - Path: /Patient/{id} - - id is the patient's NHS number (10 digits) - - X-Request-ID is mandatory and mirrored back in a response header - - X-Correlation-ID is optional and mirrored back if supplied - - ETag follows W/"" and corresponds to Patient.meta.versionId + * ``/Patient/{id}`` where ``id`` is the patient's NHS number (10 digits) + * ``X-Request-ID`` is mandatory (in strict mode) and echoed back as ``X-Request-Id`` + * ``X-Correlation-ID`` is optional and echoed back as ``X-Correlation-Id`` + if supplied + * ``ETag`` follows ``W/""`` and corresponds to ``Patient.meta.versionId`` - OpenAPI spec available from - https://github.com/NHSDigital/personal-demographics-service-api/blob/master/specification/personal-demographics.yaml + See: + https://github.com/NHSDigital/personal-demographics-service-api/blob/master/specification/personal-demographics.yaml """ def __init__(self, strict_headers: bool = True) -> None: - # strict_headers=True enforces X-Request-ID presence and UUID format. + """ + Create a new stub instance. + + :param strict_headers: If ``True``, enforce presence and UUID format of + ``X-Request-ID``. If ``False``, header validation is relaxed. + """ self.strict_headers = strict_headers + # Internal store: nhs_number -> (patient_resource, version_id_int) self._patients: dict[str, tuple[dict[str, Any], int]] = {} # Seed a deterministic example matching the spec's id example. + # Tests may overwrite this record via upsert_patient. self.upsert_patient( nhs_number="9000000009", patient={ @@ -76,16 +98,17 @@ def upsert_patient( version_id: int = 1, ) -> None: """ - Add/replace a patient in the stub store. - - Arguments - nhs_number: String NHS number, ten digits - patient: Dictionary containing details of existing patient to replace - version_id: Version of patient. Increment this if replacing a patient, at least - if you want to be able to track it + Insert or replace a patient record in the stub store. + :param nhs_number: NHS number as a 10-digit string. + :param patient: Patient resource dictionary. If ``None``, an empty Patient dict + is created and populated with required keys. + :param version_id: Version integer recorded into + ``patient["meta"]["versionId"]`` and used to generate the ETag on retrieval. + :return: ``None``. + :raises TypeError: If ``nhs_number`` is not a string. + :raises ValueError: If ``nhs_number`` is not 10 digits or fails validation. """ - try: nhsnum_match = re.fullmatch(r"(\d{10})", nhs_number) except TypeError as err: @@ -97,13 +120,15 @@ def upsert_patient( if not self._is_valid_nhs_number(nhs_number): raise ValueError("NHS Number is not valid") - patient = dict(patient) if patient is not None else {} + if patient is None: + patient = {} patient.setdefault("resourceType", "Patient") patient["id"] = nhs_number patient.setdefault("meta", {}) patient["meta"]["versionId"] = str(version_id) patient["meta"].setdefault("lastUpdated", self._now_fhir_instant()) + self._patients[nhs_number] = (patient, version_id) def get_patient( @@ -111,16 +136,28 @@ def get_patient( nhs_number: str, request_id: str | None = None, correlation_id: str | None = None, - authorization: str | None = None, # noqa F841 # Ignored in stub - role_id: str | None = None, # noqa F841 # Ignored in stub - end_user_org_ods: str | None = None, # noqa F841 # Ignored in stub + authorization: str | None = None, # noqa: F841 (ignored in stub) + role_id: str | None = None, # noqa: F841 (ignored in stub) + end_user_org_ods: str | None = None, # noqa: F841 (ignored in stub) ) -> StubResponse: """ - Implements GET /Patient/{id}. + Implements ``GET /Patient/{id}``. + + :param nhs_number: The NHS number path parameter. + :param request_id: The ``X-Request-ID`` header value. Required + (and must be UUID) when ``strict_headers=True``. + :param correlation_id: Optional ``X-Correlation-ID`` header value. + :param authorization: Authorization header (ignored by the stub). + :param role_id: Role header (ignored by the stub). + :param end_user_org_ods: End-user ODS header (ignored by the stub). + :return: A :class:`StubResponse` representing either: + * ``200`` with Patient JSON + * ``404`` with OperationOutcome JSON + * ``400`` with OperationOutcome JSON (validation failures) """ headers_out: dict[str, str] = {} - # Header handling (mirroring behavior). + # Header validation mirrors key behaviour enforced by the real service. if self.strict_headers: if not request_id: return self._bad_request( @@ -134,13 +171,14 @@ def get_patient( request_id=request_id, correlation_id=correlation_id, ) + + # Echo trace headers back (note casing). if request_id: headers_out["X-Request-Id"] = request_id if correlation_id: headers_out["X-Correlation-Id"] = correlation_id - # Path parameter validation: 10 digits and valid NHS number. - + # Path parameter validation: must be 10 digits and pass NHS-number validation. if not re.fullmatch( r"\d{10}", nhs_number or "" ) or not self._is_valid_nhs_number(nhs_number): @@ -151,7 +189,7 @@ def get_patient( display="Resource Id is invalid", ) - # Lookup. + # Lookup: not present => 404 OperationOutcome. if nhs_number not in self._patients: return self._operation_outcome( status_code=404, @@ -172,6 +210,11 @@ def get_patient( @staticmethod def _now_fhir_instant() -> str: + """ + Generate a FHIR instant timestamp in UTC with seconds precision. + + :return: Timestamp string in the format ``YYYY-MM-DDTHH:MM:SSZ``. + """ return ( datetime.now(timezone.utc) .replace(microsecond=0) @@ -181,6 +224,12 @@ def _now_fhir_instant() -> str: @staticmethod def _is_uuid(value: str) -> bool: + """ + Determine whether a string can be parsed as a UUID. + + :param value: Candidate value. + :return: ``True`` if the value parses as a UUID, otherwise ``False``. + """ try: uuid.UUID(value) return True @@ -190,12 +239,21 @@ def _is_uuid(value: str) -> bool: @staticmethod def _is_valid_nhs_number(nhs_number: str) -> bool: """ - NHS number check-digit validation (mod 11). - Rejects cases where computed check digit is 10. + Validate an NHS number. + + The intended logic is check-digit validation (mod 11), rejecting cases where the + computed check digit is 10. + + :param nhs_number: NHS number string. + :return: ``True`` if considered valid. + + .. note:: + This stub currently returns ``True`` for all values to keep unit test data + setup lightweight. Uncomment the implementation below if stricter validation + is desired. """ - # TODO: The AI did this. Check it's correct but also do we need this validation - # in the stub? In the mean time, just pass everything. return True + # digits = [int(c) for c in nhs_number] # total = sum(digits[i] * (10 - i) for i in range(9)) # weights 10..2 # remainder = total % 11 @@ -209,11 +267,20 @@ def _is_valid_nhs_number(nhs_number: str) -> bool: def _bad_request( self, message: str, *, request_id: str | None, correlation_id: str | None ) -> StubResponse: + """ + Build a 400 OperationOutcome response. + + :param message: Human-readable error message. + :param request_id: Optional request ID to echo back. + :param correlation_id: Optional correlation ID to echo back. + :return: A 400 :class:`StubResponse` containing an OperationOutcome. + """ headers: dict[str, str] = {} if request_id: headers["X-Request-Id"] = request_id if correlation_id: headers["X-Correlation-Id"] = correlation_id + return self._operation_outcome( status_code=400, headers=headers, @@ -225,7 +292,15 @@ def _bad_request( def _operation_outcome( *, status_code: int, headers: dict[str, str], spine_code: str, display: str ) -> StubResponse: - # Matches the example structure shown in the OpenAPI file. + """ + Construct an OperationOutcome response body. + + :param status_code: HTTP-like status code. + :param headers: Response headers. + :param spine_code: Spine error/warning code. + :param display: Human-readable display message. + :return: A :class:`StubResponse` containing an OperationOutcome JSON body. + """ body = { "resourceType": "OperationOutcome", "issue": [ From 08255bb23346ead6aaa9912569ab1b12a0e9d7e6 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 7 Jan 2026 10:40:33 +0000 Subject: [PATCH 20/32] Sonar being overenthusiastic --- gateway-api/src/gateway_api/pds_search.py | 17 ++++++++--------- gateway-api/stubs/stubs/stub_pds.py | 8 ++++---- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 29aa57e9..06d925dd 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -38,7 +38,8 @@ ResultStructure: TypeAlias = ( str | dict[str, "ResultStructure"] | list["ResultStructure"] ) -ResultList: TypeAlias = list[dict[str, ResultStructure]] +ResultStructureDict: TypeAlias = dict[str, ResultStructure] +ResultList: TypeAlias = list[ResultStructureDict] class ExternalServiceError(Exception): @@ -209,7 +210,6 @@ def search_patient_by_nhs_number( # In production, failures surface here (4xx/5xx -> HTTPError). response.raise_for_status() except requests.HTTPError as err: - # TODO: add logging / structured error details if required. raise ExternalServiceError("PDS request failed") from err bundle = response.json() @@ -286,7 +286,6 @@ def search_patient_by_details( try: response.raise_for_status() except requests.HTTPError as err: - # TODO: add logging / structured error details if required. raise ExternalServiceError("PDS request failed") from err bundle = response.json() @@ -319,14 +318,14 @@ def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: if gp is None: return None - identifier = cast("dict[str, ResultStructure]", gp.get("identifier", {})) + identifier = cast("ResultStructureDict", gp.get("identifier", {})) ods_code = str(identifier.get("value", None)) # Avoid returning the literal string "None" if identifier.value is absent. return None if ods_code == "None" else ods_code def _extract_single_search_result( - self, bundle: dict[str, ResultStructure] + self, bundle: ResultStructureDict ) -> SearchResults | None: """ Extract a single :class:`SearchResults` from a FHIR Bundle. @@ -350,7 +349,7 @@ def _extract_single_search_result( # (search by a human can return more, but presumably we count as an application) # See MaxResults parameter in the PDS OpenAPI spec. entry = entries[0] - patient = cast("dict[str, ResultStructure]", entry.get("resource", {})) + patient = cast("ResultStructureDict", entry.get("resource", {})) nhs_number = str(patient.get("id", "")).strip() if not nhs_number: @@ -380,7 +379,7 @@ def _extract_single_search_result( def find_current_record( records: ResultList, today: date | None = None -) -> dict[str, ResultStructure] | None: +) -> ResultStructureDict | None: """ Select the current record from a ``generalPractitioner`` list. @@ -406,7 +405,7 @@ def find_current_record( today = datetime.now(timezone.utc).date() for record in records: - identifier = cast("dict[str, ResultStructure]", record["identifier"]) + identifier = cast("ResultStructureDict", record["identifier"]) periods = cast("dict[str, str]", identifier["period"]) start_str = periods["start"] end_str = periods["end"] @@ -422,7 +421,7 @@ def find_current_record( def find_current_name_record( records: ResultList, today: date | None = None -) -> dict[str, ResultStructure] | None: +) -> ResultStructureDict | None: """ Select the current record from a ``Patient.name`` list. diff --git a/gateway-api/stubs/stubs/stub_pds.py b/gateway-api/stubs/stubs/stub_pds.py index f02a0110..dba6c1b9 100644 --- a/gateway-api/stubs/stubs/stub_pds.py +++ b/gateway-api/stubs/stubs/stub_pds.py @@ -136,9 +136,9 @@ def get_patient( nhs_number: str, request_id: str | None = None, correlation_id: str | None = None, - authorization: str | None = None, # noqa: F841 (ignored in stub) - role_id: str | None = None, # noqa: F841 (ignored in stub) - end_user_org_ods: str | None = None, # noqa: F841 (ignored in stub) + authorization: str | None = None, # noqa: F841 # NOSONAR S1172 (ignored in stub) + role_id: str | None = None, # noqa: F841 # NOSONAR S1172 (ignored in stub) + end_user_org_ods: str | None = None, # noqa: F841 # NOSONAR S1172 (ignored in stub) ) -> StubResponse: """ Implements ``GET /Patient/{id}``. @@ -254,7 +254,7 @@ def _is_valid_nhs_number(nhs_number: str) -> bool: """ return True - # digits = [int(c) for c in nhs_number] + # digits = [int(c) for c in nhs_number] # NOSONAR S125 (May be wanted later) # total = sum(digits[i] * (10 - i) for i in range(9)) # weights 10..2 # remainder = total % 11 # check = 11 - remainder From fedde87bc61008093303fd9696a04dac1867e141 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:37:49 +0000 Subject: [PATCH 21/32] Add extra test to fix coverage --- gateway-api/src/gateway_api/pds_search.py | 82 +------------- .../src/gateway_api/test_pds_search.py | 100 ++++++++++++++++++ 2 files changed, 103 insertions(+), 79 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 06d925dd..16bd3156 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -215,82 +215,6 @@ def search_patient_by_nhs_number( bundle = response.json() return self._extract_single_search_result(bundle) - def search_patient_by_details( - self, - family: str, - given: str, - gender: str, - date_of_birth: str, - postcode: str | None = None, - email: str | None = None, - phone: str | None = None, - request_id: str | None = None, - correlation_id: str | None = None, - timeout: int | None = None, - ) -> SearchResults | None: - """ - Search for a patient using demographics. - - Calls ``GET /Patient`` with query parameters and extracts the first matching - patient from the Bundle. - - :param family: Patient family name (surname). - :param given: Patient given name. - :param gender: One of ``'male' | 'female' | 'other' | 'unknown'``. - :param date_of_birth: Date of birth in ``YYYY-MM-DD`` format. The query is sent - as ``birthdate=eqYYYY-MM-DD`` for exact matching. - :param postcode: Optional patient postcode; sent as ``address-postalcode``. - :param email: Optional patient email address. - :param phone: Optional patient phone number. - :param request_id: Optional request ID override. - :param correlation_id: Optional correlation ID. - :param timeout: Optional per-call timeout in seconds. If not provided, - :attr:`timeout` is used. - :return: A :class:`SearchResults` instance if a patient can be extracted, - otherwise ``None``. - :raises ExternalServiceError: If the HTTP request returns an error status and - ``raise_for_status()`` raises :class:`requests.HTTPError`. - """ - headers = self._build_headers( - request_id=request_id, - correlation_id=correlation_id, - ) - - params: dict[str, str] = { - "family": family, - "given": given, - "gender": gender, - # FHIR search "eq" prefix means exact match. - "birthdate": f"eq{date_of_birth}", - } - - if postcode: - # Use address-postalcode (address-postcode is deprecated). - params["address-postalcode"] = postcode - - if email: - params["email"] = email - - if phone: - params["phone"] = phone - - url = f"{self.base_url}/Patient" - - response = requests.get( - url, - headers=headers, - params=params, - timeout=timeout or self.timeout, - ) - - try: - response.raise_for_status() - except requests.HTTPError as err: - raise ExternalServiceError("PDS request failed") from err - - bundle = response.json() - return self._extract_single_search_result(bundle) - # --------------- internal helpers for result extraction ----------------- @staticmethod @@ -342,7 +266,7 @@ def _extract_single_search_result( """ entries: ResultList = cast("ResultList", bundle.get("entry", [])) if not entries: - return None + raise RuntimeError("PDS response contains no patient entries") # Use the first patient entry. Search by NHS number is unique. Search by # demographics for an application is allowed to return max one entry from PDS. @@ -353,13 +277,13 @@ def _extract_single_search_result( nhs_number = str(patient.get("id", "")).strip() if not nhs_number: - return None + raise RuntimeError("PDS patient resource missing NHS number") # Select current name record and extract names. names = cast("ResultList", patient.get("name", [])) name_obj = find_current_name_record(names) if name_obj is None: - return None + raise RuntimeError("PDS patient has no current name record") given_names_list = cast("list[str]", name_obj.get("given", [])) family_name = str(name_obj.get("family", "")) or "" diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index fcb53b9a..23410fd4 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -18,6 +18,7 @@ ExternalServiceError, PdsSearch, ResultList, + find_current_name_record, find_current_record, ) @@ -202,6 +203,7 @@ def test_search_patient_by_nhs_number_get_patient_success( auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", + nhsd_session_urid="test-urid", ) result = client.search_patient_by_nhs_number(9000000009) @@ -471,3 +473,101 @@ def test_find_current_record_with_today_override() -> None: assert find_current_record(records, today=date(2020, 6, 1)) == records[0] assert find_current_record(records, today=date(2021, 6, 1)) == records[1] assert find_current_record(records, today=date(2019, 6, 1)) is None + + +def test_find_current_name_record_no_current_name() -> None: + """ + Verify that ``find_current_name_record`` returns ``None`` when no current name + exists. + + :return: ``None``. + """ + records = cast( + "ResultList", + [ + { + "use": "official", + "family": "Doe", + "given": ["John"], + "period": {"start": "2000-01-01", "end": "2010-12-31"}, + }, + { + "use": "official", + "family": "Smith", + "given": ["John"], + "period": {"start": "2011-01-01", "end": "2020-12-31"}, + }, + ], + ) + + assert find_current_name_record(records) is None + + +def test_extract_single_search_result_invalid_bundle_raises_runtime_error() -> None: + """ + Verify that ``PdsSearch._extract_single_search_result`` raises ``RuntimeError`` when + mandatory bundle/patient content is missing. + + This test asserts that a ``RuntimeError`` is raised when: + + * The bundle contains no entries (``entry`` is missing or empty). + * The first entry exists but the patient resource has no NHS number (missing/blank + ``id``). + * The first entry exists and has an NHS number, but the patient has no *current* + name record (i.e. no ``Patient.name`` period covers "today"). + """ + client = PdsSearch( + auth_token="test-token", # noqa: S106 (test token hardcoded) + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ) + + # 1) Bundle contains no entries. + bundle_no_entries: Any = {"entry": []} + with pytest.raises(RuntimeError): + client._extract_single_search_result(bundle_no_entries) # noqa SLF001 (testing private method) + + # 2) Entry exists but patient has no NHS number (Patient.id missing/blank). + bundle_missing_nhs_number: Any = { + "entry": [ + { + "resource": { + "resourceType": "Patient", + "name": [ + { + "use": "official", + "family": "Smith", + "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "9999-12-31"}, + } + ], + "generalPractitioner": [], + } + } + ] + } + with pytest.raises(RuntimeError): + client._extract_single_search_result(bundle_missing_nhs_number) # noqa SLF001 (testing private method) + + # 3) Entry exists with NHS number, but no current name record. + bundle_no_current_name: Any = { + "entry": [ + { + "resource": { + "resourceType": "Patient", + "id": "9000000009", + "name": [ + { + "use": "official", + "family": "Smith", + "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "1900-12-31"}, + } + ], + "generalPractitioner": [], + } + } + ] + } + with pytest.raises(RuntimeError): + client._extract_single_search_result(bundle_no_current_name) # noqa SLF001 (testing private method) From f2ed5481ddca8134684cdd0880be5907ef44542a Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:42:58 +0000 Subject: [PATCH 22/32] Exclude stubs from coverage testing --- sonar-project.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonar-project.properties b/sonar-project.properties index 43ad5fd7..4ce10922 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -8,6 +8,6 @@ sonar.sourceEncoding=UTF-8 sonar.sources=.github,gateway-api,infrastructure,scripts,Makefile sonar.tests=tests,gateway-api/tests sonar.exclusions=docs/**,gateway-api/test-artefacts/**,gateway-api/coverage-html/**,gateway-api/tests/**,**/__pycache__/**,**/.venv/**,**/*.drawio -sonar.coverage.exclusions=**/tests/**,**/features/**,**/test_*.py +sonar.coverage.exclusions=**/tests/**,**/features/**,**/test_*.py,**/stubs/** # Set Python version for more precise analysis sonar.python.version=3.14 From 3f549093d733bc573e83d0eb2ca3c6f5e1d2daff Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Thu, 8 Jan 2026 18:33:49 +0000 Subject: [PATCH 23/32] Address review comments --- gateway-api/src/gateway_api/pds_search.py | 76 ++++++------ .../src/gateway_api/test_pds_search.py | 115 ++++++++---------- 2 files changed, 88 insertions(+), 103 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 16bd3156..1f7447de 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -1,13 +1,6 @@ """ PDS (Personal Demographics Service) FHIR R4 patient lookup client. -Terminology ------------ - -FHIR elements such as ``Patient.name`` and ``Patient.generalPractitioner`` can contain -multiple records, each valid for a specific time range (a FHIR ``Period``). This module -selects the record that is *current* for a given date. - Contracts enforced by the helper functions: * ``Patient.name[]`` records passed to :func:`find_current_name_record` must contain:: @@ -75,7 +68,7 @@ class SearchResults: gp_ods_code: str | None -class PdsSearch: +class PdsClient: """ Simple client for PDS FHIR R4 patient retrieval. @@ -95,7 +88,7 @@ class PdsSearch: **Usage example**:: - pds = PdsSearch( + pds = PdsClient( auth_token="YOUR_ACCESS_TOKEN", end_user_org_ods="A12345", base_url="https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4", @@ -177,9 +170,8 @@ def search_patient_by_nhs_number( """ Retrieve a patient by NHS number. - Calls ``GET /Patient/{nhs_number}``, expects a Bundle-like JSON response in the - shape used by the unit test stub (``{"entry": [{"resource": }]}``), - then extracts a single :class:`SearchResults`. + Calls ``GET /Patient/{nhs_number}``, which returns a single FHIR Patient + resource on success, then extracts a single :class:`SearchResults`. :param nhs_number: NHS number to search for. :param request_id: Optional request ID to reuse for retries; if not supplied a @@ -212,8 +204,8 @@ def search_patient_by_nhs_number( except requests.HTTPError as err: raise ExternalServiceError("PDS request failed") from err - bundle = response.json() - return self._extract_single_search_result(bundle) + body = response.json() + return self._extract_single_search_result(body) # --------------- internal helpers for result extraction ----------------- @@ -238,7 +230,7 @@ def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: if len(general_practitioners) == 0: return None - gp = find_current_record(general_practitioners) + gp = find_current_gp(general_practitioners) if gp is None: return None @@ -249,31 +241,39 @@ def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: return None if ods_code == "None" else ods_code def _extract_single_search_result( - self, bundle: ResultStructureDict + self, body: ResultStructureDict ) -> SearchResults | None: """ - Extract a single :class:`SearchResults` from a FHIR Bundle. + Extract a single :class:`SearchResults` from a Patient response. - The code assumes that the API returns either zero matches (empty entry list) - or a single match; if multiple entries are present, the first entry is used. + This helper accepts either: + * a single FHIR Patient resource (as returned by ``GET /Patient/{id}``), or + * a FHIR Bundle containing Patient entries (as typically returned by searches). - :param bundle: Parsed JSON body containing at minimum an ``entry`` list, where - the first entry contains a Patient resource under ``resource``. + For Bundle inputs, the code assumes either zero matches (empty entry list) or a + single match; if multiple entries are present, the first entry is used. + :param body: Parsed JSON body containing either a Patient resource or a Bundle + whose first entry contains a Patient resource under ``resource``. :return: A populated :class:`SearchResults` if extraction succeeds, otherwise ``None``. - :raises KeyError: If required fields for current record selection - (period fields) are missing in the evaluated structures. """ - entries: ResultList = cast("ResultList", bundle.get("entry", [])) - if not entries: - raise RuntimeError("PDS response contains no patient entries") - - # Use the first patient entry. Search by NHS number is unique. Search by - # demographics for an application is allowed to return max one entry from PDS. - # (search by a human can return more, but presumably we count as an application) - # See MaxResults parameter in the PDS OpenAPI spec. - entry = entries[0] - patient = cast("ResultStructureDict", entry.get("resource", {})) + # Accept either: + # 1) Patient (GET /Patient/{id}) + # 2) Bundle with Patient in entry[0].resource (search endpoints) + if str(body.get("resourceType", "")) == "Patient": + patient = body + else: + entries: ResultList = cast("ResultList", body.get("entry", [])) + if not entries: + raise RuntimeError("PDS response contains no patient entries") + + # Use the first patient entry. Search by NHS number is unique. Search by + # demographics for an application is allowed to return max one entry from + # PDS. Search by a human can return more, but presumably we count as an + # application. + # See MaxResults parameter in the PDS OpenAPI spec. + entry = entries[0] + patient = cast("ResultStructureDict", entry.get("resource", {})) nhs_number = str(patient.get("id", "")).strip() if not nhs_number: @@ -281,12 +281,12 @@ def _extract_single_search_result( # Select current name record and extract names. names = cast("ResultList", patient.get("name", [])) - name_obj = find_current_name_record(names) - if name_obj is None: + current_name = find_current_name_record(names) + if current_name is None: raise RuntimeError("PDS patient has no current name record") - given_names_list = cast("list[str]", name_obj.get("given", [])) - family_name = str(name_obj.get("family", "")) or "" + given_names_list = cast("list[str]", current_name.get("given", [])) + family_name = str(current_name.get("family", "")) or "" given_names_str = " ".join(given_names_list).strip() # Extract GP ODS code if a current GP record exists. @@ -301,7 +301,7 @@ def _extract_single_search_result( ) -def find_current_record( +def find_current_gp( records: ResultList, today: date | None = None ) -> ResultStructureDict | None: """ diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 23410fd4..01c2dd79 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -16,10 +16,10 @@ from gateway_api.pds_search import ( ExternalServiceError, - PdsSearch, + PdsClient, ResultList, + find_current_gp, find_current_name_record, - find_current_record, ) @@ -28,7 +28,7 @@ class FakeResponse: """ Minimal substitute for :class:`requests.Response` used by tests. - Only the methods accessed by :class:`gateway_api.pds_search.PdsSearch` are + Only the methods accessed by :class:`gateway_api.pds_search.PdsClient` are implemented. :param status_code: HTTP status code. @@ -67,7 +67,7 @@ def stub() -> PdsFhirApiStub: :return: A :class:`stubs.stub_pds.PdsFhirApiStub` with strict header validation enabled. """ - # Strict header validation helps ensure PdsSearch sends X-Request-ID correctly. + # Strict header validation helps ensure PdsClient sends X-Request-ID correctly. return PdsFhirApiStub(strict_headers=True) @@ -126,11 +126,8 @@ def _fake_get( end_user_org_ods=headers.get("NHSD-End-User-Organisation-ODS"), ) - # PdsSearch expects a Bundle-like structure on success in these tests. - if stub_resp.status_code == 200: - body = {"entry": [{"resource": stub_resp.json}]} - else: - body = stub_resp.json + # GET /Patient/{id} returns a single Patient resource on success. + body = stub_resp.json return FakeResponse( status_code=stub_resp.status_code, headers=stub_resp.headers, _json=body @@ -199,7 +196,7 @@ def test_search_patient_by_nhs_number_get_patient_success( general_practitioner=[], ) - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -252,7 +249,7 @@ def test_search_patient_by_nhs_number_no_current_gp_returns_gp_ods_code_none( ], ) - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -293,7 +290,7 @@ def test_search_patient_by_nhs_number_sends_expected_headers( general_practitioner=[], ) - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -340,7 +337,7 @@ def test_search_patient_by_nhs_number_generates_request_id( general_practitioner=[], ) - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -370,7 +367,7 @@ def test_search_patient_by_nhs_number_not_found_raises_error( :param mock_requests_get: Patched ``requests.get`` fixture. :return: ``None``. """ - pds = PdsSearch( + pds = PdsClient( auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -432,7 +429,7 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( version_id=1, ) - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", @@ -446,9 +443,9 @@ def test_search_patient_by_nhs_number_extracts_current_gp_ods_code( assert result.gp_ods_code == "CURRGP" -def test_find_current_record_with_today_override() -> None: +def test_find_current_gp_with_today_override() -> None: """ - Verify that ``find_current_record`` honours an explicit ``today`` value. + Verify that ``find_current_gp`` honours an explicit ``today`` value. :return: ``None``. """ @@ -470,9 +467,9 @@ def test_find_current_record_with_today_override() -> None: ], ) - assert find_current_record(records, today=date(2020, 6, 1)) == records[0] - assert find_current_record(records, today=date(2021, 6, 1)) == records[1] - assert find_current_record(records, today=date(2019, 6, 1)) is None + assert find_current_gp(records, today=date(2020, 6, 1)) == records[0] + assert find_current_gp(records, today=date(2021, 6, 1)) == records[1] + assert find_current_gp(records, today=date(2019, 6, 1)) is None def test_find_current_name_record_no_current_name() -> None: @@ -503,71 +500,59 @@ def test_find_current_name_record_no_current_name() -> None: assert find_current_name_record(records) is None -def test_extract_single_search_result_invalid_bundle_raises_runtime_error() -> None: +def test_extract_single_search_result_invalid_body_raises_runtime_error() -> None: """ - Verify that ``PdsSearch._extract_single_search_result`` raises ``RuntimeError`` when - mandatory bundle/patient content is missing. + Verify that ``PdsClient._extract_single_search_result`` raises ``RuntimeError`` when + mandatory patient content is missing. This test asserts that a ``RuntimeError`` is raised when: - * The bundle contains no entries (``entry`` is missing or empty). - * The first entry exists but the patient resource has no NHS number (missing/blank - ``id``). - * The first entry exists and has an NHS number, but the patient has no *current* - name record (i.e. no ``Patient.name`` period covers "today"). + * The body is a bundle containing no entries (``entry`` is empty). + * The body is a patient resource with no NHS number (missing/blank ``id``). + * The body is a patient resource with an NHS number, + but the patient has no *current* """ - client = PdsSearch( + client = PdsClient( auth_token="test-token", # noqa: S106 (test token hardcoded) end_user_org_ods="A12345", base_url="https://example.test/personal-demographics/FHIR/R4", ) # 1) Bundle contains no entries. - bundle_no_entries: Any = {"entry": []} + bundle_no_entries: Any = {"resourceType": "Bundle", "entry": []} with pytest.raises(RuntimeError): client._extract_single_search_result(bundle_no_entries) # noqa SLF001 (testing private method) - # 2) Entry exists but patient has no NHS number (Patient.id missing/blank). - bundle_missing_nhs_number: Any = { - "entry": [ + # 2) Patient has no NHS number (Patient.id missing/blank). + patient_missing_nhs_number: Any = { + "resourceType": "Patient", + "name": [ { - "resource": { - "resourceType": "Patient", - "name": [ - { - "use": "official", - "family": "Smith", - "given": ["Jane"], - "period": {"start": "1900-01-01", "end": "9999-12-31"}, - } - ], - "generalPractitioner": [], - } + "use": "official", + "family": "Smith", + "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "9999-12-31"}, } - ] + ], + "generalPractitioner": [], } with pytest.raises(RuntimeError): - client._extract_single_search_result(bundle_missing_nhs_number) # noqa SLF001 (testing private method) + client._extract_single_search_result(patient_missing_nhs_number) # noqa SLF001 (testing private method) - # 3) Entry exists with NHS number, but no current name record. - bundle_no_current_name: Any = { - "entry": [ + # 3) Patient has NHS number, but no current name record. + patient_no_current_name: Any = { + "resourceType": "Patient", + "id": "9000000009", + "name": [ { - "resource": { - "resourceType": "Patient", - "id": "9000000009", - "name": [ - { - "use": "official", - "family": "Smith", - "given": ["Jane"], - "period": {"start": "1900-01-01", "end": "1900-12-31"}, - } - ], - "generalPractitioner": [], - } + "use": "official", + "family": "Smith", + "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "1900-12-31"}, } - ] + ], + "generalPractitioner": [], } + with pytest.raises(RuntimeError): - client._extract_single_search_result(bundle_no_current_name) # noqa SLF001 (testing private method) + client._extract_single_search_result(patient_no_current_name) # noqa SLF001 (testing private method) From 722c245270a7cca9041fc20ed4bbd313df0197b6 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 9 Jan 2026 10:14:33 +0000 Subject: [PATCH 24/32] Replace TypeAlias with type --- gateway-api/src/gateway_api/pds_search.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 1f7447de..3d4bad27 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -23,16 +23,14 @@ import uuid from dataclasses import dataclass from datetime import date, datetime, timezone -from typing import TypeAlias, cast +from typing import cast import requests # Recursive JSON-like structure typing used for parsed FHIR bodies. -ResultStructure: TypeAlias = ( - str | dict[str, "ResultStructure"] | list["ResultStructure"] -) -ResultStructureDict: TypeAlias = dict[str, ResultStructure] -ResultList: TypeAlias = list[ResultStructureDict] +type ResultStructure = str | dict[str, "ResultStructure"] | list["ResultStructure"] +type ResultStructureDict = dict[str, ResultStructure] +type ResultList = list[ResultStructureDict] class ExternalServiceError(Exception): From 554386b9c6a3746f924bda6074d39af62bdf6505 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 9 Jan 2026 10:23:31 +0000 Subject: [PATCH 25/32] Tweak test to fix coverage --- .../src/gateway_api/test_pds_search.py | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 01c2dd79..a600d205 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -539,20 +539,27 @@ def test_extract_single_search_result_invalid_body_raises_runtime_error() -> Non with pytest.raises(RuntimeError): client._extract_single_search_result(patient_missing_nhs_number) # noqa SLF001 (testing private method) - # 3) Patient has NHS number, but no current name record. - patient_no_current_name: Any = { - "resourceType": "Patient", - "id": "9000000009", - "name": [ + # 3) Bundle entry exists with NHS number, but no current name record. + bundle_no_current_name: Any = { + "resourceType": "Bundle", + "entry": [ { - "use": "official", - "family": "Smith", - "given": ["Jane"], - "period": {"start": "1900-01-01", "end": "1900-12-31"}, + "resource": { + "resourceType": "Patient", + "id": "9000000009", + "name": [ + { + "use": "official", + "family": "Smith", + "given": ["Jane"], + "period": {"start": "1900-01-01", "end": "1900-12-31"}, + } + ], + "generalPractitioner": [], + } } ], - "generalPractitioner": [], } with pytest.raises(RuntimeError): - client._extract_single_search_result(patient_no_current_name) # noqa SLF001 (testing private method) + client._extract_single_search_result(bundle_no_current_name) # noqa SLF001 (testing private method) From 3052f0f739121c688689321d736316e3ac2297d2 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 9 Jan 2026 10:25:32 +0000 Subject: [PATCH 26/32] Fix docstring --- gateway-api/src/gateway_api/pds_search.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 3d4bad27..83e582aa 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -74,14 +74,7 @@ class PdsClient: * :meth:`search_patient_by_nhs_number` - calls ``GET /Patient/{nhs_number}`` - There is another operation implemented for searching by demographics: - - * :meth:`search_patient_by_details` - calls ``GET /Patient`` with query parameters - - ...but this is currently not fully tested. Its implementation may be finalised - in a later phase if it is required. - - Both methods return a :class:`SearchResults` instance when a patient can be + This method returns a :class:`SearchResults` instance when a patient can be extracted, otherwise ``None``. **Usage example**:: From 29cd91503a7a622586faaa6ab3e9b9f29bd1a2b6 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Fri, 9 Jan 2026 17:07:10 +0000 Subject: [PATCH 27/32] Update date ignore option --- gateway-api/src/gateway_api/pds_search.py | 139 ++++++++++-------- .../src/gateway_api/test_pds_search.py | 18 ++- 2 files changed, 89 insertions(+), 68 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 83e582aa..7254406f 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -103,6 +103,7 @@ def __init__( base_url: str = SANDBOX_URL, nhsd_session_urid: str | None = None, timeout: int = 10, + ignore_dates: bool = False, ) -> None: """ Create a PDS client. @@ -113,12 +114,15 @@ def __init__( :attr:`INT_URL`, :attr:`PROD_URL`). Trailing slashes are stripped. :param nhsd_session_urid: Optional ``NHSD-Session-URID`` header value. :param timeout: Default timeout in seconds for HTTP calls. + :param ignore_dates: If ``True`` just get the most recent name or GP record, + ignoring the date ranges. """ self.auth_token = auth_token self.end_user_org_ods = end_user_org_ods self.base_url = base_url.rstrip("/") self.nhsd_session_urid = nhsd_session_urid self.timeout = timeout + self.ignore_dates = ignore_dates def _build_headers( self, @@ -134,12 +138,15 @@ def _build_headers( :return: Dictionary of HTTP headers for the outbound request. """ headers = { - "Authorization": f"Bearer {self.auth_token}", "X-Request-ID": request_id or str(uuid.uuid4()), "NHSD-End-User-Organisation-ODS": self.end_user_org_ods, "Accept": "application/fhir+json", } + # Trying to pass an auth token to the sandbox makes PDS unhappy + if self.base_url != self.SANDBOX_URL: + headers["Authorization"] = f"Bearer {self.auth_token}" + # NHSD-Session-URID is required in some flows; include only if configured. if self.nhsd_session_urid: headers["NHSD-Session-URID"] = self.nhsd_session_urid @@ -200,8 +207,7 @@ def search_patient_by_nhs_number( # --------------- internal helpers for result extraction ----------------- - @staticmethod - def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: + def _get_gp_ods_code(self, general_practitioners: ResultList) -> str | None: """ Extract the current GP ODS code from ``Patient.generalPractitioner``. @@ -221,7 +227,7 @@ def _get_gp_ods_code(general_practitioners: ResultList) -> str | None: if len(general_practitioners) == 0: return None - gp = find_current_gp(general_practitioners) + gp = self.find_current_gp(general_practitioners) if gp is None: return None @@ -272,7 +278,7 @@ def _extract_single_search_result( # Select current name record and extract names. names = cast("ResultList", patient.get("name", [])) - current_name = find_current_name_record(names) + current_name = self.find_current_name_record(names) if current_name is None: raise RuntimeError("PDS patient has no current name record") @@ -291,79 +297,88 @@ def _extract_single_search_result( gp_ods_code=gp_ods_code, ) + def find_current_gp( + self, records: ResultList, today: date | None = None + ) -> ResultStructureDict | None: + """ + Select the current record from a ``generalPractitioner`` list. -def find_current_gp( - records: ResultList, today: date | None = None -) -> ResultStructureDict | None: - """ - Select the current record from a ``generalPractitioner`` list. + A record is "current" if its ``identifier.period`` covers ``today`` (inclusive): - A record is "current" if its ``identifier.period`` covers ``today`` (inclusive): + ``start <= today <= end`` - ``start <= today <= end`` + Or else if self.ignore_dates is True, the last record in the list is returned. - The list may be in any of the following states: + The list may be in any of the following states: - * empty - * contains one or more records, none current - * contains one or more records, exactly one current + * empty + * contains one or more records, none current + * contains one or more records, exactly one current - :param records: List of ``generalPractitioner`` records. - :param today: Optional override date, intended for deterministic tests. - If not supplied, the current UTC date is used. - :return: The first record whose ``identifier.period`` covers ``today``, or ``None`` - if no record is current. - :raises KeyError: If required keys are missing for a record being evaluated. - :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. - """ - if today is None: - today = datetime.now(timezone.utc).date() + :param records: List of ``generalPractitioner`` records. + :param today: Optional override date, intended for deterministic tests. + If not supplied, the current UTC date is used. + :return: The first record whose ``identifier.period`` covers ``today``, or + ``None`` if no record is current. + :raises KeyError: If required keys are missing for a record being evaluated. + :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. + """ + if today is None: + today = datetime.now(timezone.utc).date() - for record in records: - identifier = cast("ResultStructureDict", record["identifier"]) - periods = cast("dict[str, str]", identifier["period"]) - start_str = periods["start"] - end_str = periods["end"] + if self.ignore_dates: + return records[-1] - start = date.fromisoformat(start_str) - end = date.fromisoformat(end_str) + for record in records: + identifier = cast("ResultStructureDict", record["identifier"]) + periods = cast("dict[str, str]", identifier["period"]) + start_str = periods["start"] + end_str = periods["end"] - if start <= today <= end: - return record + start = date.fromisoformat(start_str) + end = date.fromisoformat(end_str) - return None + if start <= today <= end: + return record + return None -def find_current_name_record( - records: ResultList, today: date | None = None -) -> ResultStructureDict | None: - """ - Select the current record from a ``Patient.name`` list. + def find_current_name_record( + self, records: ResultList, today: date | None = None + ) -> ResultStructureDict | None: + """ + Select the current record from a ``Patient.name`` list. - A record is "current" if its ``period`` covers ``today`` (inclusive): + A record is "current" if its ``period`` covers ``today`` (inclusive): - ``start <= today <= end`` + ``start <= today <= end`` - :param records: List of ``Patient.name`` records. - :param today: Optional override date, intended for deterministic tests. - If not supplied, the current UTC date is used. - :return: The first name record whose ``period`` covers ``today``, or ``None`` if no - record is current. - :raises KeyError: If required keys (``period.start`` / ``period.end``) are missing. - :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. - """ - if today is None: - today = datetime.now(timezone.utc).date() + Or else if self.ignore_dates is True, the last record in the list is returned. + + :param records: List of ``Patient.name`` records. + :param today: Optional override date, intended for deterministic tests. + If not supplied, the current UTC date is used. + :return: The first name record whose ``period`` covers ``today``, or ``None`` if + no record is current. + :raises KeyError: If required keys (``period.start`` / ``period.end``) are + missing. + :raises ValueError: If ``start`` or ``end`` are not valid ISO date strings. + """ + if today is None: + today = datetime.now(timezone.utc).date() + + if self.ignore_dates: + return records[-1] - for record in records: - periods = cast("dict[str, str]", record["period"]) - start_str = periods["start"] - end_str = periods["end"] + for record in records: + periods = cast("dict[str, str]", record["period"]) + start_str = periods["start"] + end_str = periods["end"] - start = date.fromisoformat(start_str) - end = date.fromisoformat(end_str) + start = date.fromisoformat(start_str) + end = date.fromisoformat(end_str) - if start <= today <= end: - return record + if start <= today <= end: + return record - return None + return None diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index a600d205..6d7310b2 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -18,8 +18,6 @@ ExternalServiceError, PdsClient, ResultList, - find_current_gp, - find_current_name_record, ) @@ -449,6 +447,9 @@ def test_find_current_gp_with_today_override() -> None: :return: ``None``. """ + pds = PdsClient("test-token", "A12345") + pds_ignore_dates = PdsClient("test-token", "A12345", ignore_dates=True) + records = cast( "ResultList", [ @@ -467,9 +468,10 @@ def test_find_current_gp_with_today_override() -> None: ], ) - assert find_current_gp(records, today=date(2020, 6, 1)) == records[0] - assert find_current_gp(records, today=date(2021, 6, 1)) == records[1] - assert find_current_gp(records, today=date(2019, 6, 1)) is None + assert pds.find_current_gp(records, today=date(2020, 6, 1)) == records[0] + assert pds.find_current_gp(records, today=date(2021, 6, 1)) == records[1] + assert pds.find_current_gp(records, today=date(2019, 6, 1)) is None + assert pds_ignore_dates.find_current_gp(records, today=date(2019, 6, 1)) is not None def test_find_current_name_record_no_current_name() -> None: @@ -479,6 +481,9 @@ def test_find_current_name_record_no_current_name() -> None: :return: ``None``. """ + pds = PdsClient("test-token", "A12345") + pds_ignore_date = PdsClient("test-token", "A12345", ignore_dates=True) + records = cast( "ResultList", [ @@ -497,7 +502,8 @@ def test_find_current_name_record_no_current_name() -> None: ], ) - assert find_current_name_record(records) is None + assert pds.find_current_name_record(records) is None + assert pds_ignore_date.find_current_name_record(records) is not None def test_extract_single_search_result_invalid_body_raises_runtime_error() -> None: From ba5dd35510a25d4807c49bd31acf4597d8f086a0 Mon Sep 17 00:00:00 2001 From: DWolfsNHS <229101201+DWolfsNHS@users.noreply.github.com> Date: Fri, 9 Jan 2026 15:35:12 +0000 Subject: [PATCH 28/32] [GPCAPIM-166]: Update target group name and ECS task definition architecture - Trim trailing hyphens from the target group name - Change ECS task definition CPU architecture from ARM64 to X86_64 --- infrastructure/environments/preview/main.tf | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/infrastructure/environments/preview/main.tf b/infrastructure/environments/preview/main.tf index 334669dd..cfa32753 100644 --- a/infrastructure/environments/preview/main.tf +++ b/infrastructure/environments/preview/main.tf @@ -62,7 +62,7 @@ locals { ############################ resource "aws_lb_target_group" "branch" { - name = substr(replace(local.effective_host_name, ".", "-"), 0, 32) + name = trim(substr(replace(var.branch_name, ".", "-"), 0, 32), "-") port = var.container_port protocol = "HTTP" target_type = "ip" @@ -179,7 +179,7 @@ resource "aws_ecs_task_definition" "branch" { task_role_arn = aws_iam_role.task.arn runtime_platform { - cpu_architecture = "ARM64" + cpu_architecture = "X86_64" operating_system_family = "LINUX" } From 331e00e82c8bd2e73beb83c536ff777adfc9c4d4 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 12 Jan 2026 21:56:03 +0000 Subject: [PATCH 29/32] Fix case with no names --- gateway-api/src/gateway_api/pds_search.py | 26 +++++++++++----- .../src/gateway_api/test_pds_search.py | 31 ++++++++++++++++--- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/gateway-api/src/gateway_api/pds_search.py b/gateway-api/src/gateway_api/pds_search.py index 7254406f..cddcc056 100644 --- a/gateway-api/src/gateway_api/pds_search.py +++ b/gateway-api/src/gateway_api/pds_search.py @@ -200,7 +200,9 @@ def search_patient_by_nhs_number( # In production, failures surface here (4xx/5xx -> HTTPError). response.raise_for_status() except requests.HTTPError as err: - raise ExternalServiceError("PDS request failed") from err + raise ExternalServiceError( + f"PDS request failed: {err.response.reason}" + ) from err body = response.json() return self._extract_single_search_result(body) @@ -279,12 +281,14 @@ def _extract_single_search_result( # Select current name record and extract names. names = cast("ResultList", patient.get("name", [])) current_name = self.find_current_name_record(names) - if current_name is None: - raise RuntimeError("PDS patient has no current name record") - given_names_list = cast("list[str]", current_name.get("given", [])) - family_name = str(current_name.get("family", "")) or "" - given_names_str = " ".join(given_names_list).strip() + if current_name is not None: + given_names_list = cast("list[str]", current_name.get("given", [])) + family_name = str(current_name.get("family", "")) or "" + given_names_str = " ".join(given_names_list).strip() + else: + given_names_str = "" + family_name = "" # Extract GP ODS code if a current GP record exists. gp_list = cast("ResultList", patient.get("generalPractitioner", [])) @@ -327,7 +331,10 @@ def find_current_gp( today = datetime.now(timezone.utc).date() if self.ignore_dates: - return records[-1] + if len(records) > 0: + return records[-1] + else: + return None for record in records: identifier = cast("ResultStructureDict", record["identifier"]) @@ -368,7 +375,10 @@ def find_current_name_record( today = datetime.now(timezone.utc).date() if self.ignore_dates: - return records[-1] + if len(records) > 0: + return records[-1] + else: + return None for record in records: periods = cast("dict[str, str]", record["period"]) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 6d7310b2..29a1c527 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -37,6 +37,7 @@ class FakeResponse: status_code: int headers: dict[str, str] _json: dict[str, Any] + reason: str = "" def json(self) -> dict[str, Any]: """ @@ -54,7 +55,10 @@ def raise_for_status(self) -> None: :raises requests.HTTPError: If the response status is not 200. """ if self.status_code != 200: - raise requests.HTTPError(f"{self.status_code} Error") + err = requests.HTTPError(f"{self.status_code} Error") + # requests attaches a Response to HTTPError.response; the client expects it + err.response = self + raise err @pytest.fixture @@ -126,9 +130,24 @@ def _fake_get( # GET /Patient/{id} returns a single Patient resource on success. body = stub_resp.json + # Populate a reason phrase so PdsClient can surface it in ExternalServiceError. + reason = "" + if stub_resp.status_code != 200: + # Try to use OperationOutcome display text if present. + issue0 = (stub_resp.json.get("issue") or [{}])[0] + details = issue0.get("details") or {} + coding0 = (details.get("coding") or [{}])[0] + reason = str(coding0.get("display") or "") + if not reason: + reason = {400: "Bad Request", 404: "Not Found"}.get( + stub_resp.status_code, "" + ) return FakeResponse( - status_code=stub_resp.status_code, headers=stub_resp.headers, _json=body + status_code=stub_resp.status_code, + headers=stub_resp.headers, + _json=body, + reason=reason, ) monkeypatch.setattr(requests, "get", _fake_get) @@ -567,5 +586,9 @@ def test_extract_single_search_result_invalid_body_raises_runtime_error() -> Non ], } - with pytest.raises(RuntimeError): - client._extract_single_search_result(bundle_no_current_name) # noqa SLF001 (testing private method) + # No current name record is tolerated by PdsClient; names are returned as empty. + result = client._extract_single_search_result(bundle_no_current_name) # noqa SLF001 (testing private method) + assert result is not None + assert result.nhs_number == "9000000009" + assert result.given_names == "" + assert result.family_name == "" From 1d648a41f7f715cafbf9236da4a111477fe849a9 Mon Sep 17 00:00:00 2001 From: DWolfsNHS <229101201+DWolfsNHS@users.noreply.github.com> Date: Fri, 9 Jan 2026 14:50:03 +0000 Subject: [PATCH 30/32] [GPCAPIM-166]: Improve branch name sanitization in preview workflow --- .github/workflows/preview-env.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/preview-env.yml b/.github/workflows/preview-env.yml index 06ca3dec..28901d2a 100644 --- a/.github/workflows/preview-env.yml +++ b/.github/workflows/preview-env.yml @@ -55,8 +55,9 @@ jobs: SANITIZED_BRANCH=$( printf '%s' "$RAW_BRANCH" \ | tr '[:upper:]' '[:lower:]' \ - | tr -c 'a-z0-9._-' '-' \ - | sed -E 's/^[^a-z0-9]+//; s/[^a-z0-9]+$//' + | tr '._' '-' \ + | tr -c 'a-z0-9-' '-' \ + | sed -E 's/-{2,}/-/g; s/^-+//; s/-+$//' ) # Last resort fallback if everything got stripped From 3db48fe0abeacc40f4fa0bed82ce7c357374cbd7 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 12 Jan 2026 22:15:15 +0000 Subject: [PATCH 31/32] Fix line length and comments --- .../src/gateway_api/test_pds_search.py | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 29a1c527..35bccf88 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -592,3 +592,67 @@ def test_extract_single_search_result_invalid_body_raises_runtime_error() -> Non assert result.nhs_number == "9000000009" assert result.given_names == "" assert result.family_name == "" + + +def test_search_patient_by_nhs_number_ignore_dates_true_handles_missing_name_or_gp( + stub: PdsFhirApiStub, + mock_requests_get: dict[str, Any], +) -> None: + """ + Verify that when ignore_dates=True the client tolerates: + * no Patient.name records (names returned as empty strings) + * no Patient.generalPractitioner records (gp_ods_code returned as None) + """ + # Case 1: Patient returned with no "name" field at all. + stub.upsert_patient( + nhs_number="9000000100", + patient={ + "resourceType": "Patient", + # No "name" + "generalPractitioner": [], + }, + version_id=1, + ) + + client = PdsClient( + auth_token="test-token", # noqa: S106 (test token hardcoded) + end_user_org_ods="A12345", + base_url="https://example.test/personal-demographics/FHIR/R4", + ignore_dates=True, + ) + + result_no_name = client.search_patient_by_nhs_number(9000000100) + + assert result_no_name is not None + assert result_no_name.nhs_number == "9000000100" + assert result_no_name.given_names == "" + assert result_no_name.family_name == "" + assert result_no_name.gp_ods_code is None + + # Case 2: Patient returned with a name, but no "generalPractitioner" field at all. + # Also use an out-of-date period to demonstrate ignore_dates=True still selects + # a record. + stub.upsert_patient( + nhs_number="9000000101", + patient={ + "resourceType": "Patient", + "name": [ + { + "use": "official", + "family": "Jones", + "given": ["Gwyneth"], + "period": {"start": "1900-01-01", "end": "1900-12-31"}, + } + ], + # No "generalPractitioner" + }, + version_id=1, + ) + + result_no_gp = client.search_patient_by_nhs_number(9000000101) + + assert result_no_gp is not None + assert result_no_gp.nhs_number == "9000000101" + assert result_no_gp.given_names == "Gwyneth" + assert result_no_gp.family_name == "Jones" + assert result_no_gp.gp_ods_code is None From e19050a67caffd005efb552576f3c9febed59504 Mon Sep 17 00:00:00 2001 From: Ben Taylor <59649826+Vox-Ben@users.noreply.github.com> Date: Mon, 12 Jan 2026 22:29:28 +0000 Subject: [PATCH 32/32] Actually fix coverage this time --- .../src/gateway_api/test_pds_search.py | 102 +++++++++--------- 1 file changed, 52 insertions(+), 50 deletions(-) diff --git a/gateway-api/src/gateway_api/test_pds_search.py b/gateway-api/src/gateway_api/test_pds_search.py index 35bccf88..78ed9e73 100644 --- a/gateway-api/src/gateway_api/test_pds_search.py +++ b/gateway-api/src/gateway_api/test_pds_search.py @@ -594,65 +594,67 @@ def test_extract_single_search_result_invalid_body_raises_runtime_error() -> Non assert result.family_name == "" -def test_search_patient_by_nhs_number_ignore_dates_true_handles_missing_name_or_gp( - stub: PdsFhirApiStub, - mock_requests_get: dict[str, Any], -) -> None: +def test_find_current_name_record_ignore_dates_returns_last_or_none() -> None: """ - Verify that when ignore_dates=True the client tolerates: - * no Patient.name records (names returned as empty strings) - * no Patient.generalPractitioner records (gp_ods_code returned as None) + If ignore_dates=True: + * returns the last name record even if none are current + * returns None when the list is empty """ - # Case 1: Patient returned with no "name" field at all. - stub.upsert_patient( - nhs_number="9000000100", - patient={ - "resourceType": "Patient", - # No "name" - "generalPractitioner": [], - }, - version_id=1, - ) + pds_ignore = PdsClient("test-token", "A12345", ignore_dates=True) - client = PdsClient( - auth_token="test-token", # noqa: S106 (test token hardcoded) - end_user_org_ods="A12345", - base_url="https://example.test/personal-demographics/FHIR/R4", - ignore_dates=True, + records = cast( + "ResultList", + [ + { + "use": "official", + "family": "Old", + "given": ["First"], + "period": {"start": "1900-01-01", "end": "1900-12-31"}, + }, + { + "use": "official", + "family": "Newer", + "given": ["Second"], + "period": {"start": "1901-01-01", "end": "1901-12-31"}, + }, + ], ) - result_no_name = client.search_patient_by_nhs_number(9000000100) + # Pick a date that is not covered by any record; ignore_dates should still pick last + chosen = pds_ignore.find_current_name_record(records, today=date(2026, 1, 1)) + assert chosen == records[-1] - assert result_no_name is not None - assert result_no_name.nhs_number == "9000000100" - assert result_no_name.given_names == "" - assert result_no_name.family_name == "" - assert result_no_name.gp_ods_code is None + assert pds_ignore.find_current_name_record(cast("ResultList", [])) is None - # Case 2: Patient returned with a name, but no "generalPractitioner" field at all. - # Also use an out-of-date period to demonstrate ignore_dates=True still selects - # a record. - stub.upsert_patient( - nhs_number="9000000101", - patient={ - "resourceType": "Patient", - "name": [ - { - "use": "official", - "family": "Jones", - "given": ["Gwyneth"], + +def test_find_current_gp_ignore_dates_returns_last_or_none() -> None: + """ + If ignore_dates=True: + * returns the last GP record even if none are current + * returns None when the list is empty + """ + pds_ignore = PdsClient("test-token", "A12345", ignore_dates=True) + + records = cast( + "ResultList", + [ + { + "identifier": { + "value": "GP-OLD", "period": {"start": "1900-01-01", "end": "1900-12-31"}, } - ], - # No "generalPractitioner" - }, - version_id=1, + }, + { + "identifier": { + "value": "GP-NEWER", + "period": {"start": "1901-01-01", "end": "1901-12-31"}, + } + }, + ], ) - result_no_gp = client.search_patient_by_nhs_number(9000000101) + # Pick a date that is not covered by any record; ignore_dates should still pick last + chosen = pds_ignore.find_current_gp(records, today=date(2026, 1, 1)) + assert chosen == records[-1] - assert result_no_gp is not None - assert result_no_gp.nhs_number == "9000000101" - assert result_no_gp.given_names == "Gwyneth" - assert result_no_gp.family_name == "Jones" - assert result_no_gp.gp_ods_code is None + assert pds_ignore.find_current_gp(cast("ResultList", [])) is None