Merge branch 'mr_api_todo_close' into 'master'

Closes todos for a merge request when the MR is accepted via the API by the MR assignee.

## Are there points in the code the reviewer needs to double check?

Please review refresh service test changes to see if they are correct - I think in those cases, the todos should actually be cleared instead of left pending.

## Why was this MR needed?

To make the API behavior consistent with the UI behavior (accepting your own MRs closes the todo item and prevents them from piling up).

Closes #22477

See merge request !6486

CHANGELOG

@@ -20,6 +20,7 @@ v 8.13.0 (unreleased)
   - Optimize GitHub importing for speed and memory
   - API: expose pipeline data in builds API (!6502, Guilherme Salazar)
   - Fix broken repository 500 errors in project list
+  - Close todos when accepting merge requests via the API !6486 (tonygambone)
 
 v 8.12.4 (unreleased)
 

app/controllers/projects/merge_requests_controller.rb

@@ -308,8 +308,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
-    TodoService.new.merge_merge_request(merge_request, current_user)
-
     @merge_request.update(merge_error: nil)
 
     if params[:merge_when_build_succeeds].present?

app/services/merge_requests/post_merge_service.rb

@@ -7,6 +7,7 @@ module MergeRequests
   class PostMergeService < MergeRequests::BaseService
     def execute(merge_request)
       close_issues(merge_request)
+      todo_service.merge_merge_request(merge_request, current_user)
       merge_request.mark_as_merged
       create_merge_event(merge_request, current_user)
       create_note(merge_request)

spec/services/merge_requests/merge_service_spec.rb

@@ -38,6 +38,30 @@ describe MergeRequests::MergeService, services: true do
       end
     end
 
+    context 'closes related todos' do
+      let(:merge_request) { create(:merge_request, assignee: user, author: user) }
+      let(:project) { merge_request.project }
+      let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
+      let!(:todo) do
+        create(:todo, :assigned,
+          project: project,
+          author: user,
+          user: user,
+          target: merge_request)
+      end
+
+      before do
+        allow(service).to receive(:execute_hooks)
+
+        perform_enqueued_jobs do
+          service.execute(merge_request)
+          todo.reload
+        end
+      end
+
+      it { expect(todo).to be_done }
+    end
+
     context 'remove source branch by author' do
       let(:service) do
         merge_request.merge_params['force_remove_source_branch'] = '1'

spec/services/merge_requests/refresh_service_spec.rb

@@ -79,8 +79,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_merged }
       it { expect(@fork_merge_request).to be_merged }
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
-      it { expect(@build_failed_todo).to be_pending }
-      it { expect(@fork_build_failed_todo).to be_pending }
+      it { expect(@build_failed_todo).to be_done }
+      it { expect(@fork_build_failed_todo).to be_done }
     end
 
     context 'manual merge of source branch' do
@@ -99,8 +99,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request.diffs.size).to be > 0 }
       it { expect(@fork_merge_request).to be_merged }
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
-      it { expect(@build_failed_todo).to be_pending }
-      it { expect(@fork_build_failed_todo).to be_pending }
+      it { expect(@build_failed_todo).to be_done }
+      it { expect(@fork_build_failed_todo).to be_done }
     end
 
     context 'push to fork repo source branch' do
@@ -149,8 +149,8 @@ describe MergeRequests::RefreshService, services: true do
       it { expect(@merge_request).to be_merged }
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
-      it { expect(@build_failed_todo).to be_pending }
-      it { expect(@fork_build_failed_todo).to be_pending }
+      it { expect(@build_failed_todo).to be_done }
+      it { expect(@fork_build_failed_todo).to be_done }
     end
 
     context 'push new branch that exists in a merge request' do