Skip to content

feat: Add foreign key on ecr_rr_conditions to the condition_reference table #678

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

Merged
merged 60 commits into from
Jun 2, 2025

Conversation

akasper
Copy link
Collaborator

@akasper akasper commented May 1, 2025

PULL REQUEST

Now that we have a consistent identifier for conditions (via the condition_reference table), are add a foreign key reference to those conditions in our per-eCR reportability response conditions.

Summary

  • A database schema change that adds condition_id to ecr_rr_conditions, as well as a foreign key
  • The attendant TypeScript interface definition changes for ecr_rr_conditions
  • Modifications to data saving logic
  • Logic for backfilling existing data
  • Tests that document and prove the functionality

Related Issue

Ticket #513

Acceptance Criteria

  • A nullable column called condition_id should be added to the ecr_rr_conditions table (both schemas). It should be a foreign key to condition_reference
    • postgres
    • sql server
  • On saving data to ecr_rr_conditions it should be stamped with this ID if it can be determined
  • Create a migration such that the data already in the ecr_rr_conditions table gets an accurate fk reference to the condition_reference table

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.57%. Comparing base (aa306ab) to head (b9db987).

Files with missing lines Patch % Lines
...-db/migrations/core/20250522120000_condition_fk.ts 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #678      +/-   ##
==========================================
+ Coverage   87.74%   92.57%   +4.83%     
==========================================
  Files         287      178     -109     
  Lines       14667     7956    -6711     
  Branches      993      990       -3     
==========================================
- Hits        12869     7365    -5504     
+ Misses       1780      573    -1207     
  Partials       18       18              
Flag Coverage Δ
ecr-viewer 93.01% <72.72%> (-0.06%) ⬇️
fhir-converter ?
ingestion ?
message-parser 96.12% <ø> (ø)
message-refiner ?
orchestration 85.45% <ø> (ø)
record-linkage ?
trigger-code-reference ?
validation ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@akasper akasper force-pushed the akasper/condition-ref-fk branch from aab819c to d50d5c8 Compare May 1, 2025 11:17
@akasper akasper force-pushed the akasper/condition-ref-fk branch from c1ca43d to b4b5673 Compare May 7, 2025 15:56
@austin-hall-skylight
Copy link
Collaborator

I noticed that COVID-19 has two condition codes and when we backfill the condition codes we are setting condition_code in ecr_rr_conditions to whatever value came last. I'm not sure which - if either - is the correct value but I'd like to figure that out.

in our tes.db:
image

logs of the rows we are iterating through when backfilling condition codes:
image

The final values in ecr_rr_conditions:
image

@mcmcgrath13
Copy link
Collaborator

mcmcgrath13 commented May 27, 2025

I noticed that COVID-19 has two condition codes and when we backfill the condition codes we are setting condition_code in ecr_rr_conditions to whatever value came last. I'm not sure which - if either - is the correct value but I'd like to figure that out.

I was also just noticing this in the program form page. There are a few other duplicate condition names there as well. This comes from TES having multiple codes

image

I'm not sure what we should do about that in practice though...

@austin-hall-skylight
Copy link
Collaborator

I noticed that COVID-19 has two condition codes and when we backfill the condition codes we are setting condition_code in ecr_rr_conditions to whatever value came last. I'm not sure which - if either - is the correct value but I'd like to figure that out.

I was also just noticing this in the program form page. There are a few other duplicate condition names there as well. This comes from TES having multiple codes

image I'm not sure what we should do about that in practice though...

Let's discuss at standup. Maybe normalizing the ecr_rr_condition to condition code mapping out into another table would help, but I hate to throw that wrench into things this far into it 😖

const db = getDb<Extended>();
await db.insertInto("user").values(adminUser).execute();
await db.insertInto("program_area").values(programArea).execute();
await db
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question here

@@ -18,6 +18,12 @@ const baseCoreMetadata: BundleMetadata = {
rr: [],
report_date: "12/20/2024",
};
const condition_reference = {
code: "123",
concept_name: "condition (disease)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this condition reference and before each needed?

@akasper akasper added this pull request to the merge queue Jun 2, 2025
Merged via the queue into main with commit c043e7b Jun 2, 2025
21 checks passed
@akasper akasper deleted the akasper/condition-ref-fk branch June 2, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants