Merge branch 'rs-mentionable-examples-refactor' into 'master'

Update mentionable shared examples to be (a bit) more understandable

These shared examples were super confusing, and honestly still kind of are, but they should be a bit better now.

I encountered a bunch of headaches in these examples while refactoring ReferenceExtractor, so that's how I ended up changing it.

See merge request !1780

app/models/note.rb

@@ -238,9 +238,9 @@ class Note < ActiveRecord::Base
     def cross_reference_exists?(noteable, mentioner)
       gfm_reference = mentioner_gfm_ref(noteable, mentioner)
       notes = if noteable.is_a?(Commit)
-                where(commit_id: noteable.id)
+                where(commit_id: noteable.id, noteable_type: 'Commit')
               else
-                where(noteable_id: noteable.id)
+                where(noteable_id: noteable.id, noteable_type: noteable.class)
               end
 
       notes.where('note like ?', cross_reference_note_pattern(gfm_reference)).

spec/models/commit_spec.rb

@@ -69,8 +69,9 @@ eos
   end
 
   it_behaves_like 'a mentionable' do
-    let(:subject) { commit }
-    let(:mauthor) { create :user, email: commit.author_email }
+    subject { commit }
+
+    let(:author) { create(:user, email: commit.author_email) }
     let(:backref_text) { "commit #{subject.id}" }
     let(:set_mentionable_text) { ->(txt){ subject.stub(safe_message: txt) } }
 

spec/models/issue_spec.rb

@@ -56,7 +56,8 @@ describe Issue do
   end
 
   it_behaves_like 'an editable mentionable' do
-    let(:subject) { create :issue, project: mproject }
+    subject { create(:issue, project: project) }
+
     let(:backref_text) { "issue ##{subject.iid}" }
     let(:set_mentionable_text) { ->(txt){ subject.description = txt } }
   end

spec/models/merge_request_spec.rb

@@ -116,12 +116,13 @@ describe MergeRequest do
   end
 
   it_behaves_like 'an editable mentionable' do
-    let(:subject) { create :merge_request, source_project: mproject, target_project: mproject }
+    subject { create(:merge_request, source_project: project, target_project: project) }
+
     let(:backref_text) { "merge request !#{subject.iid}" }
     let(:set_mentionable_text) { ->(txt){ subject.title = txt } }
   end
 
   it_behaves_like 'a Taskable' do
-    let(:subject) { create :merge_request, :simple }
+    subject { create :merge_request, :simple }
   end
 end

spec/models/note_spec.rb

@@ -629,8 +629,9 @@ describe Note do
   end
 
   it_behaves_like 'an editable mentionable' do
+    subject { create :note, noteable: issue, project: project }
+
     let(:issue) { create :issue, project: project }
-    let(:subject) { create :note, noteable: issue, project: project }
     let(:backref_text) { issue.gfm_reference }
     let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
   end

spec/support/mentionable_shared_examples.rb

 # Specifications for behavior common to all Mentionable implementations.
 # Requires a shared context containing:
-# - let(:subject) { "the mentionable implementation" }
+# - subject { "the mentionable implementation" }
 # - let(:backref_text) { "the way that +subject+ should refer to itself in backreferences " }
 # - let(:set_mentionable_text) { lambda { |txt| "block that assigns txt to the subject's mentionable_text" } }
 
 def common_mentionable_setup
-  # Avoid name collisions with let(:project) or let(:author) in the surrounding scope.
-  let(:mproject) { create :project }
-  let(:mauthor) { subject.author }
-
-  let(:mentioned_issue) { create :issue, project: mproject }
-  let(:other_issue) { create :issue, project: mproject }
-  let(:mentioned_mr) { create :merge_request, :simple, source_project: mproject }
-  let(:mentioned_commit) { double('commit', sha: '1234567890abcdef').as_null_object }
-
-  let(:ext_proj) { create :project, :public }
-  let(:ext_issue) { create :issue, project: ext_proj }
-  let(:other_ext_issue) { create :issue, project: ext_proj }
-  let(:ext_mr) { create :merge_request, :simple, source_project: ext_proj }
+  let(:project) { create :project }
+  let(:author)  { subject.author }
+
+  let(:mentioned_issue)  { create(:issue, project: project) }
+  let(:mentioned_mr)     { create(:merge_request, :simple, source_project: project) }
+  let(:mentioned_commit) { project.repository.commit }
+
+  let(:ext_proj)   { create(:project, :public) }
+  let(:ext_issue)  { create(:issue, project: ext_proj) }
+  let(:ext_mr)     { create(:merge_request, :simple, source_project: ext_proj) }
   let(:ext_commit) { ext_proj.repository.commit }
 
   # Override to add known commits to the repository stub.
@@ -26,20 +23,37 @@ def common_mentionable_setup
   # A string that mentions each of the +mentioned_.*+ objects above. Mentionables should add a self-reference
   # to this string and place it in their +mentionable_text+.
   let(:ref_string) do
