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

Add 99717-49 Local Code to 76030-6 LOINC mapping #1453

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

Conversation

jbiskie
Copy link
Contributor

@jbiskie jbiskie commented Oct 17, 2024

Adds 99717-49 Local Code to 76030-6 LOINC mapping

This PR:

  • Adds a new local to LOINC mapping
  • Refactors the hardcoded mapping in the transform class into a configurable arguments in transformation_definitions.json
  • Adds additional unit tests to cover cases where the codingMap json argument is not structured as expected

Issue

#1452

This PR covers engineering tasks 1 and 2. Task 3 (Add RS E2E tests for the mapping transformation) will be addressed in another PR.

Checklist

  • I have added tests to cover my changes
  • I have added logging where useful (with appropriate log level)
  • I have added JavaDocs where required
  • I have updated the documentation accordingly

Note: You may remove items that are not applicable

@jbiskie jbiskie self-assigned this Oct 17, 2024
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Prevent potential overwriting of existing keys in the map

Ensure that the key "99717-49" does not already exist in the codingMap to prevent
overwriting an existing mapping. This can be done by checking if the key exists
before putting a new value.

etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/MapLocalObservationCodes.java [155-160]

-codingMap.put(
-    "99717-49",
-    new IdentifierCode(
-        "76030-6",
-        "IDS gene full mutation analysis in Blood or Tissue by Sequencing",
-        HapiHelper.LOINC_CODE));
+if (!codingMap.containsKey("99717-49")) {
+    codingMap.put(
+        "99717-49",
+        new IdentifierCode(
+            "76030-6",
+            "IDS gene full mutation analysis in Blood or Tissue by Sequencing",
+            HapiHelper.LOINC_CODE));
+}
Suggestion importance[1-10]: 7

Why: The suggestion to check if a key already exists before adding it to the map is a good practice to avoid data integrity issues. This is particularly important in a map that might be accessed or modified in various parts of the application, ensuring that no existing data is unintentionally overwritten.

7

@jbiskie
Copy link
Contributor Author

jbiskie commented Oct 18, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:
Category Suggestion Score
Possible issue
Prevent potential overwriting of existing keys in the map

7

Non-issue. IntelliJ flags duplicates.

@halprin
Copy link
Member

halprin commented Oct 18, 2024

We should also update our RS E2E tests to test this.

@jbiskie jbiskie marked this pull request as ready for review October 23, 2024 15:35
* refactor:
use stream instead of for loop
use IllegalArgumentException instead of NullPointerException
make tests pass
add helper method isLocalCode
add helper method processCoding
add helper method validateField

* refactoring + all tests pass
@jbiskie
Copy link
Contributor Author

jbiskie commented Oct 23, 2024

Manual RS end to end testing was successful. Used file 020 and added an extra OBX for 99717-49:

OBX|12|TX|^^^99717-49^New local code added^L|1|Negative|||N|||F|||20240711034913

transformed:

OBX|113|TX|76030-6^IDS gene full mutation analysis in Blood or Tissue by Sequencing^LN^99717-49^New local code added^L|1|Negative|||N|||F|||20240711034913

Copy link
Contributor

@basiliskus basiliskus left a comment

Choose a reason for hiding this comment

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

Seems like there are some extracted functions that have a single line of code and are used only once. Are they needed? There's a tradeoff when refactoring to extract code to functions and adding too many can create unnecessary overhead. If they are added to help understand what the code does, you could consider adding a comment instead

private void logUnmappedLocalCode(Bundle bundle, Coding coding) {
var msh41Identifier = HapiHelper.getMSH4_1Identifier(bundle);
var msh41Value = msh41Identifier != null ? msh41Identifier.getValue() : null;
private void logUnmappedLocalCode(Coding coding, String msh41Identifier, String messageId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is only used once, it has a single line and the content is pretty straightforward. Does it need to extracted into its own function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good suggestion. I'll inline the extracting method and keep the logging one as is to keep the logging logic in one area.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that we probably don't need logUnmappedLocalCode as it's adding more abstraction without the benefits. You could directly call logger.logWarning without putting it into its own function

Copy link

sonarcloud bot commented Oct 24, 2024

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.

5 participants