Merge pull request #460 from dgilperez/improve_comment_email_delivery_logic

Prevents users to receive email notifications about own comments and replies

app/controllers/comments_controller.rb

@@ -8,8 +8,7 @@ class CommentsController < ApplicationController
 
   def create
     if @comment.save
-      Mailer.comment(@comment).deliver_later if email_on_debate_comment?
+      CommentNotifier.new(comment: @comment).process
-      Mailer.reply(@comment).deliver_later if email_on_comment_reply?
     else
       render :new
     end
@@ -55,14 +54,6 @@ class CommentsController < ApplicationController
       @commentable = Comment.find_commentable(comment_params[:commentable_type], comment_params[:commentable_id])
     end
 
-    def email_on_debate_comment?
-      @comment.commentable.author.email_on_debate_comment?
-    end
-
-    def email_on_comment_reply?
-      @comment.reply? && @comment.parent.author.email_on_comment_reply?
-    end
-
     def administrator_comment?
       ["1", true].include?(comment_params[:as_administrator]) && can?(:comment_as_administrator, Debate)
     end

app/models/comment.rb

@@ -22,7 +22,7 @@ class Comment < ActiveRecord::Base
 
   scope :sort_for_moderation, -> { order(flags_count: :desc, updated_at: :desc) }
   scope :pending_flag_review, -> { where(ignored_flag_at: nil, hidden_at: nil) }
-  scope :with_ignored_flag, -> { where("ignored_flag_at IS NOT NULL AND hidden_at IS NULL") }
+  scope :with_ignored_flag, -> { where(hidden_at: nil).where.not(ignored_flag_at: nil) }
   scope :flagged, -> { where("flags_count > 0") }
 
   scope :for_render, -> { with_hidden.includes(user: :organization) }

app/models/comment_notifier.rb

@@ -0,0 +1,33 @@
+class CommentNotifier
+  def initialize(args = {})
+    @comment = args.fetch(:comment)
+    @author  = @comment.author
+  end
+
+  def process
+    send_comment_email
+    send_reply_email
+  end
+
+  private
+
+  def send_comment_email
+    Mailer.comment(@comment).deliver_later if email_on_debate_comment?
+  end
+
+  def send_reply_email
+    Mailer.reply(@comment).deliver_later if email_on_comment_reply?
+  end
+
+  def email_on_debate_comment?
+    commentable_author = @comment.commentable.author
+    commentable_author != @author && commentable_author.email_on_debate_comment?
+  end
+
+  def email_on_comment_reply?
+    return false unless @comment.reply?
+    parent_author = @comment.parent.author
+    parent_author != @author && parent_author.email_on_comment_reply?
+  end
+
+end

app/models/debate.rb

@@ -27,7 +27,7 @@ class Debate < ActiveRecord::Base
 
   scope :sort_for_moderation, -> { order(flags_count: :desc, updated_at: :desc) }
   scope :pending_flag_review, -> { where(ignored_flag_at: nil, hidden_at: nil) }
-  scope :with_ignored_flag, -> { where("ignored_flag_at IS NOT NULL AND hidden_at IS NULL") }
+  scope :with_ignored_flag, -> { where.not(ignored_flag_at: nil).where(hidden_at: nil) }
   scope :flagged, -> { where("flags_count > 0") }
   scope :for_render, -> { includes(:tags) }
   scope :sort_by_hot_score , -> { order(hot_score: :desc) }

app/models/organization.rb

@@ -7,9 +7,9 @@ class Organization < ActiveRecord::Base
 
   delegate :email, :phone_number, to: :user
 
-  scope :pending, -> { where('organizations.verified_at is null and rejected_at is null') }
+  scope :pending, -> { where(verified_at: nil, rejected_at: nil) }
-  scope :verified, -> { where("organizations.verified_at is not null and (rejected_at is null or rejected_at < organizations.verified_at)") }
+  scope :verified, -> { where.not(verified_at: nil).where("(rejected_at IS NULL or rejected_at < organizations.verified_at)") }
-  scope :rejected, -> { where("rejected_at is not null and (organizations.verified_at is null or organizations.verified_at < rejected_at)") }
+  scope :rejected, -> { where.not(rejected_at: nil).where("(organizations.verified_at IS NULL or organizations.verified_at < rejected_at)") }
 
   def verify
     update(verified_at: Time.now)

lib/acts_as_paranoid_aliases.rb

@@ -31,11 +31,11 @@ module ActsAsParanoidAliases
 
   module ClassMethods
     def with_confirmed_hide
-      where("confirmed_hide_at IS NOT NULL")
+      where.not(confirmed_hide_at: nil)
     end
 
     def without_confirmed_hide
-      where("confirmed_hide_at IS NULL")
+      where(confirmed_hide_at: nil)
     end
 
     def with_hidden
@@ -57,4 +57,3 @@ module ActsAsParanoidAliases
     end
   end
 end
-

spec/features/emails_spec.rb

@@ -24,40 +24,59 @@ feature 'Emails' do
     expect(email).to have_body_text(edit_user_password_path)
   end
 
-  scenario "Debate comment", :js do
+  context 'Debate comments' do
-    user = create(:user, email_on_debate_comment: true)
+    scenario "Send email on debate comment", :js do
-    debate = create(:debate, author: user)
+      user = create(:user, email_on_debate_comment: true)
-    comment_on(debate)
+      debate = create(:debate, author: user)
+      comment_on(debate)
 
-    email = open_last_email
+      email = open_last_email
-    expect(email).to have_subject('Someone has commented on your debate')
+      expect(email).to have_subject('Someone has commented on your debate')
-    expect(email).to deliver_to(debate.author)
+      expect(email).to deliver_to(debate.author)
-    expect(email).to have_body_text(debate_path(debate))
+      expect(email).to have_body_text(debate_path(debate))
+    end
+
+    scenario 'Do not send email about own debate comments', :js do
+      user = create(:user, email_on_debate_comment: true)
+      debate = create(:debate, author: user)
+      comment_on(debate, user)
+
+      expect { open_last_email }.to raise_error "No email has been sent!"
+    end
+
+    scenario 'Do not send email about debate comment unless set in preferences', :js do
+      user = create(:user, email_on_debate_comment: false)
+      debate = create(:debate, author: user)
+      comment_on(debate)
+
+      expect { open_last_email }.to raise_error "No email has been sent!"
+    end
   end
 
-  scenario "Comment reply", :js do
+  context 'Comment replies' do
-    user = create(:user, email_on_comment_reply: true)
+    scenario "Send email on comment reply", :js do
-    reply_to(user)
+      user = create(:user, email_on_comment_reply: true)
+      reply_to(user)
 
-    email = open_last_email
+      email = open_last_email
-    expect(email).to have_subject('Someone has replied to your comment')
+      expect(email).to have_subject('Someone has replied to your comment')
-    expect(email).to deliver_to(user)
+      expect(email).to deliver_to(user)
-    expect(email).to have_body_text(debate_path(Comment.first.debate))
+      expect(email).to have_body_text(debate_path(Comment.first.debate))
+    end
+
+    scenario "Do not send email about own replies to own comments", :js do
+      user = create(:user, email_on_comment_reply: true)
+      reply_to(user, user)
+
+      expect { open_last_email }.to raise_error "No email has been sent!"
+    end
+
+    scenario "Do not send email about comment reply unless set in preferences", :js do
+      user = create(:user, email_on_comment_reply: false)
+      reply_to(user)
+
+      expect { open_last_email }.to raise_error "No email has been sent!"
+    end
   end
 
-  scenario 'Do not send email about debate comment unless set in preferences', :js do
+end
-    user = create(:user, email_on_debate_comment: false)
-    debate = create(:debate, author: user)
-    comment_on(debate)
-
-    expect { open_last_email }.to raise_error "No email has been sent!"
-  end
-
-  scenario "Do not send email about comment reply unless set in preferences", :js do
-    user = create(:user, email_on_comment_reply: false)
-    reply_to(user)
-
-    expect { open_last_email }.to raise_error "No email has been sent!"
-  end
-
-end

spec/support/common_actions.rb

@@ -44,8 +44,8 @@ module CommonActions
     click_button 'Send me reset password instructions'
   end
 
-  def comment_on(debate)
+  def comment_on(debate, user = nil)
-    user = create(:user)
+    user ||= create(:user)
 
     login_as(user)
     visit debate_path(debate)
@@ -56,10 +56,11 @@ module CommonActions
     expect(page).to have_content 'Have you thought about...?'
   end
 
-  def reply_to(user)
+  def reply_to(original_user, manuela = nil)
-    manuela = create(:user)
+    manuela ||= create(:user)
+
     debate  = create(:debate)
-    comment = create(:comment, commentable: debate, user: user)
+    comment = create(:comment, commentable: debate, user: original_user)
 
     login_as(manuela)
     visit debate_path(debate)