Commit 591c741b authored by Gary Holtz's avatar Gary Holtz Committed by Rémy Coutable

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
parent a30e2b49
...@@ -405,6 +405,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -405,6 +405,7 @@ class ProjectsController < Projects::ApplicationController
], ],
project_setting_attributes: %i[ project_setting_attributes: %i[
show_default_award_emojis show_default_award_emojis
squash_option
] ]
] ]
end end
......
...@@ -118,7 +118,7 @@ module MergeRequestsHelper ...@@ -118,7 +118,7 @@ module MergeRequestsHelper
auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS,
should_remove_source_branch: true, should_remove_source_branch: true,
sha: merge_request.diff_head_sha, sha: merge_request.diff_head_sha,
squash: merge_request.squash squash: merge_request.squash_on_merge?
} }
end end
......
...@@ -1144,6 +1144,13 @@ class MergeRequest < ApplicationRecord ...@@ -1144,6 +1144,13 @@ class MergeRequest < ApplicationRecord
end end
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? def has_ci?
return false if has_no_commits? return false if has_no_commits?
......
...@@ -363,6 +363,7 @@ class Project < ApplicationRecord ...@@ -363,6 +363,7 @@ class Project < ApplicationRecord
to: :project_setting, allow_nil: true to: :project_setting, allow_nil: true
delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?, delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?,
prefix: :import, to: :import_state, allow_nil: true 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 :no_import?, to: :import_state, allow_nil: true
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
delegate :members, to: :team, prefix: true delegate :members, to: :team, prefix: true
......
...@@ -3,7 +3,22 @@ ...@@ -3,7 +3,22 @@
class ProjectSetting < ApplicationRecord class ProjectSetting < ApplicationRecord
belongs_to :project, inverse_of: :project_setting 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 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
end end
ProjectSetting.prepend_if_ee('EE::ProjectSetting') ProjectSetting.prepend_if_ee('EE::ProjectSetting')
...@@ -74,6 +74,18 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity ...@@ -74,6 +74,18 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
diffs_project_merge_request_path(merge_request.project, merge_request) diffs_project_merge_request_path(merge_request.project, merge_request)
end 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 private
delegate :current_user, to: :request delegate :current_user, to: :request
......
...@@ -145,6 +145,18 @@ class MergeRequestPollWidgetEntity < Grape::Entity ...@@ -145,6 +145,18 @@ class MergeRequestPollWidgetEntity < Grape::Entity
presenter(merge_request).revert_in_fork_path presenter(merge_request).revert_in_fork_path
end 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 private
delegate :current_user, to: :request delegate :current_user, to: :request
......
...@@ -16,7 +16,7 @@ module MergeRequests ...@@ -16,7 +16,7 @@ module MergeRequests
merge_request.target_branch, merge_request.target_branch,
merge_request: merge_request) 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) merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha)
end end
......
...@@ -20,7 +20,7 @@ module MergeRequests ...@@ -20,7 +20,7 @@ module MergeRequests
def source def source
strong_memoize(:source) do strong_memoize(:source) do
if merge_request.squash if merge_request.squash_on_merge?
squash_sha! squash_sha!
else else
merge_request.diff_head_sha merge_request.diff_head_sha
......
...@@ -56,6 +56,8 @@ module MergeRequests ...@@ -56,6 +56,8 @@ module MergeRequests
'Only fast-forward merge is allowed for your project. Please update your source branch' 'Only fast-forward merge is allowed for your project. Please update your source branch'
elsif !@merge_request.mergeable? elsif !@merge_request.mergeable?
'Merge request is not mergeable' 'Merge request is not mergeable'
elsif !@merge_request.squash && project.squash_always?
'This project requires squashing commits when merge requests are accepted.'
end end
raise_error(error) if error raise_error(error) if error
......
...@@ -11,11 +11,14 @@ module MergeRequests ...@@ -11,11 +11,14 @@ module MergeRequests
return success(squash_sha: merge_request.diff_head_sha) return success(squash_sha: merge_request.diff_head_sha)
end end
return error(s_('MergeRequests|This project does not allow squashing commits when merge requests are accepted.')) if squash_forbidden?
if squash_in_progress? if squash_in_progress?
return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.')) return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.'))
end end
squash! || error(s_('MergeRequests|Failed to squash. Should be done manually.')) squash! || error(s_('MergeRequests|Failed to squash. Should be done manually.'))
rescue SquashInProgressError rescue SquashInProgressError
error(s_('MergeRequests|An error occurred while checking whether another squash is in progress.')) error(s_('MergeRequests|An error occurred while checking whether another squash is in progress.'))
end end
...@@ -40,6 +43,10 @@ module MergeRequests ...@@ -40,6 +43,10 @@ module MergeRequests
raise SquashInProgressError, e.message raise SquashInProgressError, e.message
end end
def squash_forbidden?
target_project.squash_never?
end
def repository def repository
target_project.repository target_project.repository
end end
......
---
title: Add squash commits options as a project setting.
merge_request: 33099
author:
type: added
# 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
...@@ -14163,6 +14163,7 @@ CREATE TABLE public.project_settings ( ...@@ -14163,6 +14163,7 @@ CREATE TABLE public.project_settings (
push_rule_id bigint, push_rule_id bigint,
show_default_award_emojis boolean DEFAULT true, show_default_award_emojis boolean DEFAULT true,
allow_merge_on_skipped_pipeline boolean, allow_merge_on_skipped_pipeline boolean,
squash_option smallint DEFAULT 3,
CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL)) CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL))
); );
...@@ -23417,6 +23418,7 @@ COPY "schema_migrations" (version) FROM STDIN; ...@@ -23417,6 +23418,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200526153844 20200526153844
20200526164946 20200526164946
20200526164947 20200526164947
20200526193555
20200526231421 20200526231421
20200527092027 20200527092027
20200527094322 20200527094322
......
...@@ -14096,6 +14096,9 @@ msgstr "" ...@@ -14096,6 +14096,9 @@ msgstr ""
msgid "MergeRequests|Squash task canceled: another squash is already in progress." msgid "MergeRequests|Squash task canceled: another squash is already in progress."
msgstr "" msgstr ""
msgid "MergeRequests|This project does not allow squashing commits when merge requests are accepted."
msgstr ""
msgid "MergeRequests|Thread stays resolved" msgid "MergeRequests|Thread stays resolved"
msgstr "" msgstr ""
......
...@@ -442,7 +442,7 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -442,7 +442,7 @@ RSpec.describe Projects::MergeRequestsController do
merge_request.update(squash: false) merge_request.update(squash: false)
merge_with_sha(squash: '1') 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
end end
...@@ -451,7 +451,7 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -451,7 +451,7 @@ RSpec.describe Projects::MergeRequestsController do
merge_request.update(squash: true) merge_request.update(squash: true)
merge_with_sha(squash: '0') 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
end end
......
...@@ -1930,7 +1930,7 @@ RSpec.describe API::MergeRequests do ...@@ -1930,7 +1930,7 @@ RSpec.describe API::MergeRequests do
it "updates the MR's squash attribute" do it "updates the MR's squash attribute" do
expect do expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { squash: true } 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) expect(response).to have_gitlab_http_status(:ok)
end end
......
...@@ -4,6 +4,7 @@ require 'spec_helper' ...@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe MergeRequestPollCachedWidgetEntity do RSpec.describe MergeRequestPollCachedWidgetEntity do
include ProjectForksHelper include ProjectForksHelper
using RSpec::Parameterized::TableSyntax
let(:project) { create :project, :repository } let(:project) { create :project, :repository }
let(:resource) { create(:merge_request, source_project: project, target_project: project) } let(:resource) { create(:merge_request, source_project: project, target_project: project) }
...@@ -181,6 +182,27 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do ...@@ -181,6 +182,27 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do
end end
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 describe 'attributes for squash commit message' do
context 'when merge request is mergeable' do context 'when merge request is mergeable' do
before do before do
......
...@@ -4,6 +4,7 @@ require 'spec_helper' ...@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe MergeRequestPollWidgetEntity do RSpec.describe MergeRequestPollWidgetEntity do
include ProjectForksHelper include ProjectForksHelper
using RSpec::Parameterized::TableSyntax
let(:project) { create :project, :repository } let(:project) { create :project, :repository }
let(:resource) { create(:merge_request, source_project: project, target_project: project) } let(:resource) { create(:merge_request, source_project: project, target_project: project) }
...@@ -171,6 +172,27 @@ RSpec.describe MergeRequestPollWidgetEntity do ...@@ -171,6 +172,27 @@ RSpec.describe MergeRequestPollWidgetEntity do
end end
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 context 'when head pipeline is finished' do
before do before do
create(:ci_pipeline, :success, project: project, create(:ci_pipeline, :success, project: project,
......
...@@ -51,7 +51,7 @@ RSpec.describe AutoMerge::BaseService do ...@@ -51,7 +51,7 @@ RSpec.describe AutoMerge::BaseService do
expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") 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['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18')
expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) 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') expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md')
end end
end end
......
...@@ -360,6 +360,25 @@ RSpec.describe MergeRequests::MergeService do ...@@ -360,6 +360,25 @@ RSpec.describe MergeRequests::MergeService do
expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message))
end 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 context 'when squashing' do
before do before do
merge_request.update!(source_branch: 'master', target_branch: 'feature') merge_request.update!(source_branch: 'master', target_branch: 'feature')
......
...@@ -131,6 +131,42 @@ RSpec.describe MergeRequests::SquashService do ...@@ -131,6 +131,42 @@ RSpec.describe MergeRequests::SquashService do
include_examples 'the squash succeeds' include_examples 'the squash succeeds'
end 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_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_never!
end
it 'raises a squash error' do
expect(service.execute).to match(
status: :error,
message: a_string_including('does not allow squashing commits when merge requests are accepted'))
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_always!
end
include_examples 'the squash succeeds'
end
context 'when squashing with files too large to display' do context 'when squashing with files too large to display' do
let(:merge_request) { merge_request_with_large_files } let(:merge_request) { merge_request_with_large_files }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment