Todo done clicking is kind of unusable.

The Done button will change to an Undo button and the line item will be greyed out.
Bold links will be unbolded.
The user can undo the task by clicking the Undo button.

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