From 26dc8ff9081d0588ab328f9c27fb320cfa79705a Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Mon, 21 Oct 2024 10:26:50 -0400 Subject: [PATCH 1/6] Resolve all todos when user submits an MR review --- app/services/draft_notes/publish_service.rb | 1 + app/services/todo_service.rb | 8 ++++++++ ee/app/services/ee/todo_service.rb | 2 ++ spec/services/draft_notes/publish_service_spec.rb | 8 ++++++++ spec/services/todo_service_spec.rb | 12 ++++++++++++ 5 files changed, 31 insertions(+) diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb index 5baaf0895f3804..28f65540ea7c04 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/app/services/draft_notes/publish_service.rb @@ -49,6 +49,7 @@ def publish_draft_notes(executing_user) keep_around_commits(created_notes) draft_notes.delete_all notification_service.async.new_review(review) + todo_service.new_review(review, current_user) MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request) GraphqlTriggers.merge_request_merge_status_updated(merge_request) after_publish diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 7b9092a1a5ff0b..6a3452c6330e68 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -187,6 +187,14 @@ def ssh_key_expired(ssh_keys) create_ssh_key_todos(ssh_keys, ::Todo::SSH_KEY_EXPIRED) end + # When a merge request receives a review + # + # * Mark all outstanding todos on this MR for the current user as done + # + def new_review(review, current_user) + resolve_todos_for_target(review.merge_request, current_user) + end + # When user marks a target as todo def mark_todo(target, current_user) project = target.project diff --git a/ee/app/services/ee/todo_service.rb b/ee/app/services/ee/todo_service.rb index c0299cfddff694..7cbd7a60a897ee 100644 --- a/ee/app/services/ee/todo_service.rb +++ b/ee/app/services/ee/todo_service.rb @@ -22,6 +22,8 @@ def merge_train_removed(merge_request) end end + # FIXME: Why isn't this called any more? + # def review_submitted(review) merge_request = review.merge_request diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index b78d66e7db3ec7..23c0e93e7468a8 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -142,6 +142,14 @@ def publish(draft: nil) publish end + it 'resolves todos for the MR' do + expect_next_instance_of(TodoService) do |todo_service| + expect(todo_service).to receive_message(:new_review).with(kind_of(Review), user) + end + + publish + end + it 'tracks the publish event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_publish_review_action) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 5a385a43f0ed63..a3e1697f33074b 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -1285,6 +1285,18 @@ should_not_create_todo(user: guest, target: mentioned_mr, action: Todo::MENTIONED) end end + + describe '#new_review' do + it 'marks related pending todos to the target MR for the user as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: mentioned_mr, author: author) + second_todo = create(:todo, :review_requested, user: john_doe, project: project, target: mentioned_mr, author: author) + review = Review.new(merge_request: mentioned_mr) + service.new_review(review, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + end end describe 'Designs' do -- GitLab From 9a41e4a422474b1e875f3865e023cc2b8a9e179c Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Mon, 21 Oct 2024 11:30:20 -0400 Subject: [PATCH 2/6] Fix publish service specs --- spec/services/draft_notes/publish_service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index 23c0e93e7468a8..1ff69e52e768cf 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -144,7 +144,7 @@ def publish(draft: nil) it 'resolves todos for the MR' do expect_next_instance_of(TodoService) do |todo_service| - expect(todo_service).to receive_message(:new_review).with(kind_of(Review), user) + expect(todo_service).to receive(:new_review).with(kind_of(Review), user) end publish @@ -251,7 +251,7 @@ def publish(draft: nil) recorder = ActiveRecord::QueryRecorder.new(skip_cached: false) { publish } - expect(recorder.count).not_to be > 112 + expect(recorder.count).not_to be > 116 end end -- GitLab From b62827c23f162acfa682a16c493d690caf1564b9 Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Mon, 21 Oct 2024 13:41:38 -0400 Subject: [PATCH 3/6] _Really_ fix the publish service specs --- spec/services/draft_notes/publish_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index 1ff69e52e768cf..1d400e3a8d87ca 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -143,7 +143,7 @@ def publish(draft: nil) end it 'resolves todos for the MR' do - expect_next_instance_of(TodoService) do |todo_service| + expect_any_instance_of(TodoService) do |todo_service| expect(todo_service).to receive(:new_review).with(kind_of(Review), user) end -- GitLab From 77aca7159a8066d085e461cdc09302b1b522422e Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Fri, 15 Nov 2024 15:03:04 +0000 Subject: [PATCH 4/6] Apply 1 suggestion(s) to 1 file(s) --- ee/app/services/ee/todo_service.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/app/services/ee/todo_service.rb b/ee/app/services/ee/todo_service.rb index 7cbd7a60a897ee..c0299cfddff694 100644 --- a/ee/app/services/ee/todo_service.rb +++ b/ee/app/services/ee/todo_service.rb @@ -22,8 +22,6 @@ def merge_train_removed(merge_request) end end - # FIXME: Why isn't this called any more? - # def review_submitted(review) merge_request = review.merge_request -- GitLab From aaef2190e6d06eb6e8f1064fb368ecb23421c929 Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Tue, 19 Nov 2024 14:13:01 -0500 Subject: [PATCH 5/6] Make #new_review spec data more clear --- spec/services/todo_service_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index a3e1697f33074b..e3f5d30816e3d9 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -1288,13 +1288,16 @@ describe '#new_review' do it 'marks related pending todos to the target MR for the user as done' do - first_todo = create(:todo, :assigned, user: john_doe, project: project, target: mentioned_mr, author: author) - second_todo = create(:todo, :review_requested, user: john_doe, project: project, target: mentioned_mr, author: author) + first_todo = create(:todo, :pending, :assigned, user: john_doe, project: project, target: mentioned_mr, author: author) + second_todo = create(:todo, :pending, :review_requested, user: john_doe, project: project, target: mentioned_mr, author: author) + third_todo = create(:todo, :pending, :mentioned, user: john_doe, project: project, target: mentioned_mr, author: author) + review = Review.new(merge_request: mentioned_mr) service.new_review(review, john_doe) expect(first_todo.reload).to be_done expect(second_todo.reload).to be_done + expect(third_todo.reload).to be_done end end end -- GitLab From 3106e6594d47c4d0eab86d9eedd06ab39ca22f9b Mon Sep 17 00:00:00 2001 From: Jeff Tucker Date: Thu, 21 Nov 2024 10:58:37 -0500 Subject: [PATCH 6/6] Add test case to cover behavior when multiple_todos FF is off --- spec/services/todo_service_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index e3f5d30816e3d9..04e552aa12fb80 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -1299,6 +1299,17 @@ expect(second_todo.reload).to be_done expect(third_todo.reload).to be_done end + + it 'marks related pending todo to the target MR for the user as done when the multiple_todos feature is off' do + stub_feature_flags(multiple_todos: false) + + only_todo = create(:todo, :pending, :assigned, user: john_doe, project: project, target: mentioned_mr, author: author) + + review = Review.new(merge_request: mentioned_mr) + service.new_review(review, john_doe) + + expect(only_todo.reload).to be_done + end end end -- GitLab