Commit 5100ad09 authored by Douwe Maan's avatar Douwe Maan

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
parents ba611fdb 02b0c37c
...@@ -51,6 +51,7 @@ v 8.6.0 (unreleased) ...@@ -51,6 +51,7 @@ v 8.6.0 (unreleased)
- Continue parameters are checked to ensure redirection goes to the same instance - 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 - 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` - 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 v 8.5.8
- Bump Git version requirement to 2.7.4 - Bump Git version requirement to 2.7.4
......
...@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end end
def destroy def destroy
todo.done! todo.done
todo_notice = 'Todo was successfully marked as done.' todo_notice = 'Todo was successfully marked as done.'
...@@ -20,7 +20,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -20,7 +20,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end end
def destroy_all def destroy_all
@todos.each(&:done!) @todos.each(&:done)
respond_to do |format| respond_to do |format|
format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' } format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' }
......
...@@ -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.target_reference}", todo_target_path(todo), { title: 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
{ {
......
...@@ -5,14 +5,15 @@ ...@@ -5,14 +5,15 @@
# id :integer not null, primary key # id :integer not null, primary key
# user_id :integer not null # user_id :integer not null
# project_id :integer not null # project_id :integer not null
# target_id :integer not null # target_id :integer
# target_type :string not null # target_type :string not null
# author_id :integer # author_id :integer
# note_id :integer
# action :integer not null # action :integer not null
# state :string not null # state :string not null
# created_at :datetime # created_at :datetime
# updated_at :datetime # updated_at :datetime
# note_id :integer
# commit_id :string
# #
class Todo < ActiveRecord::Base class Todo < ActiveRecord::Base
...@@ -27,7 +28,9 @@ class Todo < ActiveRecord::Base ...@@ -27,7 +28,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, unless: :for_commit?
validates :commit_id, presence: true, if: :for_commit?
default_scope { reorder(id: :desc) } default_scope { reorder(id: :desc) }
...@@ -36,7 +39,7 @@ class Todo < ActiveRecord::Base ...@@ -36,7 +39,7 @@ class Todo < ActiveRecord::Base
state_machine :state, initial: :pending do state_machine :state, initial: :pending do
event :done do event :done do
transition [:pending, :done] => :done transition [:pending] => :done
end end
state :pending state :pending
...@@ -50,4 +53,25 @@ class Todo < ActiveRecord::Base ...@@ -50,4 +53,25 @@ 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) rescue nil
else
super
end
end
def target_reference
if for_commit?
target.short_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? || note.for_project_snippet?
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
class ChangeTargetIdToNullOnTodos < ActiveRecord::Migration
def change
change_column_null :todos, :target_id, true
end
end
class AddCommitIdToTodos < ActiveRecord::Migration
def change
add_column :todos, :commit_id, :string
add_index :todos, :commit_id
end
end
...@@ -867,7 +867,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do ...@@ -867,7 +867,7 @@ ActiveRecord::Schema.define(version: 20160316204731) do
create_table "todos", force: :cascade do |t| create_table "todos", force: :cascade do |t|
t.integer "user_id", null: false t.integer "user_id", null: false
t.integer "project_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.string "target_type", null: false
t.integer "author_id" t.integer "author_id"
t.integer "action", null: false t.integer "action", null: false
...@@ -875,9 +875,11 @@ ActiveRecord::Schema.define(version: 20160316204731) do ...@@ -875,9 +875,11 @@ ActiveRecord::Schema.define(version: 20160316204731) do
t.datetime "created_at" t.datetime "created_at"
t.datetime "updated_at" t.datetime "updated_at"
t.integer "note_id" t.integer "note_id"
t.string "commit_id"
end end
add_index "todos", ["author_id"], name: "index_todos_on_author_id", using: :btree 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", ["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", ["project_id"], name: "index_todos_on_project_id", using: :btree
add_index "todos", ["state"], name: "index_todos_on_state", using: :btree add_index "todos", ["state"], name: "index_todos_on_state", using: :btree
......
...@@ -5,14 +5,15 @@ ...@@ -5,14 +5,15 @@
# id :integer not null, primary key # id :integer not null, primary key
# user_id :integer not null # user_id :integer not null
# project_id :integer not null # project_id :integer not null
# target_id :integer not null # target_id :integer
# target_type :string not null # target_type :string not null
# author_id :integer # author_id :integer
# note_id :integer
# action :integer not null # action :integer not null
# state :string not null # state :string not null
# created_at :datetime # created_at :datetime
# updated_at :datetime # updated_at :datetime
# note_id :integer
# commit_id :string
# #
FactoryGirl.define do FactoryGirl.define do
...@@ -30,5 +31,10 @@ FactoryGirl.define do ...@@ -30,5 +31,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
...@@ -5,19 +5,24 @@ ...@@ -5,19 +5,24 @@
# id :integer not null, primary key # id :integer not null, primary key
# user_id :integer not null # user_id :integer not null
# project_id :integer not null # project_id :integer not null
# target_id :integer not null # target_id :integer
# target_type :string not null # target_type :string not null
# author_id :integer # author_id :integer
# note_id :integer
# action :integer not null # action :integer not null
# state :string not null # state :string not null
# created_at :datetime # created_at :datetime
# updated_at :datetime # updated_at :datetime
# note_id :integer
# commit_id :string
# #
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 +38,22 @@ describe Todo, models: true do ...@@ -33,8 +38,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
...@@ -55,15 +74,69 @@ describe Todo, models: true do ...@@ -55,15 +74,69 @@ describe Todo, models: true do
end end
end end
describe '#done!' do describe '#done' do
it 'changes state to done' do it 'changes state to done' do
todo = create(:todo, state: :pending) 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 end
it 'does not raise error when is already done' do it 'does not raise error when is already done' do
todo = create(:todo, state: :done) 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 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