From b3e77b2e7cf6e07a5f2467cb669e2ec6989c6bbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Thu, 3 Nov 2022 18:53:57 +0800 Subject: [PATCH 01/11] Add MR settings support for group (EE frontend) Changelog: added EE: true --- .../components/approval_settings_checkbox.vue | 23 +++- .../approval_settings_locked_icon.vue | 8 +- .../javascripts/pages/groups/edit/index.js | 2 + .../projects/settings/merge_requests/index.js | 2 + .../merge_requests/merge_checks/app.vue | 108 ++++++++++++++++++ .../merge_requests/merge_checks/constants.js | 23 ++++ .../project_merge_request_merge_checks.js | 31 +++++ ee/app/views/groups/_merge_requests.html.haml | 11 ++ .../groups/settings/_merge_requests.html.haml | 15 +++ ...rge_checks_pipeline_and_resolved.html.haml | 11 ++ .../settings/merge_requests_settings_spec.rb | 45 ++++++++ .../settings/merge_requests_settings_spec.rb | 36 +++++- .../approval_settings_checkbox_spec.js | 41 +++++-- .../approval_settings_locked_icon_spec.js | 10 +- .../merge_requests/merge_checks/app_spec.js | 74 ++++++++++++ locale/gitlab.pot | 3 + 16 files changed, 421 insertions(+), 22 deletions(-) create mode 100644 ee/app/assets/javascripts/projects/merge_requests/merge_checks/app.vue create mode 100644 ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js create mode 100644 ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js create mode 100644 ee/app/views/groups/_merge_requests.html.haml create mode 100644 ee/app/views/groups/settings/_merge_requests.html.haml create mode 100644 ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml create mode 100644 ee/spec/features/groups/settings/merge_requests_settings_spec.rb create mode 100644 ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue index 164e55450eb456..7218be427fdac9 100644 --- a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue +++ b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue @@ -13,7 +13,7 @@ export default { required: true, }, checked: { - type: Boolean, + type: [Boolean, String], required: false, default: false, }, @@ -27,6 +27,16 @@ export default { required: false, default: '', }, + value: { + type: String, + required: false, + default: 'true', + }, + name: { + type: String, + required: false, + default: undefined, + }, }, methods: { input(value) { @@ -37,8 +47,17 @@ export default { diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue b/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue index b536edffca8da7..f123033afbf2dc 100644 --- a/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue +++ b/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue @@ -1,8 +1,9 @@ + diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js new file mode 100644 index 00000000000000..7aae8107bcede4 --- /dev/null +++ b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js @@ -0,0 +1,23 @@ +import { s__ } from '~/locale'; + +export const pipelineMustSucceedI18n = { + label: s__('ProjectSettings|Pipelines must succeed'), + help: s__( + "ProjectSettings|Merge requests can't be merged if the latest pipeline did not succeed or is still running.", + ), +}; + +export const allowMergeOnSkippedI18n = { + label: s__('ProjectSettings|Skipped pipelines are considered successful'), + help: s__( + 'ProjectSettings|Introduces the risk of merging changes that do not pass the pipeline.', + ), +}; + +export const onlyMergeWhenAllResolvedLabelI18n = s__( + 'ProjectSettings|All threads must be resolved', +); + +export const lockedTextI18n = s__( + 'ApprovalSettings|This setting is configured in %{groupName} and can only be changed in the group settings by an administrator or group owner.', +); diff --git a/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js b/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js new file mode 100644 index 00000000000000..ab0dac2f7feb98 --- /dev/null +++ b/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js @@ -0,0 +1,31 @@ +import Vue from 'vue'; +import MergeChecksApp from './merge_requests/merge_checks/app.vue'; + +export const initMergeRequestMergeChecksApp = () => { + const el = document.querySelector('.js-merge-request-merge-checks'); + + if (!el) { + return false; + } + const { + type, + allowMergeOnSkippedPipeline, + onlyAllowMergeIfAllResolved, + pipelineMustSucceed, + groupName, + } = el.dataset; + + return new Vue({ + el, + provide: { + groupName, + type, + allowMergeOnSkippedPipeline: JSON.parse(allowMergeOnSkippedPipeline), + onlyAllowMergeIfAllResolved: JSON.parse(onlyAllowMergeIfAllResolved), + pipelineMustSucceed: JSON.parse(pipelineMustSucceed), + }, + render(createElement) { + return createElement(MergeChecksApp); + }, + }); +}; diff --git a/ee/app/views/groups/_merge_requests.html.haml b/ee/app/views/groups/_merge_requests.html.haml new file mode 100644 index 00000000000000..f0bfbc759f8e0a --- /dev/null +++ b/ee/app/views/groups/_merge_requests.html.haml @@ -0,0 +1,11 @@ +- if Feature.enabled?(:support_group_level_merge_checks_setting, @group) && @group.licensed_feature_available?(:group_level_merge_checks_setting) + %section.settings.gs-merge-requests.no-animate#js-merge-requests-settings{ class: ('expanded' if expanded), data: { qa_selector: 'permission_lfs_2fa_content' } } + .settings-header + %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } + = _('Merge requests') + = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do + = expanded ? _('Collapse') : _('Expand') + %p + %p= s_('GroupSettings|Choose your merge checks.') + .settings-content + = render 'groups/settings/merge_requests' diff --git a/ee/app/views/groups/settings/_merge_requests.html.haml b/ee/app/views/groups/settings/_merge_requests.html.haml new file mode 100644 index 00000000000000..ed58c5fd069732 --- /dev/null +++ b/ee/app/views/groups/settings/_merge_requests.html.haml @@ -0,0 +1,15 @@ += gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: "merge-request-settings-form js-mr-settings-form" }, authenticity_token: true do |f| + %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-settings' } + + %fieldset + %h5= s_('ProjectSettings|Merge checks') + %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') + + .builds-feature.js-merge-request-merge-checks{ data: { type: 'namespace_setting', pipeline_must_succeed: { locked: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds_locked?, value: @group.namespace_settings.actual_only_allow_merge_if_pipeline_succeeds ? '1' : '0' }.to_json, + allow_merge_on_skipped_pipeline: { locked: @group.namespace_settings.allow_merge_on_skipped_pipeline_locked?, value: @group.namespace_settings.actual_allow_merge_on_skipped_pipeline ? '1' : '0' }.to_json, + only_allow_merge_if_all_resolved: { locked: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @group.namespace_settings.actual_only_allow_merge_if_all_discussions_are_resolved ? '1' : '0' }.to_json, + group_name: @group.name + } + } + + = f.submit _('Save changes'), class: "rspec-save-merge-request-changes", data: { qa_selector: 'save_merge_request_changes_button' }, pajamas_button: true diff --git a/ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml b/ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml new file mode 100644 index 00000000000000..dbffb7c6333fec --- /dev/null +++ b/ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml @@ -0,0 +1,11 @@ +- support_group_level_mr_setting = Feature.enabled?(:support_group_level_merge_checks_setting, @project) && @project.licensed_feature_available?(:group_level_merge_checks_setting) + +- if !support_group_level_mr_setting + = render_ce 'projects/merge_request_merge_checks_pipeline_and_resolved', form: form, project: project +- else + .builds-feature.js-merge-request-merge-checks{ data: { type: 'project', pipeline_must_succeed: { locked: @project.only_allow_merge_if_pipeline_succeeds_locked?, value: @project.actual_only_allow_merge_if_pipeline_succeeds ? '1' : '0' }.to_json, + allow_merge_on_skipped_pipeline: { locked: @project.allow_merge_on_skipped_pipeline_locked?, value: @project.actual_allow_merge_on_skipped_pipeline ? '1' : '0' }.to_json, + only_allow_merge_if_all_resolved: { locked: @project.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @project.actual_only_allow_merge_if_all_discussions_are_resolved ? '1' : '0' }.to_json, + group_name: @project.name + } + } diff --git a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb new file mode 100644 index 00000000000000..94ba3f36bd447a --- /dev/null +++ b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe 'Group settings > [EE] General' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: group) } + + before do + sign_in(user) + group.add_owner(user) + stub_licensed_features(group_level_merge_checks_setting: true) + end + + describe "Merge checks", :js do + context 'when checkboxs is locked' do + it 'shows disabled status' do + checkboxs_selectors = %w[ + [name="namespace_setting[only_allow_merge_if_pipeline_succeeds]"][disabled] + [name="namespace_setting[allow_merge_on_skipped_pipeline]"][disabled] + [name="namespace_setting[only_allow_merge_if_all_discussions_are_resolved]"][disabled] + ] + + visit(edit_group_path(sub_group)) + + checkboxs_selectors.each do |selector| + expect(page).not_to have_selector(selector) + end + + group.namespace_settings.update!( + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, + only_allow_merge_if_all_discussions_are_resolved: true + ) + + visit(edit_group_path(sub_group)) + + checkboxs_selectors.each do |selector| + expect(page).to have_selector(selector) + expect(page.find(selector)).to be_checked + end + end + end + end +end diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb index d8e51b7fd2541e..8baf73260aa002 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -5,15 +5,16 @@ include GitlabRoutingHelper let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, namespace: user.namespace, path: 'gitlab', name: 'sample', group: group) } let_it_be(:group_member) { create(:user) } before do sign_in(user) project.add_maintainer(user) - group.add_developer(user) + group.add_owner(user) group.add_developer(group_member) + stub_licensed_features(group_level_merge_checks_setting: true) end context 'Status checks' do @@ -130,4 +131,35 @@ end end end + + describe "Merge checks" do + context 'when checkboxs is locked', :js do + it 'shows disabled status' do + checkboxs_selectors = %w[ + [name="project[only_allow_merge_if_pipeline_succeeds]"][disabled] + [name="project[allow_merge_on_skipped_pipeline]"][disabled] + [name="project[only_allow_merge_if_all_discussions_are_resolved]"][disabled] + ] + + visit(project_settings_merge_requests_path(project)) + + checkboxs_selectors.each do |selector| + expect(page).not_to have_selector(selector) + end + + group.namespace_settings.update!( + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, + only_allow_merge_if_all_discussions_are_resolved: true + ) + + visit(project_settings_merge_requests_path(project)) + + checkboxs_selectors.each do |selector| + expect(page).to have_selector(selector) + expect(page.find(selector)).to be_checked + end + end + end + end end diff --git a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js index 81c1e132017e7d..353fd4ea71cf93 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js @@ -1,10 +1,9 @@ import { GlFormCheckbox } from '@gitlab/ui'; -import { shallowMount } from '@vue/test-utils'; import ApprovalSettingsCheckbox from 'ee/approvals/components/approval_settings_checkbox.vue'; import ApprovalSettingsLockedIcon from 'ee/approvals/components/approval_settings_locked_icon.vue'; import { stubComponent } from 'helpers/stub_component'; -import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; describe('ApprovalSettingsCheckbox', () => { const label = 'Foo'; @@ -13,16 +12,14 @@ describe('ApprovalSettingsCheckbox', () => { let wrapper; const createWrapper = (props = {}) => { - wrapper = extendedWrapper( - shallowMount(ApprovalSettingsCheckbox, { - propsData: { label, lockedText, ...props }, - stubs: { - GlFormCheckbox: stubComponent(GlFormCheckbox, { - props: ['checked'], - }), - }, - }), - ); + wrapper = shallowMountExtended(ApprovalSettingsCheckbox, { + propsData: { label, lockedText, ...props }, + stubs: { + GlFormCheckbox: stubComponent(GlFormCheckbox, { + props: ['checked'], + }), + }, + }); }; const findCheckbox = () => wrapper.findComponent(GlFormCheckbox); @@ -98,4 +95,24 @@ describe('ApprovalSettingsCheckbox', () => { }); }); }); + + describe('attribute passing down to checkbox', () => { + it('name attribute', () => { + const name = 'name'; + createWrapper({ + name, + }); + + expect(findCheckbox().attributes('name')).toBe(name); + }); + + it('value attribute', () => { + const value = '1'; + createWrapper({ + value, + }); + + expect(findCheckbox().attributes('value')).toBe(value); + }); + }); }); diff --git a/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js b/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js index 853e567866f00c..2106b1ef0ed075 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js @@ -3,11 +3,13 @@ import { shallowMount } from '@vue/test-utils'; import ApprovalSettingsLockedIcon from 'ee/approvals/components/approval_settings_locked_icon.vue'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; -import { slugify } from '~/lib/utils/text_utility'; + +let id = 0; describe('ApprovalSettingsLockedIcon', () => { const label = 'Foo'; - const lockIconId = `approval-settings-lock-icon-${slugify(label)}`; + // eslint-disable-next-line no-plusplus + const lockIconId = () => `approval-settings-lock-icon-${++id}`; let wrapper; @@ -36,11 +38,11 @@ describe('ApprovalSettingsLockedIcon', () => { it('shows a lock icon', () => { expect(findLockIcon().props('name')).toBe('lock'); - expect(findLockIcon().attributes('id')).toBe(lockIconId); + expect(findLockIcon().attributes('id')).toBe(lockIconId()); }); it('shows a popover for the lock icon', () => { - expect(findPopover().props('target')).toBe(lockIconId); + expect(findPopover().props('target')).toBe(lockIconId()); }); it('configures how and when the popover should show', () => { diff --git a/ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js b/ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js new file mode 100644 index 00000000000000..024ac9831bcb4d --- /dev/null +++ b/ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js @@ -0,0 +1,74 @@ +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import { sprintf } from '~/locale'; +import MergeChecksApp from 'ee/projects/merge_requests/merge_checks/app.vue'; +import { lockedTextI18n } from 'ee/projects/merge_requests/merge_checks/constants'; + +describe('', () => { + const defaultType = 'project'; + const defaultGroupName = 'test'; + const defaultCheckValue = { + locked: false, + value: false, + }; + + let wrapper; + + const createWrapper = (provides = {}) => { + wrapper = shallowMountExtended(MergeChecksApp, { + provide: { + type: defaultType, + groupName: defaultGroupName, + pipelineMustSucceed: defaultCheckValue, + allowMergeOnSkippedPipeline: defaultCheckValue, + onlyAllowMergeIfAllResolved: defaultCheckValue, + ...provides, + }, + }); + }; + + const findOnlyAllowMergeWhenPipelineSucceeds = () => + wrapper.findByTestId('only_allow_merge_if_pipeline_succeeds_checkbox'); + + afterEach(() => { + wrapper.destroy(); + }); + + describe('type', () => { + it('with default type', () => { + createWrapper(); + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('name')).toBe( + `${defaultType}[only_allow_merge_if_pipeline_succeeds]`, + ); + }); + + it('with other type', () => { + const type = 'new-type'; + createWrapper({ + type, + }); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('name')).toBe( + `${type}[only_allow_merge_if_pipeline_succeeds]`, + ); + }); + }); + + describe('name', () => { + it('with default name', () => { + createWrapper(); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(lockedTextI18n, { groupName: defaultGroupName }), + ); + }); + + it('with other name', () => { + const groupName = 'new-name'; + createWrapper({ groupName }); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(lockedTextI18n, { groupName }), + ); + }); + }); +}); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4d3a73dc3a6740..84082fdb3b4014 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20108,6 +20108,9 @@ msgstr "" msgid "GroupSettings|Choose a group path that does not start with a dash or end with a period. It can also contain alphanumeric characters and underscores." msgstr "" +msgid "GroupSettings|Choose your merge checks." +msgstr "" + msgid "GroupSettings|Compliance frameworks" msgstr "" -- GitLab From fc56d7e139ef6f7123c132b4bb054826162d637b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Wed, 4 Jan 2023 11:03:35 +0800 Subject: [PATCH 02/11] Update MR checks haml info - Rename '_merge_request_merge_checks_pipeline_and_resolved.html.haml' to '_merge_request_pipelines_and_threads_options.html.haml' - Add reference to 'groups/merge_requests' - Update the function name 'actual_[configuration]' Changed: fixed --- app/views/groups/edit.html.haml | 1 + ee/app/views/groups/settings/_merge_requests.html.haml | 6 +++--- ...merge_request_pipelines_and_threads_options.html.haml} | 8 ++++---- 3 files changed, 8 insertions(+), 7 deletions(-) rename ee/app/views/projects/{_merge_request_merge_checks_pipeline_and_resolved.html.haml => _merge_request_pipelines_and_threads_options.html.haml} (64%) diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index f1ed1311195525..0c416d57b75872 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -28,6 +28,7 @@ .settings-content = render 'groups/settings/permissions' += render_if_exists 'groups/merge_requests', expanded: expanded, group: @group = render_if_exists 'groups/merge_request_approval_settings', expanded: expanded, group: @group, user: current_user = render_if_exists 'groups/insights', expanded: expanded = render_if_exists 'groups/analytics_dashboards', expanded: expanded diff --git a/ee/app/views/groups/settings/_merge_requests.html.haml b/ee/app/views/groups/settings/_merge_requests.html.haml index ed58c5fd069732..a5955dccb873fd 100644 --- a/ee/app/views/groups/settings/_merge_requests.html.haml +++ b/ee/app/views/groups/settings/_merge_requests.html.haml @@ -5,9 +5,9 @@ %h5= s_('ProjectSettings|Merge checks') %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') - .builds-feature.js-merge-request-merge-checks{ data: { type: 'namespace_setting', pipeline_must_succeed: { locked: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds_locked?, value: @group.namespace_settings.actual_only_allow_merge_if_pipeline_succeeds ? '1' : '0' }.to_json, - allow_merge_on_skipped_pipeline: { locked: @group.namespace_settings.allow_merge_on_skipped_pipeline_locked?, value: @group.namespace_settings.actual_allow_merge_on_skipped_pipeline ? '1' : '0' }.to_json, - only_allow_merge_if_all_resolved: { locked: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @group.namespace_settings.actual_only_allow_merge_if_all_discussions_are_resolved ? '1' : '0' }.to_json, + .builds-feature.js-merge-request-merge-checks{ data: { type: 'namespace_setting', pipeline_must_succeed: { locked: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds_locked?, value: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) ? '1' : '0' }.to_json, + allow_merge_on_skipped_pipeline: { locked: @group.namespace_settings.allow_merge_on_skipped_pipeline_locked?, value: @group.namespace_settings.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) ? '1' : '0' }.to_json, + only_allow_merge_if_all_resolved: { locked: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true) ? '1' : '0' }.to_json, group_name: @group.name } } diff --git a/ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml b/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml similarity index 64% rename from ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml rename to ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml index dbffb7c6333fec..5fedc2915efec6 100644 --- a/ee/app/views/projects/_merge_request_merge_checks_pipeline_and_resolved.html.haml +++ b/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml @@ -1,11 +1,11 @@ - support_group_level_mr_setting = Feature.enabled?(:support_group_level_merge_checks_setting, @project) && @project.licensed_feature_available?(:group_level_merge_checks_setting) - if !support_group_level_mr_setting - = render_ce 'projects/merge_request_merge_checks_pipeline_and_resolved', form: form, project: project + = render_ce 'projects/merge_request_pipelines_and_threads_options', form: form, project: project - else - .builds-feature.js-merge-request-merge-checks{ data: { type: 'project', pipeline_must_succeed: { locked: @project.only_allow_merge_if_pipeline_succeeds_locked?, value: @project.actual_only_allow_merge_if_pipeline_succeeds ? '1' : '0' }.to_json, - allow_merge_on_skipped_pipeline: { locked: @project.allow_merge_on_skipped_pipeline_locked?, value: @project.actual_allow_merge_on_skipped_pipeline ? '1' : '0' }.to_json, - only_allow_merge_if_all_resolved: { locked: @project.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @project.actual_only_allow_merge_if_all_discussions_are_resolved ? '1' : '0' }.to_json, + .builds-feature.js-merge-request-merge-checks{ data: { type: 'project', pipeline_must_succeed: { locked: @project.only_allow_merge_if_pipeline_succeeds_locked?, value: @project.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) ? '1' : '0' }.to_json, + allow_merge_on_skipped_pipeline: { locked: @project.allow_merge_on_skipped_pipeline_locked?, value: @project.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) ? '1' : '0' }.to_json, + only_allow_merge_if_all_resolved: { locked: @project.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @project.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true) ? '1' : '0' }.to_json, group_name: @project.name } } -- GitLab From 44279fdd3258c695cebb9761b56bdee0a6d2cb62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 13 Jan 2023 11:18:22 +0800 Subject: [PATCH 03/11] Remove useless qa_selector --- ee/app/views/groups/_merge_requests.html.haml | 2 +- ee/app/views/groups/settings/_merge_requests.html.haml | 4 ++-- locale/gitlab.pot | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ee/app/views/groups/_merge_requests.html.haml b/ee/app/views/groups/_merge_requests.html.haml index f0bfbc759f8e0a..e6f1bd013a63b4 100644 --- a/ee/app/views/groups/_merge_requests.html.haml +++ b/ee/app/views/groups/_merge_requests.html.haml @@ -6,6 +6,6 @@ = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do = expanded ? _('Collapse') : _('Expand') %p - %p= s_('GroupSettings|Choose your merge checks.') + %p= s_("GroupSettings|Choose the merge request checks for the projects in this group.") .settings-content = render 'groups/settings/merge_requests' diff --git a/ee/app/views/groups/settings/_merge_requests.html.haml b/ee/app/views/groups/settings/_merge_requests.html.haml index a5955dccb873fd..1e5067ecc47a4e 100644 --- a/ee/app/views/groups/settings/_merge_requests.html.haml +++ b/ee/app/views/groups/settings/_merge_requests.html.haml @@ -1,7 +1,7 @@ = gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: "merge-request-settings-form js-mr-settings-form" }, authenticity_token: true do |f| %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-settings' } - %fieldset + %fieldset.form-group.gl-form-group %h5= s_('ProjectSettings|Merge checks') %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') @@ -12,4 +12,4 @@ } } - = f.submit _('Save changes'), class: "rspec-save-merge-request-changes", data: { qa_selector: 'save_merge_request_changes_button' }, pajamas_button: true + = f.submit _('Save changes'), class: "rspec-save-merge-request-changes", pajamas_button: true diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 84082fdb3b4014..d32aa8f965351f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20108,7 +20108,7 @@ msgstr "" msgid "GroupSettings|Choose a group path that does not start with a dash or end with a period. It can also contain alphanumeric characters and underscores." msgstr "" -msgid "GroupSettings|Choose your merge checks." +msgid "GroupSettings|Choose the merge request checks for the projects in this group." msgstr "" msgid "GroupSettings|Compliance frameworks" -- GitLab From 68e4b0ec43231513f1a8e7fbcc9dd63da0da8fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Wed, 18 Jan 2023 11:01:34 +0800 Subject: [PATCH 04/11] Update the review suggestions on the front end - Add feature_category for merge_requests_settings_spec.rb - Change some double quotes to single quotes to keep the style uniform - Give the component in project_merge_request_merge_checks.js a name Changelog: changed --- .../projects/project_merge_request_merge_checks.js | 1 + ee/app/views/groups/_merge_requests.html.haml | 4 ++-- ee/app/views/groups/settings/_merge_requests.html.haml | 4 ++-- .../features/groups/settings/merge_requests_settings_spec.rb | 4 ++-- .../projects/settings/merge_requests_settings_spec.rb | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js b/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js index ab0dac2f7feb98..40dfcfa1f7a4bb 100644 --- a/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js +++ b/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js @@ -17,6 +17,7 @@ export const initMergeRequestMergeChecksApp = () => { return new Vue({ el, + name: 'MergeRequestChecksRoot', provide: { groupName, type, diff --git a/ee/app/views/groups/_merge_requests.html.haml b/ee/app/views/groups/_merge_requests.html.haml index e6f1bd013a63b4..30dcb25404de2b 100644 --- a/ee/app/views/groups/_merge_requests.html.haml +++ b/ee/app/views/groups/_merge_requests.html.haml @@ -1,11 +1,11 @@ - if Feature.enabled?(:support_group_level_merge_checks_setting, @group) && @group.licensed_feature_available?(:group_level_merge_checks_setting) - %section.settings.gs-merge-requests.no-animate#js-merge-requests-settings{ class: ('expanded' if expanded), data: { qa_selector: 'permission_lfs_2fa_content' } } + %section.settings.gs-merge-requests.no-animate#js-merge-requests-settings{ class: ('expanded' if expanded) } .settings-header %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } = _('Merge requests') = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do = expanded ? _('Collapse') : _('Expand') %p - %p= s_("GroupSettings|Choose the merge request checks for the projects in this group.") + %p= s_('GroupSettings|Choose the merge request checks for the projects in this group.') .settings-content = render 'groups/settings/merge_requests' diff --git a/ee/app/views/groups/settings/_merge_requests.html.haml b/ee/app/views/groups/settings/_merge_requests.html.haml index 1e5067ecc47a4e..6856d4906879a6 100644 --- a/ee/app/views/groups/settings/_merge_requests.html.haml +++ b/ee/app/views/groups/settings/_merge_requests.html.haml @@ -1,4 +1,4 @@ -= gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: "merge-request-settings-form js-mr-settings-form" }, authenticity_token: true do |f| += gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: 'merge-request-settings-form js-mr-settings-form' }, authenticity_token: true do |f| %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-settings' } %fieldset.form-group.gl-form-group @@ -12,4 +12,4 @@ } } - = f.submit _('Save changes'), class: "rspec-save-merge-request-changes", pajamas_button: true + = f.submit _('Save changes'), class: 'rspec-save-merge-request-changes', pajamas_button: true diff --git a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb index 94ba3f36bd447a..7c5f50e0265430 100644 --- a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe 'Group settings > [EE] General' do +RSpec.describe 'Group settings > [EE] General', feature_category: :code_review do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:sub_group) { create(:group, parent: group) } @@ -12,7 +12,7 @@ stub_licensed_features(group_level_merge_checks_setting: true) end - describe "Merge checks", :js do + describe 'Merge checks', :js do context 'when checkboxs is locked' do it 'shows disabled status' do checkboxs_selectors = %w[ diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb index 8baf73260aa002..27b4a64ebae4bd 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -132,7 +132,7 @@ end end - describe "Merge checks" do + describe 'Merge checks' do context 'when checkboxs is locked', :js do it 'shows disabled status' do checkboxs_selectors = %w[ -- GitLab From 4180b5e935a11d592b1fe8017cabda440b01d72f Mon Sep 17 00:00:00 2001 From: JeremyWuuuuu Date: Thu, 2 Feb 2023 16:40:12 +0800 Subject: [PATCH 05/11] Feat: merge checks * Update code based on MR review. * Enhance approval settings checkbox for meeting requirements. --- .../components/approval_settings_checkbox.vue | 4 ++ .../javascripts/pages/groups/edit/index.js | 2 +- .../projects/settings/merge_requests/index.js | 2 +- .../merge_checks_app.vue} | 47 ++++++++++++------- .../merge_checks/index.js} | 3 +- .../approval_settings_checkbox_spec.js | 8 ++++ .../merge_checks_app_spec.js} | 29 +++++++++--- 7 files changed, 70 insertions(+), 25 deletions(-) rename ee/app/assets/javascripts/projects/merge_requests/merge_checks/{app.vue => components/merge_checks_app.vue} (64%) rename ee/app/assets/javascripts/projects/{project_merge_request_merge_checks.js => merge_requests/merge_checks/index.js} (89%) rename ee/spec/frontend/projects/merge_requests/merge_checks/{app_spec.js => components/merge_checks_app_spec.js} (68%) diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue index 7218be427fdac9..d8696d11b42f63 100644 --- a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue +++ b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue @@ -42,6 +42,9 @@ export default { input(value) { this.$emit('input', value); }, + change(value) { + this.$emit('change', value); + }, }, }; @@ -53,6 +56,7 @@ export default { :name="name" :value="value" @input="input" + @change="change" > {{ label }} diff --git a/ee/app/assets/javascripts/pages/groups/edit/index.js b/ee/app/assets/javascripts/pages/groups/edit/index.js index 88e87532513676..6b6835f57e6fe1 100644 --- a/ee/app/assets/javascripts/pages/groups/edit/index.js +++ b/ee/app/assets/javascripts/pages/groups/edit/index.js @@ -4,7 +4,7 @@ import validateRestrictedIpAddress from 'ee/groups/settings/access_restriction_f import { initGroupPermissionsFormSubmit } from 'ee/groups/settings/permissions'; import { initServicePingSettingsClickTracking } from 'ee/registration_features_discovery_message'; import { createAlert } from '~/flash'; -import { initMergeRequestMergeChecksApp } from 'ee/projects/project_merge_request_merge_checks'; +import { initMergeRequestMergeChecksApp } from 'ee/projects/merge_requests/merge_checks'; import { __ } from '~/locale'; if (gon.features.saasUserCapsAutoApprovePendingUsersOnCapIncrease) { diff --git a/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js b/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js index 208c6bd6e48ce2..3176246bb1fb8e 100644 --- a/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js +++ b/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js @@ -1,7 +1,7 @@ import '~/pages/projects/settings/merge_requests'; import mountApprovals from 'ee/approvals/mount_project_settings'; import { initMergeOptionSettings } from 'ee/pages/projects/edit/merge_options'; -import { initMergeRequestMergeChecksApp } from 'ee/projects/project_merge_request_merge_checks'; +import { initMergeRequestMergeChecksApp } from 'ee/projects/merge_requests/merge_checks'; import mountStatusChecks from 'ee/status_checks/mount'; mountApprovals(document.getElementById('js-mr-approvals-settings')); diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/app.vue b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue similarity index 64% rename from ee/app/assets/javascripts/projects/merge_requests/merge_checks/app.vue rename to ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue index 420fa159c4c160..e3ccfe00d0dfb6 100644 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/app.vue +++ b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue @@ -6,9 +6,10 @@ import { lockedTextI18n, onlyMergeWhenAllResolvedLabelI18n, pipelineMustSucceedI18n, -} from './constants'; +} from '../constants'; export default { + name: 'MergeChecksApp', components: { ApprovalSettingsCheckbox, }, @@ -34,6 +35,19 @@ export default { required: true, }, }, + data() { + // map the initial values for two way binding. + const { + pipelineMustSucceed, + allowMergeOnSkippedPipeline, + onlyAllowMergeIfAllResolved, + } = this; + return { + localPipelineMustSucceed: pipelineMustSucceed, + localAllowMergeOnSkippedPipeline: allowMergeOnSkippedPipeline, + localOnlyAllowMergeIfAllResolved: onlyAllowMergeIfAllResolved, + }; + }, computed: { lockedText() { return `${sprintf(lockedTextI18n, { groupName: this.groupName })}`; @@ -43,6 +57,10 @@ export default { formName(name) { return `${this.type}[${name}]`; }, + toggleChecked(target) { + console.log(1) + target.value = target.value === '1' ? '0' : '1'; + }, }, i18n: { allowMergeOnSkippedI18n, @@ -54,55 +72,52 @@ export default { diff --git a/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js similarity index 89% rename from ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js rename to ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js index 40dfcfa1f7a4bb..51a835adbda2ca 100644 --- a/ee/app/assets/javascripts/projects/project_merge_request_merge_checks.js +++ b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import MergeChecksApp from './merge_requests/merge_checks/app.vue'; +import MergeChecksApp from './components/merge_checks_app.vue'; export const initMergeRequestMergeChecksApp = () => { const el = document.querySelector('.js-merge-request-merge-checks'); @@ -14,6 +14,7 @@ export const initMergeRequestMergeChecksApp = () => { pipelineMustSucceed, groupName, } = el.dataset; + console.log(el.dataset); return new Vue({ el, diff --git a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js index 353fd4ea71cf93..bf2dacc56e6f88 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js @@ -59,6 +59,14 @@ describe('ApprovalSettingsCheckbox', () => { expect(wrapper.emitted('input')[0]).toStrictEqual([true]); }); + + it('emits a change event when the checkbox is changed', async () => { + createWrapper(); + + await findCheckbox().vm.$emit('change', true); + + expect(wrapper.emitted('change')[0]).toStrictEqual([true]); + }); }); describe('locked', () => { diff --git a/ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js b/ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js similarity index 68% rename from ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js rename to ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js index 024ac9831bcb4d..9c9ecf60149c41 100644 --- a/ee/spec/frontend/projects/merge_requests/merge_checks/app_spec.js +++ b/ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js @@ -1,6 +1,6 @@ -import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import { sprintf } from '~/locale'; -import MergeChecksApp from 'ee/projects/merge_requests/merge_checks/app.vue'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import MergeChecksApp from 'ee/projects/merge_requests/merge_checks/components/merge_checks_app.vue'; import { lockedTextI18n } from 'ee/projects/merge_requests/merge_checks/constants'; describe('', () => { @@ -8,7 +8,7 @@ describe('', () => { const defaultGroupName = 'test'; const defaultCheckValue = { locked: false, - value: false, + value: '0', }; let wrapper; @@ -29,6 +29,9 @@ describe('', () => { const findOnlyAllowMergeWhenPipelineSucceeds = () => wrapper.findByTestId('only_allow_merge_if_pipeline_succeeds_checkbox'); + const findOnlyAllowMergeWhenPipelineSucceedsInput = () => + wrapper.find('input[name*="only_allow_merge_if_pipeline_succeeds"]'); + afterEach(() => { wrapper.destroy(); }); @@ -36,7 +39,7 @@ describe('', () => { describe('type', () => { it('with default type', () => { createWrapper(); - expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('name')).toBe( + expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( `${defaultType}[only_allow_merge_if_pipeline_succeeds]`, ); }); @@ -47,13 +50,13 @@ describe('', () => { type, }); - expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('name')).toBe( + expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( `${type}[only_allow_merge_if_pipeline_succeeds]`, ); }); }); - describe('name', () => { + describe('group name', () => { it('with default name', () => { createWrapper(); @@ -71,4 +74,18 @@ describe('', () => { ); }); }); + + describe('binding value', () => { + it('should be able to bind and mutate value', async () => { + createWrapper(); + const checkbox = findOnlyAllowMergeWhenPipelineSucceeds(); + + expect(checkbox.props('checked')).toBe(defaultCheckValue.value); + + await checkbox.vm.$emit('change'); + expect(checkbox.props('checked')).toBe('1'); + await checkbox.vm.$emit('change'); + expect(checkbox.props('checked')).toBe('0'); + }); + }); }); -- GitLab From af28717584cc0758584242110d6aacc6f2054d58 Mon Sep 17 00:00:00 2001 From: Wu Jeremy Date: Fri, 3 Feb 2023 01:40:24 +0000 Subject: [PATCH 06/11] Remove log statement --- .../merge_checks/components/merge_checks_app.vue | 3 +-- .../javascripts/projects/merge_requests/merge_checks/index.js | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue index e3ccfe00d0dfb6..11ae3de112d5c5 100644 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue +++ b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue @@ -58,8 +58,7 @@ export default { return `${this.type}[${name}]`; }, toggleChecked(target) { - console.log(1) - target.value = target.value === '1' ? '0' : '1'; + target.value = Number(target.value) === '1' ? '0' : '1'; }, }, i18n: { diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js index 51a835adbda2ca..1d4bbbe3d50e22 100644 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js +++ b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js @@ -14,7 +14,6 @@ export const initMergeRequestMergeChecksApp = () => { pipelineMustSucceed, groupName, } = el.dataset; - console.log(el.dataset); return new Vue({ el, -- GitLab From e3db874213b034cdfabc5387fbda8cb3e8ece350 Mon Sep 17 00:00:00 2001 From: JeremyWuuuuu Date: Fri, 3 Feb 2023 10:45:57 +0800 Subject: [PATCH 07/11] Rewrite code to adapt the changes --- .../components/approval_settings_checkbox.vue | 24 ++-------------- .../components/merge_checks_app.vue | 20 +++++-------- .../merge_requests/merge_checks/index.js | 1 + .../groups/settings/_merge_requests.html.haml | 6 ++-- ...st_pipelines_and_threads_options.html.haml | 6 ++-- .../approval_settings_checkbox_spec.js | 28 ------------------- 6 files changed, 16 insertions(+), 69 deletions(-) diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue index d8696d11b42f63..08824255237963 100644 --- a/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue +++ b/ee/app/assets/javascripts/approvals/components/approval_settings_checkbox.vue @@ -13,7 +13,7 @@ export default { required: true, }, checked: { - type: [Boolean, String], + type: Boolean, required: false, default: false, }, @@ -27,37 +27,17 @@ export default { required: false, default: '', }, - value: { - type: String, - required: false, - default: 'true', - }, - name: { - type: String, - required: false, - default: undefined, - }, }, methods: { input(value) { this.$emit('input', value); }, - change(value) { - this.$emit('change', value); - }, }, }; diff --git a/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue b/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue index f123033afbf2dc..bc3baa3c298ce6 100644 --- a/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue +++ b/ee/app/assets/javascripts/approvals/components/approval_settings_locked_icon.vue @@ -1,9 +1,8 @@ + diff --git a/ee/app/assets/javascripts/merge_checks/constants.js b/ee/app/assets/javascripts/merge_checks/constants.js new file mode 100644 index 00000000000000..d791213a97351d --- /dev/null +++ b/ee/app/assets/javascripts/merge_checks/constants.js @@ -0,0 +1,19 @@ +import { s__ } from '~/locale'; + +export const I18N = { + pipelineMustSucceed: { + label: s__('MergeChecks|Pipelines must succeed'), + help: s__( + "MergeChecks|Merge requests can't be merged if the latest pipeline did not succeed or is still running.", + ), + }, + allowMergeOnSkipped: { + label: s__('MergeChecks|Skipped pipelines are considered successful'), + help: s__('MergeChecks|Introduces the risk of merging changes that do not pass the pipeline.'), + }, + onlyMergeWhenAllResolvedLabel: s__('MergeChecks|All threads must be resolved'), + lockedText: s__( + 'MergeChecks|This setting is configured in group %{groupName} and can only be changed in the group settings by an administrator or group owner.', + ), + lockedUponPipelineMustSucceed: s__('MergeChecks|Enable "Pipelines must succeed" first.'), +}; diff --git a/ee/app/assets/javascripts/merge_checks/index.js b/ee/app/assets/javascripts/merge_checks/index.js new file mode 100644 index 00000000000000..dfcd4966013522 --- /dev/null +++ b/ee/app/assets/javascripts/merge_checks/index.js @@ -0,0 +1,36 @@ +import Vue from 'vue'; +import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; + +export const initMergeRequestMergeChecksApp = async () => { + const el = document.querySelector('.js-merge-request-merge-checks'); + + if (!el) { + return false; + } + + const { default: MergeChecksApp } = await import( + /* webpackChunkName: 'mergeChecksApp' */ './components/merge_checks_app.vue' + ); + const { sourceType, settings, groupName } = el.dataset; + + const { + allowMergeOnSkippedPipeline, + onlyAllowMergeIfAllResolved, + pipelineMustSucceed, + } = convertObjectPropsToCamelCase(JSON.parse(settings)); + + return new Vue({ + el, + name: 'MergeChecksRoot', + provide: { + groupName, + sourceType, + allowMergeOnSkippedPipeline, + onlyAllowMergeIfAllResolved, + pipelineMustSucceed, + }, + render(createElement) { + return createElement(MergeChecksApp); + }, + }); +}; diff --git a/ee/app/assets/javascripts/pages/groups/edit/index.js b/ee/app/assets/javascripts/pages/groups/edit/index.js index 6b6835f57e6fe1..80f8c010f54fbe 100644 --- a/ee/app/assets/javascripts/pages/groups/edit/index.js +++ b/ee/app/assets/javascripts/pages/groups/edit/index.js @@ -4,7 +4,7 @@ import validateRestrictedIpAddress from 'ee/groups/settings/access_restriction_f import { initGroupPermissionsFormSubmit } from 'ee/groups/settings/permissions'; import { initServicePingSettingsClickTracking } from 'ee/registration_features_discovery_message'; import { createAlert } from '~/flash'; -import { initMergeRequestMergeChecksApp } from 'ee/projects/merge_requests/merge_checks'; +import { initMergeRequestMergeChecksApp } from 'ee/merge_checks'; import { __ } from '~/locale'; if (gon.features.saasUserCapsAutoApprovePendingUsersOnCapIncrease) { diff --git a/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js b/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js index 3176246bb1fb8e..fa13b027aa4875 100644 --- a/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js +++ b/ee/app/assets/javascripts/pages/projects/settings/merge_requests/index.js @@ -1,7 +1,7 @@ import '~/pages/projects/settings/merge_requests'; import mountApprovals from 'ee/approvals/mount_project_settings'; import { initMergeOptionSettings } from 'ee/pages/projects/edit/merge_options'; -import { initMergeRequestMergeChecksApp } from 'ee/projects/merge_requests/merge_checks'; +import { initMergeRequestMergeChecksApp } from 'ee/merge_checks'; import mountStatusChecks from 'ee/status_checks/mount'; mountApprovals(document.getElementById('js-mr-approvals-settings')); diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue deleted file mode 100644 index 4f301e34eaa8d9..00000000000000 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/components/merge_checks_app.vue +++ /dev/null @@ -1,116 +0,0 @@ - - diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js deleted file mode 100644 index 7aae8107bcede4..00000000000000 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/constants.js +++ /dev/null @@ -1,23 +0,0 @@ -import { s__ } from '~/locale'; - -export const pipelineMustSucceedI18n = { - label: s__('ProjectSettings|Pipelines must succeed'), - help: s__( - "ProjectSettings|Merge requests can't be merged if the latest pipeline did not succeed or is still running.", - ), -}; - -export const allowMergeOnSkippedI18n = { - label: s__('ProjectSettings|Skipped pipelines are considered successful'), - help: s__( - 'ProjectSettings|Introduces the risk of merging changes that do not pass the pipeline.', - ), -}; - -export const onlyMergeWhenAllResolvedLabelI18n = s__( - 'ProjectSettings|All threads must be resolved', -); - -export const lockedTextI18n = s__( - 'ApprovalSettings|This setting is configured in %{groupName} and can only be changed in the group settings by an administrator or group owner.', -); diff --git a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js b/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js deleted file mode 100644 index 1d4bbbe3d50e22..00000000000000 --- a/ee/app/assets/javascripts/projects/merge_requests/merge_checks/index.js +++ /dev/null @@ -1,32 +0,0 @@ -import Vue from 'vue'; -import MergeChecksApp from './components/merge_checks_app.vue'; - -export const initMergeRequestMergeChecksApp = () => { - const el = document.querySelector('.js-merge-request-merge-checks'); - - if (!el) { - return false; - } - const { - type, - allowMergeOnSkippedPipeline, - onlyAllowMergeIfAllResolved, - pipelineMustSucceed, - groupName, - } = el.dataset; - - return new Vue({ - el, - name: 'MergeRequestChecksRoot', - provide: { - groupName, - type, - allowMergeOnSkippedPipeline: JSON.parse(allowMergeOnSkippedPipeline), - onlyAllowMergeIfAllResolved: JSON.parse(onlyAllowMergeIfAllResolved), - pipelineMustSucceed: JSON.parse(pipelineMustSucceed), - }, - render(createElement) { - return createElement(MergeChecksApp); - }, - }); -}; diff --git a/ee/app/helpers/merge_checks_helper.rb b/ee/app/helpers/merge_checks_helper.rb new file mode 100644 index 00000000000000..f087a801407cd6 --- /dev/null +++ b/ee/app/helpers/merge_checks_helper.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module MergeChecksHelper + def merge_checks(source) + case source + when Group + target = source.namespace_settings + source_type = 'namespace_setting' + group_name = source.name + when Project + target = source + source_type = 'project' + # When the project has no group it should be an empty strings + group_name = source.group.nil? ? '' : source.group.name + end + + { + source_type: source_type, + settings: { + pipeline_must_succeed: { + locked: target.only_allow_merge_if_pipeline_succeeds_locked?, + value: target.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) + }, + allow_merge_on_skipped_pipeline: { + locked: target.allow_merge_on_skipped_pipeline_locked?, + value: target.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) + }, + only_allow_merge_if_all_resolved: { + locked: target.only_allow_merge_if_all_discussions_are_resolved_locked?, + value: target.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true) + } + }.to_json, + group_name: group_name + } + end +end diff --git a/ee/app/views/groups/_merge_requests.html.haml b/ee/app/views/groups/_merge_requests.html.haml index 30dcb25404de2b..3fbde12a8425e1 100644 --- a/ee/app/views/groups/_merge_requests.html.haml +++ b/ee/app/views/groups/_merge_requests.html.haml @@ -1,11 +1,10 @@ - if Feature.enabled?(:support_group_level_merge_checks_setting, @group) && @group.licensed_feature_available?(:group_level_merge_checks_setting) - %section.settings.gs-merge-requests.no-animate#js-merge-requests-settings{ class: ('expanded' if expanded) } + %section.settings.no-animate#js-merge-requests-settings{ class: ('expanded' if expanded) } .settings-header %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } = _('Merge requests') = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do = expanded ? _('Collapse') : _('Expand') - %p - %p= s_('GroupSettings|Choose the merge request checks for the projects in this group.') + %p= s_('GroupSettings|Choose the merge request checks for projects in this group. This setting overrides the same settings configured on each project in this group.') .settings-content - = render 'groups/settings/merge_requests' + = render 'groups/settings/merge_request_pipelines_and_threads_options' diff --git a/ee/app/views/groups/settings/_merge_request_pipelines_and_threads_options.html.haml b/ee/app/views/groups/settings/_merge_request_pipelines_and_threads_options.html.haml new file mode 100644 index 00000000000000..e4887be42aaa2c --- /dev/null +++ b/ee/app/views/groups/settings/_merge_request_pipelines_and_threads_options.html.haml @@ -0,0 +1,10 @@ += gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: 'merge-request-settings-form js-mr-settings-form' }, authenticity_token: true do |f| + %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-settings' } + + %fieldset.form-group.gl-form-group + %h5= s_('ProjectSettings|Merge checks') + %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') + + .js-merge-request-merge-checks{ data: merge_checks(@group) } + + = f.submit _('Save changes'), data: { testid: 'save_merge_request_changes' }, pajamas_button: true diff --git a/ee/app/views/groups/settings/_merge_requests.html.haml b/ee/app/views/groups/settings/_merge_requests.html.haml deleted file mode 100644 index de9cb1461baa65..00000000000000 --- a/ee/app/views/groups/settings/_merge_requests.html.haml +++ /dev/null @@ -1,15 +0,0 @@ -= gitlab_ui_form_for @group.namespace_settings, url: group_settings_merge_requests_path(@group), html: { multipart: true, class: 'merge-request-settings-form js-mr-settings-form' }, authenticity_token: true do |f| - %input{ name: 'update_section', type: 'hidden', value: 'js-merge-request-settings' } - - %fieldset.form-group.gl-form-group - %h5= s_('ProjectSettings|Merge checks') - %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged.') - - .builds-feature.js-merge-request-merge-checks{ data: { type: 'namespace_setting', pipeline_must_succeed: { locked: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds_locked?, value: @group.namespace_settings.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) }.to_json, - allow_merge_on_skipped_pipeline: { locked: @group.namespace_settings.allow_merge_on_skipped_pipeline_locked?, value: @group.namespace_settings.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) }.to_json, - only_allow_merge_if_all_resolved: { locked: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @group.namespace_settings.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true) }.to_json, - group_name: @group.name - } - } - - = f.submit _('Save changes'), class: 'rspec-save-merge-request-changes', pajamas_button: true diff --git a/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml b/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml index 12f119a66f7852..a15e1ae2018476 100644 --- a/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml +++ b/ee/app/views/projects/_merge_request_pipelines_and_threads_options.html.haml @@ -3,9 +3,4 @@ - if !support_group_level_mr_setting = render_ce 'projects/merge_request_pipelines_and_threads_options', form: form, project: project - else - .builds-feature.js-merge-request-merge-checks{ data: { type: 'project', pipeline_must_succeed: { locked: @project.only_allow_merge_if_pipeline_succeeds_locked?, value: @project.only_allow_merge_if_pipeline_succeeds?(inherit_group_setting: true) }.to_json, - allow_merge_on_skipped_pipeline: { locked: @project.allow_merge_on_skipped_pipeline_locked?, value: @project.allow_merge_on_skipped_pipeline?(inherit_group_setting: true) }.to_json, - only_allow_merge_if_all_resolved: { locked: @project.only_allow_merge_if_all_discussions_are_resolved_locked?, value: @project.only_allow_merge_if_all_discussions_are_resolved?(inherit_group_setting: true) }.to_json, - group_name: @project.name - } - } + .js-merge-request-merge-checks{ data: merge_checks(@project) } diff --git a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb index 2b9d0470bb3deb..b7405efea33373 100644 --- a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb @@ -2,44 +2,9 @@ require 'spec_helper' RSpec.describe 'Group settings > [EE] General', feature_category: :code_review_workflow do - let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:sub_group) { create(:group, parent: group) } + let(:merge_requests_settings_path) { edit_group_path(sub_group) } - before do - sign_in(user) - group.add_owner(user) - stub_licensed_features(group_level_merge_checks_setting: true) - end - - describe 'Merge checks', :js do - context 'when checkboxs is locked' do - it 'shows disabled status' do - checkboxs_selectors = %w[ - [name="namespace_setting[only_allow_merge_if_pipeline_succeeds]"][disabled] - [name="namespace_setting[allow_merge_on_skipped_pipeline]"][disabled] - [name="namespace_setting[only_allow_merge_if_all_discussions_are_resolved]"][disabled] - ] - - visit(edit_group_path(sub_group)) - - checkboxs_selectors.each do |selector| - expect(page).not_to have_selector(selector) - end - - group.namespace_settings.update!( - only_allow_merge_if_pipeline_succeeds: true, - allow_merge_on_skipped_pipeline: true, - only_allow_merge_if_all_discussions_are_resolved: true - ) - - visit(edit_group_path(sub_group)) - - checkboxs_selectors.each do |selector| - expect(page).to have_selector(selector) - expect(page.find(selector)).to be_checked - end - end - end - end + it_behaves_like 'MR checks settings' end diff --git a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb index 27b4a64ebae4bd..5ee220a4bbc585 100644 --- a/ee/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -14,7 +14,6 @@ project.add_maintainer(user) group.add_owner(user) group.add_developer(group_member) - stub_licensed_features(group_level_merge_checks_setting: true) end context 'Status checks' do @@ -132,34 +131,9 @@ end end - describe 'Merge checks' do - context 'when checkboxs is locked', :js do - it 'shows disabled status' do - checkboxs_selectors = %w[ - [name="project[only_allow_merge_if_pipeline_succeeds]"][disabled] - [name="project[allow_merge_on_skipped_pipeline]"][disabled] - [name="project[only_allow_merge_if_all_discussions_are_resolved]"][disabled] - ] + context 'MR checks' do + let(:merge_requests_settings_path) { project_settings_merge_requests_path(project) } - visit(project_settings_merge_requests_path(project)) - - checkboxs_selectors.each do |selector| - expect(page).not_to have_selector(selector) - end - - group.namespace_settings.update!( - only_allow_merge_if_pipeline_succeeds: true, - allow_merge_on_skipped_pipeline: true, - only_allow_merge_if_all_discussions_are_resolved: true - ) - - visit(project_settings_merge_requests_path(project)) - - checkboxs_selectors.each do |selector| - expect(page).to have_selector(selector) - expect(page.find(selector)).to be_checked - end - end - end + it_behaves_like 'MR checks settings' end end diff --git a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js index 4c4738799fbdf2..436fba743c6a2b 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js @@ -11,12 +11,19 @@ describe('ApprovalSettingsCheckbox', () => { let wrapper; - const createWrapper = (props = {}) => { + const createWrapper = (props = {}, slots = {}) => { wrapper = shallowMountExtended(ApprovalSettingsCheckbox, { propsData: { label, lockedText, ...props }, + slots, stubs: { GlFormCheckbox: stubComponent(GlFormCheckbox, { props: ['checked'], + template: ` +
+ + +
+ `, }), }, }); @@ -95,4 +102,18 @@ describe('ApprovalSettingsCheckbox', () => { }); }); }); + + describe('#slot', () => { + it('should render a default slot', () => { + const slotContent = 'test slot content'; + createWrapper( + {}, + { + help: slotContent, + }, + ); + + expect(wrapper.text()).toContain(slotContent); + }); + }); }); diff --git a/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js b/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js index 2106b1ef0ed075..4471ca46b101ed 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_locked_icon_spec.js @@ -4,13 +4,8 @@ import { shallowMount } from '@vue/test-utils'; import ApprovalSettingsLockedIcon from 'ee/approvals/components/approval_settings_locked_icon.vue'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; -let id = 0; - describe('ApprovalSettingsLockedIcon', () => { const label = 'Foo'; - // eslint-disable-next-line no-plusplus - const lockIconId = () => `approval-settings-lock-icon-${++id}`; - let wrapper; const createWrapper = (props = {}) => { @@ -38,11 +33,11 @@ describe('ApprovalSettingsLockedIcon', () => { it('shows a lock icon', () => { expect(findLockIcon().props('name')).toBe('lock'); - expect(findLockIcon().attributes('id')).toBe(lockIconId()); + expect(findLockIcon().attributes('id')).toMatch(/approval-settings-lock-icon-\d+/); }); it('shows a popover for the lock icon', () => { - expect(findPopover().props('target')).toBe(lockIconId()); + expect(findPopover().props('target')).toBe(findLockIcon().attributes('id')); }); it('configures how and when the popover should show', () => { diff --git a/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js b/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js new file mode 100644 index 00000000000000..911b907ebefaeb --- /dev/null +++ b/ee/spec/frontend/merge_checks/components/merge_checks_app_spec.js @@ -0,0 +1,134 @@ +import { sprintf } from '~/locale'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import MergeChecksApp from 'ee/merge_checks/components/merge_checks_app.vue'; +import { I18N } from 'ee/merge_checks/constants'; + +describe('MergeChecksApp', () => { + const defaultType = 'project'; + const defaultGroupName = 'test'; + const defaultCheckValue = { + locked: false, + value: false, + }; + + let wrapper; + + const createWrapper = (provides = {}) => { + wrapper = shallowMountExtended(MergeChecksApp, { + provide: { + sourceType: defaultType, + groupName: defaultGroupName, + pipelineMustSucceed: defaultCheckValue, + allowMergeOnSkippedPipeline: defaultCheckValue, + onlyAllowMergeIfAllResolved: defaultCheckValue, + ...provides, + }, + }); + }; + + const findOnlyAllowMergeWhenPipelineSucceeds = () => + wrapper.findByTestId('allow_merge_if_pipeline_succeeds_checkbox'); + + const findOnlyAllowMergeWhenPipelineSucceedsInput = () => + wrapper.find('input[name$="[only_allow_merge_if_pipeline_succeeds]"]'); + + describe('sourceType', () => { + it('with default sourceType', () => { + createWrapper(); + expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( + `${defaultType}[only_allow_merge_if_pipeline_succeeds]`, + ); + }); + + it('with other sourceType', () => { + const sourceType = 'new-type'; + createWrapper({ + sourceType, + }); + + expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( + `${sourceType}[only_allow_merge_if_pipeline_succeeds]`, + ); + }); + }); + + describe('group name', () => { + it('with default groupName', () => { + createWrapper(); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(I18N.lockedText, { groupName: defaultGroupName }), + ); + }); + + it('with other groupName', () => { + const groupName = 'new-name'; + createWrapper({ groupName }); + + expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( + sprintf(I18N.lockedText, { groupName }), + ); + }); + }); + + describe('binding value', () => { + it('should be able to bind and mutate value', async () => { + createWrapper(); + const checkbox = findOnlyAllowMergeWhenPipelineSucceeds(); + + expect(checkbox.props('checked')).toBe(defaultCheckValue.value); + + await checkbox.vm.$emit('input'); + expect(checkbox.props('checked')).toBe(true); + await checkbox.vm.$emit('input'); + expect(checkbox.props('checked')).toBe(false); + }); + }); + + describe('skipped pipeline', () => { + const findSkippedPipelineCheckbox = () => + wrapper.findByTestId('allow_merge_on_skipped_pipeline_checkbox'); + + it('should be corresponding with pipeline must succeeds', () => { + createWrapper(); + + expect(findSkippedPipelineCheckbox().props('locked')).toBe(true); + expect(findSkippedPipelineCheckbox().props('lockedText')).toBe( + I18N.lockedUponPipelineMustSucceed, + ); + + createWrapper({ + pipelineMustSucceed: { + value: true, + locked: false, + }, + }); + + expect(findSkippedPipelineCheckbox().props('locked')).toBe(false); + expect(findSkippedPipelineCheckbox().props('lockedText')).toBe( + sprintf(I18N.lockedText, { groupName: defaultGroupName }), + ); + }); + + it('should be locked and unchecked when preceding condition unmet', async () => { + createWrapper({ + pipelineMustSucceed: { + value: true, + locked: false, + }, + allowMergeOnSkippedPipeline: { + value: true, + locked: false, + }, + }); + + expect(findSkippedPipelineCheckbox().props('locked')).toBe(false); + expect(findSkippedPipelineCheckbox().props('checked')).toBe(true); + + await findOnlyAllowMergeWhenPipelineSucceeds().vm.$emit('input'); + + expect(findSkippedPipelineCheckbox().props('locked')).toBe(true); + expect(findSkippedPipelineCheckbox().props('checked')).toBe(false); + }); + }); +}); diff --git a/ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js b/ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js deleted file mode 100644 index 4ad87a91817f93..00000000000000 --- a/ee/spec/frontend/projects/merge_requests/merge_checks/components/merge_checks_app_spec.js +++ /dev/null @@ -1,91 +0,0 @@ -import { sprintf } from '~/locale'; -import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; -import MergeChecksApp from 'ee/projects/merge_requests/merge_checks/components/merge_checks_app.vue'; -import { lockedTextI18n } from 'ee/projects/merge_requests/merge_checks/constants'; - -describe('', () => { - const defaultType = 'project'; - const defaultGroupName = 'test'; - const defaultCheckValue = { - locked: false, - value: false, - }; - - let wrapper; - - const createWrapper = (provides = {}) => { - wrapper = shallowMountExtended(MergeChecksApp, { - provide: { - type: defaultType, - groupName: defaultGroupName, - pipelineMustSucceed: defaultCheckValue, - allowMergeOnSkippedPipeline: defaultCheckValue, - onlyAllowMergeIfAllResolved: defaultCheckValue, - ...provides, - }, - }); - }; - - const findOnlyAllowMergeWhenPipelineSucceeds = () => - wrapper.findByTestId('only_allow_merge_if_pipeline_succeeds_checkbox'); - - const findOnlyAllowMergeWhenPipelineSucceedsInput = () => - wrapper.find('input[name*="only_allow_merge_if_pipeline_succeeds"]'); - - afterEach(() => { - wrapper.destroy(); - }); - - describe('type', () => { - it('with default type', () => { - createWrapper(); - expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( - `${defaultType}[only_allow_merge_if_pipeline_succeeds]`, - ); - }); - - it('with other type', () => { - const type = 'new-type'; - createWrapper({ - type, - }); - - expect(findOnlyAllowMergeWhenPipelineSucceedsInput().attributes('name')).toBe( - `${type}[only_allow_merge_if_pipeline_succeeds]`, - ); - }); - }); - - describe('group name', () => { - it('with default name', () => { - createWrapper(); - - expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( - sprintf(lockedTextI18n, { groupName: defaultGroupName }), - ); - }); - - it('with other name', () => { - const groupName = 'new-name'; - createWrapper({ groupName }); - - expect(findOnlyAllowMergeWhenPipelineSucceeds().attributes('lockedtext')).toBe( - sprintf(lockedTextI18n, { groupName }), - ); - }); - }); - - describe('binding value', () => { - it('should be able to bind and mutate value', async () => { - createWrapper(); - const checkbox = findOnlyAllowMergeWhenPipelineSucceeds(); - - expect(checkbox.props('checked')).toBe(defaultCheckValue.value); - - await checkbox.vm.$emit('input'); - expect(checkbox.props('checked')).toBe(true); - await checkbox.vm.$emit('input'); - expect(checkbox.props('checked')).toBe(false); - }); - }); -}); diff --git a/ee/spec/helpers/merge_checks_helper_spec.rb b/ee/spec/helpers/merge_checks_helper_spec.rb new file mode 100644 index 00000000000000..5e3b0a50815aa7 --- /dev/null +++ b/ee/spec/helpers/merge_checks_helper_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe MergeChecksHelper, type: :helper, feature_category: :code_review_workflow do + let_it_be(:group) { create_default(:group, :public) } + let_it_be(:project) { create_default(:project, group: group) } + + before do + group.namespace_settings.update!(allow_merge_on_skipped_pipeline: true) + stub_licensed_features(group_level_merge_checks_setting: true) + end + + describe '#merge_checks' do + context 'when source is group' do + let(:source) { group } + + it 'returns the correct settings' do + expect(helper.merge_checks(source)).to eq({ + source_type: 'namespace_setting', + settings: { + pipeline_must_succeed: { + locked: false, + value: false + }, + allow_merge_on_skipped_pipeline: { + locked: false, + value: true + }, + only_allow_merge_if_all_resolved: { + locked: false, + value: false + } + }.to_json, + group_name: group.name + }) + end + end + + context 'when source is project' do + let(:source) { project } + + it 'returns the correct settings' do + expect(helper.merge_checks(source)).to eq({ + source_type: 'project', + settings: { + pipeline_must_succeed: { + locked: false, + value: false + }, + allow_merge_on_skipped_pipeline: { + locked: true, + value: true + }, + only_allow_merge_if_all_resolved: { + locked: false, + value: false + } + }.to_json, + group_name: group.name + }) + end + end + end +end diff --git a/ee/spec/support/shared_examples/features/merge_requests_settings_shared_examples.rb b/ee/spec/support/shared_examples/features/merge_requests_settings_shared_examples.rb new file mode 100644 index 00000000000000..5f4e248007e75d --- /dev/null +++ b/ee/spec/support/shared_examples/features/merge_requests_settings_shared_examples.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +RSpec.shared_examples_for 'MR checks settings' do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + group.add_owner(user) + stub_licensed_features(group_level_merge_checks_setting: true) + end + + context 'when checkboxes are not locked', :js do + it 'shows initial status' do + visit(merge_requests_settings_path) + + expect(page).not_to have_selector( + '[data-testid="allow_merge_if_pipeline_succeeds_checkbox"]>input[disabled]' + ) + expect(page).to have_selector('[data-testid="allow_merge_on_skipped_pipeline_checkbox"]>input[disabled]') + expect(page).not_to have_selector( + '[data-testid="allow_merge_if_all_discussions_are_resolved_checkbox"]>input[disabled]' + ) + end + end + + context 'when checkboxes are locked', :js do + before do + group.namespace_settings.update!( + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, + only_allow_merge_if_all_discussions_are_resolved: true + ) + end + + it 'shows disabled status' do + checkboxs_selectors = %w[ + [data-testid="allow_merge_if_pipeline_succeeds_checkbox"]>input[disabled] + [data-testid="allow_merge_on_skipped_pipeline_checkbox"]>input[disabled] + [data-testid="allow_merge_if_all_discussions_are_resolved_checkbox"]>input[disabled] + ] + + visit(merge_requests_settings_path) + + checkboxs_selectors.each do |selector| + expect(page).to have_selector(selector) + expect(page.find(selector)).to be_checked + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d32aa8f965351f..1b643d9b9421bd 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20108,7 +20108,7 @@ msgstr "" msgid "GroupSettings|Choose a group path that does not start with a dash or end with a period. It can also contain alphanumeric characters and underscores." msgstr "" -msgid "GroupSettings|Choose the merge request checks for the projects in this group." +msgid "GroupSettings|Choose the merge request checks for projects in this group. This setting overrides the same settings configured on each project in this group." msgstr "" msgid "GroupSettings|Compliance frameworks" @@ -26551,6 +26551,27 @@ msgstr "" msgid "Merge..." msgstr "" +msgid "MergeChecks|All threads must be resolved" +msgstr "" + +msgid "MergeChecks|Enable \"Pipelines must succeed\" first." +msgstr "" + +msgid "MergeChecks|Introduces the risk of merging changes that do not pass the pipeline." +msgstr "" + +msgid "MergeChecks|Merge requests can't be merged if the latest pipeline did not succeed or is still running." +msgstr "" + +msgid "MergeChecks|Pipelines must succeed" +msgstr "" + +msgid "MergeChecks|Skipped pipelines are considered successful" +msgstr "" + +msgid "MergeChecks|This setting is configured in group %{groupName} and can only be changed in the group settings by an administrator or group owner." +msgstr "" + msgid "MergeConflict|Commit to source branch" msgstr "" @@ -33478,9 +33499,6 @@ msgstr "" msgid "ProjectSettings|All merge requests and commits are made against this branch unless you specify a different one." msgstr "" -msgid "ProjectSettings|All threads must be resolved" -msgstr "" - msgid "ProjectSettings|Allow" msgstr "" @@ -33631,9 +33649,6 @@ msgstr "" msgid "ProjectSettings|Internal" msgstr "" -msgid "ProjectSettings|Introduces the risk of merging changes that do not pass the pipeline." -msgstr "" - msgid "ProjectSettings|Issues" msgstr "" @@ -33676,9 +33691,6 @@ msgstr "" msgid "ProjectSettings|Merge requests approved for merge are queued, and pipelines validate the combined results of the source and target branches before merge. %{link_start}What are merge trains?%{link_end}" msgstr "" -msgid "ProjectSettings|Merge requests can't be merged if the latest pipeline did not succeed or is still running." -msgstr "" - msgid "ProjectSettings|Merge suggestions" msgstr "" @@ -33715,9 +33727,6 @@ msgstr "" msgid "ProjectSettings|Pages for project documentation." msgstr "" -msgid "ProjectSettings|Pipelines must succeed" -msgstr "" - msgid "ProjectSettings|Prevents direct linking to potentially sensitive media files" msgstr "" @@ -33787,9 +33796,6 @@ msgstr "" msgid "ProjectSettings|Show link to create or view a merge request when pushing from the command line" msgstr "" -msgid "ProjectSettings|Skipped pipelines are considered successful" -msgstr "" - msgid "ProjectSettings|Snippets" msgstr "" diff --git a/qa/qa/page/project/settings/merge_request.rb b/qa/qa/page/project/settings/merge_request.rb index ae6a04028c9d1a..79f619130ca256 100644 --- a/qa/qa/page/project/settings/merge_request.rb +++ b/qa/qa/page/project/settings/merge_request.rb @@ -16,7 +16,11 @@ class MergeRequest < QA::Page::Base end view 'app/views/projects/_merge_request_pipelines_and_threads_options.html.haml' do - element :allow_merge_if_all_discussions_are_resolved_checkbox + element :only_allow_merge_if_all_discussions_are_resolved_checkbox + end + + view 'ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue' do + element :only_allow_merge_if_all_discussions_are_resolved_checkbox end def click_save_changes @@ -29,7 +33,7 @@ def enable_ff_only end def enable_merge_if_all_disscussions_are_resolved - check_element(:allow_merge_if_all_discussions_are_resolved_checkbox, true) + check_element(:only_allow_merge_if_all_discussions_are_resolved_checkbox, true) click_save_changes end end -- GitLab From ad6ffeb802ed59359b38e1fc64ec9c7f28b59032 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Wed, 15 Feb 2023 11:52:40 +0800 Subject: [PATCH 10/11] Adjust js spec of approval settings --- .../approval_settings_checkbox_spec.js | 34 ++++++++----------- .../ee/page/project/settings/merge_request.rb | 4 +++ qa/qa/page/project/settings/merge_request.rb | 4 --- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js index 436fba743c6a2b..0a4ea5c2acd1d1 100644 --- a/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js +++ b/ee/spec/frontend/approvals/components/approval_settings_checkbox_spec.js @@ -2,8 +2,7 @@ import { GlFormCheckbox } from '@gitlab/ui'; import ApprovalSettingsCheckbox from 'ee/approvals/components/approval_settings_checkbox.vue'; import ApprovalSettingsLockedIcon from 'ee/approvals/components/approval_settings_locked_icon.vue'; -import { stubComponent } from 'helpers/stub_component'; -import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; +import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper'; describe('ApprovalSettingsCheckbox', () => { const label = 'Foo'; @@ -11,20 +10,15 @@ describe('ApprovalSettingsCheckbox', () => { let wrapper; - const createWrapper = (props = {}, slots = {}) => { - wrapper = shallowMountExtended(ApprovalSettingsCheckbox, { + const createWrapper = ({ mountFn = shallowMountExtended, props = {}, slots = {} } = {}) => { + wrapper = mountFn(ApprovalSettingsCheckbox, { propsData: { label, lockedText, ...props }, slots, stubs: { - GlFormCheckbox: stubComponent(GlFormCheckbox, { - props: ['checked'], - template: ` -
- - -
- `, - }), + GlFormCheckbox: { + ...GlFormCheckbox, + props: ['checked', 'disabled'], + }, }, }); }; @@ -54,7 +48,7 @@ describe('ApprovalSettingsCheckbox', () => { }); it('sets the checkbox to `true` when checked is `true`', () => { - createWrapper({ checked: true }); + createWrapper({ props: { checked: true } }); expect(findCheckbox().props('checked')).toBe(true); }); @@ -85,11 +79,11 @@ describe('ApprovalSettingsCheckbox', () => { describe('when the setting is locked', () => { beforeEach(() => { - createWrapper({ locked: true }); + createWrapper({ props: { locked: true } }); }); it('disables the input', () => { - expect(findCheckbox().attributes('disabled')).toBe('disabled'); + expect(findCheckbox().props('disabled')).toBe(true); }); it('renders locked_icon', () => { @@ -106,12 +100,12 @@ describe('ApprovalSettingsCheckbox', () => { describe('#slot', () => { it('should render a default slot', () => { const slotContent = 'test slot content'; - createWrapper( - {}, - { + createWrapper({ + mountFn: mountExtended, + slots: { help: slotContent, }, - ); + }); expect(wrapper.text()).toContain(slotContent); }); diff --git a/qa/qa/ee/page/project/settings/merge_request.rb b/qa/qa/ee/page/project/settings/merge_request.rb index d32cb2cba669b2..588e5d893cc67d 100644 --- a/qa/qa/ee/page/project/settings/merge_request.rb +++ b/qa/qa/ee/page/project/settings/merge_request.rb @@ -25,6 +25,10 @@ def self.prepended(base) view 'ee/app/views/projects/_merge_trains_settings.html.haml' do element :merge_trains_checkbox end + + view 'ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue' do + element :only_allow_merge_if_all_discussions_are_resolved_checkbox + end end end diff --git a/qa/qa/page/project/settings/merge_request.rb b/qa/qa/page/project/settings/merge_request.rb index 79f619130ca256..c42a4e0bebed3e 100644 --- a/qa/qa/page/project/settings/merge_request.rb +++ b/qa/qa/page/project/settings/merge_request.rb @@ -19,10 +19,6 @@ class MergeRequest < QA::Page::Base element :only_allow_merge_if_all_discussions_are_resolved_checkbox end - view 'ee/app/assets/javascripts/merge_checks/components/merge_checks_app.vue' do - element :only_allow_merge_if_all_discussions_are_resolved_checkbox - end - def click_save_changes click_element(:save_merge_request_changes_button) end -- GitLab From d7a4a03c6c0760aaf158ed185c4e3a433e4f80d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Thu, 16 Feb 2023 09:33:30 +0800 Subject: [PATCH 11/11] Fix rubocop with a new blank line --- ee/spec/features/groups/settings/merge_requests_settings_spec.rb | 1 + ee/spec/helpers/merge_checks_helper_spec.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb index b7405efea33373..6b95c1edfe11c5 100644 --- a/ee/spec/features/groups/settings/merge_requests_settings_spec.rb +++ b/ee/spec/features/groups/settings/merge_requests_settings_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'spec_helper' RSpec.describe 'Group settings > [EE] General', feature_category: :code_review_workflow do diff --git a/ee/spec/helpers/merge_checks_helper_spec.rb b/ee/spec/helpers/merge_checks_helper_spec.rb index 5e3b0a50815aa7..853fbcb1b4ad57 100644 --- a/ee/spec/helpers/merge_checks_helper_spec.rb +++ b/ee/spec/helpers/merge_checks_helper_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'spec_helper' RSpec.describe MergeChecksHelper, type: :helper, feature_category: :code_review_workflow do -- GitLab