Fix submission history bugs and add integration test cases #15893

thetaurean · 2024-09-16T15:05:43Z

This PR fixes two bugs related to submission history and adds relevant tests to prevent reoccurrence.

Test Steps:

Run test suites

Changes

Submission history checks nextAction on latest report in lineage for reports without destinations
Submission history checks latest action instead of earliest

Checklist

Testing

Tested locally?
Added tests?

Linked Issues

Fixes [BUG] Submission History API doesn't show Filtered reports #15645
Fixes [BUG]: History endpoint intermediate overallStatus can be "Not Delivering" and then change later #15524

…n receiver filter; add details to filter result message

…acking-fixes

github-actions · 2024-09-16T15:05:59Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

…fixes

github-actions · 2024-09-16T15:28:17Z

Test Results

1 230 tests +1 1 225 ✅ ±0 7m 13s ⏱️ +4s
162 suites +1 4 💤 ±0
162 files +1 1 ❌ +1

For more details on these failures, see this check.

Results for commit f39a91f. ± Comparison against base commit fe3d9d7.

♻️ This comment has been updated with latest results.

github-actions · 2024-09-16T15:28:29Z

Integration Test Results

0 files - 53 0 suites - 53 0s ⏱️ - 27m 49s
0 tests - 410 0 ✅ - 401 0 💤 - 9 0 ❌ ±0
0 runs - 413 0 ✅ - 404 0 💤 - 9 0 ❌ ±0

Results for commit f39a91f. ± Comparison against base commit fe3d9d7.

♻️ This comment has been updated with latest results.

brick-green

a couple questions

brick-green · 2024-09-16T16:49:01Z

prime-router/src/main/kotlin/fhirengine/engine/FHIRConverter.kt

@@ -165,7 +165,7 @@ class FHIRConverter(
 ),
 metadata = this.metadata,
 topic = queueMessage.topic,
- nextAction = TaskAction.none
+ nextAction = TaskAction.destination_filter


Can you explain this change? It looks like this Report is created for a message that is not parseable and isn't going to be added to the destination filter queue.

Good catch this was left in by mistake; removing.

brick-green · 2024-09-16T17:00:43Z

prime-router/src/main/kotlin/history/SubmissionHistory.kt

 destination.organizationId == it.receivingOrg && destination.service == it.receivingOrgSvc
- }.sortedBy { it.createdAt }
- val latestAction = reportsForDestination.first().nextAction
+ } // .sortedBy { it.createdAt }


Delete this comment

brick-green · 2024-09-16T18:33:11Z

prime-router/src/test/kotlin/history/SubmissionHistoryIntegrationTests.kt

You've got a happy path test. Should there be an unhappy test?

brick-green · 2024-09-16T18:33:31Z

prime-router/src/main/kotlin/history/SubmissionHistory.kt

