Merge branch '431288-unassign-users-on-membership-deletion' into 'master'

Leave unassignment notes for removed member

See merge request !145726



Merged-by: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com>
Approved-by: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com>
Approved-by: Tiger Watson <twatson@gitlab.com>
Reviewed-by: Tiger Watson <twatson@gitlab.com>
Reviewed-by: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com>
Co-authored-by: Eulyeon Ko <eko@gitlab.com>
Co-authored-by: DANGER_GITLAB_API_TOKEN <project_278964_bot_445da1b60fc7336b6b6776383134d10f@noreply.gitlab.com>

app/models/issue_assignee.rb

 # frozen_string_literal: true
 
 class IssueAssignee < ApplicationRecord
+  include EachBatch
+
   extend SuppressCompositePrimaryKeyWarning
 
   belongs_to :issue

app/models/merge_request_assignee.rb

 # frozen_string_literal: true
 
 class MergeRequestAssignee < ApplicationRecord
+  include EachBatch
+
   belongs_to :merge_request, touch: true
   belongs_to :assignee, class_name: "User", foreign_key: :user_id, inverse_of: :merge_request_assignees
 
   validates :assignee, uniqueness: { scope: :merge_request_id }
 
   scope :in_projects, ->(project_ids) { joins(:merge_request).where(merge_requests: { target_project_id: project_ids }) }
+  scope :for_assignee, ->(user) { where(assignee: user) }
 
   def cache_key
     [model_name.cache_key, id, assignee.cache_key]

app/services/members/destroy_service.rb

@@ -206,9 +206,15 @@ def destroy_bot_member_permission(member)
 
     def enqueue_unassign_issuables(member)
       source_type = member.is_a?(GroupMember) ? 'Group' : 'Project'
+      current_user_id = current_user.id
 
       member.run_after_commit_or_now do
-        MembersDestroyer::UnassignIssuablesWorker.perform_async(member.user_id, member.source_id, source_type)
+        MembersDestroyer::UnassignIssuablesWorker.perform_async(
+          member.user_id,
+          member.source_id,
+          source_type,
+          current_user_id
+        )
       end
     end
   end

app/services/members/unassign_issuables_service.rb

@@ -2,22 +2,85 @@
 
 module Members
   class UnassignIssuablesService
-    attr_reader :user, :entity
+    attr_reader :user, :entity, :requesting_user
 
-    def initialize(user, entity)
+    # @param [User] user user whose membership is being deleted from entity
+    # @param [Group, Project] entity
+    # @param [User] requesting_user user who initiated the membership deletion of `user`
+    def initialize(user, entity, requesting_user)
       @user = user
+      @requesting_user = requesting_user
       @entity = entity
     end
 
     def execute
+      if Feature.enabled?(:new_unassignment_service) && !requesting_user
+        raise ArgumentError, 'requesting_user must be given'
+      end
+
       return unless entity && user
 
       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
+      if Feature.enabled?(:new_unassignment_service)
+        unassign_from_issues(project_ids)
+        unassign_from_merge_requests(project_ids)
+      else
+        user.issue_assignees.on_issues(Issue.in_projects(project_ids).select(:id)).delete_all
+        user.merge_request_assignees.in_projects(project_ids).delete_all
+      end
 
       user.invalidate_cache_counts
     end
+
+    private
+
+    def unassign_from_issues(project_ids)
+      IssueAssignee
+        .for_assignee(user)
+        .in_projects(project_ids)
+        .each_batch(column: :issue_id) do |assignees|
+          assignees.each do |assignee|
+            issue = assignee.issue
+            next unless issue
+
+            Issues::UpdateService.new(
+              container: issue.project,
+              current_user: requesting_user,
+              params: { assignee_ids: new_assignee_ids(issue) }
+            ).execute(issue)
+
+          rescue ActiveRecord::StaleObjectError
+            # It's possible for `issue` to be stale (removed) by the time Issues::UpdateService attempts to update it.
+            # Continue to the next item.
+          end
+        end
+    end
+
+    def unassign_from_merge_requests(project_ids)
+      MergeRequestAssignee
+        .for_assignee(user)
+        .in_projects(project_ids)
+        .each_batch(column: :merge_request_id) do |assignees|
+          assignees.each do |assignee|
+            merge_request = assignee.merge_request
+            next unless merge_request
+
+            ::MergeRequests::UpdateAssigneesService.new(
+              project: merge_request.project,
+              current_user: requesting_user,
+              params: { assignee_ids: new_assignee_ids(merge_request) }
+            ).execute(merge_request)
+
+          rescue ActiveRecord::StaleObjectError
+            # It's possible for `merge_request` to be stale (removed) by the time
+            # MergeRequests::UpdateAssigneesService attempts to update it. Continue to the next item.
+          end
+        end
+    end
+
+    def new_assignee_ids(issuable)
+      issuable.assignees.map(&:id) - [user.id]
+    end
   end
 end

