Merge pull request #455 from alvarobp/comment-mail-with-one-user-complete

Commenting on an error when there is only one user raises an exception

Merge pull request #455 from alvarobp/comment-mail-with-one-user-complete
Commenting on an error when there is only one user raises an exception
Sergey Nartimov
2 parents df2ee897 d8a4e4b4
Showing 7 changed files with 132 additions and 6 deletions Show diff stats
app/mailers/mailer.rb
app/models/comment.rb
app/models/comment_observer.rb
spec/mailers/mailer_spec.rb
spec/models/app_spec.rb
spec/models/comment_observer_spec.rb
spec/models/comment_spec.rb
@@ -34,8 +34,7 @@ class Mailer &lt; ActionMailer::Base
     @notice   = @problem.notices.first
     @app      = @problem.app
-    # Don't send comment notification to user who posted the comment
-    recipients = @app.notification_recipients - [comment.user.email]
+    recipients = @comment.notification_recipients
     mail :to      => recipients,
          :subject => "#{@user.name} commented on [#{@app.name}][#{@notice.environment_name}] #{@notice.message.truncate(50)}"
@@ -14,6 +14,14 @@ class Comment
   validates_presence_of :body
+  def notification_recipients
+    app.notification_recipients - [user.email]
+  end
+
+  def emailable?
+    app.emailable? && notification_recipients.any?
+  end
+
   protected
     def increase_counter_cache
       err.inc(:comments_count, 1)
@@ -24,4 +32,3 @@ class Comment
     end
 end
-
@@ -2,7 +2,7 @@ class CommentObserver &lt; Mongoid::Observer
   observe :comment
   def after_create(comment)
-    Mailer.comment_notification(comment).deliver if comment.app.emailable?
+    Mailer.comment_notification(comment).deliver if comment.emailable?
   end
 end
@@ -46,5 +46,36 @@ describe Mailer do
       end
     end
   end
-end
+  context "Comment Notification" do
+    include EmailSpec::Helpers
+    include EmailSpec::Matchers
+
+    let!(:notice) { Fabricate(:notice) }
+    let!(:comment) { Fabricate.build(:comment, :err => notice.problem) }
+    let!(:watcher) { Fabricate(:watcher, :app => comment.app) }
+    let(:recipients) { ['recipient@example.com', 'another@example.com']}
+
+    before do
+      comment.stub(:notification_recipients).and_return(recipients)
+      Fabricate(:notice, :err => notice.err)
+      @email = Mailer.comment_notification(comment).deliver
+    end
+
+    it "should send the email" do
+      ActionMailer::Base.deliveries.size.should == 1
+    end
+
+    it "should be sent to comment notification recipients" do
+      @email.to.should == recipients
+    end
+
+    it "should have the notices count in the body" do
+      @email.should have_body_text("This err has occurred 2 times")
+    end
+
+    it "should have the comment body" do
+      @email.should have_body_text(comment.body)
+    end
+  end
+end
@@ -124,6 +124,25 @@ describe App do
     end
   end
+  context "emailable?" do
+    it "should be true if notify on errs and there are notification recipients" do
+      app = Fabricate(:app, :notify_on_errs => true, :notify_all_users => false)
+      2.times { Fabricate(:watcher, :app => app) }
+      app.emailable?.should be_true
+    end
+
+    it "should be false if notify on errs is disabled" do
+      app = Fabricate(:app, :notify_on_errs => false, :notify_all_users => false)
+      2.times { Fabricate(:watcher, :app => app) }
+      app.emailable?.should be_false
+    end
+
+    it "should be false if there are no notification recipients" do
+      app = Fabricate(:app, :notify_on_errs => true, :notify_all_users => false)
+      app.watchers.should be_empty
+      app.emailable?.should be_false
+    end
+  end
   context "copying attributes from existing app" do
     it "should only copy the necessary fields" do
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe CommentObserver do
+  context 'when a Comment is saved' do
+    let(:comment) { Fabricate.build(:comment) }
+
+    context 'and it is emailable?' do
+      before { comment.stub(:emailable?).and_return(true) }
+
+      it 'should send an email notification' do
+        Mailer.should_receive(:comment_notification).
+          with(comment).
+          and_return(mock('email', :deliver => true))
+        comment.save
+      end
+    end
+
+    context 'and it is not emailable?' do
+      before { comment.stub(:emailable?).and_return(false) }
+
+      it 'should not send an email notification' do
+        Mailer.should_not_receive(:comment_notification)
+        comment.save
+      end
+    end
+  end
+end
@@ -8,5 +8,48 @@ describe Comment do
       comment.errors[:body].should include("can't be blank")
     end
   end
