Commit b9606f07 authored by Paul Slaughter's avatar Paul Slaughter

Merge branch '343872-remove-group-merge-request-approval-settings-feature-flag' into 'master'

Remove feature flag `group_merge_request_approval_settings_feature_flag`

See merge request gitlab-org/gitlab!81252
parents b1feb9ae ef2be582
...@@ -806,9 +806,7 @@ The group's new subgroups have push rules set for them based on either: ...@@ -806,9 +806,7 @@ The group's new subgroups have push rules set for them based on either:
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285458) in GitLab 13.9. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../administration/feature_flags.md), disabled by default. > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285458) in GitLab 13.9. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../administration/feature_flags.md), disabled by default.
> - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.5. > - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.5.
> - [Feature flag `group_merge_request_approval_settings_feature_flag`](https://gitlab.com/gitlab-org/gitlab/-/issues/343872) removed in GitLab 14.9.
FLAG:
On self-managed GitLab, by default this feature is available. To hide the feature per group, ask an administrator to [disable the feature flag](../../administration/feature_flags.md) named `group_merge_request_approval_settings_feature_flag`. On GitLab.com, this feature is available.
Group approval rules manage [project merge request approval rules](../project/merge_requests/approvals/index.md) Group approval rules manage [project merge request approval rules](../project/merge_requests/approvals/index.md)
at the top-level group level. These rules [cascade to all projects](../project/merge_requests/approvals/settings.md#settings-cascading) at the top-level group level. These rules [cascade to all projects](../project/merge_requests/approvals/settings.md#settings-cascading)
......
...@@ -140,9 +140,7 @@ To learn more, see [Coverage check approval rule](../../../../ci/pipelines/setti ...@@ -140,9 +140,7 @@ To learn more, see [Coverage check approval rule](../../../../ci/pipelines/setti
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.4. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md), disabled by default. > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.4. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md), disabled by default.
> - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.5. > - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.5.
> - [Feature flag `group_merge_request_approval_settings_feature_flag`](https://gitlab.com/gitlab-org/gitlab/-/issues/343872) removed in GitLab 14.9.
FLAG:
On self-managed GitLab, by default this feature is available. To hide the feature per group, ask an administrator to [disable the feature flag](../../../../administration/feature_flags.md) named `group_merge_request_approval_settings_feature_flag`. On GitLab.com, this feature is available.
You can also enforce merge request approval settings: You can also enforce merge request approval settings:
......
...@@ -123,20 +123,3 @@ export const mergeRequestApprovalSettingsMappers = { ...@@ -123,20 +123,3 @@ export const mergeRequestApprovalSettingsMappers = {
allow_committer_approval: !settings.preventCommittersApproval.value, allow_committer_approval: !settings.preventCommittersApproval.value,
}), }),
}; };
export const projectApprovalsMappers = {
mapDataToState: (data) => ({
preventAuthorApproval: { value: !data.merge_requests_author_approval },
preventMrApprovalRuleEdit: { value: data.disable_overriding_approvers_per_merge_request },
requireUserPassword: { value: data.require_password_to_approve },
removeApprovalsOnPush: { value: data.reset_approvals_on_push },
preventCommittersApproval: { value: data.merge_requests_disable_committers_approval },
}),
mapStateToPayload: ({ settings }) => ({
merge_requests_author_approval: !settings.preventAuthorApproval.value,
disable_overriding_approvers_per_merge_request: settings.preventMrApprovalRuleEdit.value,
require_password_to_approve: settings.requireUserPassword.value,
reset_approvals_on_push: settings.removeApprovalsOnPush.value,
merge_requests_disable_committers_approval: settings.preventCommittersApproval.value,
}),
};
...@@ -2,7 +2,7 @@ import Vue from 'vue'; ...@@ -2,7 +2,7 @@ import Vue from 'vue';
import { GlToast } from '@gitlab/ui'; import { GlToast } from '@gitlab/ui';
import { parseBoolean } from '~/lib/utils/common_utils'; import { parseBoolean } from '~/lib/utils/common_utils';
import ProjectSettingsApp from './components/project_settings/app.vue'; import ProjectSettingsApp from './components/project_settings/app.vue';
import { projectApprovalsMappers, mergeRequestApprovalSettingsMappers } from './mappers'; import { mergeRequestApprovalSettingsMappers } from './mappers';
import createStore from './stores'; import createStore from './stores';
import approvalSettingsModule from './stores/modules/approval_settings'; import approvalSettingsModule from './stores/modules/approval_settings';
import projectSettingsModule from './stores/modules/project_settings'; import projectSettingsModule from './stores/modules/project_settings';
...@@ -22,14 +22,7 @@ export default function mountProjectSettingsApprovals(el) { ...@@ -22,14 +22,7 @@ export default function mountProjectSettingsApprovals(el) {
approvals: projectSettingsModule(), approvals: projectSettingsModule(),
}; };
if (gon.features.groupMergeRequestApprovalSettingsFeatureFlag) {
modules.approvalSettings = approvalSettingsModule(mergeRequestApprovalSettingsMappers); modules.approvalSettings = approvalSettingsModule(mergeRequestApprovalSettingsMappers);
} else {
modules.approvalSettings = approvalSettingsModule({
updateMethod: 'post',
...projectApprovalsMappers,
});
}
const store = createStore(modules, { const store = createStore(modules, {
...el.dataset, ...el.dataset,
......
...@@ -13,7 +13,6 @@ module EE ...@@ -13,7 +13,6 @@ module EE
before_action :log_unarchive_audit_event, only: [:unarchive] before_action :log_unarchive_audit_event, only: [:unarchive]
before_action only: :edit do before_action only: :edit do
push_frontend_feature_flag(:group_merge_request_approval_settings_feature_flag, project.root_ancestor, default_enabled: :yaml)
push_frontend_feature_flag(:permit_all_shared_groups_for_approval, project, default_enabled: :yaml) push_frontend_feature_flag(:permit_all_shared_groups_for_approval, project, default_enabled: :yaml)
end end
......
...@@ -64,7 +64,7 @@ module EE ...@@ -64,7 +64,7 @@ module EE
can_modify_commiter_settings: can_modify_commiter_settings.to_s, can_modify_commiter_settings: can_modify_commiter_settings.to_s,
project_path: expose_path(api_v4_projects_path(id: project.id)), project_path: expose_path(api_v4_projects_path(id: project.id)),
settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)), settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)), approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)), rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
allow_multi_rule: project.multiple_approval_rules_available?.to_s, allow_multi_rule: project.multiple_approval_rules_available?.to_s,
eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'), eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
...@@ -72,13 +72,9 @@ module EE ...@@ -72,13 +72,9 @@ module EE
security_configuration_path: project_security_configuration_path(project), security_configuration_path: project_security_configuration_path(project),
vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'), vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'), license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule') coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule'),
}.tap do |data| group_name: project.root_ancestor.name
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, project.root_ancestor, default_enabled: :yaml) }
data[:approvals_path] = expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id))
data[:group_name] = project.root_ancestor.name
end
end
end end
def status_checks_app_data(project) def status_checks_app_data(project)
......
...@@ -3,7 +3,6 @@ ...@@ -3,7 +3,6 @@
module Groups module Groups
module MergeRequestApprovalSettingsHelper module MergeRequestApprovalSettingsHelper
def show_merge_request_approval_settings?(user, group) def show_merge_request_approval_settings?(user, group)
Feature.enabled?(:group_merge_request_approval_settings_feature_flag, group, default_enabled: :yaml) &&
user.can?(:admin_merge_request_approval_settings, group) user.can?(:admin_merge_request_approval_settings, group)
end end
end end
......
...@@ -543,13 +543,9 @@ module EE ...@@ -543,13 +543,9 @@ module EE
end end
def reset_approvals_on_push def reset_approvals_on_push
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self.group&.root_ancestor, default_enabled: :yaml)
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self) !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
.retain_approvals_on_push .retain_approvals_on_push
.value && feature_available?(:merge_request_approvers) .value && feature_available?(:merge_request_approvers)
else
super && feature_available?(:merge_request_approvers)
end
end end
alias_method :reset_approvals_on_push?, :reset_approvals_on_push alias_method :reset_approvals_on_push?, :reset_approvals_on_push
...@@ -577,13 +573,9 @@ module EE ...@@ -577,13 +573,9 @@ module EE
end end
def require_password_to_approve def require_password_to_approve
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self&.group&.root_ancestor, default_enabled: :yaml)
ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self) ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
.require_password_to_approve .require_password_to_approve
.value && password_authentication_enabled_for_web? .value && password_authentication_enabled_for_web?
else
super && password_authentication_enabled_for_web?
end
end end
def require_password_to_approve? def require_password_to_approve?
...@@ -750,17 +742,11 @@ module EE ...@@ -750,17 +742,11 @@ module EE
def disable_overriding_approvers_per_merge_request def disable_overriding_approvers_per_merge_request
strong_memoize(:disable_overriding_approvers_per_merge_request) do strong_memoize(:disable_overriding_approvers_per_merge_request) do
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self, default_enabled: :yaml)
super unless feature_available?(:admin_merge_request_approvers_rules) super unless feature_available?(:admin_merge_request_approvers_rules)
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self) !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_overrides_to_approver_list_per_merge_request .allow_overrides_to_approver_list_per_merge_request
.value .value
else
next super unless feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
end
end end
end end
...@@ -770,18 +756,11 @@ module EE ...@@ -770,18 +756,11 @@ module EE
def merge_requests_author_approval def merge_requests_author_approval
strong_memoize(:merge_requests_author_approval) do strong_memoize(:merge_requests_author_approval) do
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self, default_enabled: :yaml)
super unless feature_available?(:admin_merge_request_approvers_rules) super unless feature_available?(:admin_merge_request_approvers_rules)
ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self) ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_author_approval .allow_author_approval
.value .value
else
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
!!read_attribute(:merge_requests_author_approval)
end
end end
end end
...@@ -791,17 +770,11 @@ module EE ...@@ -791,17 +770,11 @@ module EE
def merge_requests_disable_committers_approval def merge_requests_disable_committers_approval
strong_memoize(:merge_requests_disable_committers_approval) do strong_memoize(:merge_requests_disable_committers_approval) do
if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self, default_enabled: :yaml)
super unless feature_available?(:admin_merge_request_approvers_rules) super unless feature_available?(:admin_merge_request_approvers_rules)
!ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self) !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
.allow_committer_approval .allow_committer_approval
.value .value
else
next super unless License.feature_available?(:admin_merge_request_approvers_rules)
::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
end
end end
end end
......
---
name: group_merge_request_approval_settings_feature_flag
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50256
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/247921
milestone: '13.8'
type: development
group: group::compliance
default_enabled: true
...@@ -35,19 +35,15 @@ module API ...@@ -35,19 +35,15 @@ module API
end end
segment ':id/merge_request_approval_setting' do segment ':id/merge_request_approval_setting' do
desc 'Get project-level MR approval settings' do desc 'Get project-level MR approval settings' do
detail 'This feature was introduced in 14.3 behind the :group_merge_request_approval_settings_feature_flag'
success EE::API::Entities::MergeRequestApprovalSettings success EE::API::Entities::MergeRequestApprovalSettings
end end
get '/', urgency: :medium do get '/', urgency: :medium do
not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_project.root_ancestor, default_enabled: :yaml)
group = user_project.group.present? ? user_project.root_ancestor : nil group = user_project.group.present? ? user_project.root_ancestor : nil
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute
present setting, with: ::API::Entities::MergeRequestApprovalSetting present setting, with: ::API::Entities::MergeRequestApprovalSetting
end end
desc 'Update existing merge request approval setting' do desc 'Update existing merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting success ::API::Entities::MergeRequestApprovalSetting
end end
params do params do
...@@ -77,13 +73,10 @@ module API ...@@ -77,13 +73,10 @@ module API
end end
resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
before do before do
not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group, default_enabled: :yaml)
authorize! :admin_merge_request_approval_settings, user_group authorize! :admin_merge_request_approval_settings, user_group
end end
segment ':id/merge_request_approval_setting' do segment ':id/merge_request_approval_setting' do
desc 'Get group merge request approval setting' do desc 'Get group merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting success ::API::Entities::MergeRequestApprovalSetting
end end
get do get do
...@@ -93,7 +86,6 @@ module API ...@@ -93,7 +86,6 @@ module API
end end
desc 'Update existing merge request approval setting' do desc 'Update existing merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting success ::API::Entities::MergeRequestApprovalSetting
end end
params do params do
......
...@@ -319,9 +319,8 @@ RSpec.describe 'Edit group settings' do ...@@ -319,9 +319,8 @@ RSpec.describe 'Edit group settings' do
create(:group_merge_request_approval_setting, group: group, allow_author_approval: false) create(:group_merge_request_approval_setting, group: group, allow_author_approval: false)
end end
context 'when feature flag is enabled and group is licensed' do context 'when group is licensed' do
before do before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(merge_request_approvers: true) stub_licensed_features(merge_request_approvers: true)
end end
...@@ -352,9 +351,8 @@ RSpec.describe 'Edit group settings' do ...@@ -352,9 +351,8 @@ RSpec.describe 'Edit group settings' do
end end
end end
context 'when feature flag is disabled and group is not licensed' do context 'when group is not licensed' do
before do before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
stub_licensed_features(merge_request_approvers: false) stub_licensed_features(merge_request_approvers: false)
end end
......
...@@ -133,13 +133,8 @@ RSpec.describe 'Projects > Audit Events', :js do ...@@ -133,13 +133,8 @@ RSpec.describe 'Projects > Audit Events', :js do
end end
end end
context 'with the group_merge_request_approval_settings feature' do
where(feature_enabled: [true, false])
with_them do
describe 'changing merge request approval permission for authors and reviewers' do describe 'changing merge request approval permission for authors and reviewers' do
before do before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: feature_enabled)
stub_licensed_features(merge_request_approvers: true) stub_licensed_features(merge_request_approvers: true)
project.add_developer(pete) project.add_developer(pete)
end end
...@@ -173,8 +168,6 @@ RSpec.describe 'Projects > Audit Events', :js do ...@@ -173,8 +168,6 @@ RSpec.describe 'Projects > Audit Events', :js do
end end
end end
end end
end
end
describe 'filter by date' do describe 'filter by date' do
let!(:audit_event_1) { create(:project_audit_event, entity_type: 'Project', entity_id: project.id, created_at: 5.days.ago) } let!(:audit_event_1) { create(:project_audit_event, entity_type: 'Project', entity_id: project.id, created_at: 5.days.ago) }
......
...@@ -4,7 +4,7 @@ import Vuex from 'vuex'; ...@@ -4,7 +4,7 @@ import Vuex from 'vuex';
import ApprovalSettings from 'ee/approvals/components/approval_settings.vue'; import ApprovalSettings from 'ee/approvals/components/approval_settings.vue';
import ProjectApprovalSettings from 'ee/approvals/components/project_settings/project_approval_settings.vue'; import ProjectApprovalSettings from 'ee/approvals/components/project_settings/project_approval_settings.vue';
import { PROJECT_APPROVAL_SETTINGS_LABELS_I18N } from 'ee/approvals/constants'; import { PROJECT_APPROVAL_SETTINGS_LABELS_I18N } from 'ee/approvals/constants';
import { projectApprovalsMappers } from 'ee/approvals/mappers'; import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
import createStore from 'ee/approvals/stores'; import createStore from 'ee/approvals/stores';
import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings'; import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings';
...@@ -18,7 +18,7 @@ describe('ProjectApprovalSettings', () => { ...@@ -18,7 +18,7 @@ describe('ProjectApprovalSettings', () => {
const setupStore = (data = {}) => { const setupStore = (data = {}) => {
store = createStore({ store = createStore({
approvalSettings: approvalSettingsModule(projectApprovalsMappers), approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
}); });
store.state.settings = data; store.state.settings = data;
......
...@@ -430,18 +430,13 @@ RSpec.describe ProjectsHelper do ...@@ -430,18 +430,13 @@ RSpec.describe ProjectsHelper do
allow(helper).to receive(:can?).and_return(true) allow(helper).to receive(:can?).and_return(true)
end end
context 'with group_merge_request_approval_settings_feature_flag disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns the correct data' do it 'returns the correct data' do
expect(subject).to eq({ expect(subject).to include(
project_id: project.id, project_id: project.id,
can_edit: 'true', can_edit: 'true',
can_modify_author_settings: 'true', can_modify_author_settings: 'true',
can_modify_commiter_settings: 'true', can_modify_commiter_settings: 'true',
approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)), approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
project_path: expose_path(api_v4_projects_path(id: project.id)), project_path: expose_path(api_v4_projects_path(id: project.id)),
settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)), settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)), rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
...@@ -451,24 +446,11 @@ RSpec.describe ProjectsHelper do ...@@ -451,24 +446,11 @@ RSpec.describe ProjectsHelper do
security_configuration_path: project_security_configuration_path(project), security_configuration_path: project_security_configuration_path(project),
vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'), vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'), license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule') coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule'),
})
end
end
context 'with group_merge_request_approval_settings_feature_flag enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns the correct data' do
expect(subject).to include(
approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
group_name: project.root_ancestor.name group_name: project.root_ancestor.name
) )
end end
end end
end
describe '#status_checks_app_data' do describe '#status_checks_app_data' do
subject { helper.status_checks_app_data(project) } subject { helper.status_checks_app_data(project) }
......
...@@ -819,10 +819,6 @@ RSpec.describe ApprovalState do ...@@ -819,10 +819,6 @@ RSpec.describe ApprovalState do
end end
describe '#authors_can_approve?' do describe '#authors_can_approve?' do
context 'group_merge_request_approval_settings_feature_flag is enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
context 'allow_author_approval is resolved to not be permitted' do context 'allow_author_approval is resolved to not be permitted' do
before do before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance| allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
...@@ -852,37 +848,7 @@ RSpec.describe ApprovalState do ...@@ -852,37 +848,7 @@ RSpec.describe ApprovalState do
end end
end end
context 'group_merge_request_approval_settings_feature_flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
context 'when project allows author approval' do
before do
project.update!(merge_requests_author_approval: true)
end
it 'returns true' do
expect(subject.authors_can_approve?).to eq(true)
end
end
context 'when project disallows author approval' do
before do
project.update!(merge_requests_author_approval: false)
end
it 'returns true' do
expect(subject.authors_can_approve?).to eq(false)
end
end
end
end
describe '#committers_can_approve?' do describe '#committers_can_approve?' do
context 'group_merge_request_approval_settings_feature_flag is enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
context 'allow_committer_approval is resolved to not be permitted' do context 'allow_committer_approval is resolved to not be permitted' do
before do before do
allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance| allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
...@@ -912,32 +878,6 @@ RSpec.describe ApprovalState do ...@@ -912,32 +878,6 @@ RSpec.describe ApprovalState do
end end
end end
context 'group_merge_request_approval_settings_feature_flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
context 'when project allows committer approval' do
before do
project.update!(merge_requests_disable_committers_approval: false)
end
it 'returns true' do
expect(subject.committers_can_approve?).to eq(true)
end
end
context 'when project disallows committer approval' do
before do
project.update!(merge_requests_disable_committers_approval: true)
end
it 'returns true' do
expect(subject.committers_can_approve?).to eq(false)
end
end
end
end
describe '#suggested_approvers' do describe '#suggested_approvers' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:public_group) { create(:group, :public) } let(:public_group) { create(:group, :public) }
......
...@@ -754,22 +754,6 @@ RSpec.describe Project do ...@@ -754,22 +754,6 @@ RSpec.describe Project do
end end
describe '#disable_overriding_approvers_per_merge_request' do describe '#disable_overriding_approvers_per_merge_request' do
context 'when group_merge_request_approval_settings_feature_flag flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it_behaves_like 'setting modified by application setting' do
let(:setting) { :disable_overriding_approvers_per_merge_request }
let(:application_setting) { :disable_overriding_approvers_per_merge_request }
end
end
context 'when group_merge_request_approval_settings_feature_flag flag is true' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns false when the resolver returns true' do it 'returns false when the resolver returns true' do
allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
allow(resolver).to receive(:allow_overrides_to_approver_list_per_merge_request) allow(resolver).to receive(:allow_overrides_to_approver_list_per_merge_request)
...@@ -792,7 +776,6 @@ RSpec.describe Project do ...@@ -792,7 +776,6 @@ RSpec.describe Project do
expect(project.disable_overriding_approvers_per_merge_request).to be true expect(project.disable_overriding_approvers_per_merge_request).to be true
end end
end end
end
shared_examples 'a predicate wrapper method' do shared_examples 'a predicate wrapper method' do
where(:wrapped_method_return, :subject_return) do where(:wrapped_method_return, :subject_return) do
...@@ -819,22 +802,6 @@ RSpec.describe Project do ...@@ -819,22 +802,6 @@ RSpec.describe Project do
end end
describe '#merge_requests_disable_committers_approval' do describe '#merge_requests_disable_committers_approval' do
context 'when group_merge_request_approval_settings_feature_flag flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it_behaves_like 'setting modified by application setting' do
let(:setting) { :merge_requests_disable_committers_approval }
let(:application_setting) { :prevent_merge_requests_committers_approval }
end
end
context 'when group_merge_request_approval_settings_feature_flag flag is true' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns false when the resolver returns true' do it 'returns false when the resolver returns true' do
allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
allow(resolver).to receive(:allow_committer_approval) allow(resolver).to receive(:allow_committer_approval)
...@@ -857,7 +824,6 @@ RSpec.describe Project do ...@@ -857,7 +824,6 @@ RSpec.describe Project do
expect(project.merge_requests_disable_committers_approval).to be true expect(project.merge_requests_disable_committers_approval).to be true
end end
end end
end
describe '#merge_requests_disable_committers_approval?' do describe '#merge_requests_disable_committers_approval?' do
it_behaves_like 'a predicate wrapper method' do it_behaves_like 'a predicate wrapper method' do
...@@ -866,17 +832,6 @@ RSpec.describe Project do ...@@ -866,17 +832,6 @@ RSpec.describe Project do
end end
describe '#require_password_to_approve?' do describe '#require_password_to_approve?' do
context 'when group_merge_request_approval_settings_feature_flag flag is disabled' do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
it_behaves_like 'a predicate wrapper method' do
let(:wrapped_method) { :require_password_to_approve }
end
end
context 'when group_merge_request_approval_settings_feature_flag flag is enabled' do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
it 'returns true when the resolver returns true' do it 'returns true when the resolver returns true' do
allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
allow(resolver).to receive(:require_password_to_approve) allow(resolver).to receive(:require_password_to_approve)
...@@ -899,17 +854,11 @@ RSpec.describe Project do ...@@ -899,17 +854,11 @@ RSpec.describe Project do
expect(project.require_password_to_approve).to be false expect(project.require_password_to_approve).to be false
end end
end end
end
describe '#merge_requests_author_approval' do describe '#merge_requests_author_approval' do
context 'when group_merge_request_approval_settings_feature_flag flag is enabled' do
let(:setting) { :merge_requests_author_approval } let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval } let(:application_setting) { :prevent_merge_requests_author_approval }
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns true when the resolver returns true' do it 'returns true when the resolver returns true' do
allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
allow(resolver).to receive(:allow_author_approval) allow(resolver).to receive(:allow_author_approval)
...@@ -933,39 +882,6 @@ RSpec.describe Project do ...@@ -933,39 +882,6 @@ RSpec.describe Project do
end end
end end
context 'when flag is disabled' do
let(:setting) { :merge_requests_author_approval }
let(:application_setting) { :prevent_merge_requests_author_approval }
where(:feature_enabled, :app_setting, :project_setting, :final_setting) do
true | true | true | false
true | false | true | true
true | true | false | false
true | false | false | false
false | true | true | true
false | false | true | true
false | true | false | false
false | false | false | false
end
with_them do
let(:project) { create(:project) }
before do
stub_licensed_features(admin_merge_request_approvers_rules: feature_enabled)
stub_application_setting(application_setting => app_setting)
project.update!(setting => project_setting)
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'shows proper setting' do
expect(project.send(setting)).to eq(final_setting)
expect(project.send("#{setting}?")).to eq(final_setting)
end
end
end
end
describe '#merge_requests_author_approval?' do describe '#merge_requests_author_approval?' do
it_behaves_like 'a predicate wrapper method' do it_behaves_like 'a predicate wrapper method' do
let(:wrapped_method) { :merge_requests_author_approval } let(:wrapped_method) { :merge_requests_author_approval }
...@@ -1510,33 +1426,6 @@ RSpec.describe Project do ...@@ -1510,33 +1426,6 @@ RSpec.describe Project do
end end
describe "#reset_approvals_on_push?" do describe "#reset_approvals_on_push?" do
context 'when group_merge_request_approval_settings_feature_flag flag is disabled' do
where(:license_value, :db_value, :expected) do
true | true | true
true | false | false
false | true | false
false | false | false
end
with_them do
let(:project) { build(:project, reset_approvals_on_push: db_value) }
subject { project.reset_approvals_on_push? }
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
stub_licensed_features(merge_request_approvers: license_value)
end
it { is_expected.to eq(expected) }
end
end
context 'when group_merge_request_approval_settings_feature_flag flag is enabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
it 'returns false when the resolver returns true' do it 'returns false when the resolver returns true' do
allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver| allow_next_instance_of(ComplianceManagement::MergeRequestApprovalSettings::Resolver) do |resolver|
allow(resolver).to receive(:retain_approvals_on_push) allow(resolver).to receive(:retain_approvals_on_push)
...@@ -1559,7 +1448,6 @@ RSpec.describe Project do ...@@ -1559,7 +1448,6 @@ RSpec.describe Project do
expect(project.reset_approvals_on_push).to be true expect(project.reset_approvals_on_push).to be true
end end
end end
end
describe '#approvals_before_merge' do describe '#approvals_before_merge' do
where(:license_value, :db_value, :expected) do where(:license_value, :db_value, :expected) do
......
...@@ -45,26 +45,9 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -45,26 +45,9 @@ RSpec.describe API::MergeRequestApprovalSettings do
describe 'GET /groups/:id/merge_request_approval_settings' do describe 'GET /groups/:id/merge_request_approval_settings' do
let(:url) { "/groups/#{group.id}/merge_request_approval_setting" } let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
get api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
before do
allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end
context 'when the user is authorised' do context 'when the user is authorised' do
before do before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?) allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, group) .with(user, :admin_merge_request_approval_settings, group)
.and_return(true) .and_return(true)
...@@ -113,7 +96,6 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -113,7 +96,6 @@ RSpec.describe API::MergeRequestApprovalSettings do
expect(json_response['require_password_to_approve']['value']).to eq(false) expect(json_response['require_password_to_approve']['value']).to eq(false)
end end
end end
end
context 'when the user is not authorised' do context 'when the user is not authorised' do
before do before do
...@@ -135,22 +117,8 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -135,22 +117,8 @@ RSpec.describe API::MergeRequestApprovalSettings do
let(:url) { "/groups/#{group.id}/merge_request_approval_setting" } let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
let(:params) { { allow_author_approval: true } } let(:params) { { allow_author_approval: true } }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
put api(url, user), params: params
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
before do before do
allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
end end
context 'when the user is authorised' do context 'when the user is authorised' do
...@@ -210,28 +178,13 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -210,28 +178,13 @@ RSpec.describe API::MergeRequestApprovalSettings do
end end
end end
end end
end
describe 'GET /projects/:id/merge_request_approval_settings' do describe 'GET /projects/:id/merge_request_approval_settings' do
let(:url) { "/projects/#{project.id}/merge_request_approval_setting" } let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
get api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
before do before do
group.add_owner(user) group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(merge_request_approvers: true) stub_licensed_features(merge_request_approvers: true)
allow(Ability).to receive(:allowed?) allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project) .with(user, :admin_merge_request_approval_settings, project)
...@@ -267,30 +220,14 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -267,30 +220,14 @@ RSpec.describe API::MergeRequestApprovalSettings do
end end
end end
end end
end
describe 'PUT /projects/:id/merge_request_approval_settings' do describe 'PUT /projects/:id/merge_request_approval_settings' do
let(:url) { "/projects/#{project.id}/merge_request_approval_setting" } let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
put api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
let(:params) { { retain_approvals_on_push: false } } let(:params) { { retain_approvals_on_push: false } }
before do before do
group.add_owner(user) group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(merge_request_approvers: true) stub_licensed_features(merge_request_approvers: true)
allow(Ability).to receive(:allowed?) allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project) .with(user, :admin_merge_request_approval_settings, project)
...@@ -305,5 +242,4 @@ RSpec.describe API::MergeRequestApprovalSettings do ...@@ -305,5 +242,4 @@ RSpec.describe API::MergeRequestApprovalSettings do
expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee')
end end
end end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment