FIX: Change UserCommScreener to use user_ids (#17489)

It makes more sense to use user_ids for the UserCommScreener
introduced in fa5f3e228c since
in most cases the ID will be available, not the username. This
was discovered while starting work on a plugin that will
use this. In the cases where only usernames are available
the extra query is negligble.

app/models/topic.rb

@@ -1040,8 +1040,8 @@ class Topic < ActiveRecord::Base
         raise UserExists.new(I18n.t("topic_invite.user_exists"))
       end
 
-      comm_screener = UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username)
+      comm_screener = UserCommScreener.new(acting_user: invited_by, target_user_ids: target_user.id)
-      if comm_screener.ignoring_or_muting_actor?(target_user.username)
+      if comm_screener.ignoring_or_muting_actor?(target_user.id)
         raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username))
       end
 
@@ -1053,11 +1053,11 @@ class Topic < ActiveRecord::Base
         raise NotAllowed.new(I18n.t("topic_invite.muted_topic"))
       end
 
-      if comm_screener.disallowing_pms_from_actor?(target_user.username)
+      if comm_screener.disallowing_pms_from_actor?(target_user.id)
         raise NotAllowed.new(I18n.t("topic_invite.receiver_does_not_allow_pm"))
       end
 
-      if UserCommScreener.new(acting_user: target_user, target_usernames: invited_by.username).disallowing_pms_from_actor?(invited_by.username)
+      if UserCommScreener.new(acting_user: target_user, target_user_ids: invited_by.id).disallowing_pms_from_actor?(invited_by.id)
         raise NotAllowed.new(I18n.t("topic_invite.sender_does_not_allow_pm"))
       end
 
@@ -1759,7 +1759,7 @@ class Topic < ActiveRecord::Base
   end
 
   def create_invite_notification!(target_user, notification_type, invited_by, post_number: 1)
-    if UserCommScreener.new(acting_user: invited_by, target_usernames: target_user.username).ignoring_or_muting_actor?(target_user.username)
+    if UserCommScreener.new(acting_user: invited_by, target_user_ids: target_user.id).ignoring_or_muting_actor?(target_user.id)
       raise NotAllowed.new(I18n.t("not_accepting_pms", username: target_user.username))
     end
 

app/services/post_alerter.rb

@@ -394,8 +394,8 @@ class PostAlerter
 
     notifier_id = opts[:user_id] || post.user_id # xxxxx look at revision history
     return if notifier_id && UserCommScreener.new(
-      acting_user_id: notifier_id, target_usernames: user.username
+      acting_user_id: notifier_id, target_user_ids: user.id
-    ).ignoring_or_muting_actor?(user.username)
+    ).ignoring_or_muting_actor?(user.id)
 
     # skip if muted on the topic
     return if TopicUser.where(

lib/post_creator.rb

@@ -113,8 +113,9 @@ class PostCreator
 
       # Make sure none of the users have muted or ignored the creator or prevented
       # PMs from being sent to them
-      UserCommScreener.new(acting_user: @user, target_usernames: names).preventing_actor_communication.each do |username|
+      target_users = User.where(username_lower: names.map(&:downcase)).pluck(:id, :username).to_h
-        errors.add(:base, I18n.t(:not_accepting_pms, username: username))
+      UserCommScreener.new(acting_user: @user, target_user_ids: target_users.keys).preventing_actor_communication.each do |user_id|
+        errors.add(:base, I18n.t(:not_accepting_pms, username: target_users[user_id]))
       end
 
       return false if errors[:base].present?

lib/user_comm_screener.rb

@@ -30,11 +30,11 @@ class UserCommScreener
   attr_reader :acting_user, :preferences
 
   class UserCommPref
-    attr_accessor :username, :is_muting, :is_ignoring, :is_disallowing_all_pms,
+    attr_accessor :user_id, :is_muting, :is_ignoring, :is_disallowing_all_pms,
       :is_disallowing_pms_from_acting_user
 
     def initialize(preferences)
-      @username = preferences[:username]
+      @user_id = preferences[:user_id]
       @is_muting = preferences[:is_muting]
       @is_ignoring = preferences[:is_ignoring]
       @is_disallowing_all_pms = preferences[:is_disallowing_all_pms]
@@ -59,12 +59,12 @@ class UserCommScreener
       acting_user.staff?
     end
 
-    def usernames
+    def user_ids
-      user_preference_map.values.map(&:username)
+      user_preference_map.keys
     end
 
-    def for_user(username)
+    def for_user(user_id)
-      user_preference_map.values.find { |pref| pref.username.downcase == username.downcase }
+      user_preference_map[user_id]
     end
 
     def allowing_actor_communication
@@ -81,36 +81,34 @@ class UserCommScreener
       end.values
     end
 
-    def ignoring_or_muting?(username)
+    def ignoring_or_muting?(user_id)
       return false if acting_user_staff?
-      pref = for_user(username)
+      pref = for_user(user_id)
       pref.present? && pref.ignoring_or_muting?
     end
 
-    def disallowing_pms?(username)
+    def disallowing_pms?(user_id)
       return false if acting_user_staff?
-      pref = for_user(username)
+      pref = for_user(user_id)
       pref.present? && pref.disallowing_pms?
     end
   end
   private_constant :UserCommPref
   private_constant :UserCommPrefs
 
-  def initialize(acting_user: nil, acting_user_id: nil, target_usernames:)
+  def initialize(acting_user: nil, acting_user_id: nil, target_user_ids:)
     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(
+    @target_users = User.where(id: target_user_ids).pluck(:id, :username).to_h
-      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,
+  # so implicitly the leftover users 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
+    (preferences.allowing_actor_communication.map(&:user_id) + users_with_no_preference).uniq
   end
 
   ##
@@ -118,14 +116,14 @@ class UserCommScreener
   # 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)
+    preferences.preventing_actor_communication.map(&:user_id)
   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)
+  def ignoring_or_muting_actor?(user_id)
-    preferences.ignoring_or_muting?(username)
+    preferences.ignoring_or_muting?(user_id)
   end
 
   ##
@@ -133,14 +131,14 @@ class UserCommScreener
   # 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)
+  def disallowing_pms_from_actor?(user_id)
-    preferences.disallowing_pms?(username) || ignoring_or_muting_actor?(username)
+    preferences.disallowing_pms?(user_id) || ignoring_or_muting_actor?(user_id)
   end
 
   private
 
-  def usernames_with_no_preference
+  def users_with_no_preference
-    @target_users.values - @preferences.usernames
+    @target_users.keys - @preferences.user_ids
   end
 
   def load_preference_map
@@ -156,7 +154,7 @@ class UserCommScreener
     # 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],
+        user_id: user.id,
         is_muting: user.is_muting,
         is_ignoring: user.is_ignoring,
         is_disallowing_all_pms: user.is_disallowing_all_pms,
@@ -181,7 +179,7 @@ class UserCommScreener
           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],
