Merge branch '25465-todo-done-clicking-is-kind-of-unsafe' into 'master'

Todo done clicking is kind of unusable.

Closes #25465

See merge request !8691

4 files changed, 94 insertions, 48 deletions

diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index 79ef3a1adad..0a3ac9f9512 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -1,16 +1,19 @@
 require 'spec_helper'
 
 describe Dashboard::TodosController do
+  include ApiHelpers
+
   let(:user) { create(:user) }
+  let(:author)  { create(:user) }
   let(:project) { create(:empty_project) }
   let(:todo_service) { TodoService.new }
 
-  describe 'GET #index' do
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
+  before do
+    sign_in(user)
+    project.team << [user, :developer]
+  end
 
+  describe 'GET #index' do
     context 'when using pagination' do
       let(:last_page) { user.todos.page.total_pages }
       let!(:issues) { create_list(:issue, 2, project: project, assignee: user) }
@@ -34,4 +37,16 @@ describe Dashboard::TodosController do
       end
     end
   end
+
+  describe 'PATCH #restore' do
+    let(:todo) { create(:todo, :done, user: user, project: project, author: author) }
+
+    it 'restores the todo to pending state' do
+      patch :restore, id: todo.id
+
+      expect(todo.reload).to be_pending
+      expect(response).to have_http_status(200)
+      expect(json_response).to eq({ "count" => 1, "done_count" => 0 })
+    end
+  end
 end
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index b4e4cd97780..a5265f1b189 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -40,6 +40,10 @@ FactoryGirl.define do
       action { Todo::UNMERGEABLE }
     end
 
+    trait :pending do
+      state :pending
+    end
+
     trait :done do
       state :done
     end
diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb
index 1b352be9331..fb19dac1d6a 100644
--- a/spec/features/todos/todos_spec.rb
+++ b/spec/features/todos/todos_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe 'Dashboard Todos', feature: true do
+  include WaitForAjax
+
   let(:user)    { create(:user) }
   let(:author)  { create(:user) }
   let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
@@ -34,40 +36,65 @@ describe 'Dashboard Todos', feature: true do
         end
       end
 
-      describe 'deleting the todo' do
+      shared_examples 'deleting the todo' do
         before do
-          first('.done-todo').click
+          first('.js-done-todo').click
+        end
+
+        it 'is marked as done-reversible in the list' do
+          expect(page).to have_selector('.todos-list .todo.todo-pending.done-reversible')
         end
 
-        it 'is removed from the list' do
-          expect(page).not_to have_selector('.todos-list .todo')
+        it 'shows Undo button' do
+          expect(page).to have_selector('.js-undo-todo', visible: true)
+          expect(page).to have_selector('.js-done-todo', visible: false)
         end
 
-        it 'shows "All done" message' do
-          expect(page).to have_selector('.todos-all-done', count: 1)
+        it 'updates todo count' do
+          expect(page).to have_content 'To do 0'
+          expect(page).to have_content 'Done 1'
+        end
+
+        it 'has not "All done" message' do
+          expect(page).not_to have_selector('.todos-all-done')
         end
       end
 
-      context 'todo is stale on the page' do
+      shared_examples 'deleting and restoring the todo' do
         before do
-          todos = TodosFinder.new(user, state: :pending).execute
-          TodoService.new.mark_todos_as_done(todos, user)
+          first('.js-done-todo').click
+          wait_for_ajax
+          first('.js-undo-todo').click
         end
 
-        describe 'deleting the todo' do
-          before do
-            first('.done-todo').click
-          end
+        it 'is marked back as pending in the list' do
+          expect(page).not_to have_selector('.todos-list .todo.todo-pending.done-reversible')
+          expect(page).to have_selector('.todos-list .todo.todo-pending')
+        end
 
-          it 'is removed from the list' do
-            expect(page).not_to have_selector('.todos-list .todo')
-          end
+        it 'shows Done button' do
+          expect(page).to have_selector('.js-undo-todo', visible: false)
+          expect(page).to have_selector('.js-done-todo', visible: true)
+        end
 
