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

16148: update FHIR reciever logic to be simpler #16370

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

Conversation

mkalish
Copy link
Collaborator

@mkalish mkalish commented Oct 28, 2024

This PR simplifies the FHIR receiver and removes the few bugs in sender parsing and event tracking. The simplification can be done because the FHIR Converter handles the same cases more robustly

Test Steps:

  1. Turn on the submissions service and reportstream
  2. Send a report to submissions
  3. Verify it gets picked up by report stream

Changes

  • Drops event handling for errors in favor of putting messages on to the queue directly
  • Removes the functionality duplicated with FHIR converter

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • (For Changes to /frontend-react/...) Ran npm run lint:write?
  • Added tests?

Process

  • Are there licensing issues with any new dependencies introduced?
  • Includes a summary of what a code reviewer should test/verify?
  • Updated the release notes?
  • Database changes are submitted as a separate PR?
  • DevOps team has been notified if PR requires ops support?

Linked Issues

To Be Done

TBD: but there might be another follow up ticket to address this more efficiently and remove the FHIR Receiver step, but it would require some updates to the how the item lineage works

Specific Security-related subjects a reviewer should pay specific attention to

Copy link

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

Copy link

Test Results

1 246 tests   - 5   1 242 ✅  - 5   7m 32s ⏱️ +9s
  164 suites ±0       4 💤 ±0 
  164 files   ±0       0 ❌ ±0 

Results for commit b71dca1. ± Comparison against base commit 7c97376.

This pull request removes 5 tests.
gov.cdc.prime.router.fhirengine.azure.FHIRReceiverIntegrationTests ‑ should handle inactive sender gracefully()
gov.cdc.prime.router.fhirengine.azure.FHIRReceiverIntegrationTests ‑ test process CSV message()
gov.cdc.prime.router.fhirengine.azure.FHIRReceiverIntegrationTests ‑ test process invalid FHIR message()
gov.cdc.prime.router.fhirengine.azure.FHIRReceiverIntegrationTests ‑ test process invalid HL7 message()
gov.cdc.prime.router.fhirengine.engine.FHIRReceiverTest ‑ test handle inactive sender()

Copy link

sonarcloud bot commented Oct 28, 2024

Copy link

Integration Test Results

 53 files   53 suites   28m 59s ⏱️
411 tests 402 ✅ 9 💤 0 ❌
414 runs  405 ✅ 9 💤 0 ❌

Results for commit b71dca1.

@@ -296,28 +276,6 @@ class ReportStreamEventService(
).send()
}

override fun sendSubmissionProcessingError(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This didn't align with how the other reports were going to work and likely would have led to confusion as this was inverting parameters compared to other builders

// Handle case where sender is not found
return if (sender == null) {
// Send an error event
reportEventService.sendSubmissionProcessingError(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than emit an event, we're now adding an item to the poison queue which will cause an alert and enables the engagement team to fix the configuration and requeue the message.

null
} else {
// Handle case where sender is inactive
if (sender.customerStatus == CustomerStatus.INACTIVE) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped this since the UP via the ReportFunction silently just drops a message. I'm of the opinion that if a sender is not suppose to sending reports we should revoke there access rather than have a flag that gets checked so late in the process.

val report: Report
val sources = listOf(ClientSource(organization = sender.organizationName, client = sender.name))

when (sender.format) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This checking is robustly handled in the convert step and does not need to be duplicated here.

@mkalish mkalish marked this pull request as ready for review October 29, 2024 13:04
@mkalish mkalish requested a review from a team as a code owner October 29, 2024 13:04
@arnejduranovic arnejduranovic added the platform Platform Team label Oct 30, 2024
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.

2 participants