Merge branch...

Merge branch '11566-do-not-fetch-all-public-groups-for-approval-rule-when-search-is-empty' into 'master'

Resolve "Do not fetch all public groups for approval rule when search is empty"

Closes #11566

See merge request gitlab-org/gitlab-ee!12628

ee/app/assets/javascripts/approvals/components/approvers_select.vue

@@ -126,9 +126,13 @@ export default {
         .then(results => ({ results }));
     },
     fetchGroups(term) {
+      // Don't includeAll when search is empty. Otherwise, the user could get a lot of garbage choices.
+      // https://gitlab.com/gitlab-org/gitlab-ee/issues/11566
+      const includeAll = term.trim().length > 0;
+
       return Api.groups(term, {
         skip_groups: this.skipGroupIds,
-        all_available: true,
+        all_available: includeAll,
       });
     },
     fetchUsers(term) {

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

@@ -9,11 +9,23 @@ describe 'Merge request > User edits MR with approval rules', :js do
 
   let(:merge_request) { create(:merge_request, source_project: project) }
   let(:mr_rule_names) { %w[foo lorem ipsum] }
+  let(:modal_id) { '#mr-edit-approvals-create-modal' }
+  let(:members_selector) { "#{modal_id} input[name=members]" }
+  let(:members_search_selector) { "#{modal_id} .select2-input" }
 
   def page_rule_names
     page.all('.js-approval-rules table .js-name')
   end
 
+  def add_approval_rule_member(type, name)
+    open_select2 members_selector
+    wait_for_requests
+    find(".select2-result-label .#{type}-result", text: name).click
+    close_select2 members_selector
+
+    find("#{modal_id} button", text: 'Add', exact_text: true).click
+  end
+
   before do
     project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
     stub_licensed_features(multiple_approval_rules: true)
@@ -34,32 +46,42 @@ describe 'Merge request > User edits MR with approval rules', :js do
     expect(names).to eq(mr_rule_names)
   end
 
+  it "allows user to create approval rule" do
+    rule_name = "Custom Approval Rule"
+
+    click_button "Add approvers"
+
+    fill_in "Name", with: rule_name
+
+    add_approval_rule_member('user', approver.name)
+
+    find("#{modal_id} button", text: 'Add approvers').click
+    wait_for_requests
+
+    expect(page_rule_names.last).to have_text(rule_name)
+  end
+
   context "with public group" do
     let!(:group) { create(:group, :public) }
 
     before do
       group.add_developer create(:user)
-    end
-
-    it "can be added by non member" do
-      members_selector = '#mr-edit-approvals-create-modal input[name=members]'
-      rule_name = "Custom Approval Rule"
 
       click_button "Add approvers"
+    end
 
-      fill_in "Name", with: rule_name
-
+    it "with empty search, does not show public group" do
       open_select2 members_selector
-      find('.select2-result-label .group-result', text: group.name).click
-      close_select2 members_selector
-
-      find('#mr-edit-approvals-create-modal button', text: 'Add', exact_text: true).click
+      wait_for_requests
 
-      find('#mr-edit-approvals-create-modal button', text: 'Add approvers').click
+      expect(page).not_to have_selector('.select2-result-label .group-result', text: group.name)
+    end
 
+    it "with non-empty search, shows public group" do
+      find(members_search_selector).set group.name
       wait_for_requests
 
-      expect(page_rule_names.last).to have_text(rule_name)
+      expect(page).to have_selector('.select2-result-label .group-result', text: group.name)
     end
   end
 end

ee/spec/javascripts/approvals/components/approvers_select_spec.js

@@ -109,52 +109,63 @@ describe('Approvals ApproversSelect', () => {
     search();
   });
 
-  it('searches with text and skips given ids', done => {
-    factory();
-
+  describe('with search term', () => {
     const term = 'lorem';
 
-    waitForEvent($input, 'select2-loaded')
-      .then(() => {
-        expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [], all_available: true });
-        expect(Api.approverUsers).toHaveBeenCalledWith(term, {
-          skip_users: [],
-          project_id: TEST_PROJECT_ID,
-        });
-      })
-      .then(done)
-      .catch(done.fail);
+    beforeEach(done => {
+      factory();
+
+      waitForEvent($input, 'select2-loaded')
+        .then(done)
+        .catch(done.fail);
+
+      search(term);
+    });
+
+    it('fetches all available groups', () => {
+      expect(Api.groups).toHaveBeenCalledWith(term, { skip_groups: [], all_available: true });
+    });
 
-    search(term);
+    it('fetches users', () => {
+      expect(Api.approverUsers).toHaveBeenCalledWith(term, {
+        skip_users: [],
+        project_id: TEST_PROJECT_ID,
+      });
+    });
   });
 
-  it('searches and skips given groups and users', done => {
+  describe('with empty seach term and skips', () => {
     const skipGroupIds = [7, 8];
     const skipUserIds = [9, 10];
 
-    factory({
-      propsData: {
-        skipGroupIds,
-        skipUserIds,
-      },
+    beforeEach(done => {
+      factory({
+        propsData: {
+          skipGroupIds,
+          skipUserIds,
+        },
+      });
+
+      waitForEvent($input, 'select2-loaded')
+        .then(done)
+        .catch(done.fail);
+
+      search();
     });
 
-    waitForEvent($input, 'select2-loaded')
-      .then(() => {
-        expect(Api.groups).toHaveBeenCalledWith('', {
-          skip_groups: skipGroupIds,
-          all_available: true,
-        });
-
-        expect(Api.approverUsers).toHaveBeenCalledWith('', {
-          skip_users: skipUserIds,
-          project_id: TEST_PROJECT_ID,
-        });
-      })
-      .then(done)
-      .catch(done.fail);
+    it('skips groups and does not fetch all available', () => {
+      expect(Api.groups).toHaveBeenCalledWith('', {
+        skip_groups: skipGroupIds,
+        all_available: false,
+      });
+    });
 
-    search();
+    it('skips users', () => {
+      expect(Api.approverUsers).toHaveBeenCalledWith('', {
+        skip_users: skipUserIds,
+        project_id: TEST_PROJECT_ID,
+      });
+    });
   });
 
   it('emits input when data changes', done => {