Merge branch 'unassign-when-leaving' into 'master'

Don't delete todos or unassign issues and MRs when a user leaves a project

Closes #43899

See merge request gitlab-org/gitlab-ce!17615

app/models/member.rb

@@ -85,6 +85,7 @@ class Member < ActiveRecord::Base
   after_create :create_notification_setting, unless: [:pending?, :importing?]
   after_create :post_create_hook, unless: [:pending?, :importing?]
   after_update :post_update_hook, unless: [:pending?, :importing?]
+  after_destroy :destroy_notification_setting
   after_destroy :post_destroy_hook, unless: :pending?
   after_commit :refresh_member_authorized_projects
 
@@ -315,6 +316,10 @@ class Member < ActiveRecord::Base
     user.notification_settings.find_or_create_for(source)
   end
 
+  def destroy_notification_setting
+    notification_setting&.destroy
+  end
+
   def notification_setting
     @notification_setting ||= user&.notification_settings_for(source)
   end

app/models/members/project_member.rb

@@ -13,8 +13,6 @@ class ProjectMember < Member
 
   scope :in_project, ->(project) { where(source_id: project.id) }
 
-  before_destroy :delete_member_todos
-
   class << self
     # Add users to projects with passed access option
     #
@@ -93,10 +91,6 @@ class ProjectMember < Member
 
   private
 
-  def delete_member_todos
-    user.todos.where(project_id: source_id).destroy_all if user
-  end
-
   def send_invite
     notification_service.invite_project_member(self, @raw_invite_token)
 

app/models/user.rb

@@ -1035,14 +1035,33 @@ class User < ActiveRecord::Base
     end
   end
 
+  def todos_done_count(force: false)
+    Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: 20.minutes) do
+      TodosFinder.new(self, state: :done).execute.count
+    end
+  end
+
+  def todos_pending_count(force: false)
+    Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: 20.minutes) do
+      TodosFinder.new(self, state: :pending).execute.count
+    end
+  end
+
   def update_cache_counts
     assigned_open_merge_requests_count(force: true)
     assigned_open_issues_count(force: true)
   end
 
+  def update_todos_count_cache
+    todos_done_count(force: true)
+    todos_pending_count(force: true)
+  end
+
   def invalidate_cache_counts
     invalidate_issue_cache_counts
     invalidate_merge_request_cache_counts
+    invalidate_todos_done_count
+    invalidate_todos_pending_count
   end
 
   def invalidate_issue_cache_counts
@@ -1053,21 +1072,12 @@ class User < ActiveRecord::Base
     Rails.cache.delete(['users', id, 'assigned_open_merge_requests_count'])
   end
 
-  def todos_done_count(force: false)
-    Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: 20.minutes) do
-      TodosFinder.new(self, state: :done).execute.count
-    end
+  def invalidate_todos_done_count
+    Rails.cache.delete(['users', id, 'todos_done_count'])
   end
 
-  def todos_pending_count(force: false)
-    Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: 20.minutes) do
-      TodosFinder.new(self, state: :pending).execute.count
-    end
-  end
-
-  def update_todos_count_cache
-    todos_done_count(force: true)
-    todos_pending_count(force: true)
+  def invalidate_todos_pending_count
+    Rails.cache.delete(['users', id, 'todos_pending_count'])
   end
 
   # This is copied from Devise::Models::Lockable#valid_for_authentication?, as our auth

app/services/members/destroy_service.rb

@@ -5,12 +5,9 @@ module Members
 
       return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
 
-      Member.transaction do
-        unassign_issues_and_merge_requests(member) unless member.invite?
-        member.notification_setting&.destroy
-
       member.destroy
-      end
+
+      member.user&.invalidate_cache_counts
 
       if member.request? && member.user != current_user
         notification_service.decline_access_request(member)
@@ -37,38 +34,5 @@ module Members
         raise "Unknown member type: #{member}!"
       end
     end
-
-    def unassign_issues_and_merge_requests(member)
-      if member.is_a?(GroupMember)
-        issues = Issue.unscoped.select(1)
-                 .joins(:project)
-                 .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id)
-          .execute
-          .update_all(assignee_id: nil)
-      else
-        project = member.source
-
-        # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X
-        issues = Issue.unscoped.select(1)
-                 .where('issues.id = issue_assignees.issue_id')
-                 .where(project_id: project.id)
-
-        # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...)
-        IssueAssignee.unscoped
-          .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues)
-          .delete_all
-
-        project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil)
-      end
-
-      member.user.invalidate_cache_counts
-    end
   end
 end

changelogs/unreleased/unassign-when-leaving.yml

+---
+title: Don't delete todos or unassign issues and MRs when a user leaves a project
+merge_request:
+author:
+type: fixed

spec/models/members/project_member_spec.rb

