Merge branch 'issue_195791' into 'master'

Accept group path as ID when fetching notes from API

See merge request gitlab-org/gitlab!23535

ee/changelogs/unreleased/issue_195791.yml

+---
+title: Accept group path as ID when fetching notes from API
+merge_request: 23535
+author:
+type: fixed

ee/lib/api/visual_review_discussions.rb

@@ -36,7 +36,7 @@ module API
           forbidden!('Anonymous visual review feedback is disabled')
         end
 
-        merge_request = find_merge_request_without_permissions_check(params[:id], params[:merge_request_iid])
+        merge_request = find_merge_request_without_permissions_check(params[:merge_request_iid])
 
         note = create_visual_review_note(merge_request, {
           note: params[:body],

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

@@ -15,17 +15,17 @@ module EE
           end
         end
 
-        def add_parent_to_finder_params(finder_params, noteable_type, parent_id)
+        def add_parent_to_finder_params(finder_params, noteable_type)
           if noteable_type.name.underscore == 'epic'
-            finder_params[:group_id] = parent_id
+            finder_params[:group_id] = user_group.id
           else
             super
           end
         end
 
         # Used only for anonymous Visual Review Tools feedback
-        def find_merge_request_without_permissions_check(parent_id, noteable_id)
-          params = finder_params_by_noteable_type_and_id(::MergeRequest, noteable_id, parent_id)
+        def find_merge_request_without_permissions_check(noteable_id)
+          params = finder_params_by_noteable_type_and_id(::MergeRequest, noteable_id)
 
           ::NotesFinder.new(current_user, params).target || not_found!(noteable_type)
         end

ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb

@@ -36,12 +36,18 @@ describe 'NotesHelpers' do
       stub_licensed_features(epics: true)
     end
 
-    it 'returns the expected epic' do
-      expect(subject.find_noteable(Group, parent_id, noteable_type, epic.id)).to eq(epic)
-    end
+    context '#find_noteable' do
+      it 'returns the expected epic' do
+        allow(subject).to receive(:user_group).and_return(group)
+
+        expect(subject.find_noteable(noteable_type, epic.id)).to eq(epic)
+      end
 
-    it 'raises not found exception when epic does not belong to group' do
-      expect { subject.find_noteable(Group, other_group.id, noteable_type, epic.id) }.to raise_error(ActiveRecord::RecordNotFound)
+      it 'raises not found exception when epic does not belong to group' do
+        allow(subject).to receive(:user_group).and_return(other_group)
+
+        expect { subject.find_noteable(noteable_type, epic.id) }.to raise_error(ActiveRecord::RecordNotFound)
+      end
     end
   end
 end

lib/api/discussions.rb

@@ -26,7 +26,7 @@ module API
         end
 
         get ":id/#{noteables_path}/:noteable_id/discussions" do
-          noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           notes = readable_discussion_notes(noteable)
           discussions = Kaminari.paginate_array(Discussion.build_collection(notes, noteable))
@@ -42,7 +42,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
           if notes.empty?
@@ -77,7 +77,7 @@ module API
           end
         end
         post ":id/#{noteables_path}/:noteable_id/discussions" do
-          noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
           type = params[:position] ? 'DiffNote' : 'DiscussionNote'
           id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
 
@@ -107,7 +107,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
           if notes.empty?
@@ -127,7 +127,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
           first_note = notes.first
 
@@ -161,7 +161,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           get_note(noteable, params[:note_id])
         end
@@ -178,7 +178,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           if params[:resolved].nil?
             update_note(noteable, params[:note_id])
@@ -196,7 +196,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           delete_note(noteable, params[:note_id])
         end
@@ -211,7 +211,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, params[:id], noteable_type, params[:noteable_id])
+            noteable = find_noteable(noteable_type, params[:noteable_id])
 
             resolve_discussion(noteable, params[:discussion_id], params[:resolved])
           end

lib/api/helpers/notes_helpers.rb

@@ -83,15 +83,15 @@ module API
         end
       end
 
-      def find_noteable(parent_type, parent_id, noteable_type, noteable_id)
-        params = finder_params_by_noteable_type_and_id(noteable_type, noteable_id, parent_id)
+      def find_noteable(noteable_type, noteable_id)
+        params = finder_params_by_noteable_type_and_id(noteable_type, noteable_id)
 
         noteable = NotesFinder.new(current_user, params).target
         noteable = nil unless can?(current_user, noteable_read_ability_name(noteable), noteable)
         noteable || not_found!(noteable_type)
       end
 
-      def finder_params_by_noteable_type_and_id(type, id, parent_id)
+      def finder_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)
@@ -100,11 +100,11 @@ module API
             h[:target_id] = id
           end
 
-          add_parent_to_finder_params(h, type, parent_id)
+          add_parent_to_finder_params(h, type)
         end
       end
 
-      def add_parent_to_finder_params(finder_params, noteable_type, parent_id)
+      def add_parent_to_finder_params(finder_params, noteable_type)
         finder_params[:project] = user_project
       end
 

lib/api/notes.rb

@@ -30,7 +30,7 @@ module API
         end
         # rubocop: disable CodeReuse/ActiveRecord
         get ":id/#{noteables_str}/:noteable_id/notes" do
-          noteable = find_noteable(parent_type, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(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
@@ -58,7 +58,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
           get_note(noteable, params[:note_id])
         end
 
@@ -71,7 +71,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           opts = {
             note: params[:body],
@@ -98,7 +98,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           update_note(noteable, params[:note_id])
         end
@@ -111,7 +111,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, params[:id], noteable_type, params[:noteable_id])
+          noteable = find_noteable(noteable_type, params[:noteable_id])
 
           delete_note(noteable, params[:note_id])
         end

lib/api/resource_label_events.rb

@@ -25,7 +25,7 @@ module API
         end
 
         get ":id/#{eventables_str}/:eventable_id/resource_label_events" do
-          eventable = find_noteable(parent_type, params[:id], eventable_type, params[:eventable_id])
+          eventable = find_noteable(eventable_type, params[:eventable_id])
 
           opts = { page: params[:page], per_page: params[:per_page] }
           events = ResourceLabelEventFinder.new(current_user, eventable, opts).execute
@@ -42,7 +42,8 @@ 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, params[:id], eventable_type, params[:eventable_id])
+          eventable = find_noteable(eventable_type, params[:eventable_id])
+
           event = eventable.resource_label_events.find(params[:event_id])
 
           not_found!('ResourceLabelEvent') unless can?(current_user, :read_resource_label_event, event)

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -20,6 +20,14 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
           expect(response_dates).to eq(response_dates.sort.reverse)
         end
 
+        it 'fetches notes using parent path as id paremeter' do
+          parent_id = CGI.escape(parent.full_path)
+
+          get api("/#{parent_type}/#{parent_id}/#{noteable_type}/#{noteable[id_name]}/notes", user)
+
+          expect(response.status).to eq(200)
+        end
+
         context '2 notes with equal created_at' do
           before do
             @first_note = Note.first