Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-opus-4-7`
Stats: 1 user messages, 15 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Review PR #241763 (doctolib/doctolib) for simplification opportunities. Do not edit any files — only report suggestions.
Context: Ruby/Rails refactor in the ti_connect engine preparing for the removal of account.organizations. The new code uses account.all_current_organization_ids and the new organization membership pattern.
Where to read:
- Worktree: /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect
- Full unified diff: /tmp/pr-241763.diff
Focus on the production code (not tests): - engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb - engines/ti_connect/app/controllers/ti_connect/practices/connector_configurations_controller.rb - engines/ti_connect/app/policies/ti_connect/application_scope.rb - engines/ti_connect/app/policies/ti_connect/configuration_policy.rb - engines/ti_connect/app/policies/ti_connect/connector_policy.rb
You may also briefly skim the test files for noisy duplication, but don't propose large test refactors — they're being adapted to the new pattern.
What to look for:
1. Duplicated organization-membership lookups across the three policy files — could a shared scope helper deduplicate?
2. Conditional branches in application_scope.rb that could be expressed declaratively.
3. Anything in the controllers that re-derives state already available on current_account / current_organization.
4. Naming clarity — does all_current_organization_ids need to be wrapped or aliased for readability inside the policy?
Constraints: - Preserve behavior. Suggest only changes that don't change authorization semantics. - Respect engine API boundaries — no suggesting reaches into other engines' internals. - Do NOT propose adding comments or documentation (per Doctolib comment policy).
DO NOT post anything to GitHub or edit files. Output suggestions only.
Output format — Conventional Comments with labels suggestion, nitpick, thought. Anchor each to file:line. Show the smallest possible "before → after" snippet for each suggestion. Start with a 2-3 line summary, then per-file suggestions. Skip if a file has no real simplification to gain.
Turn 1 — Assistant
I'll analyze the PR diff and the production code in the worktree to identify simplification opportunities.
Tools used:
Bash:ls /tmp/pr-241763.diff && wc -l /tmp/pr-241763.diff
Tool results:
→ result (ok): /tmp/pr-241763.diff 729 /tmp/pr-241763.diff
Tools used:
Bash:ls /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/ /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/e …(truncated, 45 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/: account_policy.rb application_policy.rb application_scope.rb configuration_policy.rb connector_policy.rb signature_configuration_policy.rb workplace_policy.rb
/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/: accounts application_controller.rb organizations practices signature_configurations workplaces_controller …(truncated, 3 more chars)
I'll ignore the MCP/skills system reminders since they're not relevant to this task. Let me read the diff and the production files.
Tools used:
Read:/tmp/pr-241763.diff
Tool results:
→ result (ok): 1 diff --git a/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb b/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb 2 index cc7dd1c6c2acd..c76ed5b78ed80 100644 3 --- a/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb 4 +++ b/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb 5 @@ -8,7 +8,10 @@ class Or …(truncated, 41579 more chars)
Now let me read the actual production code files in the worktree to inspect their current state.
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/application_scope.rb
Tool results:
→ result (ok): 1 module TiConnect
2 class ApplicationScope < ::V2::ApplicationScope
3 def current_account_practice_ids
4 CalendarManagement::Base::Practice.where(
5 organization_id: ::Organizations::Api::Get.organization_ids_for(account_id: current_account.id),
6 ).pluck(:id)
7 end
8
9 def current_account_managed_practice_ids
10 CalendarManagement::Base::Practice.where(organization_id: current_account.managed_organization_ids).pluck(:id)
11 end
12
13 def with_p
…(truncated, 204 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/configuration_policy.rb
Tool results:
→ result (ok): 1 class TiConnect::ConfigurationPolicy < TiConnect::ApplicationPolicy
2 allow(:write) { to(:doctor) { authorized_organization_admin? } }
3
4 allow(:read) { to(:doctor) { authorized_organization_admin? } }
5
6 class Scope < TiConnect::ApplicationScope
7 scope_for(:doctor) { with_preloaded_associations(scope).where(practice_id: current_account_managed_practice_ids) }
8 end
9
10 private
11
12 def authorized_organization_admin?
13 practice = CalendarManagement::Base::Practice
…(truncated, 160 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/connector_policy.rb
Tool results:
→ result (ok): 1 class TiConnect::ConnectorPolicy < TiConnect::ApplicationPolicy 2 class Scope < TiConnect::ApplicationScope 3 scope_for(:doctor) { with_preloaded_associations(scope).where(practice_id: current_account_practice_ids) } 4 end 5 end 6
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/workplace_policy.rb
Tool results:
→ result (ok): 1 class TiConnect::WorkplacePolicy < TiConnect::ApplicationPolicy 2 class Scope < TiConnect::ApplicationScope 3 scope_for(:doctor) { TiConnect::Workplace.where(practice_id: current_account_practice_ids) } 4 end 5 end 6
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/application_policy.rb
Tool results:
→ result (ok): 1 module TiConnect
2 class ApplicationPolicy < ::V2::ApplicationPolicy
3 ALLOWED_USER_TYPES = %i[doctor].freeze
4
5 allow(:write) { to(:doctor) { organization_admin? } }
6 allow(:read) { to(:doctor) { organization_member? } }
7
8 private
9
10 def organization_admin?
11 record.organization && current_account.admin_organization_authorizations_for?(record.organization.id)
12 end
13
14 def organization_member?
15 current_account.managed_organizations.inc
…(truncated, 56 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb
Tool results:
→ result (ok): 1 module TiConnect
2 class Organizations::ConnectorConfigurationsController < TiConnect::ApplicationController
3 before_action :validate_params!
4 after_action :verify_policy_scoped, only: %i[index]
5 after_action :verify_authorized, except: %i[index]
6
7 IndexContract = Dry::Schema.Params { required(:organization_id).filled(:string) }
8
9 def index
10 practice_ids = CalendarManagement::Base::Practice.where(organization_id: safe_params[:organization_id]).pluck(:id)
…(truncated, 4660 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/practices/connector_configurations_controller.rb
Tool results:
→ result (ok): 1 module TiConnect
2 class Practices::ConnectorConfigurationsController < TiConnect::ApplicationController
3 before_action :validate_params!
4 before_action :fetch_scoped_connectors
5 after_action :verify_policy_scoped, only: %i[index]
6 after_action :verify_authorized, except: %i[index]
7
8 IndexContract = Dry::Schema.Params { required(:practice_id).filled(:string) }
9
10 def index
11 connectors = @connectors.where(practice_id: safe_params[:practice_id]).order(
…(truncated, 507 more chars)
Now let me check what current_account.managed_organization_ids and Organizations::Api::Get.organization_ids_for represent — to verify they're conceptually distinct.
Tools used:
Bash:cd /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect && grep -rn "managed_organization_ids\|all_current_organization_ids\|all_current_organizations_ids" app/models/account.rb …(truncated, 92 more chars)
Tool results:
→ result (ERROR): Permission to use Bash has been denied. IMPORTANT: You may attempt to accomplish this action using other tools that might naturally be used to accomplish this goal, e.g. using head instead of cat. But you should not attempt to work around this denial in malicious ways, e.g. do not use your ability to run tests to execute non-test actions. You should only try to work around this restriction in reasonable ways that do not attempt to bypass the intent behind this denial. If you believe this cap …(truncated, 178 more chars)
Tools used:
Bash:grep -rn "def managed_organization_ids\|def all_current_organization_ids\|def organization_ids_for" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models /Users/clerto …(truncated, 118 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models/concerns/account_authorization.rb:88: def managed_organization_ids /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/get.rb:125: def organization_ids_for(account_id:, use_interservice_api: false)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models/concerns/account_authorization.rb(80–105)
Tool results:
→ result (ok): 80 def agenda_unique_organization_ids
81 agenda_unique_organizations.distinct.pluck(:id)
82 end
83
84 def practice_ids
85 CalendarManagement::Base::Practice.where(organization_id: agenda_unique_organization_ids).distinct.pluck(:id)
86 end
87
88 def managed_organization_ids
89 managed_organizations.pluck(:id)
90 end
91
92 # @deprecated Use {#managed_organization_ids} instead.
93 def organization_ids
94 managed_organization_ids
95 end
96
97 def patient_base_
…(truncated, 300 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/get.rb(115–145)
Tool results:
→ result (ok): 115 #
116 # @example
117 # Organizations::Api::Get.organization_ids_for(account_id: 5678)
118 #
119 # @param account_id [Integer] The account Id.
120 # @param use_interservice_api [Boolean] Whether to use the interservice API.
121 # @return Array[Integer] Organization Ids
122 sig do
123 params(account_id: T.nilable(Integer), use_interservice_api: T.nilable(T::Boolean)).returns(T::Array[Integer])
124 end
125
…(truncated, 915 more chars)
Good — these are conceptually distinct: managed_organization_ids is admin-of, while Organizations::Api::Get.organization_ids_for is organization-membership. The two scope helpers correctly map to read vs write.
Now let me look at the index controller more closely. The duplicate scoping in the index method calls policy_scope then where(practice_id:) — but the scope already filters by current_account_managed_practice_ids. Let me verify what filter intersection we have.
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/organizations/connector_configurations_controller.rb(9–26)
Tool results:
→ result (ok): 9 def index
10 practice_ids = CalendarManagement::Base::Practice.where(organization_id: safe_params[:organization_id]).pluck(:id)
11 connectors =
12 policy_scope(TiConnect::Connector, policy_scope_class: TiConnect::ConfigurationPolicy::Scope).where(
13 practice_id: practice_ids,
14 ).order(:id)
15
16 @configurations =
17 connectors.map do |connector|
18 TiConnect::Configuration.new(
19 practice_id: connector.practice_
…(truncated, 138 more chars)
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_connect/practices/connector_configurations_controller.rb
No real simplification beyond the consistency note below.
nitpick (file:26) — Now that both org and practice controllers use TiConnect::ConfigurationPolicy::Scope, TiConnect::ConnectorPolicy::Scope is only used by tests in this PR's footprint. Worth confirming whether TiConnect::ConnectorPolicy itself is still referenced in production code; if not, it could be retired in a follow-up to avoid the read/write scope ambiguity (current_account_practice_ids vs current_account_managed_practice_ids).
engines/ti_connect/app/policies/ti_connect/application_scope.rb
suggestion (file:13-16) — with_preloaded_associations is currently an instance method that takes scope as an argument, but every caller passes the implicit scope available inside scope_for. Making it a no-arg helper that returns the preloaded scope keeps the call sites tighter and removes the redundant scope token from each scope_for block.
# before
def with_preloaded_associations(scope)
# !! Specifically use preload to avoid joining tables across databases
scope.preload(:deprecated_practice, :ti_connect_clients)
end
# after — relies on `scope` being in lexical scope of scope_for
def preloaded_scope
scope.preload(:deprecated_practice, :ti_connect_clients)
end
Then call sites read:
scope_for(:doctor) { preloaded_scope.where(practice_id: current_account_managed_practice_ids) }
(Skip if scope isn't reliably available outside an explicit block argument in your V2::ApplicationScope DSL — easy to verify in one place.)
thought (file:3-7) — The two practice-id helpers differ only in their organization-id source. They could share a single private helper to remove the repeated CalendarManagement::Base::Practice.where(organization_id: ...).pluck(:id):
# after
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
This is purely a readability call — two lines of duplication is borderline. Take it or leave it.
nitpick (file:5) — Naming: current_account_practice_ids (membership) vs current_account_managed_practice_ids (admin) is hard to tell apart at a glance — the unmarked one is the broader read scope, which is the opposite of what most readers expect. Consider current_account_member_practice_ids and current_account_managed_practice_ids for symmetry. Touches two policy files.
engines/ti_connect/app/policies/ti_connect/configuration_policy.rb
suggestion (file:1-4) — The two allow lines have identical bodies. The V2::ApplicationPolicy DSL accepts an array, which collapses them:
# 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 the DSL supports an array form in your V2::ApplicationPolicy (most pundit-flavoured DSLs do; if not, leave as-is).
thought (file:13) — authorized_organization_admin? does CalendarManagement::Base::Practice.find(record.practice_id) even when record is a freshly-built TiConnect::Connector.new(practice_id: …) (the create path) — so it works, but the round-trip is per-request. If record.deprecated_practice is reliably available (it's preloaded everywhere except create), record.deprecated_practice&.organization_id || CalendarManagement::Base::Practice.where(id: record.practice_id).pick(:organization_id) would skip the find for already-loaded connectors. Out of scope for this PR — flagging as future work.
engines/ti_connect/app/policies/ti_connect/connector_policy.rb
No change needed — the file is already minimal. If ConfigurationPolicy::Scope becomes the single source for both controllers (per the controller note above), ConnectorPolicy could be deleted in a follow-up.
Files referenced
- `/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/organi …(truncated, 636 more chars)