Use NotesFinder to fetch notes on API and Controllers

Add project override for epics

Reverted previous changes

Bring back parent type


Bring back parent_type and added parent_id

Fix style error

parent_id must be an Integer

Remove to_i here

Fix call to find_noteable

Remove commented out method

Reduce code duplication by extracting
a noteable module method

The call to find_noteable was redundant so
multiple files and lines have changed in that
commit to use the newly introduced module
method `noteable`.

Replace casecmp with include check

Add parent_type parameter

Revert "Reduce code duplication by extracting
a noteable module method"

This reverts commit 8369ca8949.

Method is no longer needed

Method is no longer needed

NotesFinder is able to handle Epics

Revert "NotesFinder is able to handle Epics"

This reverts commit 097062c6933b2e26176d5ac5c0393b753f52984c.

Check whether noteable can be read by user

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

ee/app/finders/epics_finder.rb

@@ -43,7 +43,11 @@ 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)
+        def find_noteable(parent_type, parent_id, noteable_type, noteable_id)
+          if noteable_type.to_s.underscore == 'epic' && parent_type == 'group'
+            return EpicsFinder.new(current_user, group_id: parent_id.to_i).find(noteable_id)
           end
 
-        def noteable_parent_str(noteable_class)
-          parent_class = ::Epic <= noteable_class ? ::Group : ::Project
-
-          parent_class.to_s.underscore
+          super
         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,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

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