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

Use MSH-11 to identify test messages #1389

Merged
merged 17 commits into from
Oct 11, 2024
Merged

Conversation

basiliskus
Copy link
Contributor

@basiliskus basiliskus commented Oct 7, 2024

Use MSH-11 to identify test messages

  • Updated MSH-11 to D for all sample files in examples/ (except files in examples/Test/Automated/)
  • Updated MSH-11 to N for all sample files in examples/Test/Automated/
  • Removed workaround in scripts/hurl/rs/hrl to avoid unintentionally routing test messages to partners
  • Added ADR and also documented in examples/README.md

Issue

#1387

@basiliskus basiliskus marked this pull request as ready for review October 10, 2024 17:03
@pluckyswan
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Oct 10, 2024

PR Reviewer Guide 🔍

(Review updated until commit 30a73e4)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Data Consistency
Ensure that the changes in the MSH segment and the PID segment are consistent with the HL7 standards and the intended use case. The addition of 'D' in the MSH segment for message processing should be validated to ensure it aligns with the correct processing needs.

Script Modification
The removal of the 'allow_outbound' and 'msh_header_replacement' options in the script could affect the behavior of message handling. It's crucial to ensure that these changes do not unintentionally disrupt existing functionalities or security measures.

@@ -1,4 +1,4 @@
MSH|^~\&|SISGDSP|SISGDSP|SISHIERECEIVER^11903029^L,M,N|^^L,M,N|20240212103049||ORU^R01^ORU_R01|243408787|T|2.5.1
MSH|^~\&|SISGDSP|SISGDSP|SISHIERECEIVER^11903029^L,M,N|^^L,M,N|20240212103049||ORU^R01^ORU_R01|243408787|D|2.5.1
Copy link
Contributor

@somesylvie somesylvie Oct 11, 2024

Choose a reason for hiding this comment

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

Does this change need to be made in the final messages too? Like 003_CA_ORU_R01_CDPH_produced_3_hl7_translation_final.hl7 still has T in msh-11, and I assume the other final messages are like that

Edit: Looks like all the CA messages have a final version, plus MN 0004

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Not in my opinion. Files that have the 1_hl7_translation, 2_fhir_transformation, 3_hl7_translation_final suffix are snapshots of the corresponding 0_initial_message file that went through each step of the RS flow, at that specific moment. Of those files, the only one that should be sent to RS and routed is 0_initial_message. We need to send it through the flow in order to update the other ones. Which makes me think we may want a different way of storing these files so they are not constantly out of date. We could think using the new Automated Staging Test - Submit Messages workflow for this purpose

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be okay to have the final ones be like...a snapshot in time, as long as it's noted that they are. But I think if we don't update that value, they're not actually the right output for those inputs? Since I don't think MSH-11 changes through the workflow. It might be nice to either note when they were last generated, or have a way to regenerate them automatically. The nice part of having the files more static is there's fewer surprises, but having them more up to date makes them more valid for tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think regenerating them automatically is a good idea. It should be straightforward to create a bash script to do that using hurl. Though we'd need to manually get and overwrite the FHIR files from the local azure storage to update those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a new PR to add a script that will automate those updates as much as possible

examples/README.md Outdated Show resolved Hide resolved
Copy link
Member

@halprin halprin left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I am curious about the other HL7 files that weren't updated (e.g. the final ones) that @somesylvie mentioned.

examples/README.md Show resolved Hide resolved
@@ -1,4 +1,4 @@
MSH|^~\&|BaptistOracle^2.16.840.1.114222.4.1.000000^ISO|BaptistEast^2.16.840.1.114222.4.1.000001^ISO|ALlabNatus^2.16.840.1.114222.4.1.181960.2^ISO|ALlab^simulated-lab-id^ISO|20240224134009||ORM^O01^ORM_O01|Q1960841872T2476960690||2.5.1||||||8859/1
MSH|^~\&|BaptistOracle^2.16.840.1.114222.4.1.000000^ISO|BaptistEast^2.16.840.1.114222.4.1.000001^ISO|ALlabNatus^2.16.840.1.114222.4.1.181960.2^ISO|ALlab^simulated-lab-id^ISO|20240224134009||ORM^O01^ORM_O01|Q1960841872T2476960690|D|2.5.1||||||8859/1
Copy link
Contributor

Choose a reason for hiding this comment

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

should the corresponding final message for these also be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other response

@basiliskus
Copy link
Contributor Author

This looks good to me, but I am curious about the other HL7 files that weren't updated (e.g. the final ones) that @somesylvie mentioned.

You can check my reply to @somesylvie with my reasoning

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 30a73e4


### Positive

- We will be able to identify and route test messages in ReportStream without relying on fields that are overwritten by transformations; and as a result, avoid sending test messages to partners by mistake
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth mentioning the updated routing in RS as part of this ADR? Has the routing there already been updated, and will it be the same in staging and prod or different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good point. I'll add some clarification on the differences between staging and prod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more context

examples/README.md Outdated Show resolved Hide resolved
Copy link

@basiliskus basiliskus merged commit e9ccc8f into main Oct 11, 2024
16 checks passed
@basiliskus basiliskus deleted the story/1387/update-msh-11 branch October 11, 2024 21:01
tjohnson7021 pushed a commit that referenced this pull request Oct 17, 2024
* Updated test files msh-11 to be either T or N

* Updated test files msh-11 to be D instead of T

* Updated MSH-11 from D to T for CA sample OML

* Removed hurl script MSH overwrite that is not needed now that we're using MSH-11 for test message routing

* Added docs in the readme

* Updated readme and added ADR skeleton

* Updated MSH-11 to D for files in examples folder

* Added ADR

* Added to the readme

* Moved readme section up to make it more visible

* Trying to fix formatting

* Minor phrasing updates

* Added some more context to ADR

* Fixed path and added and added link to ADR

* Added additional context in the ADR on the differences between prod and non-prod environments

* Fixed path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants