Make Mentionable and Participable share a ReferenceExtractordm-separate-banzai-references

11 files changed, 69 insertions, 75 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 3fea563a487..ffde9b5b732 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -8,11 +8,10 @@ class Commit
   include StaticModel
 
   attr_mentionable :safe_message, pipeline: :single_line
-  attr_mentionable :notes_with_associations
 
-  participant :author, index: 0
-  participant :committer, index: 1
-  participant :note_authors, index: 2
+  participant :author
+  participant :committer
+  participant :notes_with_associations
 
   attr_accessor :project
 
@@ -202,10 +201,6 @@ class Commit
     notes.includes(:author, :project)
   end
 
-  def note_authors
-    User.where(id: notes.select(:author_id))
-  end
-
   def method_missing(m, *args, &block)
     @raw.send(m, *args, &block)
   end
@@ -263,12 +258,11 @@ class Commit
   end
 
   def has_been_reverted?(current_user = nil, noteable = self)
-    ext = Gitlab::ReferenceExtractor.new(project, current_user)
-    collection = noteable.notes_with_associations.system
+    ext = Gitlab::ReferenceExtractor.new(project, current_user || author)
 
-    # This is a bit of an odd setup since we want to analyze a custom
-    # collection instead of one returned by an attr_mentionable attribute.
-    noteable.analyze_references_in_collection(ext, collection)
+    noteable.notes.system.each do |note|
+      note.all_references(current_user, ext: ext)
+    end
 
     ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) }
   end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 97b7399d3d0..2c84a4be006 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -57,11 +57,10 @@ module Issuable
 
     attr_mentionable :title, pipeline: :single_line
     attr_mentionable :description
-    attr_mentionable :notes_with_associations
 
-    participant :author, index: 0
-    participant :assignee, index: 1
-    participant :note_authors, index: 2
+    participant :author
+    participant :assignee
+    participant :notes_with_associations
 
     strip_attributes :title
 
@@ -225,10 +224,6 @@ module Issuable
     notes.includes(:author, :project)
   end
 
-  def note_authors
-    User.where(id: notes.select(:author_id))
-  end
-
   def updated_tasks
     Taskable.get_updated_tasks(old_content: previous_changes['description'].first,
                                new_content: description)
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index ea542bbda49..ad6d3a9cf92 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -23,7 +23,7 @@ module Mentionable
 
   included do
     if self < Participable
-      participant ->(current_user) { mentioned_users(current_user) }
+      participant ->(current_user, ext:) { all_references(current_user, ext: ext) }
     end
   end
 
@@ -43,13 +43,18 @@ module Mentionable
     self
   end
 
-  def all_references(current_user = nil, text = nil)
-    ext = Gitlab::ReferenceExtractor.new(project, current_user || author)
+  def all_references(current_user = nil, text = nil, ext: nil)
+    ext ||= Gitlab::ReferenceExtractor.new(project, current_user || author)
 
     if text
       ext.analyze(text, author: author)
     else
-      analyze_references_in_attributes(ext)
+      self.class.mentionable_attrs.each do |attr, options|
+        text = send(attr)
+
+        options = options.merge(cache_key: [self, attr], author: author)
+        ext.analyze(text, options)
+      end
     end
 
     ext
diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index 69b47ddbced..4d0742780c6 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -15,7 +15,7 @@
 #       participant :author
 #       participant :assignee
 #       participant :notes
-#       participant ->(current_user) { mentioned_users(current_user) }
+#       participant ->(current_user, ext) { all_references(current_user, ext: ext) }
 #     end
 #
 #     issue = Issue.last
@@ -35,8 +35,8 @@ module Participable
     # index - The position of the returned object in the Array returned by
     #         `#participants`. By default the attribute is inserted at the end
     #         of the list.
-    def participant(attr, index: -1)
-      participant_attrs.insert(index, attr)
+    def participant(attr)
+      participant_attrs << attr
     end
 
     def participant_attrs
@@ -47,37 +47,45 @@ module Participable
   # Returns the users participating in a discussion.
   #
   # Returns an Array of User instances.
-  def participants(current_user = self.author)
+  def participants(current_user = nil, ext: nil, filter_access: true, load_references: true)
+    ext ||= Gitlab::ReferenceExtractor.new(project, current_user || author)
+
     participants = Set.new
 
     self.class.participant_attrs.each do |attr|
       value =
         if attr.respond_to?(:call)
-          instance_exec(current_user, &attr)
+          instance_exec(current_user, ext: ext, &attr)
         else
           send(attr)
         end
 
       next unless value
 
-      result = participants_for(value, current_user)
+      result = participants_for(value, current_user: current_user, ext: ext)
 
       participants.merge(result) if result
     end
 
-    Ability.users_that_can_read_project(participants.to_a, project)
+    participants.merge(ext.users) if load_references
+
+    users = participants.to_a
+
+    users = Ability.users_that_can_read_project(users, project) if filter_access
+
+    users
   end
 
   private
 
-  def participants_for(value, current_user = nil)
+  def participants_for(value, current_user: nil, ext: ext)
     case value
     when User
       [value]
     when Enumerable, ActiveRecord::Relation
-      value.flat_map { |v| participants_for(v, current_user) }
+      value.flat_map { |v| participants_for(v, current_user: current_user, ext: ext) }
     when Participable
-      value.participants(current_user)
+      value.participants(current_user, ext: ext, filter_access: false, load_references: false)
     end
   end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index e555ac14730..a1fb507b8f2 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -95,7 +95,13 @@ class Issue < ActiveRecord::Base
   end
 
   def referenced_merge_requests(current_user = nil)
-    all_references(current_user || author).merge_requests.sort_by(&:iid)
+    ext = Gitlab::ReferenceExtractor.new(project, current_user || author)
+
+    [self, *notes].each do |note|
+      note.all_references(current_user, ext: ext)
+    end
+
+    ext.merge_requests.sort_by(&:iid)
   end
 
   # All branches containing the current issue's ID, except for
@@ -132,9 +138,13 @@ class Issue < ActiveRecord::Base
   def closed_by_merge_requests(current_user = nil)
     return [] unless open?
 
-    notes.system.flat_map do |note|
-      note.all_references(current_user).merge_requests
-    end.uniq.select { |mr| mr.open? && mr.closes_issue?(self) }
+    ext = Gitlab::ReferenceExtractor.new(project, current_user || author)
+
+    notes.system.each do |note|
+      note.all_references(current_user, ext: ext)
+    end
+
+    ext.merge_requests.select { |mr| mr.open? && mr.closes_issue?(self) }
   end
 
   def moved?
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 5460935366c..83b1a8f96bf 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -340,14 +340,12 @@ class MergeRequest < ActiveRecord::Base
 
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
-    if target_branch == project.default_branch
-      messages = commits.map(&:safe_message) << description
+    return [] unless target_branch == project.default_branch
 
-      Gitlab::ClosingIssueExtractor.new(project, current_user).
-        closed_by_message(messages.join("\n"))
-    else
-      []
-    end
+    messages = commits.map(&:safe_message) << description
+
+    Gitlab::ClosingIssueExtractor.new(project, current_user).
+      closed_by_message(messages.join("\n"))
   end
 
   def target_project_path
diff --git a/app/models/note.rb b/app/models/note.rb
index cfb01ce9526..a929ad25489 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -7,8 +7,9 @@ class Note < ActiveRecord::Base
 
   default_value_for :system, false
 
+  participant :author
+  
   attr_mentionable :note, pipeline: :note
-  participant :author, index: 0
 
   belongs_to :project
   belongs_to :noteable, polymorphic: true, touch: true
diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb
index 74162c83949..25b5d777641 100644
--- a/app/models/project_snippet.rb
+++ b/app/models/project_snippet.rb
@@ -7,8 +7,6 @@ class ProjectSnippet < Snippet
   # Scopes
   scope :fresh, -> { order("created_at DESC") }
 
-  participant :author, index: 0
-  participant :note_authors, index: 1
-
-  attr_mentionable :notes_with_associations
+  participant :author
+  participant :notes_with_associations
 end
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index 076abae9bb0..f09c4607956 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -31,10 +31,8 @@ class Snippet < ActiveRecord::Base
   scope :public_and_internal, -> { where(visibility_level: [Snippet::PUBLIC, Snippet::INTERNAL]) }
   scope :fresh,   -> { order("created_at DESC") }
 
-  participant :author, index: 0
-  participant :note_authors, index: 1
-
-  attr_mentionable :notes_with_associations
+  participant :author
+  participant :notes_with_associations
 
   def self.reference_prefix
     '$'
@@ -108,10 +106,6 @@ class Snippet < ActiveRecord::Base
     notes.includes(:author, :project)
   end
 
-  def note_authors
-    User.where(id: notes.select(:author_id))
-  end
-
   class << self
     # Searches for snippets with a matching title or file name.
     #
diff --git a/lib/gitlab/closing_issue_extractor.rb b/lib/gitlab/closing_issue_extractor.rb
index 9bef9037ad6..c11a6f22456 100644
--- a/lib/gitlab/closing_issue_extractor.rb
+++ b/lib/gitlab/closing_issue_extractor.rb
@@ -15,13 +15,11 @@ module Gitlab
     def closed_by_message(message)
       return [] if message.nil?
 
-      closing_statements = []
       message.scan(ISSUE_CLOSING_REGEX) do
-        closing_statements << Regexp.last_match[0]
+        text = Regexp.last_match[0]
+        @extractor.analyze(text)
       end
 
-      @extractor.analyze(closing_statements.join(" "))
-
       @extractor.issues
     end
   end
diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb
index 63cfb285e37..9680ebe3fff 100644
--- a/spec/models/concerns/participable_spec.rb
+++ b/spec/models/concerns/participable_spec.rb
@@ -14,13 +14,6 @@ describe Participable, models: true do
 
       expect(model.participant_attrs).to eq([:foo, :bar])
     end
-
-    it 'supports inserting attributes at a specific location' do
-      model.participant(:foo)
-      model.participant(:bar, index: 0)
-
-      expect(model.participant_attrs).to eq([:bar, :foo])
-    end
   end
 
   describe '#participants' do
@@ -34,9 +27,9 @@ describe Participable, models: true do
       project = build(:project, :public)
       instance = model.new
 
-      expect(instance).to receive(:foo).and_return(user2)
-      expect(instance).to receive(:bar).and_return(user3)
-      expect(instance).to receive(:project).and_return(project)
+      allow(instance).to receive(:foo).and_return(user2)
+      allow(instance).to receive(:bar).and_return(user3)
+      allow(instance).to receive(:project).and_return(project)
 
       expect(instance.participants(user1)).to eq([user2, user3])
     end
@@ -51,7 +44,7 @@ describe Participable, models: true do
 
         instance = model.new
 
-        expect(instance).to receive(:project).and_return(project)
+        allow(instance).to receive(:project).and_return(project)
         expect(instance.participants(user1)).to eq([user2])
       end
 
@@ -63,7 +56,7 @@ describe Participable, models: true do
 
         instance = model.new
 
-        expect(instance).to receive(:project).and_return(project)
+        allow(instance).to receive(:project).and_return(project)
         expect(instance.participants(user1)).to eq([user1])
       end
     end