feat(sensitiveNetworks): TAM-6888: scope sync lookup and outgoing snapshots by sensitive network - #10944
feat(sensitiveNetworks): TAM-6888: scope sync lookup and outgoing snapshots by sensitive network#10944chris-bes wants to merge 22 commits into
Conversation
chris-bes
changed the title
chris-bes
changed the title
dd6b8d1 to
64e3ab2
Compare
…resh dbt models sync_lookup.sensitive_network_id is a uuid column, but the model declared it STRING. Generated test data then produced values like "SyncLookup.sensitiveNetworkId.<uuid>", which Postgres rejects, failing snapshotOutgoingChanges.syncLookup tests at insert. Also regenerates the dbt source models for the network schema: uuid for the id and both sensitive_network_id columns, text for code and name, the not_null tests on the network timestamps, and the trigger list on sensitive_networks. Both issues come from the sensitive network schema on the base branch rather than from this card's snapshot changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Making SyncLookup.sensitiveNetworkId a UUID stopped generated rows failing the column's type, but fake() then filled it with a random network id. The outgoing snapshot admits a row only when facility_id and sensitive_network_id are both null, so every faked lookup row became scoped to a network no facility belongs to and was withheld - failing six snapshotOutgoingChanges cases that set facility_id null to mean unscoped. Default the column to null in MODEL_SPECIFIC_OVERRIDES rather than adding it to each fake() call; tests that want a network still pass one, since test-specific overrides merge last. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sensitive network ids became string ids rather than uuids, so the three columns carrying them are character varying again. Points the dbt source models back at that, and restores SyncLookup.sensitiveNetworkId to STRING alongside facilityId. The UUID declaration was only ever there to stop generated test data failing the column's type; the fake-data default that leaves a lookup row unscoped is what actually fixes that, and it holds whatever the column type is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
| ), | ||
| ${patientId || 'NULL'}, | ||
| ${facilityId || 'NULL'}, | ||
| ${sensitiveNetworkId || 'NULL'}, |
There was a problem hiding this comment.
[Design & Architecture] suggestion
This adds a second UUID/string column that is matched by position to the INSERT column list in packages/central-server/app/sync/updateLookupTable.js, across two packages, immediately adjacent to the column it is most likely to be confused with (facility_id). The PR's own plan names this as a failure mode: transposing the two compiles, runs, and scopes every sensitive record to a facility id that is really a network id. Now is a good moment to remove the coupling rather than widen it — export the ordered column list from the database package and have buildLookupUpsertQuery build its INSERT list (and its ON CONFLICT DO UPDATE list) from it, so order and membership have one definition.
| // population logic it is mirroring — Notification included, whose joins reach encounters through | ||
| // its metadata. | ||
| const getEncounterScopedRecordTypes = async (query: QueryInterface) => { | ||
| const models = getModelsForPull(query.sequelize.models as any); |
There was a problem hiding this comment.
[Design & Architecture] suggestion
The migration derives the tables it rewrites from the live model registry (getModelsForPull + buildSyncLookupQueryDetails) rather than from a pinned list. A migration is a fixed point in the schema's history, but this one's effect changes every time a model is added, renamed, or has its lookup joins reworked — a deployment upgrading six months from now rescopes a different set of record_types than a deployment upgrading today, and the two DBs silently diverge. It also couples a migration file to application query-building code (buildSyncLookupQueryDetails even hits the DB per model via LocalSystemFact.isLookupRebuildingModel, ~50 round-trips, just to read joins), so any future refactor of that helper can break an already-shipped migration. The immediate precedent, 1785372544730-RebuildLookupTableForSensitiveFacilityScopedModels.ts, hardcodes MODELS_TO_REBUILD for exactly this reason. Prefer generating the list once and inlining it as a literal, with the drift guarded by the existing registry test rather than at migration time.
| for (const model of Object.values<any>(models)) { | ||
| const { joins } = (await model.buildSyncLookupQueryDetails({})) ?? {}; | ||
| if (model.tableName === 'encounters' || /JOIN\s+encounters\b/.test(joins ?? '')) { | ||
| recordTypes.push(model.tableName); |
There was a problem hiding this comment.
[Design & Architecture] suggestion
The "is this model encounter-linked" predicate (tableName === 'encounters' || /JOIN\s+encounters\b/.test(joins)) is now duplicated verbatim between this migration and packages/database/__tests__/sync/syncLookupFacilityScope.test.ts. It is load-bearing in both places — the test uses it to decide what must be network-scoped, the migration uses it to decide what to rewrite — and the two are meant to be the same set by construction, yet nothing ties them together. If the regex has to change (e.g. a model joins encounters via a CTE or an alias), one copy will be updated and the other won't, and the failure is silent in the migration. Extract a single exported helper next to the lookup builders and have both call it.
| facilityIds, | ||
| deviceId, | ||
| {}, // sending empty session config because this snapshot attempt is only for syncing new marked for sync patients | ||
| // only the network scoping carries over; this snapshot is just for newly marked for |
There was a problem hiding this comment.
[Integration tests] suggestion
The newly-marked-for-sync-patients snapshot now passes { sensitiveNetworkIds } as its whole session config, but no test exercises that pass with a networked facility. Every test in CentralSyncManager.sensitiveFacilities.test.js (including the new sharing within a network block) creates no PatientFacility rows for the network facilities, so newPatientFacilitiesCount is 0 and this snapshotOutgoingChanges call inserts nothing — the coverage all lands on the regular/incremental call below. If the network ids were ever dropped from this config, a facility that has just marked a patient for sync would silently miss that patient's entire network history on the full-sync pass, and the suite would stay green. The PR's own test-case list flags this (A facility receives its network's data only for patients it marks for sync is unticked). Add a case in the new sharing within a network describe that creates a PatientFacility for the sibling with updatedAtSyncTick > since, pulls with since below that tick, and asserts the network's encounters arrive on the full-sync pass.
| export async function down(query: QueryInterface): Promise<void> { | ||
| if (!(await hasNetworkedFacility(query))) return; | ||
|
|
||
| const encounterScopedRecordTypes = await getEncounterScopedRecordTypes(query); |
There was a problem hiding this comment.
[BES Requirements] critical
down cannot restore rows belonging to a multi-member network (the HAVING COUNT(*) = 1 filter skips them), and leaves them with facility_id = NULL and sensitive_network_id set. Under the reverted code the snapshot clause is back to facility_id IS NULL meaning "unscoped" — so every one of those confidential encounter rows is admitted to every facility until the lookup happens to be rebuilt. That is a confidentiality regression on rollback, not just an incomplete restore. At minimum down should also SET sensitive_network_id = NULL only where it successfully restores a facility, and NULL out nothing otherwise — or flag those models for a lookup rebuild so they re-materialise under the old logic immediately. Per packages/database/CLAUDE.md this down also needs a // DESTRUCTIVE: comment spelling out what is not restored; the current prose comment doesn't use the marker and understates the consequence ("the old population logic would rebuild them" only happens on the next write to each source record).
|
🦸 Review Hero Summary (round 1) Below consensus threshold (7 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
1 participant
Changes
Add a brief description of the changes in this PR to help give the reviewer context.
Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...