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 6 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
79 changes: 61 additions & 18 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,7 @@ 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.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 +260,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 @@ -450,14 +452,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 +471,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,21 +508,64 @@ class FHIRConverter(
}
// 'stamp' observations with their condition code
if (item.bundle != null) {
val isElr = item.bundle!!.isElr()
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
)
}
)
}
} else {
// Generate an ITEM_TRANSFORMED Azure Event.
val bundleDigestExtractor = BundleDigestExtractor(
FhirPathBundleDigestLabResultExtractorStrategy(
CustomContext(
item.bundle!!,
item.bundle!!,
mutableMapOf(),
CustomFhirPathFunctions()
)
)
)
val report = Report(
MimeFormat.FHIR,
emptyList(),
parentItemLineageData = listOf(
Report.ParentItemLineageData(input.reportId, item.index.toInt() + 1)
),
metadata = this.metadata,
topic = input.topic,
nextAction = TaskAction.none
)
reportEventService.sendItemEvent(
eventName = ReportStreamEventName.ITEM_TRANSFORMED,
childReport = report,
pipelineStepName = TaskAction.convert
) {
parentReportId(input.reportId)
params(
mapOf(
ReportStreamEventProperties.BUNDLE_DIGEST
to bundleDigestExtractor.generateDigest(item.bundle!!),
ReportStreamEventProperties.ORIGINAL_FORMAT to format.name,
ReportStreamEventProperties.TARGET_FORMAT to MimeFormat.FHIR.name,
ReportStreamEventProperties.ENRICHMENTS to listOf(input.schemaName)
)
)
trackingId(item.bundle!!)
}
}
}
}
Expand Down
21 changes: 21 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 @@ -20,6 +20,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 +117,26 @@ 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
}

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