Conversation
…ership at creation Registers sensitiveNetwork as an importable reference data type with its own sheet, adds a sensitiveNetworkId column to the facility sheet, and refuses any write that changes an existing facility's network. The import column matches what the reference data export writes, so an export/edit/import round trip preserves membership rather than silently dropping it. The guard is a Facility instance validator, so it covers the reference data import and provisioning's facilities block while leaving the schema card's SQL backfill and incoming sync alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ility models/index.ts re-exports each model file wholesale and initDatabase treats every export as a model class, testing `'initModel' in modelClass`. A string export made that `in` throw, so database init failed for every suite, migration run and dbt model generation. The message is now module-local and tests assert its text directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ble cases SensitiveNetwork.id is a UUID column, so the new importer fixtures' readable slugs were rejected by Postgres. They now use well-formed UUIDs. Removes two edge cases from CentralSyncManager.sensitiveFacilities that drove a facility into and out of a network through the model. Both describe transitions the membership guard refuses and the spec makes unreachable; the migration path that can still change membership is covered by the lookup-rescope tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the switch of sensitive_networks.id from uuid to the standard string primary key. Updates the generated data_type on sensitive_networks.id, facilities.sensitive_network_id and sync_lookup.sensitive_network_id to match the live schema, which the dbt-model check compares against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
| field: id | ||
| - name: sensitive_network_id | ||
| data_type: uuid | ||
| data_type: character varying(255) |
There was a problem hiding this comment.
[BES Requirements] suggestion
The dbt model is corrected to character varying(255) here (matching the DDL, which uses DataTypes.STRING), but packages/database/src/models/SyncLookup.ts:38 still declares sensitiveNetworkId: { type: DataTypes.UUID }. Sequelize's UUID type validates on write, so any model-level write carrying a real network id — now readable strings like sensitiveNetwork-srh from the backfill and the new merge step, not UUIDs — fails with "is not a valid uuid". It only stays quiet today because lookup population writes raw SQL and the fake data defaults the column to null. Change the model attribute to DataTypes.STRING so schema, model and dbt agree.
| await query.sequelize.query(` | ||
| INSERT INTO sensitive_networks (id, code, name) | ||
| SELECT 'sensitiveNetwork-' || code, code, name | ||
| SELECT 'sensitiveNetwork-' || regexp_replace(code, '[^A-Za-z0-9-]', '', 'g'), code, name |
There was a problem hiding this comment.
[BES Requirements] critical
Stripping disallowed characters out of the facility code to build the network id can collapse two distinct codes onto the same id (e.g. SRH.1 and SRH-1, or A/B and AB), and a code made only of punctuation yields the bare id sensitiveNetwork-. Both cases make the INSERT fail on the sensitive_networks primary key, which aborts the migration and blocks the whole upgrade for that deployment — and it fails only on deployments that happen to have such codes, so it won't show up in testing. Derive the id from the facility id (already a safe, unique key: 'sensitiveNetwork-' || facilities.id) rather than from a lossy transform of the code, and pair facilities to networks on that instead of on code.
| // writes it, and the Facility model refuses a membership change. A blank cell is already absent | ||
| // (sheet_to_json is called without defval), but an explicit empty string survives as '', so | ||
| // transform it back to undefined. Deliberately no default, for the same reason. | ||
| sensitiveNetworkId: yup.string().transform(value => (value === '' ? undefined : value)), |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
sensitiveNetworkId has no existence check before the insert, so a typo'd or unknown network id reaches Postgres and fails as a foreign-key violation rather than a row-level validation error. FOREIGN_KEY_SCHEMATA.Facility (referenceDataImporter/sheet.js) only resolves catchment, and the raw column name never matches findFieldName, so the value passes straight through to Model.create. Two consequences: the administrator sees a raw insert or update on table "facilities" violates foreign key constraint ... wrapped in an UpsertionError instead of the "valid foreign key expected in column ..." message every other FK column produces; and because a constraint violation aborts the surrounding Postgres transaction, every row processed after it in the same import fails with current transaction is aborted, burying the row that actually caused it. The new test only asserts errors.length > 0, so it passes either way. Add an existence check for sensitiveNetworkId in validateTableRows.js (it already loads existing rows and pushes errors with the sheet row number) — this is the fallback the plan left undecided, and the error message is the reason to add it.
| }, | ||
|
|
||
| facility: {}, | ||
| sensitiveNetwork: {}, |
There was a problem hiding this comment.
[Integration tests] suggestion
sensitiveNetwork now flows through POST /v1/admin/import/referenceData, but no test exercises the new type over HTTP with the real permission layer. sensitiveNetworkImporter.test.js calls importerTransaction directly with checkPermission: () => true, and the one permission test asserts only that a vi.fn() mock was called with ('create', 'SensitiveNetwork') — that proves the importer asks, not that a role can actually be granted it or that a user lacking it is refused. Given this gates enrolment into a confidential-data network, add a case in the style of referenceDataImporter.test.js:71 (ctx.baseApp.asRole('practitioner') → .post('/v1/admin/import/referenceData').attach('file', ...)) covering: permitted role succeeds, least-privilege user (asRole('base')) is forbidden. Same HTTP path is also where the membership-change refusal should be seen at least once, to confirm the InvalidOperationError from the Facility validator surfaces as a row-attributed error in the endpoint response rather than a bare 500.
|
🦸 Review Hero Summary (round 1) Below consensus threshold (9 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
|
Android builds 📱
|
|
View Live: https://app.pulumi.com/bes/tamanu-on-k8s/workhorse-t6/updates/5 Outputs: |
Backfill derives each network id from the facility id rather than a lossy transform of its code. Stripping the characters a code allows but an id does not could collapse two distinct codes onto one id, or yield a bare prefix for a code of pure punctuation, colliding on the primary key and failing the upgrade on just those deployments. SyncLookup.sensitiveNetworkId declared UUID after the column became a string, so any model-level write carrying a real network id would fail validation. Facility rows now check the network exists during schema validation. Reporting it in validateTableRows would not have been enough: that hook leaves the row in the upsert, so it would still have raised a foreign key violation, aborted the transaction, and buried the offending row under "current transaction is aborted" errors for every row after it. importRows puts models into the validation context so a schema can check against the database. Adds tests over POST /v1/admin/import/referenceData covering the permission layer and the refusal's error shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sensitive_networks holds code and name unique, but facilities holds neither — only facilities.id is unique. Copying a facility's code and name verbatim therefore failed the upgrade wherever two sensitive facilities shared one, such as a "Central Clinic" in two divisions, which is what the non-determinism check hit. Where a value repeats among the facilities being backfilled, the facility id now qualifies it, and an administrator can rename it through the reference data import afterwards. Corrects the spec alongside: it claimed facility codes and names are unique. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sensitive_networks is a syncable table, so its unique constraints have to be
DEFERRABLE INITIALLY IMMEDIATE for the sync-apply transaction to defer their
validation to its end — otherwise a batch only transiently in conflict, such as
two networks swapping codes, cannot be applied. They were created with
addIndex({ unique: true }), which makes a bare unique index, and Postgres
supports DEFERRABLE only on a constraint.
Regenerates the dbt model alongside: its generator reads constraint_type, so
promoting the indexes adds a unique test to code and name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Changes
Extends the sensitive networks spec with the rules for administering networks and guarding facility membership:
No code changes yet — this PR only updates the spec.
Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...