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

Stats: 4 user messages, 22 tool calls, tools used: Bash, Agent, ToolSearch, TaskCreate, TaskUpdate.

Conversation

Turn 1 — User

You are a Senior Software Architect at Doctolib and must review the PR hhttps://github.com/doctolib/doctolib/pull/241763, according with Doctolib Sowftware Development guidelines.

Review the PR using @pr-review-toolkit:code-reviewer, @pr-review-toolkit:pr-test-analyzer, @pr-review-toolkit:code-simplifier, @pr-review-toolkit:type-design-analyzer and @doctolib-frontend:frontend-architecture-reviewer agents. Do not post anything in the original PR, but post the output in the current terminal suggesting the comments to be made in the file/codeline using "Conventional Comments" pattern.

Worktree directory:
- worktrees: /Users/USER/workspace/doctolib/worktrees/
- doctolib: @/Users/USER/workspace/doctolib/worktrees/doctolib/
- treatment-plan: @/Users/USER/workspace/doctolib/worktrees/treatment-plan/
- doctolib-pro: @/Users/USER/workspace/doctolib/worktrees/doctolib-pro/

Turn 1 — Assistant

I'll review PR #241763 using the specified agents. Let me start by gathering the PR details and locating the relevant files.

★ Insight ─────────────────────────────────────
- Multi-agent code review distributes specialized analysis: code quality, test coverage, simplification, type design, and architecture each get a dedicated reviewer rather than overloading one general reviewer.
- "Conventional Comments" is a structured format (<label> [decorations]: <subject>) that makes review feedback actionable — labels like praise:, nitpick:, suggestion:, issue:, question:, thought:, chore: set clear intent.
─────────────────────────────────────────────────

Tools used:

- Bash: gh pr view 241763 --repo doctolib/doctolib --json title,body,headRefName,baseRefName,author,files,additions,deletions,state,url

Tool results:

  → result (ok): {"additions":140,"author":{"id":"MDQ6VXNlcjM1MDI3MTA1","is_bot":false,"login":"AdrienAnelli","name":"Adrien Anelli"},"baseRefName":"master","body":"## Summary\r\n\r\nUpdates are for the code and access control to work properly with the new organization membership pattern and the related changes about account helper methods like account.all_current_organization_ids (not changed in this PR yet)","deletions":101,"files":[{"path":"engines/ti_connect/app/controllers/ti_connect/organizations/connect
…(truncated, 2152 more chars)

Important context: this PR is a Ruby/Rails backend change in the ti_connect engine (policies, controllers, tests). Two of the requested agents are frontend/TS-specific and won't yield useful review:
- type-design-analyzer — designed for static type systems (TypeScript)
- frontend-architecture-reviewer — UpFront packages reviewer

I'll run the three applicable agents (code-reviewer, pr-test-analyzer, code-simplifier) and call out the inapplicable ones in the final summary. Let me first prepare the diff locally so the agents have something to read.

Tools used:

- Bash: git worktree list

Tool results:

  → result (ok): /Users/USER/workspace/doctolib/doctolib                                                                6fb60157f9f2 [fhir-2245_observability-search-documents]
/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2089                                            9c01fcf036d7 (detached HEAD)
/Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2091-nhr-document-list-pagination               0729f7b7bbaa [FHIR-2091-nhr-document-list-pagination]
/Users/USER/workspace/doctolib/worktrees/doctolib/FH
…(truncated, 2092 more chars)

Tools used:

- Bash: gh pr diff 241763 --repo doctolib/doctolib > /tmp/pr-241763.diff && wc -l /tmp/pr-241763.diff

Tool results:

  → result (ok): 729 /tmp/pr-241763.diff

No worktree exists for this branch. Let me create one so the agents can read the post-change files in full.

Tools used:

- Bash: git fetch origin LION-1307-ti-connect:LION-1307-ti-connect 2>&1 | tail -5

Tool results:

  → result (ok): From https://github.com/doctolib/doctolib
 * [new branch]                LION-1307-ti-connect -> LION-1307-ti-connect

Tools used:

- Bash: git worktree add /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect LION-1307-ti-connect 2>&1 | tail -10

Tool results:

  → result (ok): Preparing worktree (checking out 'LION-1307-ti-connect')
Updating files:   1% (1846/94195)
Updating files:   2% (1884/94195)
Updating files:   3% (2826/94195)
Updating files:   4% (3768/94195)
Updating files:   5% (4710/94195)
Updating files:   5% (5450/94195)
Updating files:   6% (5652/94195)
Updating files:   7% (6594/94195)
Updating files:   8% (7536/94195)
Updating files:   9% (8478/94195)
Updating files:   9% (9292/94195)
Updating files:  10% (9420/94195)
Updating files:  11% (10362/94195)
…(truncated, 4209 more chars)

Worktree ready at ~/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect. Launching the three applicable agents in parallel.

Tools used:

- Agent (inputs: description, subagent_type, prompt, run_in_background)

Tool results:

  → result (ok): Async agent launched successfully.
agentId: a6da4968d3798a33a (internal ID - do not mention to user. Use SendMessage with to: 'a6da4968d3798a33a' to continue this agent.)
The agent is working in the background. You will be notified automatically when it completes.
Do not duplicate this agent's work — avoid working with the same files or topics it is using. Work on non-overlapping tasks, or briefly tell the user what you launched and end your response.
output_file: /private/tmp/claude-501/-Users-
…(truncated, 342 more chars)

Tools used:

- Agent (inputs: description, subagent_type, prompt, run_in_background)

Tool results:

  → result (ok): Async agent launched successfully.
agentId: ad42edac719ffea2d (internal ID - do not mention to user. Use SendMessage with to: 'ad42edac719ffea2d' to continue this agent.)
The agent is working in the background. You will be notified automatically when it completes.
Do not duplicate this agent's work — avoid working with the same files or topics it is using. Work on non-overlapping tasks, or briefly tell the user what you launched and end your response.
output_file: /private/tmp/claude-501/-Users-
…(truncated, 342 more chars)

Tools used:

- Agent (inputs: description, subagent_type, prompt, run_in_background)

Tool results:

  → result (ok): Async agent launched successfully.
agentId: a50ab6390ce3885d8 (internal ID - do not mention to user. Use SendMessage with to: 'a50ab6390ce3885d8' to continue this agent.)
The agent is working in the background. You will be notified automatically when it completes.
Do not duplicate this agent's work — avoid working with the same files or topics it is using. Work on non-overlapping tasks, or briefly tell the user what you launched and end your response.
output_file: /private/tmp/claude-501/-Users-
…(truncated, 342 more chars)

Tools used:

- ToolSearch (inputs: query, max_results)

Tool results:

  → result (ok)

Tools used:

- TaskCreate (inputs: subject, description, activeForm)

Tool results:

  → result (ok): Task #1 created successfully: Code review (pr-review-toolkit:code-reviewer)

Tools used:

- TaskCreate (inputs: subject, description, activeForm)

Tool results:

  → result (ok): Task #2 created successfully: Test analysis (pr-review-toolkit:pr-test-analyzer)

Tools used:

- TaskCreate (inputs: subject, description, activeForm)

Tool results:

  → result (ok): Task #3 created successfully: Code simplification (pr-review-toolkit:code-simplifier)

Tools used:

- TaskCreate (inputs: subject, description, activeForm)

Tool results:

  → result (ok): Task #4 created successfully: Consolidate findings into Conventional Comments report

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #1 status

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #2 status

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #3 status

Tools used:

- TaskUpdate (inputs: taskId, addBlockedBy)

Tool results:

  → result (ok): Updated task #4 blockedBy

Tasks tracked. Three review agents are running in parallel against the worktree at ~/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect. I'll wait for completion notifications, then consolidate the findings into a single Conventional Comments report grouped by file.

Note: I deliberately skipped type-design-analyzer and frontend-architecture-reviewer — this PR is Ruby/Rails only, with no TypeScript or UpFront frontend code, so those agents would produce noise.

Turn 2 — User

<task-notification>
<task-id>a50ab6390ce3885d8</task-id>
<tool-use-id>toolu_019QcrbiVPfeYEeuFY5NqndY</tool-use-id>
<output-file>/private/tmp/claude-501/-Users-clertonra-workspace-doctolib-doctolib/2f870ed3-6c6a-4ade-b8d6-07e5176fedaf/tasks/a50ab6390ce3885d8.output</output-file>
<status>completed</status>
<summary>Agent "Code simplification for PR 241763" completed</summary>
<result>Now I have enough context. Let me also confirm the application_policy.rb has a comparable organization_member? that uses managed_organizations.include? which appears to overlap conceptually with current_account_managed_practice_ids.

Here is my review.

---

Summary

The PR cleanly splits "membership organizations" (read scope) vs "managed organizations" (write scope) and introduces a shared with_preloaded_associations helper — that's already a nice deduplication. A few small simplifications remain: the index action duplicates a practice filter the scope already enforces, the controller's base_scope duplicates the preload helper that now lives on the scope class, and the dual allow(:read)/allow(:write) lines in ConfigurationPolicy could be a single line.

---

engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb

suggestion (file:128-131) — base_scope duplicates the preload(:deprecated_practice, :ti_connect_clients) helper that already exists on TiConnect::ApplicationScope#with_preloaded_associations. Consider exposing it as a class method on the scope (or a small module include) so the policy and controllers share one source of truth. As-is, if the preload list ever changes (e.g. an extra association), three call sites must be updated in lockstep.

before
def base_scope
  # !! Specifically use preload to avoid joining tables across databases
  TiConnect::Connector.preload(:deprecated_practice, :ti_connect_clients)
end

after (one option — add a class-level helper on the scope)
def base_scope
  TiConnect::ApplicationScope.with_preloaded_associations(TiConnect::Connector)
end

If you prefer to keep with_preloaded_associations instance-only on the scope, an even simpler option is a private constant in this controller for the preload list, used in both base_scope and (if relevant) elsewhere. Either way, the # !! comment then exists once.

nitpick (file:9-14) — index first computes practice_ids from the org, then re-filters the policy scope by them. The ConfigurationPolicy::Scope already restricts to current_account_managed_practice_ids. Since verify_authorized is bypassed for :index and the URL's :organization_id is checked only via the scope intersection, this is safe — but it does two queries (org→practice_ids, then connectors). If current_account_managed_practice_ids already excludes practices outside the requested org for an authorized admin (which it does — admin org membership ⊆ managed orgs), the explicit filter is purely a safety net. Worth a quick mental check: do you want to keep it as defense-in-depth (then leave it), or rely on the scope (then drop the prelookup)?

current
practice_ids = CalendarManagement::Base::Practice.where(organization_id: safe_params[:organization_id]).pluck(:id)
connectors =
  policy_scope(TiConnect::Connector, policy_scope_class: TiConnect::ConfigurationPolicy::Scope).where(
    practice_id: practice_ids,
  ).order(:id)

If kept, no change needed. If dropped, the index becomes a single query through the scope.

thought (file:11-14) — Formatting: the inline .where(...) chained off policy_scope(...) is read-awkwardly because the closing ) of policy_scope lands mid-line. A local variable reads cleaner and matches update/destroy style:

after
scope = policy_scope(TiConnect::Connector, policy_scope_class: TiConnect::ConfigurationPolicy::Scope)
connectors = scope.where(practice_id: practice_ids).order(:id)

---

engines/ti_connect/app/controllers/ti_connec
…(truncated, 5162 more chars)

Turn 2 — Assistant

Code-simplifier finished. Marking that task complete and waiting for the other two agents.

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #3 status

Turn 3 — User

<task-notification>
<task-id>ad42edac719ffea2d</task-id>
<tool-use-id>toolu_01U1VTGA4PxAjjw8rfw3Kq34</tool-use-id>
<output-file>/private/tmp/claude-501/-Users-clertonra-workspace-doctolib-doctolib/2f870ed3-6c6a-4ade-b8d6-07e5176fedaf/tasks/ad42edac719ffea2d.output</output-file>
<status>completed</status>
<summary>Agent "Test analysis for PR 241763" completed</summary>
<result>I have enough information now. Let me deliver the analysis.

---

Verdict (3-5 lines)

Coverage is moderate-to-good for behavioural happy paths, but insufficient for the new ConfigurationPolicy::Scope it introduces. The new Scope class is exercised only via controller integration tests; there is no direct policy-level test, no negative test for non-admin doctors with agenda-only access (which is the exact semantic divergence between ConnectorPolicy::Scope and ConfigurationPolicy::Scope), and the dual-method split (current_account_practice_ids via Organizations API vs current_account_managed_practice_ids via OrganizationAdmin) is not covered as two distinct branches. Tests rely on real DB integration (good, matches Doctolib policy preference), but the discriminating behaviour of the refactor is silently untested.

Critical Gaps (rating 8-10)

file: /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/policies/ti_connect/configuration_policy_test.rb:43

issue (critical, rating 9): No test for the new ConfigurationPolicy::Scope — The PR introduces class Scope &lt; TiConnect::ApplicationScope inside ConfigurationPolicy (configuration_policy.rb:6-8), backed by the brand-new current_account_managed_practice_ids helper (application_scope.rb:9-11). This file ends at line 42 and tests only permit(account, connector) for :write. The Scope class is invoked from two controllers (organizations/connector_configurations_controller.rb:12 and practices/connector_configurations_controller.rb:26) but never asserted directly. A direct policy-scope test mirroring connector_policy_test.rb's structure would catch a regression where, e.g., the scope returns connectors for organizations the doctor only has agenda access in (which would be an authorization escalation).

suggestion (test): Add test "TiConnect::ConfigurationPolicyTest &gt; scope &gt; returns empty when doctor has only agenda access (no admin)" — Critical because this is the exact semantic difference vs ConnectorPolicy::Scope. ConnectorPolicy uses current_account_practice_ids (membership API → wider) while ConfigurationPolicy uses current_account_managed_practice_ids (admin-only → narrower). Without this test, swapping one back to the other would be silent.

suggestion (test): Add test "TiConnect::ConfigurationPolicyTest &gt; scope &gt; returns connectors only for organizations where account is admin" — Mirror of connector_policy_test.rb:35-43 but for the new Scope. Should also include the multi-practice-per-org case (lines 45-54 of connector_policy_test.rb).

suggestion (test): Add test "TiConnect::ConfigurationPolicyTest &gt; scope &gt; excludes connectors from other organizations the account is admin of when querying via .where(practice_id:)" — Directly exercises the cross-org isolation that the production code relies on (organizations/connector_configurations_controller.rb:9-13).

---

file: /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/policies/ti_connect/connector_policy_test.rb:33

issue (rating 8): The "agenda authorization" branch no longer asserts the new behavioural contract — Line 22 now adds Organizations::Api::Manage.add_member(...) next to the agenda creation. The test passes today but it doesn't separate the two preconditions: agenda + membership are now both required (or only membership matters?). If the add_member line is removed by accident, does the test still pass? If yes, the test is over-permissive; if no, then membership alone (w
…(truncated, 11748 more chars)

Turn 3 — Assistant

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #2 status

Test analyzer done. Waiting on the final agent (code-reviewer).

Turn 4 — User

<task-notification>
<task-id>a6da4968d3798a33a</task-id>
<tool-use-id>toolu_01NefToimvFjFDSnbeitBeX4</tool-use-id>
<output-file>/private/tmp/claude-501/-Users-clertonra-workspace-doctolib-doctolib/2f870ed3-6c6a-4ade-b8d6-07e5176fedaf/tasks/a6da4968d3798a33a.output</output-file>
<status>completed</status>
<summary>Agent "Code review for PR 241763" completed</summary>
<result>Hmm — the :de trait creates a :de organization unless organization: is explicitly passed. In the test, let!(:practice) { create(:practice, :de, ..., organization: organization) } does pass organization:, so no extra org is created. Good.

But here's the concern — at the file level (line 7), Organization.sole is called eagerly via let!, which fires BEFORE let!(:practice) (and the :de association would not have fired yet either). So the ordering is:

1. doctor_account creates → creates 1 organization via :doctor trait
2. organization = Organization.sole → should work, only one exists at this point

Then in #index, let!(:practice) is declared after these. At eval time, practice triggers using organization: organization (already resolved), so no extra organization is created. Good. Organization.sole is correct.

OK, I now have everything. Let me write up the review.

---

Review of PR #241763 (LION-1307)

Overall verdict: Comment / minor request changes

The refactor cleanly extracts a shared helper (with_preloaded_associations) and a new managed-practice-ids helper, and switches over to the public Organizations::Api::Get boundary. Engine-boundary compliance is good. There is one deliberate authorization narrowing for Practices::ConnectorConfigurationsController (admin-only access where previously agenda access could also reach it) that should be confirmed as intentional — no test asserts the previously-allowed agenda path, so the regression would not be caught. A few smaller cleanup items in tests.

Per-file comments

engines/ti_connect/app/controllers/ti_connect/practices/connector_configurations_controller.rb

engines/ti_connect/app/controllers/ti_connect/practices/connector_configurations_controller.rb:26
question: Authorization narrowing — intentional?
The pre-refactor scope was TiConnect::ConnectorPolicy::Scope (admin OR agenda access via current_account_practice_ids = all_current_organizations_ids). The new scope is TiConnect::ConfigurationPolicy::Scope which only honors current_account_managed_practice_ids (admin only). Doctors with agenda access in an org but no admin role used to receive this index; now they will get an empty list. There is no policy-level test asserting the agenda-only doctor path for this endpoint — please confirm this narrowing is intentional and add a regression test (e.g. "agenda-only doctor returns no configurations") to lock the new contract in.

engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb

engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb:128-131
suggestion: Duplicated preload — reuse the policy helper.
base_scope duplicates the preload list that already lives in TiConnect::ApplicationScope#with_preloaded_associations. If a future association is added to the helper, this controller-level scope will silently drift. Two options:
- expose with_preloaded_associations as a public class method on the scope and call it here, or
- simply inline TiConnect::Connector.preload(...) everywhere via the helper and drop base_scope.

engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb:9-14
thought: Index could go through base_scope for consistency.
update and #destroy now find the connector via base_scope (no policy_scope), but #index still uses policy_scope(...). That is fine because #index lacks a single record to authorize. Just noting the asymmetry — the policy_scope here 
…(truncated, 10442 more chars)

Turn 4 — Assistant

All three agents finished. Marking the final tasks complete and writing up the consolidated report.

Tools used:

- TaskUpdate (inputs: taskId, status)

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #1 status

Tool results:

  → result (ok): Updated task #4 status

★ Insight ─────────────────────────────────────
- The three reviewers converged on the same critical finding (authorization-scope narrowing for practices/connector_configurations_controller) from three different angles — semantic correctness (CR), test gap (TA), structural clarity (CS). When parallel reviewers triangulate on the same line, that's a strong "request changes" signal.
- The PR has a subtle behavioral shift on #destroy/#update: previously a wrong-org admin got 404 (record outside scoped_connectors); now they get 403 (found via base_scope, then authorize rejects). Functionally fine, but it's an externally-observable contract change worth pinning with a test.
─────────────────────────────────────────────────

---

Consolidated review of PR #241763 — fix(LION-1307): refactor to prepare account.organizations getter removal

Reviewers: code-reviewer (CR), pr-test-analyzer (TA), code-simplifier (CS)
Inapplicable agents (skipped): frontend-architecture-reviewer, type-design-analyzer — PR is Ruby/Rails only.
Worktree: ~/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect

Overall verdict — Request Changes (light)

The refactor is structurally sound: it migrates to the public Organizations::Api::Get boundary, factors a shared with_preloaded_associations helper, and cleanly splits "membership organizations" (read scope) vs "managed organizations" (write scope). However there are two issues that should not merge as-is:

1. A silent authorization narrowing on Practices::ConnectorConfigurationsController#index (admin-only now; previously admin-or-agenda) with no regression test asserting the old or new contract.
2. The new ConfigurationPolicy::Scope has zero direct policy-level tests — only indirect controller coverage — and lacks negative tests for cross-organization isolation.

Everything else is suggestion/nitpick territory.

---

File-by-file Conventional Comments

engines/ti_connect/app/controllers/ti_connect/practices/connector_configurations_controller.rb

L26 — issue (blocking, CR+TA): Authorization narrowing — confirm intentional and add regression test
The pre-refactor scope was TiConnect::ConnectorPolicy::Scope (admin OR agenda access via current_account_practice_ids ≈ all_current_organizations_ids). The new scope is TiConnect::ConfigurationPolicy::Scope, which only honours current_account_managed_practice_ids (admin-only). Doctors with agenda access in an org but no admin role used to receive this index; they will now receive an empty list. Please confirm the narrowing is intentional and add a regression test (#index when user has only agenda access (not admin) returns empty) to lock the new contract in.

---

engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb

L128–131 — suggestion (CR+CS): base_scope duplicates the preload list already in ApplicationScope#with_preloaded_associations
Three call sites currently know about :deprecated_practice, :ti_connect_clients; if a future association is added to the helper, this controller will silently drift.

before
def base_scope
  TiConnect::Connector.preload(:deprecated_practice, :ti_connect_clients)
end

after — single source of truth (expose helper as a class-level method on ApplicationScope)
def base_scope
  TiConnect::ApplicationScope.with_preloaded_associations(TiConnect::Connector)
end

L9–14 — thought (CR+CS): #index uses policy_scope while #update/#destroy use base_scope + explicit authorize
The asymmetry is fine (no single record to authorize on #index), but worth a code comment if you intend to keep both styles. Optionally extract scope = policy_scope(...) to a local for readability:
scope = policy_scope(TiConnect::Connector, policy_scope_class: TiConnect::ConfigurationPolicy::Scope)
connectors = scope.where(practice_id: practice_ids).order(:id)

L9–14 — thought (CS): index re-filters by practice_ids that are already constrained by the policy scope
The ConfigurationPolicy::Scope already restricts to current_account_managed_practice_ids. The explicit where(practice_id: practice_ids) is defense-in-depth — fine if intentional, but you could also rely on the scope and drop the CalendarManagement::Base::Practice.where(...).pluck(:id) round trip for #index.

---

engines/ti_connect/app/policies/ti_connect/application_scope.rb

L5 — praise (CR): Nice migration to the public Organizations::Api::Get.organization_ids_for boundary — engine-API compliance is correct.

L9–11 — question (CR): Should current_account_managed_practice_ids also go through a public API?
current_account.managed_organization_ids lives on the monolith AccountAuthorization concern (which itself calls OrganizationAdmin::Api::Get.organizations_for(...).pluck(:id)). For consistency with the line you just changed, consider whether OrganizationAdmin::Api::Get.organization_ids_for(account_ids: current_account.id) (or equivalent) would be the preferred boundary call. Not blocking — worth verifying with the owners.

L3–7 — nitpick (CS): Naming clarity — current_account_practice_ids vs current_account_managed_practice_ids
The unmarked one is the broader read scope, which is the opposite of what most readers expect. Rename to current_account_member_practice_ids for symmetry.

L3–7 — suggestion (CS): The two helpers differ only in their organization-id source — share a private extractor
def current_account_practice_ids
  practice_ids_for(::Organizations::Api::Get.organization_ids_for(account_id: current_account.id))
end
def current_account_managed_practice_ids
  practice_ids_for(current_account.managed_organization_ids)
end
private
def practice_ids_for(organization_ids)
  CalendarManagement::Base::Practice.where(organization_id: organization_ids).pluck(:id)
end

Minor — two lines of duplication is borderline. Take it or leave it.

L13–16 — suggestion (CS): with_preloaded_associations could drop its scope arg
Every caller passes the implicit scope from the surrounding scope_for block:
def preloaded_scope
  scope.preload(:deprecated_practice, :ti_connect_clients)
end

(Skip if scope isn't reliably available outside an explicit block argument in V2::ApplicationScope — verify in one place.)

L13 — suggestion (test, TA): Add a test that asserts preload actually loads the associations (e.g. assert_predicate scope.first.association(:deprecated_practice), :loaded?) — the comment warns about cross-database joins; without an assertion, a silent regression to joins would only show up at runtime.

---

engines/ti_connect/app/policies/ti_connect/configuration_policy.rb

L6–8 — praise (CR): Clean reuse of with_preloaded_associations keeps Scope classes symmetric.

L1–4 — suggestion (CS): Collapse the duplicated allow blocks
before
allow(:write) { to(:doctor) { authorized_organization_admin? } }
allow(:read)  { to(:doctor) { authorized_organization_admin? } }

after
allow(%i[read write]) { to(:doctor) { authorized_organization_admin? } }

Verify your V2::ApplicationPolicy DSL supports the array form before applying.

---

engines/ti_connect/app/policies/ti_connect/connector_policy.rb

No issues — clean diff.

L26 (file-level) — thought (CS): If ConfigurationPolicy::Scope ends up being the single source for both controllers, ConnectorPolicy could be retired in a follow-up — out of scope for this PR.

---

engines/ti_connect/test/policies/ti_connect/configuration_policy_test.rb (file-level)

issue (TA, critical): No direct policy-scope test for ConfigurationPolicy::Scope
The new class Scope < TiConnect::ApplicationScope is exercised only indirectly via controller tests. Add direct policy-scope tests mirroring connector_policy_test.rb's structure. Concrete miss
…(truncated, 7463 more chars)

Tools used:

- TaskUpdate (inputs: taskId, status)

Tool results:

  → result (ok): Updated task #4 status

Review complete. Three agents ran in parallel against the worktree at ~/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect; their findings are consolidated above as Conventional Comments grouped by file. Nothing was posted to the PR. The two strongest signals (worth blocking on) are the silent admin-only narrowing in practices/connector_configurations_controller.rb:26 and the absent direct tests for the new ConfigurationPolicy::Scope.