From 6b2ea1b47b770415bc5c05403d48d219ebd4ad62 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 14 Jul 2022 15:23:09 +1000 Subject: [PATCH] FIX: Change UserCommScreener to use user_ids (#17489) It makes more sense to use user_ids for the UserCommScreener introduced in fa5f3e228c102d4b9f7c6dde6eb07ef1f5880bbd 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 | 10 ++--- app/services/post_alerter.rb | 4 +- lib/post_creator.rb | 5 ++- lib/user_comm_screener.rb | 48 ++++++++++---------- spec/lib/user_comm_screener_spec.rb | 70 ++++++++++++++++------------- 5 files changed, 71 insertions(+), 66 deletions(-) diff --git a/app/models/topic.rb b/app/models/topic.rb index 437b8e6d896..805c41ab4e2 100644 --- a/app/models/topic.rb +++ b/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) - if comm_screener.ignoring_or_muting_actor?(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.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 diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index c61cb1ad2ae..53942b31d5f 100644 --- a/app/services/post_alerter.rb +++ b/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 - ).ignoring_or_muting_actor?(user.username) + acting_user_id: notifier_id, target_user_ids: user.id + ).ignoring_or_muting_actor?(user.id) # skip if muted on the topic return if TopicUser.where( diff --git a/lib/post_creator.rb b/lib/post_creator.rb index c8853e4f4c5..aeafa9fcb8e 100644 --- a/lib/post_creator.rb +++ b/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| - errors.add(:base, I18n.t(:not_accepting_pms, username: username)) + target_users = User.where(username_lower: names.map(&:downcase)).pluck(:id, :username).to_h + 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? diff --git a/lib/user_comm_screener.rb b/lib/user_comm_screener.rb index 716c5ba841a..929c7c768d9 100644 --- a/lib/user_comm_screener.rb +++ b/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 - user_preference_map.values.map(&:username) + def user_ids + user_preference_map.keys end - def for_user(username) - user_preference_map.values.find { |pref| pref.username.downcase == username.downcase } + def for_user(user_id) + 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( - username_lower: Array.wrap(target_usernames).map(&:downcase) - ).pluck(:id, :username).to_h + @target_users = User.where(id: target_user_ids).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) - preferences.ignoring_or_muting?(username) + def ignoring_or_muting_actor?(user_id) + 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) - preferences.disallowing_pms?(username) || ignoring_or_muting_actor?(username) + def disallowing_pms_from_actor?(user_id) + preferences.disallowing_pms?(user_id) || ignoring_or_muting_actor?(user_id) end private - def usernames_with_no_preference - @target_users.values - @preferences.usernames + def users_with_no_preference + @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, diff --git a/spec/lib/user_comm_screener_spec.rb b/spec/lib/user_comm_screener_spec.rb index 582b55ffb44..fb49d788733 100644 --- a/spec/lib/user_comm_screener_spec.rb +++ b/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"]) - 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"]) + screener = described_class.new(acting_user: acting_user, target_user_ids: [target_user1.id]) + expect(screener.allowing_actor_communication).to eq([target_user1.id]) + screener = described_class.new(acting_user_id: acting_user.id, target_user_ids: [target_user1.id]) + expect(screener.allowing_actor_communication).to eq([target_user1.id]) 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