Improve Member services

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/admin/groups_controller.rb

@@ -50,7 +50,7 @@ class Admin::GroupsController < Admin::ApplicationController
 
   def members_update
     member_params = params.permit(:user_ids, :access_level, :expires_at)
-    result = Members::CreateService.new(@group, current_user, member_params.merge(limit: -1)).execute
+    result = Members::CreateService.new(current_user, member_params.merge(limit: -1)).execute(@group)
 
     if result[:status] == :success
       redirect_to [:admin, @group], notice: 'Users were successfully added.'

app/controllers/concerns/membership_actions.rb

@@ -3,33 +3,31 @@ module MembershipActions
 
   def create
     create_params = params.permit(:user_ids, :access_level, :expires_at)
-    result = Members::CreateService.new(membershipable, current_user, create_params).execute
-
-    redirect_url = members_page_url
+    result = Members::CreateService.new(current_user, create_params).execute(membershipable)
 
     if result[:status] == :success
-      redirect_to redirect_url, notice: 'Users were successfully added.'
+      redirect_to members_page_url, notice: 'Users were successfully added.'
     else
-      redirect_to redirect_url, alert: result[:message]
+      redirect_to members_page_url, alert: result[:message]
     end
   end
 
   def update
+    update_params = params.require(root_params_key).permit(:access_level, :expires_at)
     member = membershipable.members_and_requesters.find(params[:id])
-    @member = Members::UpdateService
-      .new(membershipable, current_user, member_params)
+    member = Members::UpdateService
+      .new(current_user, update_params)
       .execute(member)
       .present(current_user: current_user)
 
     respond_to do |format|
-      format.js { render 'shared/members/update' }
+      format.js { render 'shared/members/update', locals: { member: member } }
     end
   end
 
   def destroy
     member = membershipable.members_and_requesters.find(params[:id])
-    Members::DestroyService.new(membershipable, current_user, params)
-      .execute(member)
+    Members::DestroyService.new(current_user).execute(member)
 
     respond_to do |format|
       format.html do
@@ -51,7 +49,7 @@ module MembershipActions
   def approve_access_request
     access_requester = membershipable.requesters.find(params[:id])
     Members::ApproveAccessRequestService
-      .new(membershipable, current_user, params)
+      .new(current_user, params)
       .execute(access_requester)
 
     redirect_to members_page_url
@@ -59,8 +57,7 @@ module MembershipActions
 
   def leave
     member = membershipable.members_and_requesters.find_by!(user_id: current_user.id)
-    Members::DestroyService.new(membershipable, current_user)
-      .execute(member)
+    Members::DestroyService.new(current_user).execute(member)
 
     notice =
       if member.request?
@@ -79,17 +76,43 @@ module MembershipActions
     end
   end
 
+  def resend_invite
+    member = membershipable.members.find(params[:id])
+
+    if member.invite?
+      member.resend_invite
+
+      redirect_to members_page_url, notice: 'The invitation was successfully resent.'
+    else
+      redirect_to members_page_url, alert: 'The invitation has already been accepted.'
+    end
+  end
+
   protected
 
   def membershipable
     raise NotImplementedError
   end
 
+  def root_params_key
+    case membershipable
+    when Namespace
+      :group_member
+    when Project
+      :project_member
+    else
+      raise "Unknown membershipable type: #{membershipable}!"
+    end
+  end
+
   def members_page_url
-    if membershipable.is_a?(Project)
+    case membershipable
+    when Namespace
+      polymorphic_url([membershipable, :members])
+    when Project
       project_project_members_path(membershipable)
     else
-      polymorphic_url([membershipable, :members])
+      raise "Unknown membershipable type: #{membershipable}!"
     end
   end
 

app/controllers/groups/group_members_controller.rb

@@ -34,26 +34,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
     @group_member = @group.group_members.new
   end
 
-  def resend_invite
-    redirect_path = group_group_members_path(@group)
-
-    @group_member = @group.group_members.find(params[:id])
-
-    if @group_member.invite?
-      @group_member.resend_invite
-
-      redirect_to redirect_path, notice: 'The invitation was successfully resent.'
-    else
-      redirect_to redirect_path, alert: 'The invitation has already been accepted.'
-    end
-  end
-
-  protected
-
-  def member_params
-    params.require(:group_member).permit(:access_level, :user_id, :expires_at)
-  end
-
   # MembershipActions concern
   alias_method :membershipable, :group
 end

app/controllers/projects/project_members_controller.rb

@@ -26,20 +26,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @project_member = @project.project_members.new
   end
 
-  def resend_invite
-    redirect_path = project_project_members_path(@project)
-
-    @project_member = @project.project_members.find(params[:id])
-
-    if @project_member.invite?
-      @project_member.resend_invite
-
-      redirect_to redirect_path, notice: 'The invitation was successfully resent.'
-    else
-      redirect_to redirect_path, alert: 'The invitation has already been accepted.'
-    end
-  end
-
   def import
     @projects = current_user.authorized_projects.order_id_desc
   end
@@ -58,12 +44,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController
                 notice: notice)
   end
 
-  protected
-
-  def member_params
-    params.require(:project_member).permit(:user_id, :access_level, :expires_at)
-  end
-
   # MembershipActions concern
   alias_method :membershipable, :project
 end

app/models/concerns/access_requestable.rb

@@ -8,6 +8,6 @@ module AccessRequestable
   extend ActiveSupport::Concern
 
   def request_access(user)
-    Members::RequestAccessService.new(self, user).execute
+    Members::RequestAccessService.new(user).execute(self)
   end
 end

app/models/member.rb

@@ -139,17 +139,25 @@ class Member < ActiveRecord::Base
       member.attributes = {
         created_by: member.created_by || current_user,
         access_level: access_level,
-        expires_at: expires_at,
+        expires_at: expires_at
+      }
+
+      ## EE-only START
+      member.attributes = {
         skip_notification: ldap,
         ldap: ldap
       }
+      ## EE-only END
 
       if member.request?
         ::Members::ApproveAccessRequestService.new(
-          source,
           current_user,
           access_level: access_level
-        ).execute(member, ldap: ldap)
+        ).execute(
+          member,
+          skip_authorization: ldap,
+          skip_log_audit_event: ldap
+        )
       else
         member.save
       end

app/services/members/approve_access_request_service.rb

@@ -2,26 +2,21 @@ module Members
   class ApproveAccessRequestService < Members::BaseService
     prepend EE::Members::ApproveAccessRequestService
 
