Merge branch 'ccr/approvals_2452' into 'master'

Add ability to have zero approvers

See merge request gitlab-org/gitlab-ee!5635

app/models/merge_request.rb

@@ -169,7 +169,6 @@ class MergeRequest < ActiveRecord::Base
   scope :unassigned, -> { where("assignee_id IS NULL") }
   scope :assigned_to, ->(u) { where(assignee_id: u.id)}
 
-  participant :participant_approvers
   participant :assignee
 
   after_save :keep_around_commit
@@ -301,10 +300,6 @@ class MergeRequest < ActiveRecord::Base
     author_id == user.id || assignee_id == user.id
   end
 
-  def participant_approvers
-    requires_approve? ? approvers_left : []
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
@@ -549,22 +544,6 @@ class MergeRequest < ActiveRecord::Base
     !source_project.in_fork_network_of?(target_project)
   end
 
-  def validate_approvals_before_merge
-    return true unless approvals_before_merge
-    return true unless target_project
-
-    # Approvals disabled
-    if target_project.approvals_before_merge == 0
-      errors.add :validate_approvals_before_merge,
-                 'Approvals disabled for target project'
-    elsif approvals_before_merge > target_project.approvals_before_merge
-      true
-    else
-      errors.add :validate_approvals_before_merge,
-                 'Number of approvals must be greater than those on target project'
-    end
-  end
-
   def reopenable?
     closed? && !source_project_missing? && source_branch_exists?
   end

app/serializers/merge_request_widget_entity.rb

@@ -22,9 +22,6 @@ class MergeRequestWidgetEntity < IssuableEntity
     merge_request.project.merge_requests_ff_only_enabled
   end
 
-  # EE-specific
-  expose :approvals_before_merge
-
   expose :rebase_commit_sha
   expose :rebase_in_progress?, as: :rebase_in_progress
 

doc/user/project/merge_requests/merge_request_approvals.md

@@ -13,9 +13,9 @@ Merge request approvals enable enforced code review by requiring specified peopl
 3. Specifying reviewers for a given proposed code change.
 4. Specifying categories of reviewers, such as BE, FE, QA, DB, etc., for all proposed code changes.
 
-## Activating approvals
+## Editing approvals
 
-To activate the merge request approvals:
+To edit the merge request approvals:
 
 1. Navigate to your project's **Settings > General** and expand the
    **Merge requests settings**
