Merge branch '582-toggle-overwrite' into 'master'

add toggle to override Approvals per-MR

Closes #582

See merge request !2016

app/controllers/projects_controller.rb

@@ -337,6 +337,7 @@ class ProjectsController < Projects::ApplicationController
   def project_params_ee
     %i[
       approvals_before_merge
+      approvals
       approver_group_ids
       approver_ids
       issues_template
@@ -345,6 +346,7 @@ class ProjectsController < Projects::ApplicationController
       mirror
       mirror_trigger_builds
       mirror_user_id
+      disable_overriding_approvers_per_merge_request
       repository_size_limit
       reset_approvals_on_push
       service_desk_enabled

app/models/ee/project.rb

@@ -165,6 +165,10 @@ module EE
       repository.fetch_upstream(self.import_url)
     end
 
+    def can_override_approvers?
+      !disable_overriding_approvers_per_merge_request?
+    end
+
     def shared_runners_available?
       super && !namespace.shared_runners_minutes_used?
     end

app/services/merge_requests/create_service.rb

@@ -9,6 +9,12 @@ module MergeRequests
 
       params[:target_project_id] ||= source_project.id
 
+      unless @project.can_override_approvers?
+        params.delete(:approvals_before_merge)
+        params.delete(:approver_ids)
+        params.delete(:approver_group_ids)
+      end
+
       merge_request = MergeRequest.new
       merge_request.source_project = source_project
       merge_request.source_branch = params[:source_branch]

app/services/merge_requests/update_service.rb

@@ -17,6 +17,12 @@ module MergeRequests
         merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
       end
 
+      unless project.can_override_approvers?
+        params.delete(:approvals_before_merge)
+        params.delete(:approver_ids)
+        params.delete(:approver_group_ids)
+      end
+
       old_approvers = merge_request.overall_approvers.to_a
 
       handle_wip_event(merge_request)

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

 - return unless project.feature_available?(:merge_request_approvers)
+- can_override_approvers = project.can_override_approvers?
 
 .form-group.reset-approvals-on-push
   .checkbox
     = label_tag :require_approvals do
       = check_box_tag :require_approvals, nil, project.approvals_before_merge.nonzero?, class: 'js-require-approvals-toggle'
-      %strong Activate merge request approvals
+      %strong Merge request approvals
       = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
-      .descr Merge request approvals allow you to set the number of necessary approvals and predefine a list of approvers that you will need to approve every merge request in a project.
 
 .nested-settings{ class: project.approvals_before_merge.nonzero? ? '' : 'hidden' }
   .form-group
@@ -19,7 +19,7 @@
       %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' }
         Add
     .help-block
-      Add an approver or group suggestion for each merge request
+      Add users or groups who are allowed to approve every merge request
 
     .panel.panel-default.prepend-top-10.js-current-approvers
       .panel-heading
@@ -61,11 +61,18 @@
       Approvals required
     = form.number_field :approvals_before_merge, class: "form-control", min: 0
     .help-block
+      Set number of approvers required before open merge requests can be merged
+
+  .form-group
+    .checkbox
+      = form.label :disable_overriding_approvers_per_merge_request do
+        = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers }, false, true)
+        %strong Can override approvers and approvals required per merge request
+        = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals", anchor: 'can-override-approvers-and-approvals-required-per-merge-request'), target: '_blank'
 
   .form-group.reset-approvals-on-push
     .checkbox
       = form.label :reset_approvals_on_push do
         = form.check_box :reset_approvals_on_push
-        %strong Reset approvals on push
-        .descr Approvals are reset when new data is pushed to the merge request
+        %strong Remove all approvals in a merge request when new commits are pushed to its source branch
 

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

@@ -4,31 +4,23 @@
 - return unless issuable.is_a?(MergeRequest)
 - return unless issuable.requires_approve?
 
-.form-group
-  = form.label :approvals_before_merge, class: 'control-label' do
-    Approvals required
-  .col-sm-10
-    = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required
-    .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(issuable.target_project.approvals_before_merge, 'user')}),
-      then it will be ignored and the project default will be used.
+- can_override_approvers = issuable.target_project.can_override_approvers?
+
 .form-group
   = form.label :approver_ids, class: 'control-label' do
     Approvers
   .col-sm-10
-    - author = issuable.author || current_user
-    - skip_users = issuable.all_approvers_including_groups + [author]
-    = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: skip_users, project: issuable.target_project)
-    .help-block
-      This merge request must be approved by these users.
-      You can override the project settings by setting your own list of approvers.
+    - if can_override_approvers
+      = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', email_user: true, skip_users: issuable.all_approvers_including_groups, project: issuable.target_project)
+      .help-block
+        This merge request must be approved by these users.
+        You can override the project settings by setting your own list of approvers.
 
-    - skip_groups = issuable.overall_approver_groups.pluck(:group_id)
-    = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
-    .help-block
-      This merge request must be approved by members of these groups.
-      You can override the project settings by setting your own list of approvers.
+      - skip_groups = issuable.overall_approver_groups.pluck(:group_id)
+      = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
+      .help-block
+        This merge request must be approved by members of these groups.
+        You can override the project settings by setting your own list of approvers.
 
     .panel.panel-default.prepend-top-10
       .panel-heading
@@ -42,29 +34,39 @@
           - issuable.overall_approvers.each do |approver|
             %li{ id: dom_id(approver.user), class: item_classes + ['approver'] }
               = link_to approver.user.name, approver.user
-              .pull-right
-                - if unsaved_approvers
-                  = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
-                    = icon("sign-out")
-                    Remove
-                - else
-                  = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
-                    = icon("sign-out")
-                    Remove
+              - if can_override_approvers
+                .pull-right
+                  - if unsaved_approvers
+                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                      = icon("sign-out")
+                      Remove
+                  - else
+                    = link_to project_merge_request_approver_path(@project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                      = icon("sign-out")
+                      Remove
           - issuable.overall_approver_groups.each do |approver_group|
             %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
               Group:
               = link_to approver_group.group.name, approver_group.group
-              .pull-right
-                - if unsaved_approvers
-                  = link_to "#", data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, class: "btn-xs btn btn-remove", title: 'Remove group' do
-                    = icon("sign-out")
-                    Remove
-                - else
-                  = link_to project_merge_request_approver_group_path(@project, issuable, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
-                    = icon("sign-out")
-                    Remove
-    .help-block.suggested-approvers
-      - if @suggested_approvers.any?
-        Suggested approvers:
-        = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")
+              - if can_override_approvers
+                .pull-right
+                  - if unsaved_approvers
+                    = link_to "#", data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, class: "btn-xs btn btn-remove", title: 'Remove group' do
+                      = icon("sign-out")
+                      Remove
+                  - else
+                    = link_to project_merge_request_approver_group_path(@project, issuable, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
+                      = icon("sign-out")
+                      Remove
+
+    .col-sm-12
+      .form-group
+        = form.label :approvals_before_merge, class: 'label-light' do
+          Approvals required
+        = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_override_approvers
+
+    - if can_override_approvers
+      .help-block.suggested-approvers
+        - if @suggested_approvers.any?
+          Suggested approvers:
+          = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")

changelogs/unreleased-ee/582-toggle-override.yml

+---
+title: add toggle for overriding approvers per MR
+merge_request:
+author:

db/migrate/20170605151307_add_disable_overriding_approvers_per_merge_request_to_project.rb

+class AddDisableOverridingApproversPerMergeRequestToProject < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :disable_overriding_approvers_per_merge_request, :boolean
+  end
+end

db/schema.rb

@@ -1357,6 +1357,7 @@ ActiveRecord::Schema.define(version: 20170706121518) do
     t.integer "cached_markdown_version"
     t.datetime "last_repository_updated_at"
     t.string "ci_config_path"
+    t.boolean "disable_overriding_approvers_per_merge_request"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
@@ -1559,7 +1560,7 @@ ActiveRecord::Schema.define(version: 20170706121518) do
 
   add_index "slack_integrations", ["team_id", "alias"], name: "index_slack_integrations_on_team_id_and_alias", unique: true, using: :btree
   add_index "slack_integrations", ["service_id"], name: "index_slack_integrations_on_service_id", using: :btree
-  
+
   create_table "snippets", force: :cascade do |t|
     t.string "title"
     t.text "content"

doc/user/project/merge_requests/merge_request_approvals.md

@@ -42,6 +42,14 @@ This sets the amount of approvals required before being able to merge a merge re
 
 The number of approvers can be higher than the required approvals.
 
+### Can override approvers and approvals required per merge request
+
+> Introduced in GitLab Enterprise Edition 9.4.
+
+When this setting is enabled, the approvers for a project can be overridden for
+a merge request. When editing a merge request you can add or remove approvers,
+and increase the number of required approvers.
+
 ### Reset approvals on push
 
 With this setting turned on, approvals are reset when a new push

spec/controllers/projects/merge_requests_controller_ee_spec.rb

@@ -104,6 +104,52 @@ describe Projects::MergeRequestsController do
       it_behaves_like 'update invalid issuable', MergeRequest
     end
 
+    context 'overriding approvers per MR' do
+      before do
+        project.update_attributes(approvals_before_merge: 1)
+      end
+
+      context 'enabled' do
+        before do
+          project.update_attributes(disable_overriding_approvers_per_merge_request: false)
+        end
+
+        it 'updates approvals' do
+          update_merge_request(approvals_before_merge: 2)
+
+          expect(merge_request.reload.approvals_before_merge).to eq(2)
+        end
+      end
+
+      context 'disabled' do
+        let(:new_approver) { create(:user) }
+        let(:new_approver_group) { create(:approver_group) }
+
+        before do
+          project.team << [new_approver, :developer]
+          project.update_attributes(disable_overriding_approvers_per_merge_request: true)
+        end
+
+        it 'does not update approvals_before_merge' do
+          update_merge_request(approvals_before_merge: 2)
+
+          expect(merge_request.reload.approvals_before_merge).to eq(nil)
+        end
+
+        it 'does not update approver_ids' do
+          update_merge_request(approver_ids: [new_approver].map(&:id).join(','))
+
+          expect(merge_request.reload.approver_ids).to be_empty
+        end
+
+        it 'does not update approver_group_ids' do
+          update_merge_request(approver_group_ids: [new_approver_group].map(&:id).join(','))
+
+          expect(merge_request.reload.approver_group_ids).to be_empty
+        end
+      end
+    end
+
     context 'the approvals_before_merge param' do
       before do
         project.update_attributes(approvals_before_merge: 2)

spec/features/merge_requests/approvals_spec.rb

@@ -178,7 +178,6 @@ feature 'Merge request approvals', js: true, feature: true do
 
         # project setting in the beginning on the edit MR page
         expect(find('#merge_request_approvals_before_merge').value).to eq('1')
-        expect(find('#merge_request_approvals_before_merge ~ .help-block')).to have_content('1 user')
 
         fill_in 'merge_request_approvals_before_merge', with: '3'
 
@@ -192,7 +191,6 @@ feature 'Merge request approvals', js: true, feature: true do
 
         # new MR setting on the edit MR page
         expect(find('#merge_request_approvals_before_merge').value).to eq('3')
-        expect(find('#merge_request_approvals_before_merge ~ .help-block')).to have_content('1 user')
       end
     end
   end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -403,6 +403,7 @@ Project:
 - merge_requests_rebase_enabled
 - approvals_before_merge
 - reset_approvals_on_push
+- disable_overriding_approvers_per_merge_request
 - merge_requests_ff_only_enabled
 - issues_template
 - repository_size_limit

spec/requests/api/merge_requests_spec.rb

@@ -529,6 +529,17 @@ describe API::MergeRequests do
              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)

spec/views/shared/issuable/_approvals.html.haml_spec.rb

+require 'spec_helper'
+
+describe 'shared/issuable/_approvals.html.haml' do
+  let(:project) { build(:empty_project) }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let(:form) { double('form') }
+
+  before do
+    allow(form).to receive(:label)
+    allow(form).to receive(:number_field)
+    allow(merge_request).to receive(:requires_approve?).and_return(true)
+    assign(:project, project)
+    assign(:suggested_approvers, [])
+  end
+
+  context 'has no approvers' do
+    it 'shows empty approvers list' do
+      render 'shared/issuable/approvals', form: form, issuable: merge_request
+
+      expect(rendered).to have_text('There are no approvers')
+    end
+
+    context 'can override approvers' do
+      before do
+        allow(project).to receive(:can_override_approvers?).and_return(true)
+        render 'shared/issuable/approvals', form: form, issuable: merge_request
+      end
+
+      it 'shows suggested approvers' do
+        expect(rendered).to have_css('.suggested-approvers')
+      end
+
+      it 'shows select approvers field' do
+        expect(rendered).to have_css('#merge_request_approver_ids')
+      end
+
+      it 'shows select approver groups field' do
+        expect(rendered).to have_css('#merge_request_approver_group_ids')
+      end
+    end
+
+    context 'can not override approvers' do
+      before do
+        allow(project).to receive(:can_override_approvers?).and_return(false)
+        render 'shared/issuable/approvals', form: form, issuable: merge_request
+      end
+
+      it 'hides suggested approvers' do
+        expect(rendered).not_to have_css('.suggested-approvers')
+      end
+
+      it 'hides select approvers field' do
+        expect(rendered).not_to have_css('#merge_request_approver_ids')
+      end
+
+      it 'hides select approver groups field' do
+        expect(rendered).not_to have_css('#merge_request_approver_group_ids')
+      end
+    end
+  end
+
+  context 'has approvers' do
+    let(:user) { create(:user) }
+    let(:approver) { create(:approver, user: user, target: merge_request) }
+    let(:approver_group) { create(:approver_group, target: merge_request) }
+
+    before do
+      assign(:approver, approver)
+      assign(:approver_group, approver_group)
+    end
+
+    it 'shows approver in table' do
+      render 'shared/issuable/approvals', form: form, issuable: merge_request, project: project
+
+      expect(rendered).to have_text(approver[:name])
+      expect(rendered).to have_text(approver_group[:name])
+    end
+
+    context 'can override approvers' do
+      it 'shows remove button for approver' do
+        render 'shared/issuable/approvals', form: form, issuable: merge_request
+
+        expect(rendered).to have_css('.btn-remove')
+      end
+    end
+
+    context 'can not override approvers' do
+      it 'hides remove button' do
+        allow(project).to receive(:can_override_approvers?).and_return(false)
+
+        render 'shared/issuable/approvals', form: form, issuable: merge_request
+
+        expect(rendered).not_to have_css('.btn-remove')
+      end
+    end
+  end
+end