Merge branch '325689-delete-issuable-todos-async' into 'master'

Delete all issuable todos asynchronously when issuable is destroyed [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57830

app/models/concerns/issuable.rb

@@ -65,7 +65,7 @@ module Issuable
 
     has_many :label_links, as: :target, dependent: :destroy, inverse_of: :target # rubocop:disable Cop/ActiveRecordDependent
     has_many :labels, through: :label_links
-    has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :todos, as: :target
 
     has_one :metrics, inverse_of: model_name.singular.to_sym, autosave: true
 

app/models/user.rb

@@ -1668,8 +1668,7 @@ class User < ApplicationRecord
   def invalidate_cache_counts
     invalidate_issue_cache_counts
     invalidate_merge_request_cache_counts
-    invalidate_todos_done_count
-    invalidate_todos_pending_count
+    invalidate_todos_cache_counts
     invalidate_personal_projects_count
   end
 
@@ -1682,11 +1681,8 @@ class User < ApplicationRecord
     Rails.cache.delete(['users', id, 'review_requested_open_merge_requests_count'])
   end
 
-  def invalidate_todos_done_count
+  def invalidate_todos_cache_counts
     Rails.cache.delete(['users', id, 'todos_done_count'])
-  end
-
-  def invalidate_todos_pending_count
     Rails.cache.delete(['users', id, 'todos_pending_count'])
   end
 

app/services/issuable/destroy_service.rb

@@ -3,11 +3,25 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      TodoService.new.destroy_target(issuable) do |issuable|
-        if issuable.destroy
-          issuable.update_project_counter_caches
-          issuable.assignees.each(&:invalidate_cache_counts)
-        end
+      if issuable.destroy
+        delete_todos(issuable)
+        issuable.update_project_counter_caches
+        issuable.assignees.each(&:invalidate_cache_counts)
+      end
+    end
+
+    private
+
+    def delete_todos(issuable)
+      actor = issuable.is_a?(Epic) ? issuable.resource_parent : issuable.resource_parent.group
+
+      if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml)
+        TodosDestroyer::DestroyedIssuableWorker
+          .perform_async(issuable.id, issuable.class.name)
+      else
+        TodosDestroyer::DestroyedIssuableWorker
+          .new
+          .perform(issuable.id, issuable.class.name)
       end
     end
   end

app/services/todos/destroy/destroyed_issuable_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    class DestroyedIssuableService
+      BATCH_SIZE = 100
+
+      def initialize(target_id, target_type)
+        @target_id = target_id
+        @target_type = target_type
+      end
+
+      def execute
+        inner_query = Todo.select(:id).for_target(target_id).for_type(target_type).limit(BATCH_SIZE)
+
+        delete_query = <<~SQL
+        DELETE FROM "#{Todo.table_name}"
+        WHERE id IN (#{inner_query.to_sql})
+        RETURNING user_id
+        SQL
+
+        loop do
+          result = ActiveRecord::Base.connection.execute(delete_query)
+
+          break if result.cmd_tuples == 0
+
+          user_ids = result.map { |row| row['user_id'] }.uniq
+
+          invalidate_todos_cache_counts(user_ids)
+        end
+      end
+
+      private
+
+      attr_reader :target_id, :target_type
+
+      def invalidate_todos_cache_counts(user_ids)
+        user_ids.each do |id|
+          # Only build a user instance since we only need its ID for
+          # `User#invalidate_todos_cache_counts` to work.
+          User.new(id: id).invalidate_todos_cache_counts
+        end
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1411,6 +1411,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: todos_destroyer:todos_destroyer_destroyed_issuable
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: todos_destroyer:todos_destroyer_entity_leave
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/todos_destroyer/destroyed_issuable_worker.rb

+# frozen_string_literal: true
+
+module TodosDestroyer
+  class DestroyedIssuableWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    idempotent!
+
+    def perform(target_id, target_type)
+      ::Todos::Destroy::DestroyedIssuableService.new(target_id, target_type).execute
+    end
+  end
+end

changelogs/unreleased/325689-delete-issuable-todos-async.yml

+---
+title: Delete all issuable todos asynchronously when issuable is destroyed
+merge_request: 57830
+author:
+type: performance

config/feature_flags/development/destroy_issuable_todos_async.yml

+---
+name: destroy_issuable_todos_async
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57830
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325689
+milestone: '13.11'
+type: development
+group: group::code review
+default_enabled: false

spec/controllers/projects/issues_controller_spec.rb

@@ -1518,12 +1518,6 @@ RSpec.describe Projects::IssuesController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for issue' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -728,12 +728,6 @@ RSpec.describe Projects::MergeRequestsController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for merge request' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/models/concerns/issuable_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Issuable do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
-    it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:todos) }
     it { is_expected.to have_many(:labels) }
     it { is_expected.to have_many(:note_authors).through(:notes) }
 

