Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-opus-4-7`

Stats: 1 user messages, 12 tool calls, tools used: Bash, Read, Grep.

Conversation

Turn 1 — User

Review the following changes from the FHIR-2209 branch in the Doctolib monorepo. The goal of these changes is to fix patient INS env transformation in DmpNhrAdapter.toSearchProps — the DMP API requires a T suffix on patient INS in non-production environments, and the old code was passing the raw INS without that suffix.

Focus your review on these two files only: - packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts - packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts

Here is the full diff:

diff --git a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts
index 9c545a6f0802..674ff416f9de 100644
--- a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts
+++ b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts
@@ -1,7 +1,21 @@
+import { getPatientInsForDMP } from '@doctolib/dmp'
 import type { DmpApi, DmpDocumentContent, SearchResult as DmpSearchResult } from '@doctolib/dmp'
 import type { NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records'
 import { DmpNhrAdapter } from './DmpNhrAdapter'

+jest.mock('@doctolib/dmp', () => ({
+  getPatientInsForDMP: jest.fn(),
+  DOCUMENT_STATUS: { validated: 1, obsolete: 2, archived: 4 },
+  DOCUMENT_VISIBILITY: {
+    sharedWithAll: 1,
+    notSharedWithPatient: 2,
+    notSharedWithPractitioner: 4,
+    notSharedWithLegalRepresentative: 8,
+    sharedOnlyWithPractitioner: 10,
+    sharedOnlyWithPatient: 12,
+  },
+}))
+
 const createMockDmpApi = (
   searchResult: DmpSearchResult = { documents: [] },
   contentResult: DmpDocumentContent[] = []
@@ -14,8 +28,6 @@ const createMockDmpApi = (
 const searchProps: NhrDocumentSearchProps = {
   patient: { nationalHealthIdentifier: 'INS-123' },
   params: {
-    // @ts-expect-error NhrDocumentSearchProps is changed in another PR
-    patientInformation: { nationalHealthIdentifier: 'INS-123' },
     categories: ['11488-4'],
     formats: ['pdf'],
     status: 'validated',
@@ -64,6 +76,11 @@ const dmpSearchResult: DmpSearchResult = {
 }

 describe('DmpNhrAdapter', () => {
+  beforeEach(() => {
+    jest.clearAllMocks()
+    jest.mocked(getPatientInsForDMP).mockReturnValue('INS-123T')
+  })
+
   describe('searchDocuments', () => {
     it('should call dmpApi.search with correctly transformed SearchProps', async () => {
       const mockDmpApi = createMockDmpApi({ documents: [] }, [])
@@ -72,7 +89,7 @@ describe('DmpNhrAdapter', () => {
       await adapter.searchDocuments(searchProps)

       expect(mockDmpApi.search).toHaveBeenCalledWith({
-        ins: 'INS-123',
+        ins: 'INS-123T',
         categories: { '#0': '11488-4' },
         formats: { '#0': 'pdf' },
         status: 1,
diff --git a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
index 5da15cc65cfe..d3716de1f901 100644
--- a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
+++ b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
@@ -6,9 +6,10 @@ import type {
   SearchProps,
   SearchResult as DmpSearchResult,
 } from '@doctolib/dmp'
-import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp'
+import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp'
 import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody'
 import { isStructured } from '@doctolib/cda/types/StructuredBody'
+import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
 import type {
   NhrAdapter,
   NhrDocumentAuthor,
@@ -47,11 +48,10 @@ function toIndexedRecord(values: string[]): Record<string, string> | undefined {
 }

 function toSearchProps(props: NhrDocumentSearchProps): Search

…(truncated, 1490 more chars)

Turn 1 — Assistant

I'll review the changes in the FHIR-2209 branch focusing on the patient INS env transformation fix.

Tools used:

• Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/business/adapters/

Tool results:

→ result (ok): DmpNhrAdapter.test.ts DmpNhrAdapter.ts

Tools used:

• Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts

Tool results:

→ result (ok): 1 import type { 2 DmpApi, 3 DmpAuthor, 4 DmpDocument, 5 DmpDocumentContent, 6 SearchProps, 7 SearchResult as DmpSearchResult, 8 } from '@doctolib/dmp' 9 import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp' 10 import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody' 11 import { isStructured } from '@doctolib/cda/types/StructuredBody' 12 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 13 import type { 14 Nh …(truncated, 5730 more chars)

Tools used:

• Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts

Tool results:

→ result (ok): 1 import { getPatientInsForDMP } from '@doctolib/dmp' 2 import type { DmpApi, DmpDocumentContent, SearchResult as DmpSearchResult } from '@doctolib/dmp' 3 import type { NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records' 4 import { DmpNhrAdapter } from './DmpNhrAdapter' 5
6 jest.mock('@doctolib/dmp', () => ({ 7 getPatientInsForDMP: jest.fn(), 8 DOCUMENT_STATUS: { validated: 1, obsolete: 2, archived: 4 }, 9 DOCUMENT_VISIBILITY: { 10 sharedWithAll: 1, 11 notSharedW …(truncated, 12025 more chars)

Now let me check a couple of related files to understand how the getPatientInsForDMP function is used elsewhere and the structure of Patient/NhrDocumentSearchProps.

Tools used:

• Grep: export function getPatientInsForDMP|export const getPatientInsForDMP in /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/dmp

Tool results:

→ result (ok): packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts:53:export const getPatientInsForDMP = (patient: Patient | undefined): string => {

Tools used:

• Grep: getPatientInsForDMP in /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/dmp

Tool results:

→ result (ok): packages/@doctolib/dmp/src/index.ts:89: getPatientInsForDMP, packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts:53:export const getPatientInsForDMP = (patient: Patient | undefined): string => { packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts:135: s_ins: getPatientInsForDMP(patient), packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts:168: s_ins: getPatientInsForDMP(patient), packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts:206: s_ins: getPatientInsFo …(truncated, 319 more chars)

Tools used:

• Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts (40–70)

Tool results:

→ result (ok): 40 referenceIds?: DmpReferenceId[] 41 eventCodes?: DmpEventCode[] 42 getTransactionFrames?: number 43 } 44
45 type IcanopeeCdaIdentifier = { 46 s_documentOid?: string 47 s_documentSetIdRoot: string 48 s_documentSetIdExtension?: string 49 s_documentVersionNumber: string 50 s_replacedDocumentUniqueId: string | undefined 51 } 52
53 export const getPatientInsForDMP = (patient: Patient | undefined): string => { 54 const insSuffix = envIsProduction() ? '' : 'T' 55 return patient …(truncated, 376 more chars)

Now I have crucial information. The function getPatientInsForDMP accesses patient.ins but the adapter passes an object with ins: patient?.nationalHealthIdentifier. Let me verify this.

Looking at line 55: patient ? \${patient.ins}${insSuffix}` : ''`

The function truthy-checks the entire patient object. The adapter code creates { ins: patient?.nationalHealthIdentifier } and casts it. Let me consider edge cases.

When patient is undefined: - patient?.nationalHealthIdentifier -> undefined - Object passed: { ins: undefined } (still truthy as an object) - getPatientInsForDMP receives truthy object, returns ${undefined}${insSuffix} → "undefinedT" in non-prod, "undefined" in prod

That's a bug. Let me verify the tests aren't covering this. Looking at the test, patient: { nationalHealthIdentifier: 'INS-123' } is always provided, and getPatientInsForDMP is mocked to always return 'INS-123T', which masks this bug.

Let me check the NhrDocumentSearchProps type.

Tools used:

• Grep: NhrDocumentSearchProps in /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/tpf-national-health-records

Tool results:

→ result (ERROR): <tool_use_error>Path does not exist: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/tpf-national-health-records. Note: your current working directory is /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation.</tool_use_error>

Tools used:

• Grep: NhrDocumentSearchProps

Tool results:

→ result (ok): Found 3 files packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts packages/@doctolib/national-health-records/src/hooks/useNationalHealthRecords.ts

Tools used:

• Grep: NhrDocumentSearchProps|type NhrDocumentSearchProps|interface NhrDocumentSearchProps in /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/hooks/useNationalHealthRecords.ts

Tool results:

→ result (ok): 1:import type { NhrDocumentSearchProps, NhrDocumentSearchResult } from '@doctolib/tpf-national-health-records' 7: searchDocuments: (props: NhrDocumentSearchProps) => Promise<NhrDocumentSearchResult>

Tools used:

• Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation -path '*/tpf-national-health-records*' -name '*.ts' -not -path '*/node_modules/*' 2>/dev/null | …(truncated, 9 more chars)

Tool results:

→ result (ok): (Bash completed with no output)

Tools used:

• Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation -name 'tpf-national-health-records' -type d -not -path '*/node_modules/*' 2>/dev/null

Tool results:

→ result (ok): (Bash completed with no output)

The package is external (not in the monorepo at that path). Let's confirm based on the context provided: the user said NhrDocumentSearchProps.patient is typed as optional (patient?). Let me also check the runtime behavior of getPatientInsForDMP when patient contains { ins: undefined }.

Tools used:

• Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts (1–57)

Tool results:

→ result (ok): 1 import { envIsProduction } from '@doctolib/variation-management/environment' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import dayjs from 'dayjs' 4 import type { CdaIdentifierFr } from '@doctolib/tpf-cda' 5 import type { 6 SharingSettings, 7 FindDocumentsRequest, 8 SendDocumentRequest, 9 DeleteDocumentRequest, 10 GetAuthentificationDmpStatusForPatientRequest, 11 GetDocumentsContentRequest, 12 GetDocumentsContentParams, 13 ReferenceId, 14 E …(truncated, 1460 more chars)

Now I have all the context I need. Let me also look at how other DMP payload builders handle their patient parameter - do they ever pass undefined? In contrast, this adapter creates a synthetic object that is ALWAYS truthy.

Here's the code review.

Code Review: FHIR-2209 Patient INS Env Transformation Fix

Files reviewed: - /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts - /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts

Also consulted: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation/packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts (for getPatientInsForDMP semantics).

Critical (90-100)

1. toSearchProps produces "undefinedT" / "undefined" when patient is missing — confidence 95

Location: DmpNhrAdapter.ts:54

ins: getPatientInsForDMP({ ins: patient?.nationalHealthIdentifier } as unknown as Patient),

NhrDocumentSearchProps.patient is optional (patient?), and getPatientInsForDMP is implemented as:

export const getPatientInsForDMP = (patient: Patient | undefined): string => {
  const insSuffix = envIsProduction() ? '' : 'T'
  return patient ? `${patient.ins}${insSuffix}` : ''
}

It guards on the entire patient object being truthy — not on patient.ins. By always passing an object literal { ins: patient?.nationalHealthIdentifier }, the guard never triggers even when the caller has no patient. The result:

• Non-prod, no patient: ins ends up as the string "undefinedT" sent to the DMP API.
• Prod, no patient: ins ends up as the string "undefined".

Both are hidden data leaks to the external DMP service (malformed request rather than a clear no-op / error), and they diverge from the empty-string semantics the helper intends for "missing patient". The pre-change behavior — params.patientInformation.nationalHealthIdentifier — would have been undefined directly, which although also wrong, was at least identifiable; wrapping it in a string is strictly worse.

Fix suggestion: Short-circuit when the INS is missing so the helper can do its real "no patient" path. For example:

ins: patient?.nationalHealthIdentifier
  ? getPatientInsForDMP({ ins: patient.nationalHealthIdentifier } as unknown as Patient)
  : '',

Or, if an absent patient is considered a programmer error here (the sibling getDocumentsContent already throws 'Patient is mandatory for DMP'), mirror that pattern in toSearchProps / searchDocuments and keep the types honest:

if (!patient?.nationalHealthIdentifier) {
  throw new Error('Patient is mandatory for DMP')
}

Either option also removes the need for the as unknown as Patient cast — you can construct the minimal object inline after the guard.

Important (80-89)

2. No test covering the new optional-patient branch — confidence 85

Location: DmpNhrAdapter.test.ts:78-146

The sole searchDocuments assertion uses a searchProps fixture that always carries patient: { nationalHealthIdentifier: 'INS-123' }, and the global beforeEach unconditionally stubs getPatientInsForDMP to return 'INS-123T'. That mock swallows any change to the arguments passed to getPatientInsForDMP, including the bug above.

At minimum, two tests should be added to lock the contract:

1. Assert the argument getPatientInsForDMP was called with (e.g. expect(getPatientInsForDMP).toHaveBeenCalledWith({ ins: 'INS-123' }) — this protects against later regressions where someone changes what gets passed in).
2. A test for patient absent / nationalHealthIdentifier absent, verifying the agreed behavior (throw, or pass undefined and get '').

Without assertion (1), the mock completely bypasses the function under test — the test would still pass even if toSearchProps passed literally nothing to getPatientInsForDMP.

3. getDocumentsContent was not updated and now uses raw INS without the T suffix — confidence 88

Location: DmpNhrAdapter.ts:163

const result = await this.dmpApi.getDocumentsContentByUniqueId({
  ins: patient.nationalHealthIdentifier,   // <-- still raw, no env suffix
  ...
})

The PR description says "DMP API requires a T suffix on patient INS in non-production environments". That constraint is a property of the DMP API, not of a single call site — yet only toSearchProps was fixed. getDocumentsContent in the same file sends the raw nationalHealthIdentifier to getDocumentsContentByUniqueId, which will fail in staging/non-prod for the same reason the search call did pre-fix. The test at line 199-204 even hard-codes ins: 'INS-123' (no T), freezing in the inconsistency.

If the scope of FHIR-2209 is only "fix search", add a TODO / follow-up ticket reference and leave a comment; otherwise apply the same getPatientInsForDMP transformation here and update the test to expect the suffixed value. The two call sites using the same API should agree on INS format.

4. Duplicated categories key in the diff output — confidence 80 (verify against the actual file)

The diff you pasted shows:

-    ins: params.patientInformation.nationalHealthIdentifier,
-    categories: toIndexedRecord(params.categories ?? []),
+    ins: getPatientInsForDMP({ ins: patient?.nationalHealthIdentifier } as unknown as Patient),
+    categories: toIndexedRecord(params.categories ?? []),
     categories: toIndexedRecord(params.categories ?? []),

Two + lines with the same categories: key appear back-to-back. The actual file on disk only has one (verified by reading DmpNhrAdapter.ts:55), so this is almost certainly a diff-rendering artifact in the review description and not a real duplication — but worth double-checking the final committed file, because a duplicated key is a silent TypeScript/lint-clean footgun: the second assignment simply overwrites the first.

Notes on things that are fine (not reportable, but worth flagging)

• The as unknown as Patient cast is necessary given Patient's many required fields and the helper's narrow patient.ins usage; acceptable as documented in the PR description. Once fix #1 is applied, the cast only happens on the happy path (valid INS present), which is less risky.
• The manual inline mock of @doctolib/dmp constants (DOCUMENT_STATUS, DOCUMENT_VISIBILITY) is a maintenance hazard — if FrDmpConstants.ts ever gains a new value, these tests silently diverge. A lightweight defense would be a small assertion in the mock factory that the keys match Object.keys(actualConstants), but since jest.requireActual is intentionally avoided (per the PR description), the current approach is defensible. Consider adding a comment in the test file explaining why requireActual is avoided so future contributors don't "clean it up".
• jest.mocked(getPatientInsForDMP).mockReturnValue('INS-123T') in beforeEach is fine, but combined with gap #2 it's load-bearing — without argument assertions, it hides a lot.

Summary

The core idea — reusing getPatientInsForDMP so DMP calls from this adapter share the same env-suffix logic as the rest of @doctolib/dmp — is correct and consistent with existing codebase patterns. The implementation has one real correctness bug (#1) that escapes through a mocked test (#2), plus an incompleteness (#3) that leaves a second call site diverging from the new convention. Fix #1 and #3 together, and strengthen tests per #2, and this is ready.