app/workers/members_destroyer/unassign_issuables_worker.rb

@@ -15,22 +15,40 @@ class UnassignIssuablesWorker
 
     idempotent!
 
-    def perform(user_id, entity_id, entity_type, _requesting_user_id = nil)
+    # Remove the default value `nil` for `requesting_user_id` after 16.10.
+    # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/442878
+    def perform(user_id, entity_id, entity_type, requesting_user_id = nil)
       unless ENTITY_TYPES.include?(entity_type)
         logger.error(
           message: "#{entity_type} is not a supported entity.",
           entity_type: entity_type,
           entity_id: entity_id,
-          user_id: user_id
+          user_id: user_id,
+          requesting_user_id: requesting_user_id
         )
 
         return
       end
 
+      if Feature.enabled?(:new_unassignment_service) && requesting_user_id.nil?
+        logger.error(
+          message: "requesting_user_id is nil.",
+          entity_type: entity_type,
+          entity_id: entity_id,
+          user_id: user_id,
+          requesting_user_id: requesting_user_id
+        )
+
+        return
+      end
+
+      # Remove the condition requesting_user_id after 16.10.
+      # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/442878
+      requesting_user = requesting_user_id && User.find(requesting_user_id)
       user = User.find(user_id)
       entity = entity_type.constantize.find(entity_id)
 
-      ::Members::UnassignIssuablesService.new(user, entity).execute
+      ::Members::UnassignIssuablesService.new(user, entity, requesting_user).execute
     end
   end
 end

config/feature_flags/gitlab_com_derisk/new_unassignment_service.yml

+---
+name: new_unassignment_service
+feature_issue_url:
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145726
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/443249
+milestone: '16.10'
+group: group::project management
+type: gitlab_com_derisk
+default_enabled: false

spec/models/merge_request_assignee_spec.rb

@@ -37,5 +37,19 @@
         expect(assignees.first.merge_request_id).to eq project_merge_request.merge_request_assignees.first.merge_request_id
       end
     end
+
+    context 'for_assignee' do
+      let_it_be(:another_user) { create(:user) }
+      let_it_be(:merge_request_assignee) { create(:merge_request, source_project: project, source_branch: 'another-branch-1', assignee_ids: [another_user.id]) }
+
+      let(:assignees) { described_class.for_assignee(user) }
+
+      it 'returns merge request assignees for a given assignee' do
+        expect(described_class.count).to eq 3
+        expect(assignees.count).to eq 2
+        expect(assignees.first.user_id).to eq user.id
+        expect(assignees.last.user_id).to eq user.id
+      end
+    end
   end
 end

spec/services/members/destroy_service_spec.rb

@@ -25,7 +25,7 @@
     before do
       type = member.is_a?(GroupMember) ? 'Group' : 'Project'
       expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type)
-      expect(MembersDestroyer::UnassignIssuablesWorker).to receive(:perform_async).with(member.user_id, member.source_id, type) if opts[:unassign_issuables]
+      expect(MembersDestroyer::UnassignIssuablesWorker).to receive(:perform_async).with(member.user_id, member.source_id, type, current_user.id) if opts[:unassign_issuables]
     end
 
     it 'destroys the member' do

spec/services/members/unassign_issuables_service_spec.rb

@@ -6,6 +6,7 @@
   let_it_be(:group) { create(:group, :private) }
   let_it_be(:project) { create(:project, group: group) }
   let_it_be(:user, reload: true) { create(:user) }
+  let_it_be(:requesting_user) { create(:user) }
   let_it_be(:assigned_issue1, reload: true) { create(:issue, project: project, assignees: [user]) }
   let_it_be(:assigned_issue2, reload: true) { create(:issue, project: project, assignees: [user]) }
 
@@ -13,15 +14,57 @@
   let!(:assigned_merge_request2) { create(:merge_request, :simple, :opened, target_project: project, source_project: project, assignees: [user], title: 'Test2') }
 
   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)
+    shared_examples 'missing request_user raises an error' do
+      context 'when requesting_user is nil' do
+        let_it_be(:requesting_user) { nil }
 
-        subject
+        it 'raises ArgumentError' do
+          expect { subject }.to raise_error(ArgumentError, 'requesting_user must be given')
+        end
+      end
+    end
+
+    shared_examples 'stale objects are ignored and skipped' do
+      context 'when Issues::UpdateService raises StaleObjectError' do
+        before do
+          allow_next_instance_of(Issues::UpdateService) do |instance|
+            allow(instance).to receive(:execute).and_raise(ActiveRecord::StaleObjectError)
+          end
+        end
+
+        it 'does not raise an error' do
+          expect { subject }.not_to raise_error
+        end
+      end
+
+      context 'when MergeRequests::UpdateAssigneesService raises StaleObjectError' do
+        before do
+          allow_next_instance_of(MergeRequests::UpdateAssigneesService) do |instance|
+            allow(instance).to receive(:execute).and_raise(ActiveRecord::StaleObjectError)
+          end
+        end
+
+        it 'does not raise an error' do
+          expect { subject }.not_to raise_error
+        end
+      end
+    end
+
+    RSpec.shared_examples 'unassignment events' do
+      # :sidekiq_inline is used b/c unlike issues, assignee changes for MRs get handled asynchronously.
+      it 'records unassignment events', :sidekiq_inline, :aggregate_failures do
+        expect { subject }
+          .to change { Note.where('note ILIKE ?', '%unassigned%').count }.by(
+            user.assigned_issues.count + user.assigned_merge_requests.count
+          )
+      end
+    end
 
