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.

Sign up for GitHub

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

[16143] Implement Convert Step Updates #16710

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
+317 −52
Open

[16143] Implement Convert Step Updates #16710

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e5d7a5a
[16143] Checking in WIP.
wcutshall Dec 3, 2024
2891d0c
[16143] Checking in WIP.
wcutshall Dec 4, 2024
f3e19c0
Merge branch 'main' into platform/bill/16143
wcutshall Dec 4, 2024
12aaeb2
[16143] Added new code to check for ELR..
wcutshall Dec 4, 2024
48cf73b
[16143] Removed tmp file.
wcutshall Dec 4, 2024
63d3649
[16143] Added additional testing to integration test.
wcutshall Dec 5, 2024
87e3c7a
[16143] Cleanup based on PR review.
wcutshall Dec 6, 2024
466e4d7
[16143] Added code based on PR review.
wcutshall Dec 9, 2024
7224ee4
Merge branch 'main' into platform/bill/16143
wcutshall Dec 10, 2024
270faef
[16143] Cleanup from PR review.
wcutshall Dec 11, 2024
98368f9
Merge branch 'main' into platform/bill/16143
wcutshall Dec 12, 2024
d8a5314
[16143] Modification based on review.
wcutshall Dec 12, 2024
44d4b60
[16143] Modification based on review.
wcutshall Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions prime-router/src/main/kotlin/fhirengine/engine/FHIRConverter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import gov.cdc.prime.router.azure.observability.event.AzureEventServiceImpl
import gov.cdc.prime.router.azure.observability.event.IReportStreamEventService
import gov.cdc.prime.router.azure.observability.event.ReportStreamEventName
import gov.cdc.prime.router.azure.observability.event.ReportStreamEventProperties
import gov.cdc.prime.router.common.BaseEngine
import gov.cdc.prime.router.fhirengine.translation.HL7toFhirTranslator
import gov.cdc.prime.router.fhirengine.translation.hl7.FhirTransformer
import gov.cdc.prime.router.fhirengine.translation.hl7.utils.CustomContext
Expand All @@ -52,6 +53,8 @@ import gov.cdc.prime.router.fhirengine.utils.FhirTranscoder
import gov.cdc.prime.router.fhirengine.utils.HL7Reader
import gov.cdc.prime.router.fhirengine.utils.HL7Reader.Companion.parseHL7Message
import gov.cdc.prime.router.fhirengine.utils.getObservations
import gov.cdc.prime.router.fhirengine.utils.getRSMessageType
import gov.cdc.prime.router.fhirengine.utils.isElr
import gov.cdc.prime.router.logging.LogMeasuredTime
import gov.cdc.prime.router.report.ReportService
import gov.cdc.prime.router.validation.IItemValidator
Expand Down Expand Up @@ -258,7 +261,7 @@ class FHIRConverter(
// TODO: https://github.com/CDCgov/prime-reportstream/issues/14287
FhirPathUtils

val processedItems = process(format, input.blobURL, input.blobDigest, input.topic, actionLogger)
val processedItems = process(format, input, actionLogger)

// processedItems can be empty in three scenarios:
// - the blob had no contents, i.e. an empty file was submitted
Expand Down Expand Up @@ -323,6 +326,10 @@ class FHIRConverter(
// We know from the null check above that this cannot be null
val bundle = processedItem.bundle!!
transformer?.process(bundle)
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some suggestions to make this log message more detailed:

  1. Do this log after you make the child report (line 335) so that you can set childReportId to an actual value
  2. (optional) You can access the processedItem var here, so add the tracking id of the item to this log message: processedItem.getTrackingId().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Changes made.

"Applied transform - parentReportId=[${input.reportId}]" +
", childReportId=[], schemaName=[${input.schemaName}]"
)

// make a 'report'
val report = Report(
Expand Down Expand Up @@ -382,7 +389,8 @@ class FHIRConverter(
mapOf(
ReportStreamEventProperties.BUNDLE_DIGEST
to bundleDigestExtractor.generateDigest(processedItem.bundle!!),
ReportStreamEventProperties.ITEM_FORMAT to format
ReportStreamEventProperties.ITEM_FORMAT to format,
ReportStreamEventProperties.ENRICHMENTS to input.schemaName
)
)
}
Expand Down Expand Up @@ -450,14 +458,12 @@ class FHIRConverter(
*/
internal fun process(
format: MimeFormat,
blobURL: String,
blobDigest: String,
topic: Topic,
input: FHIRConvertInput,
actionLogger: ActionLogger,
routeReportWithInvalidItems: Boolean = true,
): List<IProcessedItem<*>> {
val validator = topic.validator
val rawReport = BlobAccess.downloadBlob(blobURL, blobDigest)
val validator = input.topic.validator
val rawReport = BlobAccess.downloadBlob(input.blobURL, input.blobDigest)
return if (rawReport.isBlank()) {
actionLogger.error(InvalidReportMessage("Provided raw data is empty."))
emptyList()
Expand All @@ -471,7 +477,7 @@ class FHIRConverter(
"format" to format.name
)
) {
getBundlesFromRawHL7(rawReport, validator, topic.hl7ParseConfiguration)
getBundlesFromRawHL7(rawReport, validator, input.topic.hl7ParseConfiguration)
}
} catch (ex: ParseFailureError) {
actionLogger.error(
Expand Down Expand Up @@ -508,24 +514,32 @@ class FHIRConverter(
}
// 'stamp' observations with their condition code
if (item.bundle != null) {
val isElr = if (item.bundle!!.getRSMessageType() == RSMessageType.LAB_RESULT) true else false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be simplified to: item.bundle!!.getRSMessageType() == RSMessageType.LAB_RESULT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup.

item.bundle!!.getObservations().forEach { observation ->
val result = stamper.stampObservation(observation)
if (!result.success) {
val logger = actionLogger.getItemLogger(item.index + 1, observation.id)
if (result.failures.isEmpty()) {
logger.warn(UnmappableConditionMessage())
} else {
logger.warn(
result.failures.map {
UnmappableConditionMessage(
it.failures.map { it.code },
it.source
// Only do this if it is an ELR item.
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment not needed, the code is clear enough. Try to use comments to explain the "why" not the "what". In this case, no comment is needed imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned it up.

if (isElr) {
val result = stamper.stampObservation(observation)
if (!result.success) {
val logger = actionLogger.getItemLogger(item.index + 1, observation.id)
if (result.failures.isEmpty()) {
logger.warn(UnmappableConditionMessage())
} else {
logger.warn(
result.failures.map {
UnmappableConditionMessage(
it.failures.map { it.code },
it.source
)
}
)
}
)
}
}
}
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this exact log message again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This came out of the conversation we had when we were reviewing the code previously. The gist of the conversation was that you thought we should capture every time the message was modified in some way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But we already have a log for when a transform is applied? FHIRConverter line 342

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

"Applied transform - parentReportId=[${input.reportId}]" +
", childReportId=[], schemaName=[${input.schemaName}]"
)
}
item
}
Expand Down
11 changes: 11 additions & 0 deletions prime-router/src/main/kotlin/fhirengine/engine/RSMessageType.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package gov.cdc.prime.router.fhirengine.engine

/**
* This class represents a way to group message types from an RS perspective. As we add additional logical
* groupings, FHIRBundleHelpers.getRSMessageType will need to be updated.
*
*/
enum class RSMessageType {
LAB_RESULT,
UNKNOWN,
}
34 changes: 34 additions & 0 deletions prime-router/src/main/kotlin/fhirengine/utils/FHIRBundleHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import gov.cdc.prime.router.azure.ConditionStamper.Companion.BUNDLE_CODE_IDENTIF
import gov.cdc.prime.router.azure.ConditionStamper.Companion.BUNDLE_VALUE_IDENTIFIER
import gov.cdc.prime.router.azure.ConditionStamper.Companion.conditionCodeExtensionURL
import gov.cdc.prime.router.codes
import gov.cdc.prime.router.fhirengine.engine.RSMessageType
import gov.cdc.prime.router.fhirengine.translation.hl7.utils.CustomContext
import gov.cdc.prime.router.fhirengine.translation.hl7.utils.FhirPathUtils
import gov.cdc.prime.router.fhirengine.utils.FHIRBundleHelpers.Companion.getChildProperties
Expand All @@ -20,6 +21,7 @@ import org.hl7.fhir.r4.model.Coding
import org.hl7.fhir.r4.model.DateTimeType
import org.hl7.fhir.r4.model.DiagnosticReport
import org.hl7.fhir.r4.model.Extension
import org.hl7.fhir.r4.model.MessageHeader
import org.hl7.fhir.r4.model.Observation
import org.hl7.fhir.r4.model.Patient
import org.hl7.fhir.r4.model.Property
Expand Down Expand Up @@ -116,6 +118,38 @@ fun Bundle.addProvenanceReference() {
}
}

/**
* Return true if Bundle contains an ELR in the MessageHeader.
*
* @return true if has a MesssageHeader that contains an R01 or ORU_R01, otherwise false.
*/
fun Bundle.isElr(): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

RS currently receives test orders and results. Instead of isElr, perhaps a method like getMessageType that returns an enum value like LAB_TEST_RESULT or LAB_TEST_ORDER or UNKOWN would be more reusable? Then, I would say let's stamp the message if we get back either LAB_TEST_RESULT or UNKOWN just to be safe?

Implementing detection for LAB_TEST_ORDER is optional at this point as there is no need to distinguish this as of yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some code for this with only LAB_RESULT or UNKNOWN as options. Future features will need to add additional values to the enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we tried moving the logic here into getRSMessageType? I think that would be cleaner, mostly because the code in this method that reads the message header value is going to be reused by test orders as well as other message types probably, right? I don't see the value of an isELR method.

Copy link
Collaborator Author

@wcutshall wcutshall Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My thought on this is that, if we intend to add several potential groupings (i.e., enums in RSMessageType), then getRSMessageType would become quite large. Keeping just meaningful function names related to the message grouping type makes the switch statement clearer in my opinion. Additionally, there is an old programming rule of thumb that goes something like "no subroutine should be longer than one screen in length". This, obviously, is rather vague but speaks to keeping code segments small to increase "understandability". You'll often see this argument backed by references like The Magic Number Seven, Plus or Minus Two.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I just think in practice in this case this won't be longer than a page etc and the downside is you'll be copy pasting this function most likely and just changing the if check in it (violating the DRY principal). That said, there is no repeated code right now, so I'll leave this as a "nit" and we can refactor if needed when we expand if you'd like.

var isElr = false
if (this.type == Bundle.BundleType.MESSAGE && this.entry.isNotEmpty()) {
// By rule, the first entry must be a MessageHeader
arnejduranovic marked this conversation as resolved.
Show resolved Hide resolved
val resource = this.entry[0].resource
if (resource is MessageHeader) {
val event = resource.event
if (event is Coding && ((event.code == "R01") || (event.code == "ORU_R01"))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are there other codes that could be ELR in the future? Throwing those codes into a set to check against would make this a bit more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's unknown at this point. I suspect not right now, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unlikely

isElr = true
}
}
}
return isElr
}

/**
* Return RSMessageType based on grouping logic.
*
* @return RSMessageType of this Bundle.
*/
fun Bundle.getRSMessageType(): RSMessageType {
when {
Copy link
Collaborator

Choose a reason for hiding this comment

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

kotlin tip: when expressions return values so you can omit your returns in each branch and return the whole expression.

return when {
    isElr() -> RSMessageType.LAB_RESULT
    else -> RSMessageType.UNKNOWN
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Gradle build fails when your suggested change is made.

> Task :prime-router:compileKotlin FAILED
e: file:///Users/bill/projects/report-stream/prime-reportstream/prime-router/src/main/kotlin/fhirengine/utils/FHIRBundleHelpers.kt:151:1 A 'return' expression required in a function with a block body ('{...}')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops my mistake, apologies. Made the change as requested.

isElr() -> return RSMessageType.LAB_RESULT
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you update this method per my suggestion above, you can just do this instead:

event is Coding && ((event.code == "R01") || (event.code == "ORU_R01")) -> return RSMessageType.LAB_RESULT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my comment about your suggestion above.

else -> return RSMessageType.UNKNOWN
}
}

/**
* Gets all properties for a [Base] resource recursively and filters only its references
*
Expand Down
Loading
Loading