Conversation
…cific organizations
![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 — manager-role-by-organization user list
Ticket: Branch is ENG-971, but the ticket fetch returned HTTP 404/401 (see lint context) — I can't verify this query against a JIRA requirement. Treating it as an unlinked-ticket PR; please confirm the branch/ticket ID is correct.
Query logic: Straightforward lookup — emr_organizationuser joined to users_user, emr_organization, security_rolemodel, filtered to role_id = 100 and organization_id = 101. Grain (one row per user with that role at that org) and output columns look right for a manager-role roster.
One correctness finding (inline): deleted = FALSE is applied to emr_organization but missing on emr_organizationuser, users_user, and security_rolemodel — all of which carry the same soft-delete flag from BaseModel/CustomUserManager. Since none of these tables has a status column, deleted is the actual validity signal here, so a removed org-user link, deactivated account, or deleted role would still surface as an active manager.
Docs/hygiene: Both magic IDs (role_id = 100, organization_id = 101) are explained in Notes per house style, Parameters section correctly states none exist, file is in the right domain folder (Care/Organisation/) with proper _kc suffix and snake_case name, and Last updated matches the change. No other findings.
Not safe to treat as final until the deleted filters are added and the ticket link is confirmed, but this is a small, low-risk fix.
Generated by Analytics SQL Reviewer for #157 · auto · 60.6 AIC · ⌖ 2.44 AIC · ⊞ 14.7K
| ON o.id = ou.organization_id | ||
| AND o.deleted = FALSE | ||
| INNER JOIN security_rolemodel r | ||
| ON r.id = ou.role_id |
There was a problem hiding this comment.
Missing deleted = FALSE on ou, u, and r. emr_organization is filtered (o.deleted = FALSE), but emr_organizationuser, users_user, and security_rolemodel all inherit the same BaseModel.deleted flag (Django's default manager adds WHERE deleted = FALSE on each of these) and none of the three are filtered here. Since these tables have no status field, deleted is the validity flag for them (unlike clinical resources where status matters more) — so a soft-deleted org-user assignment, a deactivated user account, or a deleted role would still show up as an active "manager" in this list. Suggest:
FROM emr_organizationuser ou
INNER JOIN users_user u
ON u.id = ou.user_id AND u.deleted = FALSE
INNER JOIN emr_organization o
ON o.id = ou.organization_id
AND o.deleted = FALSE
INNER JOIN security_rolemodel r
ON r.id = ou.role_id AND r.deleted = FALSE
WHERE ou.role_id = 100
AND ou.organization_id = 101
AND ou.deleted = FALSEThere was a problem hiding this comment.
Partial fix — ou.deleted = FALSE was added to the WHERE clause, thanks. But users_user (u) and security_rolemodel (r) are still unfiltered on deleted, so a deactivated user account or a deleted role can still surface as an active manager here. Suggest adding both to the join conditions:
INNER JOIN users_user u
ON u.id = ou.user_id AND u.deleted = FALSE
INNER JOIN emr_organization o
ON o.id = ou.organization_id
AND o.deleted = FALSE
INNER JOIN security_rolemodel r
ON r.id = ou.role_id AND r.deleted = FALSELeaving this thread open until u and r are covered too.
Generated by Analytics SQL Reviewer for #157 · auto · 24.2 AIC · ⌖ 5.54 AIC · ⊞ 14.7K
There was a problem hiding this comment.
Analytics SQL Review — manager-role-by-organization user list
Re-review (delta since last round): the only change is adding ou.deleted = FALSE to the WHERE clause, per the earlier finding.
Still open: that finding covered three tables — emr_organizationuser, users_user, and security_rolemodel — all inheriting the same BaseModel/CustomUserManager soft-delete flag with no status column to fall back on. Only ou was fixed; u (users_user) and r (security_rolemodel) remain unfiltered, so a deactivated user account or a deleted role definition can still appear as an active manager in this roster. Replied on the original thread with the same suggested join predicates (u.deleted = FALSE, r.deleted = FALSE) and left it unresolved pending that.
Ticket: still unlinked — branch ENG-971 returns 404/401 from JIRA per the lint context; unable to verify against a written requirement. Not repeating this as a separate finding, just carrying the caveat forward.
Docs/hygiene: unaffected by this change — magic IDs documented, folder/naming/Last updated all fine, as noted previously.
No new findings beyond the one still-open item. Not yet safe to treat as final.
Generated by Analytics SQL Reviewer for #157 · auto · 24.2 AIC · ⌖ 5.54 AIC · ⊞ 14.7K
1 participant
No description provided.