diff --git a/ee/app/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request.rb b/ee/app/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request.rb index 6947971ada67bbd5a9320479ff267a10eb23b725..f32fb12323026d09593fa3f593cbc3ef9b203829 100644 --- a/ee/app/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request.rb +++ b/ee/app/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request.rb @@ -23,14 +23,12 @@ def resolve(user_id:, status:) raise_resource_not_available_error! unless promotion_management_applicable? && current_user.can_admin_all_resources? - user = ::Gitlab::Graphql::Lazy.force(GitlabSchema.find_by_gid(user_id)) + user = Gitlab::Graphql::Lazy.force(GitlabSchema.find_by_gid(user_id)) result = ::GitlabSubscriptions::MemberManagement::ProcessUserBillablePromotionService .new(current_user, user, status).execute - return { result: :success, errors: [] } if result.success? - - { result: :failed, errors: result.errors } + { result: result.payload[:result], errors: result.errors } end end end diff --git a/ee/app/models/ee/members/member_approval.rb b/ee/app/models/ee/members/member_approval.rb index bf954c90049c8fde101b9f3b34166960a0a01a45..4fb8e93e2cf7cd2926263f1bee1683b7a3a294e2 100644 --- a/ee/app/models/ee/members/member_approval.rb +++ b/ee/app/models/ee/members/member_approval.rb @@ -7,6 +7,8 @@ module MemberApproval extend ActiveSupport::Concern prepended do + include EachBatch + validate :validate_unique_pending_approval, on: [:create, :update] scope :pending_member_approvals, ->(member_namespace_id) do diff --git a/ee/app/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service.rb b/ee/app/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service.rb index 1d35c17fd05ed44f1ac4272333d6a35fe0dcdccc..6c7faebfb75a1648a375ab879b9c9ade32915360 100644 --- a/ee/app/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service.rb +++ b/ee/app/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service.rb @@ -3,47 +3,131 @@ module GitlabSubscriptions module MemberManagement class ProcessUserBillablePromotionService < BaseService - include ::GitlabSubscriptions::MemberManagement::PromotionManagementUtils - - FAILED_TO_APPLY_PROMOTIONS = 'FAILED_TO_APPLY_PROMOTIONS' - - attr_reader :current_user + include GitlabSubscriptions::MemberManagement::PromotionManagementUtils def initialize(current_user, user, status) @current_user = current_user @user = user @status = status + @failed_member_approvals = [] + @successful_promotion_count = 0 end def execute - return error('Unauthorized') unless - current_user.present? && promotion_management_applicable? && current_user.can_admin_all_resources? + return error('Unauthorized') unless authorized? - apply_member_approvals - success - rescue ActiveRecord::RecordInvalid - error(FAILED_TO_APPLY_PROMOTIONS) + case status + when :denied + deny_member_approvals + when :approved + apply_member_approvals + else + error("Invalid #{status}") + end + rescue ActiveRecord::ActiveRecordError => e + Gitlab::AppLogger.error(message: "Failed to update member approval status to #{status}: #{e.message}") + Gitlab::ErrorTracking.track_exception(e) + error("Failed to update member approval status to #{status}") end private - attr_reader :user, :status + attr_reader :current_user, :user, :status + attr_accessor :failed_member_approvals, :successful_promotion_count + + def authorized? + current_user.present? && + promotion_management_applicable? && + current_user.can_admin_all_resources? + end def apply_member_approvals - ::Members::MemberApproval.pending_member_approvals_for_user(user.id).each do |member_approval| - member_approval.update!(status: status) + pending_approvals.find_each do |member_approval| + response = process_member_approval(member_approval) + + if response[:status] == :error + failed_member_approvals << member_approval + Gitlab::AppLogger.error(message: "Failed to apply pending promotions: #{response[:message]}") + else + member_approval.update!(status: :approved) + self.successful_promotion_count += 1 + end + end + + return error("Failed to apply promotions") if all_promotions_failed? + + approve_failed_member_approvals + success_status = failed_member_approvals.present? ? :partial_success : :success + + success(success_status) + end + + def process_member_approval(member_approval) + source = get_source_from_member_namespace(member_approval.member_namespace) + params = member_approval_params(member_approval, source) + + Members::InviteService.new(current_user, params).execute + end + + def pending_approvals + Members::MemberApproval.pending_member_approvals_for_user(user.id) + end + + def all_promotions_failed? + successful_promotion_count == 0 && failed_member_approvals.present? + end + + def member_approval_params(member_approval, source) + params = member_approval.metadata.symbolize_keys + params.merge!( + user_id: [user.id], + source: source, + access_level: member_approval.new_access_level, + invite_source: self.class.name + ) + end + + def get_source_from_member_namespace(member_namespace) + case member_namespace + when ::Namespaces::ProjectNamespace + member_namespace.project + when ::Group + member_namespace + end + end + + def deny_member_approvals + pending_approvals.each_batch do |batch| + batch.update_all(updated_at: Time.current, status: :denied) + end + + success + end + + def approve_failed_member_approvals + failed_member_approvals.each do |member_approval| + member_approval.update!(status: :approved) end end - def success - ServiceResponse.success(payload: { - user: user, - status: status - }) + def success(result = :success) + ServiceResponse.success( + message: "Successfully processed request", + payload: { + result: result, + user: user, + status: status + } + ) end def error(message) - ServiceResponse.error(message: message) + ServiceResponse.error( + message: message, + payload: { + result: :failed + } + ) end end end diff --git a/ee/spec/requests/api/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request_spec.rb b/ee/spec/requests/api/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request_spec.rb index a5d1d9c6189d7d493aa01c897c98a8ab92d5b97b..e21a6cd142a99a0c0a2a7c355f3c03c1728315d7 100644 --- a/ee/spec/requests/api/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/gitlab_subscriptions/member_management/process_user_billable_promotion_request_spec.rb @@ -7,9 +7,15 @@ let_it_be(:user) { create(:user) } let_it_be(:current_user) { create(:admin) } - let_it_be(:member_approval) { create(:member_approval, user: user) } let_it_be(:license) { create(:license, plan: License::ULTIMATE_PLAN) } + let(:group) { create(:group) } + let(:project) { create(:project) } + let(:project_namespace) { project.project_namespace } + let!(:member_approval) do + create(:member_approval, user: user, member_namespace: group, member: nil, old_access_level: nil) + end + let(:mutation) { graphql_mutation(:process_user_billable_promotion_request, input) } let(:action) { 'APPROVED' } let(:promotion_feature) { true } @@ -29,9 +35,7 @@ subject(:mutate) { post_graphql_mutation(mutation, current_user: current_user) } - context 'when called by a non-admin' do - let(:current_user) { create(:user) } - + shared_examples 'returns an error' do it 'returns an error' do mutate @@ -44,29 +48,144 @@ end end + context 'when called by a non-admin' do + let(:current_user) { create(:user) } + + include_examples 'returns an error' + end + context 'when promotion_management_applicable? returns false' do let(:promotion_feature) { false } - it 'returns an error' do - mutate - - expect(graphql_errors).to contain_exactly( - hash_including( - 'message' => "The resource that you are attempting to access does not exist or you don't have " \ - 'permission to perform this action' - ) - ) - end + include_examples 'returns an error' end context 'when pending request exists' do context 'when Approved' do - it 'approves pending requests' do - mutate + context 'with new invited user' do + it 'adds the user to the source' do + expect(group.members.map(&:user_id)).not_to include(user.id) + + mutate + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['result']).to eq("SUCCESS") + expect(group.reload.members.map(&:user_id)).to include(user.id) + expect(member_approval.reload.status).to eq("approved") + end + end - expect(member_approval.reload.status).to eq("approved") - expect(response).to have_gitlab_http_status(:success) - expect(mutation_response['result']).to eq("SUCCESS") + shared_examples 'updates the access_level of the existing member' do + it 'updates the access_level of the existing member' do + expect(existing_member.reload.access_level).to eq(Gitlab::Access::GUEST) + mutate + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['result']).to eq("SUCCESS") + expect(existing_member.reload.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(member_approval.reload.status).to eq("approved") + end + end + + shared_examples 'with multiple pending promotions' do + it 'invites and updates all pending requests' do + expect(source.reload.members.map(&:user_id)).not_to include(user.id) + expect(existing_member_in_another_src.access_level).to eq(Gitlab::Access::GUEST) + + mutate + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['result']).to eq("SUCCESS") + expect(source.reload.members.map(&:user_id)).to include(user.id) + expect(existing_member_in_another_src.reload.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(member_approval.reload.status).to eq("approved") + expect(another_member_approval.reload.status).to eq("approved") + end + + context 'when one promotion fails' do + before do + allow(Members::InviteService).to receive(:new).and_call_original + + params = member_approval.metadata.symbolize_keys + params.merge!( + user_id: [user.id], + source: source, + access_level: member_approval.new_access_level, + invite_source: "GitlabSubscriptions::MemberManagement::ProcessUserBillablePromotionService" + ) + + allow_next_instance_of(Members::InviteService, current_user, params) do |instance| + allow(instance).to receive(:execute).and_return(status: :error) + end + end + + it 'returns partial success' do + expect(source.reload.members.map(&:user_id)).not_to include(user.id) + expect(existing_member_in_another_src.access_level).to eq(Gitlab::Access::GUEST) + + mutate + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['result']).to eq("PARTIAL_SUCCESS") + expect(source.reload.members.map(&:user_id)).not_to include(user.id) + expect(member_approval.reload.status).to eq("approved") + expect(another_member_approval.reload.status).to eq("approved") + end + end + end + + context 'for group' do + let(:source) { group } + + context 'with existing member user' do + let!(:existing_member) { create(:group_member, :guest, group: group, user: user) } + + include_examples 'updates the access_level of the existing member' + end + + context 'with multiple pending requests' do + let(:another_group) { create(:group) } + let!(:existing_member_in_another_src) { create(:group_member, :guest, group: another_group, user: user) } + + let!(:another_member_approval) do + create(:member_approval, + user: user, + member_namespace: another_group, + member: existing_member_in_another_src, + old_access_level: 10 + ) + end + + it_behaves_like 'with multiple pending promotions' + end + end + + context 'for project' do + let(:source) { project } + let!(:member_approval) do + create(:member_approval, user: user, member_namespace: project_namespace, member: nil, old_access_level: nil) + end + + context 'with existing member user' do + let!(:existing_member) { create(:project_member, :guest, project: project, user: user) } + + include_examples 'updates the access_level of the existing member' + end + + context 'with multiple pending requests' do + let(:another_project) { create(:project) } + let(:another_project_namespace) { another_project.project_namespace } + let!(:existing_member_in_another_src) do + create(:project_member, :guest, project: another_project, user: user) + end + + let!(:another_member_approval) do + create(:member_approval, + user: user, + member_namespace: another_project_namespace, + member: existing_member_in_another_src, + old_access_level: 10 + ) + end + + it_behaves_like 'with multiple pending promotions' + end end end @@ -82,18 +201,20 @@ end end - context 'when an error is encountered' do + context 'when update! fails' do before do - allow(Members::MemberApproval).to receive(:pending_member_approvals_for_user).and_return([member_approval]) - allow(member_approval).to receive(:update!).and_raise( - ActiveRecord::RecordInvalid) + allow(Members::MemberApproval).to receive_message_chain(:pending_member_approvals_for_user, :find_each) + .and_yield(member_approval) + allow(member_approval).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) end - it 'returns an error' do + it 'returns failed' do mutate - expect(mutation_response["errors"]).to include("FAILED_TO_APPLY_PROMOTIONS") - expect(mutation_response["result"]).to eq("FAILED") + expect(member_approval.reload.status).to eq("pending") + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['result']).to eq("FAILED") + expect(mutation_response['errors']).to include("Failed to update member approval status to approved") end end end diff --git a/ee/spec/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service_spec.rb b/ee/spec/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service_spec.rb index 3a1bab336b0ee75e5a9dd13cc70783f4b7550d3d..9a6943575255a349ca3cffacfed8b271e046f523 100644 --- a/ee/spec/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service_spec.rb +++ b/ee/spec/services/gitlab_subscriptions/member_management/process_user_billable_promotion_service_spec.rb @@ -5,22 +5,33 @@ RSpec.describe GitlabSubscriptions::MemberManagement::ProcessUserBillablePromotionService, feature_category: :seat_cost_management do let_it_be(:current_user) { create(:admin) } let_it_be(:user) { create(:user) } - let_it_be(:status) { :approved } - let_it_be(:service) { described_class.new(current_user, user, status) } - let_it_be(:is_admin) { true } - let_it_be(:promotion_management_feature) { true } - let!(:member_approval_for_group) { create(:member_approval, :for_group_member, user: user) } + let_it_be(:license) { create(:license, plan: License::ULTIMATE_PLAN) } + let_it_be(:billable_member_role) { create(:member_role, :guest, namespace: nil, read_vulnerability: true) } + + let(:status) { :approved } + let(:service) { described_class.new(current_user, user, status) } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + let(:another_group) { create(:group) } + let!(:member_approval) do + create(:member_approval, :to_owner, + member_namespace: group, user: user, member: nil, old_access_level: nil) + end + + let!(:another_member_approval) do + create(:member_approval, :to_developer, + member_namespace: another_group, user: user, member: nil, old_access_level: nil) + end describe '#execute' do before do - allow(current_user).to receive(:can_admin_all_resources?).and_return(is_admin) if current_user - allow(service).to receive(:promotion_management_applicable?).and_return(promotion_management_feature) + allow(License).to receive(:current).and_return(license) + stub_feature_flags(member_promotion_management: true) + stub_application_setting(enable_member_promotion_management: true) end context 'when service is not allowed to execute' do - context 'when current_user is not present' do - let(:current_user) { nil } - + shared_examples 'unauthorized response' do it 'returns an error' do response = service.execute @@ -29,73 +40,207 @@ end end - context 'when promotion_management_applicable? returns false' do - let(:promotion_management_feature) { false } + context 'when current_user is not present' do + let(:current_user) { nil } - it 'returns an error' do - response = service.execute + it_behaves_like 'unauthorized response' + end - expect(response).to be_error - expect(response.message).to eq('Unauthorized') + context 'when promotion_management_applicable? returns false' do + before do + stub_feature_flags(member_promotion_management: false) end + + it_behaves_like 'unauthorized response' end context 'when current_user is not admin' do - let(:is_admin) { false } - - it 'returns an error' do - response = service.execute + let(:current_user) { create(:user) } - expect(response).to be_error - expect(response.message).to eq('Unauthorized') - end + it_behaves_like 'unauthorized response' end end - context 'when current_user is admin' do - context 'when there are pending member approvals' do - let!(:member_approval_for_another_project) do - create(:member_approval, :for_project_member, user: user) + context 'when current_user is admin', :enable_admin_mode do + context 'with all possible promotion scenarios' do + using RSpec::Parameterized::TableSyntax + where(:source, :existing_access_level, :to_new_access_level, :member_role, :new_access_level_val) do + :group | nil | :to_guest | :billable | Gitlab::Access::GUEST + :group | nil | :to_reporter | nil | Gitlab::Access::REPORTER + :group | nil | :to_developer | nil | Gitlab::Access::DEVELOPER + :group | nil | :to_maintainer | nil | Gitlab::Access::MAINTAINER + :group | nil | :to_owner | nil | Gitlab::Access::OWNER + :group | :guest | :to_guest | :billable | Gitlab::Access::GUEST + :group | :guest | :to_reporter | nil | Gitlab::Access::REPORTER + :group | :guest | :to_developer | nil | Gitlab::Access::DEVELOPER + :group | :guest | :to_maintainer | nil | Gitlab::Access::MAINTAINER + :group | :guest | :to_owner | nil | Gitlab::Access::OWNER + :project | nil | :to_guest | :billable | Gitlab::Access::GUEST + :project | nil | :to_reporter | nil | Gitlab::Access::REPORTER + :project | nil | :to_developer | nil | Gitlab::Access::DEVELOPER + :project | nil | :to_maintainer | nil | Gitlab::Access::MAINTAINER + :project | nil | :to_owner | nil | Gitlab::Access::OWNER + :project | :guest | :to_guest | :billable | Gitlab::Access::GUEST + :project | :guest | :to_reporter | nil | Gitlab::Access::REPORTER + :project | :guest | :to_developer | nil | Gitlab::Access::DEVELOPER + :project | :guest | :to_maintainer | nil | Gitlab::Access::MAINTAINER + :project | :guest | :to_owner | nil | Gitlab::Access::OWNER end - it 'updates the status of all pending member approvals' do - service.execute - - expect(member_approval_for_group.reload.status).to eq('approved') - expect(member_approval_for_another_project.reload.status).to eq('approved') + with_them do + let(:src) { source == :group ? group : project } + let(:member_namespace) { source == :group ? group : project.project_namespace } + let(:member_role_id) { billable_member_role.id if member_role == :billable } + let(:existing_member) do + next unless existing_access_level + + if source == :group + group.add_guest(user) + else + project.add_guest(user) + end + end + + let!(:member_approval) do + old_access_level = Gitlab::Access::GUEST if existing_access_level == :guest + + create( + :member_approval, + to_new_access_level, + old_access_level: old_access_level, + member_namespace: member_namespace, + member: existing_member, + member_role_id: member_role_id, + user: user + ) + end + + context 'when there are pending member approvals' do + context 'when admin approves' do + it 'applies all the promotions' do + expect(another_group.reload.members).to be_empty + + response = service.execute + + expect(response).to be_success + expect(response.message).to eq('Successfully processed request') + expect(response.payload).to eq({ user: user, status: status, result: :success }) + expect(member_approval.reload.status).to eq('approved') + expect(another_member_approval.reload.status).to eq('approved') + + member = src.reload.members.last + expect(member.user).to eq(user) + expect(member.access_level).to eq(new_access_level_val) + expect(member.member_role_id).to eq(member_role_id) + + another_membership = another_group.reload.members.last + expect(another_membership).not_to be_nil + expect(another_membership.user).to eq(user) + expect(another_membership.access_level).to eq(Gitlab::Access::DEVELOPER) + end + end + + context 'when admin denies' do + let(:status) { :denied } + + it 'updates the approval status' do + expect(::Members::InviteService).not_to receive(:new) + response = service.execute + + expect(response).to be_success + expect(response.message).to eq('Successfully processed request') + expect(response.payload).to eq({ user: user, status: status, result: :success }) + expect(member_approval.reload.status).to eq('denied') + expect(another_member_approval.reload.status).to eq('denied') + end + end + end end + end + + context 'when there are no pending member approvals' do + let(:member_approval) { nil } + let(:another_member_approval) { nil } it 'returns a success response' do response = service.execute expect(response).to be_success - expect(response.payload).to eq({ user: user, status: status }) + expect(response.payload).to eq({ user: user, status: status, result: :success }) end end - context 'when there are no pending member approvals' do - let(:member_approval_for_group) { nil } + context 'when there are partial success while applying' do + before do + allow(Members::InviteService).to receive(:new).and_call_original + + params = member_approval.metadata.symbolize_keys + params.merge!( + user_id: [user.id], + source: group, + access_level: member_approval.new_access_level, + invite_source: "GitlabSubscriptions::MemberManagement::ProcessUserBillablePromotionService" + ) + + allow_next_instance_of(Members::InviteService, current_user, params) do |instance| + allow(instance).to receive(:execute).and_return(status: :error) + end + end - it 'returns a success response' do + it 'returns a partial success response' do response = service.execute expect(response).to be_success - expect(response.payload).to eq({ user: user, status: status }) + expect(response.message).to eq('Successfully processed request') + expect(response.payload).to eq({ user: user, status: status, result: :partial_success }) + expect(member_approval.reload.status).to eq('approved') + expect(another_member_approval.reload.status).to eq('approved') end end - context 'when updating member approvals fails' do + context 'when all promotions fail while applying' do before do - allow(::Members::MemberApproval).to receive(:pending_member_approvals_for_user) - .and_return([member_approval_for_group]) - allow(member_approval_for_group).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + allow_next_instance_of(Members::InviteService) do |instance| + allow(instance).to receive(:execute).and_return(status: :error) + end end - it 'returns an error' do + it 'returns a failure response' do + response = service.execute + + expect(response).to be_error + expect(response.message).to eq('Failed to apply promotions') + expect(response.payload).to eq({ result: :failed }) + expect(member_approval.reload.status).to eq('pending') + expect(another_member_approval.reload.status).to eq('pending') + end + end + + context 'when there is failure during update!' do + before do + allow(Members::MemberApproval).to receive_message_chain(:pending_member_approvals_for_user, :find_each) + .and_yield(member_approval).and_yield(another_member_approval) + allow(member_approval).to receive(:update!).and_raise(ActiveRecord::RecordInvalid) + end + + it 'returns a failure response' do + response = service.execute + + expect(response).to be_error + expect(response.message).to eq("Failed to update member approval status to #{status}") + expect(response.payload).to eq({ result: :failed }) + end + end + + context 'when status is neither approved or denied' do + let(:status) { :pending } + + it 'returns a failure response' do response = service.execute expect(response).to be_error - expect(response.message).to eq('FAILED_TO_APPLY_PROMOTIONS') + expect(response.message).to eq("Invalid #{status}") + expect(response.payload).to eq({ result: :failed }) end end end diff --git a/spec/factories/member_approvals.rb b/spec/factories/member_approvals.rb index b3fbbca53daa1418ca67f799919ecbb21914e040..83e32ed6c1d6d686da228e08bef48ae8128f3d65 100644 --- a/spec/factories/member_approvals.rb +++ b/spec/factories/member_approvals.rb @@ -10,6 +10,8 @@ status { ::Members::MemberApproval.statuses[:pending] } member { association(:project_member, user: user) } member_namespace { association(:namespace) } + member_role_id { nil } + metadata { { access_level: new_access_level, member_role_id: member_role_id }.compact } trait :for_new_member do member { nil }