Merge branch '1593-limited-approvers' into 'master'

restrict individual approvers to project members

Closes #1593

See merge request !1691

app/assets/javascripts/approvers_select.js

@@ -23,28 +23,29 @@ export default class ApproversSelect {
     $(document).on('click', '.js-approver-remove', e => ApproversSelect.removeApprover(e));
   }
 
-  static getApprovers(fieldName, selector, key) {
+  static getApprovers(fieldName, approverList) {
     const input = $(`[name="${fieldName}"]`);
-
-    const existingApprovers = $(selector).map((i, el) =>
+    const existingApprovers = $(approverList).map((i, el) =>
       parseInt($(el).data('id'), 10),
     );
     const selectedApprovers = input.val()
       .split(',')
       .filter(val => val !== '');
-    const approvers = {
-      [key]: [...existingApprovers, ...selectedApprovers],
-    };
-    return approvers;
+    return [...existingApprovers, ...selectedApprovers];
   }
 
   fetchGroups(term) {
-    const options = ApproversSelect.getApprovers(this.fieldNames[1], '.js-approver-group', 'skip_groups');
+    const options = {
+      skip_groups: ApproversSelect.getApprovers(this.fieldNames[1], '.js-approver-group'),
+    };
     return Api.groups(term, options);
   }
 
   fetchUsers(term) {
-    const options = ApproversSelect.getApprovers(this.fieldNames[0], '.js-approver', 'skip_users');
+    const options = {
+      skip_users: ApproversSelect.getApprovers(this.fieldNames[0], '.js-approver'),
+      project_id: $('#project_id').val(),
+    };
     return Api.users(term, options);
   }
 

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

@@ -19,7 +19,7 @@
   .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)
+    = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', 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.

doc/user/project/merge_requests/merge_request_approvals.md

@@ -11,7 +11,7 @@ merge request in a project.
 ## Configuring Approvals
 
 You can configure the approvals in the project settings, under merge requests.
-To enable it, set **Approvals required** to 1 or higher and search for the
+To enable it, turn on **Activate merge request approvals** and search for the
 users you want to be approvers.
 
 ![Merge Request Approvals in Project Settings](img/approvals_settings.png)
@@ -19,8 +19,6 @@ users you want to be approvers.
 ### Approvals Required
 
 This sets the amount of approvals required before being able to merge a merge request.
-At 0, this disables the feature. Any value above 0 requires that amount of different
-users to approve the merge request.
 
 The number of approvers can be higher than the required approvals.
 

spec/features/merge_requests/approvals_spec.rb

@@ -31,6 +31,7 @@ feature 'Merge request approvals', js: true, feature: true do
 
   context 'when creating an MR' do
     let(:other_user) { create(:user) }
+    let(:non_member) { create(:user) }
 
     before do
       project.team << [user, :developer]
@@ -49,6 +50,10 @@ feature 'Merge request approvals', js: true, feature: true do
     it 'does not allow setting the current user as an approver' do
       expect(find('.select2-results')).not_to have_content(user.name)
     end
+
+    it 'filters non members from approvers list' do
+      expect(find('.select2-results')).not_to have_content(non_member.name)
+    end
   end
 
   context "Group approvers" do

spec/features/projects/settings/ee/merge_requests_settings_spec.rb

@@ -7,13 +7,14 @@ describe 'Project settings > [EE] Merge Requests', feature: true, js: true do
   let(:user) { create(:user) }
   let(:project) { create(:empty_project, approvals_before_merge: 1) }
   let(:group) { create(:group) }
-  let(:approver) { create(:user) }
+  let(:group_member) { create(:user) }
+  let(:non_member) { create(:user) }
 
   before do
     login_as(user)
     project.team << [user, :master]
-    group.add_developer(approver)
     group.add_developer(user)
+    group.add_developer(group_member)
   end
 
   scenario 'adds approver' do
@@ -28,6 +29,18 @@ describe 'Project settings > [EE] Merge Requests', feature: true, js: true do
     click_button 'Add'
 
     expect(find('.js-current-approvers')).to have_content(user.name)
+
+    find('.js-select-user-and-group').click
+
+    expect(find('.select2-results')).not_to have_content(user.name)
+  end
+
+  scenario 'filter approvers' do
+    visit edit_project_path(project)
+    find('.js-select-user-and-group').click
+
+    expect(find('.select2-results')).to have_content(user.name)
+    expect(find('.select2-results')).not_to have_content(non_member.name)
   end
 
   scenario 'adds approver group' do