Merge branch '24976-start-of-line-mention' into 'master'

Feature to create directly addressed Todos when mentioned in beginning

Closes #24976

See merge request !7926

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
 

app/models/concerns/mentionable.rb

@@ -44,9 +44,16 @@ module Mentionable
   end
 
   def all_references(current_user = nil, extractor: nil)
-    extractor ||= Gitlab::ReferenceExtractor.
+    # 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)
       options = options.merge(
@@ -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)

app/models/directly_addressed_user.rb

+class DirectlyAddressedUser
+  class << self
+    def reference_pattern
+      User.reference_pattern
+    end
+  end
+end

app/models/todo.rb

@@ -7,6 +7,7 @@ class Todo < ActiveRecord::Base
   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"

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)

changelogs/unreleased/24976-start-of-line-mention.yml

+---
+title: Added a feature to create a 'directly addressed' Todo when mentioned in the beginning of a line.
+merge_request: 7926
+author: Ershad Kunnakkadan

lib/banzai/querying.rb

 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.
+    # 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

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

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)

lib/banzai/reference_parser/directly_addressed_user_parser.rb

+module Banzai
+  module ReferenceParser
+    class DirectlyAddressedUserParser < UserParser
+      self.reference_type = :user
+      self.reference_options = { location: :beginning }
+    end
+  end
+end

lib/gitlab/reference_extractor.rb

 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)

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"

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])

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)