From ba21c00f01bf4274d0e4cc3892293fc1e581b260 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Wed, 6 Apr 2016 01:21:02 +0200 Subject: Delete notes via API --- CHANGELOG | 5 ++++ app/controllers/projects/notes_controller.rb | 5 ++-- app/services/notes/delete_service.rb | 8 +++++ doc/api/notes.md | 45 ++++++++++++++++++++++++++++ lib/api/notes.rb | 17 +++++++++++ spec/requests/api/notes_spec.rb | 43 ++++++++++++++++++++++++++ spec/services/notes/delete_service_spec.rb | 15 ++++++++++ 7 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 app/services/notes/delete_service.rb create mode 100644 spec/services/notes/delete_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 593e8f77ab4..f4dbab8889a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -46,6 +46,11 @@ v 8.6.5 - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu). !3583 - Unblock user when active_directory is disabled and it can be found !3550 - Fix a 2FA authentication spoofing vulnerability. + - API: Delete notes of issues, snippets, and merge requests (Robert Schilling) + +v 8.6.5 (unreleased) + - Only update repository language if it is not set to improve performance + - Check permissions when user attempts to import members from another project v 8.6.4 - Don't attempt to fetch any tags from a forked repo (Stan Hu) diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 1b9dd568043..a9a69573eed 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/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| @@ -73,7 +72,7 @@ class Projects::NotesController < Projects::ApplicationController note = noteable.notes.find_by(data) if note - note.destroy + Notes::DeleteService.new(project, current_user).execute(note) else Notes::CreateService.new(project, current_user, note_params).execute end diff --git a/app/services/notes/delete_service.rb b/app/services/notes/delete_service.rb new file mode 100644 index 00000000000..7f1b30ec84e --- /dev/null +++ b/app/services/notes/delete_service.rb @@ -0,0 +1,8 @@ +module Notes + class DeleteService < BaseService + def execute(note) + note.destroy + note.reset_events_cache + end + end +end diff --git a/doc/api/notes.md b/doc/api/notes.md index 9168ab00d7e..82494bf83ff 100644 --- a/doc/api/notes.md +++ b/doc/api/notes.md @@ -105,6 +105,21 @@ Parameters: - `note_id` (required) - The ID of a note - `body` (required) - The content of a note +### Delete existing 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. + +``` +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 + ## Snippets ### List all snippet notes @@ -182,6 +197,21 @@ Parameters: - `note_id` (required) - The ID of a note - `body` (required) - The content of a note +### Delete existing 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. + +``` +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 + ## Merge Requests ### List all merge request notes @@ -262,3 +292,18 @@ 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 existing snippet 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. + +``` +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 diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 174473f5371..fd1704d395c 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -112,6 +112,23 @@ module API end end + # Delete a +notable+ 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 + # 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]) + not_found!('Note') unless note + authorize! :admin_note, note + ::Notes::DeleteService.new(user_project, current_user).execute(note) + true + end end end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 39f9a06fe1b..23d3c63bc1c 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -241,4 +241,47 @@ describe API::API, api: true do end end + describe ':id/#{noteables_str}/:#{noteable_id_str}/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) + 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 + + context 'when noteable is a Snippet' 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) + 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 + + context 'when noteable is a Merge Request' 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) + 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 + end + end diff --git a/spec/services/notes/delete_service_spec.rb b/spec/services/notes/delete_service_spec.rb new file mode 100644 index 00000000000..88e71c135d3 --- /dev/null +++ b/spec/services/notes/delete_service_spec.rb @@ -0,0 +1,15 @@ +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') } + + 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) + end + end +end -- cgit v1.2.1 From 9aefaa41ab1442f81ffc15ad9a8279bd1e92c91a Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Wed, 6 Apr 2016 19:04:17 +0200 Subject: 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 -- cgit v1.2.1 From 49484f9a2c2efb670f044028c555f00378484dd4 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 12 Apr 2016 14:20:58 +0200 Subject: Fix changelog entry --- CHANGELOG | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index f4dbab8889a..5dd1cf5f286 100644 --- a/CHANGELOG +++ b/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.) @@ -46,11 +47,6 @@ v 8.6.5 - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu). !3583 - Unblock user when active_directory is disabled and it can be found !3550 - Fix a 2FA authentication spoofing vulnerability. - - API: Delete notes of issues, snippets, and merge requests (Robert Schilling) - -v 8.6.5 (unreleased) - - Only update repository language if it is not set to improve performance - - Check permissions when user attempts to import members from another project v 8.6.4 - Don't attempt to fetch any tags from a forked repo (Stan Hu) -- cgit v1.2.1 From dc39c8372d760eceba50a35505dad8663b9e851e Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 12 Apr 2016 15:43:29 +0200 Subject: Adapt tests to new testing guidelines --- lib/api/notes.rb | 2 ++ spec/requests/api/notes_spec.rb | 12 ++++++------ spec/services/notes/delete_service_spec.rb | 9 ++++----- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 2ed986b9ec5..a1c98f5e8ff 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -124,7 +124,9 @@ module API 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 diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index b35e67b5bd3..a467bc935af 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -243,7 +243,7 @@ describe API::API, api: true 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 + it 'deletes a note' do delete api("/projects/#{project.id}/issues/#{issue.id}/"\ "notes/#{issue_note.id}", user) @@ -254,7 +254,7 @@ describe API::API, api: true do expect(response.status).to eq(404) end - it 'should return a 404 error when note id not found' do + 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) @@ -262,7 +262,7 @@ describe API::API, api: true do end context 'when noteable is a Snippet' do - it 'should delete a note' do + it 'deletes a note' do delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\ "notes/#{snippet_note.id}", user) @@ -273,7 +273,7 @@ describe API::API, api: true do expect(response.status).to eq(404) end - it 'should return a 404 error when note id not found' do + it 'returns a 404 error when note id not found' do delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\ "notes/123", user) @@ -282,7 +282,7 @@ describe API::API, api: true do end context 'when noteable is a Merge Request' do - it 'should delete a note' do + it 'deletes a note' do delete api("/projects/#{project.id}/merge_requests/"\ "#{merge_request.id}/notes/#{merge_request_note.id}", user) @@ -293,7 +293,7 @@ describe API::API, api: true do expect(response.status).to eq(404) end - it 'should return a 404 error when note id not found' do + it 'returns a 404 error when note id not found' do delete api("/projects/#{project.id}/merge_requests/"\ "#{merge_request.id}/notes/123", user) diff --git a/spec/services/notes/delete_service_spec.rb b/spec/services/notes/delete_service_spec.rb index 07aa57c4642..1d0a747a480 100644 --- a/spec/services/notes/delete_service_spec.rb +++ b/spec/services/notes/delete_service_spec.rb @@ -1,13 +1,12 @@ require 'spec_helper' describe Notes::DeleteService, services: true do - let(:project) { create(:empty_project) } - let(:issue) { create(:issue, project: project) } - let(:note) { create(:note, project: project, noteable: issue) } - describe '#execute' do it 'deletes a note' do - project = note.project + 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) -- cgit v1.2.1