Merge branch 'api-delete-note' into 'master'

Delete notes via API

Supports deleting issues, snippets, and merge requests via the API.

* Closes #14944
* Closes #14845 
* Closes #6060 

@zj I did not see that you assigned yourself in #6060. Hopefully, you did not start yet.

@rymai In #6060 this is targeted for 8.7 release. Could you review that and maybe this still lands in 8.7. 


See merge request !3557

CHANGELOG

@@ -24,6 +24,7 @@ v 8.7.0 (unreleased)
   - Ensure empty recipients are rejected in BuildsEmailService
   - API: Ability to filter milestones by state `active` and `closed` (Robert Schilling)
   - API: Fix milestone filtering by `iid` (Robert Schilling)
+  - API: Delete notes of issues, snippets, and merge requests (Robert Schilling)
   - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.)
   - Better errors handling when creating milestones inside groups
   - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.)

app/controllers/projects/notes_controller.rb

@@ -39,8 +39,7 @@ class Projects::NotesController < Projects::ApplicationController
 
   def destroy
     if note.editable?
-      note.destroy
-      note.reset_events_cache
+      Notes::DeleteService.new(project, current_user).execute(note)
     end
 
     respond_to do |format|

app/services/notes/delete_service.rb

+module Notes
+  class DeleteService < BaseService
+    def execute(note)
+      note.destroy
+      note.reset_events_cache
+    end
+  end
+end

doc/api/notes.md

@@ -105,6 +105,53 @@ Parameters:
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
+### Delete an issue note
+
+Deletes an existing note of an issue. On success, this API method returns 200
+and the deleted note. If the note does not exist, the API returns 404.
+
+```
+DELETE /projects/:id/issues/:issue_id/notes/:note_id
+```
+
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a project |
+| `issue_id` | integer | yes | The ID of an issue |
+| `note_id` | integer | yes | The ID of a note |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/5/issues/11/notes/636
+```
+
+Example Response:
+
+```json
+{
+  "id": 636,
+  "body": "This is a good idea.",
+  "attachment": null,
+  "author": {
+    "id": 1,
+    "username": "pipin",
+    "email": "admin@example.com",
+    "name": "Pip",
+    "state": "active",
+    "created_at": "2013-09-30T13:46:01Z",
+    "avatar_url": "http://www.gravatar.com/avatar/5224fd70153710e92fb8bcf79ac29d67?s=80&d=identicon",
+    "web_url": "https://gitlab.example.com/u/pipin"
+  },
+  "created_at": "2016-04-05T22:10:44.164Z",
+  "system": false,
+  "noteable_id": 11,
+  "noteable_type": "Issue",
+  "upvote": false,
+  "downvote": false
+}
+```
+
 ## Snippets
 
 ### List all snippet notes
@@ -182,6 +229,53 @@ Parameters:
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
+### Delete a snippet note
+
+Deletes an existing note of a snippet. On success, this API method returns 200
+and the deleted note. If the note does not exist, the API returns 404.
+
+```
+DELETE /projects/:id/snippets/:snippet_id/notes/:note_id
+```
+
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a project |
+| `snippet_id` | integer | yes | The ID of a snippet |
+| `note_id` | integer | yes | The ID of a note |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/5/snippets/52/notes/1659
+```
+
+Example Response:
+
+```json
+{
+  "id": 1659,
+  "body": "This is a good idea.",
+  "attachment": null,
+  "author": {
+    "id": 1,
+    "username": "pipin",
+    "email": "admin@example.com",
+    "name": "Pip",
+    "state": "active",
+    "created_at": "2013-09-30T13:46:01Z",
+    "avatar_url": "http://www.gravatar.com/avatar/5224fd70153710e92fb8bcf79ac29d67?s=80&d=identicon",
+    "web_url": "https://gitlab.example.com/u/pipin"
+  },
+  "created_at": "2016-04-06T16:51:53.239Z",
+  "system": false,
+  "noteable_id": 52,
+  "noteable_type": "Snippet",
+  "upvote": false,
+  "downvote": false
+}
+```
+
 ## Merge Requests
 
 ### List all merge request notes
