From 4c54ec0ee0a330d7d41802bed4a846520f2cd771 Mon Sep 17 00:00:00 2001 From: Zhaochen Li Date: Wed, 22 Oct 2025 11:35:35 +1100 Subject: [PATCH] Clean up FF deprecate_approver_and_approver_group Clean up FF deprecate_approver_and_approver_group and related code, unit tests FF has been 100% enabled on prod with no issues Changelog: removed --- .../deprecate_approver_and_approver_group.yml | 10 ----- .../presenters/ee/merge_request_presenter.rb | 10 ++--- ee/app/presenters/ee/project_presenter.rb | 10 ++--- ee/lib/ee/api/entities/approval_settings.rb | 22 +++------- .../presenters/ee/project_presenter_spec.rb | 44 +++---------------- .../merge_request_presenter_spec.rb | 44 +++---------------- .../requests/api/project_approvals_spec.rb | 30 +++---------- 7 files changed, 31 insertions(+), 139 deletions(-) delete mode 100644 config/feature_flags/gitlab_com_derisk/deprecate_approver_and_approver_group.yml diff --git a/config/feature_flags/gitlab_com_derisk/deprecate_approver_and_approver_group.yml b/config/feature_flags/gitlab_com_derisk/deprecate_approver_and_approver_group.yml deleted file mode 100644 index c82ddf0f178e2f..00000000000000 --- a/config/feature_flags/gitlab_com_derisk/deprecate_approver_and_approver_group.yml +++ /dev/null @@ -1,10 +0,0 @@ ---- -name: deprecate_approver_and_approver_group -description: Deprecate approvers and approver_groups model and usage safely -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/573005 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/206807 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/573012 -milestone: '18.5' -group: group::code review -type: gitlab_com_derisk -default_enabled: false diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb index 3ba7d7b2831b58..a20bb63383c4c7 100644 --- a/ee/app/presenters/ee/merge_request_presenter.rb +++ b/ee/app/presenters/ee/merge_request_presenter.rb @@ -40,18 +40,16 @@ def code_owner_rules_with_users @code_owner_rules ||= merge_request.approval_rules.code_owner.with_users.to_a end + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 delegator_override :approver_groups def approver_groups - return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - - ::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user) + [] end + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 delegator_override :approvers def approvers - return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - - merge_request.approvers + [] end def suggested_approvers diff --git a/ee/app/presenters/ee/project_presenter.rb b/ee/app/presenters/ee/project_presenter.rb index 696beb482e1740..e4ebeb1484bb4f 100644 --- a/ee/app/presenters/ee/project_presenter.rb +++ b/ee/app/presenters/ee/project_presenter.rb @@ -6,18 +6,16 @@ module ProjectPresenter extend ::Gitlab::Utils::DelegatorOverride include SafeFormatHelper + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 delegator_override :approver_groups def approver_groups - return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - - ::ApproverGroup.filtered_approver_groups(project.approver_groups, current_user) + [] end + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 delegator_override :approvers def approvers - return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - - project.approvers + [] end private diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb index e1b7e6a2da6911..cac686630fb3da 100644 --- a/ee/lib/ee/api/entities/approval_settings.rb +++ b/ee/lib/ee/api/entities/approval_settings.rb @@ -4,22 +4,12 @@ module EE module API module Entities class ApprovalSettings < Grape::Entity - expose :approvers, using: EE::API::Entities::Approver do |project| - if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - [] - else - # delegate to project presenter - project.approvers - end - end - expose :approver_groups, using: EE::API::Entities::ApproverGroup do |project| - if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) - [] - else - # delegate to project presenter - project.approver_groups - end - end + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 + # Always return empty array + expose :approvers, using: EE::API::Entities::Approver, proc: ->(_) { [] } + # Deprecated with epic https://gitlab.com/groups/gitlab-org/-/epics/19155 + # Always return empty array + expose :approver_groups, using: EE::API::Entities::ApproverGroup, proc: ->(_) { [] } expose :approvals_before_merge expose :reset_approvals_on_push diff --git a/ee/spec/presenters/ee/project_presenter_spec.rb b/ee/spec/presenters/ee/project_presenter_spec.rb index 74ad870dfd708e..f20f15707aad43 100644 --- a/ee/spec/presenters/ee/project_presenter_spec.rb +++ b/ee/spec/presenters/ee/project_presenter_spec.rb @@ -64,51 +64,19 @@ describe '#approvers' do let(:presenter) { described_class.new(root_project, current_user: user) } - let(:approvers) { [user] } - before do - allow(root_project).to receive(:approvers).and_return(approvers) - end - - context 'when deprecate_approver_and_approver_group FF set to true' do - it 'returns empty approvers' do - expect(presenter.approvers).to eq([]) - end - end - - context 'when deprecate_approver_and_approver_group FF set to false' do - before do - stub_feature_flags(deprecate_approver_and_approver_group: false) - end - - it 'returns expected approvers' do - expect(presenter.approvers).to eq(approvers) - end + it 'returns empty array for approvers' do + expect(root_project).not_to receive(:approvers) + expect(presenter.approvers).to eq([]) end end describe '#approver_groups' do let(:presenter) { described_class.new(root_project, current_user: user) } - let(:approver_groups) { [build_stubbed(:approver_group)] } - before do - allow(::ApproverGroup).to receive(:filtered_approver_groups).and_return(approver_groups) - end - - context 'when deprecate_approver_and_approver_group FF set to true' do - it 'returns empty approver_groups' do - expect(presenter.approver_groups).to eq([]) - end - end - - context 'when deprecate_approver_and_approver_group FF set to false' do - before do - stub_feature_flags(deprecate_approver_and_approver_group: false) - end - - it 'returns expected approver_groups' do - expect(presenter.approver_groups).to eq(approver_groups) - end + it 'returns empty array for approver_groups' do + expect(root_project).not_to receive(:approver_groups) + expect(presenter.approver_groups).to eq([]) end end end diff --git a/ee/spec/presenters/merge_request_presenter_spec.rb b/ee/spec/presenters/merge_request_presenter_spec.rb index c02fbd9e73b1ec..7d1c73c59b02d8 100644 --- a/ee/spec/presenters/merge_request_presenter_spec.rb +++ b/ee/spec/presenters/merge_request_presenter_spec.rb @@ -298,51 +298,19 @@ describe '#approvers' do let(:presenter) { described_class.new(merge_request, current_user: user) } - let(:approvers) { [user] } - before do - allow(merge_request).to receive(:approvers).and_return(approvers) - end - - context 'when deprecate_approver_and_approver_group FF set to true' do - it 'returns empty approvers' do - expect(presenter.approvers).to eq([]) - end - end - - context 'when deprecate_approver_and_approver_group FF set to false' do - before do - stub_feature_flags(deprecate_approver_and_approver_group: false) - end - - it 'returns expected approvers' do - expect(presenter.approvers).to eq(approvers) - end + it 'returns empty array for approvers' do + expect(merge_request).not_to receive(:approvers) + expect(presenter.approvers).to eq([]) end end describe '#approver_groups' do let(:presenter) { described_class.new(merge_request, current_user: user) } - let(:approver_groups) { [build_stubbed(:approver_group)] } - before do - allow(::ApproverGroup).to receive(:filtered_approver_groups).and_return(approver_groups) - end - - context 'when deprecate_approver_and_approver_group FF set to true' do - it 'returns empty approver_groups' do - expect(presenter.approver_groups).to eq([]) - end - end - - context 'when deprecate_approver_and_approver_group FF set to false' do - before do - stub_feature_flags(deprecate_approver_and_approver_group: false) - end - - it 'returns expected approver_groups' do - expect(presenter.approver_groups).to eq(approver_groups) - end + it 'returns empty array for approver_groups' do + expect(merge_request).not_to receive(:approver_groups) + expect(presenter.approver_groups).to eq([]) end end end diff --git a/ee/spec/requests/api/project_approvals_spec.rb b/ee/spec/requests/api/project_approvals_spec.rb index c6ebff1e7e9196..04f8496be9ee8c 100644 --- a/ee/spec/requests/api/project_approvals_spec.rb +++ b/ee/spec/requests/api/project_approvals_spec.rb @@ -39,13 +39,11 @@ end end - it 'only shows approver groups that are visible to the user' do - private_group = create(:group, :private) - create(:approval_project_rule, project: project, users: [approver], groups: [private_group]) - + it 'only returns empty approvers and approver groups' do get api(url, user) expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee') + expect(json_response["approvers"]).to be_empty expect(json_response["approver_groups"]).to be_empty end @@ -141,14 +139,12 @@ expect(json_response.symbolize_keys).to include(settings) end - it 'only shows approver groups that are visible to the current user' do - private_group = create(:group, :private) - project.approver_groups.create!(group: private_group) - + it 'only returns empty approvers and approver groups' do post api(url, current_user, admin_mode: admin_mode), params: { approvals_before_merge: 3 } expect(response).to match_response_schema('public_api/v4/project_approvers', dir: 'ee') - expect(json_response["approver_groups"].size).to eq(visible_approver_groups_count) + expect(json_response["approvers"]).to be_empty + expect(json_response["approver_groups"]).to be_empty end end end @@ -211,7 +207,6 @@ context 'as a project admin' do it_behaves_like 'a user with access' do let(:current_user) { user } - let(:visible_approver_groups_count) { 0 } end end @@ -221,7 +216,6 @@ it_behaves_like 'a user with access' do let(:current_user) { admin } let(:admin_mode) { true } - let(:visible_approver_groups_count) { 0 } end context 'updates merge requests settings' do @@ -248,20 +242,6 @@ end end - context 'as a global admin and deprecate_approver_and_approver_group FF turned off' do - let_it_be(:admin_mode) { true } - - before do - stub_feature_flags(deprecate_approver_and_approver_group: false) - end - - it_behaves_like 'a user with access' do - let(:current_user) { admin } - let(:admin_mode) { true } - let(:visible_approver_groups_count) { 1 } - end - end - context 'as a user without access' do it 'returns 403' do post api(url, user2), params: { approvals_before_merge: 4 } -- GitLab