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

Story/1650/la weight conversion #1708

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

Conversation

luis-pabon-tf
Copy link
Contributor

Description

Describe what changed in this PR at a high level.

Issue

Add a link to the issue here. Consider using
closing keywords
if the this PR isn't for a story (stories will be closed through different means).

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

Copy link

PR Reviewer Guide 🔍

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

Placeholder Code

The transformation definition contains placeholder comments that need to be replaced with actual code.

  "MSH-33 is 13^1.2.840.114350.1.13.286.2.7.2.695071^ISO //@todo this is just the description, replace with actual code"
],
"rules": [
  {
    "name": "MultiplyObservationValue",
    "args": {
      "multiplier": "1000",
      "unitText": "g^gram^UCUM //@todo this is going to OBX-6 think how to send these values"

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add error handling to prevent ClassCastException in the transformation method

Ensure that the transform method includes error handling for potential
ClassCastException when casting resource.getUnderlyingData() to Bundle. This is
crucial to prevent runtime exceptions if the underlying data is not of type Bundle.

etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/MultiplyObservationValue.java [12]

-var bundle = (Bundle) resource.getUnderlyingData();
+Object data = resource.getUnderlyingData();
+if (data instanceof Bundle) {
+    var bundle = (Bundle) data;
+} else {
+    // appropriate error handling
+}
Suggestion importance[1-10]: 8

Why: Adding error handling for ClassCastException is crucial to ensure robustness and prevent runtime errors when the data type is not as expected, which enhances the reliability of the code.

8
General
Replace placeholder texts with actual code in transformation definitions

Replace the placeholder text in the conditions and unitText fields with actual
implementation code to ensure the transformation definitions are correctly
configured and functional.

etor/src/main/resources/transformation_definitions.json [330-337]

 "conditions": [
-  "MSH-33 is 13^1.2.840.114350.1.13.286.2.7.2.695071^ISO //@todo this is just the description, replace with actual code"
+  "actual condition code here"
 ],
-"unitText": "g^gram^UCUM //@todo this is going to OBX-6 think how to send these values"
+"unitText": "actual unit text here"
Suggestion importance[1-10]: 7

Why: Replacing placeholder texts with actual implementation details is necessary to ensure the transformation definitions are correctly configured and functional, which is essential for the correct operation of the system.

7


@Override
public void transform(HealthData<?> resource, Map<String, Object> args) {
var bundle = (Bundle) resource.getUnderlyingData();

Check notice

Code scanning / CodeQL

Unread local variable Note

Variable 'Bundle bundle' is never read.
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.

3 participants