From 16ceae895e32c2474de04c42307d914bf9a4c304 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Tue, 9 Oct 2012 18:25:29 -0400 Subject: [PATCH] Separate observing of Note and MergeRequests * Move is_assigned? and is_being_xx? methods to IssueCommonality This is behavior merge requests have in common with issues. Moved methods to IssueCommonality role. Put specs directly into merge_request_spec because setup differs for issues and MRs specifically in the "closed" factory to use. * Add MergeRequestObserver. Parallels IssueObserver in almost every way. Ripe for refactoring. * Rename MailerObserver to NoteObserver With merge request observing moved out of MailerObserver, all that was left was Note logic. Renamed to NoteObserver, added tests and updated application config for new observer names. Refactored NoteObserver to use the note's author and not rely on current_user. * Set current_user for MergeRequestObserver IssueObserver and MergeRequestObserver are the only observers that need a reference to the current_user that they cannot look up on the objects they are observing. --- app/controllers/application_controller.rb | 6 +- app/models/issue.rb | 16 --- app/observers/mailer_observer.rb | 80 ----------- app/observers/merge_request_observer.rb | 31 +++++ app/observers/note_observer.rb | 36 +++++ app/roles/issue_commonality.rb | 17 +++ config/application.rb | 10 +- spec/factories.rb | 6 + spec/models/merge_request_spec.rb | 43 ++++++ spec/observers/merge_request_observer_spec.rb | 193 ++++++++++++++++++++++++++ spec/observers/note_observer_spec.rb | 109 +++++++++++++++ 11 files changed, 445 insertions(+), 102 deletions(-) delete mode 100644 app/observers/mailer_observer.rb create mode 100644 app/observers/merge_request_observer.rb create mode 100644 app/observers/note_observer.rb create mode 100644 spec/observers/merge_request_observer_spec.rb create mode 100644 spec/observers/note_observer_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c4c99204e..b597795a5 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 5450bb499..3dd1c8c89 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 331aaa35e..000000000 --- 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 000000000..9090794de --- /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 000000000..844848567 --- /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 3d9074867..2d10bfec8 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 1189035fa..74ed330ef 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 cb3541cc7..0258f8920 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 91d404f79..be40c561a 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 000000000..a9ba79271 --- /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 000000000..fd2117891 --- /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 -- 2.11.0