Commit 4d1b92fb authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Only allow multiple todos per action per object

Changes the behavior of the multiple_todos feature flag so that only one
todo per action type is allowed per object.

An exception is made for mentions so that users can still have multiple
mention todos since these link to different notes.

This reduces noise for the other todo actions where they link to the
same thing.
parent dec842e7
......@@ -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
......@@ -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
......
......@@ -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)
......
......@@ -62,8 +62,8 @@ 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, mentions for the same user on the same object
create new to-do items. Other actions are limited to one to-do item per action type.
## Create a to-do item
......
......@@ -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
......@@ -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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment