Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-opus-4-7

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

Conversation

Turn 1 — User

Review the following code changes for PR FHIR-2184 (fix: resolve EHR-eligible org server-side for medical device creation) against Doctolib code guidelines.

Context: The bug was that CreateDeviceButton picked the first admin org client-side without verifying EHR eligibility. The fix resolves the qualifying org server-side and passes it as a prop.

Changed files (worktree at /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184):

1. app/controllers/configuration/gdt_exports_controller.rb
class Configuration::GdtExportsController < Configuration::ApplicationController
  after_action :verify_authorized
  before_action -> { authorize(nil, :write, policy_class: V2::Configuration::GdtExportPolicy) }

  def show
  end

  def new
    @organization_id =
      current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) }&.id
  end
end

2. app/components/react/configuration/gdt_export_creation.rb
class React::Configuration::GdtExportCreation < React::Component
  self.selector = '.gdt-export-creation'

  def to_props(organization_id: nil)
    to_camelized_props({ organization_id: organization_id })
  end
end

3. app/views/configuration/gdt_exports/new.html.slim
- content_for(:back_button_url)
  = main_app.configuration_equipment_configurations_path
= react_component(React::Configuration::GdtExportCreation, organization_id: @organization_id)

4. packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx
type CreateDeviceButtonProps = { organizationId?: number }

const CreateDeviceButton = ({ organizationId }: CreateDeviceButtonProps) => {
  const formContext = useFormContext<MedicalDeviceWithConfigurationForm>()
  const { handleSubmit, getValues } = formContext
  const { id: workstationId } = useCurrentWorkstation()
  // ...
  const onSubmit = (deviceData: MedicalDeviceWithConfigurationForm) => {
    createDevice({
      payload: { ...deviceData, organization_id: organizationId ?? 0, workstation_id: workstationId as UUID },
    })
  }
  return <Button onClick={handleSubmit(onSubmit)}>{i18n.t('common.actions.create')}</Button>
}

const GDTExportCreation = ({ organizationId }: { organizationId?: number }) => {
  // ...
  return (
    // ...
    <CreateDeviceButton organizationId={organizationId} />
    // ...
  )
}
export default GDTExportCreation

Key additional context:
- current_account_admin_organizations returns organizations ordered by name (using OrganizationAdmin::Api::Get.organizations_for) with eager loading
- Ehr::Api::Organization.ehr?(organization_id:) does a DB query via PatientBases to check subscription existence — this means N DB queries in the controller (one per admin org)
- Ehr::Api::Organization.ehr? signature: sig { params(organization_id: Integer).returns(T::Boolean) }
- The controller test at test/controllers/configuration/gdt_exports_controller_test.rb does NOT have a test covering the new @organization_id resolution logic (multi-org scenario)
- organizationId ?? 0 in TypeScript — 0 as fallback means a request will be made with organization_id: 0 if the server resolves no eligible org, which will likely fail silently at the API level

Focus specifically on:
1. Authorization and security concerns
2. The N+1 pattern in the controller (current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(...) })
3. The ?? 0 fallback being a silent failure
4. Missing controller test coverage for the new logic
5. Prop threading pattern (organizationId passed GDTExportCreation → CreateDeviceButton)
6. Cross-engine API usage compliance (uses Ehr::Api::Organization which is correct)

Output review comments using the Conventional Comments format. For each issue, mention file and approximate line number.

Turn 1 — Assistant

I'll review the PR FHIR-2184 changes against Doctolib code guidelines, focusing on the specific concerns raised.

Tools used:

- Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 2>/dev/null && echo "---WORKTREE EXISTS---" || echo "---WORKTREE MISSING---"

Tool results:

  → result (ok): AGENTS.md
app
babel.config.js
backend.dependencies.codeowners.yml
bin
catalog-info.yaml
ci
CLAUDE.md ⇒ AGENTS.md
CODEOWNERS
CODEOWNERS.local
component_backlog.yml
config
config.ru
CONTRIBUTING.md
db
dev-env-setup-wsl
dev-tools
doc
docker
docker-compose.linux.yml
docker-compose.specmatic.yml
docker-compose.yml
docs
engine_metrics.yml
engines
fixtures
frontend-metrics.config.ts
frontend.dependencies.codeowners.yml
Gemfile
Gemfile.experimental ⇒ Gemfile
Gemfile.experimental.lock
Gemfile.lock
Gemfil
…(truncated, 613 more chars)

I'll perform the code review based on the provided diff and context. Since I have all the necessary information to review the specific concerns raised, I'll provide the review using Conventional Comments format.

Code Review: PR FHIR-2184

What I'm reviewing
- app/controllers/configuration/gdt_exports_controller.rb — new @organization_id resolution in #new
- app/components/react/configuration/gdt_export_creation.rb — props serialization
- app/views/configuration/gdt_exports/new.html.slim — view wiring
- packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx — prop threading and request payload

---

Critical (90-100)

issue (blocking): Silent failure with organizationId ?? 0 fallback
File: packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx (line ~9, in onSubmit)
Confidence: 95

payload: { ...deviceData, organization_id: organizationId ?? 0, workstation_id: workstationId as UUID },

Coalescing undefined to 0 papers over the failure mode you just fixed. When the server resolves no EHR-eligible org (the very scenario this PR addresses), the React component still sends organization_id: 0, which produces a confusing API-level rejection (or worse, an unrelated 404) instead of a clear UX state.

Suggested fix: Disable the create button (and ideally surface an empty-state message) when organizationId is undefined. Either:
- Conditionally render CreateDeviceButton only when organizationId is defined, or
- Pass disabled={organizationId == null} to <Button> and add an inline alert in GDTExportCreation.

