Add test date to Observation.issued for single-entry and bulk upload #5982

mehansen · 2023-06-13T23:37:27Z

BACKEND PULL REQUEST

Related Issue

CA wants to see test result date (OBX-19) in our flu messages. we already send them in our Covid messages. RS uses what's in Observation.issued for this when transforming to hl7.

Changes Proposed

add test result date in new Observation.issued entry for both single-entry and bulk upload
- for single entry it's the test_date we send in the Covid message and for bulk upload it's the test_result_date field
also explicitly set time zones for Observation.issued and DiagnosticReport.effective to make fhir converter behavior more predictable and easier to test
also fix rounding of test datetime to start of day in bulk upload to only happen if no time is provided
using system default time zone for now but should look into what time zone to infer for dates in bulk upload

Additional Information

breaking this up from other requested changes by CA just to make things hopefully easier to review
talked to Mike some about how to infer time zones for dates submitted in the bulk uploader (see comments on Mike/5844 tests bulk upload results fhir #5946). For now I'm sticking to system default time zone since that's what we use for our messages to the Covid pipeline.
creating a followup ticket to fix code smells I introduced in FhirConverter because it got a little involved and we are hoping to have these changes in by Friday

Testing

Check updated tests make sense and are passing
Will deploy to a lower so we can test that messages have the new field set

* also force time zones to make fhir converter easier to test * also fix rounding of test date to start of day * using system default time zone for now but should look into what time zone to infer for dates in bulk upload

backend/src/main/java/gov/cdc/usds/simplereport/api/converter/FhirConverter.java

mehansen · 2023-06-14T19:35:35Z

backend/src/main/java/gov/cdc/usds/simplereport/api/converter/FhirConverter.java

-            .setEffective(new DateTimeType(dateTested))
+            .setEffective(
+                new DateTimeType(
+                    dateTested, TemporalPrecisionEnum.MILLI, TimeZone.getTimeZone("utc")))


see above comment - this just forces the messages to look the same when run on systems with different default precision/time zone

mehansen · 2023-06-14T19:38:38Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/BulkUploadResultsToFhir.java

@@ -309,6 +321,9 @@ private Bundle convertRowToFhirBundle(TestResultRow row, UUID orgId) {
                    .testkitNameId(testKitNameId)
                    .equipmentUid(equipmentUid)
                    .deviceModel(row.getEquipmentModelName().getValue())
+                    .issued(
+                        Date.from(
+                            testResultDate.atZone(zoneIdGenerator.getSystemZoneId()).toInstant()))


this on the other hand uses the system default time zone to infer the time zone for text date time in the CSV (e.g. 06/14/23 12:36, which has no time zone)
we have to do this any time there's a date time in the CSV since the format doesn't specify time zone. we should figure out a better way to determine what time zone to use

mehansen · 2023-06-14T19:40:02Z

backend/src/test/resources/testResultUpload/test-results-upload-valid-as-fhir.json

@@ -116,7 +116,7 @@
        "subject": {
          "reference": "Patient/1234"
        },
-        "effectiveDateTime": "2021-12-20T00:00:00Z",
+        "effectiveDateTime": "2021-12-20T14:00:00.000Z",


reflects the date in the CSV since we don't round to start of day in bulk upload code anymore

mpbrown · 2023-06-14T21:43:55Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/BulkUploadResultsToFhir.java

+    } else {
+      testResultDate = ((LocalDate) temporalAccessor).atStartOfDay();
+    }
+


This is an awesome way to handle the date parsing! 💯💯💯 I'll update #5985 with this.

Do you think we should move this parsing towards the top of convertRowToFhirBundle to group it near testEventId? That value would now be used by at least observation and diagnosticReport (I think we might soon be using it for specimen.collected too)

* also missed a fix for a test

mpbrown · 2023-06-15T15:36:51Z

So I've been reading a bit more about using Z vs +/-hh:mm and it looks like there is some distinction between Z and +00:00. (also see this W3C note). This HL7 site brings up an interesting point with time zones, even if it isn't explicitly regarding FHIR.

For many applications the time of interest is the local time of the sender. For example, an application in the Eastern Standard Time zone receiving notification of an admission that takes place at 11:00 PM in San Francisco on December 11 would prefer to treat the admission as having occurred on December 11 rather than advancing the date to December 12.

What do you think of this suggestion? 🤔

If the time is system generated like timestamp when created or last updated and not connected to any specific time zone, we store the time as UTC by using Z like DateTimeType[2021-12-20T19:00:00Z]

If the time is related to something in a local time zone or extract from the uploaded data that uses a local time like test_result_date, we store the time with the offset suffix like DateTimeType[2021-12-20T14:00:00-05:00]

BobanL

Changes look good to me!

mpbrown

Everything looks good! I'll update my time zone branch with these changes too

sonarcloud · 2023-06-16T16:24:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

89.5% Coverage
0.0% Duplication

mehansen · 2023-06-16T19:54:42Z

@mpbrown

That's interesting! Tbh I'm a little puzzled by that HL7 site excerpt. Does this imply that recipient applications are potentially just throwing away the time zone when comparing or filtering by dates?

I don't feel a strong preference for how we format our dates in the message as long as we can expect them to be consistent across different systems imo. I guess I have a small preference to just have them all be in UTC so we don't have additional logic around translating them to a certain TZ depending on facility if it's not actually necessary.. 🤔 Maybe this is something we can ask RS in our next sync if there's a preferred time zone to use. I'll also ask CA how they handle time zones in their messages.

edit: hmm to me this part "All HL7 systems are required to accept the time zone offset, but its implementation is application specific." implies that it's up the system accepting the message to decide what to do with the time zone, so they could potentially have some data transform that changes the date times to be in the local time of the sender.

mehansen · 2023-06-16T20:56:50Z

update: RS converts the dates in messages we send into whatever time zone of the recipient, so doesn't seem like it matters much what tz we send it in

@mpbrown

mehansen added 5 commits June 13, 2023 16:31

Handle input dates with no time (oops)

2d1840a

clean up todo

490466c

Merge branch 'main' into merethe/move-fhir-message-dates

fbda45f

update test to reflect not rounding to start of day

50b80da

mehansen marked this pull request as ready for review June 14, 2023 19:26

mehansen requested review from BobanL, johanna-skylight, zdeveloper, DanielSass, mpbrown and emyl3 June 14, 2023 19:26

mehansen commented Jun 14, 2023

View reviewed changes

backend/src/main/java/gov/cdc/usds/simplereport/api/converter/FhirConverter.java Outdated Show resolved Hide resolved

mehansen commented Jun 14, 2023

View reviewed changes

mpbrown mentioned this pull request Jun 14, 2023

Handle timezones in bulk uploader and Fhir converter #5985

Merged

mpbrown reviewed Jun 14, 2023

View reviewed changes

mehansen added 3 commits June 14, 2023 15:28

use z format for datetimes instead of +0:00

ba322f7

code review feedback

54fe8bf

* also missed a fix for a test

Merge branch 'main' into merethe/move-fhir-message-dates

e1d5396

Merge branch 'main' into merethe/move-fhir-message-dates

ed58e16

BobanL approved these changes Jun 15, 2023

View reviewed changes

mpbrown approved these changes Jun 15, 2023

View reviewed changes

Merge branch 'main' into merethe/move-fhir-message-dates

45f49ad

mehansen merged commit e4df864 into main Jun 16, 2023

mehansen deleted the merethe/move-fhir-message-dates branch June 16, 2023 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test date to Observation.issued for single-entry and bulk upload #5982

Add test date to Observation.issued for single-entry and bulk upload #5982

mehansen commented Jun 13, 2023 •

edited

Loading

mehansen Jun 14, 2023

mehansen Jun 14, 2023

mehansen Jun 14, 2023

mpbrown Jun 14, 2023 •

edited

Loading

mpbrown commented Jun 15, 2023 •

edited

Loading

BobanL left a comment

mpbrown left a comment

sonarcloud bot commented Jun 16, 2023

mehansen commented Jun 16, 2023 •

edited

Loading

mehansen commented Jun 16, 2023

Add test date to Observation.issued for single-entry and bulk upload #5982

Add test date to Observation.issued for single-entry and bulk upload #5982

Conversation

mehansen commented Jun 13, 2023 • edited Loading

BACKEND PULL REQUEST

Related Issue

Changes Proposed

Additional Information

Testing

mehansen Jun 14, 2023

Choose a reason for hiding this comment

mehansen Jun 14, 2023

Choose a reason for hiding this comment

mehansen Jun 14, 2023

Choose a reason for hiding this comment

mpbrown Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

mpbrown commented Jun 15, 2023 • edited Loading

BobanL left a comment

Choose a reason for hiding this comment

mpbrown left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 16, 2023

mehansen commented Jun 16, 2023 • edited Loading

mehansen commented Jun 16, 2023

mehansen commented Jun 13, 2023 •

edited

Loading

mpbrown Jun 14, 2023 •

edited

Loading

mpbrown commented Jun 15, 2023 •

edited

Loading

mehansen commented Jun 16, 2023 •

edited

Loading