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

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

16394 add ack functionality #16552

wants to merge 9 commits into from

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 254 tests  +7   1 250 ✅ +7   7m 35s ⏱️ -4s
  164 suites +2       4 💤 ±0 
  164 files   +2       0 ❌ ±0 

Results for commit dde2274. ± Comparison against base commit 9258b5d.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 15, 2024

Integration Test Results

 54 files  ±0   54 suites  ±0   28m 57s ⏱️ + 1m 4s
414 tests ±0  404 ✅ ±0  10 💤 ±0  0 ❌ ±0 
417 runs  ±0  407 ✅ ±0  10 💤 ±0  0 ❌ ±0 

Results for commit dde2274. ± Comparison against base commit 9258b5d.

♻️ 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.

Copy link

sonarcloud bot commented Nov 21, 2024

@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

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