Group Approvers

CHANGELOG-EE.md

@@ -8,6 +8,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Decrease maximum time that GitLab waits for a mirror to finish !791 (Borja Aparicio)
 
 ## 8.12.5
+  - User groups (that can be assigned as approvers)
 
   - No EE-specific changes
 

app/assets/javascripts/approvals.js

 (function() {
   $(function() {
-    $(".approver-list").on("click", ".project-approvers .btn-remove", function() {
+    $(".approver-list").on("click", ".unsaved-approvers.approver .btn-remove", function() {
       var removeElement = $(this).closest("li");
       var approverId = parseInt(removeElement.attr("id").replace("user_",""));
       var approverIds = $("input#merge_request_approver_ids");
@@ -15,17 +15,45 @@
 
       return false;
     });
+
+    $(".approver-list").on("click", ".unsaved-approvers.approver-group .btn-remove", function() {
+      var removeElement = $(this).closest("li");
+      var approverGroupId = parseInt(removeElement.attr("id").replace("group_",""));
+      var approverGroupIds = $("input#merge_request_approver_group_ids");
+      var skipGroups = approverGroupIds.data("skip-groups") || [];
+      var approverGroupIndex = skipGroups.indexOf(approverGroupId);
+
+      removeElement.remove();
+
+      if(approverGroupIndex > -1) {
+        approverGroupIds.data("skip-groups", skipGroups.splice(approverGroupIndex, 1));
+      }
+
+      return false;
+    });
+
     $("form.merge-request-form").submit(function() {
-      var approver_ids, approvers_input;
+      var approverIds, approversInput, approverGroupIds, approverGroupsInput;
+
       if ($("input#merge_request_approver_ids").length) {
-        approver_ids = $.map($("li.project-approvers").not(".approver-template"), function(li, i) {
+        approverIds = $.map($("li.unsaved-approvers.approver").not(".approver-template"), function(li, i) {
           return li.id.replace("user_", "");
         });
-        approvers_input = $(this).find("input#merge_request_approver_ids");
-        approver_ids = approver_ids.concat(approvers_input.val().split(","));
-        return approvers_input.val(_.compact(approver_ids).join(","));
+        approversInput = $(this).find("input#merge_request_approver_ids");
+        approverIds = approverIds.concat(approversInput.val().split(","));
+        approversInput.val(_.compact(approverIds).join(","));
+      }
+
+      if ($("input#merge_request_approver_group_ids").length) {
+        approverGroupIds = $.map($("li.unsaved-approvers.approver-group"), function(li, i) {
+          return li.id.replace("group_", "");
+        });
+        approverGroupsInput = $(this).find("input#merge_request_approver_group_ids");
+        approverGroupIds = approverGroupIds.concat(approverGroupsInput.val().split(","));
+        approverGroupsInput.val(_.compact(approverGroupIds).join(","));
       }
     });
+
     return $(".suggested-approvers a").click(function() {
       var approver_item_html, user_id, user_name;
       user_id = this.id.replace("user_", "");
@@ -33,7 +61,7 @@
       if ($(".approver-list #user_" + user_id).length) {
         return false;
       }
-      approver_item_html = $(".project-approvers.approver-template").clone().removeClass("hide approver-template")[0].outerHTML.replace(/\{approver_name\}/g, user_name).replace(/\{user_id\}/g, user_id);
+      approver_item_html = $(".unsaved-approvers.approver-template").clone().removeClass("hide approver-template")[0].outerHTML.replace(/\{approver_name\}/g, user_name).replace(/\{user_id\}/g, user_id);
       $(".no-approvers").remove();
       $(".approver-list").append(approver_item_html);
       return false;

app/assets/javascripts/issuable_form.js

@@ -15,6 +15,7 @@
       this.handleSubmit = bind(this.handleSubmit, this);
       GitLab.GfmAutoComplete.setup();
       new UsersSelect();
+      new GroupsSelect();
       new ZenMode();
       this.titleField = this.form.find("input[name*='[title]']");
       this.descriptionField = this.form.find("textarea[name*='[description]']");

app/controllers/groups_controller.rb

@@ -93,7 +93,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def autocomplete
-    groups = Group.search(params[:search]).limit(params[:per_page])
+    groups = Group.search(params[:search]).where.not(path: params[:skip_groups]).limit(params[:per_page])
 
     render json: groups.to_json
   end

app/controllers/projects/approver_groups_controller.rb

+class Projects::ApproverGroupsController < Projects::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.approver_groups.find(params[:id]).destroy
+    else
+      authorize_admin_project!
+      project.approver_groups.find(params[:id]).destroy
+    end
+
+    redirect_back_or_default(default: { action: 'index' })
+  end
+end

app/controllers/projects/merge_requests_controller.rb

@@ -588,7 +588,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, :lock_version, label_ids: []
+      :approvals_before_merge, :lock_version, :approver_group_ids, label_ids: []
     )
   end
 

app/controllers/projects_controller.rb

@@ -325,6 +325,7 @@ class ProjectsController < Projects::ApplicationController
       # EE-only
       :approvals_before_merge,
       :approver_ids,
+      :approver_group_ids,
       :issues_template,
       :merge_method,
       :merge_requests_template,

app/helpers/selects_helper.rb

@@ -34,6 +34,7 @@ module SelectsHelper
   def groups_select_tag(id, opts = {})
     opts[:class] ||= ''
     opts[:class] << ' ajax-groups-select'
+    opts[:class] << ' multiselect' if opts[:multiple]
     select2_tag(id, opts)
   end
 
@@ -61,7 +62,7 @@ module SelectsHelper
     value = opts[:selected] || ''
     css_class = opts[:class]
 
-    hidden_field_tag(id, value, class: css_class, data: { skip_group: opts[:skip_group], url: autocomplete_groups_path })
+    hidden_field_tag(id, value, class: css_class, data: { skip_groups: opts[:skip_groups], url: autocomplete_groups_path })
   end
 
   def admin_email_select_tag(id, opts = {})

app/models/approver_group.rb

+class ApproverGroup < ActiveRecord::Base
+  belongs_to :target, polymorphic: true
+  belongs_to :group
+
+  validates :group, presence: true
+
+  delegate :users, to: :group
+end

app/models/concerns/approvable.rb

+module Approvable
+  extend ActiveSupport::Concern
+
+  included do
+    def requires_approve?
+      approvals_required.nonzero?
+    end
+
+    def approved?
+      approvals_left < 1
+    end
+
+    # Number of approvals remaining (excluding existing approvals) before the MR is
+    # considered approved. If there are fewer potential approvers than approvals left,
+    # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
+    #
+    def approvals_left
+      [
+        [approvals_required - approvals.count, number_of_potential_approvers].min,
+        0
+      ].max
+    end
+
+    def approvals_required
+      approvals_before_merge || target_project.approvals_before_merge
+    end
+
+    # An MR can potentially be approved by:
+    # - anyone in the approvers list
+    # - any other project member with developer access or higher (if there are no approvers
+    #   left)
+    #
+    # It cannot be approved by:
+    # - a user who has already approved the MR
+    # - the MR author
+    #
+    def number_of_potential_approvers
+      has_access = ['access_level > ?', Member::REPORTER]
+      wheres = [
+        "id IN (#{project.members.where(has_access).select(:user_id).to_sql})"
+      ]
+
+      all_approvers = all_approvers_including_groups
+
+      if all_approvers.any?
+        wheres << "id IN (#{all_approvers.map(&:id).join(', ')})"
+      end
+
+      if project.group
+        wheres << "id IN (#{project.group.members.where(has_access).select(:user_id).to_sql})"
+      end
+
+      User.
+        active.
+        where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql})").
+        where.not(id: author.id).
+        count
+    end
+
+    # Users in the list of approvers who have not already approved this MR.
+    #
+    def approvers_left
+      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id))
+    end
+
+    # The list of approvers from either this MR (if they've been set on the MR) or the
+    # target project. Excludes the author by default.
+    #
+    # Before a merge request has been created, author will be nil, so pass the current user
+    # on the MR create page.
+    #
+    def overall_approvers
+      approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
+
+      approvers_relation = approvers_relation.where.not(user_id: author.id) if author
+
+      approvers_relation
+    end
+
+    def overall_approver_groups
+      approvers_overwritten? ? approver_groups : target_project.approver_groups
+    end
+
+    def all_approvers_including_groups
+      approvers = []
+
+      # Approvers from direct assignment
+      approvers << approvers_from_users
+
+      approvers << approvers_from_groups
+
+      approvers.flatten
+    end
+
+    def approvers_from_users
+      overall_approvers.map(&:user)
+    end
+
+    def approvers_from_groups
+      group_approvers = []
+
+      overall_approver_groups.each do |approver_group|
+        group_approvers << approver_group.users
+      end
+
+      group_approvers.flatten!
+
+      group_approvers.delete(author)
+
+      group_approvers
+    end
+
+    def approvers_overwritten?
+      approvers.any? || approver_groups.any?
+    end
+
+    def can_approve?(user)
+      return false unless user
+      return true if approvers_left.include?(user)
+      return false if user == author
+      return false unless user.can?(:update_merge_request, self)
+
+      any_approver_allowed? && approvals.where(user: user).empty?
+    end
+
+    # Once there are fewer approvers left in the list than approvals required, allow other
+    # project members to approve the MR.
+    #
+    def any_approver_allowed?
+      approvals_left > approvers_left.count
+    end
+
+    def approved_by_users
+      approvals.map(&:user)
+    end
+
+    def approver_ids=(value)
+      value.split(",").map(&:strip).each do |user_id|
+        next if author && user_id == author.id
+
+        approvers.find_or_initialize_by(user_id: user_id, target_id: id)
+      end
+    end
+
+    def approver_group_ids=(value)
+      value.split(",").map(&:strip).each do |group_id|
+        approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
+      end
+    end
+  end
+end

app/models/merge_request.rb

@@ -6,6 +6,7 @@ class MergeRequest < ActiveRecord::Base
   include Taskable
   include Elastic::MergeRequestsSearch
   include Importable
+  include Approvable
 
   belongs_to :target_project, foreign_key: :target_project_id, class_name: "Project"
   belongs_to :source_project, foreign_key: :source_project_id, class_name: "Project"
@@ -13,6 +14,7 @@ class MergeRequest < ActiveRecord::Base
 
   has_many :approvals, dependent: :destroy
   has_many :approvers, as: :target, dependent: :destroy
+  has_many :approver_groups, as: :target, dependent: :destroy
   has_many :merge_request_diffs, dependent: :destroy
   has_one :merge_request_diff,
     -> { order('merge_request_diffs.id DESC') }
@@ -678,102 +680,6 @@ class MergeRequest < ActiveRecord::Base
     locked_at.nil? || locked_at < (Time.now - 1.day)
   end
 
-  def requires_approve?
-    approvals_required.nonzero?
-  end
-
-  def approved?
-    approvals_left < 1
-  end
-
-  # Number of approvals remaining (excluding existing approvals) before the MR is
-  # considered approved. If there are fewer potential approvers than approvals left,
-  # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
-  #
-  def approvals_left
-    [
-      [approvals_required - approvals.count, number_of_potential_approvers].min,
-      0
-    ].max
-  end
-
-  def approvals_required
-    approvals_before_merge || target_project.approvals_before_merge
-  end
-
-  # An MR can potentially be approved by:
-  # - anyone in the approvers list
-  # - any other project member with developer access or higher (if there are no approvers
-  #   left)
-  #
-  # It cannot be approved by:
-  # - a user who has already approved the MR
-  # - the MR author
-  #
-  def number_of_potential_approvers
-    has_access = ['access_level > ?', Member::REPORTER]
-    wheres = [
-      "id IN (#{overall_approvers.select(:user_id).to_sql})",
-      "id IN (#{project.members.where(has_access).select(:user_id).to_sql})"
-    ]
-
-    if project.group
-      wheres << "id IN (#{project.group.members.where(has_access).select(:user_id).to_sql})"
-    end
-
-    User.
-      active.
-      where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql}) AND id != #{author.id}").
-      count
-  end
-
-  # Users in the list of approvers who have not already approved this MR.
-  #
-  def approvers_left
-    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
-  end
-
-  # The list of approvers from either this MR (if they've been set on the MR) or the
-  # target project. Excludes the author by default.
-  #
-  # Before a merge request has been created, author will be nil, so pass the current user
-  # on the MR create page.
-  #
-  def overall_approvers(exclude_user: nil)
-    exclude_user ||= author
-    approvers_relation = approvers.any? ? approvers : target_project.approvers
-
-    exclude_user ? approvers_relation.where.not(user_id: exclude_user.id) : approvers_relation
-  end
-
-  def can_approve?(user)
-    return false unless user
-    return true if approvers_left.include?(user)
-    return false if user == author
-    return false unless user.can?(:update_merge_request, self)
-
-    any_approver_allowed? && approvals.where(user: user).empty?
-  end
-
-  # Once there are fewer approvers left in the list than approvals required, allow other
-  # project members to approve the MR.
-  #
-  def any_approver_allowed?
-    approvals_left > approvers_left.count
-  end
-
-  def approved_by_users
-    approvals.map(&:user)
-  end
-
-  def approver_ids=(value)
-    value.split(",").map(&:strip).each do |user_id|
-      next if author && user_id == author.id
-
-      approvers.find_or_initialize_by(user_id: user_id, target_id: id)
-    end
-  end
-
   def has_ci?
     source_project.ci_service && commits.any?
   end

app/models/project.rb

@@ -122,6 +122,7 @@ class Project < ActiveRecord::Base
   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 :approver_groups, as: :target, dependent: :destroy
   has_many :releases, dependent: :destroy
   has_many :lfs_objects_projects, dependent: :destroy
   has_many :lfs_objects, through: :lfs_objects_projects
@@ -1228,6 +1229,12 @@ class Project < ActiveRecord::Base
     end
   end
 
+  def approver_group_ids=(value)
+    value.split(",").map(&:strip).each do |group_id|
+      approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
+    end
+  end
+
   def allowed_to_share_with_group?
     !namespace.share_with_group_lock
   end

app/views/projects/_merge_request_settings.html.haml

@@ -61,21 +61,33 @@
     = users_select_tag("project[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
     .help-block
       Add an approver suggestion for each merge request
+    = f.label :approver_group_ids, class: 'label-light' do
+      Approver groups
+    - project_approver_group_paths = @project.approver_groups.includes(:group).map { |ag| ag.group.path }
+    = groups_select_tag('project[approver_group_ids]', multiple: true, skip_groups: project_approver_group_paths, class: 'input-large')
+    .help-block
+      Add a group as an approver suggestion for each merge request
 
     .panel.panel-default.prepend-top-10
       .panel-heading
         Approvers
-        %small
-          (#{@project.approvers.count(:all)})
-      %ul.well-list
+      %ul.well-list.approver-list
         - @project.approvers.each do |approver|
-          %li
+          %li.approver
             = 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?
+        - @project.approver_groups.each do |approver_group|
+          %li.approver-group
+            Group:
+            = link_to approver_group.group.name, approver_group.group
+            .pull-right
+              = link_to namespace_project_approver_group_path(@project.namespace, @project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
+                = icon("sign-out")
+                Remove
+        - if @project.approvers.empty? && @project.approver_groups.empty?
           %li There are no approvers
 
   .form-group.builds-feature
@@ -89,3 +101,4 @@
 
 :javascript
   new UsersSelect();
+  new GroupsSelect();

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

+- return unless issuable.is_a?(MergeRequest)
+- return unless issuable.requires_approve?
+- approvals_required = 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_required
+    .help-block
+      Number of users who need to approve this merge request before it can be accepted.
+      If this isn't greater than the project default (#{pluralize(approvals_required, '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
+    - author = issuable.author || current_user
+    - skip_users = issuable.all_approvers_including_groups + [author]
+    = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true, skip_users: skip_users)
+    .help-block
+      This merge request must be approved by these users.
+      You can override the project settings by setting your own list of approvers.
+
+    - approver_group_ids = issuable.overall_approver_groups.includes(:group).map { |ag| ag.group.id }
+    = groups_select_tag('merge_request[approver_group_ids]', multiple: true, skip_groups: approver_group_ids, class: 'input-large')
+    .help-block
+      This merge request must be approved by members of these groups.
+      You can override the project settings by setting your own list of approvers.
+
+    .panel.panel-default.prepend-top-10
+      .panel-heading
+        Approvers
+      %ul.well-list.approver-list
+        - if issuable.all_approvers_including_groups.empty?
+          %li.no-approvers There are no approvers
+        - else
+          - unsaved_approvers = !issuable.approvers_overwritten?
+          - item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
+          - issuable.overall_approvers.each do |approver|
+            %li{id: dom_id(approver.user), class: item_classes + ['approver']}
+              = link_to approver.user.name, approver.user
+              .pull-right
+                - if unsaved_approvers
+                  = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                    = icon("sign-out")
+                    Remove
+                - else
+                  = link_to namespace_project_merge_request_approver_path(@project.namespace, @project, issuable, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                    = icon("sign-out")
+                    Remove
+          - issuable.overall_approver_groups.each do |approver_group|
+            %li{id: dom_id(approver_group.group), class: item_classes + ['approver-group']}
+              Group:
+              = link_to approver_group.group.name, approver_group.group
+              .pull-right
+                - if unsaved_approvers
+                  = link_to "#", data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, class: "btn-xs btn btn-remove", title: 'Remove group' do
+                    = icon("sign-out")
+                    Remove
+                - else
+                  = link_to namespace_project_merge_request_approver_group_path(@project.namespace, @project, issuable, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove group' do
+                    = icon("sign-out")
+                    Remove
+    .help-block.suggested-approvers
+      - if @suggested_approvers.any?
+        Suggested approvers:
+        = raw @suggested_approvers.map { |approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")

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

@@ -128,53 +128,7 @@
       title: 'Moving an issue will copy the discussion to a different project and close it here. All participants will be notified of the new location.' }
         = icon('question-circle')
 
-- 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
-        - author = @merge_request.author || current_user
-        - skip_users = @merge_request.overall_approvers.map(&:user) + [author]
-        = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true, skip_users: skip_users)
-        .help-block
-          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
-          .panel-heading
-            Approvers
-          %ul.well-list.approver-list
-            - using_project_approvers = @merge_request.approvers.empty?
-            - item_class = 'project-approvers' if using_project_approvers
-            - @merge_request.overall_approvers(exclude_user: author).each do |approver|
-              %li{id: dom_id(approver.user), class: item_class}
-                = link_to approver.user.name, approver.user
-                .pull-right
-                  - if using_project_approvers
-                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
-                      = icon("sign-out")
-                      Remove
-                  - else
-                    = link_to 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.overall_approvers.empty?
-              %li.no-approvers There are no approvers
-        .help-block.suggested-approvers
-          - if @suggested_approvers.any?
-            Suggested approvers:
-            = raw @suggested_approvers.map{|approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")
+= render 'shared/issuable/approvals', issuable: issuable, f: f
 
 - if issuable.is_a?(MergeRequest) && !issuable.closed_without_fork?
   %hr
@@ -222,7 +176,7 @@
                                                                                                   method: :delete, class: 'btn btn-danger btn-grouped'
       = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
 
-%li.project-approvers.hide.approver-template{id: "user_{user_id}"}
+%li.unsaved-approvers.hide.approver.approver-template{id: "user_{user_id}"}
   = link_to "{approver_name}", "#"
   .pull-right
     = link_to "#", data: { confirm: "Are you sure you want to remove approver {approver_name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do

config/routes/project.rb

@@ -303,6 +303,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
 
         ## EE-specific
         resources :approvers, only: :destroy
+        resources :approver_groups, only: :destroy
         ## EE-specific
 
         resources :discussions, only: [], constraints: { id: /\h{40}/ } do
@@ -491,6 +492,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
 
       ## EE-specific
       resources :approvers, only: :destroy
+      resources :approver_groups, only: :destroy
       ## EE-specific
 
       resources :runner_projects, only: [:create, :destroy]

db/migrate/20160916101334_add_approver_groups.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddApproverGroups < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = true
+
+  DOWNTIME_REASON = 'Adding foreign key'
+
+  def change
+    create_table :approver_groups do |t|
+      t.integer :target_id, null: false
+      t.string :target_type, null: false
+      t.integer :group_id, null: false
+
+      t.timestamps
+
+      t.index [:target_id, :target_type]
+      t.index :group_id
+    end
+
+    add_foreign_key :approver_groups, :namespaces, column: :group_id, on_delete: :cascade
+  end
+end

db/schema.rb

@@ -116,6 +116,17 @@ ActiveRecord::Schema.define(version: 20161007133303) do
     t.datetime "updated_at"
   end
 
+  create_table "approver_groups", force: :cascade do |t|
+    t.integer  "target_id",   null: false
+    t.string   "target_type", null: false
+    t.integer  "group_id",    null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
+  add_index "approver_groups", ["group_id"], name: "index_approver_groups_on_group_id", using: :btree
+  add_index "approver_groups", ["target_id", "target_type"], name: "index_approver_groups_on_target_id_and_target_type", using: :btree
+
   create_table "approvers", force: :cascade do |t|
     t.integer "target_id", null: false
     t.string "target_type"
@@ -1375,6 +1386,7 @@ ActiveRecord::Schema.define(version: 20161007133303) do
 
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
 
+  add_foreign_key "approver_groups", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "boards", "projects"
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
   add_foreign_key "lists", "boards"

doc/user/project/merge_requests/merge_request_approvals.md

@@ -58,6 +58,10 @@ creating or editing a merge request.
 When someone is marked as a required approver for a merge request, an email is
 sent to them and a todo is added to their list of todos.
 
+## Groups
+
+You can also assign one or more groups that can be assigned as approvers, it works in the same way like regular approvers, the only difference is that you assign several users with one action. It's also possible to assign group at the project level and you can always change them later by editing the merge request.
+
 ## Using approvals
 
 After configuring approvals, you will be able to change the default set of

features/steps/project/merge_requests.rb

@@ -542,7 +542,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I see suggested approver' do
-    page.within 'ul .project-approvers' do
+    page.within 'ul .unsaved-approvers' do
       expect(page).to have_content(current_user.name)
     end
   end

spec/controllers/projects/approver_groups_controller_spec.rb

+require 'rails_helper'
+
+describe Projects::ApproverGroupsController do
+  describe '#destroy' do
+    before do
+      # Allow redirect_back_or_default to work
+      request.env['HTTP_REFERER'] = '/'
+    end
+
+    context 'on a merge request' do
+      it 'authorizes create_merge_request' do
+        merge    = create(:merge_request)
+        project  = stub_project(merge.target_project)
+        approver = create(:approver, target: merge)
+
+        expect(controller).to receive(:authorize_create_merge_request!)
+
+        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
+      end
+
+      it 'destroys the provided approver group' do
+        merge          = create(:merge_request)
+        project        = stub_project(merge.target_project)
+        approver_group = create(:approver_group, target: merge)
+
+        allow(controller).to receive(:authorize_create_merge_request!)
+
+        expect { go_delete(project, merge_request_id: merge.to_param, id: approver_group.id) }.
+          to change { merge.reload.approver_groups.count }.by(-1)
+      end
+    end
+
+    context 'on a project' do
+      it 'authorizes admin_project' do
+        project        = stub_project
+        approver_group = create(:approver_group, target: project)
+
+        expect(controller).to receive(:authorize_admin_project!)
+
+        go_delete(project, id: approver_group.id)
+      end
+
+      it 'destroys the provided approver' do
+        project        = stub_project
+        approver_group = create(:approver_group, target: project)
+
+        allow(controller).to receive(:authorize_admin_project!).and_return(true)
+
+        expect { go_delete(project, id: approver_group.id) }.
+          to change { project.approver_groups.count }.by(-1)
+      end
+    end
+
+    def go_delete(project, params = {})
+      delete :destroy, {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param
+      }.merge(params)
+    end
+
+    def stub_project(project = build_stubbed(:empty_project))
+      project.tap do |p|
+        allow(controller).to receive(:project).and_return(p)
+      end
+    end
+  end
+end

spec/controllers/projects/approvers_controller_spec.rb

@@ -15,7 +15,7 @@ describe Projects::ApproversController do
 
         expect(controller).to receive(:authorize_create_merge_request!)
 
-        go(project, merge_request_id: merge.to_param, id: approver.id)
+        go_delete(project, merge_request_id: merge.to_param, id: approver.id)
       end
 
       it 'destroys the provided approver' do
@@ -25,7 +25,7 @@ describe Projects::ApproversController do
 
         allow(controller).to receive(:authorize_create_merge_request!)
 
-        expect { go(project, merge_request_id: merge.to_param, id: approver.id) }.
+        expect { go_delete(project, merge_request_id: merge.to_param, id: approver.id) }.
           to change { merge.reload.approvers.count }.by(-1)
       end
     end
@@ -37,7 +37,7 @@ describe Projects::ApproversController do
 
         expect(controller).to receive(:authorize_admin_project!)
 
-        go(project, id: approver.id)
+        go_delete(project, id: approver.id)
       end
 
       it 'destroys the provided approver' do
@@ -46,12 +46,12 @@ describe Projects::ApproversController do
 
         allow(controller).to receive(:authorize_admin_project!).and_return(true)
 
-        expect { go(project, id: approver.id) }.
+        expect { go_delete(project, id: approver.id) }.
           to change { project.approvers.count }.by(-1)
       end
     end
 
-    def go(project, params = {})
+    def go_delete(project, params = {})
       delete :destroy, {
         namespace_id: project.namespace.to_param,
         project_id: project.to_param

spec/factories/approver_groups.rb

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

spec/factories/groups.rb

@@ -16,6 +16,12 @@ FactoryGirl.define do
       visibility_level Gitlab::VisibilityLevel::PRIVATE
     end
 
+    factory :group_with_members do
+      after(:create) do |group, evaluator|
+        group.add_developer(create :user)
+      end
+    end
+
     factory :group_with_ldap_group_link do
       transient do
         cn 'group1'

spec/features/merge_requests/approvals_spec.rb

 require 'rails_helper'
 
 feature 'Merge request approvals', js: true, feature: true do
+  include WaitForAjax
+
   let(:user) { create(:user) }
   let(:project) { create(:project, approvals_before_merge: 1) }
 
@@ -48,4 +50,143 @@ feature 'Merge request approvals', js: true, feature: true do
       expect(find('.select2-results')).not_to have_content(user.name)
     end
   end
+
+  context "Group approvers" do
+    context 'when creating an MR' do
+      let(:other_user) { create(:user) }
+
+      before do
+        project.team << [user, :developer]
+        project.team << [other_user, :developer]
+
+        login_as(user)
+      end
+
+      it 'allows setting groups as approvers' do
+        group = create :group
+        group.add_developer(other_user)
+
+        visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature' })
+        find('#s2id_merge_request_approver_group_ids .select2-input').click
+
+        wait_for_ajax
+
+        expect(find('.select2-results')).to have_content(group.name)
+
+        find('.select2-results').click
+        click_on("Submit merge request")
+        expect(page).to have_content("Requires one more approval (from #{other_user.name})")
+      end
+
+      it 'allows delete approvers group when it is set in project' do
+        approver = create :user
+        group = create :group
+        group.add_developer(other_user)
+        create :approver_group, group: group, target: project
+        create :approver, user: approver, target: project
+
+        visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature' })
+
+        within('.approver-list li.approver-group') do
+          click_on "Remove"
+        end
+
+        expect(page).to have_css('.approver-list li', count: 1)
+
+        click_on("Submit merge request")
+        expect(page).not_to have_content("Requires one more approval (from #{other_user.name})")
+      end
+    end
+
+    context 'when editing an MR with a different author' do
+      let(:other_user) { create(:user) }
+      let(:merge_request) { create(:merge_request, source_project: project) }
+
+      before do
+        project.team << [user, :developer]
+
+        login_as(user)
+      end
+
+      it 'allows setting groups as approvers' do
+        group = create :group
+        group.add_developer(other_user)
+        group.add_developer(user)
+
+        visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
+        find('#s2id_merge_request_approver_group_ids .select2-input').click
+
+        wait_for_ajax
+
+        expect(find('.select2-results')).to have_content(group.name)
+
+        find('.select2-results').click
+        click_on("Save changes")
+
+        expect(page).to have_content("Requires one more approval")
+      end
+
+      it 'allows delete approvers group when it`s set in project' do
+        approver = create :user
+        group = create :group
+        group.add_developer(other_user)
+        create :approver_group, group: group, target: project
+        create :approver, user: approver, target: project
+
+        visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+        within('.approver-list li.approver-group') do
+          click_on "Remove"
+        end
+
+        expect(page).to have_css('.approver-list li', count: 1)
+
+        click_on("Save changes")
+        expect(page).to have_content("Requires one more approval (from #{approver.name})")
+      end
+    end
+  end
+
+  context 'Approving by approvers from groups' do
+    let(:other_user) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:group) { create :group }
+
+    before do
+      project.team << [user, :developer]
+
+      group.add_developer(other_user)
+      group.add_developer(user)
+
+      login_as(user)
+    end
+
+    context 'when group is assigned to a project' do
+      it 'I am able to approve' do
+        create :approver_group, group: group, target: project
+
+        visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+        page.within '.mr-state-widget' do
+          click_button 'Approve Merge Request'
+        end
+
+        expect(page).to have_content("Approved by")
+      end
+    end
+
+    context 'when group is assigned to a merge request' do
+      it 'I am able to approve' do
+        create :approver_group, group: group, target: merge_request
+
+        visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+        page.within '.mr-state-widget' do
+          click_button 'Approve Merge Request'
+        end
+
+        expect(page).to have_content("Approved by")
+      end
+    end
+  end
 end

spec/features/projects/project_settings_spec.rb

 require 'spec_helper'
 
 describe 'Edit Project Settings', feature: true do
+  include WaitForAjax
+
   let(:user) { create(:user) }
   let(:project) { create(:empty_project, path: 'gitlab', name: 'sample') }
 
@@ -21,6 +23,45 @@ describe 'Edit Project Settings', feature: true do
       expect(page).to have_content "Name can contain only letters, digits, '_', '.', dash and space. It must start with letter, digit or '_'."
       expect(page).to have_button 'Save changes'
     end
+
+    it 'adds approver group' do
+      group = create :group
+      approver = create :user
+      group.add_developer(approver)
+      group.add_developer(user)
+
+      visit edit_namespace_project_path(project.namespace, project)
+
+      find('#s2id_project_approver_group_ids .select2-input').click
+
+      wait_for_ajax
+
+      expect(find('.select2-results')).to have_content(group.name)
+
+      find('.select2-results').click
+
+      click_button 'Save changes'
+
+      expect(page).to have_css('.approver-list li.approver-group', count: 1)
+    end
+
+    it 'removes approver group' do
+      group = create :group
+      approver = create :user
+      group.add_developer(approver)
+      group.add_developer(user)
+      create :approver_group, group: group, target: project
+
+      visit edit_namespace_project_path(project.namespace, project)
+
+      expect(find('.approver-list')).to have_content(group.name)
+
+      within('.approver-list') do
+        click_on "Remove"
+      end
+
+      expect(find('.approver-list')).not_to have_content(group.name)
+    end
   end
 
   describe 'Rename repository' do

spec/lib/gitlab/import_export/all_models.yml

@@ -78,6 +78,7 @@ merge_requests:
 - metrics
 - approvals
 - approvers
+- approver_groups
 merge_request_diff:
 - merge_request
 pipelines:
@@ -196,6 +197,7 @@ project:
 - audit_events
 - remote_mirrors
 - path_locks
+- approver_groups
 award_emoji:
 - awardable
 - user

spec/models/approver_group_spec.rb

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

spec/models/merge_request_spec.rb

@@ -10,6 +10,7 @@ describe MergeRequest, models: true do
     it { is_expected.to belong_to(:source_project).with_foreign_key(:source_project_id).class_name('Project') }
     it { is_expected.to belong_to(:merge_user).class_name("User") }
     it { is_expected.to have_many(:merge_request_diffs).dependent(:destroy) }
+    it { is_expected.to have_many(:approver_groups).dependent(:destroy) }
   end
 
   describe 'modules' do
@@ -363,6 +364,31 @@ describe MergeRequest, models: true do
 
       expect(merge_request.approvers_left).to eq [user]
     end
+
+    it "returns correct value when there is a group approver" do
+      user = create(:user)
+      user1 = create(:user)
+      user2 = create(:user)
+      group = create(:group)
+
+      group.add_developer(user2)
+      merge_request.approver_groups.create(group: group)
+      merge_request.approvers.create(user_id: user.id)
+      merge_request.approvers.create(user_id: user1.id)
+      merge_request.approvals.create(user_id: user1.id)
+
+      expect(merge_request.approvers_left).to match_array [user, user2]
+    end
+
+    it "returns correct value when there is only a group approver" do
+      user = create(:user)
+      group = create(:group)
+      group.add_developer(user)
+
+      merge_request.approver_groups.create(group: group)
+
+      expect(merge_request.approvers_left).to eq [user]
+    end
   end
 
   describe "#number_of_potential_approvers" do
@@ -376,6 +402,14 @@ describe MergeRequest, models: true do
       end.to change { merge_request.number_of_potential_approvers }.by(1)
     end
 
+    it "includes approvers from group" do
+      group = create(:group_with_members)
+
+      expect do
+        create(:approver_group, group: group, target: merge_request)
+      end.to change { merge_request.number_of_potential_approvers }.by(1)
+    end
+
     it "includes project members with developer access and up" do
       expect do
         project.team << [create(:user), :guest]
@@ -426,6 +460,73 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe "#overall_approver_groups" do
+    it 'returns a merge request group approver' do
+      project = create :empty_project
+      create :approver_group, target: project
+
+      merge_request = create :merge_request, target_project: project, source_project: project
+      approver_group2 = create :approver_group, target: merge_request
+
+      expect(merge_request.overall_approver_groups).to eq([approver_group2])
+    end
+
+    it 'returns a project group approver' do
+      project = create :empty_project
+      approver_group1 = create :approver_group, target: project
+
+      merge_request = create :merge_request, target_project: project, source_project: project
+
+      expect(merge_request.overall_approver_groups).to eq([approver_group1])
+    end
+
+    it 'returns a merge request approver if there is no project group approver' do
+      project = create :empty_project
+
+      merge_request = create :merge_request, target_project: project, source_project: project
+      approver_group1 = create :approver_group, target: merge_request
+
+      expect(merge_request.overall_approver_groups).to eq([approver_group1])
+    end
+  end
+
+  describe '#all_approvers_including_groups' do
+    it 'returns correct set of users' do
+      user = create :user
+      user1 = create :user
+      user2 = create :user
+      create :user
+
+      project = create :empty_project
+      group = create :group
+      group.add_master user
+      create :approver_group, target: project, group: group
+
+      merge_request = create :merge_request, target_project: project, source_project: project
+      group1 = create :group
+      group1.add_master user1
+      create :approver_group, target: merge_request, group: group1
+
+      create(:approver, user: user2, target: merge_request)
+
+      expect(merge_request.all_approvers_including_groups).to match_array([user1, user2])
+    end
+  end
+
+  describe '#approver_group_ids=' do
+    it 'create approver_groups' do
+      group = create :group
+      group1 = create :group
+
+      merge_request = create :merge_request
+
+      merge_request.approver_group_ids = "#{group.id}, #{group1.id}"
+      merge_request.save!
+
+      expect(merge_request.approver_groups.map(&:group)).to match_array([group, group1])
+    end
+  end
+
   describe "#approvals_required" do
     let(:merge_request) { build(:merge_request) }
     before { merge_request.target_project.update_attributes(approvals_before_merge: 3) }

spec/models/project_spec.rb

@@ -68,6 +68,7 @@ describe Project, models: true do
     it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:forks).through(:forked_project_links) }
+    it { is_expected.to have_many(:approver_groups).dependent(:destroy) }
 
     describe '#members & #requesters' do
       let(:project) { create(:project, :public) }
@@ -1904,6 +1905,22 @@ describe Project, models: true do
     end
   end
 
+  describe '#approver_group_ids=' do
+    let(:project) { create(:empty_project) }
+
+    it 'create approver_groups' do
+      group = create :group
+      group1 = create :group
+
+      project = create :project
+
+      project.approver_group_ids = "#{group.id}, #{group1.id}"
+      project.save!
+
+      expect(project.approver_groups.map(&:group)).to match_array([group, group1])
+    end
+  end
+
   describe '#reset_pushes_since_gc' do
     let(:project) { create(:project) }