Merge branch 'uassign_on_member_removing' into 'master'

Unassign all Issues and Merge Requests when member leaves a team

Closes #30768 and #24117

See merge request !10755

11 files changed, 96 insertions, 29 deletions

diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index ec57fec4f99..0b13dbf5f8d 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -165,11 +165,8 @@ module IssuablesHelper
     html.html_safe
   end
 
-  def cached_assigned_issuables_count(assignee, issuable_type, state)
-    cache_key = hexdigest(['assigned_issuables_count', assignee.id, issuable_type, state].join('-'))
-    Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
-      assigned_issuables_count(assignee, issuable_type, state)
-    end
+  def assigned_issuables_count(issuable_type)
+    current_user.public_send("assigned_open_#{issuable_type}_count")
   end
 
   def issuable_filter_params
@@ -192,10 +189,6 @@ module IssuablesHelper
 
   private
 
-  def assigned_issuables_count(assignee, issuable_type, state)
-    assignee.public_send("assigned_#{issuable_type}").public_send(state).count
-  end
-
   def sidebar_gutter_collapsed?
     cookies[:collapsed_gutter] == 'true'
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 2d85bf8df26..774d4caa806 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -99,9 +99,6 @@ class User < ActiveRecord::Base
   has_many :award_emoji,              dependent: :destroy
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id
 
-  has_many :assigned_issues,          dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
-  has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
-
   # Issues that a user owns are expected to be moved to the "ghost" user before
   # the user is destroyed. If the user owns any issues during deletion, this
   # should be treated as an exceptional condition.
@@ -891,20 +888,20 @@ class User < ActiveRecord::Base
     @global_notification_setting
   end
 
-  def assigned_open_merge_request_count(force: false)
-    Rails.cache.fetch(['users', id, 'assigned_open_merge_request_count'], force: force) do
-      assigned_merge_requests.opened.count
+  def assigned_open_merge_requests_count(force: false)
+    Rails.cache.fetch(['users', id, 'assigned_open_merge_requests_count'], force: force) do
+      MergeRequestsFinder.new(self, assignee_id: self.id, state: 'opened').execute.count
     end
   end
 
   def assigned_open_issues_count(force: false)
     Rails.cache.fetch(['users', id, 'assigned_open_issues_count'], force: force) do
-      assigned_issues.opened.count
+      IssuesFinder.new(self, assignee_id: self.id, state: 'opened').execute.count
     end
   end
 
   def update_cache_counts
-    assigned_open_merge_request_count(force: true)
+    assigned_open_merge_requests_count(force: true)
     assigned_open_issues_count(force: true)
   end
 
diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb
index b7a244c2029..1711be7211c 100644
--- a/app/services/members/authorized_destroy_service.rb
+++ b/app/services/members/authorized_destroy_service.rb
@@ -9,7 +9,11 @@ module Members
     def execute
       return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
 
-      member.destroy
+      Member.transaction do
+        unassign_issues_and_merge_requests(member)
+
+        member.destroy
+      end
 
       if member.request? && member.user != user
         notification_service.decline_access_request(member)
@@ -17,5 +21,23 @@ module Members
 
       member
     end
+
+    private
+
+    def unassign_issues_and_merge_requests(member)
+      if member.is_a?(GroupMember)
+        IssuesFinder.new(user, group_id: member.source_id, assignee_id: member.user_id).
+          execute.
+          update_all(assignee_id: nil)
+        MergeRequestsFinder.new(user, group_id: member.source_id, assignee_id: member.user_id).
+          execute.
+          update_all(assignee_id: nil)
+      else
+        project = member.source
+        project.issues.opened.assigned_to(member.user).update_all(assignee_id: nil)
+        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
+        member.user.update_cache_counts
+      end
+    end
   end
 end
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index 66f75f1c2bf..659d548df18 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -47,13 +47,13 @@
             %li
               = link_to assigned_issues_dashboard_path, title: 'Issues', aria: { label: "Issues" }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do
                 = icon('hashtag fw')
-                - issues_count = cached_assigned_issuables_count(current_user, :issues, :opened)
+                - issues_count = assigned_issuables_count(:issues)
                 %span.badge.issues-count{ class: ('hidden' if issues_count.zero?) }
                   = number_with_delimiter(issues_count)
             %li
               = link_to assigned_mrs_dashboard_path, title: 'Merge requests', aria: { label: "Merge requests" }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do
                 = custom_icon('mr_bold')
-                - merge_requests_count = cached_assigned_issuables_count(current_user, :merge_requests, :opened)
+                - merge_requests_count = assigned_issuables_count(:merge_requests)
                 %span.badge.merge-requests-count{ class: ('hidden' if merge_requests_count.zero?) }
                   = number_with_delimiter(merge_requests_count)
             %li
diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml
index 444ecc414c0..ac222ad8c82 100644
--- a/app/views/layouts/nav/_dashboard.html.haml
+++ b/app/views/layouts/nav/_dashboard.html.haml
@@ -44,7 +44,7 @@
           I
       %span
         Issues
-      .badge= number_with_delimiter(cached_assigned_issuables_count(current_user, :issues, :opened))
+      .badge= number_with_delimiter(assigned_issuables_count(:issues))
   = nav_link(path: 'dashboard#merge_requests') do
     = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'dashboard-shortcuts-merge_requests' do
       .shortcut-mappings
