diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-03-18 17:10:09 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-03-18 21:23:11 +0100 |
commit | 1793a65eeb2f75dd28c8f243e0b8acc0e7b0366b (patch) | |
tree | cdc9f47024e78ca32a84f7c49b68d39b974545af | |
parent | 17d40eab7f6669dc63e22574e05e5fbd4e92a4ad (diff) | |
download | gitlab-ce-1793a65eeb2f75dd28c8f243e0b8acc0e7b0366b.tar.gz |
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/dashboard/todos_controller.rb | 4 | ||||
-rw-r--r-- | app/helpers/todos_helper.rb | 11 | ||||
-rw-r--r-- | app/models/todo.rb | 32 | ||||
-rw-r--r-- | app/services/todo_service.rb | 65 | ||||
-rw-r--r-- | db/migrate/20160316192622_change_target_id_to_null_on_todos.rb | 5 | ||||
-rw-r--r-- | db/migrate/20160316204731_add_commit_id_to_todos.rb | 6 | ||||
-rw-r--r-- | db/schema.rb | 4 | ||||
-rw-r--r-- | spec/factories/todos.rb | 10 | ||||
-rw-r--r-- | spec/models/todo_spec.rb | 85 | ||||
-rw-r--r-- | spec/services/todo_service_spec.rb | 9 |
11 files changed, 187 insertions, 45 deletions
diff --git a/CHANGELOG b/CHANGELOG index 0a7d000e6a0..ca360c6706d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -52,6 +52,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 |