From 3cc73cfd1ed1f9054db4097c151a16db1537198d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 9 Feb 2024 12:52:22 +1000 Subject: [PATCH] FIX: Always preload admin plugin list for admin in sidebar (#25606) When we show the links to installed plugins in the admin sidebar (for plugins that have custom admin routes) we were previously only doing this if you opened /admin, not if you navigated there from the main forum. We should just always preload this data if the user is admin. This commit also changes `admin_sidebar_enabled_groups` to not be sent to the client as part of ongoing efforts to not check groups on the client, since not all a user's groups may be serialized. --- .../admin/addon/controllers/admin.js | 7 +--- .../admin/addon/routes/admin-revamp.js | 7 +--- .../javascripts/admin/addon/routes/admin.js | 14 +------ .../app/lib/sidebar/admin-sidebar.js | 4 +- .../admin-revamp-section-link.js | 8 +--- .../acceptance/admin-sidebar-section-test.js | 10 +---- app/controllers/admin/admin_controller.rb | 9 ----- app/controllers/admin/plugins_controller.rb | 9 ----- app/controllers/application_controller.rb | 16 ++++---- app/serializers/current_user_serializer.rb | 12 +++++- config/site_settings.yml | 1 - .../requests/application_controller_spec.rb | 37 +++++++++++++++++++ spec/requests/application_controller_spec.rb | 6 +++ 13 files changed, 73 insertions(+), 67 deletions(-) create mode 100644 plugins/chat/spec/requests/application_controller_spec.rb diff --git a/app/assets/javascripts/admin/addon/controllers/admin.js b/app/assets/javascripts/admin/addon/controllers/admin.js index 8b9e280aeb0..970091a2591 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin.js +++ b/app/assets/javascripts/admin/addon/controllers/admin.js @@ -7,12 +7,9 @@ export default class AdminController extends Controller { @service router; @service currentUser; - @discourseComputed("siteSettings.admin_sidebar_enabled_groups") + @discourseComputed("currentUser.use_admin_sidebar") showAdminSidebar() { - return this.siteSettings.userInAnyGroups( - "admin_sidebar_enabled_groups", - this.currentUser - ); + return this.currentUser.use_admin_sidebar; } @discourseComputed("siteSettings.enable_group_directory") diff --git a/app/assets/javascripts/admin/addon/routes/admin-revamp.js b/app/assets/javascripts/admin/addon/routes/admin-revamp.js index 5bd3536f7f8..b219bac5ef9 100644 --- a/app/assets/javascripts/admin/addon/routes/admin-revamp.js +++ b/app/assets/javascripts/admin/addon/routes/admin-revamp.js @@ -14,12 +14,7 @@ export default class AdminRoute extends DiscourseRoute { } activate() { - if ( - !this.siteSettings.userInAnyGroups( - "admin_sidebar_enabled_groups", - this.currentUser - ) - ) { + if (!this.currentUser.use_admin_sidebar) { return DiscourseURL.redirectTo("/admin"); } diff --git a/app/assets/javascripts/admin/addon/routes/admin.js b/app/assets/javascripts/admin/addon/routes/admin.js index 2a929982546..ae278472ac2 100644 --- a/app/assets/javascripts/admin/addon/routes/admin.js +++ b/app/assets/javascripts/admin/addon/routes/admin.js @@ -13,12 +13,7 @@ export default class AdminRoute extends DiscourseRoute { } activate() { - if ( - this.siteSettings.userInAnyGroups( - "admin_sidebar_enabled_groups", - this.currentUser - ) - ) { + if (this.currentUser.use_admin_sidebar) { this.sidebarState.setPanel(ADMIN_PANEL); this.sidebarState.setSeparatedMode(); this.sidebarState.hideSwitchPanelButtons(); @@ -32,12 +27,7 @@ export default class AdminRoute extends DiscourseRoute { deactivate(transition) { this.controllerFor("application").set("showTop", true); - if ( - this.siteSettings.userInAnyGroups( - "admin_sidebar_enabled_groups", - this.currentUser - ) - ) { + if (this.currentUser.use_admin_sidebar) { if (!transition?.to.name.startsWith("admin")) { this.sidebarState.setPanel(MAIN_PANEL); } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js index dcf7f455da8..1931c892b1f 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/admin-sidebar.js @@ -1,4 +1,3 @@ -import { isEmpty } from "@ember/utils"; import PreloadStore from "discourse/lib/preload-store"; import { ADMIN_NAV_MAP } from "discourse/lib/sidebar/admin-nav-map"; import BaseCustomSidebarPanel from "discourse/lib/sidebar/base-custom-sidebar-panel"; @@ -214,8 +213,9 @@ export default class AdminSidebarPanel extends BaseCustomSidebarPanel { hidden = true; get sections() { + const currentUser = getOwnerWithFallback().lookup("service:current-user"); const siteSettings = getOwnerWithFallback().lookup("service:site-settings"); - if (isEmpty(siteSettings.admin_sidebar_enabled_groups)) { + if (!currentUser.use_admin_sidebar) { return []; } diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/admin-revamp-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/admin-revamp-section-link.js index 575206d54b3..f7dad7dd980 100644 --- a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/admin-revamp-section-link.js +++ b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/admin-revamp-section-link.js @@ -29,13 +29,7 @@ export default class AdminRevampSectionLink extends BaseSectionLink { return false; } - return ( - this.currentUser.staff && - this.siteSettings.userInAnyGroups( - "admin_sidebar_enabled_groups", - this.currentUser - ) - ); + return this.currentUser.use_admin_sidebar; } get defaultPrefixValue() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-sidebar-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-sidebar-section-test.js index 3003dba58eb..74219fe8204 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-sidebar-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-sidebar-section-test.js @@ -9,10 +9,7 @@ acceptance("Admin Sidebar - Sections", function (needs) { needs.user({ admin: true, groups: [AUTO_GROUPS.admins], - }); - - needs.settings({ - admin_sidebar_enabled_groups: "1", + use_admin_sidebar: true, }); needs.hooks.beforeEach(() => { @@ -75,10 +72,7 @@ acceptance("Admin Sidebar - Sections - Plugin API", function (needs) { needs.user({ admin: true, groups: [AUTO_GROUPS.admins], - }); - - needs.settings({ - admin_sidebar_enabled_groups: "1", + use_admin_sidebar: true, }); needs.hooks.beforeEach(() => { diff --git a/app/controllers/admin/admin_controller.rb b/app/controllers/admin/admin_controller.rb index c7bd195359b..2220a511e77 100644 --- a/app/controllers/admin/admin_controller.rb +++ b/app/controllers/admin/admin_controller.rb @@ -7,13 +7,4 @@ class Admin::AdminController < ApplicationController def index render body: nil end - - private - - def preload_additional_json - store_preloaded( - "enabledPluginAdminRoutes", - MultiJson.dump(Discourse.plugins_sorted_by_name.filter_map(&:admin_route)), - ) - end end diff --git a/app/controllers/admin/plugins_controller.rb b/app/controllers/admin/plugins_controller.rb index c4e63a69949..b54ae21887c 100644 --- a/app/controllers/admin/plugins_controller.rb +++ b/app/controllers/admin/plugins_controller.rb @@ -8,13 +8,4 @@ class Admin::PluginsController < Admin::StaffController root: "plugins", ) end - - private - - def preload_additional_json - store_preloaded( - "enabledPluginAdminRoutes", - MultiJson.dump(Discourse.plugins_sorted_by_name.filter_map(&:admin_route)), - ) - end end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8e003950da6..049d10a0f0a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -446,8 +446,6 @@ class ApplicationController < ActionController::Base current_user.sync_notification_channel_position preload_current_user_data end - - preload_additional_json end def set_mobile_view @@ -669,12 +667,16 @@ class ApplicationController < ActionController::Base store_preloaded("topicTrackingStates", MultiJson.dump(hash[:data])) store_preloaded("topicTrackingStateMeta", MultiJson.dump(hash[:meta])) - # This is used in the wizard so we can preload fonts using the FontMap JS API. - store_preloaded("fontMap", MultiJson.dump(load_font_map)) if current_user.admin? - end + if current_user.admin? + # This is used in the wizard so we can preload fonts using the FontMap JS API. + store_preloaded("fontMap", MultiJson.dump(load_font_map)) - def preload_additional_json - # noop, should be defined by subcontrollers + # Used to show plugin-specific admin routes in the sidebar. + store_preloaded( + "enabledPluginAdminRoutes", + MultiJson.dump(Discourse.plugins_sorted_by_name.filter_map(&:admin_route)), + ) + end end def custom_html_json diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 4e852b58a23..bee43627766 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -69,7 +69,9 @@ class CurrentUserSerializer < BasicUserSerializer :sidebar_category_ids, :sidebar_sections, :new_new_view_enabled?, - :use_experimental_topic_bulk_actions? + :use_experimental_topic_bulk_actions?, + :use_experimental_topic_bulk_actions?, + :use_admin_sidebar delegate :user_stat, to: :object, private: true delegate :any_posts, :draft_count, :pending_posts_count, :read_faq?, to: :user_stat @@ -122,6 +124,14 @@ class CurrentUserSerializer < BasicUserSerializer scope.can_send_private_messages? end + def use_admin_sidebar + object.admin? && object.in_any_groups?(SiteSetting.admin_sidebar_enabled_groups_map) + end + + def include_user_admin_sidebar? + object.admin? + end + def can_post_anonymously SiteSetting.allow_anonymous_posting && (is_anonymous || object.in_any_groups?(SiteSetting.anonymous_posting_allowed_groups_map)) diff --git a/config/site_settings.yml b/config/site_settings.yml index dfec1cbca2f..cc868a6945b 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2332,7 +2332,6 @@ developer: hidden: true client: true admin_sidebar_enabled_groups: - client: true type: group_list list_type: compact default: "" diff --git a/plugins/chat/spec/requests/application_controller_spec.rb b/plugins/chat/spec/requests/application_controller_spec.rb new file mode 100644 index 00000000000..3313cf26b2f --- /dev/null +++ b/plugins/chat/spec/requests/application_controller_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe ApplicationController do + fab!(:user) + fab!(:admin) + + def preloaded_json + JSON.parse( + Nokogiri::HTML5.fragment(response.body).css("div#data-preloaded").first["data-preloaded"], + ) + end + + before do + SiteSetting.chat_enabled = true + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:trust_level_0] + end + + context "when user is admin" do + it "has correctly loaded preloaded data for enabledPluginAdminRoutes" do + sign_in(admin) + get "/latest" + expect(JSON.parse(preloaded_json["enabledPluginAdminRoutes"])).to include( + { "label" => "chat.admin.title", "location" => "chat" }, + ) + end + end + + context "when user is not admin" do + it "does not include preloaded data for enabledPluginAdminRoutes" do + sign_in(user) + get "/latest" + expect(preloaded_json["enabledPluginAdminRoutes"]).to eq(nil) + end + end +end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 6599a0bad04..c32894029a8 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -1308,6 +1308,7 @@ RSpec.describe ApplicationController do "topicTrackingStates", "topicTrackingStateMeta", "fontMap", + "enabledPluginAdminRoutes", ], ) end @@ -1320,6 +1321,11 @@ RSpec.describe ApplicationController do DiscourseFonts.fonts.filter { |f| f[:variants].present? }.map { |f| f[:key] }, ) end + + it "has correctly loaded enabledPluginAdminRoutes" do + get "/latest" + expect(JSON.parse(preloaded_json["enabledPluginAdminRoutes"])).to eq([]) + end end end end