From d66115d9187624f0f74a59c3e6fc6d91ff6323ca Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 4 Aug 2022 09:06:51 +1000 Subject: [PATCH] DEV: UserCommScreener fine-grained actor improvements (#17737) This commit introduces several fine-grained methods to UserCommScreener which can be used to show the actor who they are ignoring/muting/blocking DMs from in order to prevent them initiating conversation with those users or to display relevant information in the UI to the actor. This will be used in a companion PR in discourse-chat, and is a follow up to https://github.com/discourse/discourse-chat/commit/74584ff3ca2d29caf243dd413509d0cc03b29a88 Co-authored-by: Alan Guo Xiang Tan Co-authored-by: Osama Sayegh --- lib/user_comm_screener.rb | 82 ++++++++++++++++++++++++++++ spec/lib/user_comm_screener_spec.rb | 84 +++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+) diff --git a/lib/user_comm_screener.rb b/lib/user_comm_screener.rb index 3612749991a..5c3e9433bea 100644 --- a/lib/user_comm_screener.rb +++ b/lib/user_comm_screener.rb @@ -26,6 +26,11 @@ # # 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. +# +# The secondary usage of this class is to determine which users the actor themselves +# are muting, ignoring, or preventing private messages from. This is useful when +# wanting to alert the actor to these users in the UI in various ways, or prevent +# the actor from communicating with users they prefer not to talk with. class UserCommScreener attr_reader :acting_user, :preferences @@ -124,6 +129,7 @@ class UserCommScreener # 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?(user_id) + validate_user_id!(user_id) preferences.ignoring_or_muting?(user_id) end @@ -133,9 +139,47 @@ class UserCommScreener # implicitly disallows PMs, so we need to take into account those preferences # here too. def disallowing_pms_from_actor?(user_id) + validate_user_id!(user_id) preferences.disallowing_pms?(user_id) || ignoring_or_muting_actor?(user_id) end + def actor_allowing_communication + @target_users.keys - actor_preventing_communication + end + + def actor_preventing_communication + (actor_preferences[:ignoring] + actor_preferences[:muting] + actor_preferences[:disallowed_pms_from]).uniq + end + + ## + # The actor methods below are more fine-grained than the user ones, + # since we may want to display more detailed messages to the actor about + # their preferences than we do when we are informing the actor that + # they cannot communicate with certain users. + # + # In this spirit, actor_disallowing_pms? is intentionally different from + # disallowing_pms_from_actor? above. + + def actor_ignoring?(user_id) + validate_user_id!(user_id) + actor_preferences[:ignoring].include?(user_id) + end + + def actor_muting?(user_id) + validate_user_id!(user_id) + actor_preferences[:muting].include?(user_id) + end + + def actor_disallowing_pms?(user_id) + validate_user_id!(user_id) + return false if !acting_user.user_option.enable_allowed_pm_users + actor_preferences[:disallowed_pms_from].include?(user_id) + end + + def actor_disallowing_all_pms? + !acting_user.user_option.allow_private_messages + end + private def users_with_no_preference @@ -193,6 +237,21 @@ class UserCommScreener UserCommPrefs.new(acting_user, resolved_user_communication_preferences) end + def actor_preferences + @actor_preferences ||= begin + user_ids_by_preference_type = actor_communication_preferences.reduce({}) do |hash, pref| + hash[pref.preference_type] ||= [] + hash[pref.preference_type] << pref.target_user_id + hash + end + { + muting: user_ids_by_preference_type["muted"], + ignoring: user_ids_by_preference_type["ignored"], + disallowed_pms_from: user_ids_by_preference_type["disallowed_pm"] + } + end + 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, @@ -213,4 +272,27 @@ class UserCommScreener ) SQL end + + def actor_communication_preferences + @actor_communication_preferences ||= DB.query(<<~SQL, acting_user_id: acting_user.id, target_user_ids: @target_users.keys) + SELECT users.id AS target_user_id, 'disallowed_pm' AS preference_type FROM users + LEFT JOIN allowed_pm_users ON allowed_pm_users.allowed_pm_user_id = users.id + WHERE users.id IN (:target_user_ids) + AND (allowed_pm_users.user_id = :acting_user_id OR allowed_pm_users.user_id IS NULL) + AND allowed_pm_users.allowed_pm_user_id IS NULL + UNION + SELECT ignored_user_id AS target_user_id, 'ignored' AS preference_type + FROM ignored_users + WHERE user_id = :acting_user_id AND ignored_user_id IN (:target_user_ids) + UNION + SELECT muted_user_id AS target_user_id, 'muted' AS preference_type + FROM muted_users + WHERE user_id = :acting_user_id AND muted_user_id IN (:target_user_ids) + SQL + end + + def validate_user_id!(user_id) + return if user_id == acting_user.id + raise Discourse::NotFound if !@target_users.keys.include?(user_id) + end end diff --git a/spec/lib/user_comm_screener_spec.rb b/spec/lib/user_comm_screener_spec.rb index 33483422cc0..474a34f7b54 100644 --- a/spec/lib/user_comm_screener_spec.rb +++ b/spec/lib/user_comm_screener_spec.rb @@ -10,6 +10,7 @@ RSpec.describe UserCommScreener do end fab!(:target_user4) { Fabricate(:user, username: "janescreen") } fab!(:target_user5) { Fabricate(:user, username: "maryscreen") } + fab!(:other_user) { Fabricate(:user) } subject do described_class.new( @@ -70,6 +71,10 @@ RSpec.describe UserCommScreener do it "returns false for a user neither ignoring or muting the actor" do expect(subject.ignoring_or_muting_actor?(target_user3.id)).to eq(false) end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.ignoring_or_muting_actor?(other_user.id) }.to raise_error(Discourse::NotFound) + end end describe "#disallowing_pms_from_actor?" do @@ -99,6 +104,10 @@ RSpec.describe UserCommScreener do it "returns true for a user not disallowing PMs but still muting" do expect(subject.disallowing_pms_from_actor?(target_user2.id)).to eq(true) end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.disallowing_pms_from_actor?(other_user.id) }.to raise_error(Discourse::NotFound) + end end end @@ -137,6 +146,10 @@ RSpec.describe UserCommScreener do it "returns false for a user neither ignoring or muting the actor" do expect(subject.ignoring_or_muting_actor?(target_user3.id)).to eq(false) end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.ignoring_or_muting_actor?(other_user.id) }.to raise_error(Discourse::NotFound) + end end describe "#disallowing_pms_from_actor?" do @@ -160,6 +173,77 @@ RSpec.describe UserCommScreener do it "returns false for a user not disallowing PMs but still muting" do expect(subject.disallowing_pms_from_actor?(target_user2.id)).to eq(false) end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.disallowing_pms_from_actor?(other_user.id) }.to raise_error(Discourse::NotFound) + end + end + end + + describe "actor preferences" do + fab!(:acting_user) { Fabricate(:user) } + fab!(:muted_user) { Fabricate(:muted_user, user: acting_user, muted_user: target_user1) } + fab!(:ignored_user) { Fabricate(:ignored_user, user: acting_user, ignored_user: target_user2, expiring_at: 2.days.from_now) } + fab!(:allowed_pm_user1) { AllowedPmUser.create!(user: acting_user, allowed_pm_user: target_user1) } + fab!(:allowed_pm_user2) { AllowedPmUser.create!(user: acting_user, allowed_pm_user: target_user2) } + fab!(:allowed_pm_user3) { AllowedPmUser.create!(user: acting_user, allowed_pm_user: target_user4) } + + describe "#actor_preventing_communication" do + it "returns the user_ids of the users the actor is ignoring, muting, or disallowing PMs from" do + acting_user.user_option.update!(enable_allowed_pm_users: true) + expect(subject.actor_preventing_communication).to match_array([ + target_user1.id, target_user2.id, target_user3.id, target_user5.id + ]) + end + end + + describe "#actor_allowing_communication" do + it "returns the user_ids of the users who the actor is not ignoring, muting, or disallowing PMs from" do + acting_user.user_option.update!(enable_allowed_pm_users: true) + expect(subject.actor_allowing_communication).to match_array([target_user4.id]) + end + end + + describe "#actor_ignoring?" do + it "returns true for user ids that the actor is ignoring" do + expect(subject.actor_ignoring?(target_user2.id)).to eq(true) + expect(subject.actor_ignoring?(target_user4.id)).to eq(false) + end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.actor_ignoring?(other_user.id) }.to raise_error(Discourse::NotFound) + end + end + + describe "#actor_muting?" do + it "returns true for user ids that the actor is muting" do + expect(subject.actor_muting?(target_user1.id)).to eq(true) + expect(subject.actor_muting?(target_user2.id)).to eq(false) + end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.actor_muting?(other_user.id) }.to raise_error(Discourse::NotFound) + end + end + + describe "#actor_disallowing_pms?" do + it "returns true for user ids that the actor is not explicitly allowing PMs from" do + acting_user.user_option.update!(enable_allowed_pm_users: true) + expect(subject.actor_disallowing_pms?(target_user3.id)).to eq(true) + expect(subject.actor_disallowing_pms?(target_user1.id)).to eq(false) + end + + it "raises a NotFound error if the user_id passed in is not part of the target users" do + expect { subject.actor_disallowing_pms?(other_user.id) }.to raise_error(Discourse::NotFound) + end + end + + describe "#actor_disallowing_all_pms?" do + it "returns true if the acting user has disabled private messages altogether" do + expect(subject.actor_disallowing_all_pms?).to eq(false) + acting_user.user_option.update!(allow_private_messages: false) + expect(subject.actor_disallowing_all_pms?).to eq(true) + end end end end