-        expect(user.assigned_issues.count).to eq(0)
-        expect(user.assigned_merge_requests.count).to eq(0)
+    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 { subject }
+          .to change { user.assigned_issues.count }.from(issue_count).to(0)
+          .and change { user.assigned_merge_requests.count }.from(mr_count).to(0)
       end
 
       it 'invalidates user cache', :aggregate_failures, :clean_gitlab_redis_cache do
@@ -37,12 +80,26 @@
 
     context 'when a user leaves a project' do
       before do
+        project.add_maintainer(requesting_user)
         project.add_maintainer(user)
       end
 
-      subject { described_class.new(user, project).execute }
+      subject { described_class.new(user, project, requesting_user).execute }
 
       it_behaves_like 'un-assigning issuables', 2, 2, 2, 1
+      it_behaves_like 'unassignment events'
+      it_behaves_like 'missing request_user raises an error'
+      it_behaves_like 'stale objects are ignored and skipped'
+
+      context 'when the FF new_unassignment_service is disabled' do
+        let_it_be(:requesting_user) { nil }
+
+        before do
+          stub_feature_flags(new_unassignment_service: false)
+        end
+
+        it_behaves_like 'un-assigning issuables', 2, 2, 2, 1
+      end
     end
 
     context 'when a user leaves a group' do
@@ -55,12 +112,25 @@
       let!(:assigned_merge_request4) { create(:merge_request, :simple, :opened, target_project: project2, source_project: project2, assignees: [user], title: 'Test2') }
 
       before do
+        group.add_maintainer(requesting_user)
         group.add_maintainer(user)
       end
 
-      subject { described_class.new(user, group).execute }
+      subject { described_class.new(user, group, requesting_user).execute }
 
       it_behaves_like 'un-assigning issuables', 4, 4, 4, 2
+      it_behaves_like 'unassignment events'
+      it_behaves_like 'stale objects are ignored and skipped'
+
+      context 'when the FF new_unassignment_service is disabled' do
+        let_it_be(:requesting_user) { nil }
+
+        before do
+          stub_feature_flags(new_unassignment_service: false)
+        end
+
+        it_behaves_like 'un-assigning issuables', 4, 4, 4, 2
+      end
     end
   end
 end

spec/workers/members_destroyer/unassign_issuables_worker_spec.rb

@@ -5,23 +5,34 @@
 RSpec.describe MembersDestroyer::UnassignIssuablesWorker, feature_category: :user_management do
   let_it_be(:group) { create(:group, :private) }
   let_it_be(:user, reload: true) { create(:user) }
+  let_it_be(:requesting_user) { create(:user) }
 
   context 'when unsupported membership source entity' do
     it 'exits early and logs error' do
-      params = { message: "SomeEntity is not a supported entity.", entity_type: 'SomeEntity', entity_id: group.id, user_id: user.id }
+      params = { message: "SomeEntity is not a supported entity.", entity_type: 'SomeEntity', entity_id: group.id, user_id: user.id, requesting_user_id: requesting_user.id }
 
       expect(Sidekiq.logger).to receive(:error).with(params)
 
-      described_class.new.perform(user.id, group.id, 'SomeEntity')
+      described_class.new.perform(user.id, group.id, 'SomeEntity', requesting_user.id)
+    end
+  end
+
+  context 'when requesting_user_id is nil' do
+    it 'exits early and logs error' do
+      params = { message: "requesting_user_id is nil.", entity_type: 'Group', entity_id: group.id, user_id: user.id, requesting_user_id: nil }
+
+      expect(Sidekiq.logger).to receive(:error).with(params)
+
+      described_class.new.perform(user.id, group.id, 'Group', nil)
     end
   end
 
   it "calls the Members::UnassignIssuablesService with the params it was given" do
     service = double
 
-    expect(Members::UnassignIssuablesService).to receive(:new).with(user, group).and_return(service)
+    expect(Members::UnassignIssuablesService).to receive(:new).with(user, group, requesting_user).and_return(service)
     expect(service).to receive(:execute)
 
-    described_class.new.perform(user.id, group.id, 'Group')
+    described_class.new.perform(user.id, group.id, 'Group', requesting_user.id)
   end
 end