From 7629dc9982f5559972acf9d9b9d98f78ad53e54c Mon Sep 17 00:00:00 2001
From: Paco Guzman <pacoguzmanp@gmail.com>
Date: Fri, 12 Aug 2016 17:38:09 +0200
Subject: [PATCH] Add specs to ensure a successful return on the UI when mark
 as done a already done todo.

---
 app/services/todo_service.rb       |  3 ++-
 spec/features/todos/todos_spec.rb  | 21 +++++++++++++++++++++
 spec/services/todo_service_spec.rb | 21 +++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index e0ccb654590..2aab8c736d6 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -148,7 +148,8 @@ class TodoService
   def mark_todos_as_done_by_ids(ids, current_user)
     todos = current_user.todos.where(id: ids)
 
-    marked_todos = todos.update_all(state: :done)
+    # Only return those that are not really on that state
+    marked_todos = todos.where.not(state: :done).update_all(state: :done)
     current_user.update_todos_count_cache
     marked_todos
   end
diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb
index 0342f4f1d97..32544f3f538 100644
--- a/spec/features/todos/todos_spec.rb
+++ b/spec/features/todos/todos_spec.rb
@@ -41,6 +41,27 @@ describe 'Dashboard Todos', feature: true do
           expect(page).to have_content("You're all done!")
         end
       end
+
+      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
+
+        describe 'deleting the todo' do
+          before do
+            first('.done-todo').click
+          end
+
+          it 'is removed from the list' do
+            expect(page).not_to have_selector('.todos-list .todo')
+          end
+
+          it 'shows "All done" message' do
+            expect(page).to have_content("You're all done!")
+          end
+        end
+      end
     end
 
     context 'User has Todos with labels spanning multiple projects' do
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 296fd1bd5a4..cafcad3e3c0 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -496,6 +496,7 @@ describe TodoService, services: true do
 
   describe '#mark_todos_as_done' do
     let(:issue) { create(:issue, project: project, author: author, assignee: john_doe) }
+    let(:another_issue) { create(:issue, project: project, author: author, assignee: john_doe) }
 
     it 'marks a relation of todos as done' do
       create(:todo, :mentioned, user: john_doe, target: issue, project: project)
@@ -518,6 +519,26 @@ describe TodoService, services: true do
       expect(TodoService.new.mark_todos_as_done([todo], john_doe)).to eq(1)
     end
 
+    context 'when some of the todos are done already' do
+      before do
+        create(:todo, :mentioned, user: john_doe, target: issue, project: project)
+        create(:todo, :mentioned, user: john_doe, target: another_issue, project: project)
+      end
+
+      it 'returns the number of those still pending' do
+        TodoService.new.mark_pending_todos_as_done(issue, john_doe)
+
+        expect(TodoService.new.mark_todos_as_done(Todo.all, john_doe)).to eq(1)
+      end
+
+      it 'returns 0 if all are done' do
+        TodoService.new.mark_pending_todos_as_done(issue, john_doe)
+        TodoService.new.mark_pending_todos_as_done(another_issue, john_doe)
+
+        expect(TodoService.new.mark_todos_as_done(Todo.all, john_doe)).to eq(0)
+      end
+    end
+
     it 'caches the number of todos of a user', :caching do
       create(:todo, :mentioned, user: john_doe, target: issue, project: project)
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)