From eb189965854880f8562e22535b2b3051e3e18407 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Tue, 9 Jun 2020 12:20:02 -0500 Subject: [PATCH 1/3] Initial iteration for squash and merge feature spec --- .../user_edits_merge_request_spec.rb | 22 +++++++++++++++++++ ...er_manages_merge_requests_settings_spec.rb | 6 +++++ 2 files changed, 28 insertions(+) diff --git a/spec/features/merge_request/user_edits_merge_request_spec.rb b/spec/features/merge_request/user_edits_merge_request_spec.rb index 84ecd6537dccfc..b435d2337dbd80 100644 --- a/spec/features/merge_request/user_edits_merge_request_spec.rb +++ b/spec/features/merge_request/user_edits_merge_request_spec.rb @@ -4,6 +4,7 @@ RSpec.describe 'User edits a merge request', :js do include Select2Helper + using RSpec::Parameterized::TableSyntax let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } @@ -16,6 +17,27 @@ visit(edit_project_merge_request_path(project, merge_request)) end + context "when the Squash-and-Merge feature is enabled" do + where(:squash_option, :checkbox_selected, :checkbox_disabled) do + "always_squash" | true | true + "never_squash" | false | true + "enabled_with_default_on" | true | false + "enabled_with_default_off" | false | false + end + + with_them do + let(:project) { create(:project, :repository, squash_option: squash_option) } + + it 'the checkbox reflects the correct option' do + visit(edit_project_merge_request_path(project, merge_request)) + + checkbox = find("#merge_request_squash") + expect(checkbox.disabled?).to eql(checkbox_disabled) + expect(checkbox.selected?).to eql(checkbox_selected) + end + end + end + it 'changes the target branch' do expect(page).to have_content('From master into feature') diff --git a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb index 3fc1f47d98ab8e..1e2e7e6507a7aa 100644 --- a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb +++ b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb @@ -28,6 +28,12 @@ end end + it 'shows "Squash commits while merging" settings' do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Squashing is never performed and option is hidden.' + end + end + context 'when Merge Request and Pipelines are initially enabled', :js do context 'when Pipelines are initially enabled' do it 'shows the Merge Requests settings' do -- GitLab From 92411fc9098d354c2f41aa0964930d7da05189dd Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Thu, 4 Jun 2020 01:47:03 -0700 Subject: [PATCH 2/3] Configurable defaults for "Squash commits" option Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/17613 This commit adds the capability to set squash commits options. The configuration is done on a project settings level. It allows configuration for squashing commits before merging. --- app/helpers/issuables_helper.rb | 8 ++ .../_merge_request_settings.html.haml | 3 + ..._request_squash_options_settings.html.haml | 42 ++++++++++ app/views/shared/issuable/_form.html.haml | 2 +- .../issuable/form/_merge_params.html.haml | 18 ++-- .../17613-fe-squash-commits-options.yml | 5 ++ .../_merge_request_settings.html.haml | 3 + locale/gitlab.pot | 30 +++++++ .../user_edits_merge_request_spec.rb | 22 ----- ...er_manages_merge_requests_settings_spec.rb | 82 ++++++++++++++++++- spec/helpers/issuables_helper_spec.rb | 24 ++++++ 11 files changed, 207 insertions(+), 32 deletions(-) create mode 100644 app/views/projects/_merge_request_squash_options_settings.html.haml create mode 100644 changelogs/unreleased/17613-fe-squash-commits-options.yml diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 045174de25be41..dccb89eec79e54 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -385,6 +385,14 @@ def assignee_sidebar_data(assignee, merge_request: nil) end end + def issuable_squash_option?(issuable, project) + if issuable.persisted? + issuable.squash + else + project.squash_enabled_by_default? + end + end + private def sidebar_gutter_collapsed? diff --git a/app/views/projects/_merge_request_settings.html.haml b/app/views/projects/_merge_request_settings.html.haml index dc3a3fcc6473cc..5ffdeef3558558 100644 --- a/app/views/projects/_merge_request_settings.html.haml +++ b/app/views/projects/_merge_request_settings.html.haml @@ -4,6 +4,9 @@ = render 'projects/merge_request_merge_options_settings', project: @project, form: form +- if Feature.enabled?(:squash_options, @project) + = render 'projects/merge_request_squash_options_settings', form: form + = render 'projects/merge_request_merge_checks_settings', project: @project, form: form = render 'projects/merge_request_merge_suggestions_settings', project: @project, form: form diff --git a/app/views/projects/_merge_request_squash_options_settings.html.haml b/app/views/projects/_merge_request_squash_options_settings.html.haml new file mode 100644 index 00000000000000..af8f5b405c4bf4 --- /dev/null +++ b/app/views/projects/_merge_request_squash_options_settings.html.haml @@ -0,0 +1,42 @@ +- form = local_assigns.fetch(:form) + += form.fields_for :project_setting do |settings| + .form-group + %b= s_('ProjectSettings|Squash commits when merging') + %p.text-secondary + = s_('ProjectSettings|ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests.') + = link_to "What is squashing?", + help_page_path('user/project/merge_requests/squash_and_merge.md'), + target: '_blank' + + .form-check.gl-mb-2 + = settings.radio_button :squash_option, :never, class: "form-check-input" + = label_tag :project_project_setting_attributes_squash_option_never, class: 'form-check-label' do + .gl-font-weight-bold + = s_('ProjectSettings|Do not allow') + .text-secondary + = s_('ProjectSettings|Squashing is never performed and the checkbox is hidden.') + + .form-check.gl-mb-2 + = settings.radio_button :squash_option, :default_off, class: "form-check-input" + = label_tag :project_project_setting_attributes_squash_option_default_off, class: 'form-check-label' do + .gl-font-weight-bold + = s_('ProjectSettings|Allow') + .text-secondary + = s_('ProjectSettings|Checkbox is visible and unselected by default.') + + .form-check.gl-mb-2 + = settings.radio_button :squash_option, :default_on, class: "form-check-input" + = label_tag :project_project_setting_attributes_squash_option_default_on, class: 'form-check-label' do + .gl-font-weight-bold + = s_('ProjectSettings|Encourage') + .text-secondary + = s_('ProjectSettings|Checkbox is visible and selected by default.') + + .form-check.gl-mb-2 + = settings.radio_button :squash_option, :always, class: "form-check-input" + = label_tag :project_project_setting_attributes_squash_option_always, class: 'form-check-label' do + .gl-font-weight-bold + = s_('ProjectSettings|Require') + .text-secondary + = s_('ProjectSettings|Squashing is always performed. Checkbox is visible and selected, and users cannot change it.') diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 1b3ad484bccce9..cc5bd4d67c9ec4 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -35,7 +35,7 @@ = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form -= render 'shared/issuable/form/merge_params', issuable: issuable += render 'shared/issuable/form/merge_params', issuable: issuable, project: project = render 'shared/issuable/form/contribution', issuable: issuable, form: form diff --git a/app/views/shared/issuable/form/_merge_params.html.haml b/app/views/shared/issuable/form/_merge_params.html.haml index 2cd4e0ba761edc..6523e733e44685 100644 --- a/app/views/shared/issuable/form/_merge_params.html.haml +++ b/app/views/shared/issuable/form/_merge_params.html.haml @@ -1,4 +1,5 @@ - issuable = local_assigns.fetch(:issuable) +- project = local_assigns.fetch(:project) - return unless issuable.is_a?(MergeRequest) - return if issuable.closed_without_fork? @@ -14,9 +15,14 @@ = check_box_tag 'merge_request[force_remove_source_branch]', '1', issuable.force_remove_source_branch?, class: 'form-check-input' = label_tag 'merge_request[force_remove_source_branch]', class: 'form-check-label' do Delete source branch when merge request is accepted. - .form-check - = hidden_field_tag 'merge_request[squash]', '0', id: nil - = check_box_tag 'merge_request[squash]', '1', issuable.squash, class: 'form-check-input' - = label_tag 'merge_request[squash]', class: 'form-check-label' do - Squash commits when merge request is accepted. - = link_to icon('question-circle'), help_page_path('user/project/merge_requests/squash_and_merge'), target: '_blank' + - if !project.squash_never? + .form-check + - if project.squash_always? + = hidden_field_tag 'merge_request[squash]', '1', id: nil + = check_box_tag 'merge_request[squash]', '1', project.squash_enabled_by_default?, class: 'form-check-input', disabled: 'true' + - else + = hidden_field_tag 'merge_request[squash]', '0', id: nil + = check_box_tag 'merge_request[squash]', '1', issuable_squash_option?(issuable, project), class: 'form-check-input' + = label_tag 'merge_request[squash]', class: 'form-check-label' do + Squash commits when merge request is accepted. + = link_to icon('question-circle'), help_page_path('user/project/merge_requests/squash_and_merge'), target: '_blank' diff --git a/changelogs/unreleased/17613-fe-squash-commits-options.yml b/changelogs/unreleased/17613-fe-squash-commits-options.yml new file mode 100644 index 00000000000000..07e7ea0e79e9cd --- /dev/null +++ b/changelogs/unreleased/17613-fe-squash-commits-options.yml @@ -0,0 +1,5 @@ +--- +title: Configurable defaults for Squash commits option +merge_request: 33827 +author: +type: added diff --git a/ee/app/views/projects/_merge_request_settings.html.haml b/ee/app/views/projects/_merge_request_settings.html.haml index 5b802533cdf9fd..6f50b3eda5e118 100644 --- a/ee/app/views/projects/_merge_request_settings.html.haml +++ b/ee/app/views/projects/_merge_request_settings.html.haml @@ -4,6 +4,9 @@ = render_ce 'projects/merge_request_merge_options_settings', project: @project, form: form +- if Feature.enabled?(:squash_options, @project) + = render_ce 'projects/merge_request_squash_options_settings', form: form + = render_ce 'projects/merge_request_merge_checks_settings', project: @project, form: form = render 'projects/merge_request_merge_suggestions_settings', project: @project, form: form diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e20caddaf826ea..be503d16c1a649 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17748,6 +17748,9 @@ msgstr "" msgid "ProjectSettings|All discussions must be resolved" msgstr "" +msgid "ProjectSettings|Allow" +msgstr "" + msgid "ProjectSettings|Allow users to make copies of your repository to a new project" msgstr "" @@ -17763,6 +17766,12 @@ msgstr "" msgid "ProjectSettings|Build, test, and deploy your changes" msgstr "" +msgid "ProjectSettings|Checkbox is visible and selected by default." +msgstr "" + +msgid "ProjectSettings|Checkbox is visible and unselected by default." +msgstr "" + msgid "ProjectSettings|Choose your merge method, merge options, merge checks, and merge suggestions." msgstr "" @@ -17781,12 +17790,18 @@ msgstr "" msgid "ProjectSettings|Disable email notifications" msgstr "" +msgid "ProjectSettings|Do not allow" +msgstr "" + msgid "ProjectSettings|Enable 'Delete source branch' option by default" msgstr "" msgid "ProjectSettings|Enable merge trains and pipelines for merged results" msgstr "" +msgid "ProjectSettings|Encourage" +msgstr "" + msgid "ProjectSettings|Every merge creates a merge commit" msgstr "" @@ -17895,12 +17910,18 @@ msgstr "" msgid "ProjectSettings|Project visibility" msgstr "" +msgid "ProjectSettings|ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests." +msgstr "" + msgid "ProjectSettings|Public" msgstr "" msgid "ProjectSettings|Repository" msgstr "" +msgid "ProjectSettings|Require" +msgstr "" + msgid "ProjectSettings|Share code pastes with others out of Git repository" msgstr "" @@ -17916,6 +17937,15 @@ msgstr "" msgid "ProjectSettings|Snippets" msgstr "" +msgid "ProjectSettings|Squash commits when merging" +msgstr "" + +msgid "ProjectSettings|Squashing is always performed. Checkbox is visible and selected, and users cannot change it." +msgstr "" + +msgid "ProjectSettings|Squashing is never performed and the checkbox is hidden." +msgstr "" + msgid "ProjectSettings|Submit changes to be merged upstream" msgstr "" diff --git a/spec/features/merge_request/user_edits_merge_request_spec.rb b/spec/features/merge_request/user_edits_merge_request_spec.rb index b435d2337dbd80..84ecd6537dccfc 100644 --- a/spec/features/merge_request/user_edits_merge_request_spec.rb +++ b/spec/features/merge_request/user_edits_merge_request_spec.rb @@ -4,7 +4,6 @@ RSpec.describe 'User edits a merge request', :js do include Select2Helper - using RSpec::Parameterized::TableSyntax let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } @@ -17,27 +16,6 @@ visit(edit_project_merge_request_path(project, merge_request)) end - context "when the Squash-and-Merge feature is enabled" do - where(:squash_option, :checkbox_selected, :checkbox_disabled) do - "always_squash" | true | true - "never_squash" | false | true - "enabled_with_default_on" | true | false - "enabled_with_default_off" | false | false - end - - with_them do - let(:project) { create(:project, :repository, squash_option: squash_option) } - - it 'the checkbox reflects the correct option' do - visit(edit_project_merge_request_path(project, merge_request)) - - checkbox = find("#merge_request_squash") - expect(checkbox.disabled?).to eql(checkbox_disabled) - expect(checkbox.selected?).to eql(checkbox_selected) - end - end - end - it 'changes the target branch' do expect(page).to have_content('From master into feature') diff --git a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb index 1e2e7e6507a7aa..bc90393e889028 100644 --- a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb +++ b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb @@ -28,9 +28,33 @@ end end - it 'shows "Squash commits while merging" settings' do - page.within '#js-merge-request-settings' do - expect(page).to have_content 'Squashing is never performed and option is hidden.' + describe 'Squash commits while merging' do + it 'shows "Do not allow" option' do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Do not allow' + expect(page).to have_content 'Squashing is never performed and the checkbox is hidden.' + end + end + + it 'shows "Allow" option' do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Allow' + expect(page).to have_content 'Checkbox is visible and unselected by default.' + end + end + + it 'shows "Encourage" option' do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Encourage' + expect(page).to have_content 'Checkbox is visible and selected by default.' + end + end + + it 'shows "Require" option' do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Require' + expect(page).to have_content 'Squashing is always performed. Checkbox is visible and selected, and users cannot change it.' + end end end @@ -136,4 +160,56 @@ expect(project.remove_source_branch_after_merge).to be(false) end end + + describe 'Squash commits when merging', :js do + it 'initially has :squash_option set to :default_off' do + radio = find_field('project_project_setting_attributes_squash_option_default_off') + expect(radio).to be_checked + end + + it 'allows :squash_option to be set to :default_on' do + choose('project_project_setting_attributes_squash_option_default_on') + + within('.merge-request-settings-form') do + find('.qa-save-merge-request-changes') + click_on('Save changes') + end + + find('.flash-notice') + radio = find_field('project_project_setting_attributes_squash_option_default_on') + + expect(radio).to be_checked + expect(project.reload.project_setting.squash_option).to eq('default_on') + end + + it 'allows :squash_option to be set to :always' do + choose('project_project_setting_attributes_squash_option_always') + + within('.merge-request-settings-form') do + find('.qa-save-merge-request-changes') + click_on('Save changes') + end + + find('.flash-notice') + radio = find_field('project_project_setting_attributes_squash_option_always') + + expect(radio).to be_checked + expect(project.reload.project_setting.squash_option).to eq('always') + end + + it 'allows :squash_option to be set to :never' do + choose('project_project_setting_attributes_squash_option_never') + + within('.merge-request-settings-form') do + find('.qa-save-merge-request-changes') + click_on('Save changes') + end + + find('.flash-notice') + radio = find_field('project_project_setting_attributes_squash_option_never') + + expect(radio).to be_checked + expect(project.reload.project_setting.squash_option).to eq('never') + end + end end diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 5a88fc1cbf1359..4c93a8387a9ee6 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -303,4 +303,28 @@ end end end + + describe '#issuable_squash_option?' do + using RSpec::Parameterized::TableSyntax + + where(:issuable_persisted, :squash, :squash_enabled_by_default, :expectation) do + true | true | true | true + true | false | true | false + false | false | false | false + false | false | true | true + false | true | false | false + false | true | true | true + end + + with_them do + it 'returns the correct value' do + project = double( + squash_enabled_by_default?: squash_enabled_by_default + ) + issuable = double(persisted?: issuable_persisted, squash: squash) + + expect(helper.issuable_squash_option?(issuable, project)).to eq(expectation) + end + end + end end -- GitLab From ff13321c5d087c8ba6e2a360b068872bef423d12 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Fri, 3 Jul 2020 15:18:09 -0700 Subject: [PATCH 3/3] Address reviewer's comments - Remove changelog - Add aggregate_failures --- ..._request_squash_options_settings.html.haml | 2 +- .../17613-fe-squash-commits-options.yml | 5 --- locale/gitlab.pot | 6 ++-- ...er_manages_merge_requests_settings_spec.rb | 34 ++++++------------- 4 files changed, 14 insertions(+), 33 deletions(-) delete mode 100644 changelogs/unreleased/17613-fe-squash-commits-options.yml diff --git a/app/views/projects/_merge_request_squash_options_settings.html.haml b/app/views/projects/_merge_request_squash_options_settings.html.haml index af8f5b405c4bf4..a5dbfeb16d829d 100644 --- a/app/views/projects/_merge_request_squash_options_settings.html.haml +++ b/app/views/projects/_merge_request_squash_options_settings.html.haml @@ -4,7 +4,7 @@ .form-group %b= s_('ProjectSettings|Squash commits when merging') %p.text-secondary - = s_('ProjectSettings|ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests.') + = s_('ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests.') = link_to "What is squashing?", help_page_path('user/project/merge_requests/squash_and_merge.md'), target: '_blank' diff --git a/changelogs/unreleased/17613-fe-squash-commits-options.yml b/changelogs/unreleased/17613-fe-squash-commits-options.yml deleted file mode 100644 index 07e7ea0e79e9cd..00000000000000 --- a/changelogs/unreleased/17613-fe-squash-commits-options.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Configurable defaults for Squash commits option -merge_request: 33827 -author: -type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index be503d16c1a649..f3db09e5e6e02f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17910,9 +17910,6 @@ msgstr "" msgid "ProjectSettings|Project visibility" msgstr "" -msgid "ProjectSettings|ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests." -msgstr "" - msgid "ProjectSettings|Public" msgstr "" @@ -17922,6 +17919,9 @@ msgstr "" msgid "ProjectSettings|Require" msgstr "" +msgid "ProjectSettings|Set the default behavior and availability of this option in merge requests. Changes made are also applied to existing merge requests." +msgstr "" + msgid "ProjectSettings|Share code pastes with others out of Git repository" msgstr "" diff --git a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb index bc90393e889028..e97e4a2030a509 100644 --- a/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb +++ b/spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb @@ -28,33 +28,19 @@ end end - describe 'Squash commits while merging' do - it 'shows "Do not allow" option' do - page.within '#js-merge-request-settings' do - expect(page).to have_content 'Do not allow' - expect(page).to have_content 'Squashing is never performed and the checkbox is hidden.' - end - end + it 'shows Squash commit options', :aggregate_failures do + page.within '#js-merge-request-settings' do + expect(page).to have_content 'Do not allow' + expect(page).to have_content 'Squashing is never performed and the checkbox is hidden.' - it 'shows "Allow" option' do - page.within '#js-merge-request-settings' do - expect(page).to have_content 'Allow' - expect(page).to have_content 'Checkbox is visible and unselected by default.' - end - end + expect(page).to have_content 'Allow' + expect(page).to have_content 'Checkbox is visible and unselected by default.' - it 'shows "Encourage" option' do - page.within '#js-merge-request-settings' do - expect(page).to have_content 'Encourage' - expect(page).to have_content 'Checkbox is visible and selected by default.' - end - end + expect(page).to have_content 'Encourage' + expect(page).to have_content 'Checkbox is visible and selected by default.' - it 'shows "Require" option' do - page.within '#js-merge-request-settings' do - expect(page).to have_content 'Require' - expect(page).to have_content 'Squashing is always performed. Checkbox is visible and selected, and users cannot change it.' - end + expect(page).to have_content 'Require' + expect(page).to have_content 'Squashing is always performed. Checkbox is visible and selected, and users cannot change it.' end end -- GitLab