From 4374a38f0e8fbd856458a60f47083fb4d084d695 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Wed, 15 Nov 2017 09:12:04 -0500 Subject: [PATCH 1/6] fix the commit reference pattern from matching other entities --- app/models/commit.rb | 2 +- ...onger-returns-label-additions-removals.yml | 5 ++++ spec/models/commit_spec.rb | 23 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/39461-notes-api-for-issues-no-longer-returns-label-additions-removals.yml diff --git a/app/models/commit.rb b/app/models/commit.rb index 6dba154a6ea..e28bf28fab3 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze + COMMIT_SHA_PATTERN = /\b(? Date: Wed, 15 Nov 2017 10:56:17 -0500 Subject: [PATCH 2/6] try to fix the failing spec on external refs --- app/models/commit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index e28bf28fab3..b5411b2bf75 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\b(? Date: Thu, 16 Nov 2017 09:23:32 -0500 Subject: [PATCH 3/6] reverting to the simpler approach --- app/models/commit.rb | 2 +- app/models/note.rb | 14 +++++++++++++- app/models/system_note_metadata.rb | 10 ++++++++++ app/services/system_note_service.rb | 4 ++++ spec/models/commit_spec.rb | 23 ----------------------- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index b5411b2bf75..6dba154a6ea 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /(? Date: Thu, 16 Nov 2017 11:03:15 -0500 Subject: [PATCH 4/6] fix the linting error --- app/models/system_note_metadata.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 4065c1594c3..ca0673080af 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -7,7 +7,7 @@ class SystemNoteMetadata < ActiveRecord::Base TYPES_WITH_CROSS_REFERENCES = %w[ cross_reference milestone - ] + ].freeze ICON_TYPES = %w[ commit description merge confidential visible label assignee cross_reference From 9ed91479a7bbca1e420cd91a6322493d7ffda749 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Mon, 20 Nov 2017 13:00:35 -0500 Subject: [PATCH 5/6] add the missing spec --- app/models/system_note_metadata.rb | 4 ++-- spec/models/note_spec.rb | 31 ++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index ca0673080af..29035480371 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -5,8 +5,8 @@ class SystemNoteMetadata < ActiveRecord::Base # Other notes can always be safely shown as all its references are # in the same project (i.e. with the same permissions) TYPES_WITH_CROSS_REFERENCES = %w[ - cross_reference - milestone + commit cross_reference + close duplicate ].freeze ICON_TYPES = %w[ diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 1ecb50586c7..6e7e8c4c570 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -231,6 +231,37 @@ describe Note do end end + describe '#cross_reference?' do + it 'falsey for user-generated notes' do + note = create(:note, system: false) + + expect(note.cross_reference?).to be_falsy + end + + context 'when the note might contain cross references' do + SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.each do |type| + let(:note) { create(:note, :system) } + let!(:metadata) { create(:system_note_metadata, note: note, action: type) } + + it 'delegates to the cross-reference regex' do + expect(note).to receive(:matches_cross_reference_regex?).and_return(false) + + note.cross_reference? + end + end + end + + context 'when the note cannot contain cross references' do + let(:commit_note) { build(:note, note: 'mentioned in 1312312313 something else.', system: true) } + let(:label_note) { build(:note, note: 'added ~2323232323', system: true) } + + it 'scan for a `mentioned in` prefix' do + expect(commit_note.cross_reference?).to be_truthy + expect(label_note.cross_reference?).to be_falsy + end + end + end + describe 'clear_blank_line_code!' do it 'clears a blank line code before validation' do note = build(:note, line_code: ' ') From c900c21eef9235306d7d0da42b07aa2de346e263 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Tue, 21 Nov 2017 08:31:23 -0500 Subject: [PATCH 6/6] add `#with_metadata` scope to remove a N+1 from the notes' API --- app/models/note.rb | 1 + lib/api/notes.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/note.rb b/app/models/note.rb index 4bbb54ba9bf..50c9caf8529 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -110,6 +110,7 @@ class Note < ActiveRecord::Base includes(:author, :noteable, :updated_by, project: [:project_members, { group: [:group_members] }]) end + scope :with_metadata, -> { includes(:system_note_metadata) } after_initialize :ensure_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 0b9ab4eeb05..ceaaeca4046 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -33,7 +33,7 @@ module API # paginate() only works with a relation. This could lead to a # mismatch between the pagination headers info and the actual notes # array returned, but this is really a edge-case. - paginate(noteable.notes) + paginate(noteable.notes.with_metadata) .reject { |n| n.cross_reference_not_visible_for?(current_user) } present notes, with: Entities::Note else @@ -50,7 +50,7 @@ module API end get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do noteable = find_project_noteable(noteables_str, params[:noteable_id]) - note = noteable.notes.find(params[:note_id]) + note = noteable.notes.with_metadata.find(params[:note_id]) can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user) if can_read_note