Merge branch 'fix-todos' into 'master'

Does not raise an error when Todo  is already marked as done

Closes #13681

See merge request !2926

4 files changed, 15 insertions, 34 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 9a08d4d4e7f..977a93f6693 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,14 +1,15 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.5.1
-  - Add when the Builds & Runners API changes got introduced
-  - Fix Side-by-side view after loading diff results
   - Fix error 500 on some merged merge requests
-  - Issues can now be dragged & dropped into empty milestone lists. This is also
-    possible with MRs
+  - Fix error 500 when trying to marke an already done todo as "done"
+  - Fix Side-by-side view after loading diff results
   - Fix an issue where MRs weren't sortable
+  - Fix a set of small UI glitches in project, profile, and wiki pages
+  - Issues can now be dragged & dropped into empty milestone lists. This is also
+  possible with MRs
   - Restrict permissions on public/uploads
-  - Set of small UI fixes to project, profile, and wiki pages
+  - Add when the Builds & Runners API changes got introduced
   - Update sentry-raven gem to 0.15.6
 
 v 8.5.0
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 9ee9039f004..43cf8fa71af 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -15,7 +15,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy_all
-    @todos.each(&:done)
+    @todos.each(&:done!)
 
     respond_to do |format|
       format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' }
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 34d71c1b0d3..5f91991f781 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -36,7 +36,7 @@ class Todo < ActiveRecord::Base
 
   state_machine :state, initial: :pending do
     event :done do
-      transition pending: :done
+      transition [:pending, :done] => :done
     end
 
     state :pending
diff --git a/spec/models/todo_spec.rb.rb b/spec/models/todo_spec.rb
index ac481bf9fbd..fe9ea7e7d1e 100644
--- a/spec/models/todo_spec.rb.rb
+++ b/spec/models/todo_spec.rb
@@ -37,20 +37,6 @@ describe Todo, models: true do
     it { is_expected.to validate_presence_of(:user) }
   end
 
-  describe '#action_name' do
-    it 'returns proper message when action is an assigment' do
-      subject.action = Todo::ASSIGNED
-
-      expect(subject.action_name).to eq 'assigned'
-    end
-
-    it 'returns proper message when action is a mention' do
-      subject.action = Todo::MENTIONED
-
-      expect(subject.action_name).to eq 'mentioned you on'
-    end
-  end
-
   describe '#body' do
     before do
       subject.target = build(:issue, title: 'Bugfix')
@@ -69,21 +55,15 @@ describe Todo, models: true do
     end
   end
 
-  describe '#target_iid' do
-    let(:issue) { build(:issue, id: 1, iid: 5) }
-
-    before do
-      subject.target = issue
-    end
-
-    it 'returns target.iid when target respond to iid' do
-      expect(subject.target_iid).to eq 5
+  describe '#done!' do
+    it 'changes state to done' do
+      todo = create(:todo, state: :pending)
+      expect { todo.done! }.to change(todo, :state).from('pending').to('done')
     end
 
-    it 'returns target_id when target does not respond to iid' do
-      allow(issue).to receive(:respond_to?).with(:iid).and_return(false)
-
-      expect(subject.target_iid).to eq 1
+    it 'does not raise error when is already done' do
+      todo = create(:todo, state: :done)
+      expect { todo.done! }.not_to raise_error
     end
   end
 end