From 5a8b8f6f1e84150a8d7a0a094478535307d93342 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 11 Jan 2022 09:16:20 +0200 Subject: [PATCH] FEATURE: Show warning if user won't be mentioned (#15339) The new warnings cover more cases and more accurate. Most of the warnings will be visible only to staff members because otherwise they would leak information about user's preferences. --- .../app/components/composer-editor.js | 3 +- .../discourse/app/controllers/composer.js | 10 +--- .../discourse/app/lib/link-mentions.js | 6 +- app/controllers/users_controller.rb | 52 +++++++++++++--- config/locales/client.en.yml | 6 +- spec/requests/users_controller_spec.rb | 60 +++++++++++-------- 6 files changed, 90 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index 65df8bb5185..1912fded1c5 100644 --- a/app/assets/javascripts/discourse/app/components/composer-editor.js +++ b/app/assets/javascripts/discourse/app/components/composer-editor.js @@ -22,6 +22,7 @@ import { linkSeenHashtags, } from "discourse/lib/link-hashtags"; import { + cannotSee, fetchUnseenMentions, linkSeenMentions, } from "discourse/lib/link-mentions"; @@ -540,7 +541,7 @@ export default Component.extend(ComposerUploadUppy, { `.mention.cannot-see[data-name="${name}"]` )?.length > 0 ) { - this.cannotSeeMention([{ name }]); + this.cannotSeeMention([{ name, reason: cannotSee[name] }]); found.push(name); } }, diff --git a/app/assets/javascripts/discourse/app/controllers/composer.js b/app/assets/javascripts/discourse/app/controllers/composer.js index 5028b0cc313..3e81351042d 100644 --- a/app/assets/javascripts/discourse/app/controllers/composer.js +++ b/app/assets/javascripts/discourse/app/controllers/composer.js @@ -696,16 +696,12 @@ export default Controller.extend({ cannotSeeMention(mentions) { mentions.forEach((mention) => { - const translation = this.get("model.topic.isPrivateMessage") - ? "composer.cannot_see_mention.private" - : "composer.cannot_see_mention.category"; - const body = I18n.t(translation, { - username: `@${mention.name}`, - }); this.appEvents.trigger("composer-messages:create", { extraClass: "custom-body", templateName: "custom-body", - body, + body: I18n.t(`composer.cannot_see_mention.${mention.reason}`, { + username: mention.name, + }), }); }); }, diff --git a/app/assets/javascripts/discourse/app/lib/link-mentions.js b/app/assets/javascripts/discourse/app/lib/link-mentions.js index 229b2c71f71..e72575fadd9 100644 --- a/app/assets/javascripts/discourse/app/lib/link-mentions.js +++ b/app/assets/javascripts/discourse/app/lib/link-mentions.js @@ -46,7 +46,7 @@ const found = {}; const foundGroups = {}; const mentionableGroups = {}; const checked = {}; -const cannotSee = []; +export const cannotSee = {}; function updateFound(mentions, usernames) { mentions.forEach((mention, index) => { @@ -101,7 +101,9 @@ export function fetchUnseenMentions(usernames, topic_id) { r.valid.forEach((v) => (found[v] = true)); r.valid_groups.forEach((vg) => (foundGroups[vg] = true)); r.mentionable_groups.forEach((mg) => (mentionableGroups[mg.name] = mg)); - r.cannot_see.forEach((cs) => (cannotSee[cs] = true)); + Object.entries(r.cannot_see).forEach( + ([username, reason]) => (cannotSee[username] = reason) + ); maxGroupMention = r.max_users_notified_per_group_mention; usernames.forEach((u) => (checked[u] = true)); return r; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 28c87c56cd2..25355a42600 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -477,7 +477,7 @@ class UsersController < ApplicationController usernames = params[:usernames] if params[:usernames].present? usernames = [params[:username]] if params[:username].present? - raise Discourse::InvalidParameters.new(:usernames) if !usernames.kind_of?(Array) + raise Discourse::InvalidParameters.new(:usernames) if !usernames.kind_of?(Array) || usernames.size > 20 groups = Group.where(name: usernames).pluck(:name) mentionable_groups = @@ -496,15 +496,53 @@ class UsersController < ApplicationController usernames -= groups usernames.each(&:downcase!) - cannot_see = [] + users = User + .where(staged: false, username_lower: usernames) + .index_by(&:username_lower) + + cannot_see = {} here_count = nil topic_id = params[:topic_id] if topic_id.present? && topic = Topic.find_by(id: topic_id) + topic_muted_by = TopicUser + .where(topic: topic) + .where(user_id: users.values.map(&:id)) + .where(notification_level: TopicUser.notification_levels[:muted]) + .pluck(:user_id) + .to_set + + if topic.private_message? + topic_allowed_user_ids = TopicAllowedUser + .where(topic: topic) + .where(user_id: users.values.map(&:id)) + .pluck(:user_id) + .to_set + + topic_allowed_group_ids = TopicAllowedGroup + .where(topic: topic) + .pluck(:group_id) + .to_set + end + usernames.each do |username| - if !Guardian.new(User.find_by_username(username)).can_see?(topic) - cannot_see.push(username) + user = users[username] + next if user.blank? + + cannot_see_reason = nil + if !user.guardian.can_see?(topic) + cannot_see_reason = topic.private_message? ? :private : :category + elsif topic_muted_by.include?(user.id) + cannot_see_reason = :muted_topic + elsif topic.private_message? && !topic_allowed_user_ids.include?(user.id) && !user.group_ids.any? { |group_id| topic_allowed_group_ids.include?(group_id) } + cannot_see_reason = :not_allowed end + + if !guardian.is_staff? && cannot_see_reason.present? && cannot_see_reason != :private && cannot_see_reason != :category + cannot_see_reason = nil # do not leak private information + end + + cannot_see[username] = cannot_see_reason if cannot_see_reason.present? end if usernames.include?(SiteSetting.here_mention) && guardian.can_mention_here? @@ -512,12 +550,8 @@ class UsersController < ApplicationController end end - result = User.where(staged: false) - .where(username_lower: usernames) - .pluck(:username_lower) - render json: { - valid: result, + valid: users.keys, valid_groups: groups, mentionable_groups: mentionable_groups, cannot_see: cannot_see, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 39707a5cb1c..72f2e25ad65 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2110,8 +2110,10 @@ en: one: "By mentioning %{group}, you are about to notify %{count} person – are you sure?" other: "By mentioning %{group}, you are about to notify %{count} people – are you sure?" cannot_see_mention: - category: "You mentioned %{username} but they won't be notified because they do not have access to this category. You will need to add them to a group that has access to this category." - private: "You mentioned %{username} but they won't be notified because they are unable to see this personal message. You will need to invite them to this PM." + category: "You mentioned @%{username} but they won't be notified because they do not have access to this category. You will need to add them to a group that has access to this category." + private: "You mentioned @%{username} but they won't be notified because they are unable to see this personal message. You will need to invite them to this personal message." + muted_topic: "You mentioned @%{username} but they won't be notified because they muted this topic." + not_allowed: "You mentioned @%{username} but they won't be notified because they were not invited to this topic." here_mention: one: "By mentioning @%{here}, you are about to notify %{count} user – are you sure?" other: "By mentioning @%{here}, you are about to notify %{count} users – are you sure?" diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 24126c533b8..e8e877528bc 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -3073,34 +3073,30 @@ describe UsersController do get "/u/is_local_username.json", params: { username: user1.username } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["valid"][0]).to eq(user1.username) + expect(response.parsed_body["valid"][0]).to eq(user1.username) end it "finds the group" do sign_in(user1) get "/u/is_local_username.json", params: { username: group.name } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["valid_groups"]).to include(group.name) - expect(json["mentionable_groups"].find { |g| g['name'] == group.name }).to be_present + expect(response.parsed_body["valid_groups"]).to include(group.name) + expect(response.parsed_body["mentionable_groups"].find { |g| g['name'] == group.name }).to be_present end it "finds unmentionable groups" do sign_in(user1) get "/u/is_local_username.json", params: { username: unmentionable.name } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["valid_groups"]).to include(unmentionable.name) - expect(json["mentionable_groups"]).to be_blank + expect(response.parsed_body["valid_groups"]).to include(unmentionable.name) + expect(response.parsed_body["mentionable_groups"]).to be_blank end it "supports multiples usernames" do get "/u/is_local_username.json", params: { usernames: [user1.username, "system"] } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["valid"].size).to eq(2) + expect(response.parsed_body["valid"]).to contain_exactly(user1.username, "system") end it "never includes staged accounts" do @@ -3109,8 +3105,7 @@ describe UsersController do get "/u/is_local_username.json", params: { usernames: [staged.username] } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["valid"].size).to eq(0) + expect(response.parsed_body["valid"]).to be_blank end it "returns user who cannot see topic" do @@ -3121,44 +3116,57 @@ describe UsersController do } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["cannot_see"].size).to eq(1) + expect(response.parsed_body["cannot_see"][user1.username]).to eq("category") end it "never returns a user who can see the topic" do - Guardian.any_instance.expects(:can_see?).with(topic).returns(true) - get "/u/is_local_username.json", params: { usernames: [user1.username], topic_id: topic.id } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["cannot_see"].size).to eq(0) + expect(response.parsed_body["cannot_see"]).to be_blank end it "returns user who cannot see a private topic" do - Guardian.any_instance.expects(:can_see?).with(private_topic).returns(false) - get "/u/is_local_username.json", params: { usernames: [user1.username], topic_id: private_topic.id } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["cannot_see"].size).to eq(1) + expect(response.parsed_body["cannot_see"][user1.username]).to eq("private") + end + + it "returns user who was not invited to topic" do + sign_in(Fabricate(:admin)) + + get "/u/is_local_username.json", params: { + usernames: [admin.username], topic_id: private_topic.id + } + + expect(response.status).to eq(200) + expect(response.parsed_body["cannot_see"][admin.username]).to eq("not_allowed") end it "never returns a user who can see the topic" do - Guardian.any_instance.expects(:can_see?).with(private_topic).returns(true) - get "/u/is_local_username.json", params: { usernames: [allowed_user.username], topic_id: private_topic.id } expect(response.status).to eq(200) - json = response.parsed_body - expect(json["cannot_see"].size).to eq(0) + expect(response.parsed_body["cannot_see"]).to be_blank + end + + it "returns the appropriate reason why user cannot see the topic" do + TopicUser.create!(user_id: user1.id, topic_id: topic.id, notification_level: TopicUser.notification_levels[:muted]) + + sign_in(admin) + get "/u/is_local_username.json", params: { + usernames: [user1.username], topic_id: topic.id + } + + expect(response.status).to eq(200) + expect(response.parsed_body["cannot_see"][user1.username]).to eq("muted_topic") end end