From dd6202dcbbde25adfbf19f9c922c9367677b5fcc Mon Sep 17 00:00:00 2001
From: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com>
Date: Wed, 31 Jan 2024 10:20:47 +0000
Subject: [PATCH] Use update services to unassign from issuables
When a namespace membership is removed with the option
to unassign the user enabled,
the unassignment event(s) were not recorded.
This commit updates UnassignIssuablesService to
utilize the existing update services to ensure
the unassignments are treated correctly.
Changelog: fixed
---
app/services/members/destroy_service.rb | 4 +--
.../members/unassign_issuables_service.rb | 34 +++++++++++++++++--
.../update_assignees_service.rb | 10 +++++-
ee/app/services/ee/members/destroy_service.rb | 4 +--
.../unassign_issuables_service_spec.rb | 16 ++++-----
.../update_assignees_service_spec.rb | 10 ++++++
6 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index 14cdb83b4e5d6..29a6c9995f9b8 100644
--- a/app/services/members/destroy_service.rb
+++ b/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
diff --git a/app/services/members/unassign_issuables_service.rb b/app/services/members/unassign_issuables_service.rb
index 95e07deb761c4..90fcd547fe431 100644
--- a/app/services/members/unassign_issuables_service.rb
+++ b/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
diff --git a/app/services/merge_requests/update_assignees_service.rb b/app/services/merge_requests/update_assignees_service.rb
index c1e970740b97f..32a47d9f2d8cb 100644
--- a/app/services/merge_requests/update_assignees_service.rb
+++ b/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
diff --git a/ee/app/services/ee/members/destroy_service.rb b/ee/app/services/ee/members/destroy_service.rb
index 1d9cd62eccc61..55866cac57de8 100644
--- a/ee/app/services/ee/members/destroy_service.rb
+++ b/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)
diff --git a/spec/services/members/unassign_issuables_service_spec.rb b/spec/services/members/unassign_issuables_service_spec.rb
index 9623cef868b5e..dc6f93f7be09d 100644
--- a/spec/services/members/unassign_issuables_service_spec.rb
+++ b/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
diff --git a/spec/services/merge_requests/update_assignees_service_spec.rb b/spec/services/merge_requests/update_assignees_service_spec.rb
index f9b0a993b06fb..2d7d7ad3ea15a 100644
--- a/spec/services/merge_requests/update_assignees_service_spec.rb
+++ b/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
--
GitLab