Feature/gpcapim 251 #46
Conversation
- Create GPProviderClient class to interact with GPProvider FHIR API - Add initialization method with provider and consumer ASIDs - Implement get_structured_record method to fetch patient records - Update unit tests to verify successful response from GPProviderClient from example.com
- Change return type of get_structured_record method to Response - Update timeout parameter to None for quicker development - Add mock_request_get fixture to unit tests for simulating API responses
- Rename method `get_structured_record` to `access_structured_record` - Change HTTP method from GET to POST for fetching structured patient records - Update request headers to match GP Connect specifications - Enhance mock request handling in tests for POST requests - Add a GPProviderStub class to simulate structured patient record responses
- Correct header casing in GPProviderClient for FHIR API requests - Enhance GPProviderStub to return a Response object - Refactor mock_request_post to utilize GPProviderStub
…ap to https for dummy url
…PI requests - Implement _build_headers method to construct request headers - Update access_structured_record method to utilize new headers - Modify unit tests to verify correct headers are sent in requests
- Modify access_structured_record method to accept a body parameter - Update request handling to include body in POST requests - Add unit test to verify correct body is sent in requests
- Implement test to verify GPProviderClient returns correct response from stub provider - Remove outdated pseudo-code comments from test file - Update stub provider response handling for initial implementation
- Introduce a StubResponse class for simplified response representation - Update access_record_structured method to return StubResponse instead of Response - Enhance test cases to utilize FakeResponse for better testing of HTTP interactions
- Replace FakeResponse with a more generic StubResponse class - Update mock_request_post to return the actual stub response - Adjust content handling in tests to match new response structure
- Update interaction ID and base URL definitions for clarity - Adjust request timeout handling for better development flexibility - Enhance unit tests to verify correct URL formation and response status
DWolfsNHS
force-pushed
the
feature/GPCAPIM-251
branch
from
a73f501 to
89fa638
Compare
- Implement test to verify ExternalServiceError is raised on HTTP error - Simulate a 500 Internal Server Error response from the GPProvider API - Enhance test coverage for GpProviderClient error handling
|
Vox-Ben
left a comment
Vox-Ben
left a comment
There was a problem hiding this comment.
Looks good, comments mostly on non-critical stuff. :-)
| import requests | ||
| from requests import Response |
There was a problem hiding this comment.
Do you need to do this? You're basically importing from the same file twice (which AI seems to like doing and I'm not sure why). IMO better to just do from requests import Response, <whatever else you're using>.
| from requests import Response | ||
|
|
||
| # definitions | ||
| ars_interactionId = "urn:nhs:names:services:gpconnect:structured:fhir:operation:gpc.getstructuredrecord-1" # noqa: E501 this is standard InteractionID for accessRecordStructured |
There was a problem hiding this comment.
You can break this over two lines, it doesn't need to bust the line length limit.
ars_interactionId = ("urn:nhs:names:services:gpconnect:structured"
":fhir:operation:gpc.getstructuredrecord-1")
| This exception wraps :class:`requests.HTTPError` thrown by | ||
| ``response.raise_for_status()`` and re-raises it as ``ExternalServiceError`` | ||
| to decouple callers from the underlying ``requests`` library exception types. |
There was a problem hiding this comment.
I don't understand this comment. The exception as written isn't related to requests.HTTPError and doesn't have any of its own logic. I'd just delete this bit?
| timeout = None # None used for quicker dev, adjust as needed | ||
|
|
||
|
|
||
| class ExternalServiceError(Exception): |
There was a problem hiding this comment.
I think we could move this into a common library. I've started one in my current ticket anyway.
| Args: | ||
| provider_endpoint (str): The FHIR API endpoint for the provider. | ||
| provider_asid (str): The ASID for the provider. | ||
| consumer_asid (str): The ASID for the consumer. |
There was a problem hiding this comment.
I suggest we write docstrings with Sphinx-style argument listing. Then we can auto-generate our documentation (though that said I don't know if the likes of ChatGPT are going to obsolete the likes of Sphinx). Still, it's standard and it gives us the option, and it's easy enough to just ask the AI to do. Don't know if @davidhamill1-nhs and @nhsd-jack-wainwright agree?
| headers = self._build_headers(trace_id) | ||
|
|
||
| response = requests.post( | ||
| self.provider_endpoint + ars_fhir_base + "/patient/" + ars_fhir_operation, |
There was a problem hiding this comment.
I was convinced that Python had a built-in function for constructing URLs from arbitrary components, but it turns out that it doesn't. And you can't use pathlib either because if any component has a leading slash it drops all the previous components. I wonder if it's worth writing one ourselves.
| Fixtures: | ||
| - `stub`: Provides an instance of the `GpProviderStub` for simulating the GPProvider | ||
| - `mock_request_post`: Patches the `requests.post` method to intercept API calls and | ||
| route them to the stub provider. Captures request details for verification. |
There was a problem hiding this comment.
Not sure we really need to list the fixtures in the module docstring. Easy enough to find them by looking through the file, and the header will go out of date because nobody will update it.
|
|
||
| from gateway_api.provider_request import ExternalServiceError, GpProviderClient | ||
|
|
||
| ars_interactionId = "urn:nhs:names:services:gpconnect:structured:fhir:operation:gpc.getstructuredrecord-1" # noqa: E501 this is standard InteractionID for accessRecordStructured |
There was a problem hiding this comment.
As above don't need to #noqa this, just break it across two lines.
2 participants
Description
This pull request introduces the GpProviderClient class, which provides a simple client for interacting with the GPProvider FHIR GP System. The client includes methods to fetch structured patient records from a GPProvider FHIR API endpoint. Additionally, unit tests and a stub for the GPProvider FHIR API have been implemented to ensure the correctness of the client.
Context
This change is required to enable integration with the GPProvider FHIR GP System, allowing the retrieval of structured patient records. It addresses the need for a robust and reusable client to interact with the GPProvider API, ensuring compliance with the GPConnect specifications.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.