From b694cfae56da0127deefee0796ad2eb3e61017b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com> Date: Tue, 24 Nov 2020 13:33:10 +0100 Subject: [PATCH 1/5] Updated group member search Added search and sort params. Updated rspec accordingly. --- ee/lib/ee/api/helpers/members_helpers.rb | 22 +++++++++- ee/lib/ee/api/members.rb | 4 +- .../ee/api/helpers/members_helpers_spec.rb | 41 ++++++++++++++++++- ee/spec/requests/api/members_spec.rb | 27 ++++++++++-- 4 files changed, 87 insertions(+), 7 deletions(-) diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index f805205cda8ab87c..726b727b4a1565de 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -6,6 +6,7 @@ module Helpers module MembersHelpers extend ActiveSupport::Concern extend ::Gitlab::Utils::Override + include ::SortingHelper prepended do params :optional_filter_params_ee do @@ -76,8 +77,10 @@ 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)) + def paginate_billable_from_user_ids(user_ids, params = {}) + sorted_ids = params[:search].present? ? user_ids : user_ids.sort + + paginated = paginate(::Kaminari.paginate_array(sorted_ids)) users_as_hash = ::User.id_in(paginated).index_by(&:id) @@ -85,6 +88,21 @@ def paginate_billable_from_user_ids(user_ids) # instead of creating a new non-paginatable array paginated.map! { |user_id| users_as_hash[user_id] } end + + def group_billed_user_ids_for(group, params) + if params[:search].present? + sorting = params[:sort] || sort_value_name + + ::GroupMember + .with_user(group.billed_user_ids) + .search(params[:search]) + .sort_by_attribute(sorting) + .map(&:user_id) + .uniq + else + group.billed_user_ids + end + end end end end diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 1277cf81f98c34a5..516826b2b39eac0f 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 name of the subscribed member' + optional :sort, type: String, desc: 'The sorting option' end get ":id/billable_members" do group = find_group!(params[:id]) @@ -56,7 +58,7 @@ 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) + users = paginate_billable_from_user_ids(group_billed_user_ids_for(group, params), params) 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 2fcf27f4cc5340dc..2cb3e1da5a806586 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)) } @@ -65,4 +67,41 @@ end end end + + describe '#group_billed_user_ids_for' do + let_it_be(:group) { create(:group) } + let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) } + let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) } + let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) } + let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) } + let(:params) { {} } + + subject { members_helpers.group_billed_user_ids_for(group, params).to_a } + + context 'when a search parameter is present' do + let(:params) { { search: 'John', sort: sort } } + + context 'when a sorting parameter is provided (eg name descending)' do + let(:sort) { 'name_desc' } + + it 'sorts results accordingly' do + expect(subject).to eq([gm_2, gm_3].map(&:user_id)) + end + end + + context 'when a sorting parameter is not provided' do + let(:sort) { nil } + + it 'sorts results by name ascending' do + expect(subject).to eq([gm_3, gm_2].map(&:user_id)) + end + end + end + + context 'when a search parameter is not present' do + it 'returns the expected group member user ids' do + expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) + end + end + end end diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb index 97229f0a2f00c8e9..646fbe26747a0ef0 100644 --- a/ee/spec/requests/api/members_spec.rb +++ b/ee/spec/requests/api/members_spec.rb @@ -339,7 +339,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 +347,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 +362,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 +376,26 @@ 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 + 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 context 'when feature is disabled' do -- GitLab From fcf75d779fc5bae51f547b2c6ee6ae7266e8fe03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com> Date: Mon, 30 Nov 2020 16:29:22 +0100 Subject: [PATCH 2/5] Applied comments from code reviewer Adjusted newly introduced queries. --- app/models/concerns/group_member_sorting.rb | 13 +++++ app/models/members/group_member.rb | 2 + doc/api/members.md | 5 ++ ee/app/models/ee/group.rb | 13 +++++ ee/lib/ee/api/helpers/members_helpers.rb | 25 ++++------ ee/lib/ee/api/members.rb | 6 +-- .../ee/api/helpers/members_helpers_spec.rb | 47 +------------------ ee/spec/models/group_spec.rb | 45 ++++++++++++++++++ ee/spec/requests/api/members_spec.rb | 29 ++++++++++-- .../concerns/group_member_sorting_spec.rb | 26 ++++++++++ 10 files changed, 142 insertions(+), 69 deletions(-) create mode 100644 app/models/concerns/group_member_sorting.rb create mode 100644 spec/models/concerns/group_member_sorting_spec.rb diff --git a/app/models/concerns/group_member_sorting.rb b/app/models/concerns/group_member_sorting.rb new file mode 100644 index 0000000000000000..768112b17dfbe908 --- /dev/null +++ b/app/models/concerns/group_member_sorting.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module GroupMemberSorting + extend ActiveSupport::Concern + + class_methods do + include SortingHelper + + def sorting_for(sort_value) + sort_value || sort_value_name + end + end +end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 2bbcdbbe5ced78a6..e3645891e7bf943e 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -3,6 +3,7 @@ class GroupMember < Member include FromUnion include CreatedAtFilterable + include GroupMemberSorting SOURCE_TYPE = 'Namespace' @@ -22,6 +23,7 @@ class GroupMember < Member scope :of_ldap_type, -> { where(ldap: true) } scope :count_users_by_group_id, -> { group(:source_id).count } scope :with_user, -> (user) { where(user: user) } + scope :get_user_id, -> { pluck(:user_id) } after_create :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite? diff --git a/doc/api/members.md b/doc/api/members.md index fe7fffeab5df57be..5b08e9cdcd981634 100644 --- a/doc/api/members.md +++ b/doc/api/members.md @@ -241,6 +241,9 @@ 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 allows to search for billable group members by their name as well as specifying result sorting. + ```plaintext GET /groups/:id/billable_members ``` @@ -248,6 +251,8 @@ 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 their name | +| `sort` | string | no | A query string specifying the sorting attribute and order | ```shell curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/billable_members" diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 96272ecf3daf7470..6cbb96fb1d6f3eeb 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -448,6 +448,19 @@ def releases_percentage ) end + def billed_user_ids_for(search_term, order_by) + if search_term.present? + ::GroupMember + .with_user(billed_user_ids) + .search(search_term) + .sort_by_attribute(::GroupMember.sorting_for(order_by)) + .get_user_id + .uniq + else + billed_user_ids.sort + end + end + private def custom_project_templates_group_allowed diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index 726b727b4a1565de..2e5dc895bab27960 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -6,7 +6,6 @@ module Helpers module MembersHelpers extend ActiveSupport::Concern extend ::Gitlab::Utils::Override - include ::SortingHelper prepended do params :optional_filter_params_ee do @@ -78,29 +77,23 @@ def log_audit_event(member) end def paginate_billable_from_user_ids(user_ids, params = {}) - sorted_ids = params[:search].present? ? user_ids : user_ids.sort + paginated = paginate(::Kaminari.paginate_array(user_ids)) - paginated = paginate(::Kaminari.paginate_array(sorted_ids)) - - users_as_hash = ::User.id_in(paginated).index_by(&:id) + users_as_hash = ::User + .id_in(paginated) + .sort_by_attribute(::GroupMember.sorting_for(params[:sort])) + .index_by(&:id) # map! ensures same paginatable array is manipulated # instead of creating a new non-paginatable array paginated.map! { |user_id| users_as_hash[user_id] } end - def group_billed_user_ids_for(group, params) - if params[:search].present? - sorting = params[:sort] || sort_value_name + class << self + include ::SortingHelper - ::GroupMember - .with_user(group.billed_user_ids) - .search(params[:search]) - .sort_by_attribute(sorting) - .map(&:user_id) - .uniq - else - group.billed_user_ids + def member_sort_options + member_sort_options_hash.keys end end end diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 516826b2b39eac0f..5cf0588bf26680c0 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -47,8 +47,8 @@ module Members end params do use :pagination - optional :search, type: String, desc: 'The name of the subscribed member' - optional :sort, type: String, desc: 'The sorting option' + 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, default: 'name_asc' end get ":id/billable_members" do group = find_group!(params[:id]) @@ -58,7 +58,7 @@ 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_for(group, params), params) + users = paginate_billable_from_user_ids(group.billed_user_ids_for(params[:search], params[:sort]), params) 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 2cb3e1da5a806586..7cd29e4ab6d2a442 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -48,60 +48,15 @@ allow(members_helpers).to receive(:request).and_return(double(:request, url: '')) end - it 'returns paginated User array in asc order' do - results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse) - - 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)) - end - context 'when page is 2' do let(:page) { 2 } it 'returns User as paginated array' do - results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse) + results = members_helpers.paginate_billable_from_user_ids(user_ids) expect(results.size).to eq(1) expect(results.map { |result| result.id }).to contain_exactly(user_ids.last) end end end - - describe '#group_billed_user_ids_for' do - let_it_be(:group) { create(:group) } - let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) } - let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) } - let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) } - let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) } - let(:params) { {} } - - subject { members_helpers.group_billed_user_ids_for(group, params).to_a } - - context 'when a search parameter is present' do - let(:params) { { search: 'John', sort: sort } } - - context 'when a sorting parameter is provided (eg name descending)' do - let(:sort) { 'name_desc' } - - it 'sorts results accordingly' do - expect(subject).to eq([gm_2, gm_3].map(&:user_id)) - end - end - - context 'when a sorting parameter is not provided' do - let(:sort) { nil } - - it 'sorts results by name ascending' do - expect(subject).to eq([gm_3, gm_2].map(&:user_id)) - end - end - end - - context 'when a search parameter is not present' do - it 'returns the expected group member user ids' do - expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) - end - end - end end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index b32eaf21b2106fb4..f36ed649e1a123da 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -1223,4 +1223,49 @@ end end end + + describe '#billed_user_ids_for' do + let_it_be(:group) { create(:group) } + let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) } + let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) } + let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) } + let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) } + let(:search_term) { nil } + let(:order_by) { nil } + + subject { group.billed_user_ids_for(search_term, order_by) } + + context 'when a search parameter is present' do + let(:search_term) { 'John' } + let(:order_by) { sort } + + context 'when a sorting parameter is provided (eg name descending)' do + let(:sort) { 'name_desc' } + + it 'sorts results accordingly' do + expect(subject).to eq([gm_2, gm_3].map(&:user_id)) + end + end + + context 'when a sorting parameter is not provided' do + let(:sort) { nil } + + it 'sorts results by name ascending' do + expect(subject).to eq([gm_3, gm_2].map(&:user_id)) + end + end + end + + context 'when a search parameter is not present' do + it 'returns the expected group member user ids' do + expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) + end + + it 'returns user array in asc order' do + allow(group).to receive(:billed_user_ids).and_return([gm_3, gm_2, gm_4, gm_1].map(&:user_id)) + + expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) + end + end + end end diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb index 646fbe26747a0ef0..2048614b1b79e8b1 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) } @@ -388,12 +390,31 @@ end context 'with search and sort params provided' do - let(:params) { { search: 'Scott', sort: 'name_desc' } } + 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 'returns the relevant billable users' do - subject + 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_paginated_array_response(*[linked_group_user, nested_user].map(&:id)) + 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 diff --git a/spec/models/concerns/group_member_sorting_spec.rb b/spec/models/concerns/group_member_sorting_spec.rb new file mode 100644 index 0000000000000000..a54476b535834f76 --- /dev/null +++ b/spec/models/concerns/group_member_sorting_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GroupMemberSorting do + describe '.sorting_for' do + let(:sort_value) { nil } + let(:sortable_class) { GroupMember } + + subject { sortable_class.sorting_for(sort_value) } + + context 'sorting is not passed in' do + it 'returns name_asc' do + expect(subject).to eq('name_asc') + end + end + + context 'sorting is passed in' do + let(:sort_value) { 'name_desc' } + + it 'returns that sorting' do + expect(subject).to eq(sort_value) + end + end + end +end -- GitLab From c0920003c6b1823b0ae00ee4f283ad0b481b8a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com> Date: Mon, 30 Nov 2020 21:39:13 +0100 Subject: [PATCH 3/5] Applied suggestion from reviewer Did some refactoring in Group class. --- ee/app/models/ee/group.rb | 56 +++++++++++++++--------- ee/lib/ee/api/helpers/members_helpers.rb | 13 ------ ee/lib/ee/api/members.rb | 2 +- ee/spec/models/group_spec.rb | 35 ++++++++------- 4 files changed, 55 insertions(+), 51 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 6cbb96fb1d6f3eeb..db4c55ab379f704e 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -299,23 +299,43 @@ def billable_members_count(requested_hosted_plan = nil) # We are plucking the user_ids from the "Members" table in an array and # converting the array of user_ids to a Set which will have unique user_ids. def billed_user_ids(requested_hosted_plan = nil) + billed_user_members.pluck(:user_id).to_set + end + + def billed_user_members(requested_hosted_plan = nil) if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD) - strong_memoize(:gold_billed_user_ids) do - (billed_group_members.non_guests.distinct.pluck(:user_id) + - billed_project_members.non_guests.distinct.pluck(:user_id) + - billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) + - billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set + strong_memoize(:gold_billed_users) do + gold_billed_members end else - strong_memoize(:non_gold_billed_user_ids) do - (billed_group_members.distinct.pluck(:user_id) + - billed_project_members.distinct.pluck(:user_id) + - billed_shared_group_members.distinct.pluck(:user_id) + - billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set + strong_memoize(:non_gold_billed_users) do + non_gold_billed_members end end end + def non_gold_billed_members + ::Member.from_union( + [ + billed_group_members, + billed_project_members, + billed_shared_group_members, + billed_invited_group_to_project_members + ] + ).distinct + end + + def gold_billed_members + Member.from_union( + [ + billed_group_members.non_guests, + billed_project_members.non_guests, + billed_shared_non_guests_group_members.non_guests, + billed_invited_non_guests_group_to_project_members.non_guests + ] + ).distinct + end + override :supports_events? def supports_events? feature_available?(:epics) @@ -448,17 +468,11 @@ def releases_percentage ) end - def billed_user_ids_for(search_term, order_by) - if search_term.present? - ::GroupMember - .with_user(billed_user_ids) - .search(search_term) - .sort_by_attribute(::GroupMember.sorting_for(order_by)) - .get_user_id - .uniq - else - billed_user_ids.sort - end + def billed_users_for(search_term, order_by) + users = ::User.id_in(billed_user_members.pluck(:user_id).uniq) + users = users.search(search_term) if search_term + + users.sort_by_attribute(::GroupMember.sorting_for(order_by)) end private diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index 2e5dc895bab27960..d67fb617b7bbb543 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -76,19 +76,6 @@ def log_audit_event(member) ).for_member(member).security_event end - def paginate_billable_from_user_ids(user_ids, params = {}) - paginated = paginate(::Kaminari.paginate_array(user_ids)) - - users_as_hash = ::User - .id_in(paginated) - .sort_by_attribute(::GroupMember.sorting_for(params[:sort])) - .index_by(&:id) - - # map! ensures same paginatable array is manipulated - # instead of creating a new non-paginatable array - paginated.map! { |user_id| users_as_hash[user_id] } - end - class << self include ::SortingHelper diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 5cf0588bf26680c0..409496878d8a7195 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -58,7 +58,7 @@ 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_for(params[:search], params[:sort]), params) + users = paginate(group.billed_users_for(params[:search], params[:sort])) present users, with: ::API::Entities::UserBasic, current_user: current_user end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index f36ed649e1a123da..1743cd1a1037ebef 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -1224,47 +1224,50 @@ end end - describe '#billed_user_ids_for' do + describe '#billed_users_for' do let_it_be(:group) { create(:group) } - let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) } - let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) } - let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) } - let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) } + 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 } - subject { group.billed_user_ids_for(search_term, order_by) } + subject { group.billed_users_for(search_term, order_by) } context 'when a search parameter is present' do let(:search_term) { 'John' } - let(:order_by) { sort } context 'when a sorting parameter is provided (eg name descending)' do - let(:sort) { 'name_desc' } + let(:order_by) { 'name_desc' } it 'sorts results accordingly' do - expect(subject).to eq([gm_2, gm_3].map(&:user_id)) + expect(subject).to eq([john_smith, john_doe].map(&:user)) end end context 'when a sorting parameter is not provided' do - let(:sort) { nil } + let(:order_by) { nil } it 'sorts results by name ascending' do - expect(subject).to eq([gm_3, gm_2].map(&:user_id)) + expect(subject).to eq([john_doe, john_smith].map(&:user)) end end end context 'when a search parameter is not present' do - it 'returns the expected group member user ids' do - expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) + 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 array in asc order' do - allow(group).to receive(:billed_user_ids).and_return([gm_3, gm_2, gm_4, gm_1].map(&:user_id)) + context 'and when a sorting parameter is provided (eg name descending)' do + let(:order_by) { 'name_desc' } - expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id)) + it 'sorts results accordingly' do + expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user)) + end end end end -- GitLab From e592e1fbdab4262d94f21815f278ef0a14a8b8da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com> Date: Wed, 2 Dec 2020 12:39:00 +0100 Subject: [PATCH 4/5] Reverted some updates to simplify MR Reverted refactoring to be done in another MR. --- app/models/concerns/group_member_sorting.rb | 13 ------ app/models/members/group_member.rb | 1 - doc/api/members.md | 22 +++++++-- ee/app/models/ee/group.rb | 45 ++++++------------- ee/lib/ee/api/members.rb | 5 ++- .../ee/api/helpers/members_helpers_spec.rb | 26 ----------- .../concerns/group_member_sorting_spec.rb | 26 ----------- 7 files changed, 34 insertions(+), 104 deletions(-) delete mode 100644 app/models/concerns/group_member_sorting.rb delete mode 100644 spec/models/concerns/group_member_sorting_spec.rb diff --git a/app/models/concerns/group_member_sorting.rb b/app/models/concerns/group_member_sorting.rb deleted file mode 100644 index 768112b17dfbe908..0000000000000000 --- a/app/models/concerns/group_member_sorting.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module GroupMemberSorting - extend ActiveSupport::Concern - - class_methods do - include SortingHelper - - def sorting_for(sort_value) - sort_value || sort_value_name - end - end -end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index e3645891e7bf943e..03fb2194687486dc 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -3,7 +3,6 @@ class GroupMember < Member include FromUnion include CreatedAtFilterable - include GroupMemberSorting SOURCE_TYPE = 'Namespace' diff --git a/doc/api/members.md b/doc/api/members.md index 5b08e9cdcd981634..bf445ed62d4edad5 100644 --- a/doc/api/members.md +++ b/doc/api/members.md @@ -241,8 +241,9 @@ 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 allows to search for billable group members by their name as well as specifying result sorting. +[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 @@ -251,8 +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 their name | -| `sort` | string | no | A query string specifying the sorting attribute and order | +| `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/app/models/ee/group.rb b/ee/app/models/ee/group.rb index db4c55ab379f704e..91515d1300784b03 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -8,6 +8,7 @@ module EE module Group extend ActiveSupport::Concern extend ::Gitlab::Utils::Override + include ::SortingHelper prepended do include TokenAuthenticatable @@ -299,43 +300,23 @@ def billable_members_count(requested_hosted_plan = nil) # We are plucking the user_ids from the "Members" table in an array and # converting the array of user_ids to a Set which will have unique user_ids. def billed_user_ids(requested_hosted_plan = nil) - billed_user_members.pluck(:user_id).to_set - end - - def billed_user_members(requested_hosted_plan = nil) if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD) - strong_memoize(:gold_billed_users) do - gold_billed_members + strong_memoize(:gold_billed_user_ids) do + (billed_group_members.non_guests.distinct.pluck(:user_id) + + billed_project_members.non_guests.distinct.pluck(:user_id) + + billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) + + billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set end else - strong_memoize(:non_gold_billed_users) do - non_gold_billed_members + strong_memoize(:non_gold_billed_user_ids) do + (billed_group_members.distinct.pluck(:user_id) + + billed_project_members.distinct.pluck(:user_id) + + billed_shared_group_members.distinct.pluck(:user_id) + + billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set end end end - def non_gold_billed_members - ::Member.from_union( - [ - billed_group_members, - billed_project_members, - billed_shared_group_members, - billed_invited_group_to_project_members - ] - ).distinct - end - - def gold_billed_members - Member.from_union( - [ - billed_group_members.non_guests, - billed_project_members.non_guests, - billed_shared_non_guests_group_members.non_guests, - billed_invited_non_guests_group_to_project_members.non_guests - ] - ).distinct - end - override :supports_events? def supports_events? feature_available?(:epics) @@ -469,10 +450,10 @@ def releases_percentage end def billed_users_for(search_term, order_by) - users = ::User.id_in(billed_user_members.pluck(:user_id).uniq) + users = ::User.id_in(billed_user_ids) users = users.search(search_term) if search_term - users.sort_by_attribute(::GroupMember.sorting_for(order_by)) + users.sort_by_attribute(order_by || sort_value_name) end private diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 409496878d8a7195..a7db813fd6510725 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -48,7 +48,7 @@ module Members 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, default: 'name_asc' + 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]) @@ -58,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(group.billed_users_for(params[:search], params[:sort])) + sorting = params[:sort] || 'id_asc' + users = paginate(group.billed_users_for(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 7cd29e4ab6d2a442..8807de54cf33f238 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -33,30 +33,4 @@ let(:member) { create(:project_member, project: source, user: create(:user)) } end end - - describe '#paginate_billable_from_user_ids' do - subject(:members_helpers) { Class.new.include(described_class, API::Helpers::Pagination).new } - - let_it_be(:users) { create_list(:user, 3) } - let(:user_ids) { users.map(&:id) } - let(:page) { 1 } - let(:per_page) { 2 } - - 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 page is 2' do - let(:page) { 2 } - - it 'returns User as paginated array' do - results = members_helpers.paginate_billable_from_user_ids(user_ids) - - expect(results.size).to eq(1) - expect(results.map { |result| result.id }).to contain_exactly(user_ids.last) - end - end - end end diff --git a/spec/models/concerns/group_member_sorting_spec.rb b/spec/models/concerns/group_member_sorting_spec.rb deleted file mode 100644 index a54476b535834f76..0000000000000000 --- a/spec/models/concerns/group_member_sorting_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe GroupMemberSorting do - describe '.sorting_for' do - let(:sort_value) { nil } - let(:sortable_class) { GroupMember } - - subject { sortable_class.sorting_for(sort_value) } - - context 'sorting is not passed in' do - it 'returns name_asc' do - expect(subject).to eq('name_asc') - end - end - - context 'sorting is passed in' do - let(:sort_value) { 'name_desc' } - - it 'returns that sorting' do - expect(subject).to eq(sort_value) - end - end - end -end -- GitLab From 778f2456fe65caf6008027a2058beb10e10374c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com> Date: Fri, 4 Dec 2020 17:06:59 +0100 Subject: [PATCH 5/5] Applied maintainer reviewer recommendation Reorganized code based on review. --- app/models/members/group_member.rb | 1 - ee/app/models/ee/group.rb | 20 +++----- ee/lib/ee/api/helpers/members_helpers.rb | 17 +++++-- ee/lib/ee/api/members.rb | 2 +- .../ee/api/helpers/members_helpers_spec.rb | 48 +++++++++++++++++++ ee/spec/models/group_spec.rb | 48 ------------------- 6 files changed, 67 insertions(+), 69 deletions(-) diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 03fb2194687486dc..2bbcdbbe5ced78a6 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -22,7 +22,6 @@ class GroupMember < Member scope :of_ldap_type, -> { where(ldap: true) } scope :count_users_by_group_id, -> { group(:source_id).count } scope :with_user, -> (user) { where(user: user) } - scope :get_user_id, -> { pluck(:user_id) } after_create :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite? diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 91515d1300784b03..96272ecf3daf7470 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -8,7 +8,6 @@ module EE module Group extend ActiveSupport::Concern extend ::Gitlab::Utils::Override - include ::SortingHelper prepended do include TokenAuthenticatable @@ -303,16 +302,16 @@ def billed_user_ids(requested_hosted_plan = nil) if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD) strong_memoize(:gold_billed_user_ids) do (billed_group_members.non_guests.distinct.pluck(:user_id) + - billed_project_members.non_guests.distinct.pluck(:user_id) + - billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) + - billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set + billed_project_members.non_guests.distinct.pluck(:user_id) + + billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) + + billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set end else strong_memoize(:non_gold_billed_user_ids) do (billed_group_members.distinct.pluck(:user_id) + - billed_project_members.distinct.pluck(:user_id) + - billed_shared_group_members.distinct.pluck(:user_id) + - billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set + billed_project_members.distinct.pluck(:user_id) + + billed_shared_group_members.distinct.pluck(:user_id) + + billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set end end end @@ -449,13 +448,6 @@ def releases_percentage ) end - def billed_users_for(search_term, order_by) - users = ::User.id_in(billed_user_ids) - users = users.search(search_term) if search_term - - users.sort_by_attribute(order_by || sort_value_name) - end - private def custom_project_templates_group_allowed diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index d67fb617b7bbb543..12cea6b7d7cf1936 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,12 +84,11 @@ def log_audit_event(member) ).for_member(member).security_event end - class << self - include ::SortingHelper + 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 - def member_sort_options - member_sort_options_hash.keys - end + 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 a7db813fd6510725..910b6324fd6e97b3 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -59,7 +59,7 @@ module Members bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group) sorting = params[:sort] || 'id_asc' - users = paginate(group.billed_users_for(params[:search], sorting)) + 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 8807de54cf33f238..a3cc9672aa8e3ac8 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -33,4 +33,52 @@ let(:member) { create(:project_member, project: source, user: create(:user)) } end end + + 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 } + + subject { members_helpers.billed_users_for(group, search_term, order_by) } + + 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 + + context 'when a sorting parameter is not provided' do + let(:order_by) { nil } + + it 'sorts results by name ascending' do + expect(subject).to eq([john_doe, john_smith].map(&:user)) + end + end + end + + 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 + + context 'and when a sorting parameter is provided (eg name descending)' do + let(:order_by) { 'name_desc' } + + it 'sorts results accordingly' do + expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user)) + end + end + end + end end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index 1743cd1a1037ebef..b32eaf21b2106fb4 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -1223,52 +1223,4 @@ end end end - - 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 } - - subject { group.billed_users_for(search_term, order_by) } - - 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 - - context 'when a sorting parameter is not provided' do - let(:order_by) { nil } - - it 'sorts results by name ascending' do - expect(subject).to eq([john_doe, john_smith].map(&:user)) - end - end - end - - 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 - - context 'and when a sorting parameter is provided (eg name descending)' do - let(:order_by) { 'name_desc' } - - it 'sorts results accordingly' do - expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user)) - end - end - end - end end -- GitLab