-    # opts - A hash of options
-    #   :ldap - The call is from a LDAP sync: current_user can be nil in that case
-    def execute(access_requester, opts = {})
-      raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester, opts[:ldap])
+    def execute(access_requester, skip_authorization: false, skip_log_audit_event: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_update_access_requester?(access_requester)
 
       access_requester.access_level = params[:access_level] if params[:access_level]
       access_requester.accept_request
 
-      after_execute(member: access_requester, **opts)
+      after_execute(member: access_requester, skip_log_audit_event: skip_log_audit_event)
 
       access_requester
     end
 
     private
 
-    def can_update_access_requester?(access_requester, ldap)
-      access_requester && (
-        ldap ||
-        can?(current_user, update_member_permission(access_requester), access_requester)
-      )
+    def can_update_access_requester?(access_requester)
+      can?(current_user, update_member_permission(access_requester), access_requester)
     end
   end
 end

app/services/members/authorized_destroy_service.rb

-module Members
-  class AuthorizedDestroyService < BaseService
-    def initialize(current_user = nil)
-      @current_user = current_user
-    end
-
-    def execute(member)
-      return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
-
-      Member.transaction do
-        unassign_issues_and_merge_requests(member) unless member.invite?
-        member.notification_setting&.destroy
-
-        member.destroy
-      end
-
-      if member.request? && member.user != current_user
-        notification_service.decline_access_request(member)
-      end
-
-      member
-    end
-
-    private
-
-    def unassign_issues_and_merge_requests(member)
-      if member.is_a?(GroupMember)
-        issues = Issue.unscoped.select(1)
-                 .joins(:project)
-                 .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
-          .execute
-          .update_all(assignee_id: nil)
-      else
-        project = member.source
-
-        # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
-        issues = Issue.unscoped.select(1)
-                 .where('issues.id = issue_assignees.issue_id')
-                 .where(project_id: project.id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
-      end
-
-      member.user.invalidate_cache_counts
-    end
-  end
-end

app/services/members/base_service.rb

 module Members
   class BaseService < ::BaseService
-    attr_accessor :source
-
-    # source - The source object that respond to `#members` (e.g. project or group)
     # current_user - The user that performs the action
     # params - A hash of parameters
-    def initialize(source, current_user, params = {})
-      @source = source
+    def initialize(current_user = nil, params = {})
       @current_user = current_user
       @params = params
     end
 
-    def after_execute(**args)
+    def after_execute(args)
       # overriden in EE::Members modules
     end
 

app/services/members/create_service.rb

@@ -4,7 +4,7 @@ module Members
 
     DEFAULT_LIMIT = 100
 
-    def execute
+    def execute(source)
       return error('No users specified.') if params[:user_ids].blank?
 
       user_ids = params[:user_ids].split(',').uniq
@@ -19,7 +19,7 @@ module Members
         current_user: current_user
       )
 
-      members.compact.each { |member| after_execute(member: member) }
+      members.each { |member| after_execute(member: member) }
 
       success
     end

app/services/members/destroy_service.rb

@@ -2,10 +2,21 @@ module Members
   class DestroyService < Members::BaseService
     prepend EE::Members::DestroyService
 
-    def execute(member)
-      raise Gitlab::Access::AccessDeniedError unless can_destroy_member?(member)
+    def execute(member, skip_authorization: false)
+      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
 
-      AuthorizedDestroyService.new(current_user).execute(member)
+      return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
+
+      Member.transaction do
+        unassign_issues_and_merge_requests(member) unless member.invite?
+        member.notification_setting&.destroy
+
+        member.destroy
+      end
+
+      if member.request? && member.user != current_user
+        notification_service.decline_access_request(member)
+      end
 
       after_execute(member: member)
 
@@ -15,7 +26,7 @@ module Members
     private
 
     def can_destroy_member?(member)
-      member && can?(current_user, destroy_member_permission(member), member)
+      can?(current_user, destroy_member_permission(member), member)
     end
 
     def destroy_member_permission(member)
@@ -28,5 +39,38 @@ module Members
         raise "Unknown member type: #{member}!"
       end
     end
+
+    def unassign_issues_and_merge_requests(member)
+      if member.is_a?(GroupMember)
+        issues = Issue.unscoped.select(1)
+                 .joins(:project)
+                 .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
+
+        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+        IssueAssignee.unscoped
+          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+          .delete_all
+
+        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
+          .execute
+          .update_all(assignee_id: nil)
+      else
+        project = member.source
+
+        # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
+        issues = Issue.unscoped.select(1)
+                 .where('issues.id = issue_assignees.issue_id')
+                 .where(project_id: project.id)
+
+        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
+        IssueAssignee.unscoped
+          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
+          .delete_all
+
+        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
+      end
+
+      member.user.invalidate_cache_counts
+    end
   end
 end

app/services/members/request_access_service.rb

 module Members
   class RequestAccessService < Members::BaseService
-    def execute
+    def execute(source)
       raise Gitlab::Access::AccessDeniedError unless can_request_access?(source)
 
       source.members.create(
@@ -12,7 +12,7 @@ module Members
     private
 
     def can_request_access?(source)
-      source && can?(current_user, :request_access, source)
+      can?(current_user, :request_access, source)
     end
   end
 end

app/services/members/update_service.rb

@@ -4,8 +4,7 @@ module Members
 
     # returns the updated member
     def execute(member, permission: :update)
-      permission_target = permission == :override ? source : member
-      raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), permission_target)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), member)
 
       old_access_level = member.human_access
 

