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 |
| | 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
| 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
There was a problem hiding this comment.
@ellie-bound1-NHSD @miiisterjim These are currently valid statuses along with "inception" according to what's in the repo. Do we need a ticket to update these?
| 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
| 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
There was a problem hiding this comment.
@ellie-bound1-NHSD could we review these together please?
| $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.
Might be worth trying to improve the docs here.....the way this gets rendered in the schema doesn't really help implementors with what to expect in terms of what will relate to a codified status reason and what will be free text entered by a user to support the selected code.
specification/examples/requests/PATCH_Consent/replace_legal_basis.yaml
Outdated
Show resolved
Hide resolved
| 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
There was a problem hiding this comment.
@ClarksonAdam add missing meta.lastUpdated to spec
@ellie-bound1-NHSD to raise ticket for the link change
There was a problem hiding this comment.
Link updated in OAS examples
| 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
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 |
ClarksonAdam
requested review from
ClarksonAdam
and removed request for
ClarksonAdam
ClarksonAdam
force-pushed
the
task/NPA-6376/review-spec
branch
from
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 |
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |
|
This branch is work on a ticket in the NHS Digital NPA JIRA Project. Here's a handy link to the ticket: NPA-6376 |
3 participants
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.