@@ -262,3 +356,50 @@ Parameters:
 - `merge_request_id` (required) - The ID of a merge request
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
+
+### Delete a merge request note
+
+Deletes an existing note of a merge request. On success, this API method returns
+200 and the deleted note. If the note does not exist, the API returns 404.
+
+```
+DELETE /projects/:id/merge_requests/:merge_request_id/notes/:note_id
+```
+
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a project |
+| `merge_request_id` | integer | yes | The ID of a merge request |
+| `note_id` | integer | yes | The ID of a note |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/5/merge_requests/7/notes/1602
+```
+
+Example Response:
+
+```json
+{
+  "id": 1602,
+  "body": "This is a good idea.",
+  "attachment": null,
+  "author": {
+    "id": 1,
+    "username": "pipin",
+    "email": "admin@example.com",
+    "name": "Pip",
+    "state": "active",
+    "created_at": "2013-09-30T13:46:01Z",
+    "avatar_url": "http://www.gravatar.com/avatar/5224fd70153710e92fb8bcf79ac29d67?s=80&d=identicon",
+    "web_url": "https://gitlab.example.com/u/pipin"
+  },
+  "created_at": "2016-04-05T22:11:59.923Z",
+  "system": false,
+  "noteable_id": 7,
+  "noteable_type": "MergeRequest",
+  "upvote": false,
+  "downvote": false
+}
+```

lib/api/notes.rb

@@ -112,6 +112,23 @@ module API
           end
         end
 
+        # Delete a +noteable+ note
+        #
+        # Parameters:
+        #   id (required) - The ID of a project
+        #   noteable_id (required) - The ID of an issue, MR, or snippet
+        #   node_id (required) - The ID of a note
+        # Example Request:
+        #   DELETE /projects/:id/issues/:noteable_id/notes/:note_id
+        #   DELETE /projects/:id/snippets/:noteable_id/notes/:node_id
+        delete ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do
+          note = user_project.notes.find(params[:note_id])
+          authorize! :admin_note, note
+
+          ::Notes::DeleteService.new(user_project, current_user).execute(note)
+
+          present note, with: Entities::Note
+        end
       end
     end
   end

spec/requests/api/notes_spec.rb

@@ -241,4 +241,65 @@ describe API::API, api: true  do
     end
   end
 
+  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}/"\
+                   "notes/#{issue_note.id}", user)
+
+        expect(response.status).to eq(200)
+        # Check if note is really deleted
+        delete api("/projects/#{project.id}/issues/#{issue.id}/"\
+                   "notes/#{issue_note.id}", user)
+        expect(response.status).to eq(404)
+      end
+
+      it 'returns a 404 error when note id not found' do
+        delete api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context 'when noteable is a Snippet' do
+      it 'deletes a note' do
+        delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
+                   "notes/#{snippet_note.id}", user)
+
+        expect(response.status).to eq(200)
+        # Check if note is really deleted
+        delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
+                   "notes/#{snippet_note.id}", user)
+        expect(response.status).to eq(404)
+      end
+
+      it 'returns a 404 error when note id not found' do
+        delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
+                   "notes/123", user)
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    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)
+
+        expect(response.status).to eq(200)
+        # Check if note is really deleted
+        delete api("/projects/#{project.id}/merge_requests/"\
+                   "#{merge_request.id}/notes/#{merge_request_note.id}", user)
+        expect(response.status).to eq(404)
+      end
+
+      it 'returns a 404 error when note id not found' do
+        delete api("/projects/#{project.id}/merge_requests/"\
+                   "#{merge_request.id}/notes/123", user)
+
+        expect(response.status).to eq(404)
+      end
+    end
+  end
+
 end

spec/services/notes/delete_service_spec.rb

+require 'spec_helper'
+
+describe Notes::DeleteService, services: true do
+  describe '#execute' do
+    it 'deletes a note' do
+      project = create(:empty_project)
+      issue = create(:issue, project: project)
+      note = create(:note, project: project, noteable: issue)
+
+      described_class.new(project, note.author).execute(note)
+
+      expect(project.issues.find(issue.id).notes).not_to include(note)
+    end
+  end
+end