diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 817aac8b5d5..83bf015488f 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -34,8 +34,9 @@ class NotesFinder target_type = @params[:target_type] target_id = @params[:target_id] + target_iid = @params[:target_iid] - return @target = nil unless target_type && target_id + return @target = nil unless target_type && (target_id || target_iid) @target = if target_type == "commit" @@ -43,12 +44,22 @@ class NotesFinder @project.commit(target_id) end else - noteables_for_type(target_type).find(target_id) + noteables_for_type_by_id(target_type, target_id, target_iid) end end private + def noteables_for_type_by_id(type, id, iid) + query = if id + { id: id } + else + { iid: iid } + end + + noteables_for_type(type).find_by!(query) # rubocop: disable CodeReuse/ActiveRecord + end + def init_collection if target notes_on_target diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 693172b7d08..cc62ce22a1b 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -25,7 +25,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = noteable.notes .inc_relations_for_view @@ -47,7 +47,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -82,7 +82,7 @@ module API end end post ":id/#{noteables_path}/:noteable_id/discussions" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) type = params[:position] ? 'DiffNote' : 'DiscussionNote' id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id @@ -112,7 +112,7 @@ module API requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) if notes.empty? @@ -132,7 +132,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) notes = readable_discussion_notes(noteable, params[:discussion_id]) first_note = notes.first @@ -166,7 +166,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -183,7 +183,7 @@ module API exactly_one_of :body, :resolved end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) if params[:resolved].nil? update_note(noteable, params[:note_id]) @@ -201,7 +201,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end @@ -216,7 +216,7 @@ module API requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved' end put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) resolve_discussion(noteable, params[:discussion_id], params[:resolved]) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 00bcf6b055b..70b5064c72a 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -183,11 +183,6 @@ module API user_project.commit_by(oid: id) end - def find_project_snippet(id) - finder_params = { project: user_project } - SnippetsFinder.new(current_user, finder_params).find(id) - end - # rubocop: disable CodeReuse/ActiveRecord def find_merge_request_with_access(iid, access_level = :read_merge_request) merge_request = user_project.merge_requests.find_by!(iid: iid) diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index a068de4361c..c444766249d 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -73,16 +73,27 @@ module API "read_#{noteable.class.to_s.underscore}".to_sym end - def find_noteable(parent, noteables_str, noteable_id) - noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend + def find_noteable(parent_type, parent_id, noteable_type, noteable_id) + params = params_by_noteable_type_and_id(noteable_type, noteable_id) - readable = can?(current_user, noteable_read_ability_name(noteable), noteable) - - return not_found!(noteables_str) unless readable + noteable = NotesFinder.new(user_project, current_user, params).target + noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable) + return not_found!(noteable_type) unless noteable noteable end + def params_by_noteable_type_and_id(type, id) + target_type = type.name.underscore + { target_type: target_type }.tap do |h| + if %w(issue merge_request).include?(target_type) + h[:target_iid] = id + else + h[:target_id] = id + end + end + end + def noteable_parent(noteable) public_send("user_#{noteable.class.parent_class.to_s.underscore}") # rubocop:disable GitlabSecurity/PublicSend end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 416cf39d3ec..9381f045144 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -15,8 +15,6 @@ module API requires :id, type: String, desc: "The ID of a #{parent_type}" end resource parent_type.pluralize.to_sym, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - noteables_str = noteable_type.to_s.underscore.pluralize - desc "Get a list of #{noteable_type.to_s.downcase} notes" do success Entities::Note end @@ -30,7 +28,7 @@ module API end # rubocop: disable CodeReuse/ActiveRecord get ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) # We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not @@ -56,7 +54,7 @@ module API requires :noteable_id, type: Integer, desc: 'The ID of the noteable' end get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) get_note(noteable, params[:note_id]) end @@ -69,7 +67,7 @@ module API optional :created_at, type: String, desc: 'The creation date of the note' end post ":id/#{noteables_str}/:noteable_id/notes" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) opts = { note: params[:body], @@ -96,7 +94,7 @@ module API requires :body, type: String, desc: 'The content of a note' end put ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) update_note(noteable, params[:note_id]) end @@ -109,7 +107,7 @@ module API requires :note_id, type: Integer, desc: 'The ID of a note' end delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do - noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) + noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id]) delete_note(noteable, params[:note_id]) end diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb index 448bef12cec..505a6c68c9c 100644 --- a/lib/api/resource_label_events.rb +++ b/lib/api/resource_label_events.rb @@ -26,7 +26,7 @@ module API # rubocop: disable CodeReuse/ActiveRecord get ":id/#{eventables_str}/:eventable_id/resource_label_events" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) events = eventable.resource_label_events.includes(:label, :user) present paginate(events), with: Entities::ResourceLabelEvent @@ -42,7 +42,7 @@ module API requires :eventable_id, types: [Integer, String], desc: 'The ID of the eventable' end get ":id/#{eventables_str}/:eventable_id/resource_label_events/:event_id" do - eventable = find_noteable(parent_type, eventables_str, params[:eventable_id]) + eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id]) event = eventable.resource_label_events.find(params[:event_id]) present event, with: Entities::ResourceLabelEvent diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 0a685152cf9..87bde4ca2f6 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -292,5 +292,31 @@ describe NotesFinder do expect(subject.target).to eq(commit) end end + + context 'target_iid' do + let(:issue) { create(:issue, project: project) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + it 'finds issues by iid' do + iid_params = { target_type: 'issue', target_iid: issue.iid } + expect(described_class.new(project, user, iid_params).target).to eq(issue) + end + + it 'finds merge requests by iid' do + iid_params = { target_type: 'merge_request', target_iid: merge_request.iid } + expect(described_class.new(project, user, iid_params).target).to eq(merge_request) + end + + it 'returns nil if both target_id and target_iid are not given' do + params_without_any_id = { target_type: 'issue' } + expect(described_class.new(project, user, params_without_any_id).target).to be_nil + end + + it 'prioritizes target_id over target_iid' do + issue2 = create(:issue, project: project) + iid_params = { target_type: 'issue', target_id: issue2.id, target_iid: issue.iid } + expect(described_class.new(project, user, iid_params).target).to eq(issue2) + end + end end end