+            user_id: user_id,
             is_muting: false,
             is_ignoring: false,
             is_disallowing_all_pms: false,

spec/lib/user_comm_screener_spec.rb

@@ -13,22 +13,22 @@ describe UserCommScreener do
 
   subject do
     described_class.new(
-      acting_user: acting_user, target_usernames: ["bobscreen", "hughscreen", "alicescreen", "janescreen", "maryscreen"]
+      acting_user: acting_user, target_user_ids: [
+        target_user1.id,
+        target_user2.id,
+        target_user3.id,
+        target_user4.id,
+        target_user5.id
+      ]
     )
   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"])
+    screener = described_class.new(acting_user: acting_user, target_user_ids: [target_user1.id])
-    expect(screener.allowing_actor_communication).to eq(["bobscreen"])
+    expect(screener.allowing_actor_communication).to eq([target_user1.id])
-    screener = described_class.new(acting_user_id: acting_user.id, target_usernames: ["bobscreen"])
+    screener = described_class.new(acting_user_id: acting_user.id, target_user_ids: [target_user1.id])
-    expect(screener.allowing_actor_communication).to eq(["bobscreen"])
+    expect(screener.allowing_actor_communication).to eq([target_user1.id])
-  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
@@ -38,60 +38,60 @@ describe UserCommScreener do
 
     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"])
+        expect(subject.allowing_actor_communication).to match_array([target_user4.id, target_user5.id])
       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"])
+        expect(subject.preventing_actor_communication).to match_array([target_user1.id, target_user2.id, target_user3.id])
       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
+        expect { subject.ignoring_or_muting_actor?(target_user5.id) }.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)
+        expect(subject.ignoring_or_muting_actor?(target_user1.id)).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)
+        expect(subject.ignoring_or_muting_actor?(target_user2.id)).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)
+        expect(subject.ignoring_or_muting_actor?(target_user3.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user3.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user4.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user4.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user5.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user1.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user2.id)).to eq(true)
       end
     end
   end
@@ -103,7 +103,13 @@ describe UserCommScreener do
 
     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"])
+        expect(subject.allowing_actor_communication).to match_array([
+          target_user1.id,
+          target_user2.id,
+          target_user3.id,
+          target_user4.id,
+          target_user5.id
+        ])
       end
     end
 
@@ -115,38 +121,38 @@ describe UserCommScreener do
 
     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)
+        expect(subject.ignoring_or_muting_actor?(target_user1.id)).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)
+        expect(subject.ignoring_or_muting_actor?(target_user2.id)).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)
+        expect(subject.ignoring_or_muting_actor?(target_user3.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user3.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user4.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user5.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user1.id)).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)
+        expect(subject.disallowing_pms_from_actor?(target_user2.id)).to eq(false)
       end
     end
   end