Merge branch 'trigger-todo-for-mentions-on-commits-page' into 'master'

Trigger a todo for mentions on commits page

Closes #14006 

* Screenshot:

![todo-commit](/uploads/5d34de0b7afcea7548123dafddf60c45/todo-commit.png)

See merge request !3262

3 files changed, 94 insertions, 10 deletions

diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index bd85b1d798a..7ae06c27840 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -5,14 +5,15 @@
 #  id          :integer          not null, primary key
 #  user_id     :integer          not null
 #  project_id  :integer          not null
-#  target_id   :integer          not null
+#  target_id   :integer
 #  target_type :string           not null
 #  author_id   :integer
-#  note_id     :integer
 #  action      :integer          not null
 #  state       :string           not null
 #  created_at  :datetime
 #  updated_at  :datetime
+#  note_id     :integer
+#  commit_id   :string
 #
 
 FactoryGirl.define do
@@ -30,5 +31,10 @@ FactoryGirl.define do
     trait :mentioned do
       action { Todo::MENTIONED }
     end
+
+    trait :on_commit do
+      commit_id RepoHelpers.sample_commit.id
+      target_type "Commit"
+    end
   end
 end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index fe9ea7e7d1e..d9b86b9368f 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -5,19 +5,24 @@
 #  id          :integer          not null, primary key
 #  user_id     :integer          not null
 #  project_id  :integer          not null
-#  target_id   :integer          not null
+#  target_id   :integer
 #  target_type :string           not null
 #  author_id   :integer
-#  note_id     :integer
 #  action      :integer          not null
 #  state       :string           not null
 #  created_at  :datetime
 #  updated_at  :datetime
+#  note_id     :integer
+#  commit_id   :string
 #
 
 require 'spec_helper'
 
 describe Todo, models: true do
+  let(:project) { create(:project) }
+  let(:commit) { project.commit }
+  let(:issue) { create(:issue) }
+
   describe 'relationships' do
     it { is_expected.to belong_to(:author).class_name("User") }
     it { is_expected.to belong_to(:note) }
@@ -33,8 +38,22 @@ describe Todo, models: true do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of(:action) }
-    it { is_expected.to validate_presence_of(:target) }
+    it { is_expected.to validate_presence_of(:target_type) }
     it { is_expected.to validate_presence_of(:user) }
+
+    context 'for commits' do
+      subject { described_class.new(target_type: 'Commit') }
+
+      it { is_expected.to validate_presence_of(:commit_id) }
+      it { is_expected.not_to validate_presence_of(:target_id) }
+    end
+
+    context 'for issuables' do
+      subject { described_class.new(target: issue) }
+
+      it { is_expected.to validate_presence_of(:target_id) }
+      it { is_expected.not_to validate_presence_of(:commit_id) }
+    end
   end
 
   describe '#body' do
@@ -55,15 +74,69 @@ describe Todo, models: true do
     end
   end
 
-  describe '#done!' do
+  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')
+      expect { todo.done }.to change(todo, :state).from('pending').to('done')
     end
 
     it 'does not raise error when is already done' do
       todo = create(:todo, state: :done)
-      expect { todo.done! }.not_to raise_error
+      expect { todo.done }.not_to raise_error
+    end
+  end
+
+  describe '#for_commit?' do
+    it 'returns true when target is a commit' do
+      subject.target_type = 'Commit'
+      expect(subject.for_commit?).to eq true
+    end
+
+    it 'returns false when target is an issuable' do
+      subject.target_type = 'Issue'
+      expect(subject.for_commit?).to eq false
+    end
+  end
+
+  describe '#target' do
+    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
+      subject.target_id = issue.id
+      subject.target_type = issue.class.name
+      expect(subject.target).to eq issue
+    end
+  end
+
+  describe '#target_reference' do
+    it 'returns the short commit id for commits' do
+      subject.project = project
+      subject.target_type = 'Commit'
+      subject.commit_id = commit.id
+
+      expect(subject.target_reference).to eq commit.short_id
+    end
+
+    it 'returns reference for issuables' do
+      subject.target = issue
+      expect(subject.target_reference).to eq issue.to_reference
     end
   end
 end
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 96420acb31d..b4728807b8b 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -148,8 +148,13 @@ describe TodoService, services: true do
         should_not_create_todo(user: stranger, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
       end
 
-      it 'does not create todo when leaving a note on commit' do
-        should_not_create_any_todo { service.new_note(note_on_commit, john_doe) }
+      it 'creates a todo for each valid mentioned user when leaving a note on commit' do
+        service.new_note(note_on_commit, john_doe)
+
+        should_create_todo(user: michael, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
+        should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
+        should_not_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
+        should_not_create_todo(user: stranger, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
       end
 
       it 'does not create todo when leaving a note on snippet' do