From 9aefaa41ab1442f81ffc15ad9a8279bd1e92c91a Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Wed, 6 Apr 2016 19:04:17 +0200
Subject: [PATCH] Fix code review issues
---
app/controllers/projects/notes_controller.rb | 2 +-
doc/api/notes.md | 130 ++++++++++++++++---
lib/api/notes.rb | 6 +-
spec/requests/api/notes_spec.rb | 20 ++-
spec/services/notes/delete_service_spec.rb | 9 +-
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
--
GitLab