diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/models/note.rb | 25 | ||||
| -rw-r--r-- | app/services/notification_service.rb | 2 | ||||
| -rw-r--r-- | doc/api/merge_requests.md | 2 | ||||
| -rw-r--r-- | doc/api/notes.md | 2 | ||||
| -rw-r--r-- | spec/lib/gitlab/reference_extractor_spec.rb | 14 | ||||
| -rw-r--r-- | spec/models/note_spec.rb | 153 |
7 files changed, 175 insertions, 24 deletions
diff --git a/CHANGELOG b/CHANGELOG index b6c39ae5f71..0897a712eaf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ 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) diff --git a/app/models/note.rb b/app/models/note.rb index 649e9b4e852..27b583a869a 100644 --- a/app/models/note.rb +++ b/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, @@ -170,14 +170,14 @@ class Note < ActiveRecord::Base commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit') - branch = + branch = if merge_request.for_fork? "#{merge_request.target_project_namespace}:#{merge_request.target_branch}" else 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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 848ed77ebf8..cc5853144c5 100644 --- a/app/services/notification_service.rb +++ b/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 } diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 1f3fd26a241..6a272539e45 100644 --- a/doc/api/merge_requests.md +++ b/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", diff --git a/doc/api/notes.md b/doc/api/notes.md index c22e493562a..ee2f9fa0eac 100644 --- a/doc/api/notes.md +++ b/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, diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 034f8ee7c45..5ebe44f6fb7 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 17cb439c90e..a7bf5081d5b 100644 --- a/spec/models/note_spec.rb +++ b/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,11 +235,95 @@ 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 let(:project) { create(:project) } let(:author) { create(:user) } @@ -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 |