Merge branch '314-add-merge-approval-field-in-mr-form' into 'master'

Allow setting required approval count on MR

Closes #314.

See merge request !470

CHANGELOG-EE

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.9.0 (unreleased)
   - Fix JenkinsService test button
   - Fix nil user handling in UpdateMirrorService
+  - Allow overriding the number of approvers for a merge request
   - Allow LDAP to mark users as external based on their group membership. !432
   - Forbid MR authors from approving their own MRs
   - Instrument instance methods of Gitlab::InsecureKeyFingerprint class

app/controllers/projects/merge_requests_controller.rb

@@ -136,7 +136,13 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def create
     @target_branches ||= []
-    @merge_request = MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
+    create_params = merge_request_params
+
+    if create_params[:approvals_before_merge].to_i <= project.approvals_before_merge
+      create_params.delete(:approvals_before_merge)
+    end
+
+    @merge_request = MergeRequests::CreateService.new(project, current_user, create_params).execute
 
     if @merge_request.valid?
       redirect_to(merge_request_path(@merge_request))
@@ -395,6 +401,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       :title, :assignee_id, :source_project_id, :source_branch,
       :target_project_id, :target_branch, :milestone_id, :approver_ids,
       :state_event, :description, :task_num, :force_remove_source_branch,
+      :approvals_before_merge,
       label_ids: []
     )
   end

app/models/merge_request.rb

@@ -105,6 +105,7 @@ class MergeRequest < ActiveRecord::Base
   validates :merge_user, presence: true, if: :merge_when_build_succeeds?
   validate :validate_branches, unless: :allow_broken
   validate :validate_fork
+  validate :validate_approvals_before_merge
 
   scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) }
   scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) }
@@ -220,6 +221,22 @@ class MergeRequest < ActiveRecord::Base
     end
   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 update_merge_request_diff
     if source_branch_changed? || target_branch_changed?
       reload_code
@@ -479,7 +496,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def approvals_required
-    target_project.approvals_before_merge
+    approvals_before_merge || target_project.approvals_before_merge
   end
 
   def requires_approve?

app/views/shared/issuable/_form.html.haml

@@ -113,13 +113,23 @@
 
 - if issuable.is_a?(MergeRequest)
   - if @merge_request.requires_approve?
+    - approvals = issuable.target_project.approvals_before_merge
+    .form-group
+      = f.label :approvals_before_merge, class: 'control-label' do
+        Approvals required
+      .col-sm-10
+        = f.number_field :approvals_before_merge, class: 'form-control', value: approvals
+        .help-block
+          Number of users who need to approve this merge request before it can be accepted.
+          If this isn't greater than the project default (#{pluralize(approvals, 'user')}),
+          then it will be ignored and the project default will be used.
     .form-group
       = f.label :approver_ids, class: 'control-label' do
         Approvers
       .col-sm-10
         = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
         .help-block
-          Merge Request should be approved by these users.
+          This merge request must be approved by these users.
           You can override the project settings by setting your own list of approvers.
 
         .panel.panel-default.prepend-top-10

db/migrate/20160615142732_add_approvals_before_merge_to_merge_requests.rb

+class AddApprovalsBeforeMergeToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    add_column :merge_requests, :approvals_before_merge, :integer
+  end
+end

db/schema.rb

@@ -707,6 +707,7 @@ ActiveRecord::Schema.define(version: 20160616084004) do
     t.integer  "merge_user_id"
     t.string   "merge_commit_sha"
     t.datetime "deleted_at"
+    t.integer  "approvals_before_merge"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree

doc/api/merge_requests.md

@@ -68,7 +68,8 @@ Parameters:
     "merge_when_build_succeeds": true,
     "merge_status": "can_be_merged",
     "subscribed" : false,
-    "user_notes_count": 1
+    "user_notes_count": 1,
+    "approvals_before_merge": null
   }
 ]
 ```
@@ -132,7 +133,8 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "approvals_before_merge": null
 }
 ```
 
@@ -233,6 +235,7 @@ Parameters:
   "merge_status": "can_be_merged",
   "subscribed" : true,
   "user_notes_count": 1,
