Merge branch 'sy-fix-alert-todos' into 'master'

Do not create additional alert todo for already-assigned user

See merge request gitlab-org/gitlab!39397

app/services/alert_management/alerts/update_service.rb

@@ -96,12 +96,12 @@ module AlertManagement
       end
 
       def handle_assignement(old_assignees)
-        assign_todo
+        assign_todo(old_assignees)
         add_assignee_system_note(old_assignees)
       end
 
-      def assign_todo
-        todo_service.assign_alert(alert, current_user)
+      def assign_todo(old_assignees)
+        todo_service.reassigned_assignable(alert, current_user, old_assignees)
       end
 
       def add_assignee_system_note(old_assignees)

app/services/issues/update_service.rb

@@ -43,7 +43,7 @@ module Issues
       if issue.assignees != old_assignees
         create_assignee_note(issue, old_assignees)
         notification_service.async.reassigned_issue(issue, current_user, old_assignees)
-        todo_service.reassigned_issuable(issue, current_user, old_assignees)
+        todo_service.reassigned_assignable(issue, current_user, old_assignees)
       end
 
       if issue.previous_changes.include?('confidential')

app/services/merge_requests/update_service.rb

@@ -105,7 +105,7 @@ module MergeRequests
     def handle_assignees_change(merge_request, old_assignees)
       create_assignee_note(merge_request, old_assignees)
       notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
-      todo_service.reassigned_issuable(merge_request, current_user, old_assignees)
+      todo_service.reassigned_assignable(merge_request, current_user, old_assignees)
     end
 
     def create_branch_change_note(issuable, branch_type, old_branch, new_branch)

app/services/todo_service.rb

@@ -49,11 +49,11 @@ class TodoService
     todo_users.each(&:update_todos_count_cache)
   end
 
-  # When we reassign an issuable we should:
+  # When we reassign an assignable object (issuable, alert) we should:
   #
-  #  * create a pending todo for new assignee if issuable is assigned
+  #  * create a pending todo for new assignee if object is assigned
   #
-  def reassigned_issuable(issuable, current_user, old_assignees = [])
+  def reassigned_assignable(issuable, current_user, old_assignees = [])
     create_assignment_todo(issuable, current_user, old_assignees)
   end
 
@@ -154,14 +154,6 @@ class TodoService
     resolve_todos_for_target(awardable, current_user)
   end
 
-  # When assigning an alert we should:
-  #
-  #  * create a pending todo for new assignee if alert is assigned
-  #
-  def assign_alert(alert, current_user)
-    create_assignment_todo(alert, current_user, [])
-  end
-
   # When user marks a target as todo
   def mark_todo(target, current_user)
     attributes = attributes_for_todo(target.project, target, current_user, Todo::MARKED)

spec/services/alert_management/alerts/update_service_spec.rb

@@ -147,8 +147,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
         end
 
         it_behaves_like 'does not add a system note'
-        # TODO: We should not add another todo in this scenario
-        it_behaves_like 'adds a todo'
+        it_behaves_like 'does not add a todo'
       end
 
       context 'with multiple users included' do

spec/services/todo_service_spec.rb

@@ -59,6 +59,10 @@ RSpec.describe TodoService do
 
       should_not_create_todo(user: guest, target: addressed_target_assigned, action: Todo::DIRECTLY_ADDRESSED)
     end
+
+    it 'does not create a todo if already assigned' do
+      should_not_create_any_todo { service.send(described_method, target_assigned, author, [john_doe]) }
+    end
   end
 
   describe 'Issues' do
@@ -573,10 +577,10 @@ RSpec.describe TodoService do
     end
   end
 
-  describe '#reassigned_issuable' do
-    let(:described_method) { :reassigned_issuable }
+  describe '#reassigned_assignable' do
+    let(:described_method) { :reassigned_assignable }
 
-    context 'issuable is a merge request' do
+    context 'assignable is a merge request' do
       it_behaves_like 'reassigned target' do
         let(:target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
         let(:addressed_target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
@@ -584,13 +588,21 @@ RSpec.describe TodoService do
       end
     end
 
-    context 'issuable is an issue' do
+    context 'assignable is an issue' do
       it_behaves_like 'reassigned target' do
         let(:target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
         let(:addressed_target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
         let(:target_unassigned) { create(:issue, project: project, author: author, assignees: []) }
       end
     end
+
+    context 'assignable is an alert' do
+      it_behaves_like 'reassigned target' do
+        let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
+        let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
+        let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) }
+      end
+    end
   end
 
   describe 'Merge Requests' do
@@ -778,16 +790,6 @@ RSpec.describe TodoService do
       end
     end
 
-    describe '#assign_alert' do
-      let(:described_method) { :assign_alert }
-
-      it_behaves_like 'reassigned target' do
-        let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
-        let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
-        let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) }
-      end
-    end
-
     describe '#merge_request_build_failed' do
       let(:merge_participants) { [mr_unassigned.author, admin] }