Fix cross references when usernames, milestones, or project names contain underscores.

Remove emphasis from system notes to avoid Markdown conflicts in names.

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.10.0 (unreleased)
+  - Fix cross references when usernames, milestones, or project names contain underscores (Stan Hu)
   - enable line wrapping per default and remove the checkbox to toggle it (Hannes Rosenögger)
   - extend the commit calendar to show the actual commits made on a date (Hannes Rosenögger)
   - Add a service to support external wikis (Hannes Rosenögger)

app/models/note.rb

@@ -59,7 +59,7 @@ class Note < ActiveRecord::Base
 
   class << self
     def create_status_change_note(noteable, project, author, status, source)
-      body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
+      body = "Status changed to #{status}#{' by ' + source.gfm_reference if source}"
 
       create(
         noteable: noteable,
@@ -95,9 +95,9 @@ class Note < ActiveRecord::Base
 
     def create_milestone_change_note(noteable, project, author, milestone)
       body = if milestone.nil?
-               '_Milestone removed_'
+               'Milestone removed'
              else
-               "_Milestone changed to #{milestone.title}_"
+               "Milestone changed to #{milestone.title}"
              end
 
       create(
@@ -110,7 +110,7 @@ class Note < ActiveRecord::Base
     end
 
     def create_assignee_change_note(noteable, project, author, assignee)
-      body = assignee.nil? ? '_Assignee removed_' : "_Reassigned to @#{assignee.username}_"
+      body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}"
 
       create({
         noteable: noteable,
@@ -140,7 +140,7 @@ class Note < ActiveRecord::Base
       end
 
       message << ' ' << 'label'.pluralize(labels_count)
-      body = "_#{message.capitalize}_"
+      body = "#{message.capitalize}"
 
       create(
         noteable: noteable,
@@ -177,7 +177,7 @@ class Note < ActiveRecord::Base
             merge_request.target_branch
           end
 
-        message = "* #{commit_ids} - _#{commits_text} from branch `#{branch}`_"
+        message = "* #{commit_ids} - #{commits_text} from branch `#{branch}`"
         body << message
         body << "\n"
       end
@@ -240,7 +240,7 @@ class Note < ActiveRecord::Base
                 where(noteable_id: noteable.id)
               end
 
-      notes.where('note like ?', cross_reference_note_content(gfm_reference)).
+      notes.where('note like ?', cross_reference_note_pattern(gfm_reference)).
         system.any?
     end
 
@@ -249,13 +249,18 @@ class Note < ActiveRecord::Base
     end
 
     def cross_reference_note_prefix
-      '_mentioned in '
+      'mentioned in '
     end
 
     private
 
     def cross_reference_note_content(gfm_reference)
-      cross_reference_note_prefix + "#{gfm_reference}_"
+      cross_reference_note_prefix + "#{gfm_reference}"
+    end
+
+    def cross_reference_note_pattern(gfm_reference)
+      # Older cross reference notes contained underscores for emphasis
+      "%" + cross_reference_note_content(gfm_reference) + "%"
     end
 
     # Prepend the mentioner's namespaced project path to the GFM reference for

app/services/notification_service.rb

@@ -120,7 +120,7 @@ class NotificationService
     return true unless note.noteable_type.present?
 
     # ignore gitlab service messages
-    return true if note.note.start_with?('_Status changed to closed_')
+    return true if note.note.start_with?('Status changed to closed')
     return true if note.cross_reference? && note.system == true
 
     opts = { noteable_type: note.noteable_type, project_id: note.project_id }

doc/api/merge_requests.md

@@ -375,7 +375,7 @@ Parameters:
     }
   },
   {
-    "note": "_Status changed to closed_",
+    "note": "Status changed to closed",
     "author": {
       "id": 11,
       "username": "admin",

doc/api/notes.md

@@ -21,7 +21,7 @@ Parameters:
 [
   {
     "id": 302,
-    "body": "_Status changed to closed_",
+    "body": "Status changed to closed",
     "attachment": null,
     "author": {
       "id": 1,

spec/lib/gitlab/reference_extractor_spec.rb

@@ -120,4 +120,18 @@ describe Gitlab::ReferenceExtractor do
       expect(extracted[0][1].message).to eq(commit.message)
     end
   end
+
+  context 'with a project with an underscore' do
+    let(:project) { create(:project, path: 'test_project') }
+    let(:issue) { create(:issue, project: project) }
+
+    it 'handles project issue references' do
+      subject.analyze("this refers issue #{project.path_with_namespace}##{issue.iid}",
+          project)
+      extracted = subject.issues_for(project)
+      expect(extracted.size).to eq(1)
+      expect(extracted).to eq([issue])
+    end
+
+  end
 end

spec/models/note_spec.rb

@@ -182,14 +182,14 @@ describe Note do
 
     describe '#note' do
       subject { super().note }
-      it { is_expected.to match(/Status changed to #{status}/) }
+      it { is_expected.to eq("Status changed to #{status}") }
     end
 
     it 'appends a back-reference if a closing mentionable is supplied' do
       commit = double('commit', gfm_reference: 'commit 123456')
       n = Note.create_status_change_note(thing, project, author, status, commit)
 
-      expect(n.note).to match(/Status changed to #{status} by commit 123456/)
+      expect(n.note).to eq("Status changed to #{status} by commit 123456")
     end
   end
 
@@ -197,7 +197,7 @@ describe Note do
     let(:project) { create(:project) }
     let(:thing) { create(:issue, project: project) }
     let(:author) { create(:user) }
-    let(:assignee) { create(:user) }
+    let(:assignee) { create(:user, username: "assigned_user") }
 
     subject { Note.create_assignee_change_note(thing, project, author, assignee) }
 
@@ -227,7 +227,7 @@ describe Note do
 
     describe '#note' do
       subject { super().note }
-      it { is_expected.to match(/Reassigned to @#{assignee.username}/) }
+      it { is_expected.to eq('Reassigned to @assigned_user') }
     end
 
     context 'assignee is removed' do
@@ -235,9 +235,93 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to match(/Assignee removed/) }
+        it { is_expected.to eq('Assignee removed') }
+      end
+    end
+  end
+
+  describe '#create_labels_change_note' do
+    let(:project) { create(:project) }
+    let(:thing) { create(:issue, project: project) }
+    let(:author) { create(:user) }
+    let(:label1) { create(:label) }
+    let(:label2) { create(:label) }
+    let(:added_labels) { [label1, label2] }
+    let(:removed_labels) { [] }
+
+    subject { Note.create_labels_change_note(thing, project, author, added_labels, removed_labels) }
+
+    context 'creates and saves a Note' do
+      it { is_expected.to be_a Note }
+
+      describe '#id' do
+        subject { super().id }
+        it { is_expected.not_to be_nil }
       end
     end
+
+    describe '#noteable' do
+      subject { super().noteable }
+      it { is_expected.to eq(thing) }
+    end
+
+    describe '#project' do
+      subject { super().project }
+      it { is_expected.to eq(thing.project) }
+    end
+
+    describe '#author' do
+      subject { super().author }
+      it { is_expected.to eq(author) }
+    end
+
+    describe '#note' do
+      subject { super().note }
+      it { is_expected.to eq("Added ~#{label1.id} ~#{label2.id} labels") }
+    end
+
+    context 'label is removed' do
+      let(:added_labels) { [label1]  }
+      let(:removed_labels) { [label2] }
+
+      describe '#note' do
+        subject { super().note }
+        it { is_expected.to eq("Added ~#{label1.id} and removed ~#{label2.id} labels") }
+      end
+    end
+  end
+
+  describe '#create_milestone_change_note' do
+    let(:project) { create(:project) }
+    let(:thing) { create(:issue, project: project) }
+    let(:milestone) { create(:milestone, project: project, title: "first_milestone") }
+    let(:author) { create(:user) }
+
+    subject { Note.create_milestone_change_note(thing, project, author, milestone) }
+
+    context 'creates and saves a Note' do
+      it { is_expected.to be_a Note }
+
+      describe '#id' do
+        subject { super().id }
+        it { is_expected.not_to be_nil }
+      end
+    end
+
+    describe '#project' do
+      subject { super().project }
+      it { is_expected.to eq(thing.project) }
+    end
+
+    describe '#author' do
+      subject { super().author }
+      it { is_expected.to eq(author) }
+    end
+
+    describe '#note' do
+      subject { super().note }
+      it { is_expected.to eq("Milestone changed to first_milestone") }
+    end
   end
 
   describe '#create_cross_reference_note' do
@@ -272,7 +356,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in merge request !#{mergereq.iid}_") }
+        it { is_expected.to eq("mentioned in merge request !#{mergereq.iid}") }
       end
     end
 
@@ -288,7 +372,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in commit #{commit.sha}_") }
+        it { is_expected.to eq("mentioned in commit #{commit.sha}") }
       end
     end
 
@@ -309,7 +393,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in issue ##{issue.iid}_") }
+        it { is_expected.to eq("mentioned in issue ##{issue.iid}") }
       end
     end
 
@@ -330,7 +414,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in merge request !#{mergereq.iid}_") }
+        it { is_expected.to eq("mentioned in merge request !#{mergereq.iid}") }
       end
     end
 
@@ -362,7 +446,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in issue ##{issue.iid}_") }
+        it { is_expected.to eq("mentioned in issue ##{issue.iid}") }
       end
     end
 
@@ -389,7 +473,7 @@ describe Note do
 
       describe '#note' do
         subject { super().note }
-        it { is_expected.to eq("_mentioned in commit #{parent_commit.id}_") }
+        it { is_expected.to eq("mentioned in commit #{parent_commit.id}") }
       end
     end
   end
@@ -421,6 +505,41 @@ describe Note do
       it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy }
       it { expect(Note.cross_reference_exists?(commit1, commit0)).to be_falsey }
     end
+
+    context 'legacy note with Markdown emphasis' do
+      let(:issue2) { create :issue, project: project }
+      let!(:note) do
+        create :note, system: true, noteable_id: issue2.id,
+                      noteable_type: "Issue", note: "_mentioned in issue " \
+                      "#{issue.project.path_with_namespace}##{issue.iid}_"
+      end
+
+      it 'detects if a mentionable with emphasis has been mentioned' do
+        expect(Note.cross_reference_exists?(issue2, issue)).to be_truthy
+      end
+    end
+  end
+
+  describe '#cross_references_with_underscores?' do
+    let(:project) { create :project, path: "first_project" }
+    let(:second_project) { create :project, path: "second_project" }
+
+    let(:author) { create :user }
+    let(:issue0) { create :issue, project: project }
+    let(:issue1) { create :issue, project: second_project }
+    let!(:note) { Note.create_cross_reference_note(issue0, issue1, author, project) }
+
+    it 'detects if a mentionable has already been mentioned' do
+      expect(Note.cross_reference_exists?(issue0, issue1)).to be_truthy
+    end
+
+    it 'detects if a mentionable has not already been mentioned' do
+      expect(Note.cross_reference_exists?(issue1, issue0)).to be_falsey
+    end
+
+    it 'detects that text has underscores' do
+      expect(note.note).to eq("mentioned in issue #{second_project.path_with_namespace}##{issue1.iid}")
+    end
   end
 
   describe '#system?' do
@@ -429,6 +548,8 @@ describe Note do
     let(:other)   { create(:issue, project: project) }
     let(:author)  { create(:user) }
     let(:assignee) { create(:user) }
+    let(:label) { create(:label) }
+    let(:milestone) { create(:milestone) }
 
     it 'should recognize user-supplied notes as non-system' do
       @note = create(:note_on_issue)
@@ -449,6 +570,16 @@ describe Note do
       @note = Note.create_assignee_change_note(issue, project, author, assignee)
       expect(@note).to be_system
     end
+
+    it 'should identify label-change notes as system notes' do
+      @note = Note.create_labels_change_note(issue, project, author, [label], [])
+      expect(@note).to be_system
+    end
+
+    it 'should identify milestone-change notes as system notes' do
+      @note = Note.create_milestone_change_note(issue, project, author, milestone)
+      expect(@note).to be_system
+    end
   end
 
   describe :authorization do