Merge branch '2570-namespace-rebase-before-merge' into 'master'

Introduce namespace license checks for rebase before merge

Closes #2570

See merge request !2200

app/controllers/ee/projects/merge_requests_controller.rb

+module EE
+  module Projects
+    module MergeRequestsController
+      extend ActiveSupport::Concern
+
+      prepended do
+        # This module is prepended to `Projects::MergeRequestController`, which
+        # already calls `before_action :merge_request, only: [...]`. Calling it
+        # again here would *replace* the restriction, rather than extending it.
+        before_action(only: [:approve, :approvals, :unapprove, :rebase]) { merge_request }
+
+        before_action :set_suggested_approvers, only: [:new, :new_diffs, :edit]
+        before_action :check_merge_request_rebase_available!, only: [:rebase]
+      end
+
+      def rebase
+        return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+        return render_404 unless @merge_request.approved?
+
+        RebaseWorker.perform_async(@merge_request.id, current_user.id)
+
+        render nothing: true, status: 200
+      end
+
+      def approve
+        unless @merge_request.can_approve?(current_user)
+          return render_404
+        end
+
+        ::MergeRequests::ApprovalService
+          .new(project, current_user)
+          .execute(@merge_request)
+
+        render_approvals_json
+      end
+
+      def approvals
+        render_approvals_json
+      end
+
+      def unapprove
+        if @merge_request.has_approved?(current_user)
+          ::MergeRequests::RemoveApprovalService
+            .new(project, current_user)
+            .execute(@merge_request)
+        end
+
+        render_approvals_json
+      end
+
+      protected
+
+      def render_approvals_json
+        respond_to do |format|
+          format.json do
+            entity = ::API::Entities::MergeRequestApprovals.new(@merge_request, current_user: current_user)
+            render json: entity
+          end
+        end
+      end
+
+      def set_suggested_approvers
+        if @merge_request.requires_approve?
+          @suggested_approvers = ::Gitlab::AuthorityAnalyzer.new(
+            @merge_request,
+            @merge_request.author || current_user
+          ).calculate(@merge_request.approvals_required)
+        end
+      end
+
+      def merge_params_attributes
+        super + [:squash]
+      end
+
+      def merge_request_params
+        clamp_approvals_before_merge(super)
+      end
+
+      def merge_request_params_attributes
+        super + %i[
+          approvals_before_merge
+          approver_group_ids
+          approver_ids
+          squash
+        ]
+      end
+
+      # If the number of approvals is not greater than the project default, set to
+      # nil, so that we fall back to the project default. If it's not set, we can
+      # let the normal update logic handle this.
+      def clamp_approvals_before_merge(mr_params)
+        return mr_params unless mr_params[:approvals_before_merge]
+
+        target_project = @project.forked_from_project if @project.id.to_s != mr_params[:target_project_id]
+        target_project ||= @project
+
+        if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
+          mr_params[:approvals_before_merge] = nil
+        end
+
+        mr_params
+      end
+    end
+  end
+end

app/controllers/projects/merge_requests_controller.rb