@@ -53,7 +53,7 @@
           M
       %span
         Merge Requests
-      .badge= number_with_delimiter(cached_assigned_issuables_count(current_user, :merge_requests, :opened))
+      .badge= number_with_delimiter(assigned_issuables_count(:merge_requests))
   = nav_link(controller: 'dashboard/snippets') do
     = link_to dashboard_snippets_path, class: 'dashboard-shortcuts-snippets', title: 'Snippets' do
       .shortcut-mappings
diff --git a/changelogs/unreleased/uassign_on_member_removing.yml b/changelogs/unreleased/uassign_on_member_removing.yml
new file mode 100644
index 00000000000..cd60bdf5b3d
--- /dev/null
+++ b/changelogs/unreleased/uassign_on_member_removing.yml
@@ -0,0 +1,4 @@
+---
+title: Unassign all Issues and Merge Requests when member leaves a team
+merge_request:
+author:
diff --git a/doc/user/permissions.md b/doc/user/permissions.md
index 3122e95fc0e..637967510f3 100644
--- a/doc/user/permissions.md
+++ b/doc/user/permissions.md
@@ -7,6 +7,9 @@ project itself, the highest permission level is used.
 On public and internal projects the Guest role is not enforced. All users will
 be able to create issues, leave comments, and pull or download the project code.
 
+When a member leaves the team the all assigned Issues and Merge Requests
+will be unassigned automatically.
+
 GitLab administrators receive all permissions.
 
 To add or import a user, you can follow the [project users and members
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index d057c9cf6e9..11befd4edfe 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -361,7 +361,10 @@ describe Issue, models: true do
     it 'updates when assignees change' do
       user1 = create(:user)
       user2 = create(:user)
-      issue = create(:issue, assignee: user1)
+      project = create(:empty_project)
+      issue = create(:issue, assignee: user1, project: project)
+      project.add_developer(user1)
+      project.add_developer(user2)
 
       expect(user1.assigned_open_issues_count).to eq(1)
       expect(user2.assigned_open_issues_count).to eq(0)
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 90b3a2ba42d..415d3e7b200 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -820,15 +820,17 @@ describe MergeRequest, models: true do
       user1 = create(:user)
       user2 = create(:user)
       mr = create(:merge_request, assignee: user1)
+      mr.project.add_developer(user1)
+      mr.project.add_developer(user2)
 
-      expect(user1.assigned_open_merge_request_count).to eq(1)
-      expect(user2.assigned_open_merge_request_count).to eq(0)
+      expect(user1.assigned_open_merge_requests_count).to eq(1)
+      expect(user2.assigned_open_merge_requests_count).to eq(0)
 
       mr.assignee = user2
       mr.save
 
-      expect(user1.assigned_open_merge_request_count).to eq(0)
-      expect(user2.assigned_open_merge_request_count).to eq(1)
+      expect(user1.assigned_open_merge_requests_count).to eq(0)
+      expect(user2.assigned_open_merge_requests_count).to eq(1)
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 6af5ef1018c..0a2860f2505 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -24,9 +24,7 @@ describe User, models: true do
     it { is_expected.to have_many(:recent_events).class_name('Event') }
     it { is_expected.to have_many(:issues).dependent(:restrict_with_exception) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
-    it { is_expected.to have_many(:assigned_issues).dependent(:nullify) }
     it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
-    it { is_expected.to have_many(:assigned_merge_requests).dependent(:nullify) }
     it { is_expected.to have_many(:identities).dependent(:destroy) }
     it { is_expected.to have_many(:spam_logs).dependent(:destroy) }
     it { is_expected.to have_many(:todos).dependent(:destroy) }
diff --git a/spec/services/members/authorized_destroy_service_spec.rb b/spec/services/members/authorized_destroy_service_spec.rb
new file mode 100644
index 00000000000..3b35a3b8e3a
--- /dev/null
+++ b/spec/services/members/authorized_destroy_service_spec.rb
@@ -0,0 +1,45 @@
+require 'spec_helper'
+
+describe Members::AuthorizedDestroyService, services: true do
+  let(:member_user) { create(:user) }
+  let(:project) { create(:empty_project, :public) }
+  let(:group) { create(:group, :public) }
+  let(:group_project) { create(:empty_project, :public, group: group) }
+
+  def number_of_assigned_issuables(user)
+    Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
+  end
+
+  context 'Group member' do
+    it "unassigns issues and merge requests" do
+      group.add_developer(member_user)
+
+      issue = create :issue, project: group_project, assignee: member_user
+      create :issue, assignee: 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
+
+      member = group.members.find_by(user_id: member_user.id)
+
+      expect { described_class.new(member, member_user).execute }
+        .to change { number_of_assigned_issuables(member_user) }.from(4).to(2)
+
+      expect(issue.reload.assignee_id).to be_nil
+      expect(merge_request.reload.assignee_id).to be_nil
+    end
+  end
+
+  context 'Project member' do
+    it "unassigns issues and merge requests" do
+      project.team << [member_user, :developer]
+
+      create :issue, project: project, assignee: member_user
+      create :merge_request, target_project: project, source_project: project, assignee: member_user
+
+      member = project.members.find_by(user_id: member_user.id)
+
+      expect { described_class.new(member, member_user).execute }
+        .to change { number_of_assigned_issuables(member_user) }.from(2).to(0)
+    end
+  end
+end