Trigger a todo for mentions on commits page

parent 1e76245d
...@@ -16,15 +16,20 @@ module TodosHelper ...@@ -16,15 +16,20 @@ module TodosHelper
def todo_target_link(todo) def todo_target_link(todo)
target = todo.target_type.titleize.downcase 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 end
def todo_target_path(todo) def todo_target_path(todo)
anchor = dom_id(todo.note) if todo.note.present? anchor = dom_id(todo.note) if todo.note.present?
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), polymorphic_path([todo.project.namespace.becomes(Namespace),
todo.project, todo.target], anchor: anchor) todo.project, todo.target], anchor: anchor)
end end
end
def todos_filter_params def todos_filter_params
{ {
......
...@@ -27,7 +27,9 @@ class Todo < ActiveRecord::Base ...@@ -27,7 +27,9 @@ class Todo < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true, allow_nil: true 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) } default_scope { reorder(id: :desc) }
...@@ -50,4 +52,29 @@ class Todo < ActiveRecord::Base ...@@ -50,4 +52,29 @@ class Todo < ActiveRecord::Base
target.title target.title
end end
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 end
...@@ -103,24 +103,16 @@ class TodoService ...@@ -103,24 +103,16 @@ class TodoService
# * mark all pending todos related to the target for the current user as done # * mark all pending todos related to the target for the current user as done
# #
def mark_pending_todos_as_done(target, user) 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 end
private private
def create_todos(project, target, author, users, action, note = nil) def create_todos(users, attributes)
Array(users).each do |user| Array(users).each do |user|
next if pending_todos(user, project, target).exists? next if pending_todos(user, attributes).exists?
Todo.create(attributes.merge(user_id: user.id))
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
)
end end
end end
...@@ -130,8 +122,8 @@ class TodoService ...@@ -130,8 +122,8 @@ class TodoService
end end
def handle_note(note, author) def handle_note(note, author)
# Skip system notes, notes on commit, and notes on project snippet # Skip system notes, and notes on project snippet
return if note.system? || ['Commit', 'Snippet'].include?(note.noteable_type) return if note.system? || ['Snippet'].include?(note.noteable_type)
project = note.project project = note.project
target = note.noteable target = note.noteable
...@@ -142,13 +134,39 @@ class TodoService ...@@ -142,13 +134,39 @@ class TodoService
def create_assignment_todo(issuable, author) def create_assignment_todo(issuable, author)
if issuable.assignee && issuable.assignee != 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, 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 end
attributes
end end
def create_mention_todos(project, issuable, author, note = nil) def attributes_for_todo(project, target, author, action, note = nil)
mentioned_users = filter_mentioned_users(project, note || issuable, author) attributes_for_target(target).merge!(
create_todos(project, issuable, author, mentioned_users, Todo::MENTIONED, note) project_id: project.id,
author_id: author.id,
action: action,
note: note
)
end end
def filter_mentioned_users(project, target, author) def filter_mentioned_users(project, target, author)
...@@ -160,11 +178,8 @@ class TodoService ...@@ -160,11 +178,8 @@ class TodoService
mentioned_users.uniq mentioned_users.uniq
end end
def pending_todos(user, project, target) def pending_todos(user, criteria = {})
user.todos.pending.where( valid_keys = [:project_id, :target_id, :target_type, :commit_id]
project_id: project.id, user.todos.pending.where(criteria.slice(*valid_keys))
target_id: target.id,
target_type: target.class.name
)
end end
end end
...@@ -30,5 +30,10 @@ FactoryGirl.define do ...@@ -30,5 +30,10 @@ FactoryGirl.define do
trait :mentioned do trait :mentioned do
action { Todo::MENTIONED } action { Todo::MENTIONED }
end end
trait :on_commit do
commit_id RepoHelpers.sample_commit.id
target_type "Commit"
end
end end
end end
...@@ -18,6 +18,10 @@ ...@@ -18,6 +18,10 @@
require 'spec_helper' require 'spec_helper'
describe Todo, models: true do describe Todo, models: true do
let(:project) { create(:project) }
let(:commit) { project.commit }
let(:issue) { create(:issue) }
describe 'relationships' do describe 'relationships' do
it { is_expected.to belong_to(:author).class_name("User") } it { is_expected.to belong_to(:author).class_name("User") }
it { is_expected.to belong_to(:note) } it { is_expected.to belong_to(:note) }
...@@ -33,8 +37,22 @@ describe Todo, models: true do ...@@ -33,8 +37,22 @@ describe Todo, models: true do
describe 'validations' do describe 'validations' do
it { is_expected.to validate_presence_of(:action) } 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) } 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 end
describe '#body' do describe '#body' do
...@@ -66,4 +84,46 @@ describe Todo, models: true do ...@@ -66,4 +84,46 @@ describe Todo, models: true do
expect { todo.done! }.not_to raise_error expect { todo.done! }.not_to raise_error
end end
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 end
...@@ -148,8 +148,13 @@ describe TodoService, services: true do ...@@ -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) should_not_create_todo(user: stranger, target: issue, author: john_doe, action: Todo::MENTIONED, note: note)
end end
it 'does not create todo when leaving a note on commit' do it 'creates a todo for each valid mentioned user when leaving a note on commit' do
should_not_create_any_todo { service.new_note(note_on_commit, john_doe) } 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 end
it 'does not create todo when leaving a note on snippet' do it 'does not create todo when leaving a note on snippet' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment