summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorErshad Kunnakkadan <ershad92@gmail.com>2016-12-05 17:42:22 +0530
committerErshad Kunnakkadan <ershad92@gmail.com>2017-02-08 21:53:34 +0530
commit3a23639bc04729cfdc37e4b8ebf46358c3d5a137 (patch)
treedb89ec896fd36ace8c2fc642e66106d9dfbec903
parent11d33873a8da5f90fd00bf0909dd1e9f8fca1448 (diff)
downloadgitlab-ce-3a23639bc04729cfdc37e4b8ebf46358c3d5a137.tar.gz
Create directly_addressed Todos when mentioned in beginning of a line
-rw-r--r--app/helpers/todos_helper.rb4
-rw-r--r--app/models/concerns/mentionable.rb19
-rw-r--r--app/models/directly_addressed_user.rb7
-rw-r--r--app/models/todo.rb16
-rw-r--r--app/services/todo_service.rb18
-rw-r--r--changelogs/unreleased/24976-start-of-line-mention.yml4
-rw-r--r--lib/banzai/querying.rb56
-rw-r--r--lib/banzai/reference_extractor.rb5
-rw-r--r--lib/banzai/reference_parser/base_parser.rb5
-rw-r--r--lib/banzai/reference_parser/directly_addressed_user_parser.rb8
-rw-r--r--lib/gitlab/reference_extractor.rb8
-rw-r--r--spec/factories/todos.rb4
-rw-r--r--spec/lib/gitlab/reference_extractor_spec.rb75
-rw-r--r--spec/services/todo_service_spec.rb206
14 files changed, 411 insertions, 24 deletions
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index d7d51c99979..845f1a0e840 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -15,6 +15,7 @@ module TodosHelper
when Todo::MARKED then 'added a todo for'
when Todo::APPROVAL_REQUIRED then 'set you as an approver for'
when Todo::UNMERGEABLE then 'Could not merge'
+ when Todo::DIRECTLY_ADDRESSED then 'directly addressed you on'
end
end
@@ -88,7 +89,8 @@ module TodosHelper
{ id: Todo::ASSIGNED, text: 'Assigned' },
{ id: Todo::MENTIONED, text: 'Mentioned' },
{ id: Todo::MARKED, text: 'Added' },
- { id: Todo::BUILD_FAILED, text: 'Pipelines' }
+ { id: Todo::BUILD_FAILED, text: 'Pipelines' },
+ { id: Todo::DIRECTLY_ADDRESSED, text: 'Directly addressed' }
]
end
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index ef2c1e5d414..7e56e371b27 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -44,8 +44,15 @@ module Mentionable
end
def all_references(current_user = nil, extractor: nil)
- extractor ||= Gitlab::ReferenceExtractor.
- new(project, current_user)
+ # Use custom extractor if it's passed in the function parameters.
+ if extractor
+ @extractor = extractor
+ else
+ @extractor ||= Gitlab::ReferenceExtractor.
+ new(project, current_user)
+
+ @extractor.reset_memoized_values
+ end
self.class.mentionable_attrs.each do |attr, options|
text = __send__(attr)
@@ -55,16 +62,20 @@ module Mentionable
skip_project_check: skip_project_check?
)
- extractor.analyze(text, options)
+ @extractor.analyze(text, options)
end
- extractor
+ @extractor
end
def mentioned_users(current_user = nil)
all_references(current_user).users
end
+ def directly_addressed_users(current_user = nil)
+ all_references(current_user).directly_addressed_users
+ end
+
# Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
def referenced_mentionables(current_user = self.author)
refs = all_references(current_user)
diff --git a/app/models/directly_addressed_user.rb b/app/models/directly_addressed_user.rb
new file mode 100644
index 00000000000..0d519c6ac22
--- /dev/null
+++ b/app/models/directly_addressed_user.rb
@@ -0,0 +1,7 @@
+class DirectlyAddressedUser
+ class << self
+ def reference_pattern
+ User.reference_pattern
+ end
+ end
+end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 2adf494ce11..3dda7948d0b 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -1,12 +1,13 @@
class Todo < ActiveRecord::Base
include Sortable
- ASSIGNED = 1
- MENTIONED = 2
- BUILD_FAILED = 3
- MARKED = 4
- APPROVAL_REQUIRED = 5 # This is an EE-only feature
- UNMERGEABLE = 6
+ ASSIGNED = 1
+ MENTIONED = 2
+ BUILD_FAILED = 3
+ MARKED = 4
+ APPROVAL_REQUIRED = 5 # This is an EE-only feature
+ UNMERGEABLE = 6
+ DIRECTLY_ADDRESSED = 7
ACTION_NAMES = {
ASSIGNED => :assigned,
@@ -14,7 +15,8 @@ class Todo < ActiveRecord::Base
BUILD_FAILED => :build_failed,
MARKED => :marked,
APPROVAL_REQUIRED => :approval_required,
- UNMERGEABLE => :unmergeable
+ UNMERGEABLE => :unmergeable,
+ DIRECTLY_ADDRESSED => :directly_addressed
}
belongs_to :author, class_name: "User"
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 1bd6ce416ab..8ab943f4639 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -243,6 +243,12 @@ class TodoService
end
def create_mention_todos(project, target, author, note = nil)
+ # Create Todos for directly addressed users
+ directly_addressed_users = filter_directly_addressed_users(project, note || target, author)
+ attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note)
+ create_todos(directly_addressed_users, attributes)
+
+ # Create Todos for mentioned users
mentioned_users = filter_mentioned_users(project, note || target, author)
attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note)
create_todos(mentioned_users, attributes)
@@ -282,10 +288,18 @@ class TodoService
)
end
+ def filter_todo_users(users, project, target)
+ reject_users_without_access(users, project, target).uniq
+ end
+
def filter_mentioned_users(project, target, author)
mentioned_users = target.mentioned_users(author)
- mentioned_users = reject_users_without_access(mentioned_users, project, target)
- mentioned_users.uniq
+ filter_todo_users(mentioned_users, project, target)
+ end
+
+ def filter_directly_addressed_users(project, target, author)
+ directly_addressed_users = target.directly_addressed_users(author)
+ filter_todo_users(directly_addressed_users, project, target)
end
def reject_users_without_access(users, project, target)
diff --git a/changelogs/unreleased/24976-start-of-line-mention.yml b/changelogs/unreleased/24976-start-of-line-mention.yml
new file mode 100644
index 00000000000..99208aac87c
--- /dev/null
+++ b/changelogs/unreleased/24976-start-of-line-mention.yml
@@ -0,0 +1,4 @@
+---
+title: Added a feature to create a 'directly addressed' Todo when mentioned in the beginning of a line.
+merge_request: 7926
+author: Ershad Kunnakkadan
diff --git a/lib/banzai/querying.rb b/lib/banzai/querying.rb
index 1e1b51e683e..fb2faae02bc 100644
--- a/lib/banzai/querying.rb
+++ b/lib/banzai/querying.rb
@@ -1,18 +1,64 @@
module Banzai
module Querying
+ module_function
+
# Searches a Nokogiri document using a CSS query, optionally optimizing it
# whenever possible.
#
- # document - A document/element to search.
- # query - The CSS query to use.
+ # document - A document/element to search.
+ # query - The CSS query to use.
+ # reference_options - A hash with nodes filter options
#
- # Returns a Nokogiri::XML::NodeSet.
- def self.css(document, query)
+ # Returns an array of Nokogiri::XML::Element objects if location is specified
+ # in reference_options. Otherwise it would a Nokogiri::XML::NodeSet.
+ def css(document, query, reference_options = {})
# When using "a.foo" Nokogiri compiles this to "//a[...]" but
# "descendant::a[...]" is quite a bit faster and achieves the same result.
xpath = Nokogiri::CSS.xpath_for(query)[0].gsub(%r{^//}, 'descendant::')
+ xpath = restrict_to_p_nodes_at_root(xpath) if filter_nodes_at_beginning?(reference_options)
+ nodes = document.xpath(xpath)
+
+ filter_nodes(nodes, reference_options)
+ end
+
+ def restrict_to_p_nodes_at_root(xpath)
+ xpath.gsub('descendant::', './p/')
+ end
+
+ def filter_nodes(nodes, reference_options)
+ if filter_nodes_at_beginning?(reference_options)
+ filter_nodes_at_beginning(nodes)
+ else
+ nodes
+ end
+ end
+
+ def filter_nodes_at_beginning?(reference_options)
+ reference_options && reference_options[:location] == :beginning
+ end
+
+ # Selects child nodes if they are present in the beginning among other siblings.
+ #
+ # nodes - A Nokogiri::XML::NodeSet.
+ #
+ # Returns an array of Nokogiri::XML::Element objects.
+ def filter_nodes_at_beginning(nodes)
+ parents_and_nodes = nodes.group_by(&:parent)
+ filtered_nodes = []
+
+ parents_and_nodes.each do |parent, nodes|
+ children = parent.children
+ nodes = nodes.to_a
+
+ children.each do |child|
+ next if child.text.blank?
+ node = nodes.shift
+ break unless node == child
+ filtered_nodes << node
+ end
+ end
- document.xpath(xpath)
+ filtered_nodes
end
end
end
diff --git a/lib/banzai/reference_extractor.rb b/lib/banzai/reference_extractor.rb
index b26a41a1f3b..8e3b0c4db79 100644
--- a/lib/banzai/reference_extractor.rb
+++ b/lib/banzai/reference_extractor.rb
@@ -16,6 +16,11 @@ module Banzai
processor.process(html_documents)
end
+ def reset_memoized_values
+ @html_documents = nil
+ @texts_and_contexts = []
+ end
+
private
def html_documents
diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb
index d8a855ec1fe..2058a58d0ae 100644
--- a/lib/banzai/reference_parser/base_parser.rb
+++ b/lib/banzai/reference_parser/base_parser.rb
@@ -33,7 +33,7 @@ module Banzai
# they have access to.
class BaseParser
class << self
- attr_accessor :reference_type
+ attr_accessor :reference_type, :reference_options
end
# Returns the attribute name containing the value for every object to be
@@ -182,9 +182,10 @@ module Banzai
# the references.
def process(documents)
type = self.class.reference_type
+ reference_options = self.class.reference_options
nodes = documents.flat_map do |document|
- Querying.css(document, "a[data-reference-type='#{type}'].gfm").to_a
+ Querying.css(document, "a[data-reference-type='#{type}'].gfm", reference_options).to_a
end
gather_references(nodes)
diff --git a/lib/banzai/reference_parser/directly_addressed_user_parser.rb b/lib/banzai/reference_parser/directly_addressed_user_parser.rb
new file mode 100644
index 00000000000..77df9bbd024
--- /dev/null
+++ b/lib/banzai/reference_parser/directly_addressed_user_parser.rb
@@ -0,0 +1,8 @@
+module Banzai
+ module ReferenceParser
+ class DirectlyAddressedUserParser < UserParser
+ self.reference_type = :user
+ self.reference_options = { location: :beginning }
+ end
+ end
+end
diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb
index 11c0b01f0dc..437a339dd2b 100644
--- a/lib/gitlab/reference_extractor.rb
+++ b/lib/gitlab/reference_extractor.rb
@@ -1,13 +1,12 @@
module Gitlab
# Extract possible GFM references from an arbitrary String for further processing.
class ReferenceExtractor < Banzai::ReferenceExtractor
- REFERABLES = %i(user issue label milestone merge_request snippet commit commit_range)
+ REFERABLES = %i(user issue label milestone merge_request snippet commit commit_range directly_addressed_user)
attr_accessor :project, :current_user, :author
def initialize(project, current_user = nil)
@project = project
@current_user = current_user
-
@references = {}
super()
@@ -21,6 +20,11 @@ module Gitlab
super(type, project, current_user)
end
+ def reset_memoized_values
+ @references = {}
+ super()
+ end
+
REFERABLES.each do |type|
define_method("#{type}s") do
@references[type] ||= references(type)
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index 275561502cd..b4e4cd97780 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -14,6 +14,10 @@ FactoryGirl.define do
action { Todo::MENTIONED }
end
+ trait :directly_addressed do
+ action { Todo::DIRECTLY_ADDRESSED }
+ end
+
trait :on_commit do
commit_id RepoHelpers.sample_commit.id
target_type "Commit"
diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb
index 6b689c41ef6..84cfd934fa0 100644
--- a/spec/lib/gitlab/reference_extractor_spec.rb
+++ b/spec/lib/gitlab/reference_extractor_spec.rb
@@ -42,14 +42,85 @@ describe Gitlab::ReferenceExtractor, lib: true do
> @offteam
})
+
expect(subject.users).to match_array([])
end
+ describe 'directly addressed users' do
+ before do
+ @u_foo = create(:user, username: 'foo')
+ @u_foo2 = create(:user, username: 'foo2')
+ @u_foo3 = create(:user, username: 'foo3')
+ @u_foo4 = create(:user, username: 'foo4')
+ @u_foo5 = create(:user, username: 'foo5')
+
+ @u_bar = create(:user, username: 'bar')
+ @u_bar2 = create(:user, username: 'bar2')
+ @u_bar3 = create(:user, username: 'bar3')
+ @u_bar4 = create(:user, username: 'bar4')
+
+ @u_tom = create(:user, username: 'tom')
+ @u_tom2 = create(:user, username: 'tom2')
+ end
+
+ context 'when a user is directly addressed' do
+ it 'accesses the user object which is mentioned in the beginning of the line' do
+ subject.analyze('@foo What do you think? cc: @bar, @tom')
+
+ expect(subject.directly_addressed_users).to match_array([@u_foo])
+ end
+
+ it "doesn't access the user object if it's not mentioned in the beginning of the line" do
+ subject.analyze('What do you think? cc: @bar')
+
+ expect(subject.directly_addressed_users).to be_empty
+ end
+ end
+
+ context 'when multiple users are addressed' do
+ it 'accesses the user objects which are mentioned in the beginning of the line' do
+ subject.analyze('@foo @bar What do you think? cc: @tom')
+
+ expect(subject.directly_addressed_users).to match_array([@u_foo, @u_bar])
+ end
+
+ it "doesn't access the user objects if they are not mentioned in the beginning of the line" do
+ subject.analyze('What do you think? cc: @foo @bar @tom')
+
+ expect(subject.directly_addressed_users).to be_empty
+ end
+ end
+
+ context 'when multiple users are addressed in different paragraphs' do
+ it 'accesses user objects which are mentioned in the beginning of each paragraph' do
+ subject.analyze <<-NOTE.strip_heredoc
+ @foo What do you think? cc: @tom
+
+ - @bar can you please have a look?
+
+ >>>
+ @foo2 what do you think? cc: @bar2
+ >>>
+
+ @foo3 @foo4 thank you!
+
+ > @foo5 well done!
+
+ 1. @bar3 Can you please check? cc: @tom2
+ 2. @bar4 What do you this of this MR?
+ NOTE
+
+ expect(subject.directly_addressed_users).to match_array([@u_foo, @u_foo3, @u_foo4])
+ end
+ end
+ end
+
it 'accesses valid issue objects' do
@i0 = create(:issue, project: project)
@i1 = create(:issue, project: project)
subject.analyze("#{@i0.to_reference}, #{@i1.to_reference}, and #{Issue.reference_prefix}999.")
+
expect(subject.issues).to match_array([@i0, @i1])
end
@@ -58,6 +129,7 @@ describe Gitlab::ReferenceExtractor, lib: true do
@m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'feature_conflict')
subject.analyze("!999, !#{@m1.iid}, and !#{@m0.iid}.")
+
expect(subject.merge_requests).to match_array([@m1, @m0])
end
@@ -67,6 +139,7 @@ describe Gitlab::ReferenceExtractor, lib: true do
@l2 = create(:label)
subject.analyze("~#{@l0.id}, ~999, ~#{@l2.id}, ~#{@l1.id}")
+
expect(subject.labels).to match_array([@l0, @l1])
end
@@ -76,6 +149,7 @@ describe Gitlab::ReferenceExtractor, lib: true do
@s2 = create(:project_snippet)
subject.analyze("$#{@s0.id}, $999, $#{@s2.id}, $#{@s1.id}")
+
expect(subject.snippets).to match_array([@s0, @s1])
end
@@ -127,6 +201,7 @@ describe Gitlab::ReferenceExtractor, lib: true do
it 'handles project issue references' do
subject.analyze("this refers issue #{issue.to_reference(project)}")
+
extracted = subject.issues
expect(extracted.size).to eq(1)
expect(extracted).to match_array([issue])
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 13d584a8975..4320365ab57 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -9,7 +9,9 @@ describe TodoService, services: true do
let(:admin) { create(:admin) }
let(:john_doe) { create(:user) }
let(:project) { create(:project) }
- let(:mentions) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') }
+ let(:mentions) { 'FYI: ' + [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') }
+ let(:directly_addressed) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') }
+ let(:directly_addressed_and_mentioned) { member.to_reference + ", what do you think? cc: " + [guest, admin].map(&:to_reference).join(' ') }
let(:service) { described_class.new }
before do
@@ -21,8 +23,10 @@ describe TodoService, services: true do
describe 'Issues' do
let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
+ let(:addressed_issue) { create(:issue, project: project, assignee: john_doe, author: author, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
let(:unassigned_issue) { create(:issue, project: project, assignee: nil) }
let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee, description: mentions) }
+ let(:addressed_confident_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee, description: directly_addressed) }
describe '#new_issue' do
it 'creates a todo if assigned' do
@@ -52,6 +56,26 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
end
+ it 'creates a directly addressed todo for each valid addressed user' do
+ service.new_issue(addressed_issue, author)
+
+ should_create_todo(user: member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: guest, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: author, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: john_doe, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
+ it 'creates correct todos for each valid user based on the type of mention' do
+ issue.update(description: directly_addressed_and_mentioned)
+
+ service.new_issue(issue, author)
+
+ should_create_todo(user: member, target: issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: admin, target: issue, action: Todo::MENTIONED)
+ should_create_todo(user: guest, target: issue, action: Todo::MENTIONED)
+ end
+
it 'does not create todo if user can not see the issue when issue is confidential' do
service.new_issue(confidential_issue, john_doe)
@@ -63,6 +87,17 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
end
+ it 'does not create directly addressed todo if user cannot see the issue when issue is confidential' do
+ service.new_issue(addressed_confident_issue, john_doe)
+
+ should_create_todo(user: assignee, target: addressed_confident_issue, author: john_doe, action: Todo::ASSIGNED)
+ should_create_todo(user: author, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: member, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: guest, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: john_doe, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
context 'when a private group is mentioned' do
let(:group) { create :group, :private }
let(:project) { create :project, :private, group: group }
@@ -94,12 +129,38 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
end
+ it 'creates a todo for each valid user based on the type of mention' do
+ issue.update(description: directly_addressed_and_mentioned)
+
+ service.update_issue(issue, author)
+
+ should_create_todo(user: member, target: issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: guest, target: issue, action: Todo::MENTIONED)
+ should_create_todo(user: admin, target: issue, action: Todo::MENTIONED)
+ end
+
+ it 'creates a directly addressed todo for each valid addressed user' do
+ service.update_issue(addressed_issue, author)
+
+ should_create_todo(user: member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: guest, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: john_doe, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: author, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
it 'does not create a todo if user was already mentioned' do
create(:todo, :mentioned, user: member, project: project, target: issue, author: author)
expect { service.update_issue(issue, author) }.not_to change(member.todos, :count)
end
+ it 'does not create a directly addressed todo if user was already mentioned or addressed' do
+ create(:todo, :directly_addressed, user: member, project: project, target: addressed_issue, author: author)
+
+ expect { service.update_issue(addressed_issue, author) }.not_to change(member.todos, :count)
+ end
+
it 'does not create todo if user can not see the issue when issue is confidential' do
service.update_issue(confidential_issue, john_doe)
@@ -111,6 +172,17 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
end
+ it 'does not create a directly addressed todo if user can not see the issue when issue is confidential' do
+ service.update_issue(addressed_confident_issue, john_doe)
+
+ should_create_todo(user: author, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: assignee, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: member, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: admin, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: guest, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: john_doe, target: addressed_confident_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
context 'issues with a task list' do
it 'does not create todo when tasks are marked as completed' do
issue.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
@@ -125,6 +197,19 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
end
+ it 'does not create directly addressed todo when tasks are marked as completed' do
+ addressed_issue.update(description: "#{directly_addressed}\n- [x] Task 1\n- [x] Task 2\n")
+
+ service.update_issue(addressed_issue, author)
+
+ should_not_create_todo(user: admin, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: assignee, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: author, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: john_doe, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
it 'does not raise an error when description not change' do
issue.update(title: 'Sample')
@@ -244,8 +329,11 @@ describe TodoService, services: true do
let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) }
+ let(:addressed_note) { create(:note, project: project, noteable: issue, author: john_doe, note: directly_addressed) }
let(:note_on_commit) { create(:note_on_commit, project: project, author: john_doe, note: mentions) }
+ let(:addressed_note_on_commit) { create(:note_on_commit, project: project, author: john_doe, note: directly_addressed) }
let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project, note: mentions) }
+ let(:addressed_note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project, note: directly_addressed) }
let(:note_on_project_snippet) { create(:note_on_project_snippet, project: project, author: john_doe, note: mentions) }
let(:system_note) { create(:system_note, project: project, noteable: issue) }
@@ -276,6 +364,26 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
end
+ it 'creates a todo for each valid user based on the type of mention' do
+ note.update(note: directly_addressed_and_mentioned)
+
+ service.new_note(note, john_doe)
+
+ should_create_todo(user: member, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: note)
+ should_create_todo(user: admin, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
+ should_create_todo(user: guest, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
+ end
+
+ it 'creates a directly addressed todo for each valid addressed user' do
+ service.new_note(addressed_note, john_doe)
+
+ should_create_todo(user: member, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note)
+ should_create_todo(user: guest, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note)
+ should_create_todo(user: author, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note)
+ should_create_todo(user: john_doe, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note)
+ should_not_create_todo(user: non_member, target: issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note)
+ end
+
it 'does not create todo if user can not see the issue when leaving a note on a confidential issue' do
service.new_note(note_on_confidential_issue, john_doe)
@@ -287,6 +395,17 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue)
end
+ it 'does not create a directly addressed todo if user can not see the issue when leaving a note on a confidential issue' do
+ service.new_note(addressed_note_on_confidential_issue, john_doe)
+
+ should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
+ end
+
it 'creates a todo for each valid mentioned user when leaving a note on commit' do
service.new_note(note_on_commit, john_doe)
@@ -296,6 +415,15 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
end
+ it 'creates a directly addressed todo for each valid mentioned user when leaving a note on commit' do
+ service.new_note(addressed_note_on_commit, john_doe)
+
+ should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
+ should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
+ should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
+ should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
+ end
+
it 'does not create todo when leaving a note on snippet' do
should_not_create_any_todo { service.new_note(note_on_project_snippet, john_doe) }
end
@@ -324,6 +452,7 @@ describe TodoService, services: true do
describe 'Merge Requests' do
let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
+ let(:addressed_mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) }
describe '#new_merge_request' do
@@ -350,6 +479,25 @@ describe TodoService, services: true do
should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
end
+
+ it 'creates a todo for each valid user based on the type of mention' do
+ mr_assigned.update(description: directly_addressed_and_mentioned)
+
+ service.new_merge_request(mr_assigned, author)
+
+ should_create_todo(user: member, target: mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED)
+ end
+
+ it 'creates a directly addressed todo for each valid addressed user' do
+ service.new_merge_request(addressed_mr_assigned, author)
+
+ should_create_todo(user: member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: author, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: john_doe, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ end
end
describe '#update_merge_request' do
@@ -363,12 +511,37 @@ describe TodoService, services: true do
should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
end
+ it 'creates a todo for each valid user based on the type of mention' do
+ mr_assigned.update(description: directly_addressed_and_mentioned)
+
+ service.update_merge_request(mr_assigned, author)
+
+ should_create_todo(user: member, target: mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED)
+ end
+
+ it 'creates a directly addressed todo for each valid addressed user' do
+ service.update_merge_request(addressed_mr_assigned, author)
+
+ should_create_todo(user: member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: john_doe, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_create_todo(user: author, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
it 'does not create a todo if user was already mentioned' do
create(:todo, :mentioned, user: member, project: project, target: mr_assigned, author: author)
expect { service.update_merge_request(mr_assigned, author) }.not_to change(member.todos, :count)
end
+ it 'does not create a directly addressed todo if user was already mentioned or addressed' do
+ create(:todo, :directly_addressed, user: member, project: project, target: addressed_mr_assigned, author: author)
+
+ expect{ service.update_merge_request(addressed_mr_assigned, author) }.not_to change(member.todos, :count)
+ end
+
context 'with a task list' do
it 'does not create todo when tasks are marked as completed' do
mr_assigned.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
@@ -384,6 +557,20 @@ describe TodoService, services: true do
should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
end
+ it 'does not create directly addressed todo when tasks are marked as completed' do
+ addressed_mr_assigned.update(description: "#{directly_addressed}\n- [x] Task 1\n- [X] Task 2")
+
+ service.update_merge_request(addressed_mr_assigned, author)
+
+ should_not_create_todo(user: admin, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: assignee, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: author, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: john_doe, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: non_member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ end
+
it 'does not raise an error when description not change' do
mr_assigned.update(title: 'Sample')
@@ -436,6 +623,11 @@ describe TodoService, services: true do
service.reassigned_merge_request(mr_assigned, author)
should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
end
+
+ it 'does not create a directly addressed todo for guests' do
+ service.reassigned_merge_request(addressed_mr_assigned, author)
+ should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ end
end
describe '#merge_merge_request' do
@@ -452,6 +644,11 @@ describe TodoService, services: true do
service.merge_merge_request(mr_assigned, john_doe)
should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED)
end
+
+ it 'does not create directly addressed todo for guests' do
+ service.merge_merge_request(addressed_mr_assigned, john_doe)
+ should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED)
+ end
end
describe '#new_award_emoji' do
@@ -509,6 +706,7 @@ describe TodoService, services: true do
describe '#new_note' do
let(:mention) { john_doe.to_reference }
let(:diff_note_on_merge_request) { create(:diff_note_on_merge_request, project: project, noteable: mr_unassigned, author: author, note: "Hey #{mention}") }
+ let(:addressed_diff_note_on_merge_request) { create(:diff_note_on_merge_request, project: project, noteable: mr_unassigned, author: author, note: "#{mention}, hey!") }
let(:legacy_diff_note_on_merge_request) { create(:legacy_diff_note_on_merge_request, project: project, noteable: mr_unassigned, author: author, note: "Hey #{mention}") }
it 'creates a todo for mentioned user on new diff note' do
@@ -517,6 +715,12 @@ describe TodoService, services: true do
should_create_todo(user: john_doe, target: mr_unassigned, author: author, action: Todo::MENTIONED, note: diff_note_on_merge_request)
end
+ it 'creates a directly addressed todo for addressed user on new diff note' do
+ service.new_note(addressed_diff_note_on_merge_request, author)
+
+ should_create_todo(user: john_doe, target: mr_unassigned, author: author, action: Todo::DIRECTLY_ADDRESSED, note: addressed_diff_note_on_merge_request)
+ end
+
it 'creates a todo for mentioned user on legacy diff note' do
service.new_note(legacy_diff_note_on_merge_request, author)