spec/services/issuable/destroy_service_spec.rb

@@ -9,6 +9,32 @@ RSpec.describe Issuable::DestroyService do
   subject(:service) { described_class.new(project, user) }
 
   describe '#execute' do
+    shared_examples_for 'service deleting todos' do
+      it 'destroys associated todos asynchronously' do
+        expect(TodosDestroyer::DestroyedIssuableWorker)
+          .to receive(:perform_async)
+          .with(issuable.id, issuable.class.name)
+
+        subject.execute(issuable)
+      end
+
+      context 'when destroy_issuable_todos_async feature is disabled' do
+        before do
+          stub_feature_flags(destroy_issuable_todos_async: false)
+        end
+
+        it 'destroy associated todos synchronously' do
+          expect_next_instance_of(TodosDestroyer::DestroyedIssuableWorker) do |worker|
+            expect(worker)
+              .to receive(:perform)
+              .with(issuable.id, issuable.class.name)
+          end
+
+          subject.execute(issuable)
+        end
+      end
+    end
+
     context 'when issuable is an issue' do
       let!(:issue) { create(:issue, project: project, author: user, assignees: [user]) }
 
@@ -22,17 +48,14 @@ RSpec.describe Issuable::DestroyService do
         service.execute(issue)
       end
 
-      it 'updates the todo caches for users with todos on the issue' do
-        create(:todo, target: issue, user: user, author: user, project: project)
-
-        expect { service.execute(issue) }
-          .to change { user.todos_pending_count }.from(1).to(0)
-      end
-
       it 'invalidates the issues count cache for the assignees' do
         expect_any_instance_of(User).to receive(:invalidate_cache_counts).once
         service.execute(issue)
       end
+
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { issue }
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -53,11 +76,8 @@ RSpec.describe Issuable::DestroyService do
         service.execute(merge_request)
       end
 
-      it 'updates the todo caches for users with todos on the merge request' do
-        create(:todo, target: merge_request, user: user, author: user, project: project)
-
-        expect { service.execute(merge_request) }
-          .to change { user.todos_pending_count }.from(1).to(0)
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { merge_request }
       end
     end
   end

spec/services/todos/destroy/destroyed_issuable_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::DestroyedIssuableService do
+  describe '#execute' do
+    let_it_be(:target) { create(:merge_request) }
+    let_it_be(:pending_todo) { create(:todo, :pending, project: target.project, target: target, user: create(:user)) }
+    let_it_be(:done_todo) { create(:todo, :done, project: target.project, target: target, user: create(:user)) }
+
+    def execute
+      described_class.new(target.id, target.class.name).execute
+    end
+
+    it 'deletes todos for specified target ID and type' do
+      control_count = ActiveRecord::QueryRecorder.new { execute }.count
+
+      # Create more todos for the target
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+
+      expect { execute }.not_to exceed_query_limit(control_count)
+      expect(target.reload.todos.count).to eq(0)
+    end
+
+    it 'invalidates todos cache counts of todo users', :use_clean_rails_redis_caching do
+      expect { execute }
+        .to change { pending_todo.user.todos_pending_count }.from(1).to(0)
+        .and change { done_todo.user.todos_done_count }.from(1).to(0)
+    end
+  end
+end

spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TodosDestroyer::DestroyedIssuableWorker do
+  let(:job_args) { [1, 'MergeRequest'] }
+
+  it 'calls the Todos::Destroy::DestroyedIssuableService' do
+    expect_next_instance_of(::Todos::Destroy::DestroyedIssuableService, *job_args) do |service|
+      expect(service).to receive(:execute)
+    end
+
+    described_class.new.perform(*job_args)
+  end
+end