-end
+  context 'notification_recipients' do
+    let(:app) { Fabricate(:app) }
+    let!(:watcher) { Fabricate(:watcher, :app => app) }
+    let(:err) { Fabricate(:problem, :app => app) }
+    let(:comment_user) { Fabricate(:user, :email => 'author@example.com') }
+    let(:comment) { Fabricate.build(:comment, :err => err, :user => comment_user) }
+
+    before do
+      Fabricate(:user_watcher, :app => app, :user => comment_user)
+    end
+
+    it 'includes app notification_recipients except user email' do
+      comment.notification_recipients.should == [watcher.address]
+    end
+  end
+
+  context 'emailable?' do
+    let(:app) { Fabricate(:app, :notify_on_errs => true) }
+    let!(:watcher) { Fabricate(:watcher, :app => app) }
+    let(:err) { Fabricate(:problem, :app => app) }
+    let(:comment_user) { Fabricate(:user, :email => 'author@example.com') }
+    let(:comment) { Fabricate.build(:comment, :err => err, :user => comment_user) }
+
+    before do
+      Fabricate(:user_watcher, :app => app, :user => comment_user)
+    end
+
+    it 'should be true if app is emailable? and there are notification recipients' do
+      comment.emailable?.should be_true
+    end
+
+    it 'should be false if app is not emailable?' do
+      app.update_attribute(:notify_on_errs, false)
+      comment.notification_recipients.should be_any
+      comment.emailable?.should be_false
+    end
+
+    it 'should be false if there are no notification recipients' do
+      watcher.destroy
+      app.emailable?.should be_true
+      comment.emailable?.should be_false
+    end
+  end
+end
	@@ -34,8 +34,7 @@ class Mailer < ActionMailer::Base		@@ -34,8 +34,7 @@ class Mailer < ActionMailer::Base
34	@notice = @problem.notices.first	34	@notice = @problem.notices.first
35	@app = @problem.app	35	@app = @problem.app
36		36
37	- # Don't send comment notification to user who posted the comment
38	- recipients = @app.notification_recipients - [comment.user.email]	37	+ recipients = @comment.notification_recipients
39		38
40	mail :to => recipients,	39	mail :to => recipients,
41	:subject => "#{@user.name} commented on [#{@app.name}][#{@notice.environment_name}] #{@notice.message.truncate(50)}"	40	:subject => "#{@user.name} commented on [#{@app.name}][#{@notice.environment_name}] #{@notice.message.truncate(50)}"
	@@ -2,7 +2,7 @@ class CommentObserver < Mongoid::Observer		@@ -2,7 +2,7 @@ class CommentObserver < Mongoid::Observer
2	observe :comment	2	observe :comment
3		3
4	def after_create(comment)	4	def after_create(comment)
5	- Mailer.comment_notification(comment).deliver if comment.app.emailable?	5	+ Mailer.comment_notification(comment).deliver if comment.emailable?
6	end	6	end
7		7
8	end	8	end
	@@ -46,5 +46,36 @@ describe Mailer do		@@ -46,5 +46,36 @@ describe Mailer do
46	end	46	end
47	end	47	end
48	end	48	end
49	-end
50		49
		50	+ context "Comment Notification" do
		51	+ include EmailSpec::Helpers
		52	+ include EmailSpec::Matchers
		53	+
		54	+ let!(:notice) { Fabricate(:notice) }
		55	+ let!(:comment) { Fabricate.build(:comment, :err => notice.problem) }
		56	+ let!(:watcher) { Fabricate(:watcher, :app => comment.app) }
		57	+ let(:recipients) { ['recipient@example.com', 'another@example.com']}
		58	+
		59	+ before do
		60	+ comment.stub(:notification_recipients).and_return(recipients)
		61	+ Fabricate(:notice, :err => notice.err)
		62	+ @email = Mailer.comment_notification(comment).deliver
		63	+ end
		64	+
		65	+ it "should send the email" do
		66	+ ActionMailer::Base.deliveries.size.should == 1
		67	+ end
		68	+
		69	+ it "should be sent to comment notification recipients" do
		70	+ @email.to.should == recipients
		71	+ end
		72	+
		73	+ it "should have the notices count in the body" do
		74	+ @email.should have_body_text("This err has occurred 2 times")
		75	+ end
		76	+
		77	+ it "should have the comment body" do
		78	+ @email.should have_body_text(comment.body)
		79	+ end
		80	+ end
		81	+end
	@@ -124,6 +124,25 @@ describe App do		@@ -124,6 +124,25 @@ describe App do
124	end	124	end
125	end	125	end
126		126
		127	+ context "emailable?" do
		128	+ it "should be true if notify on errs and there are notification recipients" do
		129	+ app = Fabricate(:app, :notify_on_errs => true, :notify_all_users => false)
		130	+ 2.times { Fabricate(:watcher, :app => app) }
		131	+ app.emailable?.should be_true
		132	+ end
		133	+
		134	+ it "should be false if notify on errs is disabled" do
		135	+ app = Fabricate(:app, :notify_on_errs => false, :notify_all_users => false)
		136	+ 2.times { Fabricate(:watcher, :app => app) }
		137	+ app.emailable?.should be_false
		138	+ end
		139	+
		140	+ it "should be false if there are no notification recipients" do
		141	+ app = Fabricate(:app, :notify_on_errs => true, :notify_all_users => false)
		142	+ app.watchers.should be_empty
		143	+ app.emailable?.should be_false
		144	+ end
		145	+ end
127		146
128	context "copying attributes from existing app" do	147	context "copying attributes from existing app" do
129	it "should only copy the necessary fields" do	148	it "should only copy the necessary fields" do
@@ -0,0 +1,27 @@		@@ -0,0 +1,27 @@
	1	+require 'spec_helper'
	2	+
	3	+describe CommentObserver do
	4	+ context 'when a Comment is saved' do
	5	+ let(:comment) { Fabricate.build(:comment) }
	6	+
	7	+ context 'and it is emailable?' do
	8	+ before { comment.stub(:emailable?).and_return(true) }
	9	+
	10	+ it 'should send an email notification' do
	11	+ Mailer.should_receive(:comment_notification).
	12	+ with(comment).
	13	+ and_return(mock('email', :deliver => true))
	14	+ comment.save
	15	+ end
	16	+ end
	17	+
	18	+ context 'and it is not emailable?' do
	19	+ before { comment.stub(:emailable?).and_return(false) }
	20	+
	21	+ it 'should not send an email notification' do
	22	+ Mailer.should_not_receive(:comment_notification)
	23	+ comment.save
	24	+ end
	25	+ end
	26	+ end
	27	+end
	@@ -8,5 +8,48 @@ describe Comment do		@@ -8,5 +8,48 @@ describe Comment do
8	comment.errors[:body].should include("can't be blank")	8	comment.errors[:body].should include("can't be blank")
9	end	9	end
10	end	10	end
11	-end
12		11
		12	+ context 'notification_recipients' do
		13	+ let(:app) { Fabricate(:app) }
		14	+ let!(:watcher) { Fabricate(:watcher, :app => app) }
		15	+ let(:err) { Fabricate(:problem, :app => app) }
		16	+ let(:comment_user) { Fabricate(:user, :email => 'author@example.com') }
		17	+ let(:comment) { Fabricate.build(:comment, :err => err, :user => comment_user) }
		18	+
		19	+ before do
		20	+ Fabricate(:user_watcher, :app => app, :user => comment_user)
		21	+ end
		22	+
		23	+ it 'includes app notification_recipients except user email' do
		24	+ comment.notification_recipients.should == [watcher.address]
		25	+ end
		26	+ end
		27	+
		28	+ context 'emailable?' do
		29	+ let(:app) { Fabricate(:app, :notify_on_errs => true) }
		30	+ let!(:watcher) { Fabricate(:watcher, :app => app) }
		31	+ let(:err) { Fabricate(:problem, :app => app) }
		32	+ let(:comment_user) { Fabricate(:user, :email => 'author@example.com') }
		33	+ let(:comment) { Fabricate.build(:comment, :err => err, :user => comment_user) }
		34	+
		35	+ before do
		36	+ Fabricate(:user_watcher, :app => app, :user => comment_user)
		37	+ end
		38	+
		39	+ it 'should be true if app is emailable? and there are notification recipients' do
		40	+ comment.emailable?.should be_true
		41	+ end
		42	+
		43	+ it 'should be false if app is not emailable?' do
		44	+ app.update_attribute(:notify_on_errs, false)
		45	+ comment.notification_recipients.should be_any
		46	+ comment.emailable?.should be_false
		47	+ end
		48	+
		49	+ it 'should be false if there are no notification recipients' do
		50	+ watcher.destroy
		51	+ app.emailable?.should be_true
		52	+ comment.emailable?.should be_false
		53	+ end
		54	+ end
		55	+end