diff --git a/app/models/project.rb b/app/models/project.rb index 85e9371191f759fab4ddc9a703beba3e94457fca..b31ca1c71ca6736b18d80e380c341dfa45acf27d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -750,16 +750,13 @@ def self.public_or_visible_to_user(user = nil, min_access_level = nil) end end - # Defines instance methods: + # Define two instance methods: # - # - only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: false) - # - allow_merge_on_skipped_pipeline?(inherit_group_setting: false) - # - only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: false) - # - only_allow_merge_if_pipeline_succeeds_locked? - # - allow_merge_on_skipped_pipeline_locked? - # - only_allow_merge_if_all_discussions_are_resolved_locked? + # - [attribute]?(inherit_group_setting) Returns the final value after inheriting the parent group + # - [attribute]_locked? Returns true if the value is inherited from the parent group + # + # These functions will be overridden in EE to make sense afterwards def self.cascading_with_parent_namespace(attribute) - # method overriden in EE define_method("#{attribute}?") do |inherit_group_setting: false| self.public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend end diff --git a/config/feature_flags/development/support_group_level_merge_checks_setting.yml b/config/feature_flags/development/support_group_level_merge_checks_setting.yml new file mode 100644 index 0000000000000000000000000000000000000000..66cb98302610b4280c4bc8acc7a22b48ed215786 --- /dev/null +++ b/config/feature_flags/development/support_group_level_merge_checks_setting.yml @@ -0,0 +1,8 @@ +--- +name: support_group_level_merge_checks_setting +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102864 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/377723 +milestone: '15.8' +type: development +group: group::compliance +default_enabled: false diff --git a/ee/app/controllers/groups/settings/merge_requests_controller.rb b/ee/app/controllers/groups/settings/merge_requests_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..060a59a464cb2b19c61e5fa946b8fedf1af8d7cb --- /dev/null +++ b/ee/app/controllers/groups/settings/merge_requests_controller.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Groups + module Settings + class MergeRequestsController < Groups::ApplicationController + layout 'group_settings' + + before_action :authorize_admin_group! + + feature_category :code_review + + def update + return render_404 unless @group.licensed_feature_available?(:group_level_merge_checks_setting) + + if Groups::UpdateService.new(@group, current_user, group_settings_params).execute + notice = format( + _("Group '%{group_name}' was successfully updated."), + group_name: @group.name + ) + redirect_to edit_group_path(@group, anchor: 'js-merge-requests-settings'), notice: notice + else + @group.reset + alert = @group.errors.full_messages.to_sentence.presence || format( + _("Group '%{group_name}' could not be updated."), + group_name: @group.name + ) + redirect_to edit_group_path(@group, anchor: 'js-merge-requests-settings'), alert: alert + end + end + + private + + def group_settings_params + params.require(:namespace_setting).permit( + %i[ + only_allow_merge_if_pipeline_succeeds + allow_merge_on_skipped_pipeline + only_allow_merge_if_all_discussions_are_resolved + ] + ) + end + end + end +end + +Groups::Settings::MergeRequestsController.prepend_mod diff --git a/ee/app/models/ee/namespace_setting.rb b/ee/app/models/ee/namespace_setting.rb index a56fd2a7dc1b77e451121d7e27142bb25c16cda1..e35f5931a6c3b8b61c24c8b5804002ed4cf4a7cf 100644 --- a/ee/app/models/ee/namespace_setting.rb +++ b/ee/app/models/ee/namespace_setting.rb @@ -32,6 +32,35 @@ def prevent_forking_outside_group? saml_setting || root_ancestor.namespace_settings&.prevent_forking_outside_group end + # Define three instance methods: + # + # - [attribute]_of_parent_group Returns the configuration value of the parent group + # - [attribute]?(inherit_group_setting) Returns the final value after inheriting the parent group + # - [attribute]_locked? Returns true if the value is inherited from the parent group + def self.cascading_with_parent_namespace(attribute) + define_method("#{attribute}_of_parent_group") do + namespace&.parent&.namespace_settings&.public_send("#{attribute}?", inherit_group_setting: true) + end + + define_method("#{attribute}?") do |inherit_group_setting: false| + if inherit_group_setting + result = public_send(attribute.to_s) || public_send("#{attribute}_of_parent_group") # rubocop:disable GitlabSecurity/PublicSend + else + result = public_send(attribute.to_s) # rubocop:disable GitlabSecurity/PublicSend + end + + !!result + end + + define_method("#{attribute}_locked?") do + !!public_send("#{attribute}_of_parent_group") # rubocop:disable GitlabSecurity/PublicSend + end + end + + cascading_with_parent_namespace :only_allow_merge_if_pipeline_succeeds + cascading_with_parent_namespace :allow_merge_on_skipped_pipeline + cascading_with_parent_namespace :only_allow_merge_if_all_discussions_are_resolved + private def enabling_user_cap? @@ -68,6 +97,9 @@ def user_cap_enabled? unique_project_download_limit_allowlist auto_ban_user_on_excessive_projects_download default_compliance_framework_id + only_allow_merge_if_pipeline_succeeds + allow_merge_on_skipped_pipeline + only_allow_merge_if_all_discussions_are_resolved ].freeze override :allowed_namespace_settings_params diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 4ff6954d07329ba2760bc8ddca643fb7a2e58e55..27fd0f00fa7f3214e52cdb00668a0a683d73b125 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -322,6 +322,34 @@ def custom_roles_enabled? end end + def self.cascading_with_parent_namespace(attribute) + define_method("#{attribute}_of_parent_group") do + self.group&.namespace_settings&.public_send("#{attribute}?", inherit_group_setting: true) + end + + define_method("#{attribute}?") do |inherit_group_setting: false| + return super() unless licensed_feature_available?(:group_level_merge_checks_setting) && ::Feature.enabled?(:support_group_level_merge_checks_setting, self) + + if inherit_group_setting + result = self.public_send(attribute) || public_send("#{attribute}_of_parent_group") # rubocop:disable GitlabSecurity/PublicSend + else + result = self.public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend + end + + !!result + end + + define_method("#{attribute}_locked?") do + return super() unless licensed_feature_available?(:group_level_merge_checks_setting) && ::Feature.enabled?(:support_group_level_merge_checks_setting, self) + + public_send("#{attribute}_of_parent_group") # rubocop:disable GitlabSecurity/PublicSend + end + end + + cascading_with_parent_namespace :only_allow_merge_if_pipeline_succeeds + cascading_with_parent_namespace :allow_merge_on_skipped_pipeline + cascading_with_parent_namespace :only_allow_merge_if_all_discussions_are_resolved + def mirror_last_update_succeeded? !!import_state&.last_update_succeeded? end diff --git a/ee/app/models/gitlab_subscriptions/features.rb b/ee/app/models/gitlab_subscriptions/features.rb index a19e25aecaf56d0225e358fda028e2ce416ec199..0982104448b634d3da8cda412bd66fc1e53406a4 100644 --- a/ee/app/models/gitlab_subscriptions/features.rb +++ b/ee/app/models/gitlab_subscriptions/features.rb @@ -162,6 +162,7 @@ class Features coverage_check_approval_rule issuable_resource_links group_protected_branches + group_level_merge_checks_setting ].freeze ULTIMATE_FEATURES = %i[ diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 0c5cce66fbd57aac4f6077bed40a929c3c28aedd..c022432775e81eb3201195ab04dbc104328e3984 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -10,6 +10,7 @@ namespace :settings do resource :reporting, only: [:show], controller: 'reporting' resource :domain_verification, only: [:show], controller: 'domain_verification' + resource :merge_requests, only: [:update] end resources :group_members, only: [], concerns: :access_requestable do diff --git a/ee/spec/models/namespace_setting_spec.rb b/ee/spec/models/namespace_setting_spec.rb index e08f293e7891287f987c81e814f4b43bfaec120b..5dfb3a4b221c68fe36dd7300cb476d71dd1df938 100644 --- a/ee/spec/models/namespace_setting_spec.rb +++ b/ee/spec/models/namespace_setting_spec.rb @@ -272,4 +272,59 @@ ]) end end + + describe '.cascading_with_parent_namespace' do + context "when calling .cascading_with_parent_namespace" do + it 'create three instance methods for attribute' do + described_class.cascading_with_parent_namespace("any_configuration") + expect(described_class.instance_methods).to include( + :any_configuration_of_parent_group, :any_configuration_locked?, :any_configuration?) + end + end + + context 'three configurations of MR checks' do + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:subgroup) { create(:group, parent: group) } + let_it_be_with_reload(:subsubgroup) { create(:group, parent: subgroup) } + + shared_examples '[configuration](inherit_group_setting: bool) and [configuration]_locked?' do |attribute| + using RSpec::Parameterized::TableSyntax + + where(:group_attr, :subgroup_attr, :subsubgroup_attr, :group_with_inherit_attr?, :group_without_inherit_attr?, :group_locked?, :subgroup_with_inherit_attr?, :subgroup_without_inherit_attr?, :subgroup_locked?, :subsubgroup_with_inherit_attr?, :subsubgroup_without_inherit_attr?, :subsubgroup_locked?) do + true | true | true | true | true | false | true | true | true | true | true | true + true | true | false | true | true | false | true | true | true | true | false | true + true | false | false | true | true | false | true | false | true | true | false | true + false | true | true | false | false | false | true | true | false | true | true | true + false | true | false | false | false | false | true | true | false | true | false | true + false | false | false | false | false | false | false | false | false | false | false | false + end + + with_them do + before do + group.namespace_settings.update!(attribute => group_attr) + subgroup.namespace_settings.update!(attribute => subgroup_attr) + subsubgroup.namespace_settings.update!(attribute => subsubgroup_attr) + end + + it 'returns correct value' do + expect(group.namespace_settings.public_send("#{attribute}?", inherit_group_setting: true)).to eq(group_with_inherit_attr?) + expect(group.namespace_settings.public_send("#{attribute}?", inherit_group_setting: false)).to eq(group_without_inherit_attr?) + expect(group.namespace_settings.public_send("#{attribute}_locked?")).to eq(group_locked?) + + expect(subgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: true)).to eq(subgroup_with_inherit_attr?) + expect(subgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: false)).to eq(subgroup_without_inherit_attr?) + expect(subgroup.namespace_settings.public_send("#{attribute}_locked?")).to eq(subgroup_locked?) + + expect(subsubgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: true)).to eq(subsubgroup_with_inherit_attr?) + expect(subsubgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: false)).to eq(subsubgroup_without_inherit_attr?) + expect(subsubgroup.namespace_settings.public_send("#{attribute}_locked?")).to eq(subsubgroup_locked?) + end + end + end + + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :only_allow_merge_if_pipeline_succeeds + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :allow_merge_on_skipped_pipeline + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :only_allow_merge_if_all_discussions_are_resolved + end + end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 6fbde42aeee4efeab369259cf503ccc4f6049ea3..d5d5d9bb54bf206ee958bea368890dffd3217186 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -3731,6 +3731,65 @@ def stub_default_url_options(host) end end + describe '.cascading_with_parent_namespace' do + before do + stub_licensed_features(group_level_merge_checks_setting: true) + end + + context "when calling .cascading_with_parent_namespace" do + it 'create three instance methods for attribute' do + EE::Project.cascading_with_parent_namespace("any_configuration") + expect(EE::Project.instance_methods).to include( + :any_configuration_of_parent_group, :any_configuration_locked?, :any_configuration?) + end + end + + context 'three configurations of MR checks' do + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:subgroup) { create(:group, parent: group) } + let_it_be_with_reload(:project) { create(:project, group: subgroup) } + + shared_examples '[configuration](inherit_group_setting: bool) and [configuration]_locked?' do |attribute| + using RSpec::Parameterized::TableSyntax + + where(:group_attr, :subgroup_attr, :project_attr, :group_with_inherit_attr?, :group_without_inherit_attr?, :group_locked?, :subgroup_with_inherit_attr?, :subgroup_without_inherit_attr?, :subgroup_locked?, :project_with_inherit_attr?, :project_without_inherit_attr?, :project_locked?) do + true | true | true | true | true | false | true | true | true | true | true | true + true | true | false | true | true | false | true | true | true | true | false | true + true | false | false | true | true | false | true | false | true | true | false | true + false | true | true | false | false | false | true | true | false | true | true | true + false | true | false | false | false | false | true | true | false | true | false | true + false | false | false | false | false | false | false | false | false | false | false | false + end + + with_them do + before do + group.namespace_settings.update!(attribute => group_attr) + subgroup.namespace_settings.update!(attribute => subgroup_attr) + project.update!(attribute => project_attr) + end + + it 'returns correct value' do + expect(group.namespace_settings.public_send("#{attribute}?", inherit_group_setting: true)).to eq(group_with_inherit_attr?) + expect(group.namespace_settings.public_send("#{attribute}?", inherit_group_setting: false)).to eq(group_without_inherit_attr?) + expect(group.namespace_settings.public_send("#{attribute}_locked?")).to eq(group_locked?) + + expect(subgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: true)).to eq(subgroup_with_inherit_attr?) + expect(subgroup.namespace_settings.public_send("#{attribute}?", inherit_group_setting: false)).to eq(subgroup_without_inherit_attr?) + expect(subgroup.namespace_settings.public_send("#{attribute}_locked?")).to eq(subgroup_locked?) + + expect(project.public_send("#{attribute}?", inherit_group_setting: true)).to eq(project_with_inherit_attr?) + expect(project.public_send("#{attribute}?", inherit_group_setting: false)).to eq(project_without_inherit_attr?) + expect(project.public_send("#{attribute}_locked?")).to eq(project_locked?) + end + end + end + + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :only_allow_merge_if_pipeline_succeeds + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :allow_merge_on_skipped_pipeline + it_behaves_like '[configuration](inherit_group_setting: bool) and [configuration]_locked?', :only_allow_merge_if_all_discussions_are_resolved + end + end + describe '#okrs_mvc_feature_flag_enabled?' do let_it_be(:project) { create(:project) } diff --git a/ee/spec/requests/groups/settings/merge_requests_controller_spec.rb b/ee/spec/requests/groups/settings/merge_requests_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c0fd8b1e80269d5cc1145d721a102b0bc745b924 --- /dev/null +++ b/ee/spec/requests/groups/settings/merge_requests_controller_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Groups::Settings::MergeRequestsController, feature_category: :code_review do + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + describe 'PATCH #update' do + subject do + patch group_settings_merge_requests_path(group), params: { + group_id: group, + namespace_setting: { + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, + only_allow_merge_if_all_discussions_are_resolved: true + } + } + end + + context 'when user is not an admin' do + before do + group.add_owner(user) + end + + it 'respond status :not_found' do + subject + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when user is an admin' do + let(:user) { create(:admin) } + + before do + stub_licensed_features(group_level_merge_checks_setting: true) + group.add_owner(user) + end + + it { is_expected.to redirect_to(edit_group_path(group, anchor: 'js-merge-requests-settings')) } + + context 'when service execution went wrong' do + let(:update_service) { double } + + before do + allow_next_instance_of(Groups::UpdateService) do |service| + allow(service).to receive(:execute).and_return(false) + end + subject + end + + it 'returns a flash alert' do + expect(flash[:alert]).to eq("Group '#{group.name}' could not be updated.") + end + end + + context 'when service execution was successful' do + it 'returns a flash notice' do + subject + + expect(flash[:notice]).to eq("Group '#{group.name}' was successfully updated.") + expect(group.namespace_settings.reload).to have_attributes( + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, + only_allow_merge_if_all_discussions_are_resolved: true + ) + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8bc96eb4945bb826b5b935a01daab5217cbc6a53..61dc71966d64e88ba5cf52a43db9600fc78eb327 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19240,6 +19240,12 @@ msgstr "" msgid "Group %{group_name} was successfully created." msgstr "" +msgid "Group '%{group_name}' could not be updated." +msgstr "" + +msgid "Group '%{group_name}' was successfully updated." +msgstr "" + msgid "Group Access Tokens" msgstr ""