Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIAD-3217 - Send Observation (Test Result) when they do not have a Specimen attached to them #963

Open
wants to merge 91 commits into
base: main
Choose a base branch
from

Conversation

martin-nhs
Copy link
Contributor

What

Please include a summary of the changes and the related issue

Why

Please include details of the reasoning for these changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Internal change (non-breaking change with no effect on the functionality affecting end users)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the Changelog with details of my change in the UNRELEASED section if this change will affect end users

@martin-nhs martin-nhs marked this pull request as draft October 23, 2024 15:12
Comment on lines 246 to 268
"resource": {
"resourceType": "Specimen",
"id": "NON-SIGNIFICANT-SPECIMEN",
"meta": {
"profile": [
"https://fhir.nhs.uk/STU3/StructureDefinition/CareConnect-GPC-Specimen-1"
]
},
"identifier": [
{
"system": "https://wiremock/",
"value": "NON-SIGNIFICANT-SPECIMEN"
}
],
"subject": {
"reference": "Patient/DAED5527-1985-45D9-993E-C5FF51F36828"
},
"receivedTime": "2001-02-03T10:00:00+00:00",
"collection": {
"collectedDateTime": "2001-02-03T11:00:00+00:00"
}
}
},
Copy link
Contributor Author

@martin-nhs martin-nhs Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies to the reviewer - I have formatted the bundle which has introduced a bunch of formatting changes which made the git changes difficult to find, just wanted to highlight this was added.

Comment on lines 754 to 806
{
"resource": {
"resourceType": "Observation",
"id": "TEST-RESULT-NO-SPECIMEN",
"meta": {
"profile": [
"https://fhir.nhs.uk/STU3/StructureDefinition/CareConnect-GPC-Observation-1"
]
},
"identifier": [
{
"system": "https://wiremock/",
"value": "TEST-RESULT-NO-SPECIMEN"
}
],
"status": "final",
"code": {
"coding": [
{
"system": "http://read.info/readv2",
"code": "42R5.00",
"display": "Serum TIBC",
"userSelected": true
},
{
"extension": [
{
"url": "https://fhir.nhs.uk/STU3/StructureDefinition/Extension-coding-sctdescid",
"extension": [
{
"url": "descriptionId",
"valueId": "2580111000000118"
}
]
}
],
"system": "http://snomed.info/sct",
"code": "1015451000000101",
"display": "Serum total iron binding capacity"
}
]
},
"subject": {
"reference": "Patient/DAED5527-1985-45D9-993E-C5FF51F36828"
},
"effectiveDateTime": "2001-02-03T12:00:00+00:00",
"issued": "2001-02-03T13:00:00+00:00",
"valueQuantity": {
"value": 42.000,
"unit": "umol/L"
}
}
},
Copy link
Contributor Author

@martin-nhs martin-nhs Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies to the reviewer - I have formatted the bundle which has introduced a bunch of formatting changes which made the git changes difficult to find, just wanted to highlight this was added.

Everything else remains the same.

martin-nhs and others added 3 commits October 25, 2024 14:54
Scenario 1:
Files: diagnostic-report-with-one-specimen-and-one-unrelated-observation

Diagnostic Report linked to Specimen
Observation linked to Diagnostic Report
Specimen linked to Diagnostic report
(Observation is not linked to Specimen)

----

Scenario 2:
Files: diagnostic-report-with-one-specimen-one-linked-observation-and-one-unlinked-observation

Diagnostic Report linked to Specimen
Observation linked to Diagnostic Report
Specimen linked to both Diagnostic Report and Observation
Observation linked to only Diagnostic Report
(This creates an Observation unlinked to a Specimen)
<id root="test-id-3" />
<code code="109341000000100" displayName="GP to GP communication transaction" codeSystem="2.16.840.1.113883.2.1.3.2.4.15"/>
<statusCode code="COMPLETE" />
<component typeCode="COMP" contextConductionInd="true">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks sus. Looking at the JSON, the DiagnosticReport has 1 Specimen, and zero test results.

We'd expect that the Mapper would generate 1 Specimen, and no Dummy, but it appears to be generating both.

Copy link

@stevenmccullaghmadetech stevenmccullaghmadetech Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the XML, we can see two instances of <specimen typeCode="SPC">, where the JSON suggests we should only have one, which agrees with what you've said.
However, that specific diagnostic report has one specimen and zero observations. A diagnostic report must have an observation. Therefore, a dummy observation is made, and linked to a dummy specimen. This accounts for the two specimens.

Copy link
Member

@adrianclay adrianclay Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hypothesis as to what's going on:

  1. When it grabs Specimens, it knows that there is one already.
  2. When it grabs the Observations, it identifies there isn't one, so creates one.
  3. Because this dummy Observation that has been created doesn't have a Specimen it also generates a Dummy specimen to go along with it.

Hypothesis as to what was happening previously before our change:

  1. When it grabs Specimens, it knows that there is one already.
  2. When it grabs the Observations, it identifies there isn't one, so creates one with a NULL specimen.
  3. When the DiagnosticReportMapper iterates over each Specimen, the Dummy Observation never gets output as it doesn't belong to any Specimen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So while I don't think this change in behaviour is desirable, it isn't bad enough to be a blocker. We can come back and remove the superfluous Specimen at a later date.


List<Specimen> specimens = new ArrayList<>();

// At least one specimen is required to exist for any DiagnosticReport, according to the mim
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where in the mim did you see that?

private boolean hasObservationsWithoutSpecimen(List<Observation> observations) {
return observations
.stream()
.filter(observation -> !isFilingComment(observation))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this line is super well tested.

Copy link

  • Surviving mutants in this change: 1
  • Killed mutants in this change: 19
class surviving killed
🧟uk.nhs.adaptors.gp2gp.ehr.mapper.diagnosticreport.DiagnosticReportMapper 1 19

See https://pitest.org

Copy link

sonarcloud bot commented Nov 14, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants