diff options
author | Ershad Kunnakkadan <ershad92@gmail.com> | 2016-12-05 15:12:22 +0300 |
---|---|---|
committer | Ershad Kunnakkadan <ershad92@gmail.com> | 2017-02-08 19:23:34 +0300 |
commit | 3a23639bc04729cfdc37e4b8ebf46358c3d5a137 (patch) | |
tree | db89ec896fd36ace8c2fc642e66106d9dfbec903 | |
parent | 11d33873a8da5f90fd00bf0909dd1e9f8fca1448 (diff) |
Create directly_addressed Todos when mentioned in beginning of a line
-rw-r--r-- | app/helpers/todos_helper.rb | 4 | ||||
-rw-r--r-- | app/models/concerns/mentionable.rb | 19 | ||||
-rw-r--r-- | app/models/directly_addressed_user.rb | 7 | ||||
-rw-r--r-- | app/models/todo.rb | 16 | ||||
-rw-r--r-- | app/services/todo_service.rb | 18 | ||||
-rw-r--r-- | changelogs/unreleased/24976-start-of-line-mention.yml | 4 | ||||
-rw-r--r-- | lib/banzai/querying.rb | 56 | ||||
-rw-r--r-- | lib/banzai/reference_extractor.rb | 5 | ||||
-rw-r--r-- | lib/banzai/reference_parser/base_parser.rb | 5 | ||||
-rw-r--r-- | lib/banzai/reference_parser/directly_addressed_user_parser.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/reference_extractor.rb | 8 | ||||
-rw-r--r-- | spec/factories/todos.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/reference_extractor_spec.rb | 75 | ||||
-rw-r--r-- | spec/services/todo_service_spec.rb | 206 |
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) |