From d536d9590ba1aa663bd75fc529fb13b0bf164814 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 27 Mar 2017 10:23:14 +0200 Subject: [PATCH 1/2] Remove `:id/merge_requests/:merge_request_iid/comments` endpoints Comments for a merge request should be obtained to the `notes` endpoint. --- lib/api/merge_requests.rb | 35 -------------- spec/requests/api/merge_requests_spec.rb | 58 ------------------------ 2 files changed, 93 deletions(-) diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 5cc807d5bff..c8033664133 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -226,41 +226,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 diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 9aba1d75612..61d965e8974 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -623,64 +623,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) From add5cd996f2a2261c9897052609a8b9d8a47261a Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Mon, 27 Mar 2017 15:01:45 +0200 Subject: [PATCH 2/2] API: Make the /notes endpoint work with noteable iid instead of id In API V4 all endpoints were changed so Merge Requests and Issues should be referred by iid, instead of id. Except the /notes endpoint was forgotten. So change the endpoints from: - /projects/:id/issues/:issue_id/notes - /projects/:id/merge_requests/:merge_request_id/notes To: - /projects/:id/issues/:issue_iid/notes - /projects/:id/merge_requests/:merge_request_iid/notes For Project Snippets nothing changes. --- ...move-merge-requests-comments-endpoints.yml | 4 ++ doc/api/notes.md | 40 ++++++------- lib/api/helpers.rb | 5 ++ lib/api/notes.rb | 14 +++-- spec/requests/api/notes_spec.rb | 60 +++++++++---------- 5 files changed, 68 insertions(+), 55 deletions(-) create mode 100644 changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml diff --git a/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml b/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml new file mode 100644 index 00000000000..e3fb62d53b6 --- /dev/null +++ b/changelogs/unreleased/29871-api-remove-merge-requests-comments-endpoints.yml @@ -0,0 +1,4 @@ +--- +title: 'API: Make the /notes endpoint work with noteable iid instead of id' +merge_request: +author: diff --git a/doc/api/notes.md b/doc/api/notes.md index 6ef06b2c2e9..5e927143714 100644 --- a/doc/api/notes.md +++ b/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 diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index bd22b82476b..61527c1e20b 100644 --- a/lib/api/helpers.rb +++ b/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 diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 29ceffdbd2d..de39e579ac3 100644 --- a/lib/api/notes.rb +++ b/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 diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 347f8f6fa3b..d8eb8ce921e 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -34,7 +34,7 @@ describe API::Notes, api: true do describe "GET /projects/:id/noteable/:noteable_id/notes" do context "when noteable is an Issue" do it "returns an array of issue notes" do - get api("/projects/#{project.id}/issues/#{issue.id}/notes", user) + get api("/projects/#{project.id}/issues/#{issue.iid}/notes", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -50,7 +50,7 @@ describe API::Notes, api: true do context "and current user cannot view the notes" do it "returns an empty array" do - get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -62,7 +62,7 @@ describe API::Notes, api: true do before { ext_issue.update_attributes(confidential: true) } it "returns 404" do - get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", user) expect(response).to have_http_status(404) end @@ -70,7 +70,7 @@ describe API::Notes, api: true do context "and current user can view the note" do it "returns an empty array" do - get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user) + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes", private_user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -106,7 +106,7 @@ describe API::Notes, api: true do context "when noteable is a Merge Request" do it "returns an array of merge_requests notes" do - get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user) expect(response).to have_http_status(200) expect(response).to include_pagination_headers @@ -131,21 +131,21 @@ describe API::Notes, api: true do describe "GET /projects/:id/noteable/:noteable_id/notes/:note_id" do context "when noteable is an Issue" do it "returns an issue note by id" do - get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user) + get api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", user) expect(response).to have_http_status(200) expect(json_response['body']).to eq(issue_note.note) end it "returns a 404 error if issue note not found" do - get api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user) + get api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user) expect(response).to have_http_status(404) end context "and current user cannot view the note" do it "returns a 404 error" do - get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user) + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes/#{cross_reference_note.id}", user) expect(response).to have_http_status(404) end @@ -154,7 +154,7 @@ describe API::Notes, api: true do before { issue.update_attributes(confidential: true) } it "returns 404" do - get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", private_user) + get api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", private_user) expect(response).to have_http_status(404) end @@ -162,7 +162,7 @@ describe API::Notes, api: true do context "and current user can view the note" do it "returns an issue note by id" do - get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user) + get api("/projects/#{ext_proj.id}/issues/#{ext_issue.iid}/notes/#{cross_reference_note.id}", private_user) expect(response).to have_http_status(200) expect(json_response['body']).to eq(cross_reference_note.note) @@ -190,7 +190,7 @@ describe API::Notes, api: true do describe "POST /projects/:id/noteable/:noteable_id/notes" do context "when noteable is an Issue" do it "creates a new issue note" do - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!' expect(response).to have_http_status(201) expect(json_response['body']).to eq('hi!') @@ -198,13 +198,13 @@ describe API::Notes, api: true do end it "returns a 400 bad request error if body not given" do - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user) + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user) expect(response).to have_http_status(400) end it "returns a 401 unauthorized error if user not authenticated" do - post api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!' + post api("/projects/#{project.id}/issues/#{issue.iid}/notes"), body: 'hi!' expect(response).to have_http_status(401) end @@ -212,7 +212,7 @@ describe API::Notes, api: true do context 'when an admin or owner makes the request' do it 'accepts the creation date to be set' do creation_time = 2.weeks.ago - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!', created_at: creation_time expect(response).to have_http_status(201) @@ -226,7 +226,7 @@ describe API::Notes, api: true do let(:issue2) { create(:issue, project: project) } it 'creates a new issue note' do - post api("/projects/#{project.id}/issues/#{issue2.id}/notes", user), body: ':+1:' + post api("/projects/#{project.id}/issues/#{issue2.iid}/notes", user), body: ':+1:' expect(response).to have_http_status(201) expect(json_response['body']).to eq(':+1:') @@ -235,7 +235,7 @@ describe API::Notes, api: true do context 'when the user is posting an award emoji on his/her own issue' do it 'creates a new issue note' do - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: ':+1:' + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: ':+1:' expect(response).to have_http_status(201) expect(json_response['body']).to eq(':+1:') @@ -270,7 +270,7 @@ describe API::Notes, api: true do project = create(:empty_project, :private) { |p| p.add_guest(user) } issue = create(:issue, :confidential, project: project) - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'Foo' expect(response).to have_http_status(404) @@ -285,7 +285,7 @@ describe API::Notes, api: true do # from a different project, see #15577 # before do - post api("/projects/#{project.id}/issues/#{private_issue.id}/notes", user), + post api("/projects/#{private_issue.project.id}/issues/#{private_issue.iid}/notes", user), body: 'Hi!' end @@ -303,14 +303,14 @@ describe API::Notes, api: true do it "creates an activity event when an issue note is created" do expect(Event).to receive(:create) - post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' + post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!' end end describe 'PUT /projects/:id/noteable/:noteable_id/notes/:note_id' do context 'when noteable is an Issue' do it 'returns modified note' do - put api("/projects/#{project.id}/issues/#{issue.id}/"\ + put api("/projects/#{project.id}/issues/#{issue.iid}/"\ "notes/#{issue_note.id}", user), body: 'Hello!' expect(response).to have_http_status(200) @@ -318,14 +318,14 @@ describe API::Notes, api: true do end it 'returns a 404 error when note id not found' do - put api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user), + put api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user), body: 'Hello!' expect(response).to have_http_status(404) end it 'returns a 400 bad request error if body not given' do - put api("/projects/#{project.id}/issues/#{issue.id}/"\ + put api("/projects/#{project.id}/issues/#{issue.iid}/"\ "notes/#{issue_note.id}", user) expect(response).to have_http_status(400) @@ -351,7 +351,7 @@ describe API::Notes, api: true do context 'when noteable is a Merge Request' do it 'returns modified note' do - put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\ + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/"\ "notes/#{merge_request_note.id}", user), body: 'Hello!' expect(response).to have_http_status(200) @@ -359,7 +359,7 @@ describe API::Notes, api: true do end it 'returns a 404 error when note id not found' do - put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\ + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/"\ "notes/12345", user), body: "Hello!" expect(response).to have_http_status(404) @@ -370,18 +370,18 @@ describe API::Notes, api: true do 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}/"\ + delete api("/projects/#{project.id}/issues/#{issue.iid}/"\ "notes/#{issue_note.id}", user) expect(response).to have_http_status(204) # Check if note is really deleted - delete api("/projects/#{project.id}/issues/#{issue.id}/"\ + delete api("/projects/#{project.id}/issues/#{issue.iid}/"\ "notes/#{issue_note.id}", user) expect(response).to have_http_status(404) end it 'returns a 404 error when note id not found' do - delete api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user) + delete api("/projects/#{project.id}/issues/#{issue.iid}/notes/12345", user) expect(response).to have_http_status(404) end @@ -410,18 +410,18 @@ describe API::Notes, api: true do 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) + "#{merge_request.iid}/notes/#{merge_request_note.id}", user) expect(response).to have_http_status(204) # Check if note is really deleted delete api("/projects/#{project.id}/merge_requests/"\ - "#{merge_request.id}/notes/#{merge_request_note.id}", user) + "#{merge_request.iid}/notes/#{merge_request_note.id}", user) expect(response).to have_http_status(404) end it 'returns a 404 error when note id not found' do delete api("/projects/#{project.id}/merge_requests/"\ - "#{merge_request.id}/notes/12345", user) + "#{merge_request.iid}/notes/12345", user) expect(response).to have_http_status(404) end