app/views/shared/members/update.js.haml

+- member = local_assigns.fetch(:member)
+
 :plain
-  var $listItem = $('#{escape_javascript(render('shared/members/member', member: @member))}');
-  $("##{dom_id(@member)} .list-item-name").replaceWith($listItem.find('.list-item-name'));
-  gl.utils.localTimeAgo($('.js-timeago'), $("##{dom_id(@member)}"));
+  var $listItem = $('#{escape_javascript(render('shared/members/member', member: member))}');
+  $("##{dom_id(member)} .list-item-name").replaceWith($listItem.find('.list-item-name'));
+  gl.utils.localTimeAgo($('.js-timeago'), $("##{dom_id(member)}"));

app/workers/remove_expired_members_worker.rb

@@ -5,7 +5,7 @@ class RemoveExpiredMembersWorker
   def perform
     Member.expired.find_each do |member|
       begin
-        Members::AuthorizedDestroyService.new.execute(member)
+        Members::DestroyService.new.execute(member, skip_authorization: true)
       rescue => ex
         logger.error("Expired Member ID=#{member.id} cannot be removed - #{ex}")
       end

ee/app/controllers/ee/groups/group_members_controller.rb

@@ -6,7 +6,7 @@ module EE
       # rubocop:disable Gitlab/ModuleWithInstanceVariables
       def override
         member = @group.members.find_by!(id: params[:id])
-        updated_member = ::Members::UpdateService.new(@group, current_user, override_params)
+        updated_member = ::Members::UpdateService.new(current_user, override_params)
           .execute(member, permission: :override)
 
         if updated_member.valid?

ee/app/services/ee/members/approve_access_request_service.rb

 module EE
   module Members
     module ApproveAccessRequestService
-      def after_execute(member:, **opts)
+      def after_execute(member:, skip_log_audit_event: false)
         super
 
-        # Don't log this event in the case of a mass-approval, e.g. LDAP group-sync
-        log_audit_event(member: member) unless opts[:ldap]
+        log_audit_event(member: member) unless skip_log_audit_event
       end
 
       private
@@ -13,7 +12,7 @@ module EE
       def log_audit_event(member:)
         ::AuditEventService.new(
           current_user,
-          source,
+          member.source,
           action: :create
         ).for_member(member).security_event
       end

ee/app/services/ee/members/create_service.rb

@@ -12,7 +12,7 @@ module EE
       def log_audit_event(member:)
         ::AuditEventService.new(
           current_user,
-          source,
+          member.source,
           action: :create
         ).for_member(member).security_event
       end

ee/app/services/ee/members/destroy_service.rb

@@ -12,7 +12,7 @@ module EE
       def log_audit_event(member:)
         ::AuditEventService.new(
           current_user,
-          source,
+          member.source,
           action: :destroy
         ).for_member(member).security_event
       end

ee/app/services/ee/members/update_service.rb

@@ -14,7 +14,7 @@ module EE
       def log_audit_event(action:, old_access_level:, member:)
         ::AuditEventService.new(
           current_user,
-          source,
+          member.source,
           action: action,
           old_access_level: old_access_level
         ).for_member(member).security_event

lib/api/access_requests.rb

@@ -55,7 +55,7 @@ module API
 
           access_requester = source.requesters.find_by!(user_id: params[:user_id])
           member = ::Members::ApproveAccessRequestService
-            .new(source, current_user, declared_params)
+            .new(current_user, declared_params)
             .execute(access_requester)
 
           status :created
@@ -73,7 +73,7 @@ module API
           member = source.requesters.find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(source, current_user).execute(member)
+            ::Members::DestroyService.new(current_user).execute(member)
           end
         end
       end

lib/api/members.rb

