diff --git a/doc/api/members.md b/doc/api/members.md index b0992aafb7e7847a001fca39b1599c4863dd1cbc..3ffe94e6f99d973f78c8dc5230c1f6389b8336bf 100644 --- a/doc/api/members.md +++ b/doc/api/members.md @@ -76,8 +76,7 @@ Example response: }, "expires_at": "2012-10-22T14:13:35Z", "access_level": 30, - "group_saml_identity": null, - "membership_state": "active" + "group_saml_identity": null }, { "id": 2, @@ -102,8 +101,7 @@ Example response: "extern_uid":"ABC-1234567890", "provider": "group_saml", "saml_provider_id": 10 - }, - "membership_state": "active" + } } ] ``` @@ -163,8 +161,7 @@ Example response: }, "expires_at": "2012-10-22T14:13:35Z", "access_level": 30, - "group_saml_identity": null, - "membership_state": "active" + "group_saml_identity": null }, { "id": 2, @@ -189,8 +186,7 @@ Example response: "extern_uid":"ABC-1234567890", "provider": "group_saml", "saml_provider_id": 10 - }, - "membership_state": "active" + } }, { "id": 3, @@ -210,8 +206,7 @@ Example response: }, "expires_at": "2012-11-22T14:13:35Z", "access_level": 30, - "group_saml_identity": null, - "membership_state": "active" + "group_saml_identity": null } ] ``` @@ -257,8 +252,7 @@ Example response: "web_url": "http://192.168.1.8:3000/root" }, "expires_at": null, - "group_saml_identity": null, - "membership_state": "active" + "group_saml_identity": null } ``` @@ -305,8 +299,7 @@ Example response: }, "email": "john@example.com", "expires_at": null, - "group_saml_identity": null, - "membership_state": "active" + "group_saml_identity": null } ``` @@ -370,7 +363,6 @@ Example response: "web_url": "http://192.168.1.8:3000/root", "last_activity_on": "2021-01-27", "membership_type": "group_member", - "membership_state": "active", "removable": true, "created_at": "2021-01-03T12:16:02.000Z" }, @@ -384,7 +376,6 @@ Example response: "email": "john@example.com", "last_activity_on": "2021-01-25", "membership_type": "group_member", - "membership_state": "active", "removable": true, "created_at": "2021-01-04T18:46:42.000Z" }, @@ -397,7 +388,6 @@ Example response: "web_url": "http://192.168.1.8:3000/root", "last_activity_on": "2021-01-20", "membership_type": "group_invite", - "membership_state": "awaiting", "removable": false, "created_at": "2021-01-09T07:12:31.000Z" } diff --git a/ee/app/finders/billed_users_finder.rb b/ee/app/finders/billed_users_finder.rb index fbf07594050a8a7eadf58474336446943d8d9fb4..8a255b64b8c28ea8a51425959ae63da774e2b413 100644 --- a/ee/app/finders/billed_users_finder.rb +++ b/ee/app/finders/billed_users_finder.rb @@ -1,11 +1,10 @@ # frozen_string_literal: true class BilledUsersFinder - def initialize(group, search_term: nil, order_by: 'name_asc', include_awaiting_members: false) + def initialize(group, search_term: nil, order_by: 'name_asc') @group = group @search_term = search_term @order_by = order_by - @include_awaiting_members = include_awaiting_members end def execute @@ -19,26 +18,19 @@ def execute group_member_user_ids: group_billed_user_ids[:group_member_user_ids], project_member_user_ids: group_billed_user_ids[:project_member_user_ids], shared_group_user_ids: group_billed_user_ids[:shared_group_user_ids], - shared_project_user_ids: group_billed_user_ids[:shared_project_user_ids], - awaiting_user_ids: awaiting_user_ids + shared_project_user_ids: group_billed_user_ids[:shared_project_user_ids] } end private - attr_reader :group, :search_term, :order_by, :include_awaiting_members + attr_reader :group, :search_term, :order_by def user_ids - group_billed_user_ids[:user_ids] + awaiting_user_ids + group_billed_user_ids[:user_ids] end def group_billed_user_ids @group_billed_user_ids ||= group.billed_user_ids end - - def awaiting_user_ids - return [] unless include_awaiting_members - - group.awaiting_user_ids - end end diff --git a/ee/lib/ee/api/entities/billable_member.rb b/ee/lib/ee/api/entities/billable_member.rb index 62e1531fce08ec17d986c7275652b7e48500adbd..bf35da8894dcc75045bf379eabb4bdeef3d94cd5 100644 --- a/ee/lib/ee/api/entities/billable_member.rb +++ b/ee/lib/ee/api/entities/billable_member.rb @@ -9,7 +9,6 @@ class BillableMember < ::API::Entities::UserBasic expose :membership_type expose :removable expose :created_at - expose :membership_state expose :last_owner?, as: :is_last_owner private @@ -21,25 +20,12 @@ def membership_type return 'project_invite' if user_in_array?(:shared_project_user_ids) end - def membership_state - has_any_active_membership? ? 'active' : 'awaiting' - end - - def has_any_active_membership? - user_in_array?(:group_member_user_ids) || - user_in_array?(:project_member_user_ids) || - user_in_array?(:shared_group_user_ids) || - user_in_array?(:shared_project_user_ids) - end - def last_owner? options[:group].last_owner?(object) end def removable - user_in_array?(:group_member_user_ids) || - user_in_array?(:project_member_user_ids) || - user_in_array?(:awaiting_user_ids) + user_in_array?(:group_member_user_ids) || user_in_array?(:project_member_user_ids) end def user_in_array?(name) diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb index 7cf995505f1370402b5ba1c1cd75a7564b4b369d..636d22c884cf703fe284d55db1a6f154864117fe 100644 --- a/ee/lib/ee/api/helpers/members_helpers.rb +++ b/ee/lib/ee/api/helpers/members_helpers.rb @@ -74,7 +74,7 @@ def present_member(updated_member) end def billable_member?(group, user) - billed_users_finder = BilledUsersFinder.new(group, include_awaiting_members: true) + billed_users_finder = BilledUsersFinder.new(group) users = billed_users_finder.execute[:users] users.include?(user) diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb index 27f492b5f1f6bac94990213db1e424d2378307a0..6b201d26255f0ae0824d03c3dd2ae2bf9ba2d8dc 100644 --- a/ee/lib/ee/api/members.rb +++ b/ee/lib/ee/api/members.rb @@ -118,7 +118,6 @@ module Members 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 - optional :include_awaiting_members, type: Grape::API::Boolean, desc: 'Determines if awaiting members are included', default: false end get ":id/billable_members", feature_category: :subgroups do group = find_group!(params[:id]) @@ -128,10 +127,7 @@ module Members sorting = params[:sort] || 'id_asc' - result = BilledUsersFinder.new(group, - search_term: params[:search], - order_by: sorting, - include_awaiting_members: params[:include_awaiting_members]).execute + result = BilledUsersFinder.new(group, search_term: params[:search], order_by: sorting).execute present paginate(result[:users]), with: ::EE::API::Entities::BillableMember, @@ -140,8 +136,7 @@ module Members group_member_user_ids: result[:group_member_user_ids], project_member_user_ids: result[:project_member_user_ids], shared_group_user_ids: result[:shared_group_user_ids], - shared_project_user_ids: result[:shared_project_user_ids], - awaiting_user_ids: result[:awaiting_user_ids] + shared_project_user_ids: result[:shared_project_user_ids] end desc 'Changes the state of the memberships of a user in the group' diff --git a/ee/spec/finders/billed_users_finder_spec.rb b/ee/spec/finders/billed_users_finder_spec.rb index 8c5711f594aad3ed3a7dc6873719ae042c3b9f71..d6092d672b580de0fa3a137a12b9e9f2b95be2a7 100644 --- a/ee/spec/finders/billed_users_finder_spec.rb +++ b/ee/spec/finders/billed_users_finder_spec.rb @@ -7,14 +7,11 @@ let(:search_term) { nil } let(:order_by) { nil } - let(:include_awaiting_members) { false } - subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by, include_awaiting_members: include_awaiting_members).execute } + subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by).execute } describe '#execute' do context 'without members' do - let(:include_awaiting_members) { true } - it 'returns an empty object' do expect(execute).to eq({}) end @@ -25,41 +22,6 @@ 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_it_be(:alice_awaiting) { create(:group_member, :awaiting, :developer, group: group, user: create(:user, name: 'Alice Waiting')) } - - shared_examples 'with awaiting members' do - context 'when awaiting users are included' do - let(:include_awaiting_members) { true } - - it 'includes awaiting users' do - expect(execute[:users]).to include(alice_awaiting.user) - end - end - - context 'when awaiting users are excluded' do - let(:include_awaiting_members) { false } - - it 'excludes awaiting users' do - expect(execute[:users]).not_to include(alice_awaiting.user) - end - end - end - - context 'when user is awaiting and active member' do - let_it_be(:project) { create(:project, group: group) } - - let(:include_awaiting_members) { true } - - before do - create(:project_member, :maintainer, user: alice_awaiting.user, source: project) - end - - it 'is only included once' do - expect(execute[:users]).to include(alice_awaiting.user).once - end - end - - it_behaves_like 'with awaiting members' context 'when a search parameter is provided' do let(:search_term) { 'John' } @@ -79,45 +41,32 @@ expect(execute[:users]).to eq([john_doe, john_smith].map(&:user)) end end - - context 'when searching for an awaiting user' do - let(:search_term) { 'Alice' } - - it_behaves_like 'with awaiting members' - end end context 'when a search parameter is not present' do - subject(:execute) { described_class.new(group, include_awaiting_members: include_awaiting_members).execute } + subject(:execute) { described_class.new(group).execute } it 'returns expected users in name asc order when a sorting is not provided either' do expect(execute[:users]).to eq([john_doe, john_smith, maria, sophie].map(&:user)) end - it_behaves_like 'with awaiting members' - context 'and when a sorting parameter is provided (eg name descending)' do let(:order_by) { 'name_desc' } - subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by, include_awaiting_members: include_awaiting_members).execute } + subject(:execute) { described_class.new(group, search_term: search_term, order_by: order_by).execute } it 'sorts results accordingly' do expect(execute[:users]).to eq([sophie, maria, john_smith, john_doe].map(&:user)) end - - context 'when awaiting users are included' do - let(:include_awaiting_members) { true } - - it 'sorts results accordingly' do - expect(execute[:users]).to eq([sophie, maria, john_smith, john_doe, alice_awaiting].map(&:user)) - end - end end end context 'with billable group members including shared members' do let_it_be(:shared_with_group_member) { create(:group_member, user: create(:user, name: 'Shared Group User')) } - let_it_be(:shared_with_project_member) { create(:group_member, user: create(:user, name: 'Shared Project User')) } + let_it_be(:shared_with_project_member) do + create(:group_member, user: create(:user, name: 'Shared Project User')) + end + let_it_be(:project) { create(:project, group: group) } before do @@ -126,18 +75,20 @@ end it 'returns a hash of users and user ids' do - expect(execute.keys).to eq([ + keys = [ :users, :group_member_user_ids, :project_member_user_ids, :shared_group_user_ids, - :shared_project_user_ids, - :awaiting_user_ids - ]) + :shared_project_user_ids + ] + + expect(execute.keys).to eq(keys) end it 'returns the correct user ids', :aggregate_failures do - expect(execute[:group_member_user_ids]).to contain_exactly(*[maria, john_smith, john_doe, sophie].map(&:user_id)) + expect(execute[:group_member_user_ids]) + .to contain_exactly(*[maria, john_smith, john_doe, sophie].map(&:user_id)) expect(execute[:shared_group_user_ids]).to contain_exactly(shared_with_group_member.user_id) expect(execute[:shared_project_user_ids]).to contain_exactly(shared_with_project_member.user_id) end diff --git a/ee/spec/lib/ee/api/entities/billable_member_spec.rb b/ee/spec/lib/ee/api/entities/billable_member_spec.rb index a886da6d6cbed4c9f9e81a8129e0fe2d9d1e2a55..2f7542d484f5655c411989de45fe496e6f380946 100644 --- a/ee/spec/lib/ee/api/entities/billable_member_spec.rb +++ b/ee/spec/lib/ee/api/entities/billable_member_spec.rb @@ -15,8 +15,7 @@ group_member_user_ids: [], project_member_user_ids: [], shared_group_user_ids: [], - shared_project_user_ids: [], - awaiting_user_ids: [] + shared_project_user_ids: [] } end @@ -51,32 +50,6 @@ end end - context 'membership_state' do - using RSpec::Parameterized::TableSyntax - - where(:key, :result) do - :group_member_user_ids | 'active' - :project_member_user_ids | 'active' - :shared_group_user_ids | 'active' - :shared_project_user_ids | 'active' - :awaiting_user_ids | 'awaiting' - end - - with_them do - let(:options) { super().merge(key => [user.id]) } - - it { expect(entity_representation[:membership_state]).to eq(result) } - end - - context 'with multiple states' do - let(:options) { super().merge(group_member_user_ids: [user.id], awaiting_user_ids: [user.id]) } - - it 'returns the expected membership status' do - expect(entity_representation[:membership_state]).to eq 'active' - end - end - end - context 'when the user has no public_email assigned' do before do user.update!(public_email: nil) @@ -98,7 +71,6 @@ :project_member_user_ids | 'project_member' | true :shared_group_user_ids | 'group_invite' | false :shared_project_user_ids | 'project_invite' | false - :awaiting_user_ids | nil | true end with_them do 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 e26507c747997fceba46acb6da0adba8344bb0c6..d55a4156b0dbcff0e0d790a9cb67acf5f0209894 100644 --- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb @@ -28,7 +28,7 @@ subject(:billable_member) { members_helpers.billable_member?(group, user) } before do - expect_next_instance_of(BilledUsersFinder, group, include_awaiting_members: true) do |finder| + expect_next_instance_of(BilledUsersFinder, group) do |finder| expect(finder).to receive(:execute).and_return({ users: found_users }) end end diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb index af86356300923ffb3fab647311ad70d3c1b99793..7db7f1c0286740a68e055dbc83732fd5f05ae8ee 100644 --- a/ee/spec/requests/api/members_spec.rb +++ b/ee/spec/requests/api/members_spec.rb @@ -382,8 +382,6 @@ end end - let_it_be(:awaiting_user) { create(:group_member, :awaiting, group: group, user: create(:user)).user } - describe 'GET /groups/:id/billable_members' do let(:url) { "/groups/#{group.id}/billable_members" } let(:params) { {} } @@ -484,16 +482,6 @@ expect_paginated_array_response(users[0].id) end end - - context 'with include_awaiting_members is true' do - let(:params) { { include_awaiting_members: true } } - - it 'includes awaiting users' do - get_billable_members - - expect_paginated_array_response(*[owner, maintainer, nested_user, awaiting_user, project_user, linked_group_user].map(&:id)) - end - end end context 'with non owner' do @@ -648,16 +636,6 @@ expect(json_response.map { |m| m['source_full_name'] }).to include(project.full_name) end - it 'includes awaiting memberships' do - membership = developer.members.first - membership.wait - - get api("/groups/#{group.id}/billable_members/#{developer.id}/memberships", owner) - - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.map { |m| m['id'] }).to include(membership.id) - end - it 'paginates results' do subgroup = create(:group, name: 'SubGroup A', parent: group) subgroup.add_developer(developer)