diff --git a/doc/api/members.md b/doc/api/members.md index fe7fffeab5df57be8eaaac13f1e991a8711d7629..bf445ed62d4edad57f905c51ecea8023f7454a61 100644 --- a/doc/api/members.md +++ b/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" diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index f805205cda8ab87c25cfea89c9386d9b373a2423..12cea6b7d7cf193606a58dba1109835c610c3cf5 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -7,6 +7,14 @@ module MembersHelpers 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 @@ def log_audit_event(member) ).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 diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 1277cf81f98c34a54b702a1576fb10e9e62a3a9a..910b6324fd6e97b3a95aa65fcec599b3b6bd89da 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -47,6 +47,8 @@ module Members 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 Members 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 diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb index 2fcf27f4cc5340dc960708a2fd5760c58a12d52e..a3cc9672aa8e3ac8bb149ccf111c35f1e09c6b40 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/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 @@ 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 @@ 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 diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb index 97229f0a2f00c8e98613760554f655b56b4beb59..2048614b1b79e8b1357668f1911615651fb66cf5 100644 --- a/ee/spec/requests/api/members_spec.rb +++ b/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 @@ 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 @@ 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 @@ 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 @@ 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