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

11 files changed, 187 insertions, 45 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 74217e80bfe..28c1c30e208 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -51,6 +51,7 @@ v 8.6.0 (unreleased)
   - Continue parameters are checked to ensure redirection goes to the same instance
   - User deletion is now done in the background so the request can not time out
   - Canceled builds are now ignored in compound build status if marked as `allowed to fail`
+  - Trigger a todo for mentions on commits page
 
 v 8.5.8
   - Bump Git version requirement to 2.7.4
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 7857af9c5de..be488483b09 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy
-    todo.done!
+    todo.done
 
     todo_notice = 'Todo was successfully marked as done.'
 
@@ -20,7 +20,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/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 07ddc691d85..edc5686cf08 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.target_reference}", todo_target_path(todo), { title: 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..d85f7bfdf57 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.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
 #
 
 class Todo < ActiveRecord::Base
@@ -27,7 +28,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, unless: :for_commit?
+  validates :commit_id, presence: true, if: :for_commit?
 
   default_scope { reorder(id: :desc) }
 
@@ -36,7 +39,7 @@ class Todo < ActiveRecord::Base
 
   state_machine :state, initial: :pending do
     event :done do
-      transition [:pending, :done] => :done
+      transition [:pending] => :done
     end
 
     state :pending
@@ -50,4 +53,25 @@ 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) rescue nil
+    else
+      super
+    end
+  end
+
+  def target_reference
+    if for_commit?
+      target.short_id
+    else
+      target.to_reference
+    end
+  end
 end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 4392e2d17fe..f2662922e90 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? || note.for_project_snippet?
 
     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/db/migrate/20160316192622_change_target_id_to_null_on_todos.rb b/db/migrate/20160316192622_change_target_id_to_null_on_todos.rb
new file mode 100644
index 00000000000..6871b3920df
--- /dev/null
+++ b/db/migrate/20160316192622_change_target_id_to_null_on_todos.rb
@@ -0,0 +1,5 @@
+class ChangeTargetIdToNullOnTodos < ActiveRecord::Migration
+  def change
+    change_column_null :todos, :target_id, true
+  end
+end
diff --git a/db/migrate/20160316204731_add_commit_id_to_todos.rb b/db/migrate/20160316204731_add_commit_id_to_todos.rb
new file mode 100644
index 00000000000..ae19fdd1abd
--- /dev/null
+++ b/db/migrate/20160316204731_add_commit_id_to_todos.rb
@@ -0,0 +1,6 @@
+class AddCommitIdToTodos < ActiveRecord::Migration
+  def change
+    add_column :todos, :commit_id, :string
+    add_index :todos, :commit_id
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 7e6863ef47e..5b2f5aa3ddd 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -867,7 +867,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
   create_table "todos", force: :cascade do |t|
     t.integer  "user_id",     null: false
     t.integer  "project_id",  null: false
-    t.integer  "target_id",   null: false
+    t.integer  "target_id"
     t.string   "target_type", null: false
     t.integer  "author_id"
     t.integer  "action",      null: false
@@ -875,9 +875,11 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "note_id"
+    t.string   "commit_id"
   end
 
   add_index "todos", ["author_id"], name: "index_todos_on_author_id", using: :btree
+  add_index "todos", ["commit_id"], name: "index_todos_on_commit_id", using: :btree
   add_index "todos", ["note_id"], name: "index_todos_on_note_id", using: :btree
   add_index "todos", ["project_id"], name: "index_todos_on_project_id", using: :btree
   add_index "todos", ["state"], name: "index_todos_on_state", using: :btree
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