diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 6 | ||||
| -rw-r--r-- | app/models/issue.rb | 16 | ||||
| -rw-r--r-- | app/observers/mailer_observer.rb | 80 | ||||
| -rw-r--r-- | app/observers/merge_request_observer.rb | 31 | ||||
| -rw-r--r-- | app/observers/note_observer.rb | 36 | ||||
| -rw-r--r-- | app/roles/issue_commonality.rb | 17 | ||||
| -rw-r--r-- | config/application.rb | 10 | ||||
| -rw-r--r-- | spec/factories.rb | 6 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 43 | ||||
| -rw-r--r-- | spec/observers/merge_request_observer_spec.rb | 193 | ||||
| -rw-r--r-- | spec/observers/note_observer_spec.rb | 109 |
11 files changed, 445 insertions, 102 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c4c99204e03..b597795a519 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,7 +1,6 @@ class ApplicationController < ActionController::Base before_filter :authenticate_user! before_filter :reject_blocked! - before_filter :set_current_user_for_mailer before_filter :set_current_user_for_observers before_filter :dev_tools if Rails.env == 'development' @@ -41,11 +40,8 @@ class ApplicationController < ActionController::Base end end - def set_current_user_for_mailer - MailerObserver.current_user = current_user - end - def set_current_user_for_observers + MergeRequestObserver.current_user = current_user IssueObserver.current_user = current_user end diff --git a/app/models/issue.rb b/app/models/issue.rb index 5450bb499a8..3dd1c8c8991 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -14,22 +14,6 @@ class Issue < ActiveRecord::Base def self.open_for(user) opened.assigned(user) end - - def is_assigned? - !!assignee_id - end - - def is_being_reassigned? - assignee_id_changed? - end - - def is_being_closed? - closed_changed? && closed - end - - def is_being_reopened? - closed_changed? && !closed - end end # == Schema Information diff --git a/app/observers/mailer_observer.rb b/app/observers/mailer_observer.rb deleted file mode 100644 index 331aaa35e3d..00000000000 --- a/app/observers/mailer_observer.rb +++ /dev/null @@ -1,80 +0,0 @@ -class MailerObserver < ActiveRecord::Observer - observe :note, :merge_request - cattr_accessor :current_user - - def after_create(model) - new_note(model) if model.kind_of?(Note) - new_merge_request(model) if model.kind_of?(MergeRequest) - end - - def after_update(model) - changed_merge_request(model) if model.kind_of?(MergeRequest) - end - - protected - - def new_note(note) - if note.notify - # Notify whole team except author of note - notify_note(note) - elsif note.notify_author - # Notify only author of resource - Notify.note_commit_email(note.commit_author.id, note.id).deliver - else - # Otherwise ignore it - nil - end - end - - def notify_note note - # reject author of note from mail list - users = note.project.users.reject { |u| u.id == current_user.id } - - users.each do |u| - case note.noteable_type - when "Commit"; Notify.note_commit_email(u.id, note.id).deliver - when "Issue"; Notify.note_issue_email(u.id, note.id).deliver - when "Wiki"; Notify.note_wiki_email(u.id, note.id).deliver - when "MergeRequest"; Notify.note_merge_request_email(u.id, note.id).deliver - when "Snippet"; true - else - Notify.note_wall_email(u.id, note.id).deliver - end - end - end - - def new_merge_request(merge_request) - if merge_request.assignee && merge_request.assignee != current_user - Notify.new_merge_request_email(merge_request.id).deliver - end - end - - def changed_merge_request(merge_request) - status_notify_and_comment merge_request, :reassigned_merge_request_email - end - - # This method used for Issues & Merge Requests - # - # It create a comment for Issue or MR if someone close/reopen. - # It also notify via email if assignee was changed - # - def status_notify_and_comment target, mail_method - # If assigne changed - notify to recipients - if target.assignee_id_changed? - recipients_ids = target.assignee_id_was, target.assignee_id - recipients_ids.delete current_user.id - - recipients_ids.each do |recipient_id| - Notify.send(mail_method, recipient_id, target.id, target.assignee_id_was).deliver - end - end - - # Create comment about status changed - if target.closed_changed? - note = Note.new(noteable: target, project: target.project) - note.author = current_user - note.note = "_Status changed to #{target.closed ? 'closed' : 'reopened'}_" - note.save() - end - end -end diff --git a/app/observers/merge_request_observer.rb b/app/observers/merge_request_observer.rb new file mode 100644 index 00000000000..9090794ded2 --- /dev/null +++ b/app/observers/merge_request_observer.rb @@ -0,0 +1,31 @@ +class MergeRequestObserver < ActiveRecord::Observer + cattr_accessor :current_user + + def after_create(merge_request) + if merge_request.assignee && merge_request.assignee != current_user + Notify.new_merge_request_email(merge_request.id).deliver + end + end + + def after_update(merge_request) + send_reassigned_email(merge_request) if merge_request.is_being_reassigned? + + status = nil + status = 'closed' if merge_request.is_being_closed? + status = 'reopened' if merge_request.is_being_reopened? + if status + Note.create_status_change_note(merge_request, current_user, status) + end + end + + protected + + def send_reassigned_email(merge_request) + recipients_ids = merge_request.assignee_id_was, merge_request.assignee_id + recipients_ids.delete current_user.id + + recipients_ids.each do |recipient_id| + Notify.reassigned_merge_request_email(recipient_id, merge_request.id, merge_request.assignee_id_was).deliver + end + end +end diff --git a/app/observers/note_observer.rb b/app/observers/note_observer.rb new file mode 100644 index 00000000000..844848567eb --- /dev/null +++ b/app/observers/note_observer.rb @@ -0,0 +1,36 @@ +class NoteObserver < ActiveRecord::Observer + + def after_create(note) + if note.notify + # Notify whole team except author of note + notify_team_of_new_note(note) + elsif note.notify_author + # Notify only author of resource + Notify.note_commit_email(note.commit_author.id, note.id).deliver + else + # Otherwise ignore it + nil + end + end + + protected + + def notify_team_of_new_note(note) + team_without_note_author(note).map do |u| + case note.noteable_type + when "Commit"; Notify.note_commit_email(u.id, note.id).deliver + when "Issue"; Notify.note_issue_email(u.id, note.id).deliver + when "Wiki"; Notify.note_wiki_email(u.id, note.id).deliver + when "MergeRequest"; Notify.note_merge_request_email(u.id, note.id).deliver + when "Wall"; Notify.note_wall_email(u.id, note.id).deliver + when "Snippet"; true # no notifications for snippets? + else + true + end + end + end + + def team_without_note_author(note) + note.project.users.reject { |u| u.id == note.author.id } + end +end diff --git a/app/roles/issue_commonality.rb b/app/roles/issue_commonality.rb index 3d9074867b7..2d10bfec866 100644 --- a/app/roles/issue_commonality.rb +++ b/app/roles/issue_commonality.rb @@ -46,4 +46,21 @@ module IssueCommonality def new? today? && created_at == updated_at end + + def is_assigned? + !!assignee_id + end + + def is_being_reassigned? + assignee_id_changed? + end + + def is_being_closed? + closed_changed? && closed + end + + def is_being_reopened? + closed_changed? && !closed + end + end diff --git a/config/application.rb b/config/application.rb index 1189035fa86..74ed330effd 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,7 +23,15 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer, :system_hook_observer, :users_project_observer + config.active_record.observers = :activity_observer, + :issue_observer, + :key_observer, + :merge_request_observer, + :note_observer, + :project_observer, + :system_hook_observer, + :user_observer, + :users_project_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. diff --git a/spec/factories.rb b/spec/factories.rb index cb3541cc7f2..0258f8920e9 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -76,6 +76,12 @@ FactoryGirl.define do project source_branch "master" target_branch "stable" + + trait :closed do + closed true + end + + factory :closed_merge_request, traits: [:closed] end factory :note do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 91d404f79c0..be40c561adb 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -50,4 +50,47 @@ describe MergeRequest do merge_request.mr_and_commit_notes.count.should == 2 end end + + subject { Factory.create(:merge_request) } + + describe '#is_being_reassigned?' do + it 'returns true if the merge_request assignee has changed' do + subject.assignee = Factory(:user) + subject.is_being_reassigned?.should be_true + end + it 'returns false if the merge request assignee has not changed' do + subject.is_being_reassigned?.should be_false + end + end + + describe '#is_being_closed?' do + it 'returns true if the closed attribute has changed and is now true' do + subject.closed = true + subject.is_being_closed?.should be_true + end + it 'returns false if the closed attribute has changed and is now false' do + merge_request = Factory.create(:closed_merge_request) + merge_request.closed = false + merge_request.is_being_closed?.should be_false + end + it 'returns false if the closed attribute has not changed' do + subject.is_being_closed?.should be_false + end + end + + + describe '#is_being_reopened?' do + it 'returns true if the closed attribute has changed and is now false' do + merge_request = Factory.create(:closed_merge_request) + merge_request.closed = false + merge_request.is_being_reopened?.should be_true + end + it 'returns false if the closed attribute has changed and is now true' do + subject.closed = true + subject.is_being_reopened?.should be_false + end + it 'returns false if the closed attribute has not changed' do + subject.is_being_reopened?.should be_false + end + end end diff --git a/spec/observers/merge_request_observer_spec.rb b/spec/observers/merge_request_observer_spec.rb new file mode 100644 index 00000000000..a9ba79271e3 --- /dev/null +++ b/spec/observers/merge_request_observer_spec.rb @@ -0,0 +1,193 @@ +require 'spec_helper' + +describe MergeRequestObserver do + let(:some_user) { double(:user, id: 1) } + let(:assignee) { double(:user, id: 2) } + let(:author) { double(:user, id: 3) } + let(:mr) { double(:merge_request, id: 42, assignee: assignee, author: author) } + + before(:each) { subject.stub(:current_user).and_return(some_user) } + + subject { MergeRequestObserver.instance } + + describe '#after_create' do + + it 'is called when a merge request is created' do + subject.should_receive(:after_create) + + MergeRequest.observers.enable :merge_request_observer do + Factory.create(:merge_request, project: Factory.create(:project)) + end + end + + it 'sends an email to the assignee' do + Notify.should_receive(:new_merge_request_email).with(mr.id). + and_return(double(deliver: true)) + + subject.after_create(mr) + end + + it 'does not send an email to the assignee if assignee created the merge request' do + subject.stub(:current_user).and_return(assignee) + Notify.should_not_receive(:new_merge_request_email) + + subject.after_create(mr) + end + end + + context '#after_update' do + before(:each) do + mr.stub(:is_being_reassigned?).and_return(false) + mr.stub(:is_being_closed?).and_return(false) + mr.stub(:is_being_reopened?).and_return(false) + end + + it 'is called when a merge request is changed' do + changed = Factory.create(:merge_request, project: Factory.create(:project)) + subject.should_receive(:after_update) + + MergeRequest.observers.enable :merge_request_observer do + changed.title = 'I changed' + changed.save + end + end + + context 'a reassigned email' do + it 'is sent if the merge request is being reassigned' do + mr.should_receive(:is_being_reassigned?).and_return(true) + subject.should_receive(:send_reassigned_email).with(mr) + + subject.after_update(mr) + end + + it 'is not sent if the merge request is not being reassigned' do + mr.should_receive(:is_being_reassigned?).and_return(false) + subject.should_not_receive(:send_reassigned_email) + + subject.after_update(mr) + end + end + + context 'a status "closed"' do + it 'note is created if the merge request is being closed' do + mr.should_receive(:is_being_closed?).and_return(true) + Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed') + + subject.after_update(mr) + end + + it 'note is not created if the merge request is not being closed' do + mr.should_receive(:is_being_closed?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed') + + subject.after_update(mr) + end + + it 'notification is delivered if the merge request being closed' do + mr.stub(:is_being_closed?).and_return(true) + Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed') + + subject.after_update(mr) + end + + it 'notification is not delivered if the merge request not being closed' do + mr.stub(:is_being_closed?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed') + + subject.after_update(mr) + end + + it 'notification is delivered only to author if the merge request is being closed' do + mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil) + mr_without_assignee.stub(:is_being_reassigned?).and_return(false) + mr_without_assignee.stub(:is_being_closed?).and_return(true) + mr_without_assignee.stub(:is_being_reopened?).and_return(false) + Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'closed') + + subject.after_update(mr_without_assignee) + end + end + + context 'a status "reopened"' do + it 'note is created if the merge request is being reopened' do + mr.should_receive(:is_being_reopened?).and_return(true) + Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened') + + subject.after_update(mr) + end + + it 'note is not created if the merge request is not being reopened' do + mr.should_receive(:is_being_reopened?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened') + + subject.after_update(mr) + end + + it 'notification is delivered if the merge request being reopened' do + mr.stub(:is_being_reopened?).and_return(true) + Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened') + + subject.after_update(mr) + end + + it 'notification is not delivered if the merge request is not being reopened' do + mr.stub(:is_being_reopened?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened') + + subject.after_update(mr) + end + + it 'notification is delivered only to author if the merge request is being reopened' do + mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil) + mr_without_assignee.stub(:is_being_reassigned?).and_return(false) + mr_without_assignee.stub(:is_being_closed?).and_return(false) + mr_without_assignee.stub(:is_being_reopened?).and_return(true) + Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'reopened') + + subject.after_update(mr_without_assignee) + end + end + end + + describe '#send_reassigned_email' do + let(:previous_assignee) { double(:user, id: 3) } + + before(:each) do + mr.stub(:assignee_id).and_return(assignee.id) + mr.stub(:assignee_id_was).and_return(previous_assignee.id) + end + + def it_sends_a_reassigned_email_to(recipient) + Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id). + and_return(double(deliver: true)) + end + + def it_does_not_send_a_reassigned_email_to(recipient) + Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id) + end + + it 'sends a reassigned email to the previous and current assignees' do + it_sends_a_reassigned_email_to assignee.id + it_sends_a_reassigned_email_to previous_assignee.id + + subject.send(:send_reassigned_email, mr) + end + + context 'does not send an email to the user who made the reassignment' do + it 'if the user is the assignee' do + subject.stub(:current_user).and_return(assignee) + it_sends_a_reassigned_email_to previous_assignee.id + it_does_not_send_a_reassigned_email_to assignee.id + + subject.send(:send_reassigned_email, mr) + end + it 'if the user is the previous assignee' do + subject.stub(:current_user).and_return(previous_assignee) + it_sends_a_reassigned_email_to assignee.id + it_does_not_send_a_reassigned_email_to previous_assignee.id + + subject.send(:send_reassigned_email, mr) + end + end + end +end diff --git a/spec/observers/note_observer_spec.rb b/spec/observers/note_observer_spec.rb new file mode 100644 index 00000000000..fd21178918b --- /dev/null +++ b/spec/observers/note_observer_spec.rb @@ -0,0 +1,109 @@ +require 'spec_helper' + +describe NoteObserver do + subject { NoteObserver.instance } + + describe '#after_create' do + let(:note) { double :note, notify: false, notify_author: false } + + it 'is called after a note is created' do + subject.should_receive :after_create + + Note.observers.enable :note_observer do + Factory.create(:note) + end + end + + it 'notifies team of new note when flagged to notify' do + note.stub(:notify).and_return(true) + subject.should_receive(:notify_team_of_new_note).with(note) + + subject.after_create(note) + end + it 'does not notify team of new note when not flagged to notify' do + subject.should_not_receive(:notify_team_of_new_note).with(note) + + subject.after_create(note) + end + it 'notifies the author of a commit when flagged to notify the author' do + note.stub(:notify_author).and_return(true) + note.stub(:id).and_return(42) + author = double :user, id: 1 + note.stub(:commit_author).and_return(author) + Notify.should_receive(:note_commit_email).and_return(double(deliver: true)) + + subject.after_create(note) + end + it 'does not notify the author of a commit when not flagged to notify the author' do + Notify.should_not_receive(:note_commit_email) + + subject.after_create(note) + end + it 'does nothing if no notify flags are set' do + subject.after_create(note).should be_nil + end + end + + + let(:team_without_author) { (1..2).map { |n| double :user, id: n } } + + describe '#notify_team_of_new_note' do + let(:note) { double :note, id: 1 } + + before :each do + subject.stub(:team_without_note_author).with(note).and_return(team_without_author) + end + + context 'notifies team of a new note on' do + it 'a commit' do + note.stub(:noteable_type).and_return('Commit') + Notify.should_receive(:note_commit_email).twice.and_return(double(deliver: true)) + + subject.send(:notify_team_of_new_note, note) + end + it 'an issue' do + note.stub(:noteable_type).and_return('Issue') + Notify.should_receive(:note_issue_email).twice.and_return(double(deliver: true)) + + subject.send(:notify_team_of_new_note, note) + end + it 'a wiki page' do + note.stub(:noteable_type).and_return('Wiki') + Notify.should_receive(:note_wiki_email).twice.and_return(double(deliver: true)) + + subject.send(:notify_team_of_new_note, note) + end + it 'a merge request' do + note.stub(:noteable_type).and_return('MergeRequest') + Notify.should_receive(:note_merge_request_email).twice.and_return(double(deliver: true)) + + subject.send(:notify_team_of_new_note, note) + end + it 'a wall' do + note.stub(:noteable_type).and_return('Wall') + Notify.should_receive(:note_wall_email).twice.and_return(double(deliver: true)) + + subject.send(:notify_team_of_new_note, note) + end + end + + it 'does nothing for a new note on a snippet' do + note.stub(:noteable_type).and_return('Snippet') + + subject.send(:notify_team_of_new_note, note).should == [true, true] + end + end + + + describe '#team_without_note_author' do + let(:author) { double :user, id: 4 } + + let(:users) { team_without_author + [author] } + let(:project) { double :project, users: users } + let(:note) { double :note, project: project, author: author } + + it 'returns the projects user without the note author included' do + subject.send(:team_without_note_author, note).should == team_without_author + end + end +end |