Merge pull request #1695 from riyad/cleanup-note-observer-and-notifications

Code cleanup on NoteObserver and Notify

3 files changed, 117 insertions, 62 deletions

diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb
index 9e64b64a86e..7f3862adb1d 100644
--- a/app/mailers/notify.rb
+++ b/app/mailers/notify.rb
@@ -9,11 +9,11 @@ class Notify < ActionMailer::Base
 
   default from: Gitlab.config.email_from
 
-  def new_user_email(user_id, password)
-    @user = User.find(user_id)
-    @password = password
-    mail(to: @user.email, subject: subject("Account was created for you"))
-  end
+
+
+  #
+  # Issue
+  #
 
   def new_issue_email(issue_id)
     @issue = Issue.find(issue_id)
@@ -21,12 +21,46 @@ class Notify < ActionMailer::Base
     mail(to: @issue.assignee_email, subject: subject("new issue ##{@issue.id}", @issue.title))
   end
 
-  def note_wall_email(recipient_id, note_id)
-    @note = Note.find(note_id)
-    @project = @note.project
-    mail(to: recipient(recipient_id), subject: subject)
+  def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id)
+    @issue = Issue.find(issue_id)
+    @previous_assignee ||= User.find(previous_assignee_id)
+    @project = @issue.project
+    mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title))
+  end
+
+  def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
+    @issue = Issue.find issue_id
+    @issue_status = status
+    @updated_by = User.find updated_by_user_id
+    mail(to: recipient(recipient_id),
+        subject: subject("changed issue ##{@issue.id}", @issue.title))
+  end
+
+
+
+  #
+  # Merge Request
+  #
+
+  def new_merge_request_email(merge_request_id)
+    @merge_request = MergeRequest.find(merge_request_id)
+    @project = @merge_request.project
+    mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
+  end
+
+  def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
+    @merge_request = MergeRequest.find(merge_request_id)
+    @previous_assignee ||= User.find(previous_assignee_id)
+    @project = @merge_request.project
+    mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
   end
 
+
+
+  #
+  # Note
+  #
+
   def note_commit_email(recipient_id, note_id)
     @note = Note.find(note_id)
     @commit = @note.noteable
@@ -35,6 +69,13 @@ class Notify < ActionMailer::Base
     mail(to: recipient(recipient_id), subject: subject("note for commit #{@commit.short_id}", @commit.title))
   end
 
+  def note_issue_email(recipient_id, note_id)
+    @note = Note.find(note_id)
+    @issue = @note.noteable
+    @project = @note.project
+    mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}"))
+  end
+
   def note_merge_request_email(recipient_id, note_id)
     @note = Note.find(note_id)
     @merge_request = @note.noteable
@@ -42,11 +83,10 @@ class Notify < ActionMailer::Base
     mail(to: recipient(recipient_id), subject: subject("note for merge request !#{@merge_request.id}"))
   end
 
-  def note_issue_email(recipient_id, note_id)
+  def note_wall_email(recipient_id, note_id)
     @note = Note.find(note_id)
-    @issue = @note.noteable
     @project = @note.project
-    mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}"))
+    mail(to: recipient(recipient_id), subject: subject)
   end
 
   def note_wiki_email(recipient_id, note_id)
@@ -56,25 +96,11 @@ class Notify < ActionMailer::Base
     mail(to: recipient(recipient_id), subject: subject("note for wiki"))
   end
 
-  def new_merge_request_email(merge_request_id)
-    @merge_request = MergeRequest.find(merge_request_id)
-    @project = @merge_request.project
-    mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
-  end
 
-  def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
-    @merge_request = MergeRequest.find(merge_request_id)
-    @previous_assignee ||= User.find(previous_assignee_id)
-    @project = @merge_request.project
-    mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
-  end
 
-  def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id)
-    @issue = Issue.find(issue_id)
-    @previous_assignee ||= User.find(previous_assignee_id)
-    @project = @issue.project
-    mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title))
-  end
+  #
+  # Project
+  #
 
   def project_access_granted_email(user_project_id)
     @users_project = UsersProject.find user_project_id
@@ -83,14 +109,19 @@ class Notify < ActionMailer::Base
          subject: subject("access to project was granted"))
   end
 
-  def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
-    @issue = Issue.find issue_id
-    @issue_status = status
-    @updated_by = User.find updated_by_user_id
-    mail(to: recipient(recipient_id),
-        subject: subject("changed issue ##{@issue.id}", @issue.title))
+
+
+  #
+  # User
+  #
+
+  def new_user_email(user_id, password)
+    @user = User.find(user_id)
+    @password = password
+    mail(to: @user.email, subject: subject("Account was created for you"))
   end
 
+
   private
 
   # Look up a User by their ID and return their email address
diff --git a/app/observers/note_observer.rb b/app/observers/note_observer.rb
index d328338a706..083aa7058d5 100644
--- a/app/observers/note_observer.rb
+++ b/app/observers/note_observer.rb
@@ -1,9 +1,14 @@
 class NoteObserver < ActiveRecord::Observer
 
   def after_create(note)
+    send_notify_mails(note)
+  end
+
+  protected
+
+  def send_notify_mails(note)
     if note.notify
-      # Notify whole team except author of note
-      notify_team_of_new_note(note)
+      notify_team(note)
     elsif note.notify_author
       # Notify only author of resource
       Notify.note_commit_email(note.commit_author.id, note.id).deliver
@@ -13,15 +18,15 @@ class NoteObserver < ActiveRecord::Observer
     end
   end
 
-  protected
-
-  def notify_team_of_new_note(note)
-    note_is_on = note.noteable_type || 'Wall'
-    notify_method = 'note_' + note_is_on.underscore + '_email'
+  # Notifies the whole team except the author of note
+  def notify_team(note)
+    # Note: wall posts are not "attached" to anything, so fall back to "Wall"
+    noteable_type = note.noteable_type || "Wall"
+    notify_method = "note_#{noteable_type.underscore}_email".to_sym
 
     if Notify.respond_to? notify_method
       team_without_note_author(note).map do |u|
-        Notify.send(notify_method.to_sym, u.id, note.id).deliver
+        Notify.send(notify_method, u.id, note.id).deliver
       end
     end
   end
diff --git a/spec/observers/note_observer_spec.rb b/spec/observers/note_observer_spec.rb
index fb5a35447d3..203a58a4950 100644
--- a/spec/observers/note_observer_spec.rb
+++ b/spec/observers/note_observer_spec.rb
@@ -3,8 +3,11 @@ require 'spec_helper'
 describe NoteObserver do
   subject { NoteObserver.instance }
 
+  let(:team_without_author) { (1..2).map { |n| double :user, id: n } }
+  let(:delivery_success) { double deliver: true }
+
   describe '#after_create' do
-    let(:note) { double :note, notify: false, notify_author: false }
+    let(:note) { double :note }
 
     it 'is called after a note is created' do
       subject.should_receive :after_create
@@ -14,40 +17,51 @@ describe NoteObserver do
       end
     end
 
+    it 'sends out notifications' do
+      subject.should_receive(:send_notify_mails).with(note)
+
+      subject.after_create(note)
+    end
+  end
+
+  describe "#send_notify_mails" do
+    let(:note) { double :note, notify: false, notify_author: false }
+
     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.should_receive(:notify_team).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.should_not_receive(:notify_team).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))
+      Notify.should_receive(:note_commit_email).and_return(delivery_success)
 
       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
+  describe '#notify_team' do
     let(:note) { double :note, id: 1 }
 
     before :each do
@@ -57,40 +71,45 @@ describe NoteObserver do
     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))
+        Notify.should_receive(:note_commit_email).twice.and_return(delivery_success)
 
-        subject.send(:notify_team_of_new_note, note)
+        subject.send(:notify_team, 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))
+        Notify.should_receive(:note_issue_email).twice.and_return(delivery_success)
 
-        subject.send(:notify_team_of_new_note, note)
+        subject.send(:notify_team, 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))
+        Notify.should_receive(:note_wiki_email).twice.and_return(delivery_success)
 
-        subject.send(:notify_team_of_new_note, note)
+        subject.send(:notify_team, 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))
+        Notify.should_receive(:note_merge_request_email).twice.and_return(delivery_success)
 
-        subject.send(:notify_team_of_new_note, note)
+        subject.send(:notify_team, note)
       end
+
       it 'a wall' do
+        # Note: wall posts have #noteable_type of nil
         note.stub(:noteable_type).and_return(nil)
-        Notify.should_receive(:note_wall_email).twice.and_return(double(deliver: true))
+        Notify.should_receive(:note_wall_email).twice.and_return(delivery_success)
 
-        subject.send(:notify_team_of_new_note, note)
+        subject.send(:notify_team, 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 be_nil
+        subject.send(:notify_team, note).should be_nil
     end
   end