Merge branch 'tc-api-remove-comments-endpoint' into 'master'

API: Make the /notes endpoint work with noteable iid instead of id

Closes #29871

See merge request !10258

changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml

+---
+title: 'API: Make the /notes endpoint work with noteable iid instead of id'
+merge_request:
+author:

doc/api/notes.md

@@ -9,13 +9,13 @@ Notes are comments on snippets, issues or merge requests.
 Gets a list of all notes for a single issue.
 
 ```
-GET /projects/:id/issues/:issue_id/notes
+GET /projects/:id/issues/:issue_iid/notes
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `issue_id` (required) - The ID of an issue
+- `issue_iid` (required) - The IID of an issue
 
 ```json
 [
@@ -63,13 +63,13 @@ Parameters:
 Returns a single note for a specific project issue
 
 ```
-GET /projects/:id/issues/:issue_id/notes/:note_id
+GET /projects/:id/issues/:issue_iid/notes/:note_id
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `issue_id` (required) - The ID of a project issue
+- `issue_iid` (required) - The IID of a project issue
 - `note_id` (required) - The ID of an issue note
 
 ### Create new issue note
@@ -78,13 +78,13 @@ Creates a new note to a single project issue. If you create a note where the bod
 only contains an Award Emoji, you'll receive this object back.
 
 ```
-POST /projects/:id/issues/:issue_id/notes
+POST /projects/:id/issues/:issue_iid/notes
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `issue_id` (required) - The ID of an issue
+- `issue_id` (required) - The IID of an issue
 - `body` (required) - The content of a note
 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
 
@@ -93,13 +93,13 @@ Parameters:
 Modify existing note of an issue.
 
 ```
-PUT /projects/:id/issues/:issue_id/notes/:note_id
+PUT /projects/:id/issues/:issue_iid/notes/:note_id
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `issue_id` (required) - The ID of an issue
+- `issue_iid` (required) - The IID of an issue
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
@@ -108,7 +108,7 @@ Parameters:
 Deletes an existing note of an issue.
 
 ```
-DELETE /projects/:id/issues/:issue_id/notes/:note_id
+DELETE /projects/:id/issues/:issue_iid/notes/:note_id
 ```
 
 Parameters:
@@ -116,7 +116,7 @@ Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id` | integer | yes | The ID of a project |
-| `issue_id` | integer | yes | The ID of an issue |
+| `issue_iid` | integer | yes | The IID of an issue |
 | `note_id` | integer | yes | The ID of a note |
 
 ```bash
@@ -228,26 +228,26 @@ curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://git
 Gets a list of all notes for a single merge request.
 
 ```
-GET /projects/:id/merge_requests/:merge_request_id/notes
+GET /projects/:id/merge_requests/:merge_request_iid/notes
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `merge_request_id` (required) - The ID of a project merge request
+- `merge_request_iid` (required) - The IID of a project merge request
 
 ### Get single merge request note
 
 Returns a single note for a given merge request.
 
 ```
-GET /projects/:id/merge_requests/:merge_request_id/notes/:note_id
+GET /projects/:id/merge_requests/:merge_request_iid/notes/:note_id
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `merge_request_id` (required) - The ID of a project merge request
+- `merge_request_iid` (required) - The IID of a project merge request
 - `note_id` (required) - The ID of a merge request note
 
 ```json
@@ -278,13 +278,13 @@ If you create a note where the body only contains an Award Emoji, you'll receive
 this object back.
 
 ```
-POST /projects/:id/merge_requests/:merge_request_id/notes
+POST /projects/:id/merge_requests/:merge_request_iid/notes
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `merge_request_id` (required) - The ID of a merge request
+- `merge_request_iid` (required) - The IID of a merge request
 - `body` (required) - The content of a note
 
 ### Modify existing merge request note
@@ -292,13 +292,13 @@ Parameters:
 Modify existing note of a merge request.
 
 ```
-PUT /projects/:id/merge_requests/:merge_request_id/notes/:note_id
+PUT /projects/:id/merge_requests/:merge_request_iid/notes/:note_id
 ```
 
 Parameters:
 
 - `id` (required) - The ID of a project
-- `merge_request_id` (required) - The ID of a merge request
+- `merge_request_iid` (required) - The IID of a merge request
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
@@ -307,7 +307,7 @@ Parameters:
 Deletes an existing note of a merge request.
 
 ```
-DELETE /projects/:id/merge_requests/:merge_request_id/notes/:note_id
+DELETE /projects/:id/merge_requests/:merge_request_iid/notes/:note_id
 ```
 
 Parameters:
@@ -315,7 +315,7 @@ Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id` | integer | yes | The ID of a project |
-| `merge_request_id` | integer | yes | The ID of a merge request |
+| `merge_request_iid` | integer | yes | The IID of a merge request |
 | `note_id` | integer | yes | The ID of a note |
 
 ```bash

lib/api/helpers.rb

@@ -90,6 +90,11 @@ module API
       MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
     end
 
+    def find_project_snippet(id)
+      finder_params = { filter: :by_project, project: user_project }
+      SnippetsFinder.new.execute(current_user, finder_params).find(id)
+    end
+
     def find_merge_request_with_access(iid, access_level = :read_merge_request)
       merge_request = user_project.merge_requests.find_by!(iid: iid)
       authorize! access_level, merge_request

lib/api/merge_requests.rb

@@ -226,41 +226,6 @@ module API
           .cancel(merge_request)
       end
 
-      desc 'Get the comments of a merge request' do
-        success Entities::MRNote
-      end
-      params do
-        use :pagination
-      end
-      get ':id/merge_requests/:merge_request_iid/comments' do
-        merge_request = find_merge_request_with_access(params[:merge_request_iid])
-        present paginate(merge_request.notes.fresh), with: Entities::MRNote
-      end
-
-      desc 'Post a comment to a merge request' do
-        success Entities::MRNote
-      end
-      params do
-        requires :note, type: String, desc: 'The text of the comment'
-      end
-      post ':id/merge_requests/:merge_request_iid/comments' do
-        merge_request = find_merge_request_with_access(params[:merge_request_iid], :create_note)
-
-        opts = {
-          note: params[:note],
-          noteable_type: 'MergeRequest',
-          noteable_id: merge_request.id
-        }
-
-        note = ::Notes::CreateService.new(user_project, current_user, opts).execute
-
-        if note.save
-          present note, with: Entities::MRNote
-        else
-          render_api_error!("Failed to save note #{note.errors.messages}", 400)
-        end
-      end
-
       desc 'List issues that will be closed on merge' do
         success Entities::MRNote
       end

lib/api/notes.rb

@@ -21,7 +21,7 @@ module API
           use :pagination
         end
         get ":id/#{noteables_str}/:noteable_id/notes" do
-          noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
+          noteable = find_project_noteable(noteables_str, params[:noteable_id])
 
           if can?(current_user, noteable_read_ability_name(noteable), noteable)
             # We exclude notes that are cross-references and that cannot be viewed
@@ -49,7 +49,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 = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
+          noteable = find_project_noteable(noteables_str, params[:noteable_id])
           note = noteable.notes.find(params[:note_id])
           can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
 
@@ -69,14 +69,14 @@ 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_project_noteable(noteables_str, params[:noteable_id])
+
           opts = {
             note: params[:body],
             noteable_type: noteables_str.classify,
-            noteable_id: params[:noteable_id]
+            noteable_id: noteable.id
           }
 
-          noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
-
           if can?(current_user, noteable_read_ability_name(noteable), noteable)
             if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
               opts[:created_at] = params[:created_at]
@@ -137,6 +137,10 @@ module API
     end
 
     helpers do
+      def find_project_noteable(noteables_str, noteable_id)
+        public_send("find_project_#{noteables_str.singularize}", noteable_id)
+      end
+
       def noteable_read_ability_name(noteable)
         "read_#{noteable.class.to_s.underscore}".to_sym
       end

spec/requests/api/merge_requests_spec.rb

@@ -623,64 +623,6 @@ describe API::MergeRequests, api: true  do
     end
   end
 
-  describe "POST /projects/:id/merge_requests/:merge_request_iid/comments" do
-    it "returns comment" do
-      original_count = merge_request.notes.size
-
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/comments", user), note: "My comment"
-
-      expect(response).to have_http_status(201)
-      expect(json_response['note']).to eq('My comment')
-      expect(json_response['author']['name']).to eq(user.name)
-      expect(json_response['author']['username']).to eq(user.username)
-      expect(merge_request.reload.notes.size).to eq(original_count + 1)
-    end
-
-    it "returns 400 if note is missing" do
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/comments", user)
-      expect(response).to have_http_status(400)
-    end
-
-    it "returns 404 if merge request iid is invalid" do
-      post api("/projects/#{project.id}/merge_requests/404/comments", user),
-        note: 'My comment'
-      expect(response).to have_http_status(404)
-    end
-
-    it "returns 404 if merge request id is used instead of iid" do
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/comments", user),
-        note: 'My comment'
-      expect(response).to have_http_status(404)
-    end
-  end
-
-  describe "GET :id/merge_requests/:merge_request_iid/comments" do
-    let!(:note)  { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
-    let!(:note2) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "another comment on a MR") }
-
-    it "returns merge_request comments ordered by created_at" do
-      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/comments", user)
-
-      expect(response).to have_http_status(200)
-      expect(response).to include_pagination_headers
-      expect(json_response).to be_an Array
-      expect(json_response.length).to eq(2)
-      expect(json_response.first['note']).to eq("a comment on a MR")
-      expect(json_response.first['author']['id']).to eq(user.id)
-      expect(json_response.last['note']).to eq("another comment on a MR")
-    end
-
-    it "returns a 404 error if merge_request_iid is invalid" do
-      get api("/projects/#{project.id}/merge_requests/999/comments", user)
-      expect(response).to have_http_status(404)
-    end
-
-    it "returns a 404 error if merge_request id is used instead of iid" do
-      get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/comments", user)
-      expect(response).to have_http_status(404)
-    end
-  end
-
   describe 'GET :id/merge_requests/:merge_request_iid/closes_issues' do
     it 'returns the issue that will be closed on merge' do
       issue = create(:issue, project: project)