From d379edec8d58cf011a2393b6569a639ae09c0e8a Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 21 Oct 2022 14:38:33 +1000 Subject: [PATCH] FIX: Clientside checks for personal_message_enabled_groups (#18691) The clientside allowPersonalMessages function introduced in e62e93f83a77adfa80b38fbfecf82bbee14e12fe sometimes did not work correctly, because the currentUser.groups property only contained **visible** groups for the current user, which could exclude auto groups that had their permissions set to be owner-only visible. It was unnecessary to add this anyway since we already have can_send_private_messages on the CurrentUserSerializer. It's better the backend does this calculation anyway. Use that in the clientside code instead and get rid of allowPersonalMessages --- .../app/components/sidebar/user/sections.js | 2 +- .../app/components/topic-footer-buttons.js | 5 +---- .../discourse/app/components/user-menu/menu.js | 2 +- .../javascripts/discourse/app/controllers/group.js | 4 ++-- .../app/controllers/keyboard-shortcuts-help.js | 2 +- .../app/controllers/preferences/notifications.js | 4 ++-- .../discourse/app/controllers/preferences/users.js | 4 ++-- .../javascripts/discourse/app/controllers/topic.js | 4 ++-- .../javascripts/discourse/app/controllers/user.js | 6 ++++-- .../discourse/app/lib/keyboard-shortcuts.js | 2 +- .../javascripts/discourse/app/models/user.js | 14 ++------------ .../javascripts/discourse/app/widgets/user-menu.js | 2 +- .../sidebar-user-messages-section-test.js | 13 ++++++++----- .../discourse/tests/fixtures/session-fixtures.js | 1 + .../integration/components/site-header-test.js | 1 + .../integration/components/user-menu/menu-test.js | 12 ++++++++---- .../components/widgets/user-menu-test.js | 13 ++++++++++--- 17 files changed, 48 insertions(+), 43 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/sections.js b/app/assets/javascripts/discourse/app/components/sidebar/user/sections.js index 41ed90a1b26..f835ad71ab1 100644 --- a/app/assets/javascripts/discourse/app/components/sidebar/user/sections.js +++ b/app/assets/javascripts/discourse/app/components/sidebar/user/sections.js @@ -24,6 +24,6 @@ export default class SidebarUserSections extends Component { } get enableMessagesSection() { - return this.currentUser?.allowPersonalMessages; + return this.currentUser?.can_send_private_messages; } } diff --git a/app/assets/javascripts/discourse/app/components/topic-footer-buttons.js b/app/assets/javascripts/discourse/app/components/topic-footer-buttons.js index e352343170b..1d5ff71b600 100644 --- a/app/assets/javascripts/discourse/app/components/topic-footer-buttons.js +++ b/app/assets/javascripts/discourse/app/components/topic-footer-buttons.js @@ -46,10 +46,7 @@ export default Component.extend({ return !isPM || this.canSendPms; }, - @discourseComputed("currentUser.allowPersonalMessages") - canSendPms() { - return this.currentUser?.allowPersonalMessages; - }, + canSendPms: alias("currentUser.can_send_private_messages"), canInviteTo: alias("topic.details.can_invite_to"), diff --git a/app/assets/javascripts/discourse/app/components/user-menu/menu.js b/app/assets/javascripts/discourse/app/components/user-menu/menu.js index b29188b2918..89ff33435ef 100644 --- a/app/assets/javascripts/discourse/app/components/user-menu/menu.js +++ b/app/assets/javascripts/discourse/app/components/user-menu/menu.js @@ -120,7 +120,7 @@ const CORE_TOP_TABS = [ } get shouldDisplay() { - return this.currentUser?.allowPersonalMessages; + return this.currentUser?.can_send_private_messages; } get notificationTypes() { diff --git a/app/assets/javascripts/discourse/app/controllers/group.js b/app/assets/javascripts/discourse/app/controllers/group.js index a847b619c20..2c2967981d2 100644 --- a/app/assets/javascripts/discourse/app/controllers/group.js +++ b/app/assets/javascripts/discourse/app/controllers/group.js @@ -91,10 +91,10 @@ export default Controller.extend({ @discourseComputed( "model.has_messages", "model.is_group_user", - "currentUser.allowPersonalMessages" + "currentUser.can_send_private_messages" ) showMessages(hasMessages, isGroupUser) { - if (!this.currentUser?.allowPersonalMessages) { + if (!this.currentUser?.can_send_private_messages) { return false; } diff --git a/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js index efc7ed5e9f4..359ab09d013 100644 --- a/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js +++ b/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js @@ -326,7 +326,7 @@ export default Controller.extend(ModalFunctionality, { bookmarks: buildShortcut("jump_to.bookmarks", { keys1: ["g", "b"] }), profile: buildShortcut("jump_to.profile", { keys1: ["g", "p"] }), }; - if (this.currentUser?.allowPersonalMessages) { + if (this.currentUser?.can_send_private_messages) { shortcuts.messages = buildShortcut("jump_to.messages", { keys1: ["g", "m"], }); diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js b/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js index 897c9fb30a6..6db87bbe846 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/notifications.js @@ -90,9 +90,9 @@ export default Controller.extend({ ]; }, - @discourseComputed("currentUser.allowPersonalMessages") + @discourseComputed("currentUser.can_send_private_messages") showMessageSettings() { - return this.currentUser?.allowPersonalMessages; + return this.currentUser?.can_send_private_messages; }, actions: { diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/users.js b/app/assets/javascripts/discourse/app/controllers/preferences/users.js index f20db86e7ba..f47f7efec3b 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/users.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/users.js @@ -72,9 +72,9 @@ export default Controller.extend({ return !allowPrivateMessages; }, - @discourseComputed("currentUser.allowPersonalMessages") + @discourseComputed("currentUser.can_send_private_messages") showMessageSettings() { - return this.currentUser?.allowPersonalMessages; + return this.currentUser?.can_send_private_messages; }, @action diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 7b1da2f6540..f682f8e68b1 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -212,9 +212,9 @@ export default Controller.extend(bufferedProperty("model"), { ); }, - @discourseComputed("currentUser.allowPersonalMessages") + @discourseComputed("currentUser.can_send_private_messages") canSendPms() { - return this.currentUser?.allowPersonalMessages; + return this.currentUser?.can_send_private_messages; }, @discourseComputed("buffered.category_id") diff --git a/app/assets/javascripts/discourse/app/controllers/user.js b/app/assets/javascripts/discourse/app/controllers/user.js index 35d086ecb7e..c46e2c34868 100644 --- a/app/assets/javascripts/discourse/app/controllers/user.js +++ b/app/assets/javascripts/discourse/app/controllers/user.js @@ -112,10 +112,12 @@ export default Controller.extend(CanCheckEmails, { @discourseComputed( "viewingSelf", "currentUser.admin", - "currentUser.allowPersonalMessages" + "currentUser.can_send_private_messages" ) showPrivateMessages(viewingSelf, isAdmin) { - return this.currentUser?.allowPersonalMessages && (viewingSelf || isAdmin); + return ( + this.currentUser?.can_send_private_messages && (viewingSelf || isAdmin) + ); }, @discourseComputed("viewingSelf", "currentUser.admin") diff --git a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js index f6b8d156eff..e5b75854665 100644 --- a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js +++ b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js @@ -140,7 +140,7 @@ export default { this.site = this.container.lookup("service:site"); // Disable the shortcut if private messages are disabled - if (!this.currentUser?.allowPersonalMessages) { + if (!this.currentUser?.can_send_private_messages) { delete DEFAULT_BINDINGS["g m"]; } }, diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index 352aa1e09f0..80b093c568b 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -640,6 +640,8 @@ const User = RestModel.extend({ return filteredGroups.length > numGroupsToDisplay; }, + // NOTE: This only includes groups *visible* to the user via the serializer, + // so be wary when using this. isInAnyGroups(groupIds) { if (!this.groups) { return; @@ -1094,18 +1096,6 @@ const User = RestModel.extend({ return [...trackedTags, ...watchedTags, ...watchingFirstPostTags]; }, - @discourseComputed("staff", "groups.[]") - allowPersonalMessages() { - return ( - this.staff || - this.isInAnyGroups( - this.siteSettings.personal_message_enabled_groups - .split("|") - .map((groupId) => parseInt(groupId, 10)) - ) - ); - }, - showPopup(options) { const popupTypes = Site.currentProp("onboarding_popup_types"); if (!popupTypes[options.id]) { diff --git a/app/assets/javascripts/discourse/app/widgets/user-menu.js b/app/assets/javascripts/discourse/app/widgets/user-menu.js index 89aaa688aaf..374b32d7a17 100644 --- a/app/assets/javascripts/discourse/app/widgets/user-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/user-menu.js @@ -156,7 +156,7 @@ createWidget("user-menu-links", { glyphs.push(this.bookmarksGlyph()); - if (this.currentUser?.allowPersonalMessages) { + if (this.currentUser?.can_send_private_messages) { glyphs.push(this.messagesGlyph()); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-messages-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-messages-section-test.js index 464d5422e8f..fb7c120e10a 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-messages-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-messages-section-test.js @@ -13,14 +13,17 @@ import { import { NotificationLevels } from "discourse/lib/notification-levels"; acceptance( - "Sidebar - Logged on user - Messages Section - user not in personal_message_enabled_groups", + "Sidebar - Logged on user - Messages Section - user does not have can_send_private_messages permission", function (needs) { - needs.user({ moderator: false, admin: false }); + needs.user({ + moderator: false, + admin: false, + can_send_private_messages: false, + }); needs.settings({ enable_experimental_sidebar_hamburger: true, enable_sidebar: true, - personal_message_enabled_groups: "13", // trust_level_3 auto group ID; }); test("clicking on section header button", async function (assert) { @@ -35,9 +38,9 @@ acceptance( ); acceptance( - "Sidebar - Logged on user - Messages Section - user in personal_message_enabled_groups", + "Sidebar - Logged on user - Messages Section - user does have can_send_private_messages permission", function (needs) { - needs.user(); + needs.user({ can_send_private_messages: true }); needs.settings({ enable_experimental_sidebar_hamburger: true, diff --git a/app/assets/javascripts/discourse/tests/fixtures/session-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/session-fixtures.js index d99b6ab8258..b5430580e8b 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/session-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/session-fixtures.js @@ -24,6 +24,7 @@ export default { trust_level: 4, can_edit: true, can_invite_to_forum: true, + can_send_private_messages: true, should_be_redirected_to_top: false, custom_fields: {}, muted_category_ids: [], diff --git a/app/assets/javascripts/discourse/tests/integration/components/site-header-test.js b/app/assets/javascripts/discourse/tests/integration/components/site-header-test.js index 620227d0613..3a8cac10f08 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/site-header-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/site-header-test.js @@ -144,6 +144,7 @@ module("Integration | Component | site-header", function (hooks) { test("arrow up/down keys move focus between the tabs", async function (assert) { this.currentUser.set("redesigned_user_menu_enabled", true); + this.currentUser.set("can_send_private_messages", true); await render(hbs``); await click(".header-dropdown-toggle.current-user"); let activeTab = query(".menu-tabs-container .btn.active"); diff --git a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js index 6b32f8099ad..14dbaaba8d2 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js @@ -36,6 +36,7 @@ module("Integration | Component | user-menu", function (hooks) { }); test("the menu has a group of tabs at the top", async function (assert) { + this.currentUser.set("can_send_private_messages", true); await render(template); const tabs = queryAll(".top-tabs.tabs-list .btn"); assert.strictEqual(tabs.length, 6); @@ -52,6 +53,7 @@ module("Integration | Component | user-menu", function (hooks) { }); test("the menu has a group of tabs at the bottom", async function (assert) { + this.currentUser.set("can_send_private_messages", true); await render(template); const tabs = queryAll(".bottom-tabs.tabs-list .btn"); assert.strictEqual(tabs.length, 1); @@ -63,6 +65,7 @@ module("Integration | Component | user-menu", function (hooks) { test("likes tab is hidden if current user's like notifications frequency is 'never'", async function (assert) { this.currentUser.set("likes_notifications_disabled", true); + this.currentUser.set("can_send_private_messages", true); await render(template); assert.ok(!exists("#user-menu-button-likes")); @@ -79,6 +82,7 @@ module("Integration | Component | user-menu", function (hooks) { test("reviewables tab is shown if current user can review and there are pending reviewables", async function (assert) { this.currentUser.set("can_review", true); this.currentUser.set("reviewable_count", 1); + this.currentUser.set("can_send_private_messages", true); await render(template); const tab = query("#user-menu-button-review-queue"); assert.strictEqual(tab.dataset.tabNumber, "5"); @@ -100,11 +104,11 @@ module("Integration | Component | user-menu", function (hooks) { assert.notOk(exists("#user-menu-button-review-queue")); }); - test("messages tab isn't shown if current user isn't staff and user does not belong to personal_message_enabled_groups", async function (assert) { + test("messages tab isn't shown if current user does not have can_send_private_messages permission", async function (assert) { this.currentUser.set("moderator", false); this.currentUser.set("admin", false); this.currentUser.set("groups", []); - this.siteSettings.personal_message_enabled_groups = "13"; // trust_level_3 auto group ID; + this.currentUser.set("can_send_private_messages", false); await render(template); @@ -120,11 +124,11 @@ module("Integration | Component | user-menu", function (hooks) { ); }); - test("messages tab is shown if current user is staff even if they do not belong to personal_message_enabled_groups", async function (assert) { + test("messages tab is shown if user has can_send_private_messages permission", async function (assert) { this.currentUser.set("moderator", true); this.currentUser.set("admin", false); this.currentUser.set("groups", []); - this.siteSettings.personal_message_enabled_groups = "999"; + this.currentUser.set("can_send_private_messages", true); await render(template); diff --git a/app/assets/javascripts/discourse/tests/integration/components/widgets/user-menu-test.js b/app/assets/javascripts/discourse/tests/integration/components/widgets/user-menu-test.js index a3e717edc90..5290dd6628f 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/widgets/user-menu-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/widgets/user-menu-test.js @@ -92,8 +92,11 @@ module("Integration | Component | Widget | user-menu", function (hooks) { }); test("private messages - disabled", async function (assert) { - this.currentUser.setProperties({ admin: false, moderator: false }); - this.siteSettings.personal_message_enabled_groups = "13"; // trust_level_3 auto group ID; + this.currentUser.setProperties({ + admin: false, + moderator: false, + can_send_private_messages: false, + }); await render(hbs``); @@ -101,7 +104,11 @@ module("Integration | Component | Widget | user-menu", function (hooks) { }); test("private messages - enabled", async function (assert) { - this.siteSettings.personal_message_enabled_groups = "11"; // trust_level_1 auto group ID; + this.currentUser.setProperties({ + admin: false, + moderator: false, + can_send_private_messages: true, + }); await render(hbs``);