Claude comment — F3 [MEDIUM]: policy_prevents_editing_approval_rules? should be private

This method is only called internally by disable_overriding_approvers_per_merge_request and should not be part of Project's public API. Keeping it public makes it harder to change the signature later and leaks implementation details.

Fix:

private :policy_prevents_editing_approval_rules?

Claude comment — F2 [HIGH]: type_approval_policy guard in scope is untested

All four test policies are created with the default :security_policy factory which is type_approval_policy. There is no non-approval-type policy (e.g. :scan_execution_policy) with prevent_editing_approval_rules: true that should be excluded. Removing type_approval_policy from the scope would not break this test.

Fix: Add a policy of a different type and assert it is excluded:

let_it_be(:policy_e) do
  create(:security_policy, :scan_execution_policy,
    content: { approval_settings: { prevent_editing_approval_rules: true } })
end
# policy_e should NOT appear in the result
it { is_expected.to contain_exactly(policy_a) }

Claude comment — F1 [HIGH]: policy_prevents_editing_approval_rules? fires a DB query on every call when memoized value is false

|| policy_prevents_editing_approval_rules? is outside strong_memoize, so after the block caches false, every subsequent call to disable_overriding_approvers_per_merge_request still executes the approval_policies.prevent_editing_approval_rules.without_warn_mode.exists? DB query. For most projects (compliance allows overrides → memoized false), this fires on every MR widget load and approval check within the request.

Fix: Memoize policy_prevents_editing_approval_rules? independently:

def policy_prevents_editing_approval_rules?
  return false unless licensed_feature_available?(:security_orchestration_policies)
  approval_policies.prevent_editing_approval_rules.without_warn_mode.exists?
end
strong_memoize_attr :policy_prevents_editing_approval_rules?

Artur Fedorov (96629c4a) at 18 Mar 22:23