Merge branch 'api-todos-restful' into 'master'

Use POST methods to mark Todos as done

Closes #28467

See merge request !9410

changelogs/unreleased/api-todos-restful.yml

+---
+title: 'API: Use POST requests to mark todos as done'
+merge_request: 9410
+author: Robert Schilling

doc/api/todos.md

@@ -184,7 +184,7 @@ Marks a single pending todo given by its ID for the current user as done. The
 todo marked as done is returned in the response.
 ```
-DELETE /todos/:id
+POST /todos/:id/mark_as_done
 ```
 Parameters:
@@ -194,7 +194,7 @@ Parameters:
 | `id` | integer | yes | The ID of a todo |
 ```bash
-curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/todos/130
+curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/todos/130/mark_as_done
 ```
 Example Response:
@@ -277,20 +277,15 @@ Example Response:
 ## Mark all todos as done
-Marks all pending todos for the current user as done. It returns the number of marked todos.
+Marks all pending todos for the current user as done. It returns the HTTP status code `204` with an empty response.
 ```
-DELETE /todos
+POST /todos/mark_as_done
 ```
 ```bash
-curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/todos
+curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/todos/donmark_as_donee
 ```
-Example Response:
-
-```json
-3
-```
 [ce-3188]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3188

doc/api/v3_to_v4.md

@@ -24,9 +24,9 @@ changes are in V4:
   - `/gitlab_ci_ymls/:key`
   - `/dockerfiles/:key`
 - Moved `/projects/fork/:id` to `/projects/:id/fork`
+- Moved `DELETE /todos` to `POST /todos/mark_as_done` and `DELETE /todos/:todo_id` to `POST /todos/:todo_id/mark_as_done`
 - Endpoints `/projects/owned`, `/projects/visible`, `/projects/starred` & `/projects/all` are consolidated into `/projects` using query parameters
 - Return pagination headers for all endpoints that return an array
 - Removed `DELETE projects/:id/deploy_keys/:key_id/disable`. Use `DELETE projects/:id/deploy_keys/:key_id` instead
 - Moved `PUT /users/:id/(block|unblock)` to `POST /users/:id/(block|unblock)`
 - Labels filter on `projects/:id/issues` and `/issues` now matches only issues containing all labels (i.e.: Logical AND, not OR)
-

lib/api/api.rb

@@ -19,6 +19,7 @@ module API
       mount ::API::V3::Repositories
       mount ::API::V3::SystemHooks
       mount ::API::V3::Tags
+      mount ::API::V3::Todos
       mount ::API::V3::Templates
       mount ::API::V3::Users
     end

lib/api/todos.rb

@@ -58,7 +58,7 @@ module API
       params do
         requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
       end
-      delete ':id' do
+      post ':id/mark_as_done' do
         todo = current_user.todos.find(params[:id])
         TodoService.new.mark_todos_as_done([todo], current_user)
@@ -66,9 +66,11 @@ module API
       end
       desc 'Mark all todos as done'
-      delete do
+      post '/mark_as_done' do
         todos = find_todos
         TodoService.new.mark_todos_as_done(todos, current_user)
+
+        no_content!
       end
     end
   end

lib/api/v3/todos.rb

+module API
+  module V3
+    class Todos < Grape::API
+      before { authenticate! }
+
+      resource :todos do
+        desc 'Mark a todo as done' do
+          success ::API::Entities::Todo
+        end
+        params do
+          requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
+        end
+        delete ':id' do
+          todo = current_user.todos.find(params[:id])
+          TodoService.new.mark_todos_as_done([todo], current_user)
+
+          present todo.reload, with: ::API::Entities::Todo, current_user: current_user
+        end
+
+        desc 'Mark all todos as done'
+        delete do
+          todos = TodosFinder.new(current_user, params).execute
+          TodoService.new.mark_todos_as_done(todos, current_user)
+        end
+      end
+    end
+  end
+end

spec/requests/api/todos_spec.rb

@@ -107,46 +107,47 @@ describe API::Todos, api: true do
     end
   end
