Merge branch '431288-user-unassignment-note' into 'master'

Use update services to unassign users on membership deletion

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143045



Merged-by: Marc Shaw <mshaw@gitlab.com>
Approved-by: Marc Shaw <mshaw@gitlab.com>
Approved-by: Michael Becker <11881043-wandering_person@users.noreply.gitlab.com>
Reviewed-by: Marc Shaw <mshaw@gitlab.com>
Reviewed-by: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com>
Reviewed-by: Michael Becker <11881043-wandering_person@users.noreply.gitlab.com>
Co-authored-by: Eulyeon Ko <eko@gitlab.com>

app/services/members/destroy_service.rb

@@ -47,10 +47,10 @@ def mark_as_recursive_call
     def enqueue_jobs_that_needs_to_be_run_only_once_per_hierarchy(member, unassign_issuables)
       return if recursive_call?
 
-      enqueue_cleanup_jobs_once_per_heirarchy(member, unassign_issuables)
+      enqueue_cleanup_jobs_once_per_hierarchy(member, unassign_issuables)
     end
 
-    def enqueue_cleanup_jobs_once_per_heirarchy(member, unassign_issuables)
+    def enqueue_cleanup_jobs_once_per_hierarchy(member, unassign_issuables)
       enqueue_delete_todos(member)
       enqueue_unassign_issuables(member) if unassign_issuables
     end

app/services/members/unassign_issuables_service.rb

@@ -14,10 +14,40 @@ def execute
 
       project_ids = entity.is_a?(Group) ? entity.all_projects.select(:id) : [entity.id]
 
-      user.issue_assignees.on_issues(Issue.in_projects(project_ids).select(:id)).delete_all
-      user.merge_request_assignees.in_projects(project_ids).delete_all
+      unassign_from_issues(project_ids)
+      unassign_from_merge_requests(project_ids)
 
       user.invalidate_cache_counts
     end
+
+    private
+
+    def unassign_from_issues(project_ids)
+      user.issue_assignees.on_issues(Issue.in_projects(project_ids)).select(:issue_id).each do |assignee|
+        issue = Issue.find(assignee.issue_id)
+
+        Issues::UpdateService.new(
+          container: issue.project,
+          current_user: user,
+          params: { assignee_ids: new_assignee_ids(issue) }
+        ).execute(issue)
+      end
+    end
+
+    def unassign_from_merge_requests(project_ids)
+      user.merge_request_assignees.in_projects(project_ids).select(:merge_request_id).each do |assignee|
+        merge_request = MergeRequest.find(assignee.merge_request_id)
+
+        ::MergeRequests::UpdateAssigneesService.new(
+          project: merge_request.project,
+          current_user: user,
+          params: { assignee_ids: new_assignee_ids(merge_request), skip_authorization: true }
+        ).execute(merge_request)
+      end
+    end
+
+    def new_assignee_ids(issuable)
+      issuable.assignees.map(&:id) - [user.id]
+    end
   end
 end

app/services/merge_requests/update_assignees_service.rb

@@ -2,12 +2,18 @@
 
 module MergeRequests
   class UpdateAssigneesService < UpdateService
+    def initialize(project:, current_user: nil, params: {})
+      super
+
+      @skip_authorization = @params.delete(:skip_authorization) || false
+    end
+
     # a stripped down service that only does what it must to update the
     # assignees, and knows that it does not have to check for other updates.
     # This saves a lot of queries for irrelevant things that cannot possibly
     # change in the execution of this service.
     def execute(merge_request)
-      return merge_request unless current_user&.can?(:set_merge_request_metadata, merge_request)
+      return merge_request unless current_user&.can?(:set_merge_request_metadata, merge_request) || skip_authorization
 
       old_assignees = merge_request.assignees.to_a
       old_ids = old_assignees.map(&:id)
@@ -33,6 +39,8 @@ def execute(merge_request)
 
     private
 
+    attr_reader :skip_authorization
+
     def assignee_ids
       filter_sentinel_values(params.fetch(:assignee_ids)).first(1)
     end

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

@@ -97,8 +97,8 @@ def cleanup_escalation_rules(member)
         ::IncidentManagement::EscalationRules::DestroyService.new(escalation_rules: rules, user: member.user).execute
       end
 
-      override :enqueue_cleanup_jobs_once_per_heirarchy
-      def enqueue_cleanup_jobs_once_per_heirarchy(member, unassign_issuables)
+      override :enqueue_cleanup_jobs_once_per_hierarchy
+      def enqueue_cleanup_jobs_once_per_hierarchy(member, unassign_issuables)
         super
 
         enqueue_cleanup_add_on_seat_assignments(member)

spec/services/members/unassign_issuables_service_spec.rb

@@ -14,14 +14,14 @@
 
   describe '#execute' do
     RSpec.shared_examples 'un-assigning issuables' do |issue_count, mr_count, open_issue_count, open_mr_count|
-      it 'removes issuable assignments', :aggregate_failures do
-        expect(user.assigned_issues.count).to eq(issue_count)
-        expect(user.assigned_merge_requests.count).to eq(mr_count)
-
-        subject
-
-        expect(user.assigned_issues.count).to eq(0)
-        expect(user.assigned_merge_requests.count).to eq(0)
+      # :sidekiq_inline is used b/c unlike issues, assignee changes for MRs get handled asynchronously.
+      it 'removes issuable assignments', :sidekiq_inline, :aggregate_failures do
+        expect { subject }
+          .to change { user.assigned_issues.count }.from(issue_count).to(0)
+          .and change { user.assigned_merge_requests.count }.from(mr_count).to(0)
+          .and change { Note.where('note ILIKE ?', '%unassigned%').count }.by(
+            user.assigned_issues.count + user.assigned_merge_requests.count
+          )
       end
 
       it 'invalidates user cache', :aggregate_failures, :clean_gitlab_redis_cache do

spec/services/merge_requests/update_assignees_service_spec.rb

@@ -143,6 +143,16 @@ def update_merge_request
         expect { update_merge_request }
           .not_to change { merge_request.reload.assignees.to_a }
       end
+
+      context 'when skip_authorization is set' do
+        let(:opts) { { assignee_ids: [user2.id], skip_authorization: true } }
+
+        it 'updates the MR assignees' do
+          expect { update_merge_request }
+            .to change { merge_request.reload.assignees }
+            .from([user3]).to([user2])
+        end
+      end
     end
   end
 end