CDCgov / prime-reportstream

ReportStream is a public intermediary tool for delivery of data between different parts of the healthcare ecosystem.
https://reportstream.cdc.gov
Creative Commons Zero v1.0 Universal
72 stars 40 forks source link

Clean up the submission/delivery code #14350

Closed mkalish closed 1 month ago

mkalish commented 5 months ago

User Story

As a RS developer, I would like the delivery submission endpoints to work effectively and to be well tested

Description/Use Case

The code for these endpoints is brittle and does not support easily iterating on the pipeline. Since the report lineage is agnostic of the steps the code should be able to be refactored to not need to specifically pull data out of the specifically named steps.

Risks/Impacts/Considerations

Dev Notes

The problem code is below. The issue is that the route steps are hardcoded rather than being flexible to support arbitrary steps

descendants.filter { it.actionName == TaskAction.translate }.forEach { descendant ->
                enrichWithTranslateAction(descendant)
            }
            descendants.filter { it.actionName == TaskAction.route }.forEach { descendant ->
                enrichWithRouteAction(descendant)
            }
            descendants.filter { it.actionName == TaskAction.convert }.forEach { descendant ->
                enrichWithConvertAction(descendant)
            }

Acceptance Criteria

Andrey-Glazkv commented 5 months ago

Hey team! Please add your planning poker estimate with Zenhub @adegolier @arnejduranovic @brick-green @jack-h-wang @jalbinson @mkalish @thetaurean

Andrey-Glazkv commented 5 months ago

Please add your planning poker estimate with Zenhub @JFisk42

Andrey-Glazkv commented 5 months ago

Please add your planning poker estimate with Zenhub @david-navapbc

david-navapbc commented 4 months ago

talked with @mkalish and got some clarity as to the scope of the ticket

probably going to leave the recursive db call alone for now and do that in a different, specifically scoped ticket. The SubmissionHistory obj is stateful and in theory can be refactored to present to the rest of the system in the same way.

Now that I have a better understanding of what the ask is I'm going to come up with an action plan and check in again with @mkalish to ensure we're in alignment as to the definition of done.

david-navapbc commented 4 months ago

going to try something like this

    private fun actionAgnosticEnrich(descendants: List<DetailedSubmissionHistory>) {
        descendants.forEach {
            destinations += it.destinations
            errors += it.errors
            warnings += it.warnings
            overallStatus = it.overallStatus
        }
    }

seems like the simplest way to hit the ACs

david-navapbc commented 4 months ago

unified method is failing on four tests.

SubmissionHistoryTests test DetailedSubmissionHistory overallStatus (delivered) test DetailedSubmissionHistory overallStatus (partially delivered) test UP enrichWithDescendants stopped at route test UP enrichWithDescendants filterLogs populate for the corresponding receiver

david-navapbc commented 4 months ago

@mkalish @arnejduranovic

I've investigated the cause of the remaining four test failures and in essence what's happening is that, depending on the action reported to have been taken, elements may be added to SubmissionHistory.destinations. The quantity and disposition of those destination elements matters with respect to computing the summary - including the overall status value.

Unfortunately what this means is that creating a common method to handle this isn't going to work without more surgery than a 3pt ticket allows. There are too many tests and other bits of logic that work only if the action-specific logic is in play.

Going to investigate a bit and see if I can't come up with a different solution. This is an FYI that the work of this is more involved than we originally planned for.

david-navapbc commented 4 months ago

talked with @mkalish

david-navapbc commented 4 months ago

it looks like if we change the DetailedSubmissionHistory class we're changing the payload callers receive upon submission.

https://github.com/CDCgov/prime-reportstream/blob/f473828604a8602d5df7ab91f0692292ab85d014/prime-router/src/main/kotlin/azure/ReportFunction.kt#L216

david-navapbc commented 4 months ago
Screenshot 2024-06-25 at 3.25.32 PM.png

This is a screenshot of what comes back when a caller submits a report. Note the "Location" header value providing the URL of of the submission history for this submission.

What's fun about that is it's wrong. The actual URL for that record is http://localhost:7071/api/waters/report/1221/history

This is why you get a 404 from the app when you try to use the provided history endpoint. I've been operating under the assumption this whole time there was a bug in either the serialization or the fetch logic when in fact the path itself is wrong.

I will fix that in this ticket. Additionally I'm going to look at why the RECEIVE action is not being recorded to the action_log table.

Lastly - I think for lots of reasons it makes sense to change the status enum.

here's what's currently there

I want to change it to

The reasons are many. Making this change makes the it a lot easier to meet the ACs of this ticket. It also makes this possible to make this area of code less wonky. Last but not least ask yourself why in general the sender cares about whether or a message has intended receivers or whether or not the message has been downloaded by intended receivers? From their perspective why do they care about anything other than the submission being valid and whether or not its been processed? Senders aren't using us to send reports to particular receivers, they are using us to have us worry about who is supposed to get what.

Meaning why are we telling them about delivery details when the app is here to abstract that away from them? Our value is we worry about the sausage making details of delivery. They just need to know if their submission was processed and whether or not the result of that processing produced some kind of error.

david-navapbc commented 4 months ago

@mkalish I added the updated url to ReportFunction.

david-navapbc commented 3 months ago

@mkalish cc: @arnejduranovic

It appears as though you've created a new git branch to work this ticket and in the course of doing so have undone the bugfix described in the comment above. Friendly reminder to put the fix back before putting the PR up for review.

ty!