-  describe 'DELETE /todos/:id' do
+  describe 'POST /todos/:id/mark_as_done' do
     context 'when unauthenticated' do
       it 'returns authentication error' do
-        delete api("/todos/#{pending_1.id}")
-        expect(response.status).to eq(401)
+        post api("/todos/#{pending_1.id}/mark_as_done")
+        expect(response).to have_http_status(401)
       end
     end
     context 'when authenticated' do
       it 'marks a todo as done' do
-        delete api("/todos/#{pending_1.id}", john_doe)
-        expect(response.status).to eq(200)
+        post api("/todos/#{pending_1.id}/mark_as_done", john_doe)
+        expect(response).to have_http_status(201)
+        expect(json_response['id']).to eq(pending_1.id)
+        expect(json_response['state']).to eq('done')
         expect(pending_1.reload).to be_done
       end
       it 'updates todos cache' do
         expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original
-        delete api("/todos/#{pending_1.id}", john_doe)
+        post api("/todos/#{pending_1.id}/mark_as_done", john_doe)
       end
     end
   end
-  describe 'DELETE /todos' do
+  describe 'POST /mark_as_done' do
     context 'when unauthenticated' do
       it 'returns authentication error' do
-        delete api('/todos')
-        expect(response.status).to eq(401)
+        post api('/todos/mark_as_done')
+        expect(response).to have_http_status(401)
       end
     end
     context 'when authenticated' do
       it 'marks all todos as done' do
-        delete api('/todos', john_doe)
-        expect(response.status).to eq(200)
-        expect(response.body).to eq('3')
+        post api('/todos/mark_as_done', john_doe)
+        expect(response).to have_http_status(204)
         expect(pending_1.reload).to be_done
         expect(pending_2.reload).to be_done
         expect(pending_3.reload).to be_done
@@ -155,7 +156,7 @@ describe API::Todos, api: true do
       it 'updates todos cache' do
         expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original
-        delete api("/todos", john_doe)
+        post api("/todos/mark_as_done", john_doe)
       end
     end
   end

spec/requests/api/v3/todos_spec.rb

+require 'spec_helper'
+
+describe API::V3::Todos, api: true do
+  include ApiHelpers
+
+  let(:project_1) { create(:empty_project) }
+  let(:project_2) { create(:empty_project) }
+  let(:author_1) { create(:user) }
+  let(:author_2) { create(:user) }
+  let(:john_doe) { create(:user, username: 'john_doe') }
+  let!(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) }
+  let!(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe) }
+  let!(:pending_3) { create(:todo, project: project_1, author: author_2, user: john_doe) }
+  let!(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe) }
+
+  before do
+    project_1.team << [john_doe, :developer]
+    project_2.team << [john_doe, :developer]
+  end
+
+  describe 'DELETE /todos/:id' do
+    context 'when unauthenticated' do
+      it 'returns authentication error' do
+        delete v3_api("/todos/#{pending_1.id}")
+
+        expect(response.status).to eq(401)
+      end
+    end
+
+    context 'when authenticated' do
+      it 'marks a todo as done' do
+        delete v3_api("/todos/#{pending_1.id}", john_doe)
+
+        expect(response.status).to eq(200)
+        expect(pending_1.reload).to be_done
+      end
+
+      it 'updates todos cache' do
+        expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original
+
+        delete v3_api("/todos/#{pending_1.id}", john_doe)
+      end
+    end
+  end
+
+  describe 'DELETE /todos' do
+    context 'when unauthenticated' do
+      it 'returns authentication error' do
+        delete v3_api('/todos')
+
+        expect(response.status).to eq(401)
+      end
+    end
+
+    context 'when authenticated' do
+      it 'marks all todos as done' do
+        delete v3_api('/todos', john_doe)
+
+        expect(response.status).to eq(200)
+        expect(response.body).to eq('3')
+        expect(pending_1.reload).to be_done
+        expect(pending_2.reload).to be_done
+        expect(pending_3.reload).to be_done
+      end
+
+      it 'updates todos cache' do
+        expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original
+
+        delete v3_api("/todos", john_doe)
+      end
+    end
+  end
+end