@@ -10,10 +10,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :check_merge_requests_available!
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :conflicts, :conflict_for_path, :pipelines, :merge,
-    :pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds,
-    :remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content,
-    # EE
-    :approve, :approvals, :unapprove, :rebase
+    :pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds, :remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content
   ]
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :pipelines]
   before_action :define_show_vars, only: [:diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
@@ -22,7 +19,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :check_if_can_be_merged, only: :show
   before_action :apply_diff_view_cookie!, only: [:new_diffs]
   before_action :build_merge_request, only: [:new, :new_diffs]
-  before_action :set_suggested_approvers, only: [:new, :new_diffs, :edit]
 
   # Allow read any merge_request
   before_action :authorize_read_merge_request!
@@ -37,6 +33,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
 
+  # This module must be prepended *after* all before_action filters
+  prepend ::EE::Projects::MergeRequestsController
+
   def index
     @collection_type    = "MergeRequest"
     @merge_requests     = merge_requests_collection
@@ -269,9 +268,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def create
     @target_branches ||= []
-    create_params = clamp_approvals_before_merge(merge_request_params)
-
-    @merge_request = MergeRequests::CreateService.new(project, current_user, create_params).execute
+    @merge_request = MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
 
     if @merge_request.valid?
       redirect_to(merge_request_path(@merge_request))
@@ -291,9 +288,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def update
-    update_params = clamp_approvals_before_merge(merge_request_params)
-
-    @merge_request = MergeRequests::UpdateService.new(project, current_user, update_params).execute(@merge_request)
+    @merge_request = MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
 
     respond_to do |format|
       format.html do
@@ -340,15 +335,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: serializer.represent(@merge_request)
   end
 
-  def rebase
-    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-    return render_404 unless @merge_request.approved?
-
-    RebaseWorker.perform_async(@merge_request.id, current_user.id)
-
-    render nothing: true, status: 200
-  end
-
   def merge
     return access_denied! unless @merge_request.can_be_merged_by?(current_user)
     return render_404 unless @merge_request.approved?
@@ -432,9 +418,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
           metrics_url =
             if can?(current_user, :read_environment, environment) && environment.has_metrics?
               metrics_namespace_project_environment_deployment_path(environment.project.namespace,
-                                                         environment.project,
-                                                         environment,
-                                                         deployment)
+                                                                    environment.project,
+                                                                    environment,
+                                                                    deployment)
             end
 
           {
@@ -454,43 +440,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: environments
   end
 
-  def approve
-    unless @merge_request.can_approve?(current_user)
-      return render_404
-    end
-
-    ::MergeRequests::ApprovalService
-      .new(project, current_user)
-      .execute(@merge_request)
-
-    render_approvals_json
-  end
-
-  def approvals
-    render_approvals_json
-  end
-
-  def unapprove
-    if @merge_request.has_approved?(current_user)
-      ::MergeRequests::RemoveApprovalService
-        .new(project, current_user)
-        .execute(@merge_request)
-    end
-
-    render_approvals_json
-  end
-
   protected
 
-  def render_approvals_json
-    respond_to do |format|
-      format.json do
-        entity = API::Entities::MergeRequestApprovals.new(@merge_request, current_user: current_user)
-        render json: entity
-      end
-    end
-  end
-
   def selected_target_project
     if @project.id.to_s == params[:target_project_id] || @project.forked_project_link.nil?
       @project
@@ -632,15 +583,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render 'invalid'
   end
 
-  def set_suggested_approvers
-    if @merge_request.requires_approve?
-      @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request,
-        @merge_request.author || current_user
-      ).calculate(@merge_request.approvals_required)
-    end
-  end
-
   def merge_request_params
     params.require(:merge_request)
       .permit(merge_request_params_attributes)
@@ -662,36 +604,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       :title,
 
       label_ids: []
-    ] + merge_request_params_ee
-  end
-
-  def merge_request_params_ee
-    %i[
-      approvals_before_merge
-      approver_group_ids
-      approver_ids
-      squash
     ]
   end
 
-  # If the number of approvals is not greater than the project default, set to
-  # nil, so that we fall back to the project default. If it's not set, we can
-  # let the normal update logic handle this.
-  def clamp_approvals_before_merge(mr_params)
-    return mr_params unless mr_params[:approvals_before_merge]
-
-    target_project = @project.forked_from_project if @project.id.to_s != mr_params[:target_project_id]
-    target_project ||= @project
-
-    if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
-      mr_params[:approvals_before_merge] = nil
-    end
-
-    mr_params
+  def merge_params
+    params.permit(merge_params_attributes)
   end
 
-  def merge_params
-    params.permit(:should_remove_source_branch, :commit_message, :squash)
+  def merge_params_attributes
+    [:should_remove_source_branch, :commit_message]
   end
 
   # Make sure merge requests created before 8.0

app/models/ee/merge_request.rb

+module EE
+  module MergeRequest
+    def ff_merge_possible?
+      project.repository.is_ancestor?(target_branch_sha, diff_head_sha)
+    end
+
+    def should_be_rebased?
+      project.feature_available?(:merge_request_rebase) &&
+        project.ff_merge_must_be_possible? &&
+        !ff_merge_possible?
+    end
+
+    def rebase_dir_path
+      File.join(::Gitlab.config.shared.path, 'tmp/rebase', source_project.id.to_s, id.to_s).to_s
+    end
+
+    def squash_dir_path
+      File.join(::Gitlab.config.shared.path, 'tmp/squash', source_project.id.to_s, id.to_s).to_s
+    end
+
+    def rebase_in_progress?
+      # The source project can be deleted
+      return false unless source_project
+
+      File.exist?(rebase_dir_path) && !clean_stuck_rebase
+    end
+
+    def squash_in_progress?
+      # The source project can be deleted
+      return false unless source_project
+
+      File.exist?(squash_dir_path) && !clean_stuck_squash
+    end
+
+    def clean_stuck_rebase
+      if File.mtime(rebase_dir_path) < 15.minutes.ago
+        FileUtils.rm_rf(rebase_dir_path)
+        true
+      end
+    end
+
+    def clean_stuck_squash
+      if File.mtime(squash_dir_path) < 15.minutes.ago
+        FileUtils.rm_rf(squash_dir_path)
+        true
+      end
+    end
+  end
+end

app/models/license.rb

