rebuild notification on notes logic

app/services/notification_service.rb

@@ -99,22 +99,34 @@ class NotificationService
   # TODO: split on methods and refactor
   #
   def new_note(note)
-    if note.notify
-      users = note.project.users
-      users = reject_muted_users(users)
-      users.delete(note.author)
+    # ignore wall messages
+    return true unless note.noteable_type.present?
 
-      # Note: wall posts are not "attached" to anything, so fall back to "Wall"
-      noteable_type = note.noteable_type.presence || "Wall"
-      notify_method = "note_#{noteable_type.underscore}_email".to_sym
+    opts = { noteable_type: note.noteable_type, project_id: note.project_id }
 
-      if Notify.respond_to? notify_method
-        users.each do |user|
-          Notify.delay.send(notify_method, user.id, note.id)
-        end
-      end
-    elsif note.notify_author && note.commit_author
-      Notify.delay.note_commit_email(note.commit_author.id, note.id)
+    if note.commit_id
+      opts.merge!(commit_id: note.commit_id)
+    else
+      opts.merge!(noteable_id: note.noteable_id)
+    end
+
+    # Get users who left comment in thread
+    recipients = User.where(id: Note.where(opts).pluck(:author_id))
+
+    # Merge project watchers
+    recipients = recipients.concat(project_watchers(note.project)).compact.uniq
+
+    # Reject mutes users
+    recipients = reject_muted_users(recipients)
+
+    # Reject author
+    recipients.delete(note.author)
+
+    # build notify method like 'note_commit_email'
+    notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
+
+    recipients.each do |recipient|
+      Notify.delay.send(notify_method, recipient.id, note.id)
     end
   end
 

spec/services/notification_service_spec.rb

@@ -20,6 +20,45 @@ describe NotificationService do
     end
   end
 
+  describe 'Notes' do
+    let(:note) { create :note_on_commit }
+
+    before do
+      build_team(note.project)
+    end
+
+    describe :new_note do
+      it do
+        should_email(@u_watcher.id)
+        should_not_email(note.author_id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+        notification.new_note(note)
+      end
+
+      it do
+        create(:note_on_commit,
+               author: @u_participating,
+               project_id: note.project_id,
+               commit_id: note.commit_id)
+
+        should_email(@u_watcher.id)
+        should_email(@u_participating.id)
+        should_not_email(note.author_id)
+        should_not_email(@u_disabled.id)
+        notification.new_note(note)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:note_commit_email).with(user_id, note.id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:note_commit_email).with(user_id, note.id)
+      end
+    end
+  end
+
   describe 'Issues' do
     let(:issue) { create :issue, assignee: create(:user) }