Merge branch 'group_approvers' into 'master'

User groups (that can be assigned as approvers)

Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/180

See merge request !743

.flayignore

 *.erb
 lib/gitlab/sanitizers/svg/whitelist.rb
 lib/gitlab/diff/position_tracer.rb
+app/controllers/projects/approver_groups_controller.rb
+app/controllers/projects/approvers_controller.rb

CHANGELOG-EE.md

@@ -6,6 +6,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix validations related to mirroring settings form. !773
   - Fix Git access panel for Wikis when Kerberos authentication is enabled (Borja Aparicio)
   - Decrease maximum time that GitLab waits for a mirror to finish !791 (Borja Aparicio)
+  - User groups (that can be assigned as approvers)
 
 ## 8.12.5
 

app/assets/javascripts/admin_email_select.js

@@ -13,7 +13,7 @@
             minimumInputLength: 0,
             query: function(query) {
               var group_result, project_result;
-              group_result = Api.groups(query.term, skip_ldap, function(groups) {
+              group_result = Api.groups(query.term, { skip_ldap: skip_ldap }, function(groups) {
                 return groups;
               });
               project_result = Api.projects(query.term, 'id', function(projects) {

app/assets/javascripts/api.js

@@ -23,16 +23,14 @@
       });
     },
     // Return groups list. Filtered by query
-    // Only active groups retrieved
-    groups: function(query, skip_ldap, skip_groups, callback) {
+    groups: function(query, options, callback) {
       var url = Api.buildUrl(Api.groupsPath);
       return $.ajax({
         url: url,
-        data: {
+        data: $.extend({
                 search: query,
-          skip_groups: skip_groups,
                 per_page: 20
-        },
+              }, options),
         dataType: "json"
       }).done(function(groups) {
         return callback(groups);

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(ev) {
       var removeElement = $(this).closest("li");
-      var approverId = parseInt(removeElement.attr("id").replace("user_",""));
+      var approverId = parseInt(removeElement.attr("id").replace("user_",""), 10);
       var approverIds = $("input#merge_request_approver_ids");
       var skipUsers = approverIds.data("skip-users") || [];
       var approverIndex = skipUsers.indexOf(approverId);
@@ -13,19 +13,47 @@
         approverIds.data("skip-users", skipUsers.splice(approverIndex, 1));
       }
 
-      return false;
+      ev.preventDefault();
     });
+
+    $(".approver-list").on("click", ".unsaved-approvers.approver-group .btn-remove", function(ev) {
+      var removeElement = $(this).closest("li");
+      var approverGroupId = parseInt(removeElement.attr("id").replace("group_",""), 10);
+      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));
+      }
+
+      ev.preventDefault();
+    });
+
     $("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/groups_select.js

@@ -5,15 +5,17 @@
     function GroupsSelect() {
       $('.ajax-groups-select').each((function(_this) {
         return function(i, select) {
-          var skip_ldap, skip_groups;
+          var skip_ldap, all_available, skip_groups;
           skip_ldap = $(select).hasClass('skip_ldap');
+          all_available = $(select).data('all-available');
           skip_groups = $(select).data('skip-groups') || [];
           return $(select).select2({
             placeholder: "Search for a group",
             multiple: $(select).hasClass('multiselect'),
             minimumInputLength: 0,
             query: function(query) {
-              return Api.groups(query.term, skip_ldap, skip_groups, function(groups) {
+              options = { all_available: all_available, skip_groups: skip_groups }
+              return Api.groups(query.term, options, function(groups) {
                 var data;
                 data = {
                   results: groups

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/assets/javascripts/project_select.js

@@ -23,7 +23,7 @@
                   data = groups.concat(projects);
                   return finalCallback(data);
                 };
-                return Api.groups(term, false, false, groupsCallback);
+                return Api.groups(term, {}, groupsCallback);
               };
             } else {
               projectsCallback = finalCallback;
@@ -72,7 +72,7 @@
                     data = groups.concat(projects);
                     return finalCallback(data);
                   };
-                  return Api.groups(query.term, false, false, groupsCallback);
+                  return Api.groups(query.term, {}, groupsCallback);
                 };
               } else {
                 projectsCallback = finalCallback;

app/assets/javascripts/search.js

@@ -10,7 +10,7 @@
         filterable: true,
         fieldName: 'group_id',
         data: function(term, callback) {
-          return Api.groups(term, false, false, function(data) {
+          return Api.groups(term, {}, function(data) {
             data.unshift({
               name: 'Any'
             });

app/controllers/groups_controller.rb

@@ -6,7 +6,7 @@ class GroupsController < Groups::ApplicationController
   respond_to :html
 
   before_action :authenticate_user!, only: [:new, :create]
-  before_action :group, except: [:index, :new, :create, :autocomplete]
+  before_action :group, except: [:index, :new, :create]
 
   # Authorize
   before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
@@ -92,12 +92,6 @@ class GroupsController < Groups::ApplicationController
     redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
   end
 
-  def autocomplete
-    groups = Group.search(params[:search]).limit(params[:per_page])
-
-    render json: groups.to_json
-  end
-
   protected
 
   def setup_projects

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
 
@@ -59,9 +60,7 @@ module SelectsHelper
   def select2_tag(id, opts = {})
     opts[:class] << ' multiselect' if opts[:multiple]
     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, opts)
   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
+    - skip_groups = @project.approver_groups.pluck(:group_id)
+    = groups_select_tag('project[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true }, 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.
+
+    - skip_groups = issuable.overall_approver_groups.pluck(:group_id)
+    = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true }, 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/group.rb

@@ -14,12 +14,6 @@ resources :groups, constraints: { id: /[a-zA-Z.0-9_\-]+(?<!\.atom)/ }  do
     get :activity
   end
 
-  ## EE-specific
-  collection do
-    get :autocomplete
-  end
-  ## EE-specific
-
   scope module: :groups do
     ## EE-specific
     resource :analytics, only: [:show]

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/api/groups.md

@@ -2,7 +2,12 @@
 
 ## List groups
 
-Get a list of groups. (As user: my groups, as admin: all groups)
+Get a list of groups. (As user: my groups or all available, as admin: all groups).
+
+Parameters:
+
+- `all_available` (optional) - if passed, show all groups you have access to
+- `skip_groups` (optional)(array of group IDs) - if passed, skip groups
 
 ```
 GET /groups

doc/user/project/merge_requests/merge_request_approvals.md

@@ -58,6 +58,16 @@ 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.
 
+### Approver groups
+
+> [Introduced][ee-743] in GitLab Enterprise Edition 8.13.
+
+You can also define one or more groups that can be assigned as approvers. It
+works the same way like regular approvers do, the only difference is that you
+assign several users with one action. One possible scenario would be to to assign
+a group of approvers at the project level and change them later when creating
+or editing the merge request.
+
 ## Using approvals
 
 After configuring approvals, you will be able to change the default set of
@@ -81,3 +91,5 @@ Once you approve, the button will disappear and the number of approvers
 will be decreased by one.
 
 ![Merge request approval](img/approvals_mr_approved.png)
+
+[ee-743]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/743

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

lib/api/groups.rb

@@ -8,11 +8,14 @@ module API
       #
       # Parameters:
       #   skip_groups (optional) - Array of group ids to exclude from list
+      #   all_available (optional, boolean) - Show all group that you have access to
       # Example Request:
       #  GET /groups
       get do
         @groups = if current_user.admin
                     Group.all
+                  elsif params[:all_available]
+                    GroupsFinder.new.execute(current_user)
                   else
                     current_user.groups
                   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) }
 

spec/requests/api/groups_spec.rb

@@ -65,6 +65,17 @@ describe API::API, api: true  do
         expect(json_response.length).to eq(1)
       end
     end
+
+    context "when using all_available in request" do
+      it "returns all groups you have access to" do
+        public_group = create :group, :public
+        get api("/groups", user1), all_available: true
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.first['name']).to eq(public_group.name)
+      end
+    end
   end
 
   describe "GET /groups/:id" do