Merge branch '36720-add-option-to-unassign-issuables-on-user-leave' into 'master'

Allow to unassign Issues and Merge Requests when member leaves team

See merge request gitlab-org/gitlab!34388

app/controllers/concerns/membership_actions.rb

@@ -31,7 +31,10 @@ module MembershipActions
 
   def destroy
     member = membershipable.members_and_requesters.find(params[:id])
-    Members::DestroyService.new(current_user).execute(member)
+    # !! is used in case unassign_issuables contains empty string which would result in nil
+    unassign_issuables = !!ActiveRecord::Type::Boolean.new.cast(params.delete(:unassign_issuables))
+
+    Members::DestroyService.new(current_user).execute(member, unassign_issuables: unassign_issuables)
 
     respond_to do |format|
       format.html do

app/helpers/members_helper.rb

@@ -48,6 +48,14 @@ module MembersHelper
     "#{request.path}?#{options.to_param}"
   end
 
+  def member_path(member, unassign_issuables: false)
+    if member.is_a?(GroupMember)
+      group_group_member_path(member.source, member, { unassign_issuables: unassign_issuables })
+    else
+      project_project_member_path(member.source, member, { unassign_issuables: unassign_issuables })
+    end
+  end
+
   private
 
   def source_text(member)

app/models/issue_assignee.rb

@@ -2,9 +2,12 @@
 
 class IssueAssignee < ApplicationRecord
   belongs_to :issue
-  belongs_to :assignee, class_name: "User", foreign_key: :user_id
+  belongs_to :assignee, class_name: "User", foreign_key: :user_id, inverse_of: :issue_assignees
 
   validates :assignee, uniqueness: { scope: :issue_id }
+
+  scope :in_projects, ->(project_ids) { joins(:issue).where("issues.project_id in (?)", project_ids) }
+  scope :on_issues, ->(issue_ids) { where(issue_id: issue_ids) }
 end
 
 IssueAssignee.prepend_if_ee('EE::IssueAssignee')

app/models/merge_request_assignee.rb

@@ -2,7 +2,9 @@
 
 class MergeRequestAssignee < ApplicationRecord
   belongs_to :merge_request
-  belongs_to :assignee, class_name: "User", foreign_key: :user_id
+  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 in (?)", project_ids) }
 end

app/models/user.rb

@@ -163,9 +163,10 @@ class User < ApplicationRecord
   has_many :award_emoji,              dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent
 
-  has_many :issue_assignees
+  has_many :issue_assignees, inverse_of: :assignee
+  has_many :merge_request_assignees, inverse_of: :assignee
   has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue
-  has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
+  has_many :assigned_merge_requests, class_name: "MergeRequest", through: :merge_request_assignees, source: :merge_request
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
   has_many :callouts, class_name: 'UserCallout'

app/services/members/destroy_service.rb

@@ -2,7 +2,9 @@
 
 module Members
   class DestroyService < Members::BaseService
-    def execute(member, skip_authorization: false, skip_subresources: false)
+    WAIT_FOR_DELETE = 1.hour
+
+    def execute(member, skip_authorization: false, skip_subresources: false, unassign_issuables: false)
       raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member)
 
       @skip_auth = skip_authorization
@@ -19,6 +21,7 @@ module Members
 
       delete_subresources(member) unless skip_subresources
       enqueue_delete_todos(member)
+      enqueue_unassign_issuables(member) if unassign_issuables
 
       after_execute(member: member)
 
@@ -64,6 +67,14 @@ module Members
         raise "Unknown member type: #{member}!"
       end
     end
+
+    def enqueue_unassign_issuables(member)
+      source_type = member.is_a?(GroupMember) ? 'Group' : 'Project'
+
+      member.run_after_commit do
+        MembersDestroyer::UnassignIssuablesWorker.perform_in(WAIT_FOR_DELETE, member.user_id, member.source_id, source_type)
+      end
+    end
   end
 end
 

app/services/members/unassign_issuables_service.rb

+# frozen_string_literal: true
+
+module Members
+  class UnassignIssuablesService
+    attr_reader :user, :entity
+
+    def initialize(user, entity)
+      @user = user
+      @entity = entity
+    end
+
+    def execute
+      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
+
+      user.invalidate_cache_counts
+    end
+  end
+end

app/views/shared/members/_member.html.haml

@@ -118,7 +118,7 @@
                       data: { confirm: leave_confirmation_message(member.source) },
                       class: "btn btn-remove align-self-center m-0 #{'ml-sm-2' unless force_mobile_view}"
           - elsif !user&.project_bot?
-            = link_to member,
+            = link_to member_path(member.member),
                       method: :delete,
                       data: { confirm: remove_member_message(member), qa_selector: 'delete_member_button' },
                       class: "btn btn-remove align-self-center m-0 #{'ml-sm-2' unless force_mobile_view}",

app/workers/all_queues.yml

@@ -1115,6 +1115,14 @@
   :weight: 1
   :idempotent: 
   :tags: []
+- :name: unassign_issuables:members_destroyer_unassign_issuables
+  :feature_category: :authentication_and_authorization
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: update_namespace_statistics:namespaces_root_statistics
   :feature_category: :source_code_management
   :has_external_dependencies: 

app/workers/members_destroyer/unassign_issuables_worker.rb

+# frozen_string_literal: true
+
+module MembersDestroyer
+  class UnassignIssuablesWorker
+    include ApplicationWorker
+
+    ENTITY_TYPES = %w(Group Project).freeze
+
+    queue_namespace :unassign_issuables
+    feature_category :authentication_and_authorization
+
+    idempotent!
+
+    def perform(user_id, entity_id, entity_type)
+      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
+        )
+
+        return
+      end
+
+      user = User.find(user_id)
+      entity = entity_type.constantize.find(entity_id)
+
+      ::Members::UnassignIssuablesService.new(user, entity).execute
+    end
+  end
+end

changelogs/unreleased/36720-unassig-user-on-remove-option.yml

+---
+title: Extend members REST API with the option to unassign Issues and Merge Requests when member leaves team
+merge_request: 34388
+author:
+type: changed

config/sidekiq_queues.yml

@@ -262,6 +262,8 @@
   - 1
 - - todos_destroyer
   - 1
+- - unassign_issuables
+  - 1
 - - update_external_pull_requests
   - 3
 - - update_highest_role

doc/api/members.md

@@ -373,6 +373,7 @@ DELETE /projects/:id/members/:user_id
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the project or group](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `user_id` | integer | yes   | The user ID of the member |
+| `unassign_issuables` | boolean | false   | Flag indicating if the removed member should be unassigned from any issues or merge requests within given group or project |
 
 ```shell
 curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/members/:user_id"

lib/api/members.rb

@@ -145,6 +145,8 @@ module API
         desc 'Removes a user from a group or project.'
         params do
           requires :user_id, type: Integer, desc: 'The user ID of the member'
+          optional :unassign_issuables, type: Boolean, default: false,
+                   desc: 'Flag indicating if the removed member should be unassigned from any issues or merge requests within given group or project'
         end
         # rubocop: disable CodeReuse/ActiveRecord
         delete ":id/members/:user_id" do
@@ -152,7 +154,7 @@ module API
           member = source.members.find_by!(user_id: params[:user_id])
 
           destroy_conditionally!(member) do
-            ::Members::DestroyService.new(current_user).execute(member)
+            ::Members::DestroyService.new(current_user).execute(member, unassign_issuables: params[:unassign_issuables])
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

spec/models/issue_assignee_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe IssueAssignee do
+RSpec.describe IssueAssignee do
   let(:issue) { create(:issue) }
 
   subject { issue.issue_assignees.build(assignee: create(:user)) }
@@ -15,4 +15,37 @@ describe IssueAssignee do
   describe 'validations' do
     it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:issue_id) }
   end
+
+  describe 'scopes' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:project_issue) { create(:issue, project: project, assignee_ids: [user.id]) }
+
+    before do
+      issue.update!(assignee_ids: [user.id])
+    end
+
+    context 'in_projects' do
+      it 'returns issue assignees for given project' do
+        expect(IssueAssignee.count).to eq 2
+
+        assignees = IssueAssignee.in_projects([project])
+
+        expect(assignees.count).to eq 1
+        expect(assignees.first.user_id).to eq project_issue.issue_assignees.first.user_id
+        expect(assignees.first.issue_id).to eq project_issue.issue_assignees.first.issue_id
+      end
+    end
+
+    context 'on_issues' do
+      it 'returns issue assignees for given issues' do
+        expect(IssueAssignee.count).to eq 2
+
+        assignees = IssueAssignee.on_issues([project_issue])
+
+        expect(assignees.count).to eq 1
+        expect(assignees.first.issue_id).to eq project_issue.issue_assignees.first.issue_id
+      end
+    end
+  end
 end

spec/models/merge_request_assignee_spec.rb

@@ -15,4 +15,26 @@ describe MergeRequestAssignee do
   describe 'validations' do
     it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:merge_request_id) }
   end
+
+  describe 'scopes' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:project_merge_request) { create(:merge_request, target_project: project, source_project: project, assignee_ids: [user.id]) }
+
+    before do
+      merge_request.update!(assignee_ids: [user.id])
+    end
+
+    context 'in_projects' do
+      it 'returns issue assignees for given project' do
+        expect(MergeRequestAssignee.count).to eq 2
+
+        assignees = MergeRequestAssignee.in_projects([project])
+
+        expect(assignees.count).to eq 1
+        expect(assignees.first.user_id).to eq project_merge_request.merge_request_assignees.first.user_id
+        expect(assignees.first.merge_request_id).to eq project_merge_request.merge_request_assignees.first.merge_request_id
+      end
+    end
+  end
 end

spec/services/members/destroy_service_spec.rb

@@ -56,12 +56,23 @@ describe Members::DestroyService do
       expect(member_user.todos_pending_count).to be(1)
       expect(member_user.todos_done_count).to be(1)
 
-      described_class.new(current_user).execute(member, opts)
+      service = described_class.new(current_user)
+
+      if opts[:unassign_issuables]
+        expect(service).to receive(:enqueue_unassign_issuables).with(member)
+      end
+
+      service.execute(member, opts)
 
       expect(member_user.assigned_open_merge_requests_count).to be(0)
       expect(member_user.assigned_open_issues_count).to be(0)
       expect(member_user.todos_pending_count).to be(0)
       expect(member_user.todos_done_count).to be(0)
+
+      unless opts[:unassign_issuables]
+        expect(member_user.assigned_merge_requests.opened.count).to be(1)
+        expect(member_user.assigned_issues.opened.count).to be(1)
+      end
     end
   end
 
@@ -100,7 +111,7 @@ describe Members::DestroyService do
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
 
         it_behaves_like 'a service destroying a member with access' do
-          let(:opts) { { skip_authorization: true } }
+          let(:opts) { { skip_authorization: true, unassign_issuables: true } }
         end
       end
 
@@ -114,7 +125,7 @@ describe Members::DestroyService do
         it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError'
 
         it_behaves_like 'a service destroying a member with access' do
-          let(:opts) { { skip_authorization: true } }
+          let(:opts) { { skip_authorization: true, unassign_issuables: true } }
         end
       end
     end
@@ -133,6 +144,12 @@ describe Members::DestroyService do
         end
 
         it_behaves_like 'a service destroying a member with access'
+
+        context 'unassign issuables' do
+          it_behaves_like 'a service destroying a member with access' do
+            let(:opts) { { unassign_issuables: true } }
+          end
+        end
       end
 
       context 'with a group member' do
@@ -143,6 +160,12 @@ describe Members::DestroyService do
         end
 
         it_behaves_like 'a service destroying a member with access'
+
+        context 'unassign issuables' do
+          it_behaves_like 'a service destroying a member with access' do
+            let(:opts) { { unassign_issuables: true } }
+          end
+        end
       end
     end
   end

spec/services/members/unassign_issuables_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::UnassignIssuablesService do
+  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(:assigned_issue1, reload: true) { create(:issue, project: project, assignees: [user]) }
+  let_it_be(:assigned_issue2, reload: true) { create(:issue, project: project, assignees: [user]) }
+
+  let!(:assigned_merge_request1) { create(:merge_request, :simple, :closed, target_project: project, source_project: project, assignees: [user], title: 'Test1') }
+  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)
+
+        subject
+
+        expect(user.assigned_issues.count).to eq(0)
+        expect(user.assigned_merge_requests.count).to eq(0)
+      end
+
+      it 'invalidates user cache', :aggregate_failures, :clean_gitlab_redis_cache do
+        expect(user.assigned_open_merge_requests_count).to eq(open_mr_count)
+        expect(user.assigned_open_issues_count).to eq(open_issue_count)
+
+        subject
+
+        expect(user.assigned_open_merge_requests_count).to eq(0)
+        expect(user.assigned_open_issues_count).to eq(0)
+      end
+    end
+
+    context 'when a user leaves a project' do
+      before do
+        project.add_maintainer(user)
+      end
+
+      subject { described_class.new(user, project).execute }
+
+      it_behaves_like 'un-assigning issuables', 2, 2, 2, 1
+    end
+
+    context 'when a user leaves a group' do
+      let_it_be(:project2) { create(:project, group: group) }
+
+      let_it_be(:assigned_issue3, reload: true) { create(:issue, project: project2, assignees: [user]) }
+      let_it_be(:assigned_issue4, reload: true) { create(:issue, project: project2, assignees: [user]) }
+
+      let!(:assigned_merge_request3) { create(:merge_request, :simple, :closed, target_project: project2, source_project: project2, assignees: [user], title: 'Test1') }
+      let!(:assigned_merge_request4) { create(:merge_request, :simple, :opened, target_project: project2, source_project: project2, assignees: [user], title: 'Test2') }
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      subject { described_class.new(user, group).execute }
+
+      it_behaves_like 'un-assigning issuables', 4, 4, 4, 2
+    end
+  end
+end

spec/workers/members_destroyer/unassign_issuables_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MembersDestroyer::UnassignIssuablesWorker do
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:user, reload: true) { 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 }
+
+      expect(Sidekiq.logger).to receive(:error).with(params)
+
+      described_class.new.perform(user.id, group.id, 'SomeEntity')
+    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(service).to receive(:execute)
+
+    described_class.new.perform(user.id, group.id, 'Group')
+  end
+end