You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add lab tests documentation with activity definition, observations and reference ranges - #160
sonzsara
changed the title
Add lab tests documentation with activity definition, observations and refernce ranges
Add lab tests documentation with activity definition, observations and reference ranges
Sep 30, 2026
The reason will be displayed to describe this comment to others. Learn more.
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 _ssmm suffix, and no deleted = FALSE (High).
The WHERE emr_observationdefinition.status = 'active' negates the LEFT JOIN (High).
DISTINCT over patient-level observation joins hides fan-out and is costly (Medium).
Low: filename typo defintion → definition. The doc is otherwise template-conformant and Last updated is set; Notes should document the facility ID once added.
The reason will be displayed to describe this comment to others. Learn more.
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
ONemr_observationdefinition.id=emr_observation.observation_definition_idANDemr_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.
The reason will be displayed to describe this comment to others. Learn more.
Addressed: at the head SHA the join is now an explicit inner JOIN emr_observationdefinition, so the status = 'active' predicate no longer silently converts a LEFT JOIN. Note the Purpose still says "each distinct laboratory test", but tests with no observation or active definition are now dropped. Consider one line in Notes saying so.
The reason will be displayed to describe this comment to others. Learn more.
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.
This query has no parameters, but the repository template explicitly says to delete the Parameters section in that case (TEMPLATE.md:9-15). Remove the placeholder table rather than documenting a synthetic “none” parameter.
Ticket: NO TICKET FOUND. JIRA returned 404 for ENG-1017 (the branch name), so I can't review requirement fidelity. Please confirm the branch name is the ticket ID and that the ticket is visible.
Since last round
The LEFT JOIN null-rejection finding is addressed. The join is now an explicit inner JOIN, so I resolved that thread. Notes should state that tests with no observation or active definition are excluded.
The facility-scope point is settled by your explanation of the single-facility SSMM deployment. Please record that assumption in Notes.
Still open
The missing deleted = FALSE filters on emr_servicerequest and emr_chargeitem. These are correctness issues, not style.
SELECT DISTINCT over the observation → diagnostic report → specimen join, which fans out to one row per patient observation before de-duplicating. It masks the fan-out and will be slow on production volumes.
emr_chargeitem.service_resource_id = external_id::text puts a cast on the indexed side, so the external_id index can't be used.
Filename typo defintion (also raised by Copilot). Rename it to ...observation_definition_ssmm.md.
Verdict: I wouldn't publish this to the dashboard until the deleted filters are in and the DISTINCT fan-out is either justified or removed. The lint report found no parse or template issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.