Conversation
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |
1 similar comment
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |
| ## Related APIs | ||
| The following APIs are related to this API: | ||
|
|
||
| - [Personal Demographics Service - FHIR API](https://digital.nhs.uk/developer/api-catalogue/personal-demographics-service-fhir) - we use the data held in PDS as a source of data to verify relationships. |
There was a problem hiding this comment.
Worth adding an entry to this for RPN to disclose our usage - perhaps only when we actually do the work (so an AC on the ticket for implementing that)
https://digital.nhs.uk/developer/api-catalogue/related-person-network---fhir-api
There was a problem hiding this comment.
On the API catalogue page there are words in the grey section at the top that are VERY similar to the opening pargraph in the Overview section. Where are these being set? they don't appear to be in the OAS or this GH repo at all. Para in question
Use this API to access the Validated Relationships Service - the national electronic database of relationships that have been verified for the purpose of enabling individuals to access healthcare services on behalf of (proxy) those they care for.
| | 400 | `INVALID_VALUE` | Invalid Parameter or Invalid operation. | | ||
| | 401 | `ACCESS_DENIED` | Missing or invalid OAuth 2.0 bearer token in request. | | ||
| | 403 | `FORBIDDEN` | Access denied to resource. | | ||
| | 404 | `INVALIDATED_RESOURCE` | Resource that has been marked as invalid was requested - invalid resources cannot be retrieved | |
There was a problem hiding this comment.
I'm assuming this is when a patient record can't be found for a patient or proxy. This should be a 400 error in my view, not a 404. A 404 pertains to our endpoint and will likely confuse as it hints at one of the below
- The POST endpoint itself does not exist
- The client addressed the wrong URL
It's neither of these things. What we're trying to communicate is that there is a record referenced in the request which is invalid or does not exist. That's essentially validation of the provided NHS numbers, as opposed to a QuestionnaireResponse resource not existing (which is the core of what REST is all about, your status codes are always contextualised by the Resource locator (URI) that you're addressing.
Suggest we move this to a 400, and make the error message more meaningful e.g. "One or more identifiers reference resources that don't exist"
| | 401 | `ACCESS_DENIED` | Missing or invalid OAuth 2.0 bearer token in request. | | ||
| | 403 | `FORBIDDEN` | Access denied to resource. | | ||
| | 404 | `QUESTIONNAIRE_RESPONSE_NOT_FOUND` | No questionnaire response was found for the provided access request ID. | | ||
| | 404 | `INVALIDATED_RESOURCE` | Resource that has been marked as invalid was requested - invalid resources cannot be retrieved | |
There was a problem hiding this comment.
Is this really a response that this endpoint could return? Suspect it can be removed?
| | 400 | `MISSING_VALUE` | Missing header or parameter. For details, see the `diagnostics` field. | | ||
| | 401 | `ACCESS_DENIED` | Missing or invalid OAuth 2.0 bearer token in request. | | ||
| | 403 | `FORBIDDEN` | Access denied to resource. | | ||
| | 404 | `QUESTIONNAIRE_RESPONSE_NOT_FOUND` | No questionnaire response was found for the provided access request ID. | |
There was a problem hiding this comment.
Opportunity to simplify.
Suspect we're making things more complicated for ourselves by making 404's specific to the endpoint. I'd say you could probably simplify to NOT_FOUND and The resource with the specified ID does not exist and then it removes the need to have endpoint specific handling of this standard type of error
| summary: Get verified candidate relationships | ||
| description: | | ||
| ## Overview | ||
| Use this endpoint to get candidate proxy relationships for a user with a given NHS Number. This endpoint should NOT be used to retrieve proxy relationships; these should be queried from the `/Consent` endpoint. |
There was a problem hiding this comment.
Suggest reword to make this description crystal clear that we aren't returning proxy's
Use this endpoint to get verified relationship information for a user with a given NHS Number. This endpoint should NOT be used to retrieve established proxy roles; these should be queried from the
GET /Consentendpoints.
| ## Overview | ||
| Use this endpoint to get candidate proxy relationships for a user with a given NHS Number. This endpoint should NOT be used to retrieve proxy relationships; these should be queried from the `/Consent` endpoint. | ||
|
|
||
| You can (optionally) include `patient:identifier={nhsNumber}` request parameter with the patient's NHS number to get a specific verified relationship for the proxy. |
There was a problem hiding this comment.
Now that we've updated this endpoint such that it can be queried essentially bi-directionally, think it would be good to describe that here. Replacing the above with something along the lines of the below:
You can query for RelatedPerson's in the following ways:
| Outcome | Query string value |
|---|---|
| Search for patients for a related person | identifier={identifier} |
| Search for related persons for a patient | patient:identifier={identifier} |
| Search for a specific verified relationship for a given related person and patient | identifier={identifier}&patient:identifier={identifier} |
There was a problem hiding this comment.
Worth also adding a callout of some description to make it clear that at least one of identifier or patient:identifier need to be supplied for the request to be valid
| path: /status | ||
| value: active | ||
| - op: add | ||
| path: /extension/- |
There was a problem hiding this comment.
are we missing the extension path here? how do we know which extension property we're updating?
| updateProvisionEndDate: | ||
| $ref: "./examples/requests/PATCH_Consent/replace_provision_end_date.yaml#/UpdateProvisionEndDate" | ||
| multipleUpdates: | ||
| summary: Set status to active with an end date |
There was a problem hiding this comment.
Suggest move this out to a $ref for consistency with how we reference other examples
| | 400 | `MISSING_VALUE` | Missing header or parameter. For details, see the `diagnostics` field. | | ||
| | 401 | `ACCESS_DENIED` | Missing or invalid OAuth 2.0 bearer token in request. | | ||
| | 403 | `FORBIDDEN` | Access denied to resource. | | ||
| | 404 | `INVALIDATED_RESOURCE` | Resource that has been marked as invalid was requested - invalid resources cannot be retrieved | |
There was a problem hiding this comment.
Don;t think this is relevant to us. But we don;t have a "NOT_FOUND" error code documented for 404, so this should replace it (and be implemented if not already)
| | ----------- | -------------------------- | ------------------------------------------------------------- | | ||
| | 500 | `SERVER_ERROR` | An unexpected internal server error has occurred. | | ||
| | 502 | `BAD_GATEWAY` | Connection to the backend service failed. | | ||
| | 503 | `DOWNSTREAM_SERVICE_ERROR` | A downstream service has failed, request cannot be completed. | |
There was a problem hiding this comment.
Don;t think there is a downstream service to error for this endpoint is there? Suggest removing
| | ----------- | -------------------------- | ------------------------------------------------------------- | | ||
| | 500 | `SERVER_ERROR` | An unexpected internal server error has occurred. | | ||
| | 502 | `BAD_GATEWAY` | Connection to the backend service failed. | | ||
| | 503 | `DOWNSTREAM_SERVICE_ERROR` | A downstream service has failed, request cannot be completed. | |
There was a problem hiding this comment.
Don;t think there is a downstream service to error for this endpoint is there? Suggest removing
| status: | ||
| type: string | ||
| description: "The status of the consent, following the ConsentStateCodes value set." | ||
| enum: |
There was a problem hiding this comment.
do we ACTUALLY support "draft" and "entered in error" ? What would happen if we received these values as far as the state machine is concerned? Return an error? I don't think we have a use case for these, so shouldn't accept them as valid. So would need removing from docs and validation not allow them through
| type: array | ||
| items: | ||
| type: string | ||
| enum: |
There was a problem hiding this comment.
do we ACTUALLY support "draft" and "entered in error" ? What would happen if we received these values as far as the state machine is concerned? Return an error? I don't think we have a use case for these, so shouldn't accept them as valid. So would need removing from docs and validation not allow them through
| description: Unique identifier of the Consent resource | ||
| status: | ||
| type: string | ||
| description: "The status of the consent, following the ConsentStateCodes value set." |
There was a problem hiding this comment.
Suggest we link out to the FHIR codesystem so implementors can refer to the valueset looks like
|
|
||
| Consent: | ||
| type: object | ||
| required: |
There was a problem hiding this comment.
We look to be missing a few properties from the Consent schema (for the READ model). These ought to be added to examples and sandbox as well as the implementation if not just missing from the API docs.
- Consent.meta.lastUpdated
- Extensions: grantor, regulatoryBasis
There was a problem hiding this comment.
Note that we have a meta OAS component documented below, so we should leverage this here
| - deny | ||
| period: | ||
| type: object | ||
| description: "Timeframe for this provision." |
There was a problem hiding this comment.
think we can provide something more meaningful for our context here. What about
The start date on which the proxy role was granted, and (optionally) an end date for time-bound access.
| code: | ||
| type: string | ||
| description: FHIR error code. | ||
| enum: |
There was a problem hiding this comment.
will we really return all of these? If not, i'd be tempted to simplify to the ones we're using so as to help consumers out
| $ref: "#/components/schemas/CodeableConcept" | ||
| description: "Classification of the role of consent, bound to http://terminology.hl7.org/CodeSystem/v3-RoleCode" | ||
|
|
||
| StatusReasonExtension: |
There was a problem hiding this comment.
There was a problem hiding this comment.
example will need updating to reflect use of regulatory basis
| path: /status | ||
| value: inactive | ||
| - op: add | ||
| path: /extension/- |
There was a problem hiding this comment.
is the - how IOPS advised we implement this? just not sure how this works (my ignorance most likely)
| @@ -1,2 +1,2 @@ | |||
| UpdateProvisionEndDate: | |||
| summary: Set provision end date | |||
There was a problem hiding this comment.
Possible gap:
do we have a use case whereby when access of a role is re-enabled we need to be able to support PATCHing the start date as well? or are you handling that internally when a status change of inactive -> active occurs?
| - system: "https://fhir.nhs.uk/R4/CodeSystem/ValidatedRelationships-ErrorOrWarningCode" | ||
| version: "1" | ||
| code: "INVALIDATED_RESOURCE" | ||
| display: "Resource that has been marked as invalid was requested - invalid resources cannot be retrieved" |
There was a problem hiding this comment.
suspect this example will go away since i can't think of a scenario where we should be returning this (as per my other comments)
| - system: "https://fhir.nhs.uk/R4/CodeSystem/ValidatedRelationships-ErrorOrWarningCode" | ||
| version: "1" | ||
| code: "GP_PRACTICE_NOT_FOUND" | ||
| display: "GP Practice could not be found - invalid resources cannot be retrieved" |
There was a problem hiding this comment.
why would we raise this error? think i've probably said bin it off elsewhere in the review. What scenario would we check if a GP practice is found when loading a consent resource?
| display: "Required parameter(s) are missing." | ||
| system: "https://fhir.nhs.uk/R4/CodeSystem/ValidatedRelationships-ErrorOrWarningCode" | ||
| version: "1" | ||
| diagnostics: "Invalid request with error - performer:identifier or patient:identifier parameter not found." |
There was a problem hiding this comment.
uplift of performer:identifier
| search: | ||
| mode: include | ||
| - fullUrl: "https://api.service.nhs.uk/validated-relationships/FHIR/R4/Consent/BBCC67E9" | ||
| resource: |
There was a problem hiding this comment.
this goes for all examples where we have a Consent resource,
- add missing meta.lastUpdated
- update to reflect removal of performer and items from provision and the use of extensions
- policy uri's across the board can now point to the published ISN instead of "REPLACE_WITH_LINK_TO_PUBLISHED_NATIONAL_PROXY_STANDARD", use
https://digital.nhs.uk/data-and-information/information-standards/governance/latest-activity/standards-and-collections/dapb3051-identity-verification-and-authentication-standard-for-digital-health-and-care-services
IMPORTANT
as discussed with Ellie, we've identified a need to return a Patient resource for the proxy (there may not always be a relatedperson record. THIS CHANGE WILL NEED COMMUNICATING TO NHS login to be implemented before we go live with a supplier
There was a problem hiding this comment.
I'd do a wholesale find for performer across the whole repo, cus it appears in all sorts of different guises
| resourceType: Patient | ||
| search: | ||
| mode: include | ||
| - fullUrl: https://sandbox.api.service.nhs.uk/validated-relationships/FHIR/R4/RelatedPerson/BE974742 |
There was a problem hiding this comment.
As discussed with Ellie, for all Patient and RelatedPerson records that we get from PDS, we need to be careful not to return properties that our DPIA doesn't cover us for. I.e. we should trim any properties that aren't in these examples, such as contact info & address
There was a problem hiding this comment.
...as it stands I suspect the implementation doesn't match our examples (i.e. you'll get more properties on the resource back than you expect)
miiisterjim
left a comment
There was a problem hiding this comment.
OK so I think generally my comments boil down to the below. Note that i haven't reviewed the QuestionnaireResponse endpoints schema/examples because that's all going to be reworked in due course when we split the questionnaires for apply/nominate.
- some wording improvements to make the docs clearer
- some fixes i think ought to be done as a priority as they represent bugs, I've noted in my comments where i perceive there to be an impact on NHS Login
- a wholesale review of updates required to replace the use of performer with grantee - for this, I would update the schemas (leaving performer as valid in terms of runtime validation/implementation for backwards compatibility and leave support for the querystring parameter without it being documented on the OAS i.e. still valid technically, but not documented anymore) but everywhere else (examples, documentation, API specs) update to reflect the use of grantee)...leave that to your better judgement on how you wanna manage it though cus hiding things in the docs ain't great, but neither is duplicating performer and grantee documentation (especially where querystring parameter and error codes are concerned
- same wholesale addition and review of all examples of the new properties added from the IOPS changes that aren't in this branch
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |
2581b3b to
3a94ce8
Compare
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |

Pull Request
🧾 Ticket Link
https://nhsd-jira.digital.nhs.uk/browse/NPA-6376
📄 Description/Summary of Changes
🧪 Developer Testing Carried Out
🧪 Reviewer Testing Required
✅ Developer Checklist
NPA-XXXX: <short-description><type>/NPA-XXXX/<short-description>NPA-XXXX: <short-description>terraform,documentation) are added👀 Reviewer Checklist
🚀 Post-merge
After merging and deploying changes to the sandbox, Postman collection or spec examples please run the Run Postman
collection workflow.
This will run the tests within the collection to check that the sandbox is working as expected once deployed.