DEV: Refactor user mute/ignore/disallow PM checks into central class (#17366)

The idea behind this refactor is to centralise all of the user ignoring / muting / disallow PM checks in a single place, so they can be used consistently in core as well as for plugins like chat, while improving the main bulk of the checks to run in a single fast non-AR query.

Also fixed up the invite error when someone is muting/ignoring the user that is trying to invite them to the topic.

app/controllers/invites_controller.rb

@@ -482,7 +482,7 @@ class InvitesController < ApplicationController
           topic.create_invite_notification!(
             user,
             Notification.types[:invited_to_topic],
-            invite.invited_by.username
+            invite.invited_by
           )
         end
       end

app/models/topic.rb

@@ -1040,7 +1040,10 @@ class Topic < ActiveRecord::Base
         raise UserExists.new(I18n.t("topic_invite.user_exists"))
       end
 
-      ensure_can_invite!(target_user, invited_by)
+      comm_screener = UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username)
+      if comm_screener.ignoring_or_muting_actor?(target_user.username)
+        raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username))
+      end
 
       if TopicUser
           .where(topic: self,
@@ -1050,15 +1053,11 @@ class Topic < ActiveRecord::Base
         raise NotAllowed.new(I18n.t("topic_invite.muted_topic"))
       end
 
-      if !target_user.staff? &&
+      if comm_screener.disallowing_pms_from_actor?(target_user.username)
-         target_user&.user_option&.enable_allowed_pm_users &&
-         !AllowedPmUser.where(user: target_user, allowed_pm_user: invited_by).exists?
         raise NotAllowed.new(I18n.t("topic_invite.receiver_does_not_allow_pm"))
       end
 
-      if !target_user.staff? &&
+      if UserCommScreener.new(acting_user: target_user, target_usernames: invited_by.username).disallowing_pms_from_actor?(invited_by.username)
-         invited_by&.user_option&.enable_allowed_pm_users &&
-         !AllowedPmUser.where(user: invited_by, allowed_pm_user: target_user).exists?
         raise NotAllowed.new(I18n.t("topic_invite.sender_does_not_allow_pm"))
       end
 
@@ -1078,22 +1077,6 @@ class Topic < ActiveRecord::Base
     end
   end
 
-  def ensure_can_invite!(target_user, invited_by)
-    if MutedUser
-        .where(user: target_user, muted_user: invited_by)
-        .joins(:muted_user)
-        .where('NOT admin AND NOT moderator')
-        .exists?
-      raise NotAllowed
-    elsif IgnoredUser
-        .where(user: target_user, ignored_user: invited_by)
-        .joins(:ignored_user)
-        .where('NOT admin AND NOT moderator')
-        .exists?
-      raise NotAllowed
-    end
-  end
-
   def email_already_exists_for?(invite)
     invite.email_already_exists && private_message?
   end
@@ -1775,9 +1758,10 @@ class Topic < ActiveRecord::Base
     email_addresses.to_a
   end
 
-  def create_invite_notification!(target_user, notification_type, username, post_number: 1)
+  def create_invite_notification!(target_user, notification_type, invited_by, post_number: 1)
-    invited_by = User.find_by_username(username)
+    if UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username).ignoring_or_muting_actor?(target_user.username)
-    ensure_can_invite!(target_user, invited_by)
+      raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username))
+    end
 
     target_user.notifications.create!(
       notification_type: notification_type,
@@ -1785,7 +1769,7 @@ class Topic < ActiveRecord::Base
       post_number: post_number,
       data: {
         topic_title: self.title,
-        display_username: username,
+        display_username: invited_by.username,
         original_user_id: user.id,
         original_username: user.username
       }.to_json
@@ -1876,7 +1860,7 @@ class Topic < ActiveRecord::Base
       create_invite_notification!(
         target_user,
         Notification.types[:invited_to_private_message],
-        invited_by.username
+        invited_by
       )
     end
   end
@@ -1904,7 +1888,7 @@ class Topic < ActiveRecord::Base
         create_invite_notification!(
           target_user,
           Notification.types[:invited_to_topic],
-          invited_by.username
+          invited_by
         )
       end
     end

app/services/post_alerter.rb

@@ -393,18 +393,9 @@ class PostAlerter
     return if user.staged? && topic.category&.mailinglist_mirror?
 
     notifier_id = opts[:user_id] || post.user_id # xxxxx look at revision history
-
+    return if notifier_id && UserCommScreener.new(
-    # apply muting here
+      acting_user_id: notifier_id, target_usernames: user.username
-    return if notifier_id && MutedUser.where(user_id: user.id, muted_user_id: notifier_id)
+    ).ignoring_or_muting_actor?(user.username)
-      .joins(:muted_user)
-      .where('NOT admin AND NOT moderator')
-      .exists?
-
-    # apply ignored here
-    return if notifier_id && IgnoredUser.where(user_id: user.id, ignored_user_id: notifier_id)
-      .joins(:ignored_user)
-      .where('NOT admin AND NOT moderator')
-      .exists?
 
     # skip if muted on the topic
     return if TopicUser.where(

lib/post_creator.rb

@@ -111,39 +111,10 @@ class PostCreator
         return false
       end
 
-      # Make sure none of the users have muted or ignored the creator
+      # Make sure none of the users have muted or ignored the creator or prevented
-      users = User.where(username_lower: names).pluck(:id, :username).to_h
+      # PMs from being sent to them
-
+      UserCommScreener.new(acting_user: @user, target_usernames: names).preventing_actor_communication.each do |username|
-      User
+        errors.add(:base, I18n.t(:not_accepting_pms, username: username))
-        .joins("LEFT JOIN user_options ON user_options.user_id = users.id")
-        .joins("LEFT JOIN muted_users ON muted_users.user_id = users.id AND muted_users.muted_user_id = #{@user.id.to_i}")
-        .joins("LEFT JOIN ignored_users ON ignored_users.user_id = users.id AND ignored_users.ignored_user_id = #{@user.id.to_i}")
-        .where("user_options.user_id IS NOT NULL")
-        .where("
-          (user_options.user_id IN (:user_ids) AND NOT user_options.allow_private_messages) OR
-          muted_users.user_id IN (:user_ids) OR
-          ignored_users.user_id IN (:user_ids)
-        ", user_ids: users.keys)
-        .pluck(:id).each do |m|
-
-        errors.add(:base, I18n.t(:not_accepting_pms, username: users[m]))
-      end
-
-      # Is Allowed PM users list enabled for any recipients?
-      users_with_allowed_pms = allowed_pms_enabled(users).pluck(:id).uniq
-
-      # If any of the users has allowed_pm_users enabled check to see if the creator
-      # is in their list
-      if users_with_allowed_pms.any?
-        users_sender_can_pm = allowed_pms_enabled(users)
-          .where("allowed_pm_users.allowed_pm_user_id" => @user.id.to_i)
-          .pluck(:id).uniq
-
-        # If not in the list add an error
-        users_not_allowed = users_with_allowed_pms - users_sender_can_pm
-        users_not_allowed.each do |id|
-          errors.add(:base, I18n.t(:not_accepting_pms, username: users[id]))
-        end
       end
 
       return false if errors[:base].present?
@@ -487,17 +458,6 @@ class PostCreator
 
   private
 
-  def allowed_pms_enabled(users)
-    User
-      .joins("LEFT JOIN user_options ON user_options.user_id = users.id")
-      .joins("LEFT JOIN allowed_pm_users ON allowed_pm_users.user_id = users.id")
-      .where("
-        user_options.user_id IS NOT NULL AND
-        user_options.user_id IN (:user_ids) AND
-        user_options.enable_allowed_pm_users
-      ", user_ids: users.keys)
-  end
-
   def create_topic
     return if @topic
     begin

lib/user_comm_screener.rb

@@ -0,0 +1,217 @@
+# frozen_string_literal: true
+
+# There are various ways within Discourse that a user can prevent
+# other users communicating with them. The purpose of this class is to
+# find which of the target users are ignoring, muting, or preventing
+# private messages from the acting user, so we can take alternative
+# action (such as raising an error or showing a helpful message) if so.
+#
+# Users may Mute another user (the actor), which will:
+#
+#   * Prevent PMs from the actor
+#   * Prevent notifications from the actor
+#
+# Users may Ignore another user (the actor), which will:
+#
+#   * Do everything that Mute does as well as suppressing content made by
+#     the actor (such as posts) from the UI
+#
+# Users may also either:
+#
+#   a) disallow PMs from being sent to them or
+#   b) disallow PMs except from a certain allowlist of users
+#
+# A user may have this preference but have no Muted or Ignored users, which
+# necessitates the difference between methods in this class.
+#
+# An important note is that **all of these settings do not apply when the actor
+# is a staff member**. So admins and moderators can PM and notify anyone they please.
+class UserCommScreener
+  attr_reader :acting_user, :preferences
+
+  class UserCommPref
+    attr_accessor :username, :is_muting, :is_ignoring, :is_disallowing_all_pms,
+      :is_disallowing_pms_from_acting_user
+
+    def initialize(preferences)
+      @username = preferences[:username]
+      @is_muting = preferences[:is_muting]
+      @is_ignoring = preferences[:is_ignoring]
+      @is_disallowing_all_pms = preferences[:is_disallowing_all_pms]
+      @is_disallowing_pms_from_acting_user = preferences[:is_disallowing_pms_from_acting_user]
+    end
+
+    def communication_prevented?
+      ignoring_or_muting? || disallowing_pms?
+    end
+
+    def ignoring_or_muting?
+      is_muting || is_ignoring
+    end
+
+    def disallowing_pms?
+      is_disallowing_all_pms || is_disallowing_pms_from_acting_user
+    end
+  end
+
+  UserCommPrefs = Struct.new(:acting_user, :user_preference_map) do
+    def acting_user_staff?
+      acting_user.staff?
+    end
+
+    def usernames
+      user_preference_map.values.map(&:username)
+    end
+
+    def for_user(username)
+      user_preference_map.values.find { |pref| pref.username.downcase == username.downcase }
+    end
+
+    def allowing_actor_communication
+      return user_preference_map.values if acting_user_staff?
+      user_preference_map.reject do |user_id, pref|
+        pref.communication_prevented?
+      end.values
+    end
+
+    def preventing_actor_communication
+      return [] if acting_user_staff?
+      user_preference_map.select do |user_id, pref|
+        pref.communication_prevented?
+      end.values
+    end
+
+    def ignoring_or_muting?(username)
+      return false if acting_user_staff?
+      pref = for_user(username)
+      pref.present? && pref.ignoring_or_muting?
+    end
+
+    def disallowing_pms?(username)
+      return false if acting_user_staff?
+      pref = for_user(username)
+      pref.present? && pref.disallowing_pms?
+    end
+  end
+  private_constant :UserCommPref
+  private_constant :UserCommPrefs
+
+  def initialize(acting_user: nil, acting_user_id: nil, target_usernames:)
+    raise ArgumentError if acting_user.blank? && acting_user_id.blank?
+    @acting_user = acting_user.present? ? acting_user : User.find(acting_user_id)
+    @target_users = User.where(
+      username_lower: Array.wrap(target_usernames).map(&:downcase)
+    ).pluck(:id, :username).to_h
+    @preferences = load_preference_map
+  end
+
+  ##
+  # Users who have preferences are the only ones initially loaded by the query,
+  # so implicitly the leftover usernames have no preferences that mute, ignore,
+  # or disallow PMs from any other user.
+  def allowing_actor_communication
+    (preferences.allowing_actor_communication.map(&:username) + usernames_with_no_preference).uniq
+  end
+
+  ##
+  # Any users who are either ignoring, muting, or disallowing PMs from the actor.
+  # Ignoring and muting implicitly ignore PMs which is why they fall under this
+  # umbrella as well.
+  def preventing_actor_communication
+    preferences.preventing_actor_communication.map(&:username)
+  end
+
+  ##
+  # Whether the user is ignoring or muting the actor, meaning the actor cannot
+  # PM or send notifications to this target user.
+  def ignoring_or_muting_actor?(username)
+    preferences.ignoring_or_muting?(username)
+  end
+
+  ##
+  # Whether the user is disallowing PMs from the actor specifically or in general,
+  # meaning the actor cannot send PMs to this target user. Ignoring or muting
+  # implicitly disallows PMs, so we need to take into account those preferences
+  # here too.
+  def disallowing_pms_from_actor?(username)
+    preferences.disallowing_pms?(username) || ignoring_or_muting_actor?(username)
+  end
+
+  private
+
+  def usernames_with_no_preference
+    @target_users.values - @preferences.usernames
+  end
+
+  def load_preference_map
+    resolved_user_communication_preferences = {}
+
+    # Since noone can prevent staff communicating with them there is no
+    # need to load their preferences.
+    if @acting_user.staff?
+      return UserCommPrefs.new(acting_user, resolved_user_communication_preferences)
+    end
+
+    # Add all users who have muted or ignored the acting user, or have
+    # disabled PMs from them or anyone at all.
+    user_communication_preferences.each do |user|
+      resolved_user_communication_preferences[user.id] = UserCommPref.new(
+        username: @target_users[user.id],
+        is_muting: user.is_muting,
+        is_ignoring: user.is_ignoring,
+        is_disallowing_all_pms: user.is_disallowing_all_pms,
+        is_disallowing_pms_from_acting_user: false
+      )
+    end
+
+    # If any of the users has allowed_pm_users enabled check to see if the creator
+    # is in their list.
+    users_with_allowed_pms = user_communication_preferences.select(&:enable_allowed_pm_users)
+
+    if users_with_allowed_pms.any?
+      user_ids_with_allowed_pms = users_with_allowed_pms.map(&:id)
+      user_ids_acting_can_pm = AllowedPmUser.where(
+        allowed_pm_user_id: acting_user.id, user_id: user_ids_with_allowed_pms
+      ).pluck(:user_id).uniq
+
+      # If not in the list mark them as not accepting communication.
+      user_ids_acting_cannot_pm = user_ids_with_allowed_pms - user_ids_acting_can_pm
+      user_ids_acting_cannot_pm.each do |user_id|
+        if resolved_user_communication_preferences[user_id]
+          resolved_user_communication_preferences[user_id].is_disallowing_pms_from_acting_user = true
+        else
+          resolved_user_communication_preferences[user_id] = UserCommPref.new(
+            username: @target_users[user_id],
+            is_muting: false,
+            is_ignoring: false,
+            is_disallowing_all_pms: false,
+            is_disallowing_pms_from_acting_user: true
+          )
+        end
+      end
+    end
+
+    UserCommPrefs.new(acting_user, resolved_user_communication_preferences)
+  end
+
+  def user_communication_preferences
+    @user_communication_preferences ||= DB.query(<<~SQL, acting_user_id: acting_user.id, target_user_ids: @target_users.keys)
+      SELECT users.id,
+      CASE WHEN muted_users.muted_user_id IS NOT NULL THEN true ELSE false END AS is_muting,
+      CASE WHEN ignored_users.ignored_user_id IS NOT NULL THEN true ELSE false END AS is_ignoring,
+      CASE WHEN user_options.allow_private_messages THEN false ELSE true END AS is_disallowing_all_pms,
+      user_options.enable_allowed_pm_users
+      FROM users
+      LEFT JOIN user_options ON user_options.user_id = users.id
+      LEFT JOIN muted_users ON muted_users.user_id = users.id AND muted_users.muted_user_id = :acting_user_id
+      LEFT JOIN ignored_users ON ignored_users.user_id = users.id AND ignored_users.ignored_user_id = :acting_user_id
+      WHERE users.id IN (:target_user_ids) AND
+      (
+        NOT user_options.allow_private_messages OR
+        user_options.enable_allowed_pm_users OR
+        muted_users.user_id IS NOT NULL OR
+        ignored_users.user_id IS NOT NULL
+      )
+    SQL
+  end
+end

spec/lib/user_comm_screener_spec.rb

@@ -0,0 +1,153 @@
+# frozen_string_literal: true
+
+describe UserCommScreener do
+  fab!(:target_user1) { Fabricate(:user, username: "bobscreen") }
+  fab!(:target_user2) { Fabricate(:user, username: "hughscreen") }
+  fab!(:target_user3) do
+    user = Fabricate(:user, username: "alicescreen")
+    user.user_option.update(allow_private_messages: false)
+    user
+  end
+  fab!(:target_user4) { Fabricate(:user, username: "janescreen") }
+  fab!(:target_user5) { Fabricate(:user, username: "maryscreen") }
+
+  subject do
+    described_class.new(
+      acting_user: acting_user, target_usernames: ["bobscreen", "hughscreen", "alicescreen", "janescreen", "maryscreen"]
+    )
+  end
+
+  it "allows initializing the class with both an acting_user_id and an acting_user" do
+    acting_user = Fabricate(:user)
+    screener = described_class.new(acting_user: acting_user, target_usernames: ["bobscreen"])
+    expect(screener.allowing_actor_communication).to eq(["bobscreen"])
+    screener = described_class.new(acting_user_id: acting_user.id, target_usernames: ["bobscreen"])
+    expect(screener.allowing_actor_communication).to eq(["bobscreen"])
+  end
+
+  it "makes sure to lowercase target usernames" do
+    acting_user = Fabricate(:user)
+    screener = described_class.new(acting_user: acting_user, target_usernames: ["BoBscrEEN", "HUghSCreen"])
+    expect(screener.allowing_actor_communication).to eq(["bobscreen", "hughscreen"])
+  end
+
+  context "when the actor is not staff" do
+    fab!(:acting_user) { Fabricate(:user) }
+    fab!(:muted_user) { Fabricate(:muted_user, user: target_user1, muted_user: acting_user) }
+    fab!(:ignored_user) { Fabricate(:ignored_user, user: target_user2, ignored_user: acting_user, expiring_at: 2.days.from_now) }
+
+    describe "#allowing_actor_communication" do
+      it "returns the usernames of people not ignoring, muting, or disallowing PMs from the actor" do
+        expect(subject.allowing_actor_communication).to match_array(["janescreen", "maryscreen"])
+      end
+    end
+
+    describe "#preventing_actor_communication" do
+      it "returns the usernames of people ignoring, muting, or disallowing PMs from the actor" do
+        expect(subject.preventing_actor_communication).to match_array(["bobscreen", "hughscreen", "alicescreen"])
+      end
+    end
+
+    describe "#ignoring_or_muting_actor?" do
+      it "does not raise an error when looking for a user who has no communication preferences" do
+        expect { subject.ignoring_or_muting_actor?(target_user5.username) }.not_to raise_error
+      end
+
+      it "returns true for a user muting the actor" do
+        expect(subject.ignoring_or_muting_actor?(target_user1.username)).to eq(true)
+      end
+
+      it "returns true for a user ignoring the actor" do
+        expect(subject.ignoring_or_muting_actor?(target_user2.username)).to eq(true)
+      end
+
+      it "returns false for a user neither ignoring or muting the actor" do
+        expect(subject.ignoring_or_muting_actor?(target_user3.username)).to eq(false)
+      end
+    end
+
+    describe "#disallowing_pms_from_actor?" do
+      it "returns true for a user disallowing all PMs" do
+        expect(subject.disallowing_pms_from_actor?(target_user3.username)).to eq(true)
+      end
+
+      it "returns true for a user allowing only PMs for certain users but not the actor" do
+        target_user4.user_option.update!(enable_allowed_pm_users: true)
+        expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(true)
+      end
+
+      it "returns false for a user allowing only PMs for certain users which the actor allowed" do
+        target_user4.user_option.update!(enable_allowed_pm_users: true)
+        AllowedPmUser.create!(user: target_user4, allowed_pm_user: acting_user)
+        expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(false)
+      end
+
+      it "returns false for a user not disallowing PMs or muting or ignoring" do
+        expect(subject.disallowing_pms_from_actor?(target_user5.username)).to eq(false)
+      end
+
+      it "returns true for a user not disallowing PMs but still ignoring" do
+        expect(subject.disallowing_pms_from_actor?(target_user1.username)).to eq(true)
+      end
+
+      it "returns true for a user not disallowing PMs but still muting" do
+        expect(subject.disallowing_pms_from_actor?(target_user2.username)).to eq(true)
+      end
+    end
+  end
+
+  context "when the actor is staff" do
+    fab!(:acting_user) { Fabricate(:admin) }
+    fab!(:muted_user) { Fabricate(:muted_user, user: target_user1, muted_user: acting_user) }
+    fab!(:ignored_user) { Fabricate(:ignored_user, user: target_user1, ignored_user: acting_user, expiring_at: 2.days.from_now) }
+
+    describe "#allowing_actor_communication" do
+      it "returns all usernames since staff can communicate with anyone" do
+        expect(subject.allowing_actor_communication).to match_array(["bobscreen", "hughscreen", "alicescreen", "janescreen", "maryscreen"])
+      end
+    end
+
+    describe "#preventing_actor_communication" do
+      it "does not return any usernames since no users can prevent staff communicating with them" do
+        expect(subject.preventing_actor_communication).to eq([])
+      end
+    end
+
+    describe "#ignoring_or_muting_actor?" do
+      it "returns false for a user muting the staff" do
+        expect(subject.ignoring_or_muting_actor?(target_user1.username)).to eq(false)
+      end
+
+      it "returns false for a user ignoring the staff actor" do
+        expect(subject.ignoring_or_muting_actor?(target_user2.username)).to eq(false)
+      end
+
+      it "returns false for a user neither ignoring or muting the actor" do
+        expect(subject.ignoring_or_muting_actor?(target_user3.username)).to eq(false)
+      end
+    end
+
+    describe "#disallowing_pms_from_actor?" do
+      it "returns false for a user disallowing all PMs" do
+        expect(subject.disallowing_pms_from_actor?(target_user3.username)).to eq(false)
+      end
+
+      it "returns false for a user allowing only PMs for certain users but not the actor" do
+        target_user4.user_option.update!(enable_allowed_pm_users: true)
+        expect(subject.disallowing_pms_from_actor?(target_user4.username)).to eq(false)
+      end
+
+      it "returns false for a user not disallowing PMs or muting or ignoring" do
+        expect(subject.disallowing_pms_from_actor?(target_user5.username)).to eq(false)
+      end
+
+      it "returns false for a user not disallowing PMs but still ignoring" do
+        expect(subject.disallowing_pms_from_actor?(target_user1.username)).to eq(false)
+      end
+
+      it "returns false for a user not disallowing PMs but still muting" do
+        expect(subject.disallowing_pms_from_actor?(target_user2.username)).to eq(false)
+      end
+    end
+  end
+end