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

Port of "API: Make the /notes endpoint work with noteable iid instead of id" to EE

Closes #1875

See merge request !1518

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/merge_requests.md

@@ -546,15 +546,15 @@ You can request information about a merge request's approval status using the
 following endpoint:
 
 ```
-GET /projects/:id/merge_requests/:merge_request_id/approvals
+GET /projects/:id/merge_requests/:merge_request_iid/approvals
 ```
 
 **Parameters:**
 
-| Attribute          | Type    | Required | Description         |
-|--------------------|---------|----------|---------------------|
-| `id`               | integer | yes      | The ID of a project |
-| `merge_request_id` | integer | yes      | The ID of MR        |
+| Attribute           | Type    | Required | Description         |
+|---------------------|---------|----------|---------------------|
+| `id`                | integer | yes      | The ID of a project |
+| `merge_request_iid` | integer | yes      | The IID of MR       |
 
 ```json
 {
@@ -592,15 +592,15 @@ If you are allowed to, you can approve a merge request using the following
 endpoint:
 
 ```
-POST /projects/:id/merge_requests/:merge_request_id/approve
+POST /projects/:id/merge_requests/:merge_request_iid/approve
 ```
 
 **Parameters:**
 
-| Attribute          | Type    | Required | Description         |
-|--------------------|---------|----------|---------------------|
-| `id`               | integer | yes      | The ID of a project |
-| `merge_request_id` | integer | yes      | The ID of MR        |
+| Attribute           | Type    | Required | Description         |
+|---------------------|---------|----------|---------------------|
+| `id`                | integer | yes      | The ID of a project |
+| `merge_request_iid` | integer | yes      | The IID of MR       |
 
 ```json
 {
@@ -640,6 +640,24 @@ POST /projects/:id/merge_requests/:merge_request_id/approve
 }
 ```
 
+## Unapprove Merge Request
+
+>**Note:** This API endpoint is only available on 9.0 EE and above.
+
+If you did approve a merge request, you can unapprove it using the following
+endpoint:
+
+```
+POST /projects/:id/merge_requests/:merge_request_iid/unapprove
+```
+
+**Parameters:**
+
+| Attribute           | Type    | Required | Description         |
+|---------------------|---------|----------|---------------------|
+| `id`                | integer | yes      | The ID of a project |
+| `merge_request_iid` | integer | yes      | The IID of MR       |
+
 ## Cancel Merge When Pipeline Succeeds
 
 If you don't have permissions to accept this merge request - you'll get a `401`

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

doc/api/v3_to_v4.md

@@ -88,3 +88,4 @@ Below are the changes made between V3 and V4.
 - Remove the ProjectGitHook API. Use the ProjectPushRule API instead [!1301](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1301)
 - Removed `repository_storage` from `PUT /application/settings` and `GET /application/settings` (use `repository_storages` instead) [!1307](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1307)
 - Removed `elasticsearch_host` and `elasticsearch_port` from `PUT /application/settings` (use `elasticsearch_url` instead) [!1305](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1305)
+- Make approval API more RESTful. Use `POST /projects/:id/merge_requests/:merge_request_iid/unapprove` to unapprove a merge request. [!1518](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1518)

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

@@ -233,41 +233,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
@@ -284,14 +249,13 @@ module API
       #
       # Parameters:
       #   id (required)                 - The ID of a project
-      #   merge_request_id (required)   - ID of MR
+      #   merge_request_idd (required)  - IID of MR
       # Examples:
-      #   GET /projects/:id/merge_requests/:merge_request_id/approvals
+      #   GET /projects/:id/merge_requests/:merge_request_iid/approvals
       #
-      get ':id/merge_requests/:merge_request_id/approvals' do
-        merge_request = user_project.merge_requests.find(params[:merge_request_id])
+      get ':id/merge_requests/:merge_request_iid/approvals' do
+        merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
-        authorize! :read_merge_request, merge_request
         present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
       end
 
@@ -299,12 +263,12 @@ module API
       #
       # Parameters:
       #   id (required)                 - The ID of a project
-      #   merge_request_id (required)   - ID of MR
+      #   merge_request_iid (required)  - IID of MR
       # Examples:
-      #   POST /projects/:id/merge_requests/:merge_request_id/approvals
+      #   POST /projects/:id/merge_requests/:merge_request_iid/approve
       #
-      post ':id/merge_requests/:merge_request_id/approve' do
-        merge_request = user_project.merge_requests.find(params[:merge_request_id])
+      post ':id/merge_requests/:merge_request_iid/approve' do
+        merge_request = find_project_merge_request(params[:merge_request_iid])
 
         unauthorized! unless merge_request.can_approve?(current_user)
 
@@ -315,8 +279,8 @@ module API
         present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
       end
 
-      delete ':id/merge_requests/:merge_request_id/unapprove' do
-        merge_request = user_project.merge_requests.find(params[:merge_request_id])
+      post ':id/merge_requests/:merge_request_iid/unapprove' do
+        merge_request = find_project_merge_request(params[:merge_request_iid])
 
         not_found! unless merge_request.has_approved?(current_user)
 
@@ -333,8 +297,8 @@ module API
       params do
         use :pagination
       end
-      get ':id/merge_requests/:merge_request_id/closes_issues' do
-        merge_request = find_merge_request_with_access(params[:merge_request_id])
+      get ':id/merge_requests/:merge_request_iid/closes_issues' do
+        merge_request = find_merge_request_with_access(params[:merge_request_iid])
         issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
         present paginate(issues), with: issue_entity(user_project), current_user: current_user
       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

@@ -701,64 +701,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)
@@ -896,7 +838,7 @@ describe API::MergeRequests, api: true  do
     end
   end
 
-  describe 'GET :id/merge_requests/:merge_request_id/approvals' do
+  describe 'GET :id/merge_requests/:merge_request_iid/approvals' do
     it 'retrieves the approval status' do
       approver = create :user
       project.update_attribute(:approvals_before_merge, 2)
@@ -904,7 +846,7 @@ describe API::MergeRequests, api: true  do
       project.team << [create(:user), :developer]
       merge_request.approvals.create(user: approver)
 
-      get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approvals", user)
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
 
       expect(response.status).to eq(200)
       expect(json_response['approvals_required']).to eq 2
@@ -915,11 +857,11 @@ describe API::MergeRequests, api: true  do
     end
   end
 
-  describe 'POST :id/merge_requests/:merge_request_id/approve' do
+  describe 'POST :id/merge_requests/:merge_request_iid/approve' do
     before { project.update_attribute(:approvals_before_merge, 2) }
 
     context 'as the author of the merge request' do
-      before { post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user) }
+      before { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", user) }
 
       it 'returns a 401' do
         expect(response).to have_http_status(401)
@@ -933,7 +875,7 @@ describe API::MergeRequests, api: true  do
         project.team << [approver, :developer]
         project.team << [create(:user), :developer]
 
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", approver)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", approver)
       end
 
       it 'approves the merge request' do
@@ -945,7 +887,7 @@ describe API::MergeRequests, api: true  do
     end
   end
 
-  describe 'DELETE :id/merge_requests/:merge_request_id/unapprove' do
+  describe 'POST :id/merge_requests/:merge_request_iid/unapprove' do
     before { project.update_attribute(:approvals_before_merge, 2) }
 
     context 'as a user who has approved the merge request' do
@@ -959,11 +901,11 @@ describe API::MergeRequests, api: true  do
         merge_request.approvals.create(user: approver)
         merge_request.approvals.create(user: unapprover)
 
-        delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/unapprove", unapprover)
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
       end
 
       it 'unapproves the merge request' do
-        expect(response.status).to eq(200)
+        expect(response.status).to eq(201)
         expect(json_response['approvals_left']).to eq(1)
         usernames = json_response['approved_by'].map { |u| u['user']['username'] }
         expect(usernames).not_to include(unapprover.username)