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

In API V4 all endpoints were changed so Merge Requests and Issues
should be referred by iid, instead of id. Except the /notes endpoint
was forgotten. So change the endpoints from:

- /projects/:id/issues/:issue_id/notes
- /projects/:id/merge_requests/:merge_request_id/notes

To:

- /projects/:id/issues/:issue_iid/notes
- /projects/:id/merge_requests/:merge_request_iid/notes

For Project Snippets nothing changes.

5 files changed, 68 insertions, 55 deletions

diff --git a/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml b/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml
new file mode 100644
index 00000000000..e3fb62d53b6
--- /dev/null
+++ b/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml
@@ -0,0 +1,4 @@
+---
+title: 'API: Make the /notes endpoint work with noteable iid instead of id'
+merge_request:
+author:
diff --git a/doc/api/notes.md b/doc/api/notes.md
index 6ef06b2c2e9..5e927143714 100644
--- a/doc/api/notes.md
+++ b/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
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index bd22b82476b..61527c1e20b 100644
--- a/lib/api/helpers.rb
+++ b/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
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 29ceffdbd2d..de39e579ac3 100644
--- a/lib/api/notes.rb
+++ b/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
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index 347f8f6fa3b..d8eb8ce921e 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -34,7 +34,7 @@ describe API::Notes, api: true  do
   describe "GET /projects/:id/noteable/:noteable_id/notes" do
     context "when noteable is an Issue" do
       it "returns an array of issue notes" do
-        get api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
+        get api("/projects/#{project.id}/issues/#{issue.iid}/notes", user)
 
         expect(response).to have_http_status(200)
         expect(response).to include_pagination_headers
@@ -50,7 +50,7 @@ describe API::Notes, api: true  do
 
       context "and current user cannot view the notes" do
         it "returns an empty array" do
-          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
+          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", user)
 
           expect(response).to have_http_status(200)
           expect(response).to include_pagination_headers
@@ -62,7 +62,7 @@ describe API::Notes, api: true  do
           before { ext_issue.update_attributes(confidential: true) }
 
           it "returns 404" do
-            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
+            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", user)
 
             expect(response).to have_http_status(404)
           end
@@ -70,7 +70,7 @@ describe API::Notes, api: true  do
 
         context "and current user can view the note" do
           it "returns an empty array" do
-            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user)
+            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", private_user)
 
             expect(response).to have_http_status(200)
             expect(response).to include_pagination_headers
@@ -106,7 +106,7 @@ describe API::Notes, api: true  do
 
     context "when noteable is a Merge Request" do
       it "returns an array of merge_requests notes" do
-        get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user)
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user)
 
         expect(response).to have_http_status(200)
         expect(response).to include_pagination_headers
@@ -131,21 +131,21 @@ describe API::Notes, api: true  do
   describe "GET /projects/:id/noteable/:noteable_id/notes/:note_id" do
     context "when noteable is an Issue" do
       it "returns an issue note by id" do
-        get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user)
+        get api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", user)
 
         expect(response).to have_http_status(200)
         expect(json_response['body']).to eq(issue_note.note)
       end
 
       it "returns a 404 error if issue note not found" do
-        get api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
+        get api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user)
 
         expect(response).to have_http_status(404)
       end
 
       context "and current user cannot view the note" do
         it "returns a 404 error" do
-          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user)
+          get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes/#{cross_reference_note.id}", user)
 
           expect(response).to have_http_status(404)
         end
@@ -154,7 +154,7 @@ describe API::Notes, api: true  do
           before { issue.update_attributes(confidential: true) }
 
           it "returns 404" do
-            get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", private_user)
+            get api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", private_user)
 
             expect(response).to have_http_status(404)
           end
@@ -162,7 +162,7 @@ describe API::Notes, api: true  do
 
         context "and current user can view the note" do
           it "returns an issue note by id" do
-            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user)
+            get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes/#{cross_reference_note.id}", private_user)
 
             expect(response).to have_http_status(200)
             expect(json_response['body']).to eq(cross_reference_note.note)
@@ -190,7 +190,7 @@ describe API::Notes, api: true  do
   describe "POST /projects/:id/noteable/:noteable_id/notes" do
     context "when noteable is an Issue" do
       it "creates a new issue note" do
-        post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
+        post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!'
 
         expect(response).to have_http_status(201)
         expect(json_response['body']).to eq('hi!')
@@ -198,13 +198,13 @@ describe API::Notes, api: true  do
       end
 
       it "returns a 400 bad request error if body not given" do
-        post api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
+        post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user)
 
         expect(response).to have_http_status(400)
       end
 
       it "returns a 401 unauthorized error if user not authenticated" do
-        post api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!'
+        post api("/projects/#{project.id}/issues/#{issue.iid}/notes"), body: 'hi!'
 
         expect(response).to have_http_status(401)
       end
@@ -212,7 +212,7 @@ describe API::Notes, api: true  do
       context 'when an admin or owner makes the request' do
         it 'accepts the creation date to be set' do
           creation_time = 2.weeks.ago
-          post api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
+          post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user),
             body: 'hi!', created_at: creation_time
 
           expect(response).to have_http_status(201)