@@ -23,7 +23,7 @@ To activate the merge request approvals:
 1. Search for users or groups that will be [eligible to approve](#eligible-approvers)
    merge requests and click the **Add** button to add them as approvers
 1. Set the minimum number of required approvals under the "Approvals required"
-   box
+   box. Note: the minimum can be 0. 
 1. Click **Save changes**
 
     ![Approvals config project](img/approvals_config_project.png)
@@ -106,7 +106,7 @@ or a [failed CI/CD pipeline](merge_when_pipeline_succeeds.md).
 
 > Introduced in GitLab Enterprise Edition 9.4.
 
-If approvals are [activated at the project level](#activating-approvals), the
+If approvals are [set at the project level](#editing-approvals), the
 default configuration (number of required approvals and approvers) can be
 overridden for each merge request in that project.
 
@@ -151,7 +151,7 @@ Read what happens  when the
 
 ## Resetting approvals on push
 
-If approvals are [activated at the project level](#activating-approvals),
+If approvals are [set at the project level](#editing-approvals),
 you can choose whether all approvals on a merge request are removed when
 new commits are pushed to the source branch of the merge request:
 

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -41,7 +41,8 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     this.isApproved = this.isApproved || false;
     this.approvals = this.approvals || null;
     this.approvalsPath = data.approvals_path || this.approvalsPath;
-    this.approvalsRequired = data.approvalsRequired || Boolean(this.approvalsPath);
+    this.approvalsRequired =
+      data.approvalsRequired || (Boolean(this.approvalsPath) && data.approvals_before_merge > 0);
   }
 
   setApprovals(data) {

ee/app/models/concerns/approvable.rb

@@ -2,7 +2,12 @@ module Approvable
   include Gitlab::Utils::StrongMemoize
 
   def requires_approve?
-    approvals_required.nonzero?
+    # keep until UI changes implemented
+    true
+  end
+
+  def approval_needed?
+    approvals_required&.nonzero?
   end
 
   def approved?

ee/app/models/ee/merge_request.rb

@@ -46,6 +46,8 @@ module EE
       delegate :expose_sast_container_data?, to: :head_pipeline, allow_nil: true
       delegate :expose_container_scanning_data?, to: :head_pipeline, allow_nil: true
       delegate :expose_dast_data?, to: :head_pipeline, allow_nil: true
+
+      participant :participant_approvers
     end
 
     def supports_weight?
@@ -67,5 +69,22 @@ module EE
       !!(head_pipeline&.expose_performance_data? &&
          base_pipeline&.expose_performance_data?)
     end
+
+    def validate_approvals_before_merge
+      return true unless approvals_before_merge
+      return true unless target_project
+
+      # Ensure per-merge-request approvals override is valid
+      if approvals_before_merge >= target_project.approvals_before_merge
+        true
+      else
+        errors.add :validate_approvals_before_merge,
+                   'Number of approvals must be at least that of approvals on the target project'
+      end
+    end
+
+    def participant_approvers
+      approval_needed? ? approvers_left : []
+    end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -3,6 +3,8 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
+      expose :approvals_required, as: :approvals_before_merge
+
       expose :blob_path do
         expose :head_path, if: -> (mr, _) { mr.head_pipeline_sha } do |merge_request|
           project_blob_path(merge_request.project, merge_request.head_pipeline_sha)

ee/app/views/shared/issuable/_approvals.html.haml

@@ -68,6 +68,6 @@
 
     - if can_update_approvers
       .form-text.text-muted.suggested-approvers
-        - if @suggested_approvers.any?
+        - if @suggested_approvers&.any?
           Suggested approvers:
           = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")

ee/changelogs/unreleased/ccr-zero-approvers.yml

+title: Add ability to have zero approvers.
+merge_request: 5635
+type: added

ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb

+require 'rails_helper'
+
+describe 'Merge request > User selects branches for new MR', :js do
+  let(:project) { create(:project, :public, :repository) }
+  let(:user) { project.creator }
+
+  def select_source_branch(branch_name)
+    find('.js-source-branch', match: :first).click
+    find('.js-source-branch-dropdown .dropdown-input-field').native.send_keys branch_name
+    find('.js-source-branch-dropdown .dropdown-content a', text: branch_name, match: :first).click
+  end
+
+  before do
+    project.add_master(user)
+    sign_in(user)
+  end
+
+  context 'when approvals are zero for the target project' do
+    before do
+      project.update_attributes(approvals_before_merge: 0)
+
+      visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' })
+    end
+
+    it 'shows approval settings' do
+      expect(page).to have_content('Approvers')
+    end
+  end
+end

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -32,6 +32,21 @@ describe API::MergeRequestApprovals do
       expect(json_response['approvers'][0]['user']['username']).to eq(approver.username)
       expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
     end
+
+    context 'when approvers are set to zero' do
+      before do
+        project.update!(approvals_before_merge: 0)
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
+      end
+
+      it 'returns a 200' do
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it 'does not include an error in the response' do
+        expect(json_response['message']).to eq(nil)
+      end
+    end
   end
 
   describe 'POST :id/merge_requests/:merge_request_iid/approvals' do
@@ -50,6 +65,19 @@ describe API::MergeRequestApprovals do
           expect(json_response['approvals_required']).to eq(5)
         end
 
+        context 'when project approvals are zero' do
+          before do
+            project.update!(approvals_before_merge: 0)
+          end
+
+          it 'does not include an error in the response' do
+            expect do
+              post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), approvals_required: 0
+            end.to change {merge_request.reload.approvals_before_merge}.from(nil).to(0)
+            expect(json_response['message']).to eq(nil)
+          end
+        end
+
         it 'does not allow approvals required under what the project requires' do
           expect do
             post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), approvals_required: 1
@@ -57,20 +85,6 @@ describe API::MergeRequestApprovals do
 
           expect(response).to have_gitlab_http_status(400)
         end
-
-        context 'when project approvals are not enabled' do
-          before do
-            project.update_attribute(:approvals_before_merge, 0)
-          end
-
-          it 'does not allow you to override approvals required' do
-            expect do
-              post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), approvals_required: 5
-            end.not_to change { merge_request.reload.approvals_before_merge }
-
-            expect(response).to have_gitlab_http_status(400)
-          end
-        end
       end
 
       context 'when disable_overriding_approvers_per_merge_request is true on the project' do

ee/spec/requests/api/merge_requests_spec.rb

+require "spec_helper"
+
+describe API::MergeRequests do
+  include ProjectForksHelper
+
+  let(:base_time)   { Time.now }
+  let(:user)        { create(:user) }
+  let!(:project)    { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
+  let(:milestone)   { create(:milestone, title: '1.0.0', project: project) }
+  let(:milestone1)   { create(:milestone, title: '0.9', project: project) }
+  let!(:merge_request) { create(:merge_request, :simple, milestone: milestone1, author: user, assignee: user, source_project: project, target_project: project, title: "Test", created_at: base_time) }
+  let!(:label) do
+    create(:label, title: 'label', color: '#FFAABB', project: project)
+  end
+  let!(:label2) { create(:label, title: 'a-test', color: '#FFFFFF', project: project) }
+
+  before do
+    project.add_reporter(user)
+  end
+
+  describe "POST /projects/:id/merge_requests" do
+    def create_merge_request(args)
+      defaults = {
+          title: 'Test merge_request',
+          source_branch: 'feature_conflict',
+          target_branch: 'master',
+          author: user,
+          labels: 'label, label2',
+          milestone_id: milestone.id
+      }
+      defaults = defaults.merge(args)
+      post api("/projects/#{project.id}/merge_requests", user), defaults
+    end
+    context 'between branches projects' do
+      it "returns merge_request" do
+        create_merge_request(squash: true)
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['title']).to eq('Test merge_request')
+        expect(json_response['labels']).to eq(%w(label label2))
+        expect(json_response['milestone']['id']).to eq(milestone.id)
+        expect(json_response['squash']).to be_truthy
+        expect(json_response['force_remove_source_branch']).to be_falsy
+      end
+
+      context 'the approvals_before_merge param' do
+        context 'when the target project has disable_overriding_approvers_per_merge_request set to true' do
+          before do
+            project.update_attributes(disable_overriding_approvers_per_merge_request: true)
+            create_merge_request(approvals_before_merge: 1)
+          end
+
+          it 'does not update approvals_before_merge' do
+            expect(json_response['approvals_before_merge']).to eq(nil)
+          end
+        end
+
+        context 'when the target project has approvals_before_merge set to zero' do
+          before do
+            project.update_attributes(approvals_before_merge: 0)
+            create_merge_request(approvals_before_merge: 1)
+          end
+
+          it 'returns a 201' do
+            expect(response).to have_gitlab_http_status(201)
+          end
+
+          it 'does not include an error in the response' do
+            expect(json_response['message']).to eq(nil)
+          end
+        end
+
+        context 'when the target project has a non-zero approvals_before_merge' do
+          context 'when the approvals_before_merge param is less than or equal to the value in the target project' do
+            before do
+              project.update_attributes(approvals_before_merge: 2)
+              create_merge_request(approvals_before_merge: 1)
+            end
+
+            it 'returns a 400' do
+              expect(response).to have_gitlab_http_status(400)
+            end
+
+            it 'includes the error in the response' do
+              expect(json_response['message']['validate_approvals_before_merge']).not_to be_empty
+            end
+          end
+
+          context 'when the approvals_before_merge param is greater than the value in the target project' do
+            before do
+              project.update_attributes(approvals_before_merge: 1)
+              create_merge_request(approvals_before_merge: 2)
+            end
+
+            it 'returns a created status' do
+              expect(response).to have_gitlab_http_status(201)
+            end
+
+            it 'sets approvals_before_merge of the newly-created MR' do
+              expect(json_response['approvals_before_merge']).to eq(2)
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -51,6 +51,10 @@ describe MergeRequestWidgetEntity do
     expect(subject.as_json).to include(:codeclimate)
   end
 
+  it 'sets approvals_before_merge to 0 if nil' do
+    expect(subject.as_json[:approvals_before_merge]).to eq(0)
+  end
+
   it 'has performance data' do
     build = create(:ci_build, name: 'job')
 

spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb

@@ -86,14 +86,6 @@ describe 'Merge request > User selects branches for new MR', :js do
     expect(target_items.count).to be > 1
   end
 
-  context 'when approvals are disabled for the target project' do
-    it 'does not show approval settings' do
-      visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' })
-
-      expect(page).not_to have_content('Approvers')
-    end
-  end
-
   context 'when approvals are enabled for the target project' do
     before do
       project.update_attributes(approvals_before_merge: 1)

spec/presenters/merge_request_presenter_spec.rb

@@ -483,7 +483,7 @@ describe MergeRequestPresenter do
 
       it 'returns path' do
         is_expected
-          .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/approvals")
+            .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}/approvals")
       end
     end
 

spec/requests/api/merge_requests_spec.rb

@@ -694,77 +694,6 @@ describe API::MergeRequests do
         expect(response).to have_gitlab_http_status(201)
       end
     end
-
-    context 'the approvals_before_merge param' do
-      def create_merge_request(approvals_before_merge)
-        post api("/projects/#{project.id}/merge_requests", user),
-             title: 'Test merge_request',
-             source_branch: 'feature_conflict',
-             target_branch: 'master',
-             author: user,
-             labels: 'label, label2',
-             milestone_id: milestone.id,
-             approvals_before_merge: approvals_before_merge
-      end
-
-      context 'when the target project has disable_overriding_approvers_per_merge_request set to true' do
-        before do
-          project.update_attributes(disable_overriding_approvers_per_merge_request: true)
-          create_merge_request(1)
-        end
-
-        it 'does not update approvals_before_merge' do
-          expect(json_response['approvals_before_merge']).to eq(nil)
-        end
-      end
-
-      context 'when the target project has approvals_before_merge set to zero' do
-        before do
-          project.update_attributes(approvals_before_merge: 0)
-          create_merge_request(1)
-        end
-
-        it 'returns a 400' do
-          expect(response).to have_gitlab_http_status(400)
-        end
-
-        it 'includes the error in the response' do
-          expect(json_response['message']['validate_approvals_before_merge']).not_to be_empty
-        end
-      end
-
-      context 'when the target project has a non-zero approvals_before_merge' do
-        context 'when the approvals_before_merge param is less than or equal to the value in the target project' do
-          before do
-            project.update_attributes(approvals_before_merge: 1)
-            create_merge_request(1)
-          end
-
-          it 'returns a 400' do
-            expect(response).to have_gitlab_http_status(400)
-          end
-
-          it 'includes the error in the response' do
-            expect(json_response['message']['validate_approvals_before_merge']).not_to be_empty
-          end
-        end
-
-        context 'when the approvals_before_merge param is greater than the value in the target project' do
-          before do
-            project.update_attributes(approvals_before_merge: 1)
-            create_merge_request(2)
-          end
-
-          it 'returns a created status' do
-            expect(response).to have_gitlab_http_status(201)
-          end
-
-          it 'sets approvals_before_merge of the newly-created MR' do
-            expect(json_response['approvals_before_merge']).to eq(2)
-          end
-        end
-      end
-    end
   end
 
   describe "DELETE /projects/:id/merge_requests/:merge_request_iid" do

spec/services/notification_service_spec.rb

@@ -1100,7 +1100,7 @@ describe NotificationService, :mailer do
           project_approvers.each { |approver| should_email(approver) }
         end
 
-        it 'does not email the approvers when approving is disabled' do
+        it 'does not email the approvers when approval is not necessary' do
           merge_request.target_project.update_attributes(approvals_before_merge: 0)
           notification.new_merge_request(merge_request, @u_disabled)