+  "approvals_before_merge": null,
   "changes": [
     {
     "old_path": "VERSION",
@@ -257,15 +260,25 @@ POST /projects/:id/merge_requests
 
 Parameters:
 
-- `id` (required)                - The ID of a project
-- `source_branch` (required)     - The source branch
-- `target_branch` (required)     - The target branch
-- `assignee_id` (optional)       - Assignee user ID
-- `title` (required)             - Title of MR
-- `description` (optional)       - Description of MR
-- `target_project_id` (optional) - The target project (numeric id)
-- `labels` (optional)            - Labels for MR as a comma-separated list
-- `milestone_id` (optional)      - Milestone ID
+- `id` (required)                      - The ID of a project
+- `source_branch` (required)           - The source branch
+- `target_branch` (required)           - The target branch
+- `assignee_id` (optional)             - Assignee user ID
+- `title` (required)                   - Title of MR
+- `description` (optional)             - Description of MR
+- `target_project_id` (optional)       - The target project (numeric id)
+- `labels` (optional)                  - Labels for MR as a comma-separated list
+- `milestone_id` (optional)            - Milestone ID
+- `approvals_before_merge` (optional)  - Number of approvals required before this can be merged (see below)
+
+If `approvals_before_merge` is not provided, it inherits the value from the
+target project. If it is provided, then the following conditions must hold in
+order for it to take effect:
+
+1. The target project's `approvals_before_merge` must be greater than zero. (A
+   value of zero disables approvals for that project.)
+2. The provided value of `approvals_before_merge` must be greater than the
+   target project's `approvals_before_merge`.
 
 ```json
 {
@@ -312,7 +325,8 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 0
+  "user_notes_count": 0,
+  "approvals_before_merge": null
 }
 ```
 
@@ -383,7 +397,8 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "approvals_before_merge": null
 }
 ```
 
@@ -481,7 +496,8 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "approvals_before_merge": null
 }
 ```
 
@@ -535,7 +551,7 @@ GET /projects/:id/merge_requests/:merge_request_id/approvals
 
 >**Note:** This API endpoint is only available on 8.9 EE and above.
 
-If you are allowed to, you can approve a merge request using the following 
+If you are allowed to, you can approve a merge request using the following
 endpoint:
 
 ```
@@ -649,7 +665,8 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "approvals_before_merge": null
 }
 ```
 
@@ -715,7 +732,8 @@ Example response when the GitLab issue tracker is used:
       "created_at" : "2016-01-04T15:31:51.081Z",
       "iid" : 6,
       "labels" : [],
