Commit 623f6c5f authored by Hannes Rosenögger's avatar Hannes Rosenögger

Merge branch 'fix-underscore-notes' into 'master'

Fix cross-references that contain underscores

### What does this MR do?

Cross-references that contain underscores confuse the Markdown renderer and don't work properly. For example:

1. In  `myacct/project_one#456`, add a comment that includes a reference to `myacct/project_two#123`.
2. The comment in `myacct/project_one#456` links correctly.
3. The system note in `myacct/project_two#123` is incorrectly linked as:

`mentioned in issue myacct/projectone#456_`

This MR removes the use of the `_` character in the system notes to prevent Markdown confusion.

See a full discussion in #1160.

### Are there points in the code the reviewer needs to double check?

To preserve backwards compatibility with existing system notes, there is a SQL wildcard match for notes having underscores. This seemed safer than migrating all notes.

### Why was this MR needed?

In an ideal world, the Markdown parser would be able to handle references containing underscores with or without escape sequences. However, there are a number of issues:

1. RedCarpet's parser for emphasis block is pretty dumb: it treats `#` as a word break and thus even the `intra_word_emphasis` flag has no effect.
2. The parser is in C and can't be changed easily.
3. There is no way to insert an escape sequence for emphasis blocks.

The only way around this is to switch to something like CommonMark, which does support escape sequences.

### What are the relevant issue numbers / [Feature requests](http://feedback.gitlab.com/)?

Issue #1160

### Screenshots

Before:

![Screen_Shot_2015-03-19_at_1.39.29_AM](https://gitlab.com/gitlab-org/gitlab-ce/uploads/a1db4b96c0df2f10d9855ed5abe976ca/Screen_Shot_2015-03-19_at_1.39.29_AM.png)

After:

![Screen_Shot_2015-03-19_at_1.39.35_AM](https://gitlab.com/gitlab-org/gitlab-ce/uploads/189a062c2d19645eec1782ac1f0d4f2f/Screen_Shot_2015-03-19_at_1.39.35_AM.png)

See merge request !385
parents 01241181 fad71576
...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. ...@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 7.10.0 (unreleased) v 7.10.0 (unreleased)
- Update poltergeist to version 1.6.0 to support PhantomJS 2.0 (Zeger-Jan van de Weg) - Update poltergeist to version 1.6.0 to support PhantomJS 2.0 (Zeger-Jan van de Weg)
- 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) - 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) - 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) - Add a service to support external wikis (Hannes Rosenögger)
......
...@@ -59,7 +59,7 @@ class Note < ActiveRecord::Base ...@@ -59,7 +59,7 @@ class Note < ActiveRecord::Base
class << self class << self
def create_status_change_note(noteable, project, author, status, source) 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( create(
noteable: noteable, noteable: noteable,
...@@ -95,9 +95,9 @@ class Note < ActiveRecord::Base ...@@ -95,9 +95,9 @@ class Note < ActiveRecord::Base
def create_milestone_change_note(noteable, project, author, milestone) def create_milestone_change_note(noteable, project, author, milestone)
body = if milestone.nil? body = if milestone.nil?
'_Milestone removed_' 'Milestone removed'
else else
"_Milestone changed to #{milestone.title}_" "Milestone changed to #{milestone.title}"
end end
create( create(
...@@ -110,7 +110,7 @@ class Note < ActiveRecord::Base ...@@ -110,7 +110,7 @@ class Note < ActiveRecord::Base
end end
def create_assignee_change_note(noteable, project, author, assignee) 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({ create({
noteable: noteable, noteable: noteable,
...@@ -140,7 +140,7 @@ class Note < ActiveRecord::Base ...@@ -140,7 +140,7 @@ class Note < ActiveRecord::Base
end end
message << ' ' << 'label'.pluralize(labels_count) message << ' ' << 'label'.pluralize(labels_count)
body = "_#{message.capitalize}_" body = "#{message.capitalize}"
create( create(
noteable: noteable, noteable: noteable,
...@@ -170,14 +170,14 @@ class Note < ActiveRecord::Base ...@@ -170,14 +170,14 @@ class Note < ActiveRecord::Base
commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit') commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit')
branch = branch =
if merge_request.for_fork? if merge_request.for_fork?
"#{merge_request.target_project_namespace}:#{merge_request.target_branch}" "#{merge_request.target_project_namespace}:#{merge_request.target_branch}"
else else
merge_request.target_branch merge_request.target_branch
end end
message = "* #{commit_ids} - _#{commits_text} from branch `#{branch}`_" message = "* #{commit_ids} - #{commits_text} from branch `#{branch}`"
body << message body << message
body << "\n" body << "\n"
end end
...@@ -240,7 +240,7 @@ class Note < ActiveRecord::Base ...@@ -240,7 +240,7 @@ class Note < ActiveRecord::Base
where(noteable_id: noteable.id) where(noteable_id: noteable.id)
end end
notes.where('note like ?', cross_reference_note_content(gfm_reference)). notes.where('note like ?', cross_reference_note_pattern(gfm_reference)).
system.any? system.any?
end end
...@@ -249,13 +249,18 @@ class Note < ActiveRecord::Base ...@@ -249,13 +249,18 @@ class Note < ActiveRecord::Base
end end
def cross_reference_note_prefix def cross_reference_note_prefix
'_mentioned in ' 'mentioned in '
end end
private private
def cross_reference_note_content(gfm_reference) 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 end
# Prepend the mentioner's namespaced project path to the GFM reference for # Prepend the mentioner's namespaced project path to the GFM reference for
......
...@@ -120,7 +120,7 @@ class NotificationService ...@@ -120,7 +120,7 @@ class NotificationService
return true unless note.noteable_type.present? return true unless note.noteable_type.present?
# ignore gitlab service messages # 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 return true if note.cross_reference? && note.system == true
opts = { noteable_type: note.noteable_type, project_id: note.project_id } opts = { noteable_type: note.noteable_type, project_id: note.project_id }
......
...@@ -375,7 +375,7 @@ Parameters: ...@@ -375,7 +375,7 @@ Parameters:
} }
}, },
{ {
"note": "_Status changed to closed_", "note": "Status changed to closed",
"author": { "author": {
"id": 11, "id": 11,
"username": "admin", "username": "admin",
......
...@@ -21,7 +21,7 @@ Parameters: ...@@ -21,7 +21,7 @@ Parameters:
[ [
{ {
"id": 302, "id": 302,
"body": "_Status changed to closed_", "body": "Status changed to closed",
"attachment": null, "attachment": null,
"author": { "author": {
"id": 1, "id": 1,
......
...@@ -120,4 +120,18 @@ describe Gitlab::ReferenceExtractor do ...@@ -120,4 +120,18 @@ describe Gitlab::ReferenceExtractor do
expect(extracted[0][1].message).to eq(commit.message) expect(extracted[0][1].message).to eq(commit.message)
end end
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 end
...@@ -182,14 +182,14 @@ describe Note do ...@@ -182,14 +182,14 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } subject { super().note }
it { is_expected.to match(/Status changed to #{status}/) } it { is_expected.to eq("Status changed to #{status}") }
end end
it 'appends a back-reference if a closing mentionable is supplied' do it 'appends a back-reference if a closing mentionable is supplied' do
commit = double('commit', gfm_reference: 'commit 123456') commit = double('commit', gfm_reference: 'commit 123456')
n = Note.create_status_change_note(thing, project, author, status, commit) 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
end end
...@@ -197,7 +197,7 @@ describe Note do ...@@ -197,7 +197,7 @@ describe Note do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:thing) { create(:issue, project: project) } let(:thing) { create(:issue, project: project) }
let(:author) { create(:user) } 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) } subject { Note.create_assignee_change_note(thing, project, author, assignee) }
...@@ -227,7 +227,7 @@ describe Note do ...@@ -227,7 +227,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } subject { super().note }
it { is_expected.to match(/Reassigned to @#{assignee.username}/) } it { is_expected.to eq('Reassigned to @assigned_user') }
end end
context 'assignee is removed' do context 'assignee is removed' do
...@@ -235,11 +235,95 @@ describe Note do ...@@ -235,11 +235,95 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } subject { super().note }
it { is_expected.to match(/Assignee removed/) } it { is_expected.to eq('Assignee removed') }
end end
end 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 describe '#create_cross_reference_note' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:author) { create(:user) } let(:author) { create(:user) }
...@@ -272,7 +356,7 @@ describe Note do ...@@ -272,7 +356,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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
end end
...@@ -288,7 +372,7 @@ describe Note do ...@@ -288,7 +372,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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
end end
...@@ -309,7 +393,7 @@ describe Note do ...@@ -309,7 +393,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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
end end
...@@ -330,7 +414,7 @@ describe Note do ...@@ -330,7 +414,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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
end end
...@@ -362,7 +446,7 @@ describe Note do ...@@ -362,7 +446,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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
end end
...@@ -389,7 +473,7 @@ describe Note do ...@@ -389,7 +473,7 @@ describe Note do
describe '#note' do describe '#note' do
subject { super().note } 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 end
end end
...@@ -421,6 +505,41 @@ describe Note do ...@@ -421,6 +505,41 @@ describe Note do
it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy } it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy }
it { expect(Note.cross_reference_exists?(commit1, commit0)).to be_falsey } it { expect(Note.cross_reference_exists?(commit1, commit0)).to be_falsey }
end 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 end
describe '#system?' do describe '#system?' do
...@@ -429,6 +548,8 @@ describe Note do ...@@ -429,6 +548,8 @@ describe Note do
let(:other) { create(:issue, project: project) } let(:other) { create(:issue, project: project) }
let(:author) { create(:user) } let(:author) { create(:user) }
let(:assignee) { create(:user) } let(:assignee) { create(:user) }
let(:label) { create(:label) }
let(:milestone) { create(:milestone) }
it 'should recognize user-supplied notes as non-system' do it 'should recognize user-supplied notes as non-system' do
@note = create(:note_on_issue) @note = create(:note_on_issue)
...@@ -449,6 +570,16 @@ describe Note do ...@@ -449,6 +570,16 @@ describe Note do
@note = Note.create_assignee_change_note(issue, project, author, assignee) @note = Note.create_assignee_change_note(issue, project, author, assignee)
expect(@note).to be_system expect(@note).to be_system
end 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 end
describe :authorization do describe :authorization do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment