Conversation
sonzsara
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The observation status filter unintentionally excludes laboratory tests without active observation definitions, and the filename contains a typo.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds an SSMM laboratory-test query documenting linked clinical definitions and reference ranges.
Changes:
- Adds a static SQL report for laboratory service requests.
- Includes charge items, specimens, activity definitions, observations, and qualified ranges.
| File | Description |
|---|---|
Care/Clinical/lab_test_with_activity_definition_reference_ranges_and_observation_defintion_ssmm.md |
Documents the laboratory-test reference-range query. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| LEFT JOIN emr_observationdefinition | ||
| ON emr_observationdefinition.id = emr_observation.observation_definition_id | ||
| WHERE emr_servicerequest.status != 'entered_in_error' | ||
| AND emr_chargeitem.status != 'entered_in_error' | ||
| AND emr_servicerequest.category = 'laboratory' | ||
| AND emr_observationdefinition.status = 'active' |
| @@ -0,0 +1,56 @@ | |||
| # Lab Tests with Activity Definition, Reference Ranges and Observation Definition - SSMM | |||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Analytics SQL Review — SSMM lab test / activity definition / reference range query
Ticket: none could be retrieved for branch ENG-1017 (JIRA returned 404/401 for the review bot's token). I can't verify requirement fidelity; please confirm the ticket is accessible and the branch name matches it. Assumed ask: list lab tests with charge item, specimen, activity and observation definitions and reference ranges for SSMM.
Verdict: not yet safe to publish. The parse is clean (lint: no findings), but:
- No facility scope despite the
_ssmmsuffix, and nodeleted = FALSE(High). - The
WHERE emr_observationdefinition.status = 'active'negates the LEFT JOIN (High). DISTINCTover patient-level observation joins hides fan-out and is costly (Medium).- Low: filename typo
defintion→definition. The doc is otherwise template-conformant andLast updatedis set; Notes should document the facility ID once added.
Generated by Analytics SQL Reviewer for #160 · auto · 14.1 AIC · ⊞ 14.3K
| WHERE emr_servicerequest.status != 'entered_in_error' | ||
| AND emr_chargeitem.status != 'entered_in_error' | ||
| AND emr_servicerequest.category = 'laboratory' | ||
| AND emr_observationdefinition.status = 'active' |
There was a problem hiding this comment.
High – this WHERE predicate turns the LEFT JOIN emr_observationdefinition into an inner join, so tests with no observations/active definition silently disappear (same point as the Copilot review). If tests without an active definition should still be listed, move it into the join:
LEFT JOIN emr_observationdefinition
ON emr_observationdefinition.id = emr_observation.observation_definition_id
AND emr_observationdefinition.status = 'active'If dropping them is intended, use JOIN and say so in Notes. Same applies to emr_chargeitem.status != 'entered_in_error' — fine as an inner join, but consider whitelisting valid statuses.
| ## Query | ||
|
|
||
| ```sql | ||
| SELECT DISTINCT |
There was a problem hiding this comment.
Medium – SELECT DISTINCT over a join through emr_observation → emr_diagnosticreport → emr_specimen builds one row per patient observation before de-duplicating, which masks fan-out and will be slow on production volumes. Since only definition-level attributes are returned, consider joining emr_activitydefinition → its observation definitions directly (if the model links them) instead of via patient results. Also, emr_chargeitem.service_resource_id = external_id::text is a cast on the indexed side, so it can't use the external_id index; confirm this is the only link. Note: the Purpose says "each distinct test", but the observation path only surfaces definitions that have actually been used.
| ON emr_observation.diagnostic_report_id = emr_diagnosticreport.id | ||
| LEFT JOIN emr_observationdefinition | ||
| ON emr_observationdefinition.id = emr_observation.observation_definition_id | ||
| WHERE emr_servicerequest.status != 'entered_in_error' |
There was a problem hiding this comment.
High – no facility scope (and no deleted filter). The _ssmm suffix says this is SSMM-specific, but nothing restricts the query to that facility, so on a shared DB it returns lab tests from every facility. Also add deleted = FALSE on the tables the ORM would filter.
Suggested (verify column names against care/emr/models):
WHERE emr_servicerequest.facility_id = <ssmm facility id> -- document in Notes
AND emr_servicerequest.deleted = FALSE
AND emr_chargeitem.deleted = FALSEIf the chargeitem facility is the intended scope, use that instead. Whichever ID you use needs a "what it is / when to update" line in ## Notes.
No description provided.