The controller already knows when no eligible org exists — propagate that signal explicitly rather than masking it with 0. Bonus: this also tightens types — organizationId: number (required) on CreateDeviceButtonProps, with the conditional render at the parent boundary.

---

issue (blocking): N+1 DB queries in controller for org eligibility resolution
File: app/controllers/configuration/gdt_exports_controller.rb (lines 8-9, #new)
Confidence: 92

@organization_id =
  current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) }&.id

Ehr::Api::Organization.ehr? issues a DB query per call (subscription existence check via PatientBases). For an admin with N orgs, this is N sequential queries on every #new page load — directly violating the CLAUDE.md rule: "prevent N+1 queries with .includes()".

Suggested fix: Add a bulk variant to Ehr::Api::Organization (e.g. ehr_organization_ids(organization_ids:) returning a Set/Array) and use it here:

admin_org_ids = current_account_admin_organizations.map(&:id)
ehr_org_ids = Ehr::Api::Organization.ehr_organization_ids(organization_ids: admin_org_ids)
@organization_id = current_account_admin_organizations.find { |org| ehr_org_ids.include?(org.id) }&.id

If the bulk API doesn't exist yet, per CLAUDE.md you should propose adding it under engines/ehr/app/public/ (with user confirmation) rather than ship the per-row query. Cross-engine API compliance is satisfied here, but the shape of the API needs to be batch-friendly for controller use.

---

Important (80-89)

issue: Missing controller test for new @organization_id resolution
File: test/controllers/configuration/gdt_exports_controller_test.rb
Confidence: 88

The new branching logic in #new has no test coverage. Per CLAUDE.md test pyramid rules ("New controller action → integration test covering success path + main error paths") and the bug-fix rule ("Bug fix → add a regression test that would have caught the bug before the fix"), this PR must include:

1. Multi-org admin with one EHR-eligible org → @organization_id is the EHR org (not the first by name)
2. Multi-org admin with no EHR-eligible org → @organization_id is nil
3. Single-org admin, EHR-eligible → @organization_id is set
4. The exact regression scenario: admin's first-by-name org is non-EHR but a later org is EHR — must select the EHR one

Without test 4, the same bug can re-emerge if OrganizationAdmin::Api::Get.organizations_for ordering changes. Add a comment with the Jira key (# FHIR-2184) above each test method.

---

issue: Authorization check disconnected from selected organization
File: app/controllers/configuration/gdt_exports_controller.rb (line 3)
Confidence: 82

before_action -> { authorize(nil, :write, policy_class: V2::Configuration::GdtExportPolicy) }

The policy is invoked with nil and no org context, but #new then resolves an org server-side that the user will subsequently submit to. There's an implicit trust chain: "the resolved org is admin-of (because we filter current_account_admin_organizations) and EHR-eligible", but the actual create endpoint must independently re-authorize against organization_id from the payload — otherwise a client could submit any organization_id (including ones the user does not admin) since the prop is only a hint.

Suggested fix: Confirm the corresponding #create action (in whichever controller handles createDevice) authorizes the submitted organization_id against the admin scope and EHR eligibility. If it does, no change here. If not, this is a missing-authorization bug — the server cannot rely on a client-controlled organization_id. Please add a test that submits a foreign organization_id and asserts a 403.

---

suggestion: Use detect and consider memoization semantics
File: app/controllers/configuration/gdt_exports_controller.rb (line 8)
Confidence: 80

Beyond the N+1 issue, the resolved org is computed inside #new only. If current_account_admin_organizations is itself memoized but reloaded across requests, that's fine — but consider extracting this into a private method qualifying_ehr_organization_id so the (eventual batched) lookup is testable in isolation and reusable if #create needs the same check for defense-in-depth.

private

def qualifying_ehr_organization_id
  @qualifying_ehr_organization_id ||= begin
    org_ids = current_account_admin_organizations.map(&:id)
    ehr_ids = Ehr::Api::Organization.ehr_organization_ids(organization_ids: org_ids)
    current_account_admin_organizations.find { |org| ehr_ids.include?(org.id) }&.id
  end
end

---

Praise (positive feedback)

praise: Correct cross-engine API usage
File: app/controllers/configuration/gdt_exports_controller.rb (line 8)

Calling Ehr::Api::Organization.ehr? from app/public/ namespace correctly respects the engine boundary rule from CLAUDE.md. Good adherence to the API discipline — no internal-model leaks here.

praise: Server-side resolution is the right architectural fix
File: app/controllers/configuration/gdt_exports_controller.rb

Moving "which org am I creating this for?" off the client and onto the server is correct. The client should never have been trusted to pick the right org by ordering — this PR removes a real authorization-adjacent bug. The prop-threading pattern (new.html.slim → GDTExportCreation → CreateDeviceButton) is also clean and idiomatic.

---

Summary

The architectural direction of this PR is correct, but two issues should block merge:
1. N+1 DB queries in the controller (CLAUDE.md violation, performance bug at scale)
2. organizationId ?? 0 silent failure in the React component (defeats the purpose of server-side resolution when no eligible org exists)

Two issues should be addressed before merge but are non-blocking design-wise:
3. Missing regression test for the multi-org-with-non-EHR-first scenario
4. Defense-in-depth authorization on the #create endpoint (verify, not necessarily change)

Files referenced:
- /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/controllers/configuration/gdt_exports_controller.rb
- `/Users/USER/workspace/doctolib/worktrees/d
…(truncated, 243 more chars)