Merge branch '20827-async-todo-creation-resolution' into 'master'

Resolve merge request todos asynchronously on update [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58647

app/services/merge_requests/resolve_todos_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class ResolveTodosService
+    include BaseServiceUtility
+
+    def initialize(merge_request, user)
+      @merge_request = merge_request
+      @user = user
+    end
+
+    def async_execute
+      if Feature.enabled?(:resolve_merge_request_todos_async, merge_request.target_project, default_enabled: :yaml)
+        MergeRequests::ResolveTodosWorker.perform_async(merge_request.id, user.id)
+      else
+        execute
+      end
+    end
+
+    def execute
+      todo_service.resolve_todos_for_target(merge_request, user)
+    end
+
+    private
+
+    attr_reader :merge_request, :user
+  end
+end

app/services/merge_requests/update_service.rb

@@ -147,7 +147,11 @@ module MergeRequests
     def resolve_todos(merge_request, old_labels, old_assignees, old_reviewers)
       return unless has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees, old_reviewers: old_reviewers)
 
-      todo_service.resolve_todos_for_target(merge_request, current_user)
+      service_user = current_user
+
+      merge_request.run_after_commit_or_now do
+        ::MergeRequests::ResolveTodosService.new(merge_request, service_user).async_execute
+      end
     end
 
     def handle_target_branch_change(merge_request)

app/workers/all_queues.yml

@@ -1908,6 +1908,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: merge_requests_resolve_todos
+  :feature_category: :code_review
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: metrics_dashboard_prune_old_annotations
   :feature_category: :metrics
   :has_external_dependencies:

app/workers/merge_requests/resolve_todos_worker.rb

+# frozen_string_literal: true
+
+class MergeRequests::ResolveTodosWorker
+  include ApplicationWorker
+
+  feature_category :code_review
+  urgency :high
+  deduplicate :until_executed
+  idempotent!
+
+  def perform(merge_request_id, user_id)
+    merge_request = MergeRequest.find(merge_request_id)
+    user = User.find(user_id)
+
+    MergeRequests::ResolveTodosService.new(merge_request, user).execute
+  rescue ActiveRecord::RecordNotFound
+  end
+end

changelogs/unreleased/20827-async-todo-creation-resolution.yml

+---
+title: Resolve merge request todos asynchronously on update
+merge_request: 58647
+author:
+type: performance

config/feature_flags/development/resolve_merge_request_todos_async.yml

+---
+name: resolve_merge_request_todos_async
+introduced_by_url:
+rollout_issue_url:
+milestone: '13.11'
+type: development
+group: group::code review
+default_enabled: false

config/sidekiq_queues.yml

@@ -218,6 +218,8 @@
   - 1
 - - merge_requests_handle_assignees_change
   - 1
+- - merge_requests_resolve_todos
+  - 1
 - - metrics_dashboard_prune_old_annotations
   - 1
 - - metrics_dashboard_sync_dashboards

spec/services/merge_requests/resolve_todos_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ResolveTodosService do
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:user) { create(:user) }
+
+  let(:service) { described_class.new(merge_request, user) }
+
+  describe '#async_execute' do
+    def async_execute
+      service.async_execute
+    end
+
+    it 'performs MergeRequests::ResolveTodosWorker asynchronously' do
+      expect(MergeRequests::ResolveTodosWorker)
+        .to receive(:perform_async)
+        .with(
+          merge_request.id,
+          user.id
+        )
+
+      async_execute
+    end
+
+    context 'when resolve_merge_request_todos_async feature is disabled' do
+      before do
+        stub_feature_flags(resolve_merge_request_todos_async: false)
+      end
+
+      it 'calls #execute' do
+        expect(service).to receive(:execute)
+
+        async_execute
+      end
+    end
+  end
+
+  describe '#execute' do
+    it 'marks pending todo as done' do
+      pending_todo = create(:todo, :pending, user: user, project: merge_request.project, target: merge_request)
+
+      service.execute
+
+      expect(pending_todo.reload).to be_done
+    end
+  end
+end

spec/services/merge_requests/update_service_spec.rb

@@ -590,48 +590,54 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       let!(:pending_todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) }
 
       context 'when the title change' do
-        before do
-          update_merge_request({ title: 'New title' })
-        end
+        it 'calls MergeRequest::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-        it 'marks pending todos as done' do
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ title: 'New title' })
         end
 
         it 'does not create any new todos' do
+          update_merge_request({ title: 'New title' })
+
           expect(Todo.count).to eq(1)
         end
       end
 
       context 'when the description change' do
-        before do
-          update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" })
-        end
+        it 'calls MergeRequest::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-        it 'marks pending todos as done' do
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" })
         end
 
         it 'creates only 1 new todo' do
+          update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" })
+
           expect(Todo.count).to eq(2)
         end
       end
 
       context 'when is reassigned' do
-        before do
-          update_merge_request({ assignee_ids: [user2.id] })
-        end
+        it 'calls MergeRequest::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-        it 'marks previous assignee pending todos as done' do
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ assignee_ids: [user2.id] })
         end
       end
 
       context 'when reviewers gets changed' do
-        it 'marks pending todo as done' do
-          update_merge_request({ reviewer_ids: [user2.id] })
+        it 'calls MergeRequest::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ reviewer_ids: [user2.id] })
         end
 
         it 'creates a pending todo for new review request' do
@@ -709,10 +715,12 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           end
         end
 
-        it 'marks pending todos as done' do
-          update_merge_request({ milestone: create(:milestone, project: project) })
+        it 'calls MergeRequests::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ milestone: create(:milestone, project: project) })
         end
 
         it 'sends notifications for subscribers of changed milestone', :sidekiq_might_not_need_inline do
@@ -726,17 +734,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when the labels change' do
-        before do
-          travel_to(1.minute.from_now) do
-            update_merge_request({ label_ids: [label.id] })
+        it 'calls MergeRequests::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
           end
-        end
 
-        it 'marks pending todos as done' do
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ label_ids: [label.id] })
         end
 
         it 'updates updated_at' do
+          travel_to(1.minute.from_now) do
+            update_merge_request({ label_ids: [label.id] })
+          end
+
           expect(merge_request.reload.updated_at).to be > Time.current
         end
       end
@@ -751,24 +761,26 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when the target branch change' do
-        before do
-          update_merge_request({ target_branch: 'target' })
-        end
+        it 'calls MergeRequests::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
 
-        it 'marks pending todos as done' do
-          expect(pending_todo.reload).to be_done
+          update_merge_request({ target_branch: 'target' })
         end
       end
 
       context 'when auto merge is enabled and target branch changed' do
         before do
           AutoMergeService.new(project, user, { sha: merge_request.diff_head_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
-
-          update_merge_request({ target_branch: 'target' })
         end
 
-        it 'marks pending todos as done' do
-          expect(pending_todo.reload).to be_done
+        it 'calls MergeRequests::ResolveTodosService#async_execute' do
+          expect_next_instance_of(MergeRequests::ResolveTodosService, merge_request, user) do |service|
+            expect(service).to receive(:async_execute)
+          end
+
+          update_merge_request({ target_branch: 'target' })
         end
       end
     end

spec/workers/merge_requests/resolve_todos_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::ResolveTodosWorker do
+  include AfterNextHelpers
+
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:user) { create(:user) }
+
+  let(:worker) { described_class.new }
+
+  it_behaves_like 'an idempotent worker' do
+    let(:job_args) { [merge_request.id, user.id] }
+  end
+
+  describe '#perform' do
+    it 'calls MergeRequests::ResolveTodosService#execute' do
+      expect_next(::MergeRequests::ResolveTodosService, merge_request, user)
+        .to receive(:execute)
+
+      worker.perform(merge_request.id, user.id)
+    end
+
+    context 'with a non-existing merge request' do
+      it 'does nothing' do
+        expect(::MergeRequests::ResolveTodosService).not_to receive(:new)
+
+        worker.perform(non_existing_record_id, user.id)
+      end
+    end
+
+    context 'with a non-existing user' do
+      it 'does nothing' do
+        expect(::MergeRequests::ResolveTodosService).not_to receive(:new)
+
+        worker.perform(merge_request.id, non_existing_record_id)
+      end
+    end
+  end
+end