@@ -83,11 +83,9 @@ module API
 
           member = source.members.find_by!(user_id: params[:user_id])
           updated_member =
-            ::Members::UpdateService.new(
-              source,
-              current_user,
-              declared_params(include_missing: false)
-            ).execute(member)
+            ::Members::UpdateService
+              .new(current_user, declared_params(include_missing: false))
+              .execute(member)
 
           if updated_member.valid?
             present updated_member, with: Entities::Member
@@ -105,7 +103,7 @@ module API
           member = source.members.find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(source, current_user).execute(member)
+            ::Members::DestroyService.new(current_user).execute(member)
           end
         end
       end

lib/api/v3/members.rb

@@ -124,7 +124,7 @@ module API
               status(200  )
               { message: "Access revoked", id: params[:user_id].to_i }
             else
-              ::Members::DestroyService.new(source, current_user).execute(member)
+              ::Members::DestroyService.new(current_user).execute(member)
 
               present member, with: ::API::Entities::Member
             end

spec/services/members/approve_access_request_service_spec.rb

@@ -6,28 +6,27 @@ describe Members::ApproveAccessRequestService do
   let(:current_user) { create(:user) }
   let(:access_requester_user) { create(:user) }
   let(:access_requester) { source.requesters.find_by!(user_id: access_requester_user.id) }
-  let(:params) { {} }
   let(:opts) { {} }
 
   shared_examples 'a service raising ActiveRecord::RecordNotFound' do
     it 'raises ActiveRecord::RecordNotFound' do
