Merge branch 'multiple-todos-per-action' into 'master'

Only allow multiple todos per action per object

See merge request gitlab-org/gitlab!82470

app/finders/pending_todos_finder.rb

@@ -27,7 +27,8 @@ class PendingTodosFinder
     todos = by_target_id(todos)
     todos = by_target_type(todos)
     todos = by_discussion(todos)
-    by_commit_id(todos)
+    todos = by_commit_id(todos)
+    by_action(todos)
   end
 
   def by_project(todos)
@@ -69,4 +70,10 @@ class PendingTodosFinder
       todos
     end
   end
+
+  def by_action(todos)
+    return todos if params[:action].blank?
+
+    todos.for_action(params[:action])
+  end
 end

app/models/todo.rb

@@ -34,6 +34,8 @@ class Todo < ApplicationRecord
     ATTENTION_REQUESTED => :attention_requested
   }.freeze
 
+  ACTIONS_MULTIPLE_ALLOWED = [Todo::MENTIONED, Todo::DIRECTLY_ADDRESSED].freeze
+
   belongs_to :author, class_name: "User"
   belongs_to :note
   belongs_to :project

app/services/todo_service.rb

@@ -9,6 +9,7 @@
 #
 class TodoService
   include Gitlab::Utils::UsageData
+
   # When create an issue we should:
   #
   #  * create a todo for assignee if issue is assigned
@@ -229,8 +230,24 @@ class TodoService
 
     return if users.empty?
 
-    users_with_pending_todos = pending_todos(users, attributes).distinct_user_ids
-    users.reject! { |user| users_with_pending_todos.include?(user.id) && Feature.disabled?(:multiple_todos, user) }
+    users_single_todos, users_multiple_todos = users.partition { |u| Feature.disabled?(:multiple_todos, u) }
+    excluded_user_ids = []
+
+    if users_single_todos.present?
+      excluded_user_ids += pending_todos(
+        users_single_todos,
+        attributes.slice(:project_id, :target_id, :target_type, :commit_id, :discussion)
+      ).distinct_user_ids
+    end
+
+    if users_multiple_todos.present? && !Todo::ACTIONS_MULTIPLE_ALLOWED.include?(attributes.fetch(:action))
+      excluded_user_ids += pending_todos(
+        users_multiple_todos,
+        attributes.slice(:project_id, :target_id, :target_type, :commit_id, :discussion, :action)
+      ).distinct_user_ids
+    end
+
+    users.reject! { |user| excluded_user_ids.include?(user.id) }
 
     todos = users.map do |user|
       issue_type = attributes.delete(:issue_type)

doc/user/todos.md

@@ -54,7 +54,8 @@ To-do items aren't affected by [GitLab notification email settings](profile/noti
 
 <!-- When the feature flag is removed, integrate this topic into the one above. -->
 
-> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/28355) in GitLab 13.8 [with a flag](../administration/feature_flags.md) named `multiple_todos`. Disabled by default.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/28355) in GitLab 13.8 [with a flag](../administration/feature_flags.md) named `multiple_todos`. Disabled by default.
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82470) in GitLab 14.9: only mentions create multiple to-do items.
 
 FLAG:
 On self-managed GitLab, by default this feature is not available. To make it available per user,
@@ -62,8 +63,11 @@ ask an administrator to [enable the feature flag](../administration/feature_flag
 On GitLab.com, this feature is not available.
 The feature is not ready for production use.
 
-When you enable this feature, new actions for the same user on the same object
-create new to-do items.
+When you enable this feature:
+
+- Every time you're mentioned, GitLab creates a new to-do item for you.
+- Other [actions that create to-do items](#actions-that-create-to-do-items)
+  create one to-do item per action type on the issue, MR, and so on.
 
 ## Create a to-do item
 

spec/finders/pending_todos_finder_spec.rb

@@ -75,5 +75,15 @@ RSpec.describe PendingTodosFinder do
 
       expect(todos).to contain_exactly(todo1, todo2)
     end
+
+    it 'supports retrieving of todos for a specific action' do
+      todo = create(:todo, :pending, user: user, target: issue, action: Todo::MENTIONED)
+
+      create(:todo, :pending, user: user, target: issue, action: Todo::ASSIGNED)
+
+      todos = described_class.new(users, action: Todo::MENTIONED).execute
+
+      expect(todos).to contain_exactly(todo)
+    end
   end
 end

spec/services/todo_service_spec.rb

@@ -628,12 +628,32 @@ RSpec.describe TodoService do
         stub_feature_flags(multiple_todos: true)
       end
 
-      it 'creates a todo even if user already has a pending todo' do
+      it 'creates a MENTIONED todo even if user already has a pending MENTIONED todo' do
         create(:todo, :mentioned, user: member, project: project, target: issue, author: author)
 
         expect { service.update_issue(issue, author) }.to change(member.todos, :count)
       end
 
+      it 'creates a DIRECTLY_ADDRESSED todo even if user already has a pending DIRECTLY_ADDRESSED todo' do
+        create(:todo, :directly_addressed, user: member, project: project, target: issue, author: author)
+
+        issue.update!(description: "#{member.to_reference}, what do you think?")
+
+        expect { service.update_issue(issue, author) }.to change(member.todos, :count)
+      end
+
+      it 'creates an ASSIGNED todo even if user already has a pending MARKED todo' do
+        create(:todo, :marked, user: john_doe, project: project, target: assigned_issue, author: author)
+
+        expect { service.reassigned_assignable(assigned_issue, author) }.to change(john_doe.todos, :count)
+      end
+
+      it 'does not create an ASSIGNED todo if user already has an ASSIGNED todo' do
+        create(:todo, :assigned, user: john_doe, project: project, target: assigned_issue, author: author)
+
+        expect { service.reassigned_assignable(assigned_issue, author) }.not_to change(john_doe.todos, :count)
+      end
+
       it 'creates multiple todos if a user is assigned and mentioned in a new issue' do
         assigned_issue.description = mentions
         service.new_issue(assigned_issue, author)