Merge branch 'merge_request_approvals' into 'master'

Ability to set certain approvers for MRs

https://dev.gitlab.org/gitlab/gitlabhq/issues/2258

See merge request !447

CHANGELOG-EE

@@ -2,6 +2,7 @@ v 7.13 (Unrelease)
   - Fix git hook validation on initial push to master branch.
   - Reset approvals on push
   - Fix 500 error when the source project of an MR is deleted 
+  - Ability to define merge request approvers
 
 v 7.12.2
   - Fixed the alignment of project settings icons

app/controllers/projects/approvers_controller.rb

+class Projects::ApproversController < ApplicationController
+  def destroy
+    if params[:merge_request_id]
+      authorize_create_merge_request!
+      merge_request = project.merge_requests.find_by!(iid: params[:merge_request_id])
+      merge_request.approvers.find(params[:id]).destroy
+    else
+      authorize_admin_project!
+      project.approvers.find(params[:id]).destroy
+    end
+  
+    redirect_to :back
+  end
+end

app/controllers/projects/merge_requests_controller.rb

@@ -149,6 +149,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def automerge
     return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+    return render_404 unless @merge_request.approved?
 
     if @merge_request.automergeable?
       AutoMergeWorker.perform_async(@merge_request.id, current_user.id, params)
@@ -201,6 +202,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def approve
+    unless @merge_request.can_approve?(current_user)
+      return render_404
+    end
+
     @approval = @merge_request.approvals.new
     @approval.user = current_user
 
@@ -286,7 +291,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def merge_request_params
     params.require(:merge_request).permit(
       :title, :assignee_id, :source_project_id, :source_branch,
-      :target_project_id, :target_branch, :milestone_id,
+      :target_project_id, :target_branch, :milestone_id, :approver_ids,
       :state_event, :description, :task_num, label_ids: []
     )
   end

app/controllers/projects_controller.rb

@@ -195,7 +195,7 @@ class ProjectsController < ApplicationController
       :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch,
       :wiki_enabled, :merge_requests_template, :visibility_level, :merge_requests_rebase_enabled,
       :import_url, :last_activity_at, :namespace_id, :avatar, :merge_requests_rebase_default,
-      :approvals_before_merge
+      :approvals_before_merge, :approver_ids
     )
   end
 

app/helpers/merge_requests_helper.rb

@@ -68,4 +68,42 @@ module MergeRequestsHelper
       }
     )
   end
+
+  def render_items_list(items, separator = "and")
+    items_cnt = items.size
+
+    case items_cnt
+    when 1
+      items.first
+    when 2
+      "#{items.first} #{separator} #{items.last}"
+    else
+      last_item = items.pop
+      "#{items.join(", ")} #{separator} #{last_item}"
+    end
+  end
+
+  def render_require_section(merge_request)
+    str = if merge_request.approvals_left == 1
+            "Requires one more approval"
+          else
+            "Requires #{merge_request.approvals_left} more approvals"
+          end
+
+    if merge_request.approvers_left.any?
+      more_approvals = merge_request.approvals_left - merge_request.approvers_left.count
+      approvers_names = merge_request.approvers_left.map(&:name)
+
+      case true
+      when more_approvals > 0
+        str << " (from #{render_items_list(approvers_names + ["#{more_approvals} more"])})"
+      when more_approvals < 0
+        str << " (from #{render_items_list(approvers_names, "or")})"
+      else
+        str << " (from #{render_items_list(approvers_names)})"
+      end
+    end
+
+    str
+  end
 end

app/models/approval.rb

+# == Schema Information
+#
+# Table name: approvals
+#
+#  id               :integer          not null, primary key
+#  merge_request_id :integer          not null
+#  user_id          :integer          not null
+#  created_at       :datetime
+#  updated_at       :datetime
+#
+
 class Approval < ActiveRecord::Base
   belongs_to :user
   belongs_to :merge_request

app/models/approver.rb

+# == Schema Information
+#
+# Table name: approvers
+#
+#  id          :integer          not null, primary key
+#  target_id   :integer          not null
+#  target_type :string(255)
+#  user_id     :integer          not null
+#  created_at  :datetime
+#  updated_at  :datetime
+#
+
+class Approver < ActiveRecord::Base
+  belongs_to :target, polymorphic: true
+  belongs_to :user
+end

app/models/merge_request.rb

@@ -36,6 +36,7 @@ class MergeRequest < ActiveRecord::Base
 
   has_one :merge_request_diff, dependent: :destroy
   has_many :approvals, dependent: :destroy
+  has_many :approvers, as: :target, dependent: :destroy
 
   after_create :create_merge_request_diff
   after_update :update_merge_request_diff
@@ -419,16 +420,29 @@ class MergeRequest < ActiveRecord::Base
     approvals_required - approvals.count
   end
 
+  def approvers_left
+    user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id)
+    User.where id: user_ids
+  end
+
   def approvals_required
     target_project.approvals_before_merge
   end
 
   def requires_approve?
-    !approvals_required.zero?
+    approvals_required.nonzero?
+  end
+
+  def overall_approvers
+    if approvers.any?
+      approvers
+    else
+      target_project.approvers
+    end
   end
 
   def approved?
-    approvals.count >= approvals_required
+    approvals_left.zero?
   end
 
   def approved_by?(user)
@@ -439,6 +453,15 @@ class MergeRequest < ActiveRecord::Base
     approvals.map(&:user)
   end
 
+  def can_approve?(user)
+    approvers_left.include?(user) ||
+    (any_approver_allowed? && !approved_by?(user))
+  end
+
+  def any_approver_allowed?
+    approvals_left > approvers_left.count
+  end
+
   def has_ci?
     source_project.ci_service && commits.any?
   end
@@ -460,4 +483,10 @@ class MergeRequest < ActiveRecord::Base
       "Open"
     end
   end
+
+  def approver_ids=(value)
+    value.split(",").map(&:strip).each do |user_id|
+      approvers.find_or_create_by(user_id: user_id, target_id: id)
+    end
+  end
 end

app/models/project.rb

@@ -117,6 +117,7 @@ class Project < ActiveRecord::Base
   has_many :deploy_keys, through: :deploy_keys_projects
   has_many :users_star_projects, dependent: :destroy
   has_many :starrers, through: :users_star_projects, source: :user
+  has_many :approvers, as: :target, dependent: :destroy
 
   has_many :project_group_links, dependent: :destroy
   has_many :invited_groups, through: :project_group_links, source: :group
@@ -760,4 +761,10 @@ class Project < ActiveRecord::Base
   def jira_tracker_active?
     jira_tracker? && jira_service.active
   end
+
+  def approver_ids=(value)
+    value.split(",").map(&:strip).each do |user_id|
+      approvers.find_or_create_by(user_id: user_id, target_id: id)
+    end
+  end
 end

app/views/projects/_merge_request_settings.html.haml

@@ -36,3 +36,31 @@
         = f.label :reset_approvers_on_push do
           = f.check_box :reset_approvers_on_push
           %span.descr Reset approvers on push
+
+  .form-group
+    = f.label :approver_ids, class: 'control-label' do
+      Approvers
+    .col-sm-10
+      = users_select_tag("project[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
+      .help-block
+        Default approvers for each merge request.
+
+      .panel.panel-default.prepend-top-10
+        .panel-heading
+          Approvers
+          %small
+            (#{@project.approvers.count(:all)})
+        %ul.well-list
+          - @project.approvers.each do |approver|
+            %li
+              = link_to approver.user.name, approver.user
+              .pull-right
+                = link_to namespace_project_approver_path(@project.namespace, @project, 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 @project.approvers.empty?
+            %li There are no approvers
+
+
+:coffeescript
+  new UsersSelect()

app/views/projects/edit.html.haml

 .project-edit-container
   .project-edit-errors
   .project-edit-content
-    %div
+    .panel
       %h3.page-title
         Project settings
       %hr

app/views/projects/merge_requests/widget/open/_approve.html.haml

 %div
   %h4
-    Requires #{pluralize(@merge_request.approvals_left, 'more approval')}
-  %p Each merge request in this project must be approved by #{pluralize(@merge_request.approvals_required, 'person')} before it can be accepted.
-  - unless @merge_request.approved_by?(current_user)
+    = render_require_section(@merge_request)
+    
+  - if @merge_request.can_approve?(current_user)
     .append-bottom-10
       = form_for [:approve, @project.namespace.becomes(Namespace), @project, @merge_request], method: :post do |f|
         = f.submit "Approve Merge Request", class: "btn btn-primary approve-btn"

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

@@ -81,6 +81,33 @@
         = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank
 
 - if issuable.is_a?(MergeRequest)
+  - if @project.approvals_before_merge.nonzero?
+    .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. 
+          - if @project.approvers.any?
+            By default: #{render_items_list(@project.approvers.map{ |approver| approver.user.name})}.
+            You can override the project settings by setting your own list of approvers.
+        
+        .panel.panel-default.prepend-top-10
+          .panel-heading
+            Approvers
+            %small
+              (#{@merge_request.approvers.count(:all)})
+          %ul.well-list
+            - @merge_request.approvers.each do |approver|
+              %li
+                = link_to approver.user.name, approver.user
+                .pull-right
+                  = link_to namespace_project_merge_request_approver_path(@project.namespace, @project, @merge_request, 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 @merge_request.approvers.empty?
+              %li There are no approvers
   %hr
     - if @merge_request.new_record?
       .form-group

config/routes.rb

@@ -507,6 +507,7 @@ Gitlab::Application.routes.draw do
             get :branch_to
             get :update_branches
           end
+          resources :approvers, only: :destroy
         end
 
         resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
@@ -569,6 +570,8 @@ Gitlab::Application.routes.draw do
             get ":secret/:filename", action: :show, as: :show, constraints: { filename: /[^\/]+/ }
           end
         end
+
+        resources :approvers, only: :destroy
       end
 
       get "/audit_events" => "audit_events#project_log"

db/migrate/20150707222220_add_approvers_table.rb

+class AddApproversTable < ActiveRecord::Migration
+  def change
+    create_table :approvers do |t|
+      t.integer :target_id, null: false
+      t.string :target_type
+      t.integer :user_id, null: false
+
+      t.timestamps
+
+      t.index [:target_id, :target_type]
+      t.index :user_id
+    end
+  end
+end

db/schema.rb

@@ -57,6 +57,17 @@ ActiveRecord::Schema.define(version: 20150713160110) do
     t.datetime "updated_at"
   end
 
+  create_table "approvers", force: true do |t|
+    t.integer  "target_id",   null: false
+    t.string   "target_type"
+    t.integer  "user_id",     null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
+  add_index "approvers", ["target_id", "target_type"], name: "index_approvers_on_target_id_and_target_type", using: :btree
+  add_index "approvers", ["user_id"], name: "index_approvers_on_user_id", using: :btree
+
   create_table "audit_events", force: true do |t|
     t.integer  "author_id",   null: false
     t.string   "type",        null: false

features/project/merge_requests.feature

@@ -240,6 +240,21 @@ Feature: Project Merge Requests
     When I click link "Approve"
     Then I should see approved merge request "Bug NS-04"
 
+  Scenario: I approve merge request if I am an approver
+    Given merge request 'Bug NS-04' must be approved by current user
+    And I click link "Bug NS-04"
+    And I should not see merge button
+    And I should see message that MR require an approval from me
+    When I click link "Approve"
+    Then I should see approved merge request "Bug NS-04"
+
+  Scenario: I can not approve merge request if I am not an approver
+    Given merge request 'Bug NS-04' must be approved by some user
+    And I click link "Bug NS-04"
+    And I should not see merge button
+    When I should not see Approve button
+    And I should see message that MR require an approval
+
   Scenario: I should see rebase checkbox
     Given project "Shop" have "Bug NS-05" open merge request with diffs inside
     And rebase before merge enabled

features/steps/project/merge_requests.rb

@@ -346,6 +346,24 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     project.save!
   end
 
+  step 'merge request \'Bug NS-04\' must be approved by current user' do
+    merge_request = MergeRequest.find_by!(title: "Bug NS-04")
+    project = merge_request.target_project
+    project.approvals_before_merge = 1
+    project.approvers.create(user_id: current_user.id)
+    project.ensure_satellite_exists
+    project.save!
+  end
+
+  step 'merge request \'Bug NS-04\' must be approved by some user' do
+    merge_request = MergeRequest.find_by!(title: "Bug NS-04")
+    project = merge_request.target_project
+    project.approvals_before_merge = 1
+    project.approvers.create(user_id: create(:user).id)
+    project.ensure_satellite_exists
+    project.save!
+  end
+
   step 'I click link "Approve"' do
     page.within '.mr-state-widget' do
       click_button 'Approve Merge Request'
@@ -358,12 +376,30 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     end
   end
 
+  step 'I should not see Approve button' do
+    page.within '.mr-state-widget' do
+      expect(page).not_to have_button("Approve")
+    end
+  end
+
   step 'I should see approved merge request "Bug NS-04"' do
     page.within '.mr-state-widget' do
       expect(page).to have_button("Accept Merge Request")
     end
   end
 
+  step 'I should see message that MR require an approval from me' do
+    page.within '.mr-state-widget' do
+      expect(page).to have_content("Requires one more approval (from #{current_user.name})")
+    end
+  end
+
+  step 'I should see message that MR require an approval' do
+    page.within '.mr-state-widget' do
+      expect(page).to have_content("Requires one more approval")
+    end
+  end
+
   step 'rebase before merge enabled' do
     project = merge_request.target_project
     project.merge_requests_rebase_enabled = true

spec/helpers/merge_request_helper_spec.rb

@@ -40,4 +40,61 @@ describe MergeRequestsHelper do
       it { is_expected.to eq('#JIRA-123, #JIRA-456, and #FOOBAR-7890') }
     end
   end
+
+  describe 'render_items_list' do
+    it "returns one item in the list" do
+      expect(render_items_list(["user"])).to eq("user")
+    end
+
+    it "returns two items in the list" do
+      expect(render_items_list(["user", "user1"])).to eq("user and user1")
+    end
+
+    it "returns three items in the list" do
+      expect(render_items_list(["user", "user1", "user2"])).to eq("user, user1 and user2")
+    end
+  end
+
+  describe 'render_require_section' do
+    it "returns correct value in case - one approval" do
+      project.update(approvals_before_merge: 1)
+      merge_request = create(:merge_request, target_project: project, source_project: project)
+      expect(render_require_section(merge_request)).to eq("Requires one more approval")
+    end
+
+    it "returns correct value in case - two approval" do
+      project.update(approvals_before_merge: 2)
+      merge_request = create(:merge_request, target_project: project, source_project: project)
+      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals")
+    end
+
+    it "returns correct value in case - one approver" do
+      project.update(approvals_before_merge: 1)
+      merge_request = create(:merge_request, target_project: project, source_project: project)
+      user = create :user
+      merge_request.approvers.create(user_id: user.id)
+
+      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{user.name})")
+    end
+
+    it "returns correct value in case - one approver and one more" do
+      project.update(approvals_before_merge: 2)
+      merge_request = create(:merge_request, target_project: project, source_project: project)
+      user = create :user
+      merge_request.approvers.create(user_id: user.id)
+
+      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{user.name} and 1 more)")
+    end
+
+    it "returns correct value in case - two approver and one more" do
+      project.update(approvals_before_merge: 3)
+      merge_request = create(:merge_request, target_project: project, source_project: project)
+      user = create :user
+      user1 = create :user
+      merge_request.approvers.create(user_id: user.id)
+      merge_request.approvers.create(user_id: user1.id)
+
+      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{user1.name}, #{user.name} and 1 more)")
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -175,6 +175,30 @@ describe MergeRequest do
     end
   end
 
+  describe "approvers_left" do
+    let(:merge_request) {create :merge_request}
+
+    it "returns correct value" do
+      user = create(:user)
+      user1 = create(:user)
+      merge_request.target_project.approvers.create(user_id: user.id)
+      merge_request.target_project.approvers.create(user_id: user1.id)
+      merge_request.approvals.create(user_id: user1.id)
+
+      expect(merge_request.approvers_left).to eq [user]
+    end
+  end
+
+  describe "approvals_required" do
+    let(:merge_request) {create :merge_request}
+
+    it "takes approvals_before_merge" do
+      merge_request.target_project.update(approvals_before_merge: 2)
+      
+      expect(merge_request.approvals_required).to eq 2
+    end
+  end
+
   it_behaves_like 'an editable mentionable' do
     subject { create(:merge_request, source_project: project) }