-      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { described_class.new(current_user).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(current_user).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service approving an access request' do
     it 'succeeds' do
-      expect { described_class.new(source, current_user, params).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
+      expect { described_class.new(current_user).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1)
     end
 
     it 'returns a <Source>Member' do
-      member = described_class.new(source, current_user, params).execute(access_requester, opts)
+      member = described_class.new(current_user).execute(access_requester, opts)
 
       expect(member).to be_a "#{source.class}Member".constantize
       expect(member.requested_at).to be_nil
@@ -35,7 +34,7 @@ describe Members::ApproveAccessRequestService do
 
     context 'with a custom access level' do
       it 'returns a ProjectMember with the custom access level' do
-        member = described_class.new(source, current_user, params.merge(access_level: Gitlab::Access::MASTER)).execute(access_requester, opts)
+        member = described_class.new(current_user, access_level: Gitlab::Access::MASTER).execute(access_requester, opts)
 
         expect(member.access_level).to eq(Gitlab::Access::MASTER)
       end
@@ -61,8 +60,8 @@ describe Members::ApproveAccessRequestService do
         end
       end
 
-      context 'and :ldap option is false' do
-        let(:opts) { { ldap: false } }
+      context 'and :skip_authorization option is false' do
+        let(:opts) { { skip_authorization: false } }
 
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
           let(:source) { project }
@@ -73,8 +72,8 @@ describe Members::ApproveAccessRequestService do
         end
       end
 
-      context 'and :ldap option is true' do
-        let(:opts) { { ldap: true } }
+      context 'and :skip_authorization option is true' do
+        let(:opts) { { skip_authorization: true } }
 
         it_behaves_like 'a service approving an access request' do
           let(:source) { project }
@@ -84,18 +83,6 @@ describe Members::ApproveAccessRequestService do
           let(:source) { group }
         end
       end
-
-      context 'and :ldap param is true' do
-        let(:params) { { ldap: true } }
-
-        it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-          let(:source) { project }
-        end
-
-        it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-          let(:source) { group }
-        end
-      end
     end
 
     context 'when current user cannot approve access request to the project' do

spec/services/members/authorized_destroy_service_spec.rb

-require 'spec_helper'
-
-describe Members::AuthorizedDestroyService do
-  let(:member_user) { create(:user) }
-  let(:project) { create(:project, :public) }
-  let(:group) { create(:group, :public) }
-  let(:group_project) { create(:project, :public, group: group) }
-
-  def number_of_assigned_issuables(user)
-    Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
-  end
-
-  context 'Invited users' do
-    # Regression spec for issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/32504
-    it 'destroys invited project member' do
-      project.add_developer(member_user)
-
-      member = create :project_member, :invited, project: project
-
-      expect { described_class.new(member_user).execute(member) }
-        .to change { Member.count }.from(3).to(2)
-    end
-
-    it "doesn't destroy invited project member notification_settings" do
-      project.add_developer(member_user)
-
-      member = create :project_member, :invited, project: project
-
-      expect { described_class.new(member_user).execute(member) }
-        .not_to change { NotificationSetting.count }
-    end
-
-    it 'destroys invited group member' do
-      group.add_developer(member_user)
-
-      member = create :group_member, :invited, group: group
-
-      expect { described_class.new(member_user).execute(member) }
-        .to change { Member.count }.from(2).to(1)
-    end
-
-    it "doesn't destroy invited group member notification_settings" do
-      group.add_developer(member_user)
-
-      member = create :group_member, :invited, group: group
-
-      expect { described_class.new(member_user).execute(member) }
-        .not_to change { NotificationSetting.count }
-    end
-  end
-
-  context 'Requested user' do
-    it "doesn't destroy member notification_settings" do
-      member = create(:project_member, user: member_user, requested_at: Time.now)
-
-      expect { described_class.new(member_user).execute(member) }
-        .not_to change { NotificationSetting.count }
-    end
-  end
-
-  context 'Group member' do
-    let(:member) { group.members.find_by(user_id: member_user.id) }
-
-    before do
-      group.add_developer(member_user)
-    end
-
-    it "unassigns issues and merge requests" do
-      issue = create :issue, project: group_project, assignees: [member_user]
-      create :issue, assignees: [member_user]
-      merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
-      create :merge_request, target_project: project, source_project: project, assignee: member_user
-
-      expect { described_class.new(member_user).execute(member) }
-        .to change { number_of_assigned_issuables(member_user) }.from(4).to(2)
-
-      expect(issue.reload.assignee_ids).to be_empty
-      expect(merge_request.reload.assignee_id).to be_nil
-    end
-
-    it 'destroys member notification_settings' do
-      group.add_developer(member_user)
-      member = group.members.find_by(user_id: member_user.id)
-
-      expect { described_class.new(member_user).execute(member) }
-        .to change { member_user.notification_settings.count }.by(-1)
-    end
-  end
-
-  context 'Project member' do
-    let(:member) { project.members.find_by(user_id: member_user.id) }
-
-    before do
-      project.add_developer(member_user)
-    end
-
-    it "unassigns issues and merge requests" do
-      create :issue, project: project, assignees: [member_user]
-      create :merge_request, target_project: project, source_project: project, assignee: member_user
-
-      expect { described_class.new(member_user).execute(member) }
-        .to change { number_of_assigned_issuables(member_user) }.from(2).to(0)
-    end
-
-    it 'destroys member notification_settings' do
-      expect { described_class.new(member_user).execute(member) }
-        .to change { member_user.notification_settings.count }.by(-1)
-    end
-  end
-end

spec/services/members/create_service_spec.rb

@@ -11,7 +11,7 @@ describe Members::CreateService do
 
   it 'adds user to members' do
     params = { user_ids: project_user.id.to_s, access_level: Gitlab::Access::GUEST }
-    result = described_class.new(project, user, params).execute
+    result = described_class.new(user, params).execute(project)
 
     expect(result[:status]).to eq(:success)
     expect(project.users).to include project_user
@@ -19,7 +19,7 @@ describe Members::CreateService do
 
   it 'adds no user to members' do
     params = { user_ids: '', access_level: Gitlab::Access::GUEST }
-    result = described_class.new(project, user, params).execute
+    result = described_class.new(user, params).execute(project)
 
     expect(result[:status]).to eq(:error)
     expect(result[:message]).to be_present
@@ -30,7 +30,7 @@ describe Members::CreateService do
     user_ids = 1.upto(101).to_a.join(',')
     params = { user_ids: user_ids, access_level: Gitlab::Access::GUEST }
 
-    result = described_class.new(project, user, params).execute
+    result = described_class.new(user, params).execute(project)
 
     expect(result[:status]).to eq(:error)
     expect(result[:message]).to be_present

spec/services/members/destroy_service_spec.rb

@@ -3,113 +3,200 @@ require 'spec_helper'
 describe Members::DestroyService do
   let(:current_user) { create(:user) }
   let(:member_user) { create(:user) }
-  let(:project) { create(:project, :public) }
   let(:group) { create(:group, :public) }
+  let(:group_project) { create(:project, :public, group: group) }
+  let(:opts) { {} }
 
   shared_examples 'a service raising ActiveRecord::RecordNotFound' do
     it 'raises ActiveRecord::RecordNotFound' do
-      expect { described_class.new(source, current_user).execute(member) }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { described_class.new(current_user).execute(member) }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(current_user).execute(member) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
+  def number_of_assigned_issuables(user)
+    Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
+  end
+
   shared_examples 'a service destroying a member' do
     it 'destroys the member' do
-      expect { described_class.new(source, current_user).execute(member) }.to change { source.members.count }.by(-1)
+      expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1)
+    end
+
+    it 'unassigns issues and merge requests' do
+      if member.invite?
+        expect { described_class.new(current_user).execute(member, opts) }
+          .not_to change { number_of_assigned_issuables(member_user) }
+      else
+        create :issue, assignees: [member_user]
+        issue = create :issue, project: group_project, assignees: [member_user]
+        merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
+
+        expect { described_class.new(current_user).execute(member, opts) }
+          .to change { number_of_assigned_issuables(member_user) }.from(3).to(1)
+
+        expect(issue.reload.assignee_ids).to be_empty
+        expect(merge_request.reload.assignee_id).to be_nil
+      end
+    end
+
+    it 'destroys member notification_settings' do
+      if member_user.notification_settings.any?
+        expect { described_class.new(current_user).execute(member, opts) }
+          .to change { member_user.notification_settings.count }.by(-1)
+      else
+        expect { described_class.new(current_user).execute(member, opts) }
+          .not_to change { member_user.notification_settings.count }
+      end
     end
   end
 
   shared_examples 'a service destroying an access requester' do
-    it 'destroys the access requester' do
-      expect { described_class.new(source, current_user).execute(access_requester) }.to change { source.requesters.count }.by(-1)
-    end
+    it_behaves_like 'a service destroying a member'
 
     it 'calls Member#after_decline_request' do
-      expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(access_requester)
+      expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member)
 