-          it 'shows "All done" message' do
-            expect(page).to have_selector('.todos-all-done', count: 1)
-          end
+        it 'updates todo count' do
+          expect(page).to have_content 'To do 1'
+          expect(page).to have_content 'Done 0'
         end
       end
+
+      it_behaves_like 'deleting the todo'
+      it_behaves_like 'deleting and restoring the todo'
+
+      context 'todo is stale on the page' do
+        before do
+          todos = TodosFinder.new(user, state: :pending).execute
+          TodoService.new.mark_todos_as_done(todos, user)
+        end
+
+        it_behaves_like 'deleting the todo'
+        it_behaves_like 'deleting and restoring the todo'
+      end
     end
 
     context 'User has Todos with labels spanning multiple projects' do
@@ -113,18 +140,6 @@ describe 'Dashboard Todos', feature: true do
         expect(page).to have_selector('.gl-pagination .page', count: 2)
       end
 
-      describe 'completing last todo from last page', js: true do
-        it 'redirects to the previous page' do
-          visit dashboard_todos_path(page: 2)
-          expect(page).to have_css("#todo_#{Todo.last.id}")
-
-          click_link('Done')
-
-          expect(current_path).to eq dashboard_todos_path
-          expect(page).to have_css("#todo_#{Todo.first.id}")
-        end
-      end
-
       describe 'mark all as done', js: true do
         before do
           visit dashboard_todos_path
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 4320365ab57..9f24cc0f3f2 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -287,39 +287,51 @@ describe TodoService, services: true do
       end
     end
 
-    shared_examples 'marking todos as done' do |meth|
-      let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
-      let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
+    shared_examples 'updating todos state' do |meth, state, new_state|
+      let!(:first_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) }
+      let!(:second_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) }
 
-      it 'marks related todos for the user as done' do
+      it 'updates related todos for the user with the new_state' do
         service.send(meth, collection, john_doe)
 
-        expect(first_todo.reload).to be_done
-        expect(second_todo.reload).to be_done
+        expect(first_todo.reload.state?(new_state)).to be true
+        expect(second_todo.reload.state?(new_state)).to be true
       end
 
       describe 'cached counts' do
         it 'updates when todos change' do
-          expect(john_doe.todos_done_count).to eq(0)
-          expect(john_doe.todos_pending_count).to eq(2)
+          expect(john_doe.todos.where(state: new_state).count).to eq(0)
+          expect(john_doe.todos.where(state: state).count).to eq(2)
           expect(john_doe).to receive(:update_todos_count_cache).and_call_original
 
           service.send(meth, collection, john_doe)
 
-          expect(john_doe.todos_done_count).to eq(2)
-          expect(john_doe.todos_pending_count).to eq(0)
+          expect(john_doe.todos.where(state: new_state).count).to eq(2)
+          expect(john_doe.todos.where(state: state).count).to eq(0)
         end
       end
     end
 
     describe '#mark_todos_as_done' do
-      it_behaves_like 'marking todos as done', :mark_todos_as_done do
+      it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :done do
         let(:collection) { [first_todo, second_todo] }
       end
     end
 
     describe '#mark_todos_as_done_by_ids' do
-      it_behaves_like 'marking todos as done', :mark_todos_as_done_by_ids do
+      it_behaves_like 'updating todos state', :mark_todos_as_done_by_ids, :pending, :done do
+        let(:collection) { [first_todo, second_todo].map(&:id) }
+      end
+    end
+
+    describe '#mark_todos_as_pending' do
+      it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :pending do
+        let(:collection) { [first_todo, second_todo] }
+      end
+    end
+
+    describe '#mark_todos_as_pending_by_ids' do
+      it_behaves_like 'updating todos state', :mark_todos_as_pending_by_ids, :done, :pending do
         let(:collection) { [first_todo, second_todo].map(&:id) }
       end
     end