FIX: Staged users getting user_linked and user_quoted emails

discourse
,
#1

FIX: Staged users getting user_linked and user_quoted emails

This fix ensures that if a staged user is linked to or quoted they won’t be emailed about it.

A staged user could email into a category, and another user could quote them inside of a completely different category and we don’t want a staged user to receive an email for this.

Bug report:

https://meta.discourse.org/t/-/145202/9

diff --git a/app/services/notification_emailer.rb b/app/services/notification_emailer.rb
index 40e0441..db51246 100644
--- a/app/services/notification_emailer.rb
+++ b/app/services/notification_emailer.rb
@@ -90,6 +90,7 @@ class NotificationEmailer
       user = notification.user
       return unless user.active? || user.staged?
       return if SiteSetting.must_approve_users? && !user.approved? && !user.staged?
+      return if user.staged? && (type == :user_linked || type == :user_quoted)
 
       return unless EMAILABLE_POST_TYPES.include?(post_type)
 
diff --git a/spec/services/notification_emailer_spec.rb b/spec/services/notification_emailer_spec.rb
index e28e172..9ca6d40 100644
--- a/spec/services/notification_emailer_spec.rb
+++ b/spec/services/notification_emailer_spec.rb
@@ -36,17 +36,24 @@ describe NotificationEmailer do
         NotificationEmailer.process_notification(notification)
       end
 
-      it "enqueues a job if the user is staged" do
+      it "enqueues a job if the user is staged for non-linked and non-quoted types" do
         notification.user.staged = true
-        Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification, type))
+        if type == :user_linked || type == :user_quoted
+          Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never
+        else
+          Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification, type))
+        end
         NotificationEmailer.process_notification(notification)
       end
 
-      it "enqueues a job if the user is staged even if site requires user approval" do
-        SiteSetting.must_approve_users = true
-
+      it "enqueues a job if the user is staged even if site requires user approval for non-linked and non-quoted typed" do
         notification.user.staged = true
-        Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification, type))
+        SiteSetting.must_approve_users = true
+        if type == :user_linked || type == :user_quoted
+          Jobs.expects(:enqueue_in).with(delay, :user_email, has_entry(type: type)).never
+        else
+          Jobs.expects(:enqueue_in).with(delay, :user_email, NotificationEmailer::EmailUser.notification_params(notification, type))
+        end
         NotificationEmailer.process_notification(notification)
       end
     end

GitHub sha: 04b431b6

#2

I wonder if we should whitelist here instead of blacklist, should be more stable longer term

#3

I know the diff between white/black list, but I’m not sure I follow how I can do that here with the passed in notification type and staged users?

Do you want something similar to EMAILABLE_POST_TYPES? like STAGED_NOTIFICATION_TYPES?

But it would still look like

return if user.staged? && STAGED_NOTIFICATION_TYPES.include?(type)

I’m thinking you want something different though that I’m not seeing which is why I’m asking.

#4

Ignore me … I totally misread this, looks perfect. sorry.

1 Like
#5