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

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

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
 

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

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

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

changelogs/unreleased/uassign_on_member_removing.yml

+---
+title: Unassign all Issues and Merge Requests when member leaves a team
+merge_request:
+author:

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

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)

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
 

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) }

spec/services/members/authorized_destroy_service_spec.rb

+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