Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ee into fix/stuck-remote_mirror

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.9.0 (unreleased)
+v 8.10.0 (unreleased)
+
+v 8.9.0
   - Fix Error 500 when using closes_issues API with an external issue tracker
   - Add more information into RSS feed for issues (Alexander Matyushentsev)
   - Bulk assign/unassign labels to issues.

CHANGELOG-EE

 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.9.0 (unreleased)
+v 8.10.0 (unreleased)
+
+v 8.9.0
   - 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
   - Add API endpoint for Merge Request Approvals !449
+  - Send notification email when merge request is approved
   - Distribute RepositoryUpdateMirror jobs in time and add exclusive lease on them by project_id
   - [Elastic] Move ES settings to application settings
   - Disable mirror flag for projects without import_url
   - UpdateMirror service return an error status when no mirror
+  - Don't reset approvals when rebasing an MR from the UI
   - Show flash notice when Git Hooks are updated successfully
   - Remove explicit Gitlab::Metrics.action assignments, are already automatic.
   - [Elastic] Project members with guest role can't access confidential issues

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))
@@ -385,7 +391,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def set_suggested_approvers
     if @merge_request.requires_approve?
       @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request
+        @merge_request,
+        current_user
       ).calculate(@merge_request.approvals_required)
     end
   end
@@ -395,6 +402,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/helpers/path_locks_helper.rb

 module PathLocksHelper
-  def can_unlock?(path_lock, current_user = @current_user)
-    can?(current_user, :admin_locks, path_lock) || path_lock.user == current_user
+  def can_unlock?(path_lock, current_user = @current_user, project = @project)
+    can?(current_user, :admin_path_locks, project) || path_lock.user == current_user
   end
 
   def license_allows_file_locks?
-    ::License.current && ::License.current.add_on?('GitLab_FileLocks')
+    @license_allows_file_locks ||= (::License.current && ::License.current.add_on?('GitLab_FileLocks'))
   end
 end

app/mailers/emails/merge_requests.rb

@@ -42,6 +42,13 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
+    def approved_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
+      @approved_by_users = @merge_request.approved_by_users.map(&:name)
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+    end
+
     private
 
     def setup_merge_request_mail(merge_request_id, recipient_id)

app/models/ability.rb

@@ -301,7 +301,7 @@ class Ability
         :admin_pipeline,
         :admin_environment,
         :admin_deployment,
-        :admin_locks
+        :admin_path_locks
       ]
     end
 

app/models/concerns/issuable.rb

@@ -45,8 +45,6 @@ module Issuable
     scope :order_milestone_due_desc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date DESC, milestones.id DESC') }
     scope :order_milestone_due_asc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date ASC, milestones.id ASC') }
     scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
-    scope :order_weight_desc, -> { reorder('weight IS NOT NULL, weight DESC') }
-    scope :order_weight_asc, -> { reorder('weight ASC') }
 
     scope :left_joins_milestones,    -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") }
     scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date DESC') }
@@ -122,8 +120,6 @@ module Issuable
       when 'milestone_due_desc' then order_milestone_due_desc
       when 'downvotes_desc' then order_downvotes_desc
       when 'upvotes_desc' then order_upvotes_desc
-      when 'weight_desc' then order_weight_desc
-      when 'weight_asc' then order_weight_asc
       when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
       else
         order_by(method)

app/models/issue.rb

@@ -37,6 +37,8 @@ class Issue < ActiveRecord::Base
 
   scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
   scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
+  scope :order_weight_desc, -> { reorder('weight IS NOT NULL, weight DESC') }
+  scope :order_weight_asc, -> { reorder('weight ASC') }
 
   state_machine :state, initial: :opened do
     event :close do
@@ -93,6 +95,8 @@ class Issue < ActiveRecord::Base
     case method.to_s
     when 'due_date_asc' then order_due_date_asc
     when 'due_date_desc' then order_due_date_desc
+    when 'weight_desc' then order_weight_desc
+    when 'weight_asc' then order_weight_asc
     else
       super
     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
@@ -475,11 +492,12 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def approvers_left
-    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
+    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
+    approvals_before_merge || target_project.approvals_before_merge
   end
 
   def requires_approve?
@@ -507,10 +525,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_approve?(user)
-    return false if user == self.author
-
     approvers_left.include?(user) ||
