Skip to content

fix(displayId): TAMOC-401: Hotfix 2.45: Corrupted snapshot records changelog value created in resolveDuplicatedPatientIds hook #9261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dannash100 wants to merge 4 commits into
base: release/2.45
Choose a base branch
from
Open

fix(displayId): TAMOC-401: Hotfix 2.45: Corrupted snapshot records changelog value created in resolveDuplicatedPatientIds hook #9261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6da95e6
fix
dannash100 Mar 2, 2026
2ada215
Merge branch 'release/2.45' into fix/remove-duplicate-corrupt-changel…
dannash100 Mar 2, 2026
5ba547b
create an actual change log
dannash100 Mar 3, 2026
3497067
add tests
dannash100 Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 182 additions & 0 deletions packages/central-server/__tests__/sync/resolveDuplicatedPatientDisplayIds.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
import { FACT_CURRENT_SYNC_TICK } from '@tamanu/constants/facts';
import { SYNC_SESSION_DIRECTION } from '@tamanu/database/sync';
import { fake } from '@tamanu/fake-data/fake';
import { SYSTEM_USER_UUID } from '@tamanu/constants';

import {
createTestContext,
waitForSession,
waitForPushCompleted,
initializeCentralSyncManagerWithContext,
} from '../utilities';

const DEFAULT_MAX_RECORDS_PER_SNAPSHOT_CHUNKS = 100000000;

describe('resolveDuplicatedPatientDisplayIds', () => {
let ctx;
let models;

const initializeCentralSyncManager = config =>
initializeCentralSyncManagerWithContext(ctx, config);

beforeAll(async () => {
ctx = await createTestContext();
({ models } = ctx.store);
});

beforeEach(async () => {
await models.LocalSystemFact.set(FACT_CURRENT_SYNC_TICK, 2);
await models.SyncLookupTick.truncate({ force: true });
await models.SyncDeviceTick.truncate({ force: true });
await models.Facility.truncate({ cascade: true, force: true });
await models.ReferenceData.truncate({ cascade: true, force: true });
await models.User.truncate({ cascade: true, force: true });
await models.User.create({
id: SYSTEM_USER_UUID,
email: 'system',
displayName: 'System',
role: 'system',
});
await models.Setting.set('audit.changes.enabled', true);
await models.SyncLookup.truncate({ force: true });
await models.DebugLog.truncate({ force: true });
});

afterAll(() => ctx.close());

const pushChangesToCentral = async (changes, facilityId) => {
const centralSyncManager = initializeCentralSyncManager({
sync: {
lookupTable: { enabled: true },
maxRecordsPerSnapshotChunk: DEFAULT_MAX_RECORDS_PER_SNAPSHOT_CHUNKS,
},
});
await centralSyncManager.updateLookupTable();
const { sessionId } = await centralSyncManager.startSession();
await waitForSession(centralSyncManager, sessionId);
await centralSyncManager.addIncomingChanges(sessionId, changes);
await centralSyncManager.completePush(sessionId, facilityId);
await waitForPushCompleted(centralSyncManager, sessionId);
return { centralSyncManager, sessionId };
};

it('should rename both patients when displayId collides during sync push', async () => {
const facility = await models.Facility.create(fake(models.Facility));
const existingPatient = await models.Patient.create(
fake(models.Patient, { displayId: 'DUP-001' }),
);
await models.PatientFacility.create({
id: models.PatientFacility.generateId(),
patientId: existingPatient.id,
facilityId: facility.id,
});

const incomingPatientData = fake(models.Patient, { displayId: 'DUP-001' });
const changes = [
{
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: false,
recordType: 'patients',
recordId: incomingPatientData.id,
data: incomingPatientData,
},
];

await models.LocalSystemFact.set(FACT_CURRENT_SYNC_TICK, 15);
await pushChangesToCentral(changes, facility.id);

const updatedExisting = await models.Patient.findByPk(existingPatient.id);
const newPatient = await models.Patient.findByPk(incomingPatientData.id);

expect(updatedExisting.displayId).toBe('DUP-001_duplicate_1');
expect(newPatient.displayId).toBe('DUP-001_duplicate_2');
});

it('should create a changelog record for the existing patient rename', async () => {
const facility = await models.Facility.create(fake(models.Facility));
const existingPatient = await models.Patient.create(
fake(models.Patient, { displayId: 'DUP-002' }),
);
await models.PatientFacility.create({
id: models.PatientFacility.generateId(),
patientId: existingPatient.id,
facilityId: facility.id,
});

const incomingPatientData = fake(models.Patient, { displayId: 'DUP-002' });
const changes = [
{
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: false,
recordType: 'patients',
recordId: incomingPatientData.id,
data: incomingPatientData,
},
];

await models.LocalSystemFact.set(FACT_CURRENT_SYNC_TICK, 15);
await pushChangesToCentral(changes, facility.id);

const changelog = await models.ChangeLog.findOne({
where: {
recordId: existingPatient.id,
tableName: 'patients',
reason: 'Automated: duplicate displayId resolution during sync',
},
});

expect(changelog).not.toBeNull();
expect(changelog.updatedByUserId).toBe(SYSTEM_USER_UUID);
expect(changelog.recordData.displayId).toBe('DUP-002_duplicate_1');
});

it('should not modify patients when there is no displayId collision', async () => {
const facility = await models.Facility.create(fake(models.Facility));
await models.Patient.create(fake(models.Patient, { displayId: 'UNIQUE-001' }));

const incomingPatientData = fake(models.Patient, { displayId: 'UNIQUE-002' });
const changes = [
{
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: false,
recordType: 'patients',
recordId: incomingPatientData.id,
data: incomingPatientData,
},
];

await models.LocalSystemFact.set(FACT_CURRENT_SYNC_TICK, 15);
await pushChangesToCentral(changes, facility.id);

const newPatient = await models.Patient.findByPk(incomingPatientData.id);
expect(newPatient.displayId).toBe('UNIQUE-002');
});

it('should not treat a patient syncing back its own record as a duplicate', async () => {
const facility = await models.Facility.create(fake(models.Facility));
const existingPatient = await models.Patient.create(
fake(models.Patient, { displayId: 'SAME-001' }),
);
await models.PatientFacility.create({
id: models.PatientFacility.generateId(),
patientId: existingPatient.id,
facilityId: facility.id,
});

const changes = [
{
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: false,
recordType: 'patients',
recordId: existingPatient.id,
data: existingPatient.get({ plain: true }),
},
];

await models.LocalSystemFact.set(FACT_CURRENT_SYNC_TICK, 15);
await pushChangesToCentral(changes, facility.id);

const patient = await models.Patient.findByPk(existingPatient.id);
expect(patient.displayId).toBe('SAME-001');
});
});
75 changes: 63 additions & 12 deletions packages/database/src/sync/resolveDuplicatedPatientDisplayIds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { Op } from 'sequelize';
import { Op, QueryTypes } from 'sequelize';
import { randomUUID } from 'node:crypto';
import { SYSTEM_USER_UUID } from '@tamanu/constants';
import { FACT_DEVICE_ID, FACT_CURRENT_VERSION } from '@tamanu/constants/facts';
import type { SyncHookSnapshotChanges, SyncSnapshotAttributes } from 'types/sync';
import type { Patient } from 'models';
import { SYNC_SESSION_DIRECTION } from './constants';
Expand Down Expand Up @@ -34,24 +37,72 @@ export const resolveDuplicatedPatientDisplayIds = async (
);

if (duplicatedDisplayIds.length > 0) {
const sequelize = PatientModel.sequelize!;
const [tableOidResult] = await sequelize.query<{ oid: number }>(
`SELECT oid FROM pg_class WHERE relname = 'patients' AND relnamespace = 'public'::regnamespace`,
{ type: QueryTypes.SELECT },
);
const tableOid = tableOidResult!.oid;
const [deviceIdFact, versionFact] = await Promise.all([
sequelize.query<{ value: string }>(
`SELECT value FROM local_system_facts WHERE key = :key`,
{ replacements: { key: FACT_DEVICE_ID }, type: QueryTypes.SELECT },
),
sequelize.query<{ value: string }>(
`SELECT value FROM local_system_facts WHERE key = :key`,
{ replacements: { key: FACT_CURRENT_VERSION }, type: QueryTypes.SELECT },
),
]);
const deviceId = deviceIdFact[0]?.value ?? 'unknown';
const version = versionFact[0]?.value ?? 'unknown';

// Create a new incoming snapshot change for the existing patient's display ID and append '_duplicate_1'
const updatedExistingPatientSnapshotRecords = existingPatientsWithDuplicatedDisplayIds.map(
(r) => ({
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: !!r.deletedAt,
recordType: PatientModel.tableName,
recordId: r.id,
data: { ...sanitizeRecord(r), displayId: `${r.displayId}_duplicate_1` },
}),
(r) => {
const newData = { ...sanitizeRecord(r), displayId: `${r.displayId}_duplicate_1` };
const now = new Date();
return {
direction: SYNC_SESSION_DIRECTION.INCOMING,
isDeleted: !!r.deletedAt,
recordType: PatientModel.tableName,
recordId: r.id,
data: newData,
changelogRecords: [
{
id: randomUUID(),
tableOid,
tableSchema: 'public',
tableName: PatientModel.tableName,
loggedAt: now,
updatedByUserId: SYSTEM_USER_UUID,
recordId: r.id,
recordCreatedAt: r.createdAt,
recordUpdatedAt: now,
recordDeletedAt: r.deletedAt ?? null,
recordData: newData,
deviceId,
version,
reason: 'Automated: duplicate displayId resolution during sync',
migrationContext: null,
},
],
};
},
);

// Update the to-be-synced patient's display ID to append '_duplicate_2'
// Strip changelogRecords so updateSnapshotRecords doesn't overwrite the column;
// the original value in the DB row (pushed by the facility) is preserved
const updatedIncomingPatientSnapshotRecords = changes
.filter((c) => !c.isDeleted && duplicatedDisplayIds.includes(c.data.displayId))
.map((c) => ({
...c,
data: { ...c.data, displayId: `${c.data.displayId}_duplicate_2` },
}));
.map((c) => {
const updated = {
...c,
data: { ...c.data, displayId: `${c.data.displayId}_duplicate_2` },
};
delete (updated as any).changelogRecords;
return updated;
});
Comment on lines +98 to +105
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While the current implementation is correct, you can achieve the same result more declaratively by using object destructuring to omit the changelogRecords property. This avoids mutating the updated object with delete and is generally considered a cleaner, more functional approach.

      .map((c) => {
        const { changelogRecords, ...rest } = c as any;
        return {
          ...rest,
          data: { ...c.data, displayId: `${c.data.displayId}_duplicate_2` },
        };
      });


return {
inserts: updatedExistingPatientSnapshotRecords,
Expand Down
Loading