Merge branch '262875-seat-members-search-be' into 'master'

Update group member search

See merge request gitlab-org/gitlab!48368

doc/api/members.md

@@ -241,6 +241,10 @@ Unlike other API endpoints, billable members is updated once per day at 12:00 UT
 
 This function takes [pagination](README.md#pagination) parameters `page` and `per_page` to restrict the list of users.
 
+[Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/262875) in GitLab 13.7, the `search` and
+`sort` parameters allow you to search for billable group members by name and sort the results,
+respectively.
+
 ```plaintext
 GET /groups/:id/billable_members
 ```
@@ -248,6 +252,21 @@ GET /groups/:id/billable_members
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `search`  | string         | no  | A query string to search for group members by name, username, or email. |
+| `sort`    | string         | no  | A query string containing parameters that specify the sort attribute and order. See supported values below.|
+
+The supported values for the `sort` attribute are:
+
+| Value               | Description              |
+| ------------------- | ------------------------ |
+| `access_level_asc`  | Access level, ascending  |
+| `access_level_desc` | Access level, descending |
+| `last_joined`       | Last joined              |
+| `name_asc`          | Name, ascending          |
+| `name_desc`         | Name, descending         |
+| `oldest_joined`     | Oldest joined            |
+| `oldest_sign_in`    | Oldest sign in           |
+| `recent_sign_in`    | Recent sign in           |
 
 ```shell
 curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/billable_members"

ee/lib/ee/api/helpers/members_helpers.rb

@@ -7,6 +7,14 @@ module EE
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
 
+        class << self
+          include ::SortingHelper
+
+          def member_sort_options
+            member_sort_options_hash.keys
+          end
+        end
+
         prepended do
           params :optional_filter_params_ee do
             optional :with_saml_identity, type: Grape::API::Boolean, desc: "List only members with linked SAML identity"
@@ -76,14 +84,11 @@ module EE
           ).for_member(member).security_event
         end
 
-        def paginate_billable_from_user_ids(user_ids)
-          paginated = paginate(::Kaminari.paginate_array(user_ids.sort))
-
-          users_as_hash = ::User.id_in(paginated).index_by(&:id)
+        def billed_users_for(group, search_term, order_by)
+          users = ::User.id_in(group.billed_user_ids)
+          users = users.search(search_term) if search_term
 
-          # map! ensures same paginatable array is manipulated
-          # instead of creating a new non-paginatable array
-          paginated.map! { |user_id| users_as_hash[user_id] }
+          users.sort_by_attribute(order_by || 'name_asc')
         end
       end
     end

ee/lib/ee/api/members.rb

@@ -47,6 +47,8 @@ module EE
           end
           params do
             use :pagination
+            optional :search, type: String, desc: 'The exact name of the subscribed member'
+            optional :sort, type: String, desc: 'The sorting option', values: Helpers::MembersHelpers.member_sort_options
           end
           get ":id/billable_members" do
             group = find_group!(params[:id])
@@ -56,7 +58,8 @@ module EE
             bad_request!(nil) if group.subgroup?
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
-            users = paginate_billable_from_user_ids(group.billed_user_ids)
+            sorting = params[:sort] || 'id_asc'
+            users = paginate(billed_users_for(group, params[:search], sorting))
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end

ee/spec/lib/ee/api/helpers/members_helpers_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe EE::API::Helpers::MembersHelpers do
-  subject(:members_helpers) { Class.new.include(described_class).new }
+  let(:members_helpers) { Class.new.include(described_class).new }
 
   before do
     allow(members_helpers).to receive(:current_user).and_return(create(:user))
@@ -21,6 +21,8 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
   end
 
   describe '#log_audit_event' do
+    subject { members_helpers }
+
     it_behaves_like 'creates security_event', 'group' do
       let(:source) { create(:group) }
       let(:member) { create(:group_member, :owner, group: source, user: create(:user)) }
@@ -32,36 +34,50 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
     end
   end
 
-  describe '#paginate_billable_from_user_ids' do
-    subject(:members_helpers) { Class.new.include(described_class, API::Helpers::Pagination).new }
+  describe '#billed_users_for' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
+    let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
+    let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
+    let_it_be(:sophie) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
+    let(:search_term) { nil }
+    let(:order_by) { nil }
 
-    let_it_be(:users) { create_list(:user, 3) }
-    let(:user_ids) { users.map(&:id) }
-    let(:page) { 1 }
-    let(:per_page) { 2 }
+    subject { members_helpers.billed_users_for(group, search_term, order_by) }
 
-    before do
-      allow(members_helpers).to receive(:params).and_return({ page: page, per_page: per_page })
-      allow(members_helpers).to receive(:header) { }
-      allow(members_helpers).to receive(:request).and_return(double(:request, url: ''))
-    end
+    context 'when a search parameter is present' do
+      let(:search_term) { 'John' }
+
+      context 'when a sorting parameter is provided (eg name descending)' do
+        let(:order_by) { 'name_desc' }
+
+        it 'sorts results accordingly' do
+          expect(subject).to eq([john_smith, john_doe].map(&:user))
+        end
+      end
 
-    it 'returns paginated User array in asc order' do
-      results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse)
+      context 'when a sorting parameter is not provided' do
+        let(:order_by) { nil }
 
-      expect(results).to all be_a(User)
-      expect(results.size).to eq(per_page)
-      expect(results.map { |result| result.id }).to eq(user_ids.first(2))
+        it 'sorts results by name ascending' do
+          expect(subject).to eq([john_doe, john_smith].map(&:user))
+        end
+      end
     end
 
-    context 'when page is 2' do
-      let(:page) { 2 }
+    context 'when a search parameter is not present' do
+      it 'returns expected users in name asc order' do
+        allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
+
+        expect(subject).to eq([john_doe, john_smith, maria, sophie].map(&:user))
+      end
 
-      it 'returns User as paginated array' do
-        results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse)
+      context 'and when a sorting parameter is provided (eg name descending)' do
+        let(:order_by) { 'name_desc' }
 
-        expect(results.size).to eq(1)
-        expect(results.map { |result| result.id }).to contain_exactly(user_ids.last)
+        it 'sorts results accordingly' do
+          expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+        end
       end
     end
   end

ee/spec/requests/api/members_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe API::Members do
+  include EE::API::Helpers::MembersHelpers
+
   context 'group members endpoints for group with minimal access feature' do
     let_it_be(:group) { create(:group) }
     let_it_be(:minimal_access_member) { create(:group_member, :minimal_access, source: group) }
@@ -339,7 +341,7 @@ RSpec.describe API::Members do
       end
     end
 
-    let_it_be(:nested_user) { create(:user) }
+    let_it_be(:nested_user) { create(:user, name: 'Scott Anderson') }
     let_it_be(:nested_group) do
       create(:group, parent: group) do |nested_group|
         nested_group.add_developer(nested_user)
@@ -347,9 +349,10 @@ RSpec.describe API::Members do
     end
 
     let(:url) { "/groups/#{group.id}/billable_members" }
+    let(:params) { {} }
 
     subject do
-      get api(url, owner)
+      get api(url, owner), params: params
       json_response
     end
 
@@ -361,7 +364,7 @@ RSpec.describe API::Members do
         end
       end
 
-      let!(:linked_group_user) { create(:user) }
+      let!(:linked_group_user) { create(:user, name: 'Scott McNeil') }
       let!(:linked_group) do
         create(:group) do |linked_group|
           linked_group.add_developer(linked_group_user)
@@ -375,6 +378,45 @@ RSpec.describe API::Members do
 
         expect_paginated_array_response(*[owner, maintainer, nested_user, project_user, linked_group_user].map(&:id))
       end
+
+      context 'with seach params provided' do
+        let(:params) { { search: nested_user.name } }
+
+        it 'returns the relevant billable users' do
+          subject
+
+          expect_paginated_array_response([nested_user.id])
+        end
+      end
+
+      context 'with search and sort params provided' do
+        it 'accepts only sorting options defined in a list' do
+          EE::API::Helpers::MembersHelpers.member_sort_options.each do |sorting|
+            get api(url, owner), params: { search: 'name', sort: sorting }
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
+
+        it 'does not accept query string not defined in a list' do
+          defined_query_strings = EE::API::Helpers::MembersHelpers.member_sort_options
+          sorting = 'fake_sorting'
+
+          get api(url, owner), params: { search: 'name', sort: sorting }
+
+          expect(defined_query_strings).not_to include(sorting)
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        context 'when a specific sorting is provided' do
+          let(:params) { { search: 'Scott', sort: 'name_desc' } }
+
+          it 'returns the relevant billable users' do
+            subject
+
+            expect_paginated_array_response(*[linked_group_user, nested_user].map(&:id))
+          end
+        end
+      end
     end
 
     context 'when feature is disabled' do