@@ -226,7 +226,7 @@ describe API::Notes, api: true  do
         let(:issue2) { create(:issue, project: project) }
 
         it 'creates a new issue note' do
-          post api("/projects/#{project.id}/issues/#{issue2.id}/notes", user), body: ':+1:'
+          post api("/projects/#{project.id}/issues/#{issue2.iid}/notes", user), body: ':+1:'
 
           expect(response).to have_http_status(201)
           expect(json_response['body']).to eq(':+1:')
@@ -235,7 +235,7 @@ describe API::Notes, api: true  do
 
       context 'when the user is posting an award emoji on his/her own issue' do
         it 'creates a new issue note' do
-          post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: ':+1:'
+          post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: ':+1:'
 
           expect(response).to have_http_status(201)
           expect(json_response['body']).to eq(':+1:')
@@ -270,7 +270,7 @@ describe API::Notes, api: true  do
         project = create(:empty_project, :private) { |p| p.add_guest(user) }
         issue = create(:issue, :confidential, project: project)
 
-        post api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
+        post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user),
           body: 'Foo'
 
         expect(response).to have_http_status(404)
@@ -285,7 +285,7 @@ describe API::Notes, api: true  do
       # from a different project, see #15577
       #
       before do
-        post api("/projects/#{project.id}/issues/#{private_issue.id}/notes", user),
+        post api("/projects/#{private_issue.project.id}/issues/#{private_issue.iid}/notes", user),
              body: 'Hi!'
       end
 
@@ -303,14 +303,14 @@ describe API::Notes, api: true  do
     it "creates an activity event when an issue note is created" do
       expect(Event).to receive(:create)
 
-      post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
+      post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!'
     end
   end
 
   describe 'PUT /projects/:id/noteable/:noteable_id/notes/:note_id' do
     context 'when noteable is an Issue' do
       it 'returns modified note' do
-        put api("/projects/#{project.id}/issues/#{issue.id}/"\
+        put api("/projects/#{project.id}/issues/#{issue.iid}/"\
                   "notes/#{issue_note.id}", user), body: 'Hello!'
 
         expect(response).to have_http_status(200)
@@ -318,14 +318,14 @@ describe API::Notes, api: true  do
       end
 
       it 'returns a 404 error when note id not found' do
-        put api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user),
+        put api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user),
                 body: 'Hello!'
 
         expect(response).to have_http_status(404)
       end
 
       it 'returns a 400 bad request error if body not given' do
-        put api("/projects/#{project.id}/issues/#{issue.id}/"\
+        put api("/projects/#{project.id}/issues/#{issue.iid}/"\
                   "notes/#{issue_note.id}", user)
 
         expect(response).to have_http_status(400)
@@ -351,7 +351,7 @@ describe API::Notes, api: true  do
 
     context 'when noteable is a Merge Request' do
       it 'returns modified note' do
-        put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/"\
                   "notes/#{merge_request_note.id}", user), body: 'Hello!'
 
         expect(response).to have_http_status(200)
@@ -359,7 +359,7 @@ describe API::Notes, api: true  do
       end
 
       it 'returns a 404 error when note id not found' do
-        put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\
+        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/"\
                   "notes/12345", user), body: "Hello!"
 
         expect(response).to have_http_status(404)
@@ -370,18 +370,18 @@ describe API::Notes, api: true  do
   describe 'DELETE /projects/:id/noteable/:noteable_id/notes/:note_id' do
     context 'when noteable is an Issue' do
       it 'deletes a note' do
-        delete api("/projects/#{project.id}/issues/#{issue.id}/"\
+        delete api("/projects/#{project.id}/issues/#{issue.iid}/"\
                    "notes/#{issue_note.id}", user)
 
         expect(response).to have_http_status(204)
         # Check if note is really deleted
-        delete api("/projects/#{project.id}/issues/#{issue.id}/"\
+        delete api("/projects/#{project.id}/issues/#{issue.iid}/"\
                    "notes/#{issue_note.id}", user)
         expect(response).to have_http_status(404)
       end
 
       it 'returns a 404 error when note id not found' do
-        delete api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
+        delete api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user)
 
         expect(response).to have_http_status(404)
       end
@@ -410,18 +410,18 @@ describe API::Notes, api: true  do
     context 'when noteable is a Merge Request' do
       it 'deletes a note' do
         delete api("/projects/#{project.id}/merge_requests/"\
-                   "#{merge_request.id}/notes/#{merge_request_note.id}", user)
+                   "#{merge_request.iid}/notes/#{merge_request_note.id}", user)
 
         expect(response).to have_http_status(204)
         # Check if note is really deleted
         delete api("/projects/#{project.id}/merge_requests/"\
-                   "#{merge_request.id}/notes/#{merge_request_note.id}", user)
+                   "#{merge_request.iid}/notes/#{merge_request_note.id}", user)
         expect(response).to have_http_status(404)
       end
 
       it 'returns a 404 error when note id not found' do
         delete api("/projects/#{project.id}/merge_requests/"\
-                   "#{merge_request.id}/notes/12345", user)
+                   "#{merge_request.iid}/notes/12345", user)
 
         expect(response).to have_http_status(404)
       end