From e65922c43e78b52804c6d553fa6ab5352b5042b8 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Tue, 16 Jun 2020 19:24:03 +0000 Subject: [PATCH 1/4] Adds a backend component to for the "Squash Commits" feature In particular this: * Adds an enum to the project_settings model with squash defaults * Adds logic to reject merges when they don't meet the criteria * Adds specs for the above * Adds some endpoints for the frontend work * Adds specs for the above Thanks to @enzuru for the awesome groundwork on this! A lot of the code is taken from his initial MR: https://gitlab.com/gitlab-org/git lab/-/merge_requests/27833 --- app/controllers/projects_controller.rb | 1 + app/helpers/merge_requests_helper.rb | 2 +- app/models/merge_request.rb | 7 ++++ app/models/project.rb | 1 + app/models/project_setting.rb | 19 ++++++++++ ...merge_request_poll_cached_widget_entity.rb | 12 +++++++ .../merge_request_poll_widget_entity.rb | 12 +++++++ .../merge_requests/ff_merge_service.rb | 2 +- .../merge_requests/merge_base_service.rb | 2 +- app/services/merge_requests/merge_service.rb | 2 ++ app/services/merge_requests/squash_service.rb | 7 ++++ ...ble-defaults-for-squash-commits-option.yml | 5 +++ ...0526193555_add_squash_option_to_project.rb | 9 +++++ db/structure.sql | 2 ++ locale/gitlab.pot | 3 ++ .../merge_requests_controller_spec.rb | 4 +-- spec/requests/api/merge_requests_spec.rb | 2 +- ..._request_poll_cached_widget_entity_spec.rb | 22 ++++++++++++ .../merge_request_poll_widget_entity_spec.rb | 22 ++++++++++++ spec/services/auto_merge/base_service_spec.rb | 2 +- .../merge_requests/merge_service_spec.rb | 19 ++++++++++ .../merge_requests/squash_service_spec.rb | 36 +++++++++++++++++++ 22 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml create mode 100644 db/migrate/20200526193555_add_squash_option_to_project.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 36ca88da1c7de3..00c01c86ae2bf9 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -405,6 +405,7 @@ def project_params_attributes ], project_setting_attributes: %i[ show_default_award_emojis + squash_option ] ] end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 7940ec1162bf25..caf39741543e53 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -118,7 +118,7 @@ def merge_params(merge_request) auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, should_remove_source_branch: true, sha: merge_request.diff_head_sha, - squash: merge_request.squash + squash: merge_request.squash_on_merge? } end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 699f82c7e6be71..662986cea4e0bd 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1142,6 +1142,13 @@ def default_squash_commit_message end end + def squash_on_merge? + return true if target_project.squash_always? + return false if target_project.squash_never? + + squash? + end + def has_ci? return false if has_no_commits? diff --git a/app/models/project.rb b/app/models/project.rb index d464c39a43ece5..9b1e453216b4be 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -363,6 +363,7 @@ class Project < ApplicationRecord to: :project_setting, allow_nil: true delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?, prefix: :import, to: :import_state, allow_nil: true + delegate :squash_always?, :squash_never?, :squash_enabled_by_default?, :squash_readonly?, to: :project_setting delegate :no_import?, to: :import_state, allow_nil: true delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 9022d3e879d0c4..6be8c90625709b 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -3,7 +3,26 @@ class ProjectSetting < ApplicationRecord belongs_to :project, inverse_of: :project_setting + enum squash_option: { + never: 0, + always: 1, + default_on: 2, + default_off: 3 + }, _prefix: 'squash' + self.primary_key = :project_id + + def squash_enabled_by_default? + %w[always default_on].include?(squash_option) + end + + def squash_readonly? + %w[always never].include?(squash_option) + end + + def self.where_or_create_by(attrs) + where(primary_key => safe_find_or_create_by(attrs)) + end end ProjectSetting.prepend_if_ee('EE::ProjectSetting') diff --git a/app/serializers/merge_request_poll_cached_widget_entity.rb b/app/serializers/merge_request_poll_cached_widget_entity.rb index 72f629b350753e..23266bfed7ab04 100644 --- a/app/serializers/merge_request_poll_cached_widget_entity.rb +++ b/app/serializers/merge_request_poll_cached_widget_entity.rb @@ -74,6 +74,18 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity diffs_project_merge_request_path(merge_request.project, merge_request) end + expose :squash_enabled_by_default do |merge_request| + presenter(merge_request).project.squash_enabled_by_default? + end + + expose :squash_readonly do |merge_request| + presenter(merge_request).project.squash_readonly? + end + + expose :squash_on_merge do |merge_request| + presenter(merge_request).squash_on_merge? + end + private delegate :current_user, to: :request diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb index aad607f358a81b..b0d311681c02d0 100644 --- a/app/serializers/merge_request_poll_widget_entity.rb +++ b/app/serializers/merge_request_poll_widget_entity.rb @@ -145,6 +145,18 @@ class MergeRequestPollWidgetEntity < Grape::Entity presenter(merge_request).revert_in_fork_path end + expose :squash_enabled_by_default do |merge_request| + presenter(merge_request).project.squash_enabled_by_default? + end + + expose :squash_readonly do |merge_request| + presenter(merge_request).project.squash_readonly? + end + + expose :squash_on_merge do |merge_request| + presenter(merge_request).squash_on_merge? + end + private delegate :current_user, to: :request diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb index 6f1fa607ef9ed0..b3896d61a78913 100644 --- a/app/services/merge_requests/ff_merge_service.rb +++ b/app/services/merge_requests/ff_merge_service.rb @@ -16,7 +16,7 @@ def commit merge_request.target_branch, merge_request: merge_request) - if merge_request.squash + if merge_request.squash_on_merge? merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha) end diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index 27b5e31faab1be..fe09c92aab925a 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -20,7 +20,7 @@ def hooks_validation_error(_merge_request) def source strong_memoize(:source) do - if merge_request.squash + if merge_request.squash_on_merge? squash_sha! else merge_request.diff_head_sha diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 8d57a76f7d027a..48c1ef3cefe9ab 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -56,6 +56,8 @@ def error_check! 'Only fast-forward merge is allowed for your project. Please update your source branch' elsif !@merge_request.mergeable? 'Merge request is not mergeable' + elsif !@merge_request.squash && project.squash_always? + 'This project requires squashing commits when merge requests are accepted.' end raise_error(error) if error diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb index 4b04d42b48ec1a..f1e3f1557b33f1 100644 --- a/app/services/merge_requests/squash_service.rb +++ b/app/services/merge_requests/squash_service.rb @@ -11,11 +11,14 @@ def execute return success(squash_sha: merge_request.diff_head_sha) end + return error(s_('MergeRequests|This repository prohibits squashing for merge requests.')) if squash_forbidden? + if squash_in_progress? return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.')) end squash! || error(s_('MergeRequests|Failed to squash. Should be done manually.')) + rescue SquashInProgressError error(s_('MergeRequests|An error occurred while checking whether another squash is in progress.')) end @@ -40,6 +43,10 @@ def squash_in_progress? raise SquashInProgressError, e.message end + def squash_forbidden? + target_project.squash_never? + end + def repository target_project.repository end diff --git a/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml b/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml new file mode 100644 index 00000000000000..bb7749213615fc --- /dev/null +++ b/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml @@ -0,0 +1,5 @@ +--- +title: Add squash commits options as a project setting. +merge_request: 33099 +author: +type: added diff --git a/db/migrate/20200526193555_add_squash_option_to_project.rb b/db/migrate/20200526193555_add_squash_option_to_project.rb new file mode 100644 index 00000000000000..6dd4e1c7c8e1fa --- /dev/null +++ b/db/migrate/20200526193555_add_squash_option_to_project.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddSquashOptionToProject < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :project_settings, :squash_option, :integer, default: 3, limit: 2 + end +end diff --git a/db/structure.sql b/db/structure.sql index 4ffd1e379ac55e..0918541fe4aed3 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14140,6 +14140,7 @@ CREATE TABLE public.project_settings ( push_rule_id bigint, show_default_award_emojis boolean DEFAULT true, allow_merge_on_skipped_pipeline boolean, + squash_option smallint DEFAULT 3, CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL)) ); @@ -23380,6 +23381,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200526153844 20200526164946 20200526164947 +20200526193555 20200526231421 20200527092027 20200527094322 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d9e4037ff16bc3..1440d9d768add6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14084,6 +14084,9 @@ msgstr "" msgid "MergeRequests|Squash task canceled: another squash is already in progress." msgstr "" +msgid "MergeRequests|This repository prohibits squashing for merge requests." +msgstr "" + msgid "MergeRequests|Thread stays resolved" msgstr "" diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 382593fd7cb046..4327e0bbb7a885 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -442,7 +442,7 @@ def merge_with_sha(params = {}) merge_request.update(squash: false) merge_with_sha(squash: '1') - expect(merge_request.reload.squash).to be_truthy + expect(merge_request.reload.squash_on_merge?).to be_truthy end end @@ -451,7 +451,7 @@ def merge_with_sha(params = {}) merge_request.update(squash: true) merge_with_sha(squash: '0') - expect(merge_request.reload.squash).to be_falsey + expect(merge_request.reload.squash_on_merge?).to be_falsey end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 803178d590f529..d71cd843a4749b 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1930,7 +1930,7 @@ it "updates the MR's squash attribute" do expect do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { squash: true } - end.to change { merge_request.reload.squash } + end.to change { merge_request.reload.squash_on_merge? } expect(response).to have_gitlab_http_status(:ok) end diff --git a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb index 1357f7cf624276..51564de6041e8d 100644 --- a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do include ProjectForksHelper + using RSpec::Parameterized::TableSyntax let(:project) { create :project, :repository } let(:resource) { create(:merge_request, source_project: project, target_project: project) } @@ -181,6 +182,27 @@ def find_matching_commit(short_id) end end + describe 'squash defaults for projects' do + where(:squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: squash_option) + end + + it 'the key reflects the correct value' do + expect(subject[:squash_on_merge]).to eq(value) + expect(subject[:squash_enabled_by_default]).to eq(default) + expect(subject[:squash_readonly]).to eq(readonly) + end + end + end + describe 'attributes for squash commit message' do context 'when merge request is mergeable' do before do diff --git a/spec/serializers/merge_request_poll_widget_entity_spec.rb b/spec/serializers/merge_request_poll_widget_entity_spec.rb index 2b6159e883d462..f0493699209b01 100644 --- a/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_widget_entity_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequestPollWidgetEntity do include ProjectForksHelper + using RSpec::Parameterized::TableSyntax let(:project) { create :project, :repository } let(:resource) { create(:merge_request, source_project: project, target_project: project) } @@ -171,6 +172,27 @@ end end + describe 'squash defaults for projects' do + where(:squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: squash_option) + end + + it 'the key reflects the correct value' do + expect(subject[:squash_on_merge]).to eq(value) + expect(subject[:squash_enabled_by_default]).to eq(default) + expect(subject[:squash_readonly]).to eq(readonly) + end + end + end + context 'when head pipeline is finished' do before do create(:ci_pipeline, :success, project: project, diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 9673a65344d2e1..98fa601208957f 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -51,7 +51,7 @@ expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) - expect(merge_request.squash).to eq(false) + expect(merge_request.squash_on_merge?).to eq(false) expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 94ecd1830c2d6d..11e341994f76db 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -360,6 +360,25 @@ expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end + context 'when squashing is required' do + before do + merge_request.update!(source_branch: 'master', target_branch: 'feature') + merge_request.target_project.project_setting.squash_always! + end + + it 'raises an error if squashing is not done' do + error_message = 'requires squashing commits' + + service.execute(merge_request) + + expect(merge_request).to be_open + + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.merge_error).to include(error_message) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + end + context 'when squashing' do before do merge_request.update!(source_branch: 'master', target_branch: 'feature') diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index b51b7d9fbc6c43..9eec964d6619ae 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -131,6 +131,42 @@ include_examples 'the squash succeeds' end + context 'when squashing is disabled by default on the project' do + # Squashing is disabled by default, but it should still allow you + # to squash-and-merge if selected through the UI + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_option.squash_default_off! + end + + include_examples 'the squash succeeds' + end + + context 'when squashing is forbidden on the project' do + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_option.squash_never! + end + + it 'raises a squash error' do + expect(service.execute).to match( + status: :error, + message: a_string_including('prohibits squashing for merge requests')) + end + end + + context 'when squashing is enabled by default on the project' do + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_option.squash_always! + end + + include_examples 'the squash succeeds' + end + context 'when squashing with files too large to display' do let(:merge_request) { merge_request_with_large_files } -- GitLab From e51b5eb225c9010c57266c86ed88ee2f3dc70e79 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Mon, 22 Jun 2020 20:13:09 -0500 Subject: [PATCH 2/4] Fixing some failing specs in SquashService --- spec/services/merge_requests/squash_service_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 9eec964d6619ae..b17b27b572aba9 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -137,7 +137,7 @@ let(:merge_request) { merge_request_with_only_new_files } before do - merge_request.project.project_setting.squash_option.squash_default_off! + merge_request.project.project_setting.squash_default_off! end include_examples 'the squash succeeds' @@ -147,7 +147,7 @@ let(:merge_request) { merge_request_with_only_new_files } before do - merge_request.project.project_setting.squash_option.squash_never! + merge_request.project.project_setting.squash_never! end it 'raises a squash error' do @@ -161,7 +161,7 @@ let(:merge_request) { merge_request_with_only_new_files } before do - merge_request.project.project_setting.squash_option.squash_always! + merge_request.project.project_setting.squash_always! end include_examples 'the squash succeeds' -- GitLab From a8a319734e40f3cf49b7a30cd1c8070299606e34 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Tue, 23 Jun 2020 15:02:55 -0500 Subject: [PATCH 3/4] Removing an extra function that was leftover from a merge --- app/models/project_setting.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 6be8c90625709b..aca7eec3382dbd 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -19,10 +19,6 @@ def squash_enabled_by_default? def squash_readonly? %w[always never].include?(squash_option) end - - def self.where_or_create_by(attrs) - where(primary_key => safe_find_or_create_by(attrs)) - end end ProjectSetting.prepend_if_ee('EE::ProjectSetting') -- GitLab From 652d69441298bab25460d454aba83f649f51c125 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 25 Jun 2020 10:13:07 -0500 Subject: [PATCH 4/4] Updates some error message text --- app/services/merge_requests/squash_service.rb | 2 +- locale/gitlab.pot | 2 +- spec/services/merge_requests/squash_service_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb index f1e3f1557b33f1..faa2e921581261 100644 --- a/app/services/merge_requests/squash_service.rb +++ b/app/services/merge_requests/squash_service.rb @@ -11,7 +11,7 @@ def execute return success(squash_sha: merge_request.diff_head_sha) end - return error(s_('MergeRequests|This repository prohibits squashing for merge requests.')) if squash_forbidden? + return error(s_('MergeRequests|This project does not allow squashing commits when merge requests are accepted.')) if squash_forbidden? if squash_in_progress? return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.')) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1440d9d768add6..97d8acee29336a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14084,7 +14084,7 @@ msgstr "" msgid "MergeRequests|Squash task canceled: another squash is already in progress." msgstr "" -msgid "MergeRequests|This repository prohibits squashing for merge requests." +msgid "MergeRequests|This project does not allow squashing commits when merge requests are accepted." msgstr "" msgid "MergeRequests|Thread stays resolved" diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index b17b27b572aba9..1ec1dc0f6eb434 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -153,7 +153,7 @@ it 'raises a squash error' do expect(service.execute).to match( status: :error, - message: a_string_including('prohibits squashing for merge requests')) + message: a_string_including('does not allow squashing commits when merge requests are accepted')) end end -- GitLab