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

Added member oid extension for each observation #16615

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

Conversation

kant777
Copy link
Collaborator

@kant777 kant777 commented Nov 20, 2024

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 November 20, 2024 19:17
Copy link
Contributor

github-actions bot commented Nov 20, 2024

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

Copy link
Contributor

github-actions bot commented Nov 20, 2024

Test Results

1 272 tests  +7   1 268 ✅ +7   7m 19s ⏱️ -3s
  165 suites +1       4 💤 ±0 
  165 files   +1       0 ❌ ±0 

Results for commit 94a4472. ± Comparison against base commit fbede22.

This pull request removes 2 and adds 9 tests. Note that renamed tests count towards both.
gov.cdc.prime.router.azure.ReportFunctionTests ‑ getReceiver()
gov.cdc.prime.router.azure.ReportFunctionTests ‑ processFhirDataRequest no environment, receiver name, or org name and output format blank()
gov.cdc.prime.router.azure.ReportFunctionTests ‑ processFhirDataRequest nonCLI request in staging without access token should fail()
gov.cdc.prime.router.azure.ReportFunctionTests ‑ processFhirDataRequest receiver name, or org name and output format blank()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest exception error()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest ok()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest with bad sender()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest with no sender()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest with unauthenticated sender()
gov.cdc.prime.router.azure.SenderFunctionTest ‑ test SenderFunction conditionCodeComparisonPostRequest with unauthorized sender()
gov.cdc.prime.router.fhirengine.engine.FhirConverterTests ‑ test condition code and member OID stamping()

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 20, 2024

Integration Test Results

 60 files   60 suites   43m 24s ⏱️
428 tests 418 ✅ 10 💤 0 ❌
431 runs  421 ✅ 10 💤 0 ❌

Results for commit 94a4472.

♻️ This comment has been updated with latest results.

@jack-h-wang jack-h-wang added the platform Platform Team label Nov 20, 2024
@mkalish mkalish removed their request for review November 20, 2024 20:10
@arnejduranovic arnejduranovic self-assigned this Nov 20, 2024
@kant777 kant777 force-pushed the platform/kant/14511-add-test-member-oid-extension branch from 7023bfc to 0edf955 Compare December 3, 2024 10:32
@kant777 kant777 requested a review from a team as a code owner December 3, 2024 10:32
@kant777 kant777 force-pushed the platform/kant/14511-add-test-member-oid-extension branch 2 times, most recently from 0edf955 to 7023bfc Compare December 3, 2024 10:46
@kant777 kant777 force-pushed the platform/kant/14511-add-test-member-oid-extension branch from 7023bfc to d14dae0 Compare December 4, 2024 21:19
@jalbinson
Copy link
Collaborator

jalbinson commented Dec 16, 2024

The fhir output seems strange with heavily nested and repeated urls. Is this intentional?

        "code":{
          "coding":[
            {
              "extension":[
                {
                  "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/condition-code",
                  "extension":[
                    {
                      "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/condition-code",
                      "valueCoding":{
                        "system":"SNOMEDCT",
                        "code":"6142004",
                        "display":"Influenza (disorder)"
                      }
                    },
                    {
                      "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
                      "extension":[
                        {
                          "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
                          "valueString":"OID12345"
                        }
                      ]
                    }
                  ]
                }
              ],
              "system":"http://loinc.org",
              "code":"80382-5"
            }
          ],
          "text":"Flu A"
        },

Is this what engagement team expects? I would assume they may want it a few levels up instead of code.coding.(condition-code).(test-performed-member-oid).(test-performed-member-oid).valueString

@arnejduranovic
Copy link
Collaborator

arnejduranovic commented Dec 16, 2024

I agree with Jamie's comment above. Specifically:

  1. Can we simplify:
{
  "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
  "extension":[
    {
      "url":"https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
      "valueString":"OID12345"
    }
  ]
}

to

{
  "url": "https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
  "valueString": "OID12345"
}
  • This change will also break the code that populates the events (probably)
    • Jessica's dashboard will also break since it reads this in a specific way

I spoke with Victor and he recommended this pattern (will need to verify this is valid, though):

  {
    "url": "https://reportstream.cdc.gov/fhir/StructureDefinition/condition-code",
    "valueCoding": {
      "system": "SNOMEDCT",
      "code": "7180009",
      "display": "Meningitis (disorder)"
      "extension": {
          "url": "https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",
          "valueString": "<menengitis OID goes here>"
      }
    }
  }

@jalbinson
Copy link
Collaborator

Getting unit test failures. Other than that it looks good given the structure has been validated by Engagement.

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.

I reviewed this from a perspective of trying to help @kant777 figure out the failing tests. You can ignore 90% of what I'm saying if you already figured them out :D

I did some manual testing and ran the tests locally. The new stamping works as expected (see details below) so great job there! As for the unit tests I left a couple comments, mostly it boils down to "the unit tests in FHIRConverterTests have some oddities". I'd like us to log a ticket to iron out some of that.

Once those are fixed the build is going to fail on the smoke tests because the output for those has changed, but apparently only for the MARS_OTC scenarios. (I didn't look into why that is but I'm assuming it is because of the specific condition that is being used in those scenarios).

To run our smoke tests locally you can use the command ./gradlew testEnd2EndUP. They can be a little confusing to parse at the moment but the basics of this test is that it runs through 5 scenarios. It begins by uploading each scenario. Then, one scenario at a time, it will poll the history endpoint to determine if that has been delivered. You'll see output similar to this 5 times:

Starting validation for scenario: Sending FHIR Report, Receiving FHIR (mars-otc-elr) // Scenario name and details
Polling for history endpoint for report ID: 6d57baa4-9899-4f96-8da7-4691b680bfab. (Max poll time 150 seconds) // Log of Report ID
Report 6d57baa4-9899-4f96-8da7-4691b680bfab status was Delivered after 5 seconds // Final status of history endpoint for that Scenario's Report
{
//  This is the output of the final history endpoint result. It's useful for finding out the names of the received reports //
}
Examining sent reports for the posted report ID 6d57baa4-9899-4f96-8da7-4691b680bfab // Now onto result verification
The report was received by MARS_OTC_ELR_FHIR_A_E2E // This is a/the expected receiver for that scenario
File successfully uploaded to blob store as 0507cdf1-f771-4523-826a-cbcd4957fce7.fhir // Verified that the report was found
The contents of 0507cdf1-f771-4523-826a-cbcd4957fce7.fhir matches the expected data // Verified the contents of the report 

Ultimately, to fix the test all that was needed was to paste in the new Observation.code results that included the stamping for the expected MARS FHIR results. We can walk through this after standup tomorrow. You can find the settings (and location of expected data files) for each scenario here.

Manual testing results were a success: I submitted an ORU_R01 report through ignore.ignore-full-elr-e2e. This successfully made its way through the pipeline and the resulting FHIR in my sftp folder matched the expected format. 🎉 🎉

"code": {  
  "coding": [  
    {  
      "extension": [  
        ...  
        {  
          "url": "https://reportstream.cdc.gov/fhir/StructureDefinition/condition-code",  
          "valueCoding": {  
            "extension": [  
              {  
                "url": "https://reportstream.cdc.gov/fhir/StructureDefinition/test-performed-member-oid",  
                "valueString": "2.16.840.1.113762.1.4.1146.1470"  
              }  
            ],  
            "system": "SNOMEDCT",  
            "code": "406575008",  
            "display": "Infection caused by vancomycin resistant enterococcus (disorder)"  
          }  
        },
        ... 

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.

Worked with Kant over the last few days on the tests and a small implementation tweak. Everything looks good and is passing locally now 👍

|"coding":[{"display":"BD Veritor System for Rapid Detection of SARS-CoV-2 & Flu A+B*"}]},"specimen":{"reference":"Specimen/52a582e4-d389-42d0-b738-bee51cf5244d"},"device":{"reference":"Device/78dc4d98-2958-43a3-a445-76ceef8c0698"}}}]}
""".trimMargin()

// Setup metadata (already present in your code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the meaning of this comment?

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.

Include new event structure in PR so Jessica can attend to the dashboard

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.

Map CSTE Member OID to member-oid extension during convert-step
5 participants