Commit f562e69e authored by Sean McGivern's avatar Sean McGivern

Merge branch '39461-notes-api-for-issues-no-longer-returns-label-additions-removals' into 'master'

Resolve "Notes API for issues no longer returns label additions/removals"

Closes #39461

See merge request gitlab-org/gitlab-ce!15080
parents 9024875e c900c21e
...@@ -110,6 +110,7 @@ class Note < ActiveRecord::Base ...@@ -110,6 +110,7 @@ class Note < ActiveRecord::Base
includes(:author, :noteable, :updated_by, includes(:author, :noteable, :updated_by,
project: [:project_members, { group: [:group_members] }]) project: [:project_members, { group: [:group_members] }])
end end
scope :with_metadata, -> { includes(:system_note_metadata) }
after_initialize :ensure_discussion_id after_initialize :ensure_discussion_id
before_validation :nullify_blank_type, :nullify_blank_line_code before_validation :nullify_blank_type, :nullify_blank_line_code
...@@ -169,7 +170,13 @@ class Note < ActiveRecord::Base ...@@ -169,7 +170,13 @@ class Note < ActiveRecord::Base
end end
def cross_reference? def cross_reference?
system? && matches_cross_reference_regex? return unless system?
if force_cross_reference_regex_check?
matches_cross_reference_regex?
else
SystemNoteService.cross_reference?(note)
end
end end
def diff_note? def diff_note?
...@@ -382,4 +389,10 @@ class Note < ActiveRecord::Base ...@@ -382,4 +389,10 @@ class Note < ActiveRecord::Base
def set_discussion_id def set_discussion_id
self.discussion_id ||= discussion_class.discussion_id(self) self.discussion_id ||= discussion_class.discussion_id(self)
end end
def force_cross_reference_regex_check?
return unless system?
SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.include?(system_note_metadata&.action)
end
end end
class SystemNoteMetadata < ActiveRecord::Base class SystemNoteMetadata < ActiveRecord::Base
# These notes's action text might contain a reference that is external.
# We should always force a deep validation upon references that are found
# in this note type.
# 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[
commit cross_reference
close duplicate
].freeze
ICON_TYPES = %w[ ICON_TYPES = %w[
commit description merge confidential visible label assignee cross_reference commit description merge confidential visible label assignee cross_reference
title time_tracking branch milestone discussion task moved title time_tracking branch milestone discussion task moved
......
...@@ -583,6 +583,10 @@ module SystemNoteService ...@@ -583,6 +583,10 @@ module SystemNoteService
create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action)) create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action))
end end
def cross_reference?(note_text)
note_text =~ /\A#{cross_reference_note_prefix}/i
end
private private
def notes_for_mentioner(mentioner, noteable, notes) def notes_for_mentioner(mentioner, noteable, notes)
......
---
title: Label addition/removal are not going to be redacted wrongfully in the API.
merge_request: 15080
author:
type: fixed
...@@ -33,7 +33,7 @@ module API ...@@ -33,7 +33,7 @@ module API
# paginate() only works with a relation. This could lead to a # paginate() only works with a relation. This could lead to a
# mismatch between the pagination headers info and the actual notes # mismatch between the pagination headers info and the actual notes
# array returned, but this is really a edge-case. # 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) } .reject { |n| n.cross_reference_not_visible_for?(current_user) }
present notes, with: Entities::Note present notes, with: Entities::Note
else else
...@@ -50,7 +50,7 @@ module API ...@@ -50,7 +50,7 @@ module API
end end
get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
noteable = find_project_noteable(noteables_str, params[:noteable_id]) 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) can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
if can_read_note if can_read_note
......
...@@ -231,6 +231,37 @@ describe Note do ...@@ -231,6 +231,37 @@ describe Note do
end end
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 describe 'clear_blank_line_code!' do
it 'clears a blank line code before validation' do it 'clears a blank line code before validation' do
note = build(:note, line_code: ' ') note = build(:note, line_code: ' ')
......
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