Fix sp_d_morbidity_report_postprocessing: rewrite user-comment join via NRT staging CSV#837
Fix sp_d_morbidity_report_postprocessing: rewrite user-comment join via NRT staging CSV#837nullflux wants to merge 3 commits into
Conversation
…PR routines in orchestrator Two changes: 1. Extend bug #5b's patient_id-NULL fix to the 10 multi-condition nrt_investigation variants (22000010-22000100) and the Tetanus variant (22000200). The original cherry-pick (bb7cec9) only set patient_id on the foundation Inv (20000100); the Tier 3 variants left it NULL, so the same DELETE-by-sentinel-PATIENT_UID cascade that blocks HEPATITIS_DATAMART would block any condition-datamart SP whose query path goes through F_PAGE_CASE -> D_PATIENT lookup. 2. New orchestrator step 2.5 apply_pending_pr_routines pulls each pending-PR routine (PRs #837, #839, #840) from its bug branch via `git show` and applies it to the freshly-reset DB before fixtures. Without this the orchestrator aborts at step 8 on ldf_answers_tetanus.sql (bug #7 early-RETURN guard fires on a SUB data_type ldf_uid, and bug #8 SUBSTRING fires on the tetanus datamart SP). Remove a branch from the list once its PR merges and the baseline image is refreshed. End-to-end coverage uplift vs. coverage_tier_3.md baseline: HEPATITIS_DATAMART 0 -> 1 (5b) COVID_CASE_DATAMART 0 -> 1 (5b on 22000070) BMIRD_STREP_PNEUMO_DATAMART 0 -> 1 (5b on 22000100) F_PAGE_CASE 1 -> 6 (multi-condition + 5b) LDF_DIMENSIONAL_DATA 0 -> 5 (PR #839 bug-7) MORB_RPT_USER_COMMENT 0 -> 1 (PR #837 bug-3) Still 0 (separate per-condition blockers, not 5b-related): HEPATITIS_CASE, HEP100, F_STD_PAGE_CASE, STD_HIV_DATAMART, TB_DATAMART, VAR_DATAMART, LDF_TETANUS, LDF_HEPATITIS — see coverage_hep_datamart_investigation.md for per-condition diagnoses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thoughts on adding a unit test to cover this scenario? |
|
can you explain what is meant by staging CSV? |
I think its just that the UID column is an intermediate comma separated list of values, not a single UID.
|
|
have you been able to repro this in the UI? I haven't been able to get data into this table via either MasterETL or RTR. the data dictionary says this table is for comments on an "externally entered Morbidity Report" so maybe there's some non-UI route for morb reports? edit: 🕳️ 🐇 |
@mehansen I hadn't reproed this in the UI, only with generated / synthetic ODSE fixtures. That |
…PR routines in orchestrator Two changes: 1. Extend bug #5b's patient_id-NULL fix to the 10 multi-condition nrt_investigation variants (22000010-22000100) and the Tetanus variant (22000200). The original cherry-pick (bb7cec9) only set patient_id on the foundation Inv (20000100); the Tier 3 variants left it NULL, so the same DELETE-by-sentinel-PATIENT_UID cascade that blocks HEPATITIS_DATAMART would block any condition-datamart SP whose query path goes through F_PAGE_CASE -> D_PATIENT lookup. 2. New orchestrator step 2.5 apply_pending_pr_routines pulls each pending-PR routine (PRs #837, #839, #840) from its bug branch via `git show` and applies it to the freshly-reset DB before fixtures. Without this the orchestrator aborts at step 8 on ldf_answers_tetanus.sql (bug #7 early-RETURN guard fires on a SUB data_type ldf_uid, and bug #8 SUBSTRING fires on the tetanus datamart SP). Remove a branch from the list once its PR merges and the baseline image is refreshed. End-to-end coverage uplift vs. coverage_tier_3.md baseline: HEPATITIS_DATAMART 0 -> 1 (5b) COVID_CASE_DATAMART 0 -> 1 (5b on 22000070) BMIRD_STREP_PNEUMO_DATAMART 0 -> 1 (5b on 22000100) F_PAGE_CASE 1 -> 6 (multi-condition + 5b) LDF_DIMENSIONAL_DATA 0 -> 5 (PR #839 bug-7) MORB_RPT_USER_COMMENT 0 -> 1 (PR #837 bug-3) Still 0 (separate per-condition blockers, not 5b-related): HEPATITIS_CASE, HEP100, F_STD_PAGE_CASE, STD_HIV_DATAMART, TB_DATAMART, VAR_DATAMART, LDF_TETANUS, LDF_HEPATITIS — see coverage_hep_datamart_investigation.md for per-condition diagnoses.
… NRT staging CSV Squashed from branch aw/app-471/bug-3 (was PR #837, never merged upstream — landed here to keep aw/odse-test-seed self-contained). sp_d_morbidity_report_postprocessing's user-comment query attempted a two-hop nbs_odse.dbo.act_relationship traversal from the postprocessing layer, which violates the convention that postprocessing SPs read NRT staging only (see STRATEGY.md "Convention: postprocessing SPs read NRT staging only — never ODSE"). The join+filter was also self-defeating — filtered rows produced 0 user comments even when followups existed. Rewrite using nrt_morbidity_observation.followup_observation_uid (the CSV column the NRT staging row already projects from the act_relationship graph) joined to #morb_obs_reference filtered by obs_domain_cd_st_1. Original upstream branch retained at aw/app-471/bug-3 for reference; squashed commit identity: a32456a.
Bug-3 (#837), bug-7 (#839), and bug-8 (#840) fixes are now squashed into aw/odse-test-seed as standalone commits (marked [SQUASH bug-N] in the log). Bug-5a's fix landed earlier today in the same branch. The orchestrator no longer needs to reach into other branches via `git show` mid-process to overlay routine files. liquibase's `runOnChange: true` flag on every routine entry in db.rdb_modern.changelog-16.1.yaml means the locally-built liquibase image picks up our routine files automatically on `docker compose up -d liquibase`. The merge pipeline is now fully deterministic from the working tree alone — no branch state required.
ericbuckley
left a comment
ericbuckley
left a comment
There was a problem hiding this comment.
TIL what CROSS APPLY does!
…aging CSV
The step "Generating ##SAS_morb_Rpt_User_Comment" joined the Order
observation to itself via root.morb_rpt_uid = obs.observation_uid and
then filtered obs.obs_domain_cd_st_1 IN ('C_Order','C_Result'), which
is impossible because the Order row's obs_domain_cd_st_1 is 'Order'.
The temp table was therefore always empty and MORB_RPT_USER_COMMENT
never populated.
Replaced the broken self-join with a staging-side walk: the upstream
NRT row #nrt_morbidity_observation already carries
followup_observation_uid as a CSV of the Order's children (mixed
C_Order / C_Result / Result), so this step expands that CSV via
CROSS APPLY string_split and joins to #morb_obs_reference filtered to
obs_domain_cd_st_1 = 'C_Result' to pull the user-comment row.
Stays entirely within RDB_MODERN staging — no cross-DB read of
nbs_odse.dbo.act_relationship, consistent with the
sp_nrt_*_postprocessing layer's "NRT-only" convention (cross-DB ODSE
joins live exclusively in sp_*_event SPs).
Verified locally: applied the routine, truncated MORB_RPT_USER_COMMENT,
ran sp_d_morbidity_report_postprocessing for the seeded morb (uid
20080010), confirmed 1 row populated with the seeded user-comment
text. job_flow_log shows step 19 (build ##SAS_morb_Rpt_User_Comment)
and step 27 (Insert into morb_Rpt_User_Comment) both at row_count=1
with no errors.
nullflux
force-pushed
the
aw/app-471/bug-3
branch
from
a32456a to
51597a4
Compare
Regression coverage for the fix in 51597a4. Seeds a minimal externally- entered morb report into RDB_MODERN staging — patient, the Order, a C_Order (MorbComment) sibling, and the C_Result (MRB180) user comment with its text — then EXECs sp_d_morbidity_report_postprocessing and asserts one row lands in MORB_RPT_USER_COMMENT with the comment text. The fixture mirrors the real UI-entered shape: NBS flattens the Order's descendant observation_uids into nrt_observation.followup_observation_uid as a CSV, and the comment text lives on the C_Result child. The C_Order sibling is included so the test exercises the obs_domain_cd_st_1='C_Result' discriminator. Against the pre-fix self-join the query returns 0 rows; against the fix it returns 1. Runs under DataDrivenUnitTests (test-unit).
|
@mehansen Thanks again to both you and Hailey for documenting how to make this data in the UI! There's one extra step that matters... after the external user submits, the report lands in the DRSA queue with no program area or jurisdiction. We have to open it as an internal user with As for why it worked in MasterETL but looked broken in RTR: the The updated fix should be right for the real data shape. I added a unit test ( |
#26 KEYSTONE (morbidity.sql): add PATSBJ participation morb Order 20080010 -> patient 20000000 so nrt_observation.patient_id resolves -> sp_nrt_morbidity_report_postprocessing stops throwing Error 515 -> OBSERVATION entity no longer fails -> the service's fail-fast batch short-circuit no longer skips CONTACT/VACCINATION/lab. This DETERMINISTICALLY fixes the 'flakiness' (covid_contact +51, d_contact_record +39, f_contact_record_case +11, d_place +6 now stable) AND populates morb_rpt_user_comment 0->8 (closes the PR #837 / task #26 morb PATIENT_KEY gap at its source) + lab_rpt_user_comment +8, lab100 +5. Phase C COVID (zz_covid_dedicated_entities.sql + enriched covid_investigation_full_chain.sql PHC + coordinated SubjOfPHC ownership in zz_investigation_patient_links.sql): dedicated rich patient/provider/ org (22055xxx) -> covid_case_datamart +54 (318->372) PATIENT_*/PHYS_*/RPT_*/HOSPITAL + PHC-core scalars. Net +183 cols, idle-verified, zero per-table regressions.
4 participants
Description
The user-comment query at lines 802-816 of
016-sp_nrt_morbidity_report_postprocessing-001.sqljoined the morb Order observation to itself and then filteredobs_domain_cd_st_1 IN ('C_Order','C_Result'). This is impossible, since the Order row's domain is'Order'. The temp table was always empty andMORB_RPT_USER_COMMENTnever populated.This PR rewrites it to walk Order to C_Result via the staging CSV
nrt_morbidity_observation.followup_observation_uid(which the NRT layer already projects from the act_relationship graph), then filter#morb_obs_referencetoobs_domain_cd_st_1 = 'C_Result'for the user-comment row'sactivity_to_time,add_user_id, and linked text.Verified locally by truncating
MORB_RPT_USER_COMMENT, ran the SP for a morb Order from the ODSE generated test fixtures (uid=20080010), with table then showing the expected row.job_flow_logsteps 19 and 27 at row_count=1, no errors.Related Issue
APP-471
Additional Notes
No
testData/unitfixture — exercising the SP needs ~10 seeded tables across both DBs. End-to-end repro is inutilities/comparison-fixtures/bugs/03_morb_rpt_user_comment/repro.sql.Checklist