Merge branch 'mr-approves'

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Conflicts:
	db/schema.rb

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 7.12 (Unreleased)
   - Enhance LDAP group synchronization to check also for submember attributes
   - Prevent LDAP group sync from removing a group's last owner 
   - Add Git hook to validate maximum file size.
+  - Project setting: approve merge request by N users before accept
 
 v 7.11.4
   - no changes specific to EE

app/assets/javascripts/project_new.js.coffee

@@ -4,13 +4,20 @@ class @ProjectNew
       $('.project-edit-container').hide()
       $('.save-project-loader').show()
 
+    @toggleSettings()
     @initEvents()
 
 
   initEvents: ->
     disableButtonIfEmptyField '#project_name', '.project-submit'
 
-    $("#project_merge_requests_enabled").change ->
-      checked = $(this).prop("checked")
-      $("#project_merge_requests_template").prop "disabled", not checked
-      $("#project_merge_requests_rebase_enabled").prop "disabled", not checked
+    $("#project_merge_requests_enabled").change =>
+      @toggleSettings()
+
+  toggleSettings: ->
+    checked = $("#project_merge_requests_enabled").prop("checked")
+    if checked
+      $('.merge-request-feature').show()
+    else
+      $('.merge-request-feature').hide()
+

app/assets/stylesheets/pages/merge_requests.scss

@@ -188,3 +188,11 @@
 .merge-request-form .select2-container {
   width: 250px !important;
 }
+
+.approve-btn {
+  margin-right: 10px;
+}
+
+.approved-by-users {
+  padding: 5px 0;
+}

app/controllers/projects/merge_requests_controller.rb

@@ -4,7 +4,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :module_enabled
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :automerge, :automerge_check,
-    :ci_status, :toggle_subscription
+    :ci_status, :toggle_subscription, :approve
   ]
   before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits]
   before_action :validates_merge_request, only: [:show, :diffs, :commits]
@@ -195,6 +195,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render nothing: true
   end
 
+  def approve
+    @approval = @merge_request.approvals.new
+    @approval.user = current_user
+    @approval.save
+
+    redirect_to merge_request_path(@merge_request)
+  end
+
   protected
 
   def selected_target_project

app/controllers/projects_controller.rb

@@ -151,7 +151,7 @@ class ProjectsController < ApplicationController
   end
 
   def markdown_preview
-    text = params[:text] 
+    text = params[:text]
 
     ext = Gitlab::ReferenceExtractor.new(@project, current_user)
     ext.analyze(text)
@@ -188,7 +188,8 @@ class ProjectsController < ApplicationController
       :name, :path, :description, :issues_tracker, :tag_list,
       :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
+      :import_url, :last_activity_at, :namespace_id, :avatar, :merge_requests_rebase_default,
+      :approvals_before_merge
     )
   end
 

app/models/approval.rb

+class Approval < ActiveRecord::Base
+  belongs_to :user
+  belongs_to :merge_request
+
+  validates :merge_request_id, presence: true
+  validates :user_id, presence: true, uniqueness: { scope: [:merge_request_id] }
+end

app/models/merge_request.rb

@@ -35,6 +35,7 @@ class MergeRequest < ActiveRecord::Base
   belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project"
 
   has_one :merge_request_diff, dependent: :destroy
+  has_many :approvals, dependent: :destroy
 
   after_create :create_merge_request_diff
   after_update :update_merge_request_diff
@@ -408,4 +409,28 @@ class MergeRequest < ActiveRecord::Base
 
     locked_at.nil? || locked_at < (Time.now - 1.day)
   end
+
+  def approvals_left
+    approvals_required - approvals.count
+  end
+
+  def approvals_required
+    target_project.approvals_before_merge
+  end
+
+  def requires_approve?
+    !approvals_required.zero?
+  end
+
+  def approved?
+    approvals.count >= approvals_required
+  end
+
+  def approved_by?(user)
+    approved_by_users.include?(user)
+  end
+
+  def approved_by_users
+    approvals.map(&:user)
+  end
 end

app/models/project.rb

@@ -152,6 +152,7 @@ class Project < ActiveRecord::Base
   validate :avatar_type,
     if: ->(project) { project.avatar.present? && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
+  validates :approvals_before_merge, numericality: true, allow_blank: true
 
   mount_uploader :avatar, AvatarUploader
 

app/models/user.rb

@@ -130,6 +130,7 @@ class User < ActiveRecord::Base
   has_many :assigned_issues,          dependent: :destroy, foreign_key: :assignee_id, class_name: "Issue"
   has_many :assigned_merge_requests,  dependent: :destroy, foreign_key: :assignee_id, class_name: "MergeRequest"
   has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy
+  has_many :approvals, dependent: :destroy
 
 
   #

app/views/projects/_merge_request_settings.html.haml

+%fieldset.merge-request-feature
+  %legend
+    Merge requests:
+
+  .form-group
+    .col-sm-offset-2.col-sm-10
+      .checkbox
+        = f.label :merge_requests_rebase_enabled do
+          = f.check_box :merge_requests_rebase_enabled
+          %span.descr Allows rebasing of merge requests before merging.
+
+  .form-group.rebase-default
+    .col-sm-offset-2.col-sm-10
+      .checkbox
+        = f.label :merge_requests_rebase_default do
+          = f.check_box :merge_requests_rebase_default
+          %span.descr Rebase enabled by default
+
+  .form-group
+    = f.label :merge_requests_template, class: 'control-label' do
+      Description template
+    .col-sm-10
+      = f.text_area :merge_requests_template, placeholder: "This MR should have: *", class: "form-control", rows: 3
+
+  .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", min: 0
+      .help-block
+        How many users should approve merge request before it can be accepted. 0 - approval is disabled

app/views/projects/edit.html.haml

@@ -54,26 +54,6 @@
                   = f.check_box :merge_requests_enabled
                   %span.descr Submit changes to be merged upstream.
 
-            .form-group
-              = f.label :merge_requests_rebase_enabled, "Merge Requests Rebase", class: 'control-label'
-              .col-sm-10
-                .checkbox
-                  = f.check_box :merge_requests_rebase_enabled
-                  %span.descr Allows rebasing of merge requests before merging.
-
-              .rebase-default
-                .col-sm-10.col-sm-offset-3
-                  .checkbox
-                    = f.check_box :merge_requests_rebase_default
-                    %span.descr Enabled by default
-
-            .form-group
-              = f.label :merge_requests_template, class: 'control-label' do
-                Merge request template
-                %span.light (optional)
-              .col-sm-10
-                = f.text_area :merge_requests_template, placeholder: "This MR should have: *", disabled: !@project.merge_requests_enabled, class: "form-control", rows: 3
-
             .form-group
               = f.label :wiki_enabled, "Wiki", class: 'control-label'
               .col-sm-10
@@ -88,6 +68,8 @@
                   = f.check_box :snippets_enabled
                   %span.descr Share code pastes with others out of git repository
 
+          = render 'merge_request_settings', f: f
+
           %fieldset.features
             %legend
               Project avatar:

app/views/projects/merge_requests/show/_approve.html.haml

+.clearfix
+  - unless @merge_request.approved_by?(current_user)
+    .pull-left
+      = form_for [:approve, @project.namespace.becomes(Namespace), @project, @merge_request], method: :post do |f|
+        = f.submit "Approve Merge Request", class: "btn btn-reopen approve-btn"
+
+  - if @merge_request.approvals.any?
+    .pull-left.approved-by-users
+      Approved by
+      - @merge_request.approved_by_users.each do |user|
+        = link_to_member(@project, user, name: false, size: 24)
+
+%br
+%p This merge request must be approved by #{pluralize(@merge_request.approvals_required, 'user')} before it can be merged

app/views/projects/merge_requests/show/_mr_accept.html.haml

@@ -17,34 +17,37 @@
 - if @show_merge_controls
   .automerge_widget.can_be_merged.hide
     .clearfix
-      = form_for [:automerge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post do |f|
-        .accept-merge-holder.clearfix.js-toggle-container
-          .accept-action
-            = f.submit "Accept Merge Request", class: "btn btn-create accept_merge_request"
-          - if can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && !@merge_request.for_fork?
-            .accept-control.checkbox
-              = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
-                = check_box_tag :should_remove_source_branch
-                Remove source-branch
-          - if @merge_request.target_project.merge_requests_rebase_enabled && can_rebase?(@merge_request.target_project, @merge_request.target_branch)
-            .accept-control.remove_branch_holder
-              = label_tag :should_rebase, class: "checkbox" do
-                = check_box_tag :should_rebase, "1", @project.merge_requests_rebase_default
-                Rebase before merge
-          .accept-control
-            = link_to "#", class: "modify-merge-commit-link js-toggle-button", title: "Modify merge commit message" do
-              %i.fa.fa-edit
-              Modify commit message
-          .js-toggle-content.hide.prepend-top-20
-            = render 'shared/commit_message_container', params: params,
-                text: @merge_request.merge_commit_message,
-                rows: 14, hint: true
+      - if @merge_request.requires_approve? && !@merge_request.approved?
+        = render 'projects/merge_requests/show/approve'
+      - else
+        = form_for [:automerge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post do |f|
+          .accept-merge-holder.clearfix.js-toggle-container
+            .accept-action
+              = f.submit "Accept Merge Request", class: "btn btn-create accept_merge_request"
+            - if can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && !@merge_request.for_fork?
+              .accept-control.checkbox
+                = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
+                  = check_box_tag :should_remove_source_branch
+                  Remove source-branch
+            - if @merge_request.target_project.merge_requests_rebase_enabled && can_rebase?(@merge_request.target_project, @merge_request.target_branch)
+              .accept-control.remove_branch_holder
+                = label_tag :should_rebase, class: "checkbox" do
+                  = check_box_tag :should_rebase, "1", @project.merge_requests_rebase_default
+                  Rebase before merge
+            .accept-control
+              = link_to "#", class: "modify-merge-commit-link js-toggle-button", title: "Modify merge commit message" do
+                %i.fa.fa-edit
+                Modify commit message
+            .js-toggle-content.hide.prepend-top-20
+              = render 'shared/commit_message_container', params: params,
+                  text: @merge_request.merge_commit_message,
+                  rows: 14, hint: true
 
-        %br
-        .light
-          If you want to merge this request manually, you can use the
-          %strong
-            = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"
+          %br
+          .light
+            If you want to merge this request manually, you can use the
+            %strong
+              = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal"
 
 
   .automerge_widget.no_satellite.hide

config/routes.rb

@@ -491,6 +491,7 @@ Gitlab::Application.routes.draw do
             get :automerge_check
             get :ci_status
             post :toggle_subscription
+            post :approve
           end
 
           collection do

db/migrate/20150609113337_create_approves.rb

+class CreateApproves < ActiveRecord::Migration
+  def change
+    create_table :approvals do |t|
+      t.integer :merge_request_id, null: false
+      t.integer :user_id, null: false
+
+      t.timestamps
+    end
+  end
+end

db/migrate/20150609125332_add_project_merge_approves.rb

+class AddProjectMergeApproves < ActiveRecord::Migration
+  def change
+    add_column :projects, :approvals_before_merge, :integer, null: false, default: 0
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150605131047) do
+ActiveRecord::Schema.define(version: 20150609125332) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -49,6 +49,13 @@ ActiveRecord::Schema.define(version: 20150605131047) do
     t.string   "after_sign_out_path"
   end
 
+  create_table "approvals", force: true do |t|
+    t.integer  "merge_request_id", null: false
+    t.integer  "user_id",          null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
   create_table "audit_events", force: true do |t|
     t.integer  "author_id",   null: false
     t.string   "type",        null: false
@@ -434,6 +441,7 @@ ActiveRecord::Schema.define(version: 20150605131047) do
     t.string   "import_type"
     t.string   "import_source"
     t.boolean  "merge_requests_rebase_default", default: true
+    t.integer  "approvals_before_merge",        default: 0,        null: false
   end
 
   add_index "projects", ["created_at", "id"], name: "index_projects_on_created_at_and_id", using: :btree

features/project/merge_requests.feature

@@ -220,3 +220,10 @@ Feature: Project Merge Requests
     When I click the "Target branch" dropdown
     And I select a new target branch
     Then I should see new target branch changes
+
+  Scenario: I approve merge request
+    Given merge request 'Bug NS-04' must be approved
+    And I click link "Bug NS-04"
+    And I should not see merge button
+    When I click link "Approve"
+    Then I should see approved merge request "Bug NS-04"

features/steps/project/merge_requests.rb

@@ -329,6 +329,29 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     page.should have_content 'Target branch changed from master to feature'
   end
 
+  step 'merge request \'Bug NS-04\' must be approved' do
+    merge_request = MergeRequest.find_by!(title: "Bug NS-04")
+    project = merge_request.target_project
+    project.approvals_before_merge = 1
+    project.save!
+  end
+
+  step 'I click link "Approve"' do
+    click_button 'Approve Merge Request'
+  end
+
+  step 'I should not see merge button' do
+    within '.can_be_merged' do
+      page.should_not have_button("Accept Merge Request")
+    end
+  end
+
+  step 'I should see approved merge request "Bug NS-04"' do
+    within '.can_be_merged' do
+      page.should have_button("Accept Merge Request")
+    end
+  end
+
   def merge_request
     @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05")
   end

spec/factories/approvals.rb

+# Read about factories at https://github.com/thoughtbot/factory_girl
+
+FactoryGirl.define do
+  factory :approval do
+    merge_request
+    user
+  end
+end

spec/models/approval_spec.rb

+require 'spec_helper'
+
+describe Approval do
+  subject { create(:approval) }
+
+  it { should be_valid }
+end