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 new file mode 100644 index 0000000000000000000000000000000000000000..c82ddf0f178e2f09c977eecf24df64088d94e5bc --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/deprecate_approver_and_approver_group.yml @@ -0,0 +1,10 @@ +--- +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 a1c63207ca0fc2aeb48283af41a86cd72cd95a0a..3ba7d7b2831b58df47db9d2178d59b5779629551 100644 --- a/ee/app/presenters/ee/merge_request_presenter.rb +++ b/ee/app/presenters/ee/merge_request_presenter.rb @@ -42,9 +42,18 @@ def code_owner_rules_with_users 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 + delegator_override :approvers + def approvers + return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) + + merge_request.approvers + end + def suggested_approvers merge_request.approval_state.suggested_approvers(current_user: current_user) end diff --git a/ee/app/presenters/ee/project_presenter.rb b/ee/app/presenters/ee/project_presenter.rb index e2d4c719733f32bca5556d997c43e4209f75e078..696beb482e1740c4913f4ab294fad45ca5c2469d 100644 --- a/ee/app/presenters/ee/project_presenter.rb +++ b/ee/app/presenters/ee/project_presenter.rb @@ -8,9 +8,18 @@ module ProjectPresenter 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 + delegator_override :approvers + def approvers + return [] if ::Feature.enabled?(:deprecate_approver_and_approver_group, project) + + project.approvers + end + private override :storage_anchor_text diff --git a/ee/lib/ee/api/entities/approval_settings.rb b/ee/lib/ee/api/entities/approval_settings.rb index d392c28940f6f2d3a5faf70a7590abee96b170ee..e1b7e6a2da691119e800cfcb8488c3c9ab7a71c3 100644 --- a/ee/lib/ee/api/entities/approval_settings.rb +++ b/ee/lib/ee/api/entities/approval_settings.rb @@ -4,8 +4,22 @@ module EE module API module Entities class ApprovalSettings < Grape::Entity - expose :approvers, using: EE::API::Entities::Approver - expose :approver_groups, using: EE::API::Entities::ApproverGroup + 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 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 e566ce151426049def45ad5e38267c77e5e0340b..74ad870dfd708e06edf9b466235e81ee6afd6459 100644 --- a/ee/spec/presenters/ee/project_presenter_spec.rb +++ b/ee/spec/presenters/ee/project_presenter_spec.rb @@ -61,4 +61,54 @@ end end end + + 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 + 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 + end + end end diff --git a/ee/spec/presenters/merge_request_presenter_spec.rb b/ee/spec/presenters/merge_request_presenter_spec.rb index 7d91b6d90e1d59955182faee24864d17a22dfb24..c02fbd9e73b1ecd7b2181b06b0c19a0de0ec52f4 100644 --- a/ee/spec/presenters/merge_request_presenter_spec.rb +++ b/ee/spec/presenters/merge_request_presenter_spec.rb @@ -295,4 +295,54 @@ expect(presenter.require_saml_auth_to_approve).to be true end end + + 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 + 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 + end + end end diff --git a/ee/spec/requests/api/project_approvals_spec.rb b/ee/spec/requests/api/project_approvals_spec.rb index b392efe7ff4cc180d76b4fa4a025eca1e70aed73..c6ebff1e7e91961a0e719dea1ea750a947c7d29a 100644 --- a/ee/spec/requests/api/project_approvals_spec.rb +++ b/ee/spec/requests/api/project_approvals_spec.rb @@ -221,7 +221,7 @@ it_behaves_like 'a user with access' do let(:current_user) { admin } let(:admin_mode) { true } - let(:visible_approver_groups_count) { 1 } + let(:visible_approver_groups_count) { 0 } end context 'updates merge requests settings' do @@ -248,6 +248,20 @@ 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 }