Trigger a todo for mentions on commits page

6 files changed, 149 insertions, 32 deletions

diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 07ddc691d85..9604a4e01b4 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -16,14 +16,19 @@ module TodosHelper
 
   def todo_target_link(todo)
     target = todo.target_type.titleize.downcase
-    link_to "#{target} #{todo.target.to_reference}", todo_target_path(todo), { title: h(todo.target.title) }
+    link_to "#{target} #{todo.to_reference}", todo_target_path(todo), { title: h(todo.target.title) }
   end
 
   def todo_target_path(todo)
     anchor = dom_id(todo.note) if todo.note.present?
 
-    polymorphic_path([todo.project.namespace.becomes(Namespace),
-                      todo.project, todo.target], anchor: anchor)
+    if todo.for_commit?
+      namespace_project_commit_path(todo.project.namespace.becomes(Namespace), todo.project,
+                                    todo.target, anchor: anchor)
+    else
+      polymorphic_path([todo.project.namespace.becomes(Namespace),
+                        todo.project, todo.target], anchor: anchor)
+    end
   end
 
   def todos_filter_params
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 5f91991f781..b00a1b3dc7d 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -27,7 +27,9 @@ class Todo < ActiveRecord::Base
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
 
-  validates :action, :project, :target, :user, presence: true
+  validates :action, :project, :target_type, :user, presence: true
+  validates :target_id, presence: true, if: ->(t) { t.target_type.present? && t.target_type != 'Commit' }
+  validates :commit_id, presence: true, if: ->(t) { t.target_type.present? && t.target_type == 'Commit' }
 
   default_scope { reorder(id: :desc) }
 
@@ -50,4 +52,29 @@ class Todo < ActiveRecord::Base
       target.title
     end
   end
+
+  def for_commit?
+    target_type == "Commit"
+  end
+
+  # override to return commits, which are not active record
+  def target
+    if for_commit?
+      project.commit(commit_id)
+    else
+      super
+    end
+  # Temp fix to prevent app crash
+  # if note commit id doesn't exist
+  rescue
+    nil
+  end
+
+  def to_reference
+    if for_commit?
+      Commit.truncate_sha(commit_id)
+    else
+      target.to_reference
+    end
+  end
 end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 4392e2d17fe..1027c309121 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -103,24 +103,16 @@ class TodoService
   #  * mark all pending todos related to the target for the current user as done
   #
   def mark_pending_todos_as_done(target, user)
-    pending_todos(user, target.project, target).update_all(state: :done)
+    attributes = attributes_for_target(target)
+    pending_todos(user, attributes).update_all(state: :done)
   end
 
   private
 
-  def create_todos(project, target, author, users, action, note = nil)
+  def create_todos(users, attributes)
     Array(users).each do |user|
-      next if pending_todos(user, project, target).exists?
-
-      Todo.create(
-        project: project,
-        user_id: user.id,
-        author_id: author.id,
-        target_id: target.id,
-        target_type: target.class.name,
-        action: action,
-        note: note
-      )
+      next if pending_todos(user, attributes).exists?
+      Todo.create(attributes.merge(user_id: user.id))
     end
   end
 
@@ -130,8 +122,8 @@ class TodoService
   end
 
   def handle_note(note, author)
-    # Skip system notes, notes on commit, and notes on project snippet
-    return if note.system? || ['Commit', 'Snippet'].include?(note.noteable_type)
+    # Skip system notes, and notes on project snippet
+    return if note.system? || ['Snippet'].include?(note.noteable_type)
 
     project = note.project
     target  = note.noteable
@@ -142,13 +134,39 @@ class TodoService
 
   def create_assignment_todo(issuable, author)
     if issuable.assignee && issuable.assignee != author
-      create_todos(issuable.project, issuable, author, issuable.assignee, Todo::ASSIGNED)
+      attributes = attributes_for_todo(issuable.project, issuable, author, Todo::ASSIGNED)
+      create_todos(issuable.assignee, attributes)
     end
   end
 
-  def create_mention_todos(project, issuable, author, note = nil)
-    mentioned_users = filter_mentioned_users(project, note || issuable, author)
-    create_todos(project, issuable, author, mentioned_users, Todo::MENTIONED, note)
+  def create_mention_todos(project, target, author, note = nil)
+    mentioned_users = filter_mentioned_users(project, note || target, author)
+    attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note)
+    create_todos(mentioned_users, attributes)
+  end
+
+  def attributes_for_target(target)
+    attributes = {
+      project_id: target.project.id,
+      target_id: target.id,
+      target_type: target.class.name,
+      commit_id: nil
+    }
+
+    if target.is_a?(Commit)
+      attributes.merge!(target_id: nil, commit_id: target.id)
+    end
+
+    attributes
+  end
+
+  def attributes_for_todo(project, target, author, action, note = nil)
+    attributes_for_target(target).merge!(
+      project_id: project.id,
+      author_id: author.id,
+      action: action,
+      note: note
+    )
   end
 
   def filter_mentioned_users(project, target, author)
@@ -160,11 +178,8 @@ class TodoService
     mentioned_users.uniq
   end
 
-  def pending_todos(user, project, target)
-    user.todos.pending.where(
-      project_id: project.id,
-      target_id: target.id,
-      target_type: target.class.name
-    )
+  def pending_todos(user, criteria = {})
+    valid_keys = [:project_id, :target_id, :target_type, :commit_id]
+    user.todos.pending.where(criteria.slice(*valid_keys))
   end
 end
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index bd85b1d798a..b484315ed12 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -30,5 +30,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..3e59443589c 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -18,6 +18,10 @@
 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 +37,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
@@ -66,4 +84,46 @@ describe Todo, models: true do
       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
+    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
+    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 '#to_reference' do
+    it 'returns the short commit id for commits' do
+      subject.target_type = 'Commit'
+      subject.commit_id = commit.id
+      expect(subject.to_reference).to eq Commit.truncate_sha(commit.id)
+    end
+
+    it 'returns reference for issuables' do
+      subject.target = issue
+      expect(subject.to_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