Fix code review issues

5 files changed, 140 insertions, 27 deletions

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index a9a69573eed..707a0d0e5c6 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -72,7 +72,7 @@ class Projects::NotesController < Projects::ApplicationController
     note = noteable.notes.find_by(data)
 
     if note
-      Notes::DeleteService.new(project, current_user).execute(note)
+      note.destroy
     else
       Notes::CreateService.new(project, current_user, note_params).execute
     end
diff --git a/doc/api/notes.md b/doc/api/notes.md
index 82494bf83ff..2e0936f11b5 100644
--- a/doc/api/notes.md
+++ b/doc/api/notes.md
@@ -105,10 +105,10 @@ Parameters:
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
-### Delete existing issue note
+### Delete an issue note
 
-Deletes an existing note of an issue. On success, this API method returns 200.
-If the note does not exist, the API returns 404.
+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
@@ -116,9 +116,41 @@ DELETE /projects/:id/issues/:issue_id/notes/:note_id
 
 Parameters:
 
-- `id` (required) - The ID of a project
-- `issue_id` (required) - The ID of an issue
-- `note_id` (required) - The ID of a note
+| 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
 
@@ -197,10 +229,10 @@ Parameters:
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
-### Delete existing snippet note
+### Delete a snippet note
 
-Deletes an existing note of a snippet. On success, this API method returns 200.
-If the note does not exist, the API returns 404.
+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
@@ -208,9 +240,41 @@ DELETE /projects/:id/snippets/:snippet_id/notes/:note_id
 
 Parameters:
 
-- `id` (required) - The ID of a project
-- `snippet_id` (required) - The ID of a snippet
-- `note_id` (required) - The ID of a note
+| 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
 
@@ -293,10 +357,10 @@ Parameters:
 - `note_id` (required) - The ID of a note
 - `body` (required) - The content of a note
 
-### Delete existing snippet note
+### Delete a merge request note
 
 Deletes an existing note of a merge request. On success, this API method returns
-200. If the note does not exist, the API returns 404.
+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
@@ -304,6 +368,38 @@ DELETE /projects/:id/merge_requests/:merge_request_id/notes/:note_id
 
 Parameters:
 
-- `id` (required) - The ID of a project
-- `merge_request_id` (required) - The ID of a merge request
-- `note_id` (required) - The ID of a note
+| 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
+}
+```
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index fd1704d395c..2ed986b9ec5 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -112,10 +112,9 @@ module API
           end
         end
 
-        # Delete a +notable+ note
+        # Delete a +noteable+ note
         #
         # Parameters:
-        # 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
@@ -124,10 +123,9 @@ module API
         #   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])
-          not_found!('Note') unless note
           authorize! :admin_note, note
           ::Notes::DeleteService.new(user_project, current_user).execute(note)
-          true
+          present note, with: Entities::Note
         end
       end
     end
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index 23d3c63bc1c..b35e67b5bd3 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -241,16 +241,22 @@ describe API::API, api: true  do
     end
   end
 
-  describe ':id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id' do
+  describe 'DELETE /projects/:id/noteable/:noteable_id/notes/:note_id' do
     context 'when noteable is an Issue' do
       it 'should delete 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 'should return 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
@@ -259,12 +265,18 @@ describe API::API, api: true  do
       it 'should delete 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 'should return 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
@@ -273,12 +285,18 @@ describe API::API, api: true  do
       it 'should delete 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 'should return 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
diff --git a/spec/services/notes/delete_service_spec.rb b/spec/services/notes/delete_service_spec.rb
index 88e71c135d3..07aa57c4642 100644
--- a/spec/services/notes/delete_service_spec.rb
+++ b/spec/services/notes/delete_service_spec.rb
@@ -3,13 +3,14 @@ require 'spec_helper'
 describe Notes::DeleteService, services: true do
   let(:project) { create(:empty_project) }
   let(:issue) { create(:issue, project: project) }
-  let(:user) { create(:user) }
-  let(:note) { create(:note, project: project, noteable: issue, author: user, note: 'Note') }
+  let(:note) { create(:note, project: project, noteable: issue) }
 
   describe '#execute' do
     it 'deletes a note' do
-      Notes::DeleteService.new(project, user).execute(note)
-      expect(project.issues.find(issue.id).notes).to_not include(note)
+      project = note.project
+      described_class.new(project, note.author).execute(note)
+
+      expect(project.issues.find(issue.id).notes).not_to include(note)
     end
   end
 end