Improve Comment's email logic (#699)

* Improve comment's email logic

* Change #activity_notify logic

* Update comment_spec

* Remove focus

* Update should_send_email_notification?

app/models/comment.rb

@@ -21,7 +21,7 @@ class Comment < ApplicationRecord
   after_save     :calculate_score
   after_save     :bust_cache
   before_destroy :before_destroy_actions
-  after_create   :send_email_notification
+  after_create   :send_email_notification, if: :should_send_email_notification?
   after_create   :create_first_reaction
   after_create   :send_to_moderator
   before_save    :set_markdown_character_count
@@ -70,7 +70,7 @@ class Comment < ApplicationRecord
           profile_pic: ProfileImage.new(user).get(90),
           profile_image_90: ProfileImage.new(user).get(90),
           github_username: user.github_username,
-          twitter_username: user.twitter_username,
+          twitter_username: user.twitter_username
         }
       end
       attribute :commentable do
@@ -78,7 +78,7 @@ class Comment < ApplicationRecord
           path: commentable&.path,
           title: commentable&.title,
           tag_list: commentable&.tag_list,
-          id: commentable&.id,
+          id: commentable&.id
         }
       end
       tags do
@@ -155,15 +155,9 @@ class Comment < ApplicationRecord
   # notifications
 
   def activity_notify
-    if ancestors.empty? && user != commentable.user
-      [StreamNotifier.new(commentable.user.id).notify]
-    elsif ancestors
-      # notify all ancestors unless it's yourself
-      user_ids = ancestors.map(&:user_id).uniq - [user_id]
-      user_ids.map do |id|
-        StreamNotifier.new(id).notify
-      end
-    end
+    user_ids = ancestors.map(&:user_id).to_set
+    user_ids.add(commentable.user.id) if user_ids.empty?
+    user_ids.delete(user_id).map { |id| StreamNotifier.new(id).notify }
   end
 
   def custom_css
@@ -227,7 +221,7 @@ class Comment < ApplicationRecord
         subject_name: commentable.title,
         user_image_link: user_image_link,
         background_color: user.bg_color_hex,
-        text_color: user.text_color_hex,
+        text_color: user.text_color_hex
       },
     )
   end
@@ -327,13 +321,15 @@ class Comment < ApplicationRecord
   handle_asynchronously :async_bust
 
   def send_email_notification
-    NotifyMailer.new_reply_email(self).deliver if parent_email_exist?
+    NotifyMailer.new_reply_email(self).deliver
   end
   handle_asynchronously :send_email_notification
 
-  def parent_email_exist?
-    parent_user && parent_user.email.present? &&
-      parent_user.email_comment_notifications
+  def should_send_email_notification?
+    parent_user.class.name != "Podcast" &&
+      parent_user != user &&
+      parent_user.email_comment_notifications &&
+      parent_user.email
   end
 
   def strip_url(url)

spec/models/comment_spec.rb

@@ -207,29 +207,33 @@ RSpec.describe Comment, type: :model do
   end
 
   describe "::StreamRails::Activity(notification callbacks)" do
+    before do
+      StreamRails.enabled = true
+      allow(StreamNotifier).to receive(:new).and_call_original
+    end
+
     after { StreamRails.enabled = false }
 
     context "when a comment without ancestor is created" do
-      before do
-        StreamRails.enabled = true
-        allow(StreamNotifier).to receive(:new).and_call_original
-      end
+      let(:test_comment) { build(:comment, commentable_id: article.id) }
+
+      before { allow(test_comment).to receive(:send_email_notification).and_call_original }
 
       it "notifies the author" do
-        create(:comment, commentable_id: article.id)
-        expect(StreamNotifier).to have_received(:new).with(user.id).at_least(:once)
+        test_comment.save!
+        expect(StreamNotifier).to have_received(:new).at_least(:once)
       end
 
-      it " does not notify author if self is author" do
-        create(:comment, commentable_id: article.id, user_id: user.id)
+      it "does not notify author if self is author" do
+        test_comment.user = user
+        test_comment.save
         expect(StreamNotifier).not_to have_received(:new).with(user.id)
+        expect(test_comment).not_to have_received :send_email_notification
       end
 
       it "does not notify anybody else" do
-        comment1 = create(:comment, commentable_id: article.id)
-        comment2 = create(:comment, commentable_id: article.id)
-        expect(StreamNotifier).not_to have_received(:new).with(comment1.user.id)
-        expect(StreamNotifier).not_to have_received(:new).with(comment2.user.id)
+        test_comment.save
+        expect(StreamNotifier).not_to have_received(:new).with(test_comment.user_id)
       end
     end
 
@@ -245,9 +249,9 @@ RSpec.describe Comment, type: :model do
       let(:nest_comments_authors) { [nest_comment_1.user_id, nest_comment_2.user_id] }
 
       before do
+        StreamRails.enabled = false
         nest_comments_authors # this needs to be evoked before StreamRails is enabled
         StreamRails.enabled = true
-        allow(StreamNotifier).to receive(:new).and_call_original
       end
 
       it "notifies all the ancestors" do
@@ -271,9 +275,11 @@ RSpec.describe Comment, type: :model do
 
       it "does not notify self if self is among the ancestors" do
         me = create(:user)
+        test_comment = create(:comment, parent_id: author_comment.id, commentable_id: article.id, user_id: me.id)
         create(:comment, parent_id: author_comment.id, commentable_id: article.id, user_id: me.id)
-        create(:comment, parent_id: author_comment.id, commentable_id: article.id, user_id: me.id)
+        allow(test_comment).to receive(:send_email_notification).and_call_original
         expect(StreamNotifier).not_to have_received(:new).with(me.id)
+        expect(test_comment).not_to have_received(:send_email_notification)
       end
     end
   end