diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index f08549f3977..2c8cd343389 100644 --- a/app/assets/javascripts/discourse/app/components/composer-editor.js +++ b/app/assets/javascripts/discourse/app/components/composer-editor.js @@ -23,7 +23,6 @@ import { linkSeenHashtagsInContext, } from "discourse/lib/hashtag-autocomplete"; import { - cannotSee, fetchUnseenMentions, linkSeenMentions, } from "discourse/lib/link-mentions"; @@ -126,6 +125,7 @@ export default Component.extend(ComposerUploadUppy, { init() { this._super(...arguments); this.warnedCannotSeeMentions = []; + this.warnedGroupMentions = []; }, @discourseComputed("composer.requiredCategoryMissing") @@ -474,13 +474,15 @@ export default Component.extend(ComposerUploadUppy, { }, _renderUnseenMentions(preview, unseen) { - // 'Create a New Topic' scenario is not supported (per conversation with codinghorror) - // https://meta.discourse.org/t/taking-another-1-7-release-task/51986/7 - fetchUnseenMentions(unseen, this.get("composer.topic.id")).then((r) => { + fetchUnseenMentions({ + names: unseen, + topicId: this.get("composer.topic.id"), + allowedNames: this.get("composer.targetRecipients")?.split(","), + }).then((response) => { linkSeenMentions(preview, this.siteSettings); this._warnMentionedGroups(preview); this._warnCannotSeeMention(preview); - this._warnHereMention(r.here_count); + this._warnHereMention(response.here_count); }); }, @@ -506,28 +508,27 @@ export default Component.extend(ComposerUploadUppy, { } }, + @debounce(2000) _warnMentionedGroups(preview) { schedule("afterRender", () => { - let found = this.warnedGroupMentions || []; - preview?.querySelectorAll(".mention-group.notify")?.forEach((mention) => { - if (this._isInQuote(mention)) { - return; - } + preview + .querySelectorAll(".mention-group[data-mentionable-user-count]") + .forEach((mention) => { + const { name } = mention.dataset; + if ( + this.warnedGroupMentions.includes(name) || + this._isInQuote(mention) + ) { + return; + } - let name = mention.dataset.name; - if (!found.includes(name)) { - this.groupsMentioned([ - { - name, - user_count: mention.dataset.mentionableUserCount, - max_mentions: mention.dataset.maxMentions, - }, - ]); - found.push(name); - } - }); - - this.set("warnedGroupMentions", found); + this.warnedGroupMentions.push(name); + this.groupsMentioned({ + name, + userCount: mention.dataset.mentionableUserCount, + maxMentions: mention.dataset.maxMentions, + }); + }); }); }, @@ -539,22 +540,35 @@ export default Component.extend(ComposerUploadUppy, { return; } - const warnings = []; - - preview.querySelectorAll(".mention.cannot-see").forEach((mention) => { + preview.querySelectorAll(".mention[data-reason]").forEach((mention) => { const { name } = mention.dataset; - if (this.warnedCannotSeeMentions.includes(name)) { return; } this.warnedCannotSeeMentions.push(name); - warnings.push({ name, reason: cannotSee[name] }); + this.cannotSeeMention({ + name, + reason: mention.dataset.reason, + }); }); - if (warnings.length > 0) { - this.cannotSeeMention(warnings); - } + preview + .querySelectorAll(".mention-group[data-reason]") + .forEach((mention) => { + const { name } = mention.dataset; + if (this.warnedCannotSeeMentions.includes(name)) { + return; + } + + this.warnedCannotSeeMentions.push(name); + this.cannotSeeMention({ + name, + reason: mention.dataset.reason, + notifiedCount: mention.dataset.notifiedUserCount, + isGroup: true, + }); + }); }, _warnHereMention(hereCount) { @@ -562,13 +576,7 @@ export default Component.extend(ComposerUploadUppy, { return; } - discourseLater( - this, - () => { - this.hereMention(hereCount); - }, - 2000 - ); + this.hereMention(hereCount); }, @bind diff --git a/app/assets/javascripts/discourse/app/controllers/composer.js b/app/assets/javascripts/discourse/app/controllers/composer.js index d62d80bf7c1..54aa7b54515 100644 --- a/app/assets/javascripts/discourse/app/controllers/composer.js +++ b/app/assets/javascripts/discourse/app/controllers/composer.js @@ -782,51 +782,62 @@ export default Controller.extend({ } }, - groupsMentioned(groups) { + groupsMentioned({ name, userCount, maxMentions }) { if ( - !this.get("model.creatingPrivateMessage") && - !this.get("model.topic.isPrivateMessage") + this.get("model.creatingPrivateMessage") || + this.get("model.topic.isPrivateMessage") ) { - groups.forEach((group) => { - let body; - const groupLink = getURL(`/g/${group.name}/members`); - const maxMentions = parseInt(group.max_mentions, 10); - const userCount = parseInt(group.user_count, 10); + return; + } - if (maxMentions < userCount) { - body = I18n.t("composer.group_mentioned_limit", { - group: `@${group.name}`, - count: maxMentions, - group_link: groupLink, - }); - } else if (group.user_count > 0) { - body = I18n.t("composer.group_mentioned", { - group: `@${group.name}`, - count: userCount, - group_link: groupLink, - }); - } + maxMentions = parseInt(maxMentions, 10); + userCount = parseInt(userCount, 10); - if (body) { - this.appEvents.trigger("composer-messages:create", { - extraClass: "custom-body", - templateName: "education", - body, - }); - } + let body; + const groupLink = getURL(`/g/${name}/members`); + + if (userCount > maxMentions) { + body = I18n.t("composer.group_mentioned_limit", { + group: `@${name}`, + count: maxMentions, + group_link: groupLink, + }); + } else if (userCount > 0) { + body = I18n.t("composer.group_mentioned", { + group: `@${name}`, + count: userCount, + group_link: groupLink, + }); + } + + if (body) { + this.appEvents.trigger("composer-messages:create", { + extraClass: "custom-body", + templateName: "education", + body, }); } }, - cannotSeeMention(mentions) { - mentions.forEach((mention) => { - this.appEvents.trigger("composer-messages:create", { - extraClass: "custom-body", - templateName: "education", - body: I18n.t(`composer.cannot_see_mention.${mention.reason}`, { - username: mention.name, - }), + cannotSeeMention({ name, reason, notifiedCount, isGroup }) { + notifiedCount = parseInt(notifiedCount, 10); + + let body; + if (isGroup) { + body = I18n.t(`composer.cannot_see_group_mention.${reason}`, { + group: name, + count: notifiedCount, }); + } else { + body = I18n.t(`composer.cannot_see_mention.${reason}`, { + username: name, + }); + } + + this.appEvents.trigger("composer-messages:create", { + extraClass: "custom-body", + templateName: "education", + body, }); }, diff --git a/app/assets/javascripts/discourse/app/lib/link-mentions.js b/app/assets/javascripts/discourse/app/lib/link-mentions.js index 0af0ef6b443..6757a9f258d 100644 --- a/app/assets/javascripts/discourse/app/lib/link-mentions.js +++ b/app/assets/javascripts/discourse/app/lib/link-mentions.js @@ -1,112 +1,110 @@ -import deprecated from "discourse-common/lib/deprecated"; -import { ajax } from "discourse/lib/ajax"; -import { formatUsername } from "discourse/lib/utilities"; import getURL from "discourse-common/lib/get-url"; +import { ajax } from "discourse/lib/ajax"; import { userPath } from "discourse/lib/url"; -import jQuery from "jquery"; +import { formatUsername } from "discourse/lib/utilities"; +let checked = {}; +let foundUsers = {}; +let userReasons = {}; +let foundGroups = {}; +let groupReasons = {}; let maxGroupMention; -function replaceSpan(element, username, opts) { - let extra = {}; - let extraClass = []; +export function resetMentions() { + checked = {}; + foundUsers = {}; + userReasons = {}; + foundGroups = {}; + groupReasons = {}; + maxGroupMention = null; +} + +function replaceSpan(element, name, opts) { const a = document.createElement("a"); - if (opts && opts.group) { - if (opts.mentionable) { - extra = { - name: username, - mentionableUserCount: opts.mentionable.user_count, - maxMentions: maxGroupMention, - }; - extraClass.push("notify"); - } + if (opts.group) { + a.href = getURL(`/g/${name}`); + a.innerText = `@${name}`; + a.classList.add("mention-group"); - a.setAttribute("href", getURL("/g/") + username); - a.classList.add("mention-group", ...extraClass); - a.innerText = `@${username}`; + if (!opts.reason && opts.details) { + a.dataset.mentionableUserCount = opts.details.user_count; + a.dataset.maxMentions = maxGroupMention; + } } else { - if (opts && opts.cannot_see) { - extra = { name: username }; - extraClass.push("cannot-see"); - } - - a.href = userPath(username.toLowerCase()); - a.classList.add("mention", ...extraClass); - a.innerText = `@${formatUsername(username)}`; + a.href = userPath(name.toLowerCase()); + a.innerText = `@${formatUsername(name)}`; + a.classList.add("mention"); } - Object.keys(extra).forEach((key) => { - a.dataset[key] = extra[key]; - }); + a.dataset.name = name; + if (opts.reason) { + a.dataset.reason = opts.reason; + + if (opts.details) { + a.dataset.notifiedUserCount = opts.details.notified_count; + } + } element.replaceWith(a); } -const found = {}; -const foundGroups = {}; -const mentionableGroups = {}; -const checked = {}; -export const cannotSee = {}; - -function updateFound(mentions, usernames) { +function updateFound(mentions, names) { mentions.forEach((mention, index) => { - const username = usernames[index]; - if (found[username.toLowerCase()]) { - replaceSpan(mention, username, { cannot_see: cannotSee[username] }); - } else if (mentionableGroups[username]) { - replaceSpan(mention, username, { - group: true, - mentionable: mentionableGroups[username], + const name = names[index]; + if (foundUsers[name.toLowerCase()]) { + replaceSpan(mention, name, { + reason: userReasons[name], }); - } else if (foundGroups[username]) { - replaceSpan(mention, username, { group: true }); - } else if (checked[username]) { + } else if (foundGroups[name]) { + replaceSpan(mention, name, { + group: true, + details: foundGroups[name], + reason: groupReasons[name], + }); + } else if (checked[name]) { mention.classList.add("mention-tested"); } }); } -export function linkSeenMentions(elem, siteSettings) { - if (elem instanceof jQuery) { - elem = elem[0]; - - deprecated("linkSeenMentions now expects a DOM node as first parameter", { - since: "2.8.0.beta7", - dropFrom: "2.9.0.beta1", - id: "discourse.link-mentions.dom-node", - }); - } - +export function linkSeenMentions(element, siteSettings) { const mentions = [ - ...elem.querySelectorAll("span.mention:not(.mention-tested)"), + ...element.querySelectorAll("span.mention:not(.mention-tested)"), ]; - if (mentions.length) { - const usernames = mentions.map((m) => m.innerText.slice(1)); - updateFound(mentions, usernames); - return usernames - .uniq() - .filter( - (u) => !checked[u] && u.length >= siteSettings.min_username_length - ); + + if (mentions.length === 0) { + return []; } - return []; + + const names = mentions.map((mention) => mention.innerText.slice(1)); + updateFound(mentions, names); + + return names + .uniq() + .filter( + (name) => + !checked[name] && name.length >= siteSettings.min_username_length + ); } -// 'Create a New Topic' scenario is not supported (per conversation with codinghorror) -// https://meta.discourse.org/t/taking-another-1-7-release-task/51986/7 -export function fetchUnseenMentions(usernames, topic_id) { - return ajax(userPath("is_local_username"), { - data: { usernames, topic_id }, - }).then((r) => { - r.valid.forEach((v) => (found[v] = true)); - r.valid_groups.forEach((vg) => (foundGroups[vg] = true)); - r.mentionable_groups.forEach((mg) => (mentionableGroups[mg.name] = mg)); - 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; +export async function fetchUnseenMentions({ names, topicId, allowedNames }) { + const response = await ajax("/composer/mentions", { + data: { names, topic_id: topicId, allowed_names: allowedNames }, }); + + names.forEach((name) => (checked[name] = true)); + response.users.forEach((username) => (foundUsers[username] = true)); + Object.entries(response.user_reasons).forEach( + ([username, reason]) => (userReasons[username] = reason) + ); + Object.entries(response.groups).forEach( + ([name, details]) => (foundGroups[name] = details) + ); + Object.entries(response.group_reasons).forEach( + ([name, reason]) => (groupReasons[name] = reason) + ); + maxGroupMention = response.max_users_notified_per_group_mention; + + return response; } diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-image-preview-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-image-preview-test.js index 75103a3c9ba..c70bd81d921 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-image-preview-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-image-preview-test.js @@ -20,12 +20,12 @@ acceptance("Composer - Image Preview", function (needs) { server.get("/posts/419", () => { return helper.response({ id: 419 }); }); - server.get("/u/is_local_username", () => { + server.get("/composer/mentions", () => { return helper.response({ - valid: [], - valid_groups: ["staff"], - mentionable_groups: [{ name: "staff", user_count: 30 }], - cannot_see: [], + users: [], + user_reasons: {}, + groups: { staff: { user_count: 30 } }, + group_reasons: {}, max_users_notified_per_group_mention: 100, }); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js index 25f9cae229e..8d78ac5114a 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-messages-test.js @@ -3,7 +3,7 @@ import { exists, query, } from "discourse/tests/helpers/qunit-helpers"; -import { click, triggerKeyEvent, visit } from "@ember/test-helpers"; +import { click, fillIn, triggerKeyEvent, visit } from "@ember/test-helpers"; import { test } from "qunit"; import I18n from "I18n"; @@ -39,3 +39,60 @@ acceptance("Composer - Messages", function (needs) { ); }); }); + +acceptance("Composer - Messages - Cannot see group", function (needs) { + needs.user(); + needs.pretender((server, helper) => { + server.get("/composer/mentions", () => { + return helper.response({ + users: [], + user_reasons: {}, + groups: { + staff: { user_count: 30 }, + staff2: { user_count: 30, notified_count: 10 }, + }, + group_reasons: { staff: "not_allowed", staff2: "some_not_allowed" }, + max_users_notified_per_group_mention: 100, + }); + }); + }); + + test("Shows warning in composer if group hasn't been invited", async function (assert) { + await visit("/t/130"); + await click("button.create"); + assert.ok( + !exists(".composer-popup"), + "composer warning is not shown by default" + ); + await fillIn(".d-editor-input", "Mention @staff"); + assert.ok(exists(".composer-popup"), "shows composer warning message"); + assert.ok( + query(".composer-popup").innerHTML.includes( + I18n.t("composer.cannot_see_group_mention.not_allowed", { + group: "staff", + }) + ), + "warning message has correct body" + ); + }); + + test("Shows warning in composer if group hasn't been invited, but some members have access already", async function (assert) { + await visit("/t/130"); + await click("button.create"); + assert.ok( + !exists(".composer-popup"), + "composer warning is not shown by default" + ); + await fillIn(".d-editor-input", "Mention @staff2"); + assert.ok(exists(".composer-popup"), "shows composer warning message"); + assert.ok( + query(".composer-popup").innerHTML.includes( + I18n.t("composer.cannot_see_group_mention.some_not_allowed", { + group: "staff2", + count: 10, + }) + ), + "warning message has correct body" + ); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js index b250106bd91..9637fd77d29 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-test.js @@ -70,12 +70,12 @@ acceptance("Composer", function (needs) { server.get("/posts/419", () => { return helper.response({ id: 419 }); }); - server.get("/u/is_local_username", () => { + server.get("/composer/mentions", () => { return helper.response({ - valid: [], - valid_groups: ["staff"], - mentionable_groups: [{ name: "staff", user_count: 30 }], - cannot_see: [], + users: [], + user_reasons: {}, + groups: { staff: { user_count: 30 } }, + group_reasons: {}, max_users_notified_per_group_mention: 100, }); }); diff --git a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js index da7ab2e5f74..d694826cf52 100644 --- a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js +++ b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js @@ -173,12 +173,13 @@ export function applyDefaultHandlers(pretender) { return response({ email: "eviltrout@example.com" }); }); - pretender.get("/u/is_local_username", () => + pretender.get("/composer/mentions", () => response({ - valid: [], - valid_groups: [], - mentionable_groups: [], - cannot_see: [], + users: [], + user_reasons: {}, + groups: {}, + group_reasons: {}, + max_users_notified_per_group_mention: 100, }) ); diff --git a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js index 06b4062601b..b4d49f2f6dc 100644 --- a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js +++ b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js @@ -75,6 +75,7 @@ import { resetSidebarSection } from "discourse/lib/sidebar/custom-sections"; import { resetNotificationTypeRenderers } from "discourse/lib/notification-types-manager"; import { resetUserMenuTabs } from "discourse/lib/user-menu/tab"; import { reset as resetLinkLookup } from "discourse/lib/link-lookup"; +import { resetMentions } from "discourse/lib/link-mentions"; import { resetModelTransformers } from "discourse/lib/model-transformers"; import { cleanupTemporaryModuleRegistrations } from "./temporary-module-helper"; @@ -208,6 +209,7 @@ export function testCleanup(container, app) { resetUserMenuTabs(); resetLinkLookup(); resetModelTransformers(); + resetMentions(); cleanupTemporaryModuleRegistrations(); } diff --git a/app/assets/javascripts/discourse/tests/integration/components/composer-editor-test.js b/app/assets/javascripts/discourse/tests/integration/components/composer-editor-test.js index 6bd6016b72a..28405f9cd36 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/composer-editor-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/composer-editor-test.js @@ -8,26 +8,28 @@ module("Integration | Component | ComposerEditor", function (hooks) { setupRenderingTest(hooks); test("warns about users that will not see a mention", async function (assert) { - assert.expect(1); + assert.expect(2); this.set("model", {}); this.set("noop", () => {}); - this.set("expectation", (warnings) => { - assert.deepEqual(warnings, [ - { name: "user-no", reason: "a reason" }, - { name: "user-nope", reason: "a reason" }, - ]); + this.set("expectation", (warning) => { + if (warning.name === "user-no") { + assert.deepEqual(warning, { name: "user-no", reason: "a reason" }); + } else if (warning.name === "user-nope") { + assert.deepEqual(warning, { name: "user-nope", reason: "a reason" }); + } }); - pretender.get("/u/is_local_username", () => { + pretender.get("/composer/mentions", () => { return response({ - cannot_see: { + users: ["user-ok", "user-no", "user-nope"], + user_reasons: { "user-no": "a reason", "user-nope": "a reason", }, - mentionable_groups: [], - valid: ["user-ok", "user-no", "user-nope"], - valid_groups: [], + groups: {}, + group_reasons: {}, + max_users_notified_per_group_mention: 100, }); }); diff --git a/app/assets/javascripts/discourse/tests/unit/lib/link-mentions-test.js b/app/assets/javascripts/discourse/tests/unit/lib/link-mentions-test.js index 4eeb1f8d075..8aaf3779d79 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/link-mentions-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/link-mentions-test.js @@ -8,27 +8,22 @@ import domFromString from "discourse-common/lib/dom-from-string"; module("Unit | Utility | link-mentions", function () { test("linkSeenMentions replaces users and groups", async function (assert) { - pretender.get("/u/is_local_username", () => + pretender.get("/composer/mentions", () => response({ - valid: ["valid_user"], - valid_groups: ["valid_group"], - mentionable_groups: [ - { - name: "mentionable_group", - user_count: 1, - }, - ], - cannot_see: [], + users: ["valid_user"], + user_reasons: {}, + groups: { + valid_group: { user_count: 1 }, + mentionable_group: { user_count: 1 }, + }, + group_reasons: { valid_group: "not_mentionable" }, max_users_notified_per_group_mention: 100, }) ); - await fetchUnseenMentions([ - "valid_user", - "mentionable_group", - "valid_group", - "invalid", - ]); + await fetchUnseenMentions({ + names: ["valid_user", "mentionable_group", "valid_group", "invalid"], + }); const root = domFromString(`
@@ -43,7 +38,7 @@ module("Unit | Utility | link-mentions", function () { assert.strictEqual(root.querySelector("a").innerText, "@valid_user"); assert.strictEqual(root.querySelectorAll("a")[1].innerText, "@valid_group"); assert.strictEqual( - root.querySelector("a.notify").innerText, + root.querySelector("a[data-mentionable-user-count]").innerText, "@mentionable_group" ); assert.strictEqual( diff --git a/app/controllers/composer_controller.rb b/app/controllers/composer_controller.rb new file mode 100644 index 00000000000..fd4847e115a --- /dev/null +++ b/app/controllers/composer_controller.rb @@ -0,0 +1,184 @@ +# frozen_string_literal: true + +class ComposerController < ApplicationController + requires_login + + def mentions + @names = params.require(:names) + raise Discourse::InvalidParameters.new(:names) if !@names.kind_of?(Array) || @names.size > 20 + + if params[:topic_id].present? + @topic = Topic.find_by(id: params[:topic_id]) + guardian.ensure_can_see!(@topic) + end + + # allowed_names is necessary just for new private messages. + @allowed_names = if params[:allowed_names].present? + raise Discourse::InvalidParameters(:allowed_names) if !params[:allowed_names].is_a?(Array) + params[:allowed_names] << current_user.username + else + [] + end + + user_reasons = {} + group_reasons = {} + @names.each do |name| + if user = users[name] + reason = user_reason(user) + user_reasons[name] = reason if reason.present? + elsif group = groups[name] + reason = group_reason(group) + group_reasons[name] = reason if reason.present? + end + end + + if @topic && @names.include?(SiteSetting.here_mention) && guardian.can_mention_here? + here_count = PostAlerter.new.expand_here_mention(@topic.first_post, exclude_ids: [current_user.id]).size + end + + serialized_groups = groups.values.reduce({}) do |hash, group| + serialized_group = { user_count: group.user_count } + + if group_reasons[group.name] == :not_allowed && + members_visible_group_ids.include?(group.id) && + (@topic&.private_message? || @allowed_names.present?) + + # Find users that are notified already because they have been invited + # directly or via a group + notified_count = GroupUser + # invited directly + .where(user_id: topic_allowed_user_ids) + .or( + # invited via a group + GroupUser.where( + user_id: GroupUser.where(group_id: topic_allowed_group_ids).select(:user_id) + ) + ) + .where(group_id: group.id) + .select(:user_id).distinct.count + + if notified_count > 0 + group_reasons[group.name] = :some_not_allowed + serialized_group[:notified_count] = notified_count + end + end + + hash[group.name] = serialized_group + hash + end + + render json: { + users: users.keys, + user_reasons: user_reasons, + groups: serialized_groups, + group_reasons: group_reasons, + here_count: here_count, + max_users_notified_per_group_mention: SiteSetting.max_users_notified_per_group_mention, + } + end + + private + + def user_reason(user) + reason = if @topic && !user.guardian.can_see?(@topic) + @topic.private_message? ? :private : :category + elsif @allowed_names.present? && !is_user_allowed?(user, topic_allowed_user_ids, topic_allowed_group_ids) + # This would normally be handled by the previous if, but that does not work for new private messages. + :private + elsif topic_muted_by.include?(user.id) + :muted_topic + elsif @topic&.private_message? && !is_user_allowed?(user, topic_allowed_user_ids, topic_allowed_group_ids) + # Admins can see the topic, but they will not be mentioned if they were not invited. + :not_allowed + end + + # Regular users can see only basic information why the users cannot see the topic. + reason = nil if !guardian.is_staff? && reason != :private && reason != :category + + reason + end + + def group_reason(group) + if !mentionable_group_ids.include?(group.id) + :not_mentionable + elsif (@topic&.private_message? || @allowed_names.present?) && !topic_allowed_group_ids.include?(group.id) + :not_allowed + end + end + + def is_user_allowed?(user, user_ids, group_ids) + user_ids.include?(user.id) || user.group_ids.any? { |group_id| group_ids.include?(group_id) } + end + + def users + @users ||= User + .not_staged + .where(username_lower: @names.map(&:downcase)) + .index_by(&:username_lower) + end + + def groups + @groups ||= Group + .visible_groups(current_user) + .where('lower(name) IN (?)', @names.map(&:downcase)) + .index_by(&:name) + end + + def mentionable_group_ids + @mentionable_group_ids ||= Group + .mentionable(current_user, include_public: false) + .where(name: @names) + .pluck(:id) + .to_set + end + + def members_visible_group_ids + @members_visible_group_ids ||= Group + .members_visible_groups(current_user) + .where(name: @names) + .pluck(:id) + .to_set + end + + def topic_muted_by + @topic_muted_by ||= if @topic.present? + TopicUser + .where(topic: @topic) + .where(user_id: users.values.map(&:id)) + .where(notification_level: TopicUser.notification_levels[:muted]) + .pluck(:user_id) + .to_set + else + Set.new + end + end + + def topic_allowed_user_ids + @topic_allowed_user_ids ||= if @allowed_names.present? + User + .where(username_lower: @allowed_names.map(&:downcase)) + .pluck(:id) + .to_set + elsif @topic&.private_message? + TopicAllowedUser + .where(topic: @topic) + .pluck(:user_id) + .to_set + end + end + + def topic_allowed_group_ids + @topic_allowed_group_ids ||= if @allowed_names.present? + Group + .messageable(current_user) + .where(name: @allowed_names) + .pluck(:id) + .to_set + elsif @topic&.private_message? + TopicAllowedGroup + .where(topic: @topic) + .pluck(:group_id) + .to_set + end + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3875747d5e5..8dc4cca7e75 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -497,93 +497,6 @@ class UsersController < ApplicationController end end - def is_local_username - usernames = params[:usernames] if params[:usernames].present? - usernames = [params[:username]] if params[:username].present? - - raise Discourse::InvalidParameters.new(:usernames) if !usernames.kind_of?(Array) || usernames.size > 20 - - groups = Group.where(name: usernames).pluck(:name) - mentionable_groups = - if current_user - Group.mentionable(current_user, include_public: false) - .where(name: usernames) - .pluck(:name, :user_count) - .map do |name, user_count| - { - name: name, - user_count: user_count - } - end - end - - usernames -= groups - usernames.each(&:downcase!) - - 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| - 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? - here_count = PostAlerter.new.expand_here_mention(topic.first_post, exclude_ids: [current_user.id]).size - end - end - - render json: { - valid: users.keys, - valid_groups: groups, - mentionable_groups: mentionable_groups, - cannot_see: cannot_see, - here_count: here_count, - max_users_notified_per_group_mention: SiteSetting.max_users_notified_per_group_mention - } - end - def render_available_true render(json: { available: true }) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9f1f3d8d407..4e632641b1f 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2233,6 +2233,12 @@ en: 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." + cannot_see_group_mention: + not_mentionable: "You cannot mention group @%{group}." + some_not_allowed: + one: "You mentioned @%{group} but only %{count} member will be notified because the other members are unable to see this personal message. You will need to invite them to this personal message." + other: "You mentioned @%{group} but only %{count} members will be notified because the other members are unable to see this personal message. You will need to invite them to this personal message." + not_allowed: "You mentioned @%{group} but none of its members will be notified because they are unable to see this personal message. You will need to invite them to this personal message." 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/config/routes.rb b/config/routes.rb index 6bb1a357a59..3a557741405 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -393,6 +393,7 @@ Discourse::Application.routes.draw do post "session/2fa/test-action" => "session#test_second_factor_restricted_route" end get "session/scopes" => "session#scopes" + get "composer/mentions" => "composer#mentions" get "composer_messages" => "composer_messages#index" get "composer_messages/user_not_seen_in_a_while" => "composer_messages#user_not_seen_in_a_while" @@ -425,7 +426,6 @@ Discourse::Application.routes.draw do collection do get "check_username" get "check_email" - get "is_local_username" end end diff --git a/spec/requests/composer_controller_spec.rb b/spec/requests/composer_controller_spec.rb new file mode 100644 index 00000000000..ca67ce8a04a --- /dev/null +++ b/spec/requests/composer_controller_spec.rb @@ -0,0 +1,251 @@ +# frozen_string_literal: true + +RSpec.describe ComposerController do + describe '#mentions' do + fab!(:current_user) { Fabricate(:user) } + fab!(:user) { Fabricate(:user) } + + fab!(:group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone], mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:invisible_group) { Fabricate(:group, visibility_level: Group.visibility_levels[:owners]) } + fab!(:unmessageable_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:nobody], mentionable_level: Group::ALIAS_LEVELS[:everyone]) } + fab!(:unmentionable_group) { Fabricate(:group, messageable_level: Group::ALIAS_LEVELS[:everyone], mentionable_level: Group::ALIAS_LEVELS[:nobody]) } + + before do + sign_in(current_user) + end + + context 'without a topic' do + it 'finds mentions' do + get '/composer/mentions.json', params: { + names: [ + 'invaliduserorgroup', + user.username, + group.name, + invisible_group.name, + unmessageable_group.name, + unmentionable_group.name + ], + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['users']).to contain_exactly(user.username) + expect(response.parsed_body['user_reasons']).to eq({}) + + expect(response.parsed_body['groups']).to eq({ + group.name => { 'user_count' => group.user_count }, + unmessageable_group.name => { 'user_count' => unmessageable_group.user_count }, + unmentionable_group.name => { 'user_count' => unmentionable_group.user_count }, + }) + expect(response.parsed_body['group_reasons']).to eq({ + unmentionable_group.name => 'not_mentionable', + }) + + expect(response.parsed_body['max_users_notified_per_group_mention']) + .to eq(SiteSetting.max_users_notified_per_group_mention) + end + end + + context 'with a regular topic' do + fab!(:topic) { Fabricate(:topic) } + + it 'finds mentions' do + get '/composer/mentions.json', params: { + names: [ + 'invaliduserorgroup', + user.username, + group.name, + invisible_group.name, + unmessageable_group.name, + unmentionable_group.name + ], + topic_id: topic.id, + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['users']).to contain_exactly(user.username) + expect(response.parsed_body['user_reasons']).to eq({}) + + expect(response.parsed_body['groups']).to eq( + group.name => { 'user_count' => group.user_count }, + unmessageable_group.name => { 'user_count' => unmessageable_group.user_count }, + unmentionable_group.name => { 'user_count' => unmentionable_group.user_count }, + ) + expect(response.parsed_body['group_reasons']).to eq( + unmentionable_group.name => 'not_mentionable', + ) + + expect(response.parsed_body['max_users_notified_per_group_mention']) + .to eq(SiteSetting.max_users_notified_per_group_mention) + end + end + + context 'with a private message' do + fab!(:allowed_user) { Fabricate(:user) } + fab!(:topic) { Fabricate(:private_message_topic, user: allowed_user) } + + it 'does not work if topic is not visible' do + get '/composer/mentions.json', params: { + names: [allowed_user.username], topic_id: topic.id + } + + expect(response.status).to eq(403) + end + + it 'finds mentions' do + sign_in(allowed_user) + topic.invite_group(Discourse.system_user, unmentionable_group) + + get '/composer/mentions.json', params: { + names: [ + 'invaliduserorgroup', + user.username, + allowed_user.username, + group.name, + invisible_group.name, + unmessageable_group.name, + unmentionable_group.name + ], + topic_id: topic.id, + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['users']).to contain_exactly( + user.username, allowed_user.username + ) + expect(response.parsed_body['user_reasons']).to eq( + user.username => 'private' + ) + + expect(response.parsed_body['groups']).to eq( + group.name => { 'user_count' => group.user_count }, + unmessageable_group.name => { 'user_count' => unmessageable_group.user_count }, + unmentionable_group.name => { 'user_count' => unmentionable_group.user_count }, + ) + expect(response.parsed_body['group_reasons']).to eq( + group.name => 'not_allowed', + unmessageable_group.name => 'not_allowed', + unmentionable_group.name => 'not_mentionable', + ) + + expect(response.parsed_body['max_users_notified_per_group_mention']) + .to eq(SiteSetting.max_users_notified_per_group_mention) + end + + it 'returns notified_count' do + sign_in(allowed_user) + group.add(user) + topic.invite_group(Discourse.system_user, group) + + other_group = Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) + other_group.add(allowed_user) + other_group.add(user) + other_group.add(Fabricate(:user)) + + # Trying to mention other_group which has not been invited, but two of + # its members have been (allowed_user directly and user via group). + get '/composer/mentions.json', params: { + names: [other_group.name], + topic_id: topic.id, + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['groups']).to eq( + other_group.name => { 'user_count' => 3, 'notified_count' => 2 } + ) + expect(response.parsed_body['group_reasons']).to eq( + other_group.name => 'some_not_allowed' + ) + end + end + + context 'with a new private message' do + fab!(:allowed_user) { Fabricate(:user) } + + it 'finds mentions' do + get '/composer/mentions.json', params: { + names: [ + 'invaliduserorgroup', + user.username, + allowed_user.username, + group.name, + invisible_group.name, + unmessageable_group.name, + unmentionable_group.name + ], + allowed_names: [allowed_user.username, unmentionable_group.name], + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['users']).to contain_exactly( + user.username, allowed_user.username + ) + expect(response.parsed_body['user_reasons']).to eq( + user.username => 'private', + ) + + expect(response.parsed_body['groups']).to eq( + group.name => { 'user_count' => group.user_count }, + unmessageable_group.name => { 'user_count' => unmessageable_group.user_count }, + unmentionable_group.name => { 'user_count' => unmentionable_group.user_count }, + ) + expect(response.parsed_body['group_reasons']).to eq( + group.name => 'not_allowed', + unmessageable_group.name => 'not_allowed', + unmentionable_group.name => 'not_mentionable', + ) + + expect(response.parsed_body['max_users_notified_per_group_mention']) + .to eq(SiteSetting.max_users_notified_per_group_mention) + end + + it 'returns notified_count' do + sign_in(allowed_user) + group.add(user) + + other_group = Fabricate(:group, mentionable_level: Group::ALIAS_LEVELS[:everyone]) + other_group.add(allowed_user) + other_group.add(user) + other_group.add(Fabricate(:user)) + + # Trying to mention other_group which has not been invited, but two of + # its members have been (allowed_user directly and user via group). + get '/composer/mentions.json', params: { + names: [other_group.name], + allowed_names: [allowed_user.username, group.name], + } + + expect(response.status).to eq(200) + + expect(response.parsed_body['groups']).to eq( + other_group.name => { 'user_count' => 3, 'notified_count' => 2 } + ) + expect(response.parsed_body['group_reasons']).to eq( + other_group.name => 'some_not_allowed' + ) + end + end + + context 'with an invalid topic' do + it 'returns an error' do + get '/composer/mentions.json', params: { + names: [ + 'invaliduserorgroup', + user.username, + group.name, + invisible_group.name, + unmessageable_group.name, + unmentionable_group.name + ], + topic_id: -1, + } + + expect(response.status).to eq(403) + end + end + end +end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 1d926afff87..267bb80ce10 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -3670,116 +3670,6 @@ RSpec.describe UsersController do end end - describe '#is_local_username' do - fab!(:group) { Fabricate(:group, name: "Discourse", mentionable_level: Group::ALIAS_LEVELS[:everyone]) } - let(:unmentionable) { - Fabricate(:group, name: "Unmentionable", mentionable_level: Group::ALIAS_LEVELS[:nobody]) - } - fab!(:topic) { Fabricate(:topic) } - fab!(:allowed_user) { Fabricate(:user) } - fab!(:private_topic) { Fabricate(:private_message_topic, user: allowed_user) } - - it "finds the user" do - get "/u/is_local_username.json", params: { username: user1.username } - - expect(response.status).to eq(200) - 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) - 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) - 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) - expect(response.parsed_body["valid"]).to contain_exactly(user1.username, "system") - end - - it "never includes staged accounts" do - staged = Fabricate(:user, staged: true) - - get "/u/is_local_username.json", params: { usernames: [staged.username] } - - expect(response.status).to eq(200) - expect(response.parsed_body["valid"]).to be_blank - end - - it "returns user who cannot see topic" do - Guardian.any_instance.expects(:can_see?).with(topic).returns(false) - - 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("category") - end - - it "never returns a user who can see the topic" do - 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"]).to be_blank - end - - it "returns user who cannot see a private topic" do - get "/u/is_local_username.json", params: { - usernames: [user1.username], topic_id: private_topic.id - } - - expect(response.status).to eq(200) - 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 - get "/u/is_local_username.json", params: { - usernames: [allowed_user.username], topic_id: private_topic.id - } - - expect(response.status).to eq(200) - 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 - describe '#topic_tracking_state' do context 'when anon' do it "raises an error on anon for topic_tracking_state" do