@@ -45,14 +45,6 @@ describe ProjectMember do
     let(:project) { owner.project }
     let(:master)  { create(:project_member, project: project) }
 
-    let(:owner_todos)  { (0...2).map { create(:todo, user: owner.user, project: project) } }
-    let(:master_todos) { (0...3).map { create(:todo, user: master.user, project: project) } }
-
-    before do
-      owner_todos
-      master_todos
-    end
-
     it "creates an expired event when left due to expiry" do
       expired = create(:project_member, project: project, expires_at: Time.now - 6.days)
       expired.destroy
@@ -63,21 +55,6 @@ describe ProjectMember do
       master.destroy
       expect(Event.recent.first.action).to eq(Event::LEFT)
     end
-
-    it "destroys itself and delete associated todos" do
-      expect(owner.user.todos.size).to eq(2)
-      expect(master.user.todos.size).to eq(3)
-      expect(Todo.count).to eq(5)
-
-      master_todo_ids = master_todos.map(&:id)
-      master.destroy
-
-      expect(owner.user.todos.size).to eq(2)
-      expect(Todo.count).to eq(2)
-      master_todo_ids.each do |id|
-        expect(Todo.exists?(id)).to eq(false)
-      end
-    end
   end
 
   describe '.import_team' do

spec/services/members/destroy_service_spec.rb

@@ -19,32 +19,11 @@ describe Members::DestroyService do
     end
   end
 
-  def number_of_assigned_issuables(user)
-    Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count
-  end
-
   shared_examples 'a service destroying a member' do
     it 'destroys the member' do
       expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1)
     end
 
-    it 'unassigns issues and merge requests' do
-      if member.invite?
-        expect { described_class.new(current_user).execute(member, opts) }
-          .not_to change { number_of_assigned_issuables(member_user) }
-      else
-        create :issue, assignees: [member_user]
-        issue = create :issue, project: group_project, assignees: [member_user]
-        merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user
-
-        expect { described_class.new(current_user).execute(member, opts) }
-          .to change { number_of_assigned_issuables(member_user) }.from(3).to(1)
-
-        expect(issue.reload.assignee_ids).to be_empty
-        expect(merge_request.reload.assignee_id).to be_nil
-      end
-    end
-
     it 'destroys member notification_settings' do
       if member_user.notification_settings.any?
         expect { described_class.new(current_user).execute(member, opts) }
@@ -56,6 +35,29 @@ describe Members::DestroyService do
     end
   end
 
+  shared_examples 'a service destroying a member with access' do
+    it_behaves_like 'a service destroying a member'
+
+    it 'invalidates cached counts for todos and assigned issues and merge requests', :aggregate_failures do
+      create(:issue, project: group_project, assignees: [member_user])
+      create(:merge_request, source_project: group_project, assignee: member_user)
+      create(:todo, :pending, project: group_project, user: member_user)
+      create(:todo, :done, project: group_project, user: member_user)
+
+      expect(member_user.assigned_open_merge_requests_count).to be(1)
+      expect(member_user.assigned_open_issues_count).to be(1)
+      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)
+
+      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)
+    end
+  end
+
   shared_examples 'a service destroying an access requester' do
     it_behaves_like 'a service destroying a member'
 
@@ -74,29 +76,39 @@ describe Members::DestroyService do
     end
   end
 
-  context 'with a member' do
+  context 'with a member with access' do
     before do
-      group_project.add_developer(member_user)
-      group.add_developer(member_user)
+      group_project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
+      group.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
     end
 
     context 'when current user cannot destroy the given member' do
-      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      context 'with a project member' do
         let(:member) { group_project.members.find_by(user_id: member_user.id) }
+
+        before do
+          group_project.add_developer(member_user)
         end
 
-      it_behaves_like 'a service destroying a member' 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(:member) { group_project.members.find_by(user_id: member_user.id) }
+        end
       end
 
-      it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do
+      context 'with a group member' do
         let(:member) { group.members.find_by(user_id: member_user.id) }
+
+        before do
+          group.add_developer(member_user)
         end
 
-      it_behaves_like 'a service destroying a member' 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(:member) { group.members.find_by(user_id: member_user.id) }
+        end
       end
     end
 
@@ -106,12 +118,24 @@ describe Members::DestroyService do
         group.add_owner(current_user)
       end
 
-      it_behaves_like 'a service destroying a member' do
+      context 'with a project member' do
         let(:member) { group_project.members.find_by(user_id: member_user.id) }
+
+        before do
+          group_project.add_developer(member_user)
         end
 
-      it_behaves_like 'a service destroying a member' do
+        it_behaves_like 'a service destroying a member with access'
+      end
+
+      context 'with a group member' do
         let(:member) { group.members.find_by(user_id: member_user.id) }
+
+        before do
+          group.add_developer(member_user)
+        end
+
+        it_behaves_like 'a service destroying a member with access'
       end
     end
   end