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

16394 add ack functionality #16552

Merged
merged 22 commits into from
Dec 4, 2024
Merged

16394 add ack functionality #16552

merged 22 commits into from
Dec 4, 2024

Conversation

jalbinson
Copy link
Collaborator

@jalbinson jalbinson commented Nov 15, 2024

This PR adds ACK functionality to the waters and report submission endpoints.

Test Steps:

  1. Run ReportStream
  2. Submit the following HL7. Ensure the content type header has been specified as "application/hl7-v2"
MSH|^~\&|Epic|Hospital|LIMS|StatePHL|20241003000000||ORM^O01^ORM_O01|4AFA57FE-D41D-4631-9500-286AAAF797E4|T|2.5.1|||AL|NE 
  1. The response body should be HL7

Changes

  • New ACK functionality
  • Ability to pick apart more MSH header segments

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • Added tests?

Linked Issues

@jalbinson jalbinson requested a review from a team as a code owner November 15, 2024 18:54
Copy link

github-actions bot commented Nov 15, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

val ackMsh = outgoingAck.msh
ackMsh.msh1_FieldSeparator.value = "|"
ackMsh.msh2_EncodingCharacters.value = "^~\\&"
ackMsh.msh3_SendingApplication.parse("ReportStream")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took these values directly from the Spec document so just double check that these are exactly what we want.

Copy link

github-actions bot commented Nov 15, 2024

Test Results

1 255 tests  +7   1 251 ✅ +7   7m 46s ⏱️ +18s
  164 suites +2       4 💤 ±0 
  164 files   +2       0 ❌ ±0 

Results for commit 5ea93e2. ± Comparison against base commit 62207ec.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 15, 2024

Integration Test Results

 57 files  ±0   57 suites  ±0   38m 22s ⏱️ - 1m 15s
421 tests ±0  411 ✅ ±0  10 💤 ±0  0 ❌ ±0 
424 runs  ±0  414 ✅ ±0  10 💤 ±0  0 ❌ ±0 

Results for commit 5ea93e2. ± Comparison against base commit 62207ec.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@JFisk42 JFisk42 left a comment

Choose a reason for hiding this comment

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

Looks good and works well. I tested a few different scenarios: different message types (ORU), missing fields that should populate certain values in the response (MSH.3/MSH.4), and setting MSH.15 to AL when a fully populated message is sent. In each scenario the ACK response was correctly populated.

prime-router/src/main/kotlin/azure/ReportFunction.kt Outdated Show resolved Hide resolved
prime-router/src/test/kotlin/azure/ReportFunctionTests.kt Outdated Show resolved Hide resolved
prime-router/src/main/kotlin/fhirengine/utils/HL7Reader.kt Outdated Show resolved Hide resolved
@jalbinson
Copy link
Collaborator Author

@arnejduranovic
Just looked this up. Are we want to handle the SU value as well?

Screenshot 2024-11-22 at 10 29 23 AM

Copy link
Collaborator

@JFisk42 JFisk42 left a comment

Choose a reason for hiding this comment

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

The new changes look good and fit with what I recall from what Arnej requested be changed. 👍

every { mockActionHistory.action } returns mockAction
every { mockAction.actionId } returns 5
every { mockEngine.db } returns mockDb
// I don't agree with ktlint on this one
Copy link
Collaborator

Choose a reason for hiding this comment

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

LOL I've seen some odd looking ktlint rules before. This takes the cake.

@arnejduranovic
Copy link
Collaborator

I tested locally with the test data in the PR description and got this response when I set the ignore.ignore-etor-ti sender's hl7AcknowledgementEnabled to true:

MSH|^~\&|ReportStream|CDC|Epic|Hospital|20241202194106.427+0000||ACK|372acc8a-8b21-4d6f-a78e-4f13dbabc920|T|2.5.1|||NE|NE
MSA|CA|4AFA57FE-D41D-4631-9500-286AAAF797E4

@brick-green FYI

Copy link

sonarcloud bot commented Dec 4, 2024

@jalbinson jalbinson merged commit 6c6b77b into main Dec 4, 2024
25 checks passed
@jalbinson jalbinson deleted the platform/jamie/16394-ack branch December 4, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
etor platform Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Basic Synchronous Accept ACK in Submission API per LRI/LOI spec to meet immediate Flexion ETOR needs
4 participants