From 8ed434cc8b5f3b13cdd10cfaee21e417640329e3 Mon Sep 17 00:00:00 2001 From: David Gil Date: Thu, 10 Sep 2015 13:53:47 +0200 Subject: [PATCH 1/4] uses more rails-like syntax in scopes --- app/models/comment.rb | 2 +- app/models/debate.rb | 2 +- app/models/organization.rb | 6 +++--- lib/acts_as_paranoid_aliases.rb | 5 ++--- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 493dbeb1c..73bd0b732 100644 --- a/app/models/comment.rb +++ b/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) } diff --git a/app/models/debate.rb b/app/models/debate.rb index fdf9cb6b9..0347f0edd 100644 --- a/app/models/debate.rb +++ b/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) } diff --git a/app/models/organization.rb b/app/models/organization.rb index dc340b08c..fc9b8cec1 100644 --- a/app/models/organization.rb +++ b/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 :verified, -> { where("organizations.verified_at is not null and (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 :pending, -> { where(verified_at: nil, rejected_at: nil) } + scope :verified, -> { where.not(verified_at: nil).where("(rejected_at IS NULL or rejected_at < organizations.verified_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) diff --git a/lib/acts_as_paranoid_aliases.rb b/lib/acts_as_paranoid_aliases.rb index e8abab530..6fd875ef6 100644 --- a/lib/acts_as_paranoid_aliases.rb +++ b/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 - From d9300f1c10cd512bb8d1bff18e5db41bae73fb9d Mon Sep 17 00:00:00 2001 From: David Gil Date: Thu, 10 Sep 2015 14:16:45 +0200 Subject: [PATCH 2/4] prevents users to receive email notifications about own comments and replies --- app/controllers/comments_controller.rb | 13 +---- app/models/comment_notifier.rb | 33 +++++++++++ spec/features/emails_spec.rb | 81 ++++++++++++++++---------- spec/support/common_actions.rb | 10 ++-- 4 files changed, 89 insertions(+), 48 deletions(-) create mode 100644 app/models/comment_notifier.rb diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 96aecf083..358ff5516 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -7,10 +7,7 @@ class CommentsController < ApplicationController respond_to :html, :js def create - if @comment.save - Mailer.comment(@comment).deliver_later if email_on_debate_comment? - Mailer.reply(@comment).deliver_later if email_on_comment_reply? - else + unless @comment.save render :new end end @@ -55,14 +52,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 diff --git a/app/models/comment_notifier.rb b/app/models/comment_notifier.rb new file mode 100644 index 000000000..8a82e9df0 --- /dev/null +++ b/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 diff --git a/spec/features/emails_spec.rb b/spec/features/emails_spec.rb index 86c794fac..cd4d85116 100644 --- a/spec/features/emails_spec.rb +++ b/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 - user = create(:user, email_on_debate_comment: true) - debate = create(:debate, author: user) - comment_on(debate) + context 'Debate comments' do + scenario "Send email on debate comment", :js do + user = create(:user, email_on_debate_comment: true) + debate = create(:debate, author: user) + comment_on(debate) - email = open_last_email - expect(email).to have_subject('Someone has commented on your debate') - expect(email).to deliver_to(debate.author) - expect(email).to have_body_text(debate_path(debate)) + email = open_last_email + expect(email).to have_subject('Someone has commented on your debate') + expect(email).to deliver_to(debate.author) + 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: 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 - user = create(:user, email_on_comment_reply: true) - reply_to(user) + context 'Comment replies' do + scenario "Send email on comment reply", :js do + user = create(:user, email_on_comment_reply: true) + reply_to(user) - email = open_last_email - expect(email).to have_subject('Someone has replied to your comment') - expect(email).to deliver_to(user) - expect(email).to have_body_text(debate_path(Comment.first.debate)) + email = open_last_email + expect(email).to have_subject('Someone has replied to your comment') + expect(email).to deliver_to(user) + 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: 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 - 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 \ No newline at end of file +end diff --git a/spec/support/common_actions.rb b/spec/support/common_actions.rb index 61974864d..d7f2106a7 100644 --- a/spec/support/common_actions.rb +++ b/spec/support/common_actions.rb @@ -44,8 +44,8 @@ module CommonActions click_button 'Send me reset password instructions' end - def comment_on(debate) - user = create(:user) + def comment_on(debate, options = {}) + user = options.fetch(:user) { create(:user) } login_as(user) visit debate_path(debate) @@ -56,10 +56,10 @@ module CommonActions expect(page).to have_content 'Have you thought about...?' end - def reply_to(user) - manuela = create(:user) + def reply_to(original_user, options = {}) + manuela = options.fetch(:user) { 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) From 3f652262e7fa054e50bbf5986c2d60a6da7a59ce Mon Sep 17 00:00:00 2001 From: David Gil Date: Thu, 10 Sep 2015 18:47:45 +0200 Subject: [PATCH 3/4] makes the controller send the emails instead of Comment model --- app/controllers/comments_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 358ff5516..1573ed590 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -7,7 +7,9 @@ class CommentsController < ApplicationController respond_to :html, :js def create - unless @comment.save + if @comment.save + CommentNotifier.new(comment: @comment).process + else render :new end end From 3386a29eb43f181c01e912d06f58976a765cc8e5 Mon Sep 17 00:00:00 2001 From: David Gil Date: Thu, 10 Sep 2015 18:55:03 +0200 Subject: [PATCH 4/4] reverts options hash for optional attributes in spec helpers --- spec/features/emails_spec.rb | 4 ++-- spec/support/common_actions.rb | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/features/emails_spec.rb b/spec/features/emails_spec.rb index cd4d85116..b6a9d1db5 100644 --- a/spec/features/emails_spec.rb +++ b/spec/features/emails_spec.rb @@ -39,7 +39,7 @@ feature 'Emails' do 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: user) + comment_on(debate, user) expect { open_last_email }.to raise_error "No email has been sent!" end @@ -66,7 +66,7 @@ feature 'Emails' do 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: user) + reply_to(user, user) expect { open_last_email }.to raise_error "No email has been sent!" end diff --git a/spec/support/common_actions.rb b/spec/support/common_actions.rb index d7f2106a7..508daa297 100644 --- a/spec/support/common_actions.rb +++ b/spec/support/common_actions.rb @@ -44,8 +44,8 @@ module CommonActions click_button 'Send me reset password instructions' end - def comment_on(debate, options = {}) - user = options.fetch(:user) { create(:user) } + def comment_on(debate, user = nil) + user ||= create(:user) login_as(user) visit debate_path(debate) @@ -56,8 +56,9 @@ module CommonActions expect(page).to have_content 'Have you thought about...?' end - def reply_to(original_user, options = {}) - manuela = options.fetch(:user) { create(:user) } + def reply_to(original_user, manuela = nil) + manuela ||= create(:user) + debate = create(:debate) comment = create(:comment, commentable: debate, user: original_user)