From 93859037ef776b83b9840e528a739050c600922c Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Mon, 5 Dec 2022 20:22:05 +0200 Subject: [PATCH] FEATURE: Improve composer warnings for mentions (#18796) * FEATURE: Show warning if group cannot be mentioned A similar warning is displayed when the user cannot be mentioned because they have not been invited to the topic. * FEATURE: Resolve mentions for new topic This commit improves several improvements and refactors /u/is_local_username route to a better /composer/mentions route that can handle new topics too. * FEATURE: Show warning if only some are notified Sometimes users are still notified even if the group that was mentioned was not invited to the message. This happens because its members were invited directly or are members of other groups that were invited. * DEV: Refactor _warnCannotSeeMention --- .../app/components/composer-editor.js | 86 +++--- .../discourse/app/controllers/composer.js | 83 +++--- .../discourse/app/lib/link-mentions.js | 164 ++++++------ .../acceptance/composer-image-preview-test.js | 10 +- .../acceptance/composer-messages-test.js | 59 +++- .../tests/acceptance/composer-test.js | 10 +- .../tests/helpers/create-pretender.js | 11 +- .../discourse/tests/helpers/qunit-helpers.js | 2 + .../components/composer-editor-test.js | 24 +- .../tests/unit/lib/link-mentions-test.js | 29 +- app/controllers/composer_controller.rb | 184 +++++++++++++ app/controllers/users_controller.rb | 87 ------ config/locales/client.en.yml | 6 + config/routes.rb | 2 +- spec/requests/composer_controller_spec.rb | 251 ++++++++++++++++++ spec/requests/users_controller_spec.rb | 110 -------- 16 files changed, 718 insertions(+), 400 deletions(-) create mode 100644 app/controllers/composer_controller.rb create mode 100644 spec/requests/composer_controller_spec.rb 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