Refactor `Todo#target`

2 files changed, 18 insertions, 12 deletions

diff --git a/app/models/todo.rb b/app/models/todo.rb
index ab804d712a0..d85f7bfdf57 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -61,14 +61,10 @@ class Todo < ActiveRecord::Base
   # override to return commits, which are not active record
   def target
     if for_commit?
-      project.commit(commit_id)
+      project.commit(commit_id) rescue nil
     else
       super
     end
-  # Temp fix to prevent app crash
-  # if note commit id doesn't exist
-  rescue
-    nil
   end
 
   def target_reference
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index 22acfdfe2c0..d9b86b9368f 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -99,13 +99,23 @@ describe Todo, models: true do
   end
 
   describe '#target' do
-    it 'returns an instance of Commit for commits' do
-      subject.project = project
-      subject.target_type = 'Commit'
-      subject.commit_id = commit.id
-
-      expect(subject.target).to be_a(Commit)
-      expect(subject.target).to eq commit
+    context 'for commits' do
+      it 'returns an instance of Commit when exists' do
+        subject.project = project
+        subject.target_type = 'Commit'
+        subject.commit_id = commit.id
+
+        expect(subject.target).to be_a(Commit)
+        expect(subject.target).to eq commit
+      end
+
+      it 'returns nil when does not exists' do
+        subject.project = project
+        subject.target_type = 'Commit'
+        subject.commit_id = 'xxxx'
+
+        expect(subject.target).to be_nil
+      end
     end
 
     it 'returns the issuable for issuables' do