@@ -384,7 +398,7 @@ class DetailedSubmissionHistory(
 * the receivers.
 */
 return if (
- reports.size > 1
+ reports.size > 1 && !hasNextAction


Do you know why this is checking if reports.size is > 1 instead of reports.size is > 0?

I believe the assumption is that if we only have one report we must have only received it so far.

mkalish

Main thought is if we can add more tests to the existing tests file rather than introducing a new pattern.

mkalish · 2024-09-16T20:36:35Z

prime-router/src/main/kotlin/history/SubmissionHistory.kt

+ * Flag to check if there's a next action for a report related to this submission
+ */
+ @JsonIgnore
+ var hasNextAction = false


I totally forget how the nextActionScheduled is used, but maybe they can get collapsed?

Initially I did just modify the related check that sets this flag. But it is used differently in relation to Batch/Send steps so it seemed better to track both states separately.

mkalish · 2024-09-16T20:37:38Z

prime-router/src/test/kotlin/history/SubmissionHistoryIntegrationTests.kt

Curious if there is a way we can avoid this test since its pretty heavy. Is there a reason we can't do this by just seeding the db directly like the other tests do?

The intent was to create tests that couple the SubmissionHistory to the pipeline steps.

Thinking it over I could have added simpler tests to ensure the pipeline steps output the expected state (with respect to ActionHistory, etc.), then add corresponding tests to the Submission side with that expected state.

Working on this now, though I will leave this new pattern in a branch somewhere in case it becomes useful later on. Arnej has some vision for testing various SubmissionHistory scenarios that might require it.

The only problem I see with this approach is that if someone changes the pipeline such that the outputs (ActionLogs, etc.) change, the SubmissionHistory tests won't break because their input is agnostic to any changes in the pipeline. The associated pipeline tests will break (and get fixed) but the SubmissionHistory tests stay happy.

Maybe that's okay though, thoughts? We could always add a comment to the relevant pipeline tests to be wary of the loose coupling to history tests.

So I think thats ok? My two cents is that we're just testing that we can correctly read and parse what's been inserted into the DB and that a broader smoketest is what verifies that the whole thing working together.

The broader smoketest suite does not catch these bugs. So maybe enhancement is needed there?

Thought for one of these bugs I'm not sure current smoketest design pattern can catch the issue since it only presents at a specific point in the pipeline (after receiver step, before destination filter completion) and smoke tests can't reliably time such a history API call.

mkalish · 2024-09-16T20:38:04Z

prime-router/src/test/kotlin/history/azure/SubmissionFunctionIntegrationTests.kt

@@ -237,6 +240,9 @@ class SubmissionFunctionIntegrationTests {
 log(ActionLog(InvalidParamMessage("log"), type = ActionLogLevel.warning))
 reportGraphNode {
 action(TaskAction.route)
+ reportGraphNode {


Should there be another assertion change?

I took a look and did not think of any in the context of status/history.

I was thinking maybe an assertion to ensure the lineage is "terminated" so to to speak, but that would just assert against data we inserted.

mkalish · 2024-09-16T20:38:44Z

prime-router/src/main/kotlin/Receiver.kt

@@ -115,7 +116,8 @@ open class Receiver(
 timeZone = timeZone,
 dateTimeFormat = dateTimeFormat,
 reverseTheQualityFilter = reverseTheQualityFilter,
- enrichmentSchemaNames = enrichmentSchemaNames
+ enrichmentSchemaNames = enrichmentSchemaNames,
+ transport = transport


Is this used?

Yes used for setting the transport on a receiver to NullTransport so I can run the send step.

If I change the tests to not run the whole pipeline it will no longer be needed.

thetaurean added 7 commits September 11, 2024 09:02

Track filter result for filtered reports; set reportID on actionLog i…

2bd213e

…n receiver filter; add details to filter result message

Updated integration tests

e68bc0b

Updated unit test mocks

319ea20

Merge branch 'master' into platform/thetaurean/report-filterResult-tr…

c82962a

…acking-fixes

Merge branch 'master' into platform/thetaurean/report-filterResult-tr…

f08ab4a

…acking-fixes

Merge branch 'master' into platform/thetaurean/report-filterResult-tr…

1305b1f

…acking-fixes

Submission history tests and fixes

57500b2

thetaurean added the platform Platform Team label Sep 16, 2024

thetaurean requested a review from a team as a code owner September 16, 2024 15:05

thetaurean temporarily deployed to staging September 16, 2024 15:06 — with GitHub Actions Inactive

Merge branch 'master' into platform/thetaurean/submission-history-bug…

83f3bae

…fixes

thetaurean temporarily deployed to staging September 16, 2024 15:10 — with GitHub Actions Inactive

Andrey-Glazkv assigned brick-green Sep 16, 2024

Attempted fix for test

f39a91f

thetaurean temporarily deployed to staging September 16, 2024 16:40 — with GitHub Actions Inactive

brick-green reviewed Sep 16, 2024

View reviewed changes

mkalish reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix submission history bugs and add integration test cases #15893

Fix submission history bugs and add integration test cases #15893

thetaurean commented Sep 16, 2024

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

brick-green left a comment

brick-green Sep 16, 2024

thetaurean Sep 17, 2024

brick-green Sep 16, 2024

brick-green Sep 16, 2024

brick-green Sep 16, 2024

thetaurean Sep 17, 2024

mkalish left a comment

mkalish Sep 16, 2024

thetaurean Sep 17, 2024

mkalish Sep 16, 2024

thetaurean Sep 17, 2024

thetaurean Sep 17, 2024

mkalish Sep 17, 2024

thetaurean Sep 17, 2024 •

edited

Loading

mkalish Sep 16, 2024

thetaurean Sep 17, 2024

mkalish Sep 16, 2024

thetaurean Sep 17, 2024

Fix submission history bugs and add integration test cases #15893

Are you sure you want to change the base?

Fix submission history bugs and add integration test cases #15893

Conversation

thetaurean commented Sep 16, 2024

Changes

Checklist

Testing

Linked Issues

github-actions bot commented Sep 16, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

github-actions bot commented Sep 16, 2024 • edited Loading

Test Results

github-actions bot commented Sep 16, 2024 • edited Loading

Integration Test Results

brick-green left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thetaurean Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

thetaurean Sep 17, 2024 •

edited

Loading