Merge pull request #1675 from robbkidd/separate_mr_observer

Separate observing of Note and MergeRequests

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
 

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

app/observers/mailer_observer.rb

@@ -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

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

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

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

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.

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

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

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

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