@@ -10,6 +10,7 @@ class License < ActiveRecord::Base
   ELASTIC_SEARCH_FEATURE = 'GitLab_ElasticSearch'.freeze
   RELATED_ISSUES_FEATURE = 'RelatedIssues'.freeze
   EXPORT_ISSUES_FEATURE  = 'GitLab_ExportIssues'.freeze
+  MERGE_REQUEST_REBASE_FEATURE = 'GitLab_MergeRequestRebase'.freeze
 
   FEATURE_CODES = {
     geo: GEO_FEATURE,
@@ -21,7 +22,8 @@ class License < ActiveRecord::Base
     # Features that make sense to Namespace:
     deploy_board: DEPLOY_BOARD_FEATURE,
     file_lock: FILE_LOCK_FEATURE,
-    export_issues: EXPORT_ISSUES_FEATURE
+    export_issues: EXPORT_ISSUES_FEATURE,
+    merge_request_rebase: MERGE_REQUEST_REBASE_FEATURE
   }.freeze
 
   STARTER_PLAN = 'starter'.freeze
@@ -32,7 +34,8 @@ class License < ActiveRecord::Base
   EES_FEATURES = [
     { ELASTIC_SEARCH_FEATURE => 1 },
     { RELATED_ISSUES_FEATURE => 1 },
-    { EXPORT_ISSUES_FEATURE => 1 }
+    { EXPORT_ISSUES_FEATURE => 1 },
+    { MERGE_REQUEST_REBASE_FEATURE => 1 }
   ].freeze
 
   EEP_FEATURES = [
@@ -65,7 +68,8 @@ class License < ActiveRecord::Base
     { AUDITOR_USER_FEATURE => 1 },
     { SERVICE_DESK_FEATURE => 1 },
     { OBJECT_STORAGE_FEATURE => 1 },
-    { EXPORT_ISSUES_FEATURE => 1 }
+    { EXPORT_ISSUES_FEATURE => 1 },
+    { MERGE_REQUEST_REBASE_FEATURE => 1 }
   ].freeze
 
   FEATURES_BY_PLAN = {

app/models/merge_request.rb

@@ -10,6 +10,8 @@ class MergeRequest < ActiveRecord::Base
 
   ignore_column :position
 
+  include ::EE::MergeRequest
+
   belongs_to :target_project, class_name: "Project"
   belongs_to :source_project, class_name: "Project"
   belongs_to :merge_user, class_name: "User"
@@ -820,50 +822,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def ff_merge_possible?
-    project.repository.is_ancestor?(target_branch_sha, diff_head_sha)
-  end
-
-  def should_be_rebased?
-    self.project.ff_merge_must_be_possible? && !ff_merge_possible?
-  end
-
-  def rebase_dir_path
-    File.join(Gitlab.config.shared.path, 'tmp/rebase', source_project.id.to_s, id.to_s).to_s
-  end
-
-  def squash_dir_path
-    File.join(Gitlab.config.shared.path, 'tmp/squash', source_project.id.to_s, id.to_s).to_s
-  end
-
-  def rebase_in_progress?
-    # The source project can be deleted
-    return false unless source_project
-
-    File.exist?(rebase_dir_path) && !clean_stuck_rebase
-  end
-
-  def clean_stuck_rebase
-    if File.mtime(rebase_dir_path) < 15.minutes.ago
-      FileUtils.rm_rf(rebase_dir_path)
-      true
-    end
-  end
-
-  def squash_in_progress?
-    # The source project can be deleted
-    return false unless source_project
-
-    File.exist?(squash_dir_path) && !clean_stuck_squash
-  end
-
-  def clean_stuck_squash
-    if File.mtime(squash_dir_path) < 15.minutes.ago
-      FileUtils.rm_rf(squash_dir_path)
-      true
-    end
-  end
-
   def diverged_commits_count
     cache = Rails.cache.read(:"merge_request_#{id}_diverged_commits")
 

app/views/projects/ee/_merge_request_settings.html.haml

@@ -20,9 +20,10 @@
       %span.descr
         A merge commit is created for every merge, but merging is only allowed if fast-forward merge is possible.
         This way you could make sure that if this merge request would build, after merging to target branch it would also build.
-      %br
-      %span.descr
-        When fast-forward merge is not possible, the user is given the option to rebase.
+      - if project.feature_available?(:merge_request_rebase)
+        %br
+        %span.descr
+          When fast-forward merge is not possible, the user is given the option to rebase.
 
   .radio
     = label_tag :project_merge_method_ff do
@@ -31,9 +32,10 @@
       %br
       %span.descr
         No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded.
-      %br
-      %span.descr
-        When fast-forward merge is not possible, the user is given the option to rebase.
+      - if project.feature_available?(:merge_request_rebase)
+        %br
+        %span.descr
+          When fast-forward merge is not possible, the user is given the option to rebase.
 
 .form-group
   = form.label :merge_requests_template, class: 'label-light' do

changelogs/unreleased-ee/2570-namespace-rebase-before-merge.yml

+---
+title: Introduce namespace license checks for rebase before merge
+merge_request: 2200
+author:

spec/models/ee/merge_request_spec.rb

+require 'spec_helper'
+
+describe MergeRequest, models: true do
+  let(:project) { create(:project) }
+
+  subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+  describe '#should_be_rebased?' do
+    subject { merge_request.should_be_rebased? }
+
+    context 'project forbids rebase' do
+      it { is_expected.to be_falsy }
+    end
+
+    context 'project allows rebase' do
+      let(:project) { create(:project, merge_requests_rebase_enabled: true) }
+
+      it 'returns false when the project feature is unavailable' do
+        expect(merge_request.target_project).to receive(:feature_available?).with(:merge_request_rebase).and_return(false)
+
+        is_expected.to be_falsy
+      end
+
+      it 'returns true when the project feature is available' do
+        expect(merge_request.target_project).to receive(:feature_available?).with(:merge_request_rebase).and_return(true)
+
+        is_expected.to be_truthy
+      end
+    end
+  end
+
+  describe '#rebase_in_progress?' do
+    it 'returns true when there is a current rebase directory' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(subject.rebase_in_progress?).to be_truthy
+    end
+
+    it 'returns false when there is no rebase directory' do
+      allow(File).to receive(:exist?).with(subject.rebase_dir_path).and_return(false)
+
+      expect(subject.rebase_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the rebase directory has expired' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(20.minutes.ago)
+
+      expect(subject.rebase_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the source project has been removed' do
+      allow(subject).to receive(:source_project).and_return(nil)
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(File).not_to have_received(:exist?)
+      expect(subject.rebase_in_progress?).to be_falsey
+    end
+  end
+
+  describe '#squash_in_progress?' do
+    it 'returns true when there is a current squash directory' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(subject.squash_in_progress?).to be_truthy
+    end
+
+    it 'returns false when there is no squash directory' do
+      allow(File).to receive(:exist?).with(subject.squash_dir_path).and_return(false)
+
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the squash directory has expired' do
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(20.minutes.ago)
+
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+
+    it 'returns false when the source project has been removed' do
+      allow(subject).to receive(:source_project).and_return(nil)
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:mtime).and_return(Time.now)
+
+      expect(File).not_to have_received(:exist?)
+      expect(subject.squash_in_progress?).to be_falsey
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -913,68 +913,6 @@ describe MergeRequest, models: true do
     subject { create :merge_request, :simple }
   end
 
-  describe '#rebase_in_progress?' do
-    it 'returns true when there is a current rebase directory' do
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(Time.now)
-
-      expect(subject.rebase_in_progress?).to be_truthy
-    end
-
-    it 'returns false when there is no rebase directory' do
-      allow(File).to receive(:exist?).with(subject.rebase_dir_path).and_return(false)
-
-      expect(subject.rebase_in_progress?).to be_falsey
-    end
-
-    it 'returns false when the rebase directory has expired' do
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(20.minutes.ago)
-
-      expect(subject.rebase_in_progress?).to be_falsey
-    end
-
-    it 'returns false when the source project has been removed' do
-      allow(subject).to receive(:source_project).and_return(nil)
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(Time.now)
-
-      expect(File).not_to have_received(:exist?)
-      expect(subject.rebase_in_progress?).to be_falsey
-    end
-  end
-
-  describe '#squash_in_progress?' do
-    it 'returns true when there is a current squash directory' do
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(Time.now)
-
-      expect(subject.squash_in_progress?).to be_truthy
-    end
-
-    it 'returns false when there is no squash directory' do
-      allow(File).to receive(:exist?).with(subject.squash_dir_path).and_return(false)
-
-      expect(subject.squash_in_progress?).to be_falsey
-    end
-
-    it 'returns false when the squash directory has expired' do
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(20.minutes.ago)
-
-      expect(subject.squash_in_progress?).to be_falsey
-    end
-
-    it 'returns false when the source project has been removed' do
-      allow(subject).to receive(:source_project).and_return(nil)
-      allow(File).to receive(:exist?).and_return(true)
-      allow(File).to receive(:mtime).and_return(Time.now)
-
-      expect(File).not_to have_received(:exist?)
-      expect(subject.squash_in_progress?).to be_falsey
-    end
-  end
-
   describe '#commits_sha' do
     before do
       allow(subject.merge_request_diff).to receive(:commits_sha)