Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-opus-4-7`
Stats: 1 user messages, 47 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Review PR #241763 (doctolib/doctolib) titled "fix(LION-1307): refactor to prepare account.organizations getter removal and related changes" by Adrien Anelli.
Context: This Ruby/Rails PR in the ti_connect engine prepares for the upcoming removal of the account.organizations getter. It refactors policies and controllers to use the new organization membership pattern (e.g. account.all_current_organization_ids) and updates tests accordingly. It is a backend-only PR (no frontend changes).
Where to read code:
- Worktree: /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect
- Full unified diff: /tmp/pr-241763.diff
Files changed (focus your review on these — read them in full from the worktree, not just the diff): Source: - 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 Tests: - engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb - engines/ti_connect/test/controllers/ti_connect/practices/connector_configurations_controller_test.rb - engines/ti_connect/test/controllers/ti_connect/practices/connectors_controller_test.rb - engines/ti_connect/test/controllers/ti_connect/workplaces_controller_test.rb - engines/ti_connect/test/policies/ti_connect/connector_policy_test.rb - engines/ti_connect/test/policies/ti_connect/workplace_policy_test.rb - engines/ti_connect/test/public/ti_connect/api/connector_test.rb - test/integration/configuration/ti_connect/ti_connect_configuration_page_test.rb - test/integration/configuration/ti_connect/ti_workplace_configuration_page_test.rb
Key things to verify against Doctolib guidelines:
1. Engine boundaries: any cross-engine model access must go through EngineName::Api::* (the public folder). Flag direct internal model access from other engines or the monolith.
2. Patient base / patient account isolation rules (probably not applicable here, but check).
3. Pundit policy patterns: scope correctness, no permission leaks, consistent use of current_organization vs current_account.
4. Code style: rubocop conventions, no SQL strings in scopes when ActiveRecord can express it, not_trashed usage if relevant.
5. Behaviour preservation: did the refactor change authorization semantics (broader/narrower access than before)?
DO NOT post anything to GitHub. Output your findings as terminal text.
Output format — use Conventional Comments (<label> [decorations]: <subject>) with these labels: praise, nitpick, suggestion, issue, question, thought, chore. For each comment, give: file:line anchor, label, one-line subject, and a short body when needed. Group by file. Begin with a 3-5 line overall verdict (approve / request changes / comment), then the per-file comments. Keep total output focused — skip generic praise.
Turn 1 — Assistant
I'll review this PR by examining the diff and the changed files in the worktree.
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/
Tool results:
→ result (ok): account_policy.rb application_policy.rb application_scope.rb configuration_policy.rb connector_policy.rb signature_configuration_policy.rb workplace_policy.rb
I'll ignore the system reminders about MCP/skills - they're not relevant to this code review task. Let me read the changed 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 files in the worktree to understand the full context.
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)
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
Now let me check the parent ApplicationScope class, and verify what scope resolves to in pundit's scope_for block.
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies -type f | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/workplace_policy.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/signature_configuration_policy.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/policies/ti_connect/application_policy.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_ …(truncated, 456 more chars)
Tools used:
Bash:grep -rn "class ApplicationScope" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/lib 2>/dev …(truncated, 16 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/application_scope.rb:1:class ApplicationScope /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/external_api/application_policy.rb:26: class ApplicationScope /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/external_api/smart_reader_application_policy.rb:21: class ApplicationScope
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect -path '*/v2/application_scope*' -type f 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/application_scope.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/siilo_integration/app/policies/siilo_integration/v2/application_scope.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/resource_identifiers/app/policies/resource_identifiers/v2/application_scope.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/par …(truncated, 219 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/application_scope.rb
Tool results:
→ result (ok): 1 class V2::ApplicationScope
2 include V2::ContextValidator
3 include V2::PolicyScopeFramework
4 include V2::PolicyHelper
5 attr_reader :current_context, :current_account, :scope, :current_user_type, :filter_ids, :user_actor
6
7 ALLOWED_USER_TYPES = AccessControl::ALLOWED_USER_TYPES
8
9 def initialize(current_account, current_user_type, scope, current_context: {}, filter_ids: nil)
10 @user_actor = current_context[:user_actor] if !current_context.is_a?(SafePundit::SanitizedConte
…(truncated, 307 more chars)
Good - scope is the attribute set during init. Now let me look at the policy framework to verify the policy_scope API:
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2 -type f
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/application_policy.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/application_scope.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/policy_scope.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/policy_scope_framework.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-conn …(truncated, 14389 more chars)
Tools used:
Bash:grep -n "scope_for\|def initialize\|policy_scope" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/*.rb 2>/dev/null | head -40
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/application_scope.rb:9: def initialize(current_account, current_user_type, scope, current_context: {}, filter_ids: nil) /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/policy_scope_framework.rb:9: # e.g. scope_for(:staff) { ... } /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/policy_scope_framework.rb:13: def scope_for(*use …(truncated, 1031 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/policies/v2/policy_scope_framework.rb
Tool results:
→ result (ok): 1 module V2::PolicyScopeFramework
2 def self.included(base)
3 base.extend(ClassMethods)
4 end
5
6 module ClassMethods
7 # Define the policy scope for given user type(s)
8 #
9 # e.g. scope_for(:staff) { ... }
10 #
11 # For further information visit
12 # https://doctolib.atlassian.net/wiki/spaces/PTA/pages/1313670552/SPICE+AC+Scoping+070+-+Structuring+authorization+by+user+types
13 def scope_for(*user_types, &block)
14 user_types.each do |user_type|
1
…(truncated, 806 more chars)
Good, so scope resolves to the attr_reader from V2::ApplicationScope. The new with_preloaded_associations(scope) call uses the scope reader - this works correctly because the scope class is initialized with TiConnect::Connector as the scope.
Now let me look at the managed_organization_ids method to understand what changed semantically:
Tools used:
Bash:grep -rn "def managed_organization_ids\|def all_current_organizations_ids\|def all_current_organization_ids" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app /Users/cler …(truncated, 95 more chars)
Tools used:
Bash:grep -rn "def all_current_organizations_ids\|def all_current_organization_ids\|def managed_organization_ids" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect 2>/dev/null | g …(truncated, 27 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models/concerns/account_authorization.rb:22: def all_current_organizations_ids /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/siilo_integration/app/models/siilo_integration/doctolib_account.rb:77: def all_current_organizations_ids
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models/concerns/account_authorization.rb:22: def all_current_organizations_ids /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/siilo_integration/app/models/siilo_integration/doctolib_account.rb:77: def all_current_organizations_ids
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/app/models/concerns/account_authorization.rb
Tool results:
→ result (ok): 1 module AccountAuthorization
2 extend ActiveSupport::Concern
3
4 included do
5 attr_accessor :current_agenda_authorizations, :agenda_ids_for_current_account
6
7 scope(
8 :for_organizations,
9 lambda do |organization_ids|
10 account_ids =
11 Array(organization_ids)
12 .flat_map { |organization_id| Organizations::Api::Get.member_ids(organization_id: organization_id) }
13 .uniq
14 where(id: account_ids)
15 end,
16
…(truncated, 4397 more chars)
Now I have a critical insight. Let me verify the semantic change carefully:
Before refactor:
- current_account_practice_ids used current_account.all_current_organizations_ids, which returns organizations from agenda authorizations OR organization admin rights (a UNION).
After refactor:
- current_account_practice_ids (used by ConnectorPolicy::Scope, called from Practices::ConnectorsController) now uses Organizations::Api::Get.organization_ids_for(account_id:) — returns ALL organizations the account belongs to (membership).
- New current_account_managed_practice_ids (used by new ConfigurationPolicy::Scope) uses only managed_organization_ids (admin only).
Let me verify what Organizations::Api::Get.organization_ids_for does, and check the assignment of policy scopes for each controller.
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public -type f 2>/dev/null | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/get.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/manage.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/organization_drn.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/ …(truncated, 789 more chars)
Tools used:
Bash:grep -rn "def organization_ids_for\|def member_ids\|module Get" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public 2>/dev/null | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/public/organizations/api/get.rb:43: def member_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/engines/organizations/app/public/organizations/api/get.rb(115–175)
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, 1339 more chars)
Now I have a clear picture. Let me verify which controllers use which policy scope so I can assess semantic preservation:
Tools used:
Bash:grep -rn "policy_scope.*ConnectorPolicy\|policy_scope.*ConfigurationPolicy\|TiConnect::ConnectorPolicy::Scope\|TiConnect::ConfigurationPolicy::Scope" /Users/USER/workspace/doctolib/worktrees/doct …(truncated, 56 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/public/ti_connect/api/connector.rb:23: .policy_scope(@current_context, TiConnect::Connector, policy_class: TiConnect::ConnectorPolicy) /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/public/ti_connect/api/connector.rb:40: policy_scope_class: TiConnect::ConnectorPolicy::Scope, /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-conn …(truncated, 1504 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/controllers/ti_connect/practices/connectors_controller.rb
Tool results:
→ result (ok): 1 module TiConnect
2 class Practices::ConnectorsController < TiConnect::ApplicationController
3 before_action :authorize_user, except: %i[index]
4
5 after_action :verify_policy_scoped
6 after_action :verify_authorized, except: %i[index]
7
8 def index
9 @connectors =
10 scoped_connectors.select(:id, :practice_id, :gsmck_certificate_expires_on, :product_name, :product_type_version)
11 end
12
13 def update
14 scoped_connectors.find(params.require(:id
…(truncated, 432 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/public/ti_connect/api/connector.rb
Tool results:
→ result (ok): 1 module TiConnect::Api
2 class Connector
3 # @param current_account [Account] The current account
4 # @param current_user_type [Symbol] The current user type. Must be :doctor
5 def initialize(current_account:, current_user_type:)
6 @current_context = { current_account: current_account, current_user_type: current_user_type }
7 end
8
9 # @param organization_id [Integer] The unique identifier for the organization
10 # @return [Array<TiConnect::PracticeConnectorsDto
…(truncated, 2164 more chars)
Now I understand the full picture. Let me check the existing usage of add_member calls and tests carefully — there's a meaningful semantic change here. Let me also analyze the practices/connector_configurations_controller change:
Key semantic change:
In practices/connector_configurations_controller.rb, the previous policy was TiConnect::ConnectorPolicy::Scope (using current_account_practice_ids = agenda+admin orgs). Now it's TiConnect::ConfigurationPolicy::Scope which uses current_account_managed_practice_ids (admin only). This narrows access — agenda-only users who used to be able to read configurations via their agenda assignment can no longer see them. That's a potential authorization-semantics change.
Let me check the test for that controller to see if this change is indeed intentional:
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/controllers/ti_connect/practices/connector_configurations_controller_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::Practices::ConnectorConfigurationsControllerTest < Doctolib::Testing::ActionDispatch::IntegrationTest
4 self.subdomain = :pro
5
6 describe '#index' do
7 let!(:organization) { create(:organization, country: Country::DE) }
8 let!(:doctor_account) { create(:hcp_account, :verified, :with_unique_email, locale: :de) }
9 let!(:agenda) { create(:agenda, organization_id: organization.id, practitioner_id: doctor_account.id) }
10 let!(:pati
…(truncated, 2764 more chars)
The "when user is not an admin" test exists with the agenda still attached, and asserts no results. So the behavior under the policy change is consistent — the previous test was likely the same (scoped_connectors checked if user is admin via authorized_organization_admin? though scope was wider). Let me also check the connector_policy_test.rb closely:
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/policies/ti_connect/connector_policy_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::ConnectorPolicyTest < AuthorizationMinitestSpec::Policy::TestCase
4 self.policy_class = TiConnect::ConnectorPolicy
5
6 let!(:organization) { create(:organization, :with_german_practice) }
7 let!(:practice) { organization.practices.first }
8 let!(:account) { create(:hcp_account) }
9 let!(:connector) { create(:ti_connect_connector, deprecated_practice: practice) }
10
11 describe 'scope' do
12 user_type(:doctor) do
13 describe "w
…(truncated, 1922 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/policies/ti_connect/workplace_policy_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::WorkplacePolicyTest < AuthorizationMinitestSpec::Policy::TestCase
4 self.policy_class = TiConnect::WorkplacePolicy
5
6 let!(:organization) { create(:organization, :with_german_practice) }
7 let!(:practice) { organization.practices.first }
8 let!(:account) { create(:hcp_account) }
9 let!(:workplace) { create(:ti_connect_workplace, deprecated_practice: practice) }
10
11 describe 'scope' do
12 user_type(:doctor) do
13 describe "w
…(truncated, 1531 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/controllers/ti_connect/workplaces_controller_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::WorkplacesControllerTest < Doctolib::Testing::ActionDispatch::IntegrationTest
4 self.subdomain = :pro
5
6 let!(:doctor_account) { create(:hcp_account, :verified, :with_unique_email, locale: :de) }
7 let!(:practice) { create(:practice, :de) }
8
9 describe '#show' do
10 let!(:workplace_1) { create(:ti_connect_workplace, practice_id: practice.id) }
11 let!(:workplace_2) { create(:ti_connect_workplace, practice_id: practice.id) }
12
13
…(truncated, 12868 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/controllers/ti_connect/practices/connectors_controller_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::Practices::ConnectorsControllerTest < Doctolib::Testing::ActionDispatch::IntegrationTest
4 self.subdomain = :pro
5
6 let!(:practice) { create(:practice, :de) }
7 let!(:doctor_account) { create(:hcp_account, :verified, :with_unique_email, locale: :de) }
8
9 describe '#index' do
10 before { login_session(doctor_account) }
11
12 describe 'when practice organization is not managed by user' do
13 let!(:connector) { create(:ti_conn
…(truncated, 3173 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 class TiConnect::Organizations::ConnectorConfigurationsControllerTest < Doctolib::Testing::ActionDispatch::IntegrationTest
4 self.subdomain = :pro
5
6 let!(:doctor_account) { create(:hcp_account, :doctor, :verified, :with_unique_email, locale: :de) }
7 let!(:organization) { Organization.sole }
8 let!(:patient_base) do
9 PatientBases::Api::PatientBaseRepository.find_by_query({ organization_id: organization.id }).value_or([]).sole
10 end
11 let!(:agend
…(truncated, 15413 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/public/ti_connect/api/connector_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 module TiConnect::Api
4 class ConnectorTest < Doctolib::Testing::ActiveSupport::TestCase
5 describe '#information_for' do
6 let!(:account) { create(:hcp_account, :doctor) }
7 let!(:first_organization) { create(:organization, country: Country::DE) }
8
9 describe 'invalid authorization context' do
10 describe 'when current user type is not compatible with account' do
11 it 'raises an error' do
12 connector_api = Ti
…(truncated, 5691 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/integration/configuration/ti_connect/ti_connect_configuration_page_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 module Integration::Configuration::TiConnect
4 class TiConnectConfigurationPageTest < Doctolib::Testing::Legacy::ActionDispatch::SystemTestCase
5 include ToastsTestHelper
6
7 self.subdomain = :pro
8
9 let!(:patient_base_one) { create(:patient_base, name: 'Patient Base A') }
10 let!(:organization) { patient_base_one.organization }
11 let!(:practice) { create(:practice, :de, formal_name: 'Practice A', organization: organization) }
12 let!(:a
…(truncated, 15024 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/integration/configuration/ti_connect/ti_workplace_configuration_page_test.rb
Tool results:
→ result (ok): 1 require 'test_helper'
2
3 module Integration::Configuration::TiWorkplace
4 class TiWorkplaceConfigurationPageTest < Doctolib::Testing::Legacy::ActionDispatch::SystemTestCase
5 include ToastsTestHelper
6
7 self.subdomain = :pro
8
9 let!(:organization) { create(:organization, country: Country::DE) }
10 let!(:practice) { create(:practice, :de, formal_name: 'Practice A', organization: organization) }
11 let!(:account) { create(:hcp_account, :admin, for_organizations: [orga
…(truncated, 7839 more chars)
Now I have a strong understanding. Let me check one more thing — does current_account.add_as_admin_for(organization:) add membership? That matters because tests using only add_as_admin_for (i.e., the organizations connector_configurations test) might break under the new scope.
Looking at the concern: add_as_admin_for calls OrganizationAdmin::Api::Manage.create!. The new current_account_managed_practice_ids uses managed_organization_ids which queries OrganizationAdmin::Api::Get.organizations_for(account_id: id). So adding admin → managed_organization_ids gives that org → ok, no membership needed for the configuration path. Good — those tests still pass.
But for ConnectorPolicy::Scope (now using current_account_practice_ids via Organizations::Api::Get.organization_ids_for), tests need explicit Organizations::Api::Manage.add_member since admin alone doesn't imply membership. That explains why connector_policy_test.rb, workplace_policy_test.rb, connectors_controller_test.rb, workplaces_controller_test.rb, connector_test.rb (public api) all add explicit add_member calls. Looks consistent.
Let me also re-verify the original practices/connector_configurations_controller.rb. The old policy was ConnectorPolicy (admin OR agenda). The new policy is ConfigurationPolicy::Scope (admin only). This is a deliberate authorization narrowing — agenda-only users no longer see practice configurations. Let me check that test for "user is not an admin":
Line 17-26 of practices/connector_configurations_controller_test.rb — note the test now no longer creates an explicit admin role for the agenda-only user. The agenda is associated with doctor_account via practitioner_id. But importantly: simply being a practitioner of an agenda doesn't grant agenda access — that requires AgendaAccess::Api::Manage.create!. So the test setup never had the agenda-access path actually wired. The behavior remains "no agenda access, no admin → empty result". The change in policy class doesn't alter test outcomes here, but it does narrow agenda-based access in production. I'll flag this as a question for the author.
Let me also check whether there is a similar test that explicitly covers the agenda-only case — the connector_policy_test does (line 19-32) and it's still using ConnectorPolicy::Scope which still includes agenda users.
Now check the current_account_practice_ids semantic change more carefully — the old version used all_current_organizations_ids (agenda-orgs + admin-orgs). The new one uses Organizations::Api::Get.organization_ids_for (full membership). Test still passes because adding agenda OR admin should now trigger membership. Let me confirm:
Tools used:
Bash:grep -rn "Membership\.organization_ids_for\|def organization_ids_for" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app 2>/dev/null | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/models/organizations/membership.rb:128: def organization_ids_for(account_id:, use_interservice_api: false) /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/organizations/app/models/concerns/organizations/observability/membership.rb:24: def organization_ids_for(account_id:, use_interservice_api: false) /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-c …(truncated, 334 more chars)
OK. The cross-engine API call (Organizations::Api::Get.organization_ids_for) is correctly going through the app/public/ boundary. Same for Organizations::Api::Manage.add_member and PatientBases::Api::PatientBaseRepository. Good engine-boundary compliance.
Now I have enough to write a full review. Let me also check if there's a duplicate let!(:patient_base) in the organizations test (line 8 and line 133):
Looking at the test again — line 8 declares let!(:patient_base) at the file-wide scope using the Organization.sole's patient base. But in #create (line 133), there's a redefinition: let!(:patient_base) { create(:patient_base) }. That creates a separate patient_base unrelated to the organization. This could be a problem if the create endpoint requires the patient_base to belong to the same organization as the practice. But the original code had the same pattern — let me check:
Looking at the diff, the file-wide declaration is new (moved up from #index), so previously #create had the only patient_base. Now #create shadows the new file-wide one. That's likely intentional (it's a create test that needs an explicit one). But the issue is: in #create, let!(:practice) is for organization (the Organization.sole), but patient_base is create(:patient_base) — likely a different organization. Will the create succeed? This isn't a regression from the PR, but worth noting. Let me verify:
Tools used:
Bash:grep -rn "def create_connector_config\|class ConfigurationsService" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app 2>/dev/null | head -5
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/services/ti_connect/configurations_service.rb:2: class ConfigurationsService /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/app/services/ti_connect/configurations_service.rb:4: def create_connector_config(practice_id, connector_params, clients_params)
OK that's not in scope of this PR, leaving aside.
One more thing — in the organizations tests file, line 8 patient_base has a related concern. The agenda is now doctor_account.agendas.sole. This relies on the :doctor factory automatically creating an agenda. Let me double-check there's no test ordering issue:
Tools used:
Bash:grep -rn ":doctor\b\|trait :doctor" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/hcp_account_factory.rb /Users/USER/workspace/doctolib/worktrees/doct …(truncated, 82 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories -name "*hcp*" -o -name "*account*" 2>/dev/null | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/account_pin_code_tracker.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/inactive_agenda_account_notification.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/abstract_account.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/inactive_account.rb /Users/USER/workspace/doctolib/worktrees/doctoli …(truncated, 639 more chars)
Tools used:
Bash:grep -rln "factory :hcp_account\|factory(:hcp_account" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test 2>/dev/null | head -5
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rln "factory :hcp_account" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect 2>/dev/null | head -5
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account_organization_provider.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account_health_professional_provider.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account_health_professional.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-con …(truncated, 54 more chars)
Tools used:
Bash:grep -n "trait :doctor\|trait :admin\|for_organizations" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account.rb 2>/dev/null | hea …(truncated, 5 more chars)
Tool results:
→ result (ok): 79: trait :administrative_agent do 147: trait :admin do 148: transient { for_organizations { [] } } 151: evaluator.for_organizations.presence || 537: trait :doctor do
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account.rb(147–177)
Tool results:
→ result (ok): 147 trait :admin do 148 transient { for_organizations { [] } } 149 after(:create) do |doctor, evaluator| 150 organizations = 151 evaluator.for_organizations.presence || 152 create_list( 153 :organization, 154 1, 155 account_manager: create(:staff_accounts_account), 156 country: 157 doctor.locale.to_s == 'en' ? Country::FR : Country.deprecated_find_by_default_locale!(doctor.locale …(truncated, 877 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/hcp_account/test/factories/hcp_account.rb(530–570)
Tool results:
→ result (ok): 530 account: account, 531 level: evaluator.level, 532 ) 533 end 534 end 535 end 536 537 trait :doctor do 538 agenda_access 539 540 after(:create) do |doctor| 541 # rubocop:todo DoctolibInternal/NoLegacyKycApi 542 KnowYourCustomer::Api::LegacyIdentity.set_from_legacy_identity_status( 543 account_id: doctor.id, 544 status: Profile::IdentityVerificationStatus::VERIFICATION_NOT_NEEDED, # rubocop …(truncated, 1004 more chars)
Excellent — this confirms several things:
1. :doctor trait calls agenda_access which creates an agenda AND adds the account as an org member. So doctor_account.agendas.sole works and Organization.sole also works.
2. The :admin trait with for_organizations: [organization] adds membership AND admin authorization. So in the integration tests using :admin, for_organizations: [organization], the account is both member and admin — perfect for the new current_account_managed_practice_ids (admin-only) scope.
Let me check the tests for the organizations controller closely. Line 11-15 the agenda is created via doctor_account.agendas.sole — but the original was create(:agenda, practitioner_id: doctor_account.id). The new version relies on the :doctor trait's auto-created agenda. The update!(practitioner_id: doctor_account.id) updates that agenda. The new test seems to imply the test only wants to check default-trait behavior, but resetting the practitioner is unnecessarily side-effect-y. Let me check if this agenda is even used by any it block:
Tools used:
Bash:grep -n "agenda\b" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb
Tool results:
→ result (ok): 11: let!(:agenda) do 12: agenda = doctor_account.agendas.sole 13: agenda.update!(practitioner_id: doctor_account.id) 14: agenda
The agenda let! is declared but never referenced anywhere else in the test. The original let!(:agenda) { create(:agenda, practitioner_id: doctor_account.id) } was probably also unreferenced (it served as setup). But now this let! is dead code, just a side-effecting setup. Worth flagging as cleanup.
Let me verify the Organization.sole assumption — it relies on the :doctor trait creating exactly ONE organization. If any other code creates an organization (e.g. in :de practice trait), Organization.sole would raise. Let me verify:
Tools used:
Bash:grep -n "trait :de\b\|trait(:de)" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories/practice.rb 2>/dev/null | head -5
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rln "factory :practice" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/test/factories /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/eng …(truncated, 26 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/calendar_management/test/factories/practice.rb /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/calendar_management/test/factories/calendar_management/practice_accessibility_feature.rb
Tools used:
Bash:grep -n "trait :de\|country.*Country::DE" /Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/calendar_management/test/factories/practice.rb 2>/dev/null | head -10
Tool results:
→ result (ok): 36: trait :de do
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/LION-1307-ti-connect/engines/calendar_management/test/factories/practice.rb(30–60)
Tool results:
→ result (ok): 30 end
31
32 trait :aphp do
33 external_id { 'uf-129-005' }
34 end
35
36 trait :de do
37 address { 'Dehnhaide 120' }
38 latitude { 53.577222 }
39 longitude { 10.053889 }
40 zipcode { '22081' }
41 city { 'Hamburg' }
42 landline_number { '+49 04 020920' }
43 association :organization, :de
44 end
45
46 trait :reunion_region do
47 address { '60 Rue Bertin' }
48 latitude { 48.577222 }
49 longitude { 10.053889
…(truncated, 299 more chars)
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:
doctor_accountcreates → creates 1 organization via:doctortraitorganization = 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 doubles as authorization, while #update/#destroy do an explicit authorize(...). Worth a comment in code if you keep both styles.
engines/ti_connect/app/policies/ti_connect/application_scope.rb
engines/ti_connect/app/policies/ti_connect/application_scope.rb:5
praise: Nice migration to the public Organizations::Api::Get.organization_ids_for boundary.
engines/ti_connect/app/policies/ti_connect/application_scope.rb:9-11
question: Should current_account_managed_practice_ids also go through a public API?
current_account.managed_organization_ids is a method on the monolith AccountAuthorization concern that internally calls OrganizationAdmin::Api::Get.organizations_for(...) followed by .pluck(:id). That is fine within the monolith, but for consistency with the change you just made one method above (current_account.all_current_organizations_ids → Organizations::Api::Get.organization_ids_for), consider whether OrganizationAdmin::Api::Get.organization_ids_for(account_ids: current_account.id) (or equivalent) would be the preferred public-API call now that you are tightening the surface. Not blocking, but worth checking with the owners.
engines/ti_connect/app/policies/ti_connect/configuration_policy.rb
engines/ti_connect/app/policies/ti_connect/configuration_policy.rb:6-8
praise: Good refactor — reusing with_preloaded_associations(scope) keeps ConnectorPolicy::Scope and ConfigurationPolicy::Scope symmetric.
engines/ti_connect/app/policies/ti_connect/connector_policy.rb
No issues — clean diff.
engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb
engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb:11-15
nitpick: The agenda let! is dead code.
agenda is declared at file scope and never referenced in any it block or other let. The old version (create(:agenda, practitioner_id: doctor_account.id)) was likely the same, but the new version is more involved (fetches the auto-generated agenda from the :doctor trait, then update!s its practitioner_id back to itself which is a no-op since the trait already wired it via agenda_access). Consider deleting it entirely.
engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb:7-10
question: Reliance on Organization.sole is brittle.
This implicitly assumes that the only organization in the DB is the one auto-created by the :doctor trait. If a future trait change creates a second organization (e.g. someone adds a default org elsewhere), sole will raise ActiveRecord::SoleRecordExceeded and every test in the file will fail with a confusing error. Prefer naming the org explicitly, e.g. doctor_account.managed_organizations.first or make the :doctor factory invocation explicit about its organization. Same comment for patient_base line 8-10.
engines/ti_connect/test/controllers/ti_connect/organizations/connector_configurations_controller_test.rb:131-133
nitpick: Shadowed patient_base in #create.
Line 8 declares a file-wide patient_base (the org's PB), and line 133 redeclares it as create(:patient_base) (a different org's PB). The redefinition is intentional but easy to miss; consider renaming to create_patient_base or removing the file-wide one if not needed by the create describe.
engines/ti_connect/test/controllers/ti_connect/practices/connector_configurations_controller_test.rb
engines/ti_connect/test/controllers/ti_connect/practices/connector_configurations_controller_test.rb:28-32
issue: Lost test coverage for before block reuse.
Before this PR, describe 'when user is admin of orga of the practice' had before do ... add_as_admin_for ... login_session ... end. The PR moved the calls inline into the single it. With only one it today this is functionally equivalent, but if anyone adds a second it to that describe block they will silently get an unauthenticated session. Suggest reverting to a before block so any future test inherits the setup.
engines/ti_connect/test/controllers/ti_connect/practices/connector_configurations_controller_test.rb:17-26
issue: Missing regression test for the policy-class change.
The PR switches this controller from `ConnectorPo
…(truncated, 5926 more chars)