-      "user_notes_count": 1
+      "user_notes_count": 1,
+      "approvals_before_merge": null
    },
 ]
 ```

doc/workflow/merge_request_approvals.md

@@ -56,10 +56,11 @@ After configuring Approvals, you will see the following during merge request cre
 
 ![Choosing approvers in merge request creation](merge_request_approvals/approvals_mr.png)
 
-You can change the default set of approvers before creating the merge request.
-You can't change the amount of required approvals. This ensures that you're
-not forced to adjust settings when someone is unavailable for approval, yet
-the process is still enforced.
+You can change the default set of approvers and the amount of required approvals
+before creating the merge request. The amount of required approvals, if changed,
+must be greater than the default set at the project level. This ensures that
+you're not forced to adjust settings when someone is unavailable for approval,
+yet the process is still enforced.
 
 To approve a merge request, simply press the button.
 

lib/api/entities.rb

@@ -211,6 +211,7 @@ module API
         merge_request.subscribed?(options[:current_user])
       end
       expose :user_notes_count
+      expose :approvals_before_merge
     end
 
     class MergeRequestChanges < MergeRequest

lib/api/merge_requests.rb

@@ -63,15 +63,16 @@ module API
       #
       # Parameters:
       #
-      #   id (required)            - The ID of a project - this will be the source of the merge request
-      #   source_branch (required) - The source branch
-      #   target_branch (required) - The target branch
-      #   target_project_id        - The target project of the merge request defaults to the :id of the project
-      #   assignee_id              - Assignee user ID
-      #   title (required)         - Title of MR
-      #   description              - Description of MR
-      #   labels (optional)        - Labels for MR as a comma-separated list
-      #   milestone_id (optional)   - Milestone ID
+      #   id (required)                      - The ID of a project - this will be the source of the merge request
+      #   source_branch (required)           - The source branch
+      #   target_branch (required)           - The target branch
+      #   target_project_id (optional)       - The target project of the merge request defaults to the :id of the project
+      #   assignee_id (optional)             - Assignee user ID
+      #   title (required)                   - Title of MR
+      #   description (optional)             - Description of MR
+      #   labels (optional)                  - Labels for MR as a comma-separated list
+      #   milestone_id (optional)            - Milestone ID
+      #   approvals_before_merge (optional)  - Number of approvals required before this can be merged
       #
       # Example:
       #   POST /projects/:id/merge_requests
@@ -79,7 +80,7 @@ module API
       post ":id/merge_requests" do
         authorize! :create_merge_request, user_project
         required_attributes! [:source_branch, :target_branch, :title]
-        attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description, :milestone_id]
+        attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :target_project_id, :description, :milestone_id, :approvals_before_merge]
 
         # Validate label names in advance
         if (errors = validate_label_params(params)).any?

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -34,6 +34,73 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  describe 'POST #create' do
+    def create_merge_request(overrides = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param,
+        merge_request: {
+          title: 'Test',
+          source_branch: 'feature_conflict',
+          target_branch: 'master',
+          author: user
+        }.merge(overrides)
+      }
+
+      post :create, params
+    end
+
+    context 'the approvals_before_merge param' do
+      before { project.update_attributes(approvals_before_merge: 2) }
+      let(:created_merge_request) { assigns(:merge_request) }
+
+      context 'when it is less than the one in the target project' do
+        before { create_merge_request(approvals_before_merge: 1) }
+
+        it 'sets the param to nil' do
+          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+
+      context 'when it is equal to the one in the target project' do
+        before { create_merge_request(approvals_before_merge: 2) }
+
+        it 'sets the param to nil' do
+          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+
+      context 'when it is greater than the one in the target project' do
+        before { create_merge_request(approvals_before_merge: 3) }
+
+        it 'saves the param in the merge request' do
+          expect(created_merge_request.approvals_before_merge).to eq(3)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+    end
+
+    context 'when the merge request is invalid' do
+      it 'shows the #new form' do
+        expect(create_merge_request(title: nil)).to render_template(:new)
+      end
+    end
+  end
+
   describe "#show" do
     shared_examples "export merge as" do |format|
       it "should generally work" do

spec/features/merge_requests/create_new_mr_spec.rb

@@ -31,6 +31,24 @@ feature 'Create New Merge Request', feature: true, js: true do
     expect(page).to have_content 'git checkout -b orphaned-branch origin/orphaned-branch'
   end
 
+  context 'when approvals are disabled for the target project' do
+    it 'does not show approval settings' do
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature_conflict' })
+
+      expect(page).not_to have_content('Approvers')
+    end
+  end
+
+  context 'when approvals are enabled for the target project' do
+    before { project.update_attributes(approvals_before_merge: 1) }
+
+    it 'shows approval settings' do
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature_conflict' })
+
+      expect(page).to have_content('Approvers')
+    end
+  end
+
   context 'when target project cannot be viewed by the current user' do
     it 'does not leak the private project name & namespace' do
       private_project = create(:project, :private)

spec/models/merge_request_spec.rb

@@ -238,12 +238,21 @@ describe MergeRequest, models: true do
   end
 
   describe "#approvals_required" do
-    let(:merge_request) {create :merge_request}
+    let(:merge_request) { build(:merge_request) }
+    before { merge_request.target_project.update_attributes(approvals_before_merge: 3) }
 
-    it "takes approvals_before_merge" do
-      merge_request.target_project.update(approvals_before_merge: 2)
+    context "when the MR has approvals_before_merge set" do
+      before { merge_request.update_attributes(approvals_before_merge: 1) }
 
-      expect(merge_request.approvals_required).to eq 2
+      it "uses the approvals_before_merge from the MR" do
+        expect(merge_request.approvals_required).to eq(1)
+      end
+    end
+
+    context "when the MR doesn't have approvals_before_merge set" do
+      it "takes approvals_before_merge from the target project" do
+        expect(merge_request.approvals_required).to eq(3)
+      end
     end
   end
 

spec/requests/api/merge_requests_spec.rb

@@ -353,6 +353,67 @@ describe API::API, api: true  do
         expect(response.status).to eq(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 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_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_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_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_id" do