-      described_class.new(source, current_user).execute(access_requester)
+      described_class.new(current_user).execute(member)
     end
 
     context 'when current user is the member' do
       it 'does not call Member#after_decline_request' do
-        expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(access_requester)
+        expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member)
 
-        described_class.new(source, member_user).execute(access_requester)
+        described_class.new(member_user).execute(member)
       end
     end
   end
 
   context 'with a member' do
     before do
-      project.add_developer(member_user)
+      group_project.add_developer(member_user)
       group.add_developer(member_user)
     end
-    let(:member) { source.members.find_by(user_id: member_user.id) }
 
     context 'when current user cannot destroy the given member' do
       it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-        let(:source) { project }
+        let(:member) { group_project.members.find_by(user_id: member_user.id) }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { group_project.members.find_by(user_id: member_user.id) }
       end
 
       it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-        let(:source) { group }
+        let(:member) { group.members.find_by(user_id: member_user.id) }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { group.members.find_by(user_id: member_user.id) }
       end
     end
 
     context 'when current user can destroy the given member' do
       before do
-        project.add_master(current_user)
+        group_project.add_master(current_user)
         group.add_owner(current_user)
       end
 
       it_behaves_like 'a service destroying a member' do
-        let(:source) { project }
+        let(:member) { group_project.members.find_by(user_id: member_user.id) }
       end
 
       it_behaves_like 'a service destroying a member' do