-    "mentions ##{mentioned_issue.iid} twice ##{mentioned_issue.iid}, " +
-    "!#{mentioned_mr.iid}, " +
-    "#{ext_proj.path_with_namespace}##{ext_issue.iid}, " +
-    "#{ext_proj.path_with_namespace}!#{ext_mr.iid}, " +
-    "#{ext_proj.path_with_namespace}@#{ext_commit.short_id}, " +
-    "#{mentioned_commit.sha[0..10]} and itself as #{backref_text}"
+    cross = ext_proj.path_with_namespace
+
+    <<-MSG.strip_heredoc
+      These references are new:
+        Issue:  ##{mentioned_issue.iid}
+        Merge:  !#{mentioned_mr.iid}
+        Commit: #{mentioned_commit.id}
+
+      This reference is a repeat and should only be mentioned once:
+        Repeat: ##{mentioned_issue.iid}
+
+      These references are cross-referenced:
+        Issue:  #{cross}##{ext_issue.iid}
+        Merge:  #{cross}!#{ext_mr.iid}
+        Commit: #{cross}@#{ext_commit.short_id}
+
+      This is a self-reference and should not be mentioned at all:
+        Self: #{backref_text}
+    MSG
   end
 
   before do
-    # Wire the project's repository to return the mentioned commit, and +nil+ for any
-    # unrecognized commits.
-    commitmap = { '1234567890a' => mentioned_commit }
+    # Wire the project's repository to return the mentioned commit, and +nil+
+    # for any unrecognized commits.
+    commitmap = {
+      mentioned_commit.id => mentioned_commit
+    }
     extra_commits.each { |c| commitmap[c.short_id] = c }
-    allow(mproject.repository).to receive(:commit) { |sha| commitmap[sha] }
+
+    allow(project.repository).to receive(:commit) { |sha| commitmap[sha] }
+
     set_mentionable_text.call(ref_string)
   end
 end
@@ -53,7 +67,7 @@ shared_examples 'a mentionable' do
 
   it "extracts references from its reference property" do
     # De-duplicate and omit itself
-    refs = subject.references(mproject)
+    refs = subject.references(project)
     expect(refs.size).to eq(6)
     expect(refs).to include(mentioned_issue)
     expect(refs).to include(mentioned_mr)
@@ -68,17 +82,18 @@ shared_examples 'a mentionable' do
                          ext_issue, ext_mr, ext_commit]
 
     mentioned_objects.each do |referenced|
-      expect(Note).to receive(:create_cross_reference_note).with(referenced, subject.local_reference, mauthor, mproject)
+      expect(Note).to receive(:create_cross_reference_note).
+        with(referenced, subject.local_reference, author, project)
     end
 
-    subject.create_cross_references!(mproject, mauthor)
+    subject.create_cross_references!(project, author)
   end
 
   it 'detects existing cross-references' do
-    Note.create_cross_reference_note(mentioned_issue, subject.local_reference, mauthor, mproject)
+    Note.create_cross_reference_note(mentioned_issue, subject.local_reference, author, project)
 
-    expect(subject.has_mentioned?(mentioned_issue)).to be_truthy
-    expect(subject.has_mentioned?(mentioned_mr)).to be_falsey
+    expect(subject).to have_mentioned(mentioned_issue)
+    expect(subject).not_to have_mentioned(mentioned_mr)
   end
 end
 
@@ -87,29 +102,41 @@ shared_examples 'an editable mentionable' do
 
   it_behaves_like 'a mentionable'
 
+  let(:new_issues) do
+    [create(:issue, project: project), create(:issue, project: ext_proj)]
+  end
+
   it 'creates new cross-reference notes when the mentionable text is edited' do
-    new_text = "still mentions ##{mentioned_issue.iid}, " +
-      "#{mentioned_commit.sha[0..10]}, " +
-      "#{ext_issue.iid}, " +
-      "new refs: ##{other_issue.iid}, " +
-      "#{ext_proj.path_with_namespace}##{other_ext_issue.iid}"
+    cross = ext_proj.path_with_namespace
+
+    new_text = <<-MSG
+      These references already existed:
+        Issue:  ##{mentioned_issue.iid}
+        Commit: #{mentioned_commit.id}
+
+      This cross-project reference already existed:
+        Issue:  #{cross}##{ext_issue.iid}
+
+      These two references are introduced in an edit:
+        Issue: ##{new_issues[0].iid}
+        Cross: #{cross}##{new_issues[1].iid}
+    MSG
 
+    # These three objects were already referenced, and should not receive new
+    # notes
     [mentioned_issue, mentioned_commit, ext_issue].each do |oldref|
-      expect(Note).not_to receive(:create_cross_reference_note).with(oldref, subject.local_reference,
-        mauthor, mproject)
+      expect(Note).not_to receive(:create_cross_reference_note).
+        with(oldref, any_args)
     end
 
-    [other_issue, other_ext_issue].each do |newref|
-      expect(Note).to receive(:create_cross_reference_note).with(
-        newref,
-        subject.local_reference,
-        mauthor,
-        mproject
-      )
+    # These two issues are new and should receive reference notes
+    new_issues.each do |newref|
+      expect(Note).to receive(:create_cross_reference_note).
+        with(newref, subject.local_reference, author, project)
     end
 
     subject.save
     set_mentionable_text.call(new_text)
-    subject.notice_added_references(mproject, mauthor)
+    subject.notice_added_references(project, author)
   end
 end

spec/support/taskable_shared_examples.rb

 # Specs for task state functionality for issues and merge requests.
 #
 # Requires a context containing:
-#   let(:subject) { Issue or MergeRequest }
+#   subject { Issue or MergeRequest }
 shared_examples 'a Taskable' do
   before do
     subject.description = <<EOT.gsub(/ {6}/, '')