From 27f7cf18b1b4cd1a762c71594b962c6cfeecd99f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Tue, 7 Mar 2023 17:58:14 +0100 Subject: [PATCH] =?UTF-8?q?FIX:=20Don=E2=80=99t=20email=20suspended=20user?= =?UTF-8?q?s=20from=20group=20PM?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when a suspended user belongs to a group PM (private message with more than two people in it) and a staff member sends a message to this group PM, then the suspended user will receive an email. This happens because a suspended user can only receive emails from staff members. But in this case, this can be seen as a bug as the expected behavior would be instead to not send any email to the suspended user. A staff member can participate in active discussions like any other member and so their messages in this context shouldn’t be treated differently than the ones from regular users. This patch addresses this issue by checking if a suspended user receives a message from a group PM or not. If that’s the case then an email won’t be sent no matter if the post originated from a staff member or not. --- app/jobs/regular/user_email.rb | 8 +++- app/models/topic.rb | 4 ++ spec/jobs/user_email_spec.rb | 87 ++++++++++++++++++++++++---------- spec/models/topic_spec.rb | 24 ++++++++++ 4 files changed, 97 insertions(+), 26 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index 6bbbe6be8fb..73bdad09e92 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -108,8 +108,12 @@ module Jobs return skip_message(SkippedEmailLog.reason_types[:user_email_anonymous_user]) end - if user.suspended? && !%w[user_private_message account_suspended].include?(type.to_s) - return skip_message(SkippedEmailLog.reason_types[:user_email_user_suspended_not_pm]) + if user.suspended? + if !type.to_s.in?(%w[user_private_message account_suspended]) + return skip_message(SkippedEmailLog.reason_types[:user_email_user_suspended_not_pm]) + elsif post.topic.group_pm? + return skip_message(SkippedEmailLog.reason_types[:user_email_user_suspended]) + end end if type.to_s == "digest" diff --git a/app/models/topic.rb b/app/models/topic.rb index 155736aa056..d50b52cafcc 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -2032,6 +2032,10 @@ class Topic < ActiveRecord::Base end end + def group_pm? + private_message? && all_allowed_users.count > 2 + end + private def invite_to_private_message(invited_by, target_user, guardian) diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 3a52f547be4..ca79668a9b0 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -423,37 +423,76 @@ RSpec.describe Jobs::UserEmail do end context "when user is suspended" do - it "doesn't send email for a pm from a regular user" do - Jobs::UserEmail.new.execute( - type: :user_private_message, - user_id: suspended.id, - post_id: post.id, - ) + context "when topic is a private message" do + subject(:send_email) do + described_class.new.execute( + type: :user_private_message, + user_id: suspended.id, + post_id: post.id, + notification_id: pm_notification.id, + ) + end - expect(ActionMailer::Base.deliveries).to eq([]) - end - - it "does send an email for a pm from a staff user" do - pm_from_staff = Fabricate(:post, user: Fabricate(:moderator)) - pm_from_staff.topic.topic_allowed_users.create!(user_id: suspended.id) - - pm_notification = + let(:pm_notification) do Fabricate( :notification, user: suspended, - topic: pm_from_staff.topic, - post_number: pm_from_staff.post_number, - data: { original_post_id: pm_from_staff.id }.to_json, + topic: post.topic, + post_number: post.post_number, + data: { original_post_id: post.id }.to_json, ) + end + fab!(:moderator) { Fabricate(:moderator) } + fab!(:regular_user) { Fabricate(:user) } - Jobs::UserEmail.new.execute( - type: :user_private_message, - user_id: suspended.id, - post_id: pm_from_staff.id, - notification_id: pm_notification.id, - ) + context "when this is not a group PM" do + let(:post) { Fabricate(:private_message_post, user: user, recipient: suspended) } - expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(suspended.email) + context "when post is from a staff user" do + let(:user) { moderator } + + it "does send an email" do + send_email + expect(ActionMailer::Base.deliveries.first.to).to contain_exactly(suspended.email) + end + end + + context "when post is from a regular user" do + let(:user) { regular_user } + + it "doesn't send email" do + send_email + expect(ActionMailer::Base.deliveries).to eq([]) + end + end + end + + context "when this is a group PM" do + fab!(:group) { Fabricate(:group) } + fab!(:users) { Fabricate.times(2, :user) } + + let(:post) { Fabricate(:group_private_message_post, user: user, recipients: group) } + + before { group.users << [suspended, *users] } + + context "when post is from a staff user" do + let(:user) { moderator } + + it "does not send an email" do + send_email + expect(ActionMailer::Base.deliveries).to be_empty + end + end + + context "when post is from a regular user" do + let(:user) { regular_user } + + it "does not send an email" do + send_email + expect(ActionMailer::Base.deliveries).to be_empty + end + end + end end it "doesn't send PM from system user" do diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 9f8e466a7be..2304fae1fd5 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -3458,4 +3458,28 @@ RSpec.describe Topic do end end end + + describe "#group_pm?" do + context "when topic is not a private message" do + subject(:public_topic) { Fabricate(:topic) } + + it { is_expected.not_to be_a_group_pm } + end + + context "when topic is a private message" do + subject(:pm_topic) { Fabricate(:private_message_topic) } + + context "when more than two people have access" do + let(:other_user) { Fabricate(:user) } + + before { pm_topic.allowed_users << other_user } + + it { is_expected.to be_a_group_pm } + end + + context "when no more than two people have access" do + it { is_expected.not_to be_a_group_pm } + end + end + end end