Normalize employeeid, unit, and category on write in compliance tables - #1191
Normalize employeeid, unit, and category on write in compliance tables#1191labkey-martyp wants to merge 1 commit into
Conversation
employeeperunit was the only table in ehr_compliancedb with no trigger script, so its employeeid, unit, and category values were never canonicalized against their lookups - the reason the ONPRC report procedures had to wrap every unit and category comparison in lower(). Its schema metadata already declares all three columns as lookups, so the script is a drop-in copy of the ten siblings. sopdates had no trigger script either. sopid is deliberately left out of its lookupFields: the fk to sops is commented out in ehr_compliancedb.xml, and getLookupValue returns null for a column with no fk, which would reject every row. Pairs with the lower() removal in onprcEHRModules (PR 1859). Merge this first, or those reports will silently drop rows whose unit or category differs only by case. Existing rows carrying off-lookup values need a backfill before this deploys, since beforeUpdate revalidates the merged old row and will otherwise block edits that work today.
|
@labkey-martyp and @labkey-jeckels: the pattern outlined here probably need to occur in more places. Are there ways to do this that are more efficiency and wont require repeating a lot of boilerplate code? Some thoughts:
|
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
@bbimber Good point on the duplication. I have another PR that does something more like your first point. I will open that and close this one. It's combining the JS code into a reusable function in LDK.
Converting to a java trigger is probably the next step on this. Need to do more investigation into the scope of that trigger though.
To your third point, the EHR does have this feature, using the same LDK LookupValidationHelper only for registered fields - lookupValidationFields. It does not look very widely used though, and it probably should be.
A JS schema customizer sounds interesting but I'm not aware of any current implementation that matches that.
|
Closing in favor of #1199, which carries these same two trigger scripts — |
…loop (#1199) ## Rationale `employeeperunit` and `sopdates` were the only tables in `ehr_compliancedb` without trigger scripts, so their lookup values were never canonicalized on write, and this branch both adds those scripts and replaces the normalization loop that every other script in the schema had copied verbatim. The missing canonicalization is why the ONPRC report procedures had to wrap every unit and category comparison in `lower()`; that `lower()` removal has already merged, so those reports are exposed to case drift until this deploys. Existing rows carrying off-lookup values need a backfill before this deploys. The scripts merge the old row into the new one before validating, so a stored value that no longer matches its lookup will block edits that work today. ## Related Pull Requests - LabKey/LabDevKitModules#310 — adds the shared helper this depends on, and must merge first. - LabKey/onprcEHRModules#1885 — the same conversion in the ONPRC modules. - LabKey/onprcEHRModules#1859 — removed the `lower()` calls these triggers replace. - #1191 — superseded by this branch, which carries the same two new scripts written against the shared helper. ## Changes - Adds trigger scripts for `employeeperunit` and `sopdates`, normalizing the lookup columns their schema metadata already declares. - Leaves `sopdates.sopid` out deliberately: its fk to `sops` is commented out, and a lookup against a column with no fk would reject every row. - Converts the six existing `ehr_compliancedb` scripts to the shared helper, with field lists and error text unchanged so behavior is identical.
2 participants
Rationale
The ported PostgreSQL report procedures in ONPRC's compliance module wrapped every unit and category comparison in
lower()to emulate the case-insensitive default collation the MS SQL originals relied on implicitly. All of those comparisons joinemployeeperunitagainstrequirementspercategory, and the latter already canonicalizesunitandcategoryon write through its trigger script — soemployeeperunit, the only table inehr_compliancedbwithout one, was the sole source of case drift. Normalizing on write instead removes the need forlower()downstream and keeps the joins indexable.Related Pull Requests
lower()calls this change makes unnecessary. Merge this PR first, or those reports will silently drop rows whose unit or category differs only by case.Changes
employeeperunit.js, normalizingemployeeid,unit, andcategoryagainst the lookups its schema metadata already declares.sopdates.js, normalizingemployeeid.sopidis excluded deliberately — its fk tosopsis commented out inehr_compliancedb.xml, andgetLookupValuereturns null for a column with no fk, which would reject every row.Tasks 📍