-      (any_approver_allowed? && !approved_by?(user))
+    (any_approver_allowed? && !approved_by?(user))
   end
 
   def any_approver_allowed?

app/services/merge_requests/approval_service.rb

@@ -8,6 +8,7 @@ module MergeRequests
         mark_pending_todos_as_done(merge_request)
 
         if merge_request.approvals_left.zero?
+          notification_service.approve_mr(merge_request, current_user)
           execute_hooks(merge_request, 'approved')
         end
       end

app/services/merge_requests/rebase_service.rb

@@ -52,6 +52,19 @@ module MergeRequests
         return false
       end
 
+      output, status = popen(
+        %W(git rev-parse #{merge_request.source_branch}),
+        tree_path,
+        git_env
+      )
+
+      unless status.zero?
+        log('Failed to get SHA of rebased branch:')
+        log(output)
+        return false
+      end
+
+      merge_request.update_attributes(rebase_commit_sha: output.chomp)
       # Push
       output, status = popen(
         %W(git push -f origin #{merge_request.source_branch}),

app/services/merge_requests/refresh_service.rb

@@ -83,7 +83,10 @@ module MergeRequests
       merge_requests_for_source_branch.each do |merge_request|
         target_project = merge_request.target_project
 
-        if target_project.approvals_before_merge.nonzero? && target_project.reset_approvals_on_push
+        if target_project.approvals_before_merge.nonzero? &&
+           target_project.reset_approvals_on_push &&
+           merge_request.rebase_commit_sha != @newrev
+
           merge_request.approvals.destroy_all
         end
       end

app/services/notification_service.rb

@@ -115,6 +115,10 @@ class NotificationService
     )
   end
 
+  def approve_mr(merge_request, current_user)
+    approve_mr_email(merge_request, merge_request.target_project, current_user)
+  end
+
   # Notify new user with email after creation
   def new_user(user, token = nil)
     # Don't email omniauth created users
@@ -477,6 +481,14 @@ class NotificationService
     end
   end
 
+  def approve_mr_email(merge_request, project, current_user)
+    recipients = build_recipients(merge_request, project, current_user)
+
+    recipients.each do |recipient|
+      mailer.approved_merge_request_email(recipient.id, merge_request.id, current_user.id).deliver_later
+    end
+  end
+
   def build_recipients(target, project, current_user, action: nil, previous_assignee: nil)
     recipients = target.participants(current_user)
     recipients = add_project_watchers(recipients, project)

app/views/layouts/nav/_project.html.haml

@@ -36,7 +36,7 @@
           Activity
 
     - if project_nav_tab? :files
-      = nav_link(controller: %w(tree blob blame edit_tree new_tree find_file commit commits compare repositories tags branches releases network)) do
+      = nav_link(controller: %w(tree blob blame edit_tree new_tree find_file commit commits compare repositories tags branches releases network path_locks)) do
         = link_to project_files_path(@project), title: 'Code',  class: 'shortcuts-tree' do
           %span
             Code

app/views/notify/approved_merge_request_email.html.haml

+%p
+  = "Merge Request #{@merge_request.to_reference} was approved by #{@approved_by_users.to_sentence}"

app/views/notify/approved_merge_request_email.text.haml

+= "Merge Request #{@merge_request.to_reference} was approved by #{@approved_by_users.to_sentence}"
+
+Merge Request url: #{namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request)}
+
+= merge_path_description(@merge_request, 'to')
+
+Author: #{@merge_request.author_name}
+Assignee: #{@merge_request.assignee_name}

app/views/projects/path_locks/index.html.haml

+- @no_container = true
 - page_title "File Locks"
 = render "projects/commits/head"
 

app/views/projects/refs/logs_tree.js.haml

@@ -12,7 +12,7 @@
   - if path_lock_info
     :plain
       var label = $("<span>")
-        .attr('title', 'Locked by #{escape_javascript path_lock_info.user.name }')
+        .attr('title', 'Locked by #{escape_javascript path_lock_info.user.name}')
         .attr('data-toggle', 'tooltip')
         .addClass('fa fa-lock prepend-left-5');
       row.find('td.tree-item-file-name').append(label);

app/views/shared/_sort_dropdown.html.haml

@@ -18,10 +18,11 @@
         = sort_title_recently_updated
       = link_to page_filter_path(sort: sort_value_oldest_updated) do
         = sort_title_oldest_updated
-      = link_to page_filter_path(sort: sort_value_more_weight) do
-        = sort_title_more_weight
-      = link_to page_filter_path(sort: sort_value_less_weight) do
-        = sort_title_less_weight
+      - if local_assigns[:type] == :issues
+        = link_to page_filter_path(sort: sort_value_more_weight) do
+          = sort_title_more_weight
+        = link_to page_filter_path(sort: sort_value_less_weight) do
+          = sort_title_less_weight
       = link_to page_filter_path(sort: sort_value_milestone_soon) do
         = sort_title_milestone_soon
       = link_to page_filter_path(sort: sort_value_milestone_later) do

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

@@ -26,7 +26,7 @@
         .filter-item.inline.labels-filter
           = render "shared/issuable/label_dropdown"
 
-        - if controller.controller_name == 'issues'
+        - if local_assigns[:type] == :issues
           .filter-item.inline.weight-filter
             - if params[:weight]
               = hidden_field_tag(:weight, params[:weight])
@@ -39,7 +39,7 @@
                       = weight
 
         .pull-right
-          = render 'shared/sort_dropdown'
+          = render 'shared/sort_dropdown', type: local_assigns[:type]
 
     - if controller.controller_name == 'issues'
       .issues_bulk_update.hide

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/migrate/20160621123729_add_rebase_commit_sha_to_merge_requests.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRebaseCommitShaToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :merge_requests, :rebase_commit_sha, :string
+  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: 20160616084004) do
+ActiveRecord::Schema.define(version: 20160621123729) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -707,6 +707,8 @@ 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"
+    t.string   "rebase_commit_sha"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree

doc/administration/auth/ldap.md

@@ -208,7 +208,7 @@ production:
 8.9 and above.
 
 Using the `external_groups` setting will allow you to mark all users belonging
-to these groups as [external users](../../permissions/). Group membership is
+to these groups as [external users](../../permissions/permissions.md). Group membership is
 checked periodically through the `LdapGroupSync` background task.
 
 **Configuration**

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/gitlab-geo/README.md

@@ -7,13 +7,12 @@ We recommend you use with at least GitLab 8.6 EE.
 GitLab Geo allows you to replicate your GitLab instance to other geographical
 locations as a read-only fully operational version.
 
-<!-- START doctoc generated TOC please keep comment here to allow auto update -->
-<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
-
-
 - [Overview](#overview)
 - [Setup instructions](#setup-instructions)
+    - [Database Replication](database.md)
+    - [Configuration](configuration.md)
 - [Current limitations](#current-limitations)
+- [Disaster Recovery](disaster-recovery.md)
 - [Frequently Asked Questions](#frequently-asked-questions)
     - [Can I use Geo in a disaster recovery situation?](#can-i-use-geo-in-a-disaster-recovery-situation)
     - [What data is replicated to a secondary node?](#what-data-is-replicated-to-a-secondary-node)
@@ -54,7 +53,7 @@ Geo instances. Follow the steps below in the order that they appear:
 
 1. Install GitLab Enterprise Edition on the server that will serve as the
    secondary Geo node
-1. [Setup a database replication](./database.md) in `primary <-> secondary (read-only)` topology
+1. [Setup a database replication](database.md) in `primary <-> secondary (read-only)` topology
 1. [Configure GitLab](configuration.md) and set the primary and secondary nodes
 After you set up the database replication and configure the GitLab Geo nodes,
 there are a few things to consider:
@@ -69,6 +68,7 @@ there are a few things to consider:
 - You cannot push code to secondary nodes
 - Git LFS is not supported yet
 - Git Annex is not supported yet
+- Primary node has to be online for OAuth login to happen (existing sessions and git are not affected)
 
 ## Frequently Asked Questions
 
@@ -78,6 +78,9 @@ There are limitations to what we replicate (see Current limitations).
 In an extreme data-loss situation you can make a secondary Geo into your
 primary, but this is not officially supported yet.
 
+If you still want to proceed, see our step-by-step instructions on how to 
+manually [promote a secondary node](disaster-recovery.md) into primary. 
+
 ### What data is replicated to a secondary node?
 
 We currently replicate project repositories and the whole database. This

doc/gitlab-geo/configuration.md

@@ -56,8 +56,13 @@ primary node (**Admin Area > Geo Nodes**) when adding a new one:
 sudo -u git -H ssh-keygen
 ```
 
-The public key for Omnibus installations will be at `/var/opt/gitlab/.ssh/id_rsa.pub`,
-whereas for installation from source it will be at `/home/git/.ssh/id_rsa.pub`.
+Remember to add your primary node to the `known_hosts` file of your `git` user.
+
+You can find ssh key files and `know_hosts` at `/var/opt/gitlab/.ssh/` in
+Omnibus installations or at `/home/git/.ssh/` when following the source
+installation guide.
+
+
 
 If for any reason you generate the key using a different name from the default
 `id_rsa`, or you want to generate an extra key only for the repository
@@ -93,11 +98,11 @@ add any secondary servers as well**.
 
 In the following table you can see what all these settings mean:
 
-| Setting | Description |
-| ------- | ----------- |
-| Primary | This marks a Geo Node as primary. There can be only one primary, make sure that you first add the primary node and then all the others.
-| URL | Your instance's full URL, in the same way it is configured in `gitlab.yml` (source based installations) or `/etc/gitlab/gitlab.rb` (omnibus installations). |
-|Public Key | The SSH public key of the user that your GitLab instance runs on (unless changed, should be the user `git`). That means that you have to go in each Geo Node separately and create an SSH key pair. See the [SSH key creation](#create-ssh-key-pairs-for-geo-nodes) section.
+| Setting   | Description |
+| --------- | ----------- |
+| Primary   | This marks a Geo Node as primary. There can be only one primary, make sure that you first add the primary node and then all the others. |
+| URL       | Your instance's full URL, in the same way it is configured in `gitlab.yml` (source based installations) or `/etc/gitlab/gitlab.rb` (omnibus installations). |
+|Public Key | The SSH public key of the user that your GitLab instance runs on (unless changed, should be the user `git`). That means that you have to go in each Geo Node separately and create an SSH key pair. See the [SSH key creation](#create-ssh-key-pairs-for-geo-nodes) section. |
 
 First, add your primary node by providing its full URL and the public SSH key
 you created previously. Make sure to check the box 'This is a primary node'
@@ -147,3 +152,37 @@ gitlab-rake gitlab:shell:setup
 # For source installations
 sudo -u git -H bundle exec rake gitlab:shell:setup RAILS_ENV=production
 ```
+
+## Troubleshooting
+
+Setting up Geo requires careful attention to details and sometimes it's easy to
+miss a step.
+
+Here is a checklist of questions you should ask to try to detect where you have
+to fix (all commands and path locations are for Omnibus installs):
+
+- Is Postgres replication working?
+- Are my nodes pointing to the correct database instance?
+    - You should make sure your primary Geo node points to the instance with
+      writting permissions.
+    - Any secondary nodes should point only to read-only instances.
+- Can Geo detect my current node correctly?
+    - Geo uses your defined node from `Admin > Geo` screen, and tries to match
+      with the value defined in `/etc/gitlab/gitlab.rb` configuration file.
+      The relevant line looks like: `external_url "http://gitlab.example.com"`.
+    - To check if node on current machine is correctly detected type:
+      `sudo gitlab-rails runner "Gitlab::Geo.current_node"`,
+      expect something like: `#<GeoNode id: 2, schema: "https", host: "gitlab.example.com", port: 443, relative_url_root: "", primary: false, ...>`
+    - By running the command above, `primary` should be `true` when executed in
+      the primary node, and `false` on any secondary
+- Did I defined the correct SSH Key for the node?
+    - You must create an SSH Key for `git` user
+    - This key is the one you have to inform at `Admin > Geo`
+- Can primary node communicate with secondary node by HTTP/HTTPS ports?
+- Can secondary nodes communicate with primary node by HTTP/HTTPS/SSH ports?
+- Can secondary nodes execute a succesfull git clone using git user's own
+  SSH Key to primary node repository?
+
+> This list is an atempt to document all the moving parts that can go wrong.
+We are working into getting all this steps verified automatically in a
+rake task in the future. :)

doc/gitlab-geo/database.md

@@ -220,6 +220,4 @@ When prompted, enter the password you set up for the `gitlab_replicator` user.
 
 ## MySQL replication
 
-TODO
-
-[reconfigure gitlab]: ../restart_gitlab.md#omnibus-gitlab-reconfigure
+We don't support MySQL replication for GitLab Geo.

doc/gitlab-geo/disaster-recovery.md

+# GitLab Geo Disaster Recovery
+
+> **Note:**
+This is not officially supported yet, please don't use as your only
+Disaster Recovery strategy as you may lose data.
+
+GitLab Geo replicates your database and your Git repositories. We will
+support and replicate more data in the future, that will enable you to
+fail-over with minimal effort, in a disaster situation.
+
+See [current limitations](README.md#current-limitations)
+for more information.
+
+
+## Promoting a secondary node
+
+We don't provide yet an automated way to promote a node and do fail-over,
+but you can do it manually if you have `root` access to the machine.
+
+You must make the changes in the exact specific order:
+
+1. Take down your primary node (or make sure it will not go up during this
+   process or you may lose data)
+2. Wait for any database replication to finish
+3. Promote the Postgres in your secondary node as primary
+4. Log-in to your secondary node with a user with `sudo` permission
+5. Open the interactive rails console: `sudo gitlab-rails console` and execute:
+    * List your primary node and note down it's id:
+
+        ```ruby
+        Gitlab::Geo.primary_node
+        ```
+    * Turn your primary into a secondary:
+
+        ```ruby
+        Gitlab::Geo.primary_node.update(primary: false)
+        ```
+    * List your secondary nodes and note down the id of the one you want to promote:
+
+        ```ruby
+        Gitlab::Geo.secondary_nodes
+        ```
+    * To promote a node with id `2` execute:
+
+        ```ruby
+        GeoNode.find(2).update!(primary: true)
+        ```
+    * Now you have to cleanup your new promoted node by running:
+
+        ```ruby
+        Gitlab::Geo.primary_node.oauth_application.destroy!
+        Gitlab::Geo.primary_node.system_hook.destroy!
+        ```
+    * And refresh your old primary node to behave correctly as secondary (assuming id is `1`)
+
+        ```ruby
+        GeoNode.find(1).save!
+        ```
+    * To exit the interactive console, type: `exit`
+
+6. Rsync everything in `/var/opt/gitlab/gitlab-rails/uploads` and
+   `/var/opt/gitlab/gitlab-rails/shared` from your old node to the new one.

doc/workflow/merge_request_approvals.md

@@ -33,7 +33,7 @@ independent of changes to the merge request.
 ### Approvers
 
 At approvers you can define the default set of users that need to approve a
-merge request. The author of a merge request cannot approve that merge request.
+merge request.
 
 If there are more approvers than required approvals, any subset of these users
 can approve the merge request.
@@ -56,11 +56,12 @@ 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.
 
-![Merge request approval](merge_request_approvals/2_approvals.png)
+![Merge request approval](merge_request_approvals/2_approvals.png)
\ No newline at end of file

features/project/merge_requests.feature

@@ -327,6 +327,13 @@ Feature: Project Merge Requests
     And I click link "Close"
     Then I should see closed merge request "Bug NS-04"
 
+  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"
+
   Scenario: Reporter can approve merge request
     Given I am a "Shop" reporter
     And I visit project "Shop" merge requests page
@@ -336,12 +343,13 @@ Feature: Project Merge Requests
     When I click link "Approve"
     Then I should see message that merge request can be merged
 
-  Scenario: I can not approve Merge request if I am the author
-    Given merge request 'Bug NS-04' must be approved
+  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
-    When I should not see Approve button
-    And I should see message that MR require an approval
+    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

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?

lib/gitlab/authority_analyzer.rb

@@ -2,8 +2,9 @@ module Gitlab
   class AuthorityAnalyzer
     COMMITS_TO_CONSIDER = 5
 
-    def initialize(merge_request)
+    def initialize(merge_request, current_user)
       @merge_request = merge_request
+      @current_user = current_user
       @users = Hash.new(0)
     end
 
@@ -11,7 +12,7 @@ module Gitlab
       involved_users
 
       # Picks most active users from hash like: {user1: 2, user2: 6}
-      @users.sort_by { |user, count| -count }.map(&:first).take(number_of_approvers)
+      @users.sort_by { |user, count| count }.map(&:first).take(number_of_approvers)
     end
 
     private
@@ -21,9 +22,7 @@ module Gitlab
 
       list_of_involved_files.each do |path|
         @repo.commits(@merge_request.target_branch, path: path, limit: COMMITS_TO_CONSIDER).each do |commit|
-          if commit.author && commit.author != @merge_request.author
-            @users[commit.author] += 1
-          end
+          @users[commit.author] += 1 if commit.author
         end
       end
     end

lib/gitlab/path_locks_finder.rb

 # The database stores locked paths as following:
-# 'app/models/project.rb' or 'lib/gitlab'
-# To determine that 'lib/gitlab/some_class.rb' is locked we need to generate
+# 'app/models/user.rb' or 'app/models'
+# To determine that 'app/models/user.rb' is locked we need to generate
 # tokens for every requested paths and check every token whether it exist in path locks table or not.
-# So for 'lib/gitlab/some_class.rb' path we would need to search next paths:
-# 'lib', 'lib/gitlab' and 'lib/gitlab/some_class.rb'
-# This class also implements a memoization for common paths like 'lib' 'lib/gitlab', 'app', etc.
+# So for 'app/models/user.rb' path we would need to search next paths:
+# 'app', 'app/models' and 'app/models/user.rb'
+# This class also implements a memoization for common paths like 'app' 'app/models', 'vendor', etc.
 
 class Gitlab::PathLocksFinder
   def initialize(project)

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
@@ -268,49 +335,6 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  describe 'POST #approve' do
-    def approve(user)
-      post :approve, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid, user: user
-    end
-
-    context 'when the user is the author of the MR' do
-      before { merge_request.approvers.create(user: merge_request.author) }
-
-      it "returns a 404" do
-        approve(merge_request.author)
-
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the user is not allowed to approve the MR' do
-      it "returns a 404" do
-        approve(user)
-
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the user is allowed to approve the MR' do
-      before { merge_request.approvers.create(user: user) }
-
-      it 'creates an approval' do
-        service = double(:approval_service)
-
-        expect(MergeRequests::ApprovalService).to receive(:new).with(project, anything).and_return(service)
-        expect(service).to receive(:execute).with(merge_request)
-
-        approve(user)
-      end
-
-      it 'redirects to the MR' do
-        approve(user)
-
-        expect(response).to redirect_to(namespace_project_merge_request_path)
-      end
-    end
-  end
-
   describe "DELETE #destroy" do
     it "denies access to users unless they're admin or project owner" do
       delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid

spec/factories/path_locks.rb

 FactoryGirl.define do
   factory :path_lock do
     project
-    user { build :user }
+    user
     sequence(:path) { |n| "app/model#{n}" }
   end
 end

spec/features/issues/sort_by_weight_spec.rb

+require 'spec_helper'
+
+describe 'Issue sorting by Weight', feature: true do
+  include SortingHelper
+
+  let(:project) { create(:project, :public) }
+  let(:foo) { create(:issue, title: 'foo', project: project) }
+  let(:bar) { create(:issue, title: 'bar', project: project) }
+
+  before do
+    login_as :user
+  end
+
+  describe 'sorting by weight' do
+    before do
+      foo.update(weight: 5)
+      bar.update(weight: 10)
+    end
+
+    it 'sorts by more weight' do
+      visit namespace_project_issues_path(project.namespace, project, sort: sort_value_more_weight)
+
+      expect(first_issue).to include('bar')
+    end
+
+    it 'sorts by less weight' do
+      visit namespace_project_issues_path(project.namespace, project, sort: sort_value_less_weight)
+
+      expect(first_issue).to include('foo')
+    end
+  end
+
+  def first_issue
+    page.all('ul.issues-list > li').first.text
+  end
+end

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/lib/gitlab/authority_analyzer_spec.rb

-require 'spec_helper'
-
-describe Gitlab::AuthorityAnalyzer, lib: true do
-  describe '#calculate' do
-    let(:project) { create(:project) }
-    let(:author) { create(:user) }
-    let(:user_a) { create(:user) }
-    let(:user_b) { create(:user) }
-    let(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: author) }
-    let(:files) { [double(:file, deleted_file: true, old_path: 'foo')] }
-
-    let(:commits) do
-      [
-        double(:commit, author: author),
-        double(:commit, author: user_a),
-        double(:commit, author: user_a),
-        double(:commit, author: user_b),
-        double(:commit, author: author)
-      ]
-    end
-
-    def calculate_approvers(count)
-      described_class.new(merge_request).calculate(count)
-    end
-
-    before do
-      merge_request.compare = double(:compare, diffs: files)
-      allow(merge_request.target_project.repository).to receive(:commits).and_return(commits)
-    end
-
-    context 'when the MR author is in the top contributors' do
-      it 'does not include the MR author' do
-        approvers = calculate_approvers(2)
-
-        expect(approvers).not_to include(author)
-      end
-
-      it 'returns the correct number of contributors' do
-        approvers = calculate_approvers(2)
-
-        expect(approvers.length).to eq(2)
-      end
-    end
-
-    context 'when there are fewer contributors than requested' do
-      it 'returns the full number of users' do
-        approvers = calculate_approvers(5)
-
-        expect(approvers.length).to eq(2)
-      end
-    end
-
-    context 'when there are more contributors than requested' do
-      it 'returns only the top n contributors' do
-        approvers = calculate_approvers(1)
-
-        expect(approvers).to contain_exactly(user_a)
-      end
-    end
-  end
-end

spec/mailers/notify_spec.rb

@@ -366,6 +366,46 @@ describe Notify do
           end
         end
 
+        describe 'that are approved' do
+          let(:last_approver) { create(:user) }
+          subject { Notify.approved_merge_request_email(recipient.id, merge_request.id, last_approver.id) }
+
+          before do
+            merge_request.approvals.create(user: merge_request.assignee)
+            merge_request.approvals.create(user: last_approver)
+          end
+
+          it_behaves_like 'a multiple recipients email'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
+          it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like 'an unsubscribeable thread'
+
+          it 'is sent as the last approver' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(last_approver.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{merge_request.title} \(#{merge_request.to_reference}\)/
+          end
+
+          it 'contains the new status' do
+            is_expected.to have_body_text /approved/i
+          end
+
+          it 'contains a link to the merge request' do
+            is_expected.to have_body_text /#{namespace_project_merge_request_path project.namespace, project, merge_request}/
+          end
+
+          it 'contains the names of all of the approvers' do
+            is_expected.to have_body_text /#{merge_request.assignee.name}/
+            is_expected.to have_body_text /#{last_approver.name}/
+          end
+        end
+
         describe 'that are merged' do
           subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
 

spec/models/merge_request_spec.rb

@@ -223,7 +223,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "#approvers_left" do
+  describe "approvers_left" do
     let(:merge_request) {create :merge_request}
 
     it "returns correct value" do
@@ -238,63 +238,20 @@ 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
-    end
-  end
-
-  describe "#can_approve?" do
-    let(:author) { create(:user) }
-    let(:user) { create(:user) }
-    let(:merge_request) { create(:merge_request, author: author) }
-
-    context "when the user is the MR author" do
-      it "returns false" do
-        expect(merge_request.can_approve?(author)).to eq(false)
+      it "uses the approvals_before_merge from the MR" do
+        expect(merge_request.approvals_required).to eq(1)
       end
     end
 
-    context "when the user is not the MR author" do
-      context "when the user is in the approvers list" do
-        before { merge_request.approvers.create(user: user) }
-
-        context "when the user has not already approved the MR" do
-          it "returns true" do
-            expect(merge_request.can_approve?(user)).to eq(true)
-          end
-        end
-
-        context "when the user has already approved the MR" do
-          before { merge_request.approvals.create(user: user) }
-
-          it "returns false" do
-            expect(merge_request.can_approve?(user)).to eq(false)
-          end
-        end
-      end
-
-      context "when the user is not in the approvers list" do
-        context "when anyone is allowed to approve the MR" do
-          before { merge_request.target_project.update_attributes(approvals_before_merge: 1) }
-
-          context "when the user has not already approved the MR" do
-            it "returns true" do
-              expect(merge_request.can_approve?(user)).to eq(true)
-            end
-          end
-
-          context "when the user has already approved the MR" do
-            before { merge_request.approvals.create(user: user) }
-
-            it "returns false" do
-              expect(merge_request.can_approve?(user)).to eq(false)
-            end
-          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
@@ -634,24 +695,14 @@ describe API::API, api: true  do
   end
 
   describe 'POST :id/merge_requests/:merge_request_id/approve' do
-    context 'when the user is an allowed approver' do
-      it 'approves the merge request' do
-        project.update_attribute(:approvals_before_merge, 2)
-
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", admin)
-
-        expect(response.status).to eq(201)
-        expect(json_response['approvals_left']).to eq(1)
-        expect(json_response['approved_by'][0]['user']['username']).to eq(admin.username)
-      end
-    end
+    it 'approves the merge request' do
+      project.update_attribute(:approvals_before_merge, 2)
 
-    context 'when the user is the MR author' do
-      it 'returns a not authorised response' do
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
 
-        expect(response.status).to eq(401)
-      end
+      expect(response.status).to eq(201)
+      expect(json_response['approvals_left']).to eq(1)
+      expect(json_response['approved_by'][0]['user']['username']).to eq(user.username)
     end
   end
 

spec/services/merge_requests/approval_service_spec.rb

@@ -47,15 +47,36 @@ describe MergeRequests::ApprovalService, services: true do
 
           service.execute(merge_request)
         end
+
+        it 'does not send an email' do
+          expect(merge_request).to receive(:approvals_left).and_return(5)
+          expect(service).not_to receive(:notification_service)
+
+          service.execute(merge_request)
+        end
       end
 
       context 'with required approvals' do
-        it 'fires a webhook' do
+        let(:notification_service) { NotificationService.new }
+
+        before do
           expect(merge_request).to receive(:approvals_left).and_return(0)
+          allow(service).to receive(:notification_service).and_return(notification_service)
+          allow(service).to receive(:execute_hooks)
+          allow(notification_service).to receive(:approve_mr)
+        end
+
+        it 'fires a webhook' do
           expect(service).to receive(:execute_hooks).with(merge_request, 'approved')
 
           service.execute(merge_request)
         end
+
+        it 'sends an email' do
+          expect(notification_service).to receive(:approve_mr).with(merge_request, user)
+
+          service.execute(merge_request)
+        end
       end
     end
   end

spec/services/merge_requests/rebase_service_spec.rb

@@ -26,6 +26,11 @@ describe MergeRequests::RebaseService do
         target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
         expect(parent_sha).to eq(target_branch_sha)
       end
+
+      it 'records the new SHA on the merge request' do
+        head_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
+        expect(merge_request.reload.rebase_commit_sha).to eq(head_sha)
+      end
     end
   end
 end

spec/services/merge_requests/refresh_service_spec.rb

@@ -173,24 +173,59 @@ describe MergeRequests::RefreshService, services: true do
     end
 
     context 'resetting approvals if they are enabled' do
-      it "does not reset approvals if approvals_before_merge is disabled" do
-        @project.update(approvals_before_merge: 0)
-        refresh_service = service.new(@project, @user)
-        allow(refresh_service).to receive(:execute_hooks)
-        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-        reload_mrs
+      context 'when approvals_before_merge is disabled' do
+        before do
+          @project.update(approvals_before_merge: 0)
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'does not reset approvals' do
+          expect(@merge_request.approvals).not_to be_empty
+        end
+      end
 
-        expect(@merge_request.approvals).not_to be_empty
+      context 'when approvals_before_merge is disabled' do
+        before do
+          @project.update(reset_approvals_on_push: false)
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'does not reset approvals' do
+          expect(@merge_request.approvals).not_to be_empty
+        end
       end
 
-      it "does not reset approvals if reset_approvals_on_push is disabled" do
-        @project.update(reset_approvals_on_push: false)
-        refresh_service = service.new(@project, @user)
-        allow(refresh_service).to receive(:execute_hooks)
-        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-        reload_mrs
+      context 'when the rebase_commit_sha on the MR matches the pushed SHA' do
+        before do
+          @merge_request.update(rebase_commit_sha: @newrev)
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'does not reset approvals' do
+          expect(@merge_request.approvals).not_to be_empty
+        end
+      end
 
-        expect(@merge_request.approvals).not_to be_empty
+      context 'when there are approvals to be reset' do
+        before do
+          refresh_service = service.new(@project, @user)
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'resets the approvals' do
+          expect(@merge_request.approvals).to be_empty
+        end
       end
     end