From c29da3f8ca1d759b8cbecb91b6e0529b18cc5c85 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 16 Mar 2016 20:31:30 -0300 Subject: [PATCH] Trigger a todo for mentions on commits page --- app/helpers/todos_helper.rb | 11 +++-- app/models/todo.rb | 29 ++++++++++++- app/services/todo_service.rb | 65 ++++++++++++++++++------------ spec/factories/todos.rb | 5 +++ spec/models/todo_spec.rb | 62 +++++++++++++++++++++++++++- spec/services/todo_service_spec.rb | 9 ++++- 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 -- GitLab