Merge branch '17613-configurable-defaults-for-squash-commits-option' into 'master'

Backend work for configurable project defaults for squashing merges

See merge request gitlab-org/gitlab!33099

app/controllers/projects_controller.rb

@@ -405,6 +405,7 @@ class ProjectsController < Projects::ApplicationController
       ],
       project_setting_attributes: %i[
         show_default_award_emojis
+        squash_option
       ]
     ]
   end

app/helpers/merge_requests_helper.rb

@@ -118,7 +118,7 @@ module MergeRequestsHelper
       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
 

app/models/merge_request.rb

@@ -1144,6 +1144,13 @@ class MergeRequest < ApplicationRecord
     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?
 

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

app/models/project_setting.rb

@@ -3,7 +3,22 @@
 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
 end
 
 ProjectSetting.prepend_if_ee('EE::ProjectSetting')

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

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

app/services/merge_requests/ff_merge_service.rb

@@ -16,7 +16,7 @@ module MergeRequests
                                      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
 

app/services/merge_requests/merge_base_service.rb

@@ -20,7 +20,7 @@ module MergeRequests
 
     def source
       strong_memoize(:source) do
-        if merge_request.squash
+        if merge_request.squash_on_merge?
           squash_sha!
         else
           merge_request.diff_head_sha

app/services/merge_requests/merge_service.rb

@@ -56,6 +56,8 @@ module MergeRequests
           '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

app/services/merge_requests/squash_service.rb

@@ -11,11 +11,14 @@ module MergeRequests
         return success(squash_sha: merge_request.diff_head_sha)
       end
 
+      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.'))
       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 @@ module MergeRequests
       raise SquashInProgressError, e.message
     end
 
+    def squash_forbidden?
+      target_project.squash_never?
+    end
+
     def repository
       target_project.repository
     end

changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml

+---
+title: Add squash commits options as a project setting.
+merge_request: 33099
+author:
+type: added

db/migrate/20200526193555_add_squash_option_to_project.rb

+# 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

db/structure.sql

@@ -14163,6 +14163,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))
 );
 
@@ -23417,6 +23418,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200526153844
 20200526164946
 20200526164947
+20200526193555
 20200526231421
 20200527092027
 20200527094322

locale/gitlab.pot

@@ -14099,6 +14099,9 @@ msgstr ""
 msgid "MergeRequests|Squash task canceled: another squash is already in progress."
 msgstr ""
 
+msgid "MergeRequests|This project does not allow squashing commits when merge requests are accepted."
+msgstr ""
+
 msgid "MergeRequests|Thread stays resolved"
 msgstr ""
 

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -442,7 +442,7 @@ RSpec.describe Projects::MergeRequestsController do
           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 @@ RSpec.describe Projects::MergeRequestsController do
           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
 

spec/requests/api/merge_requests_spec.rb

@@ -1930,7 +1930,7 @@ RSpec.describe API::MergeRequests do
     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

spec/serializers/merge_request_poll_cached_widget_entity_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 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 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do
     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

spec/serializers/merge_request_poll_widget_entity_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 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 @@ RSpec.describe MergeRequestPollWidgetEntity do
       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,

spec/services/auto_merge/base_service_spec.rb

@@ -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['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

spec/services/merge_requests/merge_service_spec.rb

@@ -360,6 +360,25 @@ RSpec.describe MergeRequests::MergeService do
         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')

spec/services/merge_requests/squash_service_spec.rb

@@ -131,6 +131,42 @@ RSpec.describe MergeRequests::SquashService do
       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_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
       let(:merge_request) { merge_request_with_large_files }