From da1183e6936b15b7f8931ee2a8033c00a1621000 Mon Sep 17 00:00:00 2001 From: Stephen Kenworthy Date: Sat, 21 Sep 2024 10:41:37 +0800 Subject: [PATCH 1/4] Don't send comment email to suspended or inactive users. --- app/models/polymorphic/comment.rb | 3 ++- spec/factories/user_factories.rb | 1 + spec/models/polymorphic/comment_spec.rb | 32 +++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/models/polymorphic/comment.rb b/app/models/polymorphic/comment.rb index d930a24c64..256dcb429e 100644 --- a/app/models/polymorphic/comment.rb +++ b/app/models/polymorphic/comment.rb @@ -53,7 +53,8 @@ def subscribe_user_to_entity(u = user) # Notify subscribed users when a comment is added, unless user created this comment def notify_subscribers commentable.subscribed_users.reject { |user_id| user_id == user.id }.each do |subscriber_id| - if subscriber = User.find_by_id(subscriber_id) + subscriber = User.find_by_id(subscriber_id) + if subscriber && subscriber.confirmed? && !subscriber.awaits_approval? && !subscriber.suspended? && subscriber.email.present? SubscriptionMailer.comment_notification(subscriber, self).deliver_later end end diff --git a/spec/factories/user_factories.rb b/spec/factories/user_factories.rb index 95746b3638..7933f5c61c 100755 --- a/spec/factories/user_factories.rb +++ b/spec/factories/user_factories.rb @@ -31,6 +31,7 @@ deleted_at { nil } updated_at { FactoryBot.generate(:time) } created_at { FactoryBot.generate(:time) } + confirmed_at { Time.now.utc } suspended_at { nil } password { "password" } password_confirmation { "password" } diff --git a/spec/models/polymorphic/comment_spec.rb b/spec/models/polymorphic/comment_spec.rb index b291506f89..0395af9663 100644 --- a/spec/models/polymorphic/comment_spec.rb +++ b/spec/models/polymorphic/comment_spec.rb @@ -42,4 +42,36 @@ expect(entity.subscribed_users).to include(user.id) end end + + describe "notify_subscribers" do + let(:subscriber) { create(:user) } + let(:entity) { create(:lead, subscribed_users: [subscriber.id]) } + before(:each) do + allow(SubscriptionMailer).to receive_message_chain(:comment_notification, :deliver_later) + end + + it "should notify subscribers when a comment is added" do + Comment.create!(comment: "Hello", user: create(:user), commentable: entity) + expect(SubscriptionMailer).to have_received(:comment_notification).with(subscriber, instance_of(Comment)) + end + + it "should not notify the user who created the comment" do + user = create(:user, confirmed_at: Time.now, email: "user@example.com") + Comment.create!(comment: "Hello", user: user, commentable: entity) + expect(SubscriptionMailer).not_to have_received(:comment_notification).with(user, instance_of(Comment)) + end + + it "should not notify suspended users" do + subscriber.update(suspended_at: Time.now) + Comment.create!(comment: "Hello", user: create(:user), commentable: entity) + expect(SubscriptionMailer).not_to have_received(:comment_notification).with(subscriber, instance_of(Comment)) + end + + it "should not notify users awaiting approval" do + subscriber.update(sign_in_count: 0, suspended_at: Time.now) + allow(Setting).to receive(:user_signup).and_return(:not_allowed) + Comment.create!(comment: "Hello", user: create(:user), commentable: entity) + expect(SubscriptionMailer).not_to have_received(:comment_notification).with(subscriber, instance_of(Comment)) + end + end end From 76f58e56bb868c14d8d8217a99ea50cad68d9f55 Mon Sep 17 00:00:00 2001 From: Daniel O'Connor Date: Sat, 21 Sep 2024 12:37:53 +0930 Subject: [PATCH 2/4] Update app/models/polymorphic/comment.rb --- app/models/polymorphic/comment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/polymorphic/comment.rb b/app/models/polymorphic/comment.rb index 256dcb429e..89b4a78056 100644 --- a/app/models/polymorphic/comment.rb +++ b/app/models/polymorphic/comment.rb @@ -54,7 +54,7 @@ def subscribe_user_to_entity(u = user) def notify_subscribers commentable.subscribed_users.reject { |user_id| user_id == user.id }.each do |subscriber_id| subscriber = User.find_by_id(subscriber_id) - if subscriber && subscriber.confirmed? && !subscriber.awaits_approval? && !subscriber.suspended? && subscriber.email.present? + if subscriber&.confirmed? && !subscriber.awaits_approval? && !subscriber.suspended? && subscriber.email.present? SubscriptionMailer.comment_notification(subscriber, self).deliver_later end end From 52fdc53c8ab890910e6cc948b59d2fd63078de08 Mon Sep 17 00:00:00 2001 From: Stephen Kenworthy Date: Sat, 21 Sep 2024 14:15:00 +0800 Subject: [PATCH 3/4] Fix broken spec --- spec/features/devise/sign_in_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/devise/sign_in_spec.rb b/spec/features/devise/sign_in_spec.rb index 7c913b8dcf..8bc39b8c33 100644 --- a/spec/features/devise/sign_in_spec.rb +++ b/spec/features/devise/sign_in_spec.rb @@ -15,7 +15,8 @@ password: 'password', password_confirmation: 'password', email: 'john@example.com', - sign_in_count: 0 + sign_in_count: 0, + confirmed_at: nil end scenario 'without confirmation' do From 25b42a485e36683de5d19fa8ff8dac6cfb07142b Mon Sep 17 00:00:00 2001 From: Stephen Kenworthy Date: Sat, 21 Sep 2024 14:35:54 +0800 Subject: [PATCH 4/4] Moved emailable logic to user model. Adjust AREL clause to only load necessary users. --- app/models/polymorphic/comment.rb | 8 +++----- app/models/users/user.rb | 6 ++++++ spec/models/users/user_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/app/models/polymorphic/comment.rb b/app/models/polymorphic/comment.rb index 89b4a78056..53cde72ef9 100644 --- a/app/models/polymorphic/comment.rb +++ b/app/models/polymorphic/comment.rb @@ -52,11 +52,9 @@ def subscribe_user_to_entity(u = user) # Notify subscribed users when a comment is added, unless user created this comment def notify_subscribers - commentable.subscribed_users.reject { |user_id| user_id == user.id }.each do |subscriber_id| - subscriber = User.find_by_id(subscriber_id) - if subscriber&.confirmed? && !subscriber.awaits_approval? && !subscriber.suspended? && subscriber.email.present? - SubscriptionMailer.comment_notification(subscriber, self).deliver_later - end + users_to_notify = User.where(id: commentable.subscribed_users.reject { |user_id| user_id == user.id }) + users_to_notify.select(&:emailable?).each do |subscriber| + SubscriptionMailer.comment_notification(subscriber, self).deliver_later end end diff --git a/app/models/users/user.rb b/app/models/users/user.rb index df3adf5256..ba1d611a93 100644 --- a/app/models/users/user.rb +++ b/app/models/users/user.rb @@ -134,6 +134,12 @@ def inactive_message end end + # Send emails to active users only + #---------------------------------------------------------------------------- + def emailable? + confirmed? && !awaits_approval? && !suspended? && email.present? + end + #---------------------------------------------------------------------------- def preference @preference ||= preferences.build diff --git a/spec/models/users/user_spec.rb b/spec/models/users/user_spec.rb index a4ca26f022..268270a159 100644 --- a/spec/models/users/user_spec.rb +++ b/spec/models/users/user_spec.rb @@ -235,4 +235,24 @@ expect(search.first).to eql(user) end end + + describe "emailable?" do + let(:user) { create(:user) } + it "should return true for standard user" do + expect(user.emailable?).to eql(true) + end + it "should return false for unconfirmed user" do + user.update(confirmed_at: nil) + expect(user.emailable?).to eql(false) + end + it "should return false for user awaiting approval" do + user.update(sign_in_count: 0, suspended_at: Time.now) + allow(Setting).to receive(:user_signup).and_return(:not_allowed) + expect(user.emailable?).to eql(false) + end + it "should return false for suspended user" do + user.update(suspended_at: Time.now) + expect(user.emailable?).to eql(false) + end + end end