-        let(:source) { group }
+        let(:member) { group.members.find_by(user_id: member_user.id) }
       end
     end
   end
 
   context 'with an access requester' do
     before do
-      project.update_attributes(request_access_enabled: true)
+      group_project.update_attributes(request_access_enabled: true)
       group.update_attributes(request_access_enabled: true)
-      project.request_access(member_user)
+      group_project.request_access(member_user)
       group.request_access(member_user)
     end
-    let(:access_requester) { source.requesters.find_by(user_id: member_user.id) }
 
     context 'when current user cannot destroy the given access requester' do
       it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-        let(:source) { project }
-        let(:member) { access_requester }
+        let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
       end
 
       it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
-        let(:source) { group }
-        let(:member) { access_requester }
+        let(:member) { group.requesters.find_by(user_id: member_user.id) }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { group.requesters.find_by(user_id: member_user.id) }
       end
     end
 
     context 'when current user can destroy the given access requester' do
       before do
-        project.add_master(current_user)
+        group_project.add_master(current_user)
         group.add_owner(current_user)
       end
 
       it_behaves_like 'a service destroying an access requester' do
-        let(:source) { project }
+        let(:member) { group_project.requesters.find_by(user_id: member_user.id) }
       end
 
       it_behaves_like 'a service destroying an access requester' do
-        let(:source) { group }
+        let(:member) { group.requesters.find_by(user_id: member_user.id) }
+      end
+    end
+  end
+
+  context 'with an invited user' do
+    let(:project_invited_member) { create(:project_member, :invited, project: group_project) }
+    let(:group_invited_member) { create(:group_member, :invited, group: group) }
+
+    context 'when current user cannot destroy the given invited user' do
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+        let(:member) { project_invited_member }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { project_invited_member }
+      end
+
+      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+        let(:member) { group_invited_member }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:opts) { { skip_authorization: true } }
+        let(:member) { group_invited_member }
+      end
+    end
+
+    context 'when current user can destroy the given invited user' do
+      before do
+        group_project.add_master(current_user)
+        group.add_owner(current_user)
+      end
+
+      # Regression spec for issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/32504
+      it_behaves_like 'a service destroying a member' do
+        let(:member) { project_invited_member }
+      end
+
+      it_behaves_like 'a service destroying a member' do
+        let(:member) { group_invited_member }
       end
     end
   end

spec/services/members/request_access_service_spec.rb

@@ -5,17 +5,17 @@ describe Members::RequestAccessService do
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+      expect { described_class.new(user).execute(source) }.to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service creating a access request' do
     it 'succeeds' do
-      expect { described_class.new(source, user).execute }.to change { source.requesters.count }.by(1)
+      expect { described_class.new(user).execute(source) }.to change { source.requesters.count }.by(1)
     end
 
     it 'returns a <Source>Member' do
-      member = described_class.new(source, user).execute
+      member = described_class.new(user).execute(source)
 
       expect(member).to be_a "#{source.class}Member".constantize
       expect(member.requested_at).to be_present

spec/services/members/update_service_spec.rb

@@ -13,14 +13,14 @@ describe Members::UpdateService do
 
   shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do
     it 'raises Gitlab::Access::AccessDeniedError' do
-      expect { described_class.new(source, current_user, params).execute(member, permission: permission) }
+      expect { described_class.new(current_user, params).execute(member, permission: permission) }
         .to raise_error(Gitlab::Access::AccessDeniedError)
     end
   end
 
   shared_examples 'a service updating a member' do
     it 'updates the member' do
-      updated_member = described_class.new(source, current_user, params).execute(member, permission: permission)
+      updated_member = described_class.new(current_user, params).execute(member, permission: permission)
 
       expect(updated_member).to be_valid
       expect(updated_member.access_level).to eq(Gitlab::Access::MASTER)