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

Adding test member oid as the extension to all observations in the el… #16397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kant777
Copy link
Collaborator

@kant777 kant777 commented Oct 30, 2024

…r messages

This PR adds the member OID as the extension to all observations in the elr messages.

Test Steps:

  1. Include steps to test these changes

image

Changes

  • The ConditionStamper will now handle both condition lookups and Member OID stamping based on the logic and constants from ObservationMappingConstants

Checklist

Testing

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

Linked Issues

@kant777 kant777 requested a review from a team as a code owner October 30, 2024 03:04
Copy link

github-actions bot commented Oct 30, 2024

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

Copy link

github-actions bot commented Oct 30, 2024

Test Results

1 249 tests  +2   1 243 ✅ ±0   7m 5s ⏱️ -45s
  162 suites ±0       4 💤 ±0 
  162 files   ±0       2 ❌ +2 

For more details on these failures, see this check.

Results for commit 5536e4a. ± Comparison against base commit abfc0c0.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Oct 30, 2024

Integration Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 5536e4a.

♻️ This comment has been updated with latest results.

@kant777 kant777 changed the title Adding test member oid as the extension to all observations in the el… [DRAFT] Adding test member oid as the extension to all observations in the el… Oct 30, 2024
@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from c4d718e to 884f3fe Compare November 7, 2024 13:14
@kant777 kant777 changed the title [DRAFT] Adding test member oid as the extension to all observations in the el… Adding test member oid as the extension to all observations in the el… Nov 7, 2024
Copy link

sonarcloud bot commented Nov 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
62.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link
Collaborator

@david-navapbc david-navapbc left a comment

Choose a reason for hiding this comment

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

I don't see where AC "The process to determine results are antigen is documented" is met.

I also don't see any new tests.

Please address the above and I'll take another look.

ty!

@@ -34,16 +42,32 @@ class LookupTableConditionMapper(metadata: Metadata) : IConditionMapper {
acc
}
}

override fun lookupMemberOid(codings: List<Coding>): Map<String, String> {
return mappingTable.FilterBuilder()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this statement is dense and difficult to parse. In future please don't pack so much logic into the return statement

Copy link
Collaborator

@arnejduranovic arnejduranovic left a comment

Choose a reason for hiding this comment

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

Changes look good, but I agree with the need for tests. At a minimum, an integration test similar to: should send valid FHIR report filtered by mapped condition filter. This would also show what a FHIRExpression filter would look like. Also see FHIRBundleHelpersTest (like addMappedCondition supports adding a condition when code is null) for guidance on unit tests.

}
acc
}
}
}

class ConditionStamper(private val conditionMapper: IConditionMapper) {
companion object {
const val conditionCodeExtensionURL = "https://reportstream.cdc.gov/fhir/StructureDefinition/condition-code"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can fix the IntelliJ warning on this var name while we're here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes please. Trying to see how to fix this?

@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from 884f3fe to 9943de0 Compare November 12, 2024 19:32
@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from 9943de0 to 04bd870 Compare November 12, 2024 19:33
@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from 04bd870 to ead1fd8 Compare November 12, 2024 19:52
@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from ead1fd8 to cb7de06 Compare November 12, 2024 19:53
@kant777
Copy link
Collaborator Author

kant777 commented Nov 12, 2024

I don't see where AC "The process to determine results are antigen is documented" is met.

I also don't see any new tests.

Please address the above and I'll take another look.

ty!

Sorry the tests were in another branch. rebased and merged.

@kant777 kant777 force-pushed the feature/14511-add-test-member-oid-extension branch from cb7de06 to 5536e4a Compare November 12, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants