Merge branch 'pderichs-52123' into 'master'

Use NotesFinder to fetch notes on API and Controllers

See merge request gitlab-org/gitlab-ee!13265

app/finders/notes_finder.rb

@@ -34,8 +34,10 @@ 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
+    return @target = nil unless target_id || target_iid
 
     @target =
       if target_type == "commit"
@@ -43,12 +45,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

ee/app/finders/epics_finder.rb

@@ -43,7 +43,10 @@ class EpicsFinder < IssuableFinder
     return @group if defined?(@group)
 
     group = Group.find(params[:group_id])
-    group = nil unless Ability.allowed?(current_user, :read_epic, group)
+
+    unless Ability.allowed?(current_user, :read_epic, group)
+      raise ActiveRecord::RecordNotFound.new("Could not find a Group with ID #{params[:group_id]}")
+    end
 
     @group = group
   end

ee/lib/ee/api/helpers/notes_helpers.rb

@@ -15,15 +15,12 @@ module EE
           end
         end
 
-        def find_group_epic(id)
-          finder_params = { group_id: user_group.id }
-          EpicsFinder.new(current_user, finder_params).find(id)
-        end
-
-        def noteable_parent_str(noteable_class)
-          parent_class = ::Epic <= noteable_class ? ::Group : ::Project
-
-          parent_class.to_s.underscore
+        def find_noteable(parent_type, parent_id, noteable_type, noteable_id)
+          if noteable_type == ::Epic
+            EpicsFinder.new(current_user, group_id: parent_id).find(noteable_id)
+          else
+            super
+          end
         end
       end
     end

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

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)

lib/api/helpers/notes_helpers.rb

@@ -73,14 +73,23 @@ 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)
+        noteable || not_found!(noteable_type)
+      end
 
-        noteable
+      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)

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

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

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