Conversation
…r/Appointment Details query
There was a problem hiding this comment.
Pull request overview
Adds a new Markdown documentation page describing the “Patients with Billable Charge Item and Encounter/Appointment Details” SSMM query so analysts can understand intent, parameters, and the exact SQL used.
Changes:
- Introduces a new SSMM query documentation file with Purpose/Parameters/Notes sections.
- Documents the SQL query that returns encounters with associated token bookings and billable charge items.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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 — billable charge item + encounter/appointment report (ENG-910)
Ticket ask: report billable charge items with associated patient, encounter, and appointment details. The query delivers the right shape (patient → encounter → token booking → charge item, filtered to status = 'billable' and total_price > 0) and column set matches the ask, so requirement fidelity is fine at the grain level.
Numbers are not yet trustworthy — two correctness issues found:
- Critical — no facility scoping. Despite the
_ssmmfilename, no join orWHEREclause constrains to a single facility. This returns billable charge items across every facility, not just SSMM. - High — no
entered_in_error/deletedexclusion onemr_encounter,emr_tokenbooking, oremr_patientidentifier. Only the charge item's own status is checked; a mistakenly-created encounter/booking can still surface with a real charge item attached.
Documentation: Purpose/Notes/Parameters are accurate to the SQL and the earlier reviewer's "Purpose vs actual columns" comment was addressed. One gap remains: the template's ### Output column table is missing entirely (Medium).
Hygiene: correct domain folder, correct filename convention with _ssmm suffix, one file per PR, magic config_id = 21 documented in Notes — all fine.
Verdict: not yet safe to publish — fix the facility scope (Critical) and add status/deleted filters on the joined tables (High) before this feeds the dashboard. Output table doc gap is a quick follow-up, not a blocker for correctness.
Generated by Analytics SQL Reviewer for #147 · auto · 37.3 AIC · ⌖ 3.2 AIC · ⊞ 14.7K
| ON emr_tokenbooking.associated_encounter_id = emr_encounter.id | ||
| JOIN emr_chargeitem | ||
| ON emr_chargeitem.id = emr_tokenbooking.charge_item_id | ||
| WHERE emr_chargeitem.status = 'billable' |
There was a problem hiding this comment.
[Critical] No facility scoping — the _ssmm suffix promises a single-facility query, but nothing in the SQL enforces it.
Facility boundaries are normally enforced by the application, not by Postgres — a raw query has to add it explicitly (care-sql-code-review skill, inversion 2). This query has no facility_id predicate anywhere: not on emr_encounter, not on emr_tokenbooking, not on emr_chargeitem. As written it returns billable charge items across every facility in the deployment, not just SSMM, which is both a wrong-number bug for a per-facility dashboard and a cross-tenant data exposure.
Fix — scope through the encounter (or another table that carries facility_id):
WHERE emr_chargeitem.status = 'billable'
AND emr_chargeitem.total_price > 0
AND emr_encounter.facility_id = <ssmm_facility_id>
...If the intent is genuinely "all facilities, but named _ssmm for another reason," please say so in ## Notes — as it stands the filename and the SQL disagree.
There was a problem hiding this comment.
yes, there is only one facility and thats ssmm
There was a problem hiding this comment.
Understood — thanks for confirming there's only one facility in this deployment today.
One thing worth flagging even so: this repo's convention for other single-facility assumptions (e.g. the config_id = 21 identifier mapping right below) is to say so explicitly in ## Notes, precisely because these facts aren't visible from the SQL itself and can silently stop being true (a second facility gets provisioned, a new encounter type is added elsewhere, etc.) without anyone noticing until the dashboard numbers are wrong. Right now ## Notes documents the identifier mapping but says nothing about the facility assumption.
Suggest adding a line such as:
Facility scope: This deployment currently has only one facility (SSMM); no explicit
facility_idfilter is applied. If a second facility is ever added to this instance, addAND emr_encounter.facility_id = <ssmm_facility_id>to keep this report scoped correctly.
That turns a query-level assumption into a documented, checkable one rather than an implicit fact a future reader has to rediscover. Not blocking — resolving this thread since the underlying data-scope concern is a conscious, confirmed decision for the current environment.
Generated by Analytics SQL Reviewer for #147 · auto · 30.9 AIC · ⌖ 2.38 AIC · ⊞ 14.7K
| --[[AND {{encounter_status}}]] | ||
| --[[AND emr_patientidentifier.value = {{ssmm_id}}]] | ||
| ORDER BY emr_encounter.created_date DESC, emr_patient.name; | ||
| ``` |
There was a problem hiding this comment.
[Medium] Missing ### Output section required by TEMPLATE.md.
The template calls for an ### Output table (column/type/description) directly under ## Query, documenting the 8 selected columns for the dashboard consumer. This doc jumps straight from the SQL fence to ## Notes — please add the output table before merging.
There was a problem hiding this comment.
Outputs are not needed, they are optional.
There was a problem hiding this comment.
Fair enough — ### Output is indeed optional per TEMPLATE.md conventions in this repo (only ## Purpose, ## Parameters, and ## Query are consistently required across existing docs). Not blocking on this. Resolving.
Generated by Analytics SQL Reviewer for #147 · auto · 30.9 AIC · ⌖ 2.38 AIC · ⊞ 14.7K
No description provided.