From 2d5f323ca3f7d6e427df0abbd60678ebe73c8e02 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 30 Jul 2024 15:41:28 +1000 Subject: [PATCH] DEV: Move config area site setting fetch into new controller (#28136) Followup 4aea12fdcb21216a528451c0f8803e02dff24998 In certain config areas (like About) we want to be able to fetch specific site settings by name. In this case, sometimes we need to be able to fetch hidden settings, in cases where a config area is still experimental. Splitting out a different endpoint for this purpose allows us to be stricter with what we return for config areas without affecting the main site settings UI, revealing hidden settings before they are ready. --- .../admin/addon/routes/admin-config-about.js | 2 +- .../admin/config/site_settings_controller.rb | 27 ++++++++ .../admin/site_settings_controller.rb | 11 ---- config/routes.rb | 2 + .../config/site_settings_controller_spec.rb | 65 +++++++++++++++++++ .../admin/site_settings_controller_spec.rb | 25 +++---- 6 files changed, 103 insertions(+), 29 deletions(-) create mode 100644 app/controllers/admin/config/site_settings_controller.rb create mode 100644 spec/requests/admin/config/site_settings_controller_spec.rb diff --git a/app/assets/javascripts/admin/addon/routes/admin-config-about.js b/app/assets/javascripts/admin/addon/routes/admin-config-about.js index 9c60eedbb84..5eff95861ea 100644 --- a/app/assets/javascripts/admin/addon/routes/admin-config-about.js +++ b/app/assets/javascripts/admin/addon/routes/admin-config-about.js @@ -3,7 +3,7 @@ import { ajax } from "discourse/lib/ajax"; export default class AdminConfigAboutRoute extends Route { model() { - return ajax("/admin/site_settings.json", { + return ajax("/admin/config/site_settings.json", { data: { filter_names: [ "title", diff --git a/app/controllers/admin/config/site_settings_controller.rb b/app/controllers/admin/config/site_settings_controller.rb new file mode 100644 index 00000000000..cebfc0f17c4 --- /dev/null +++ b/app/controllers/admin/config/site_settings_controller.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class Admin::Config::SiteSettingsController < Admin::AdminController + ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS = %i[ + extended_site_description + about_banner_image + community_owner + ] + + # This endpoint is intended to be used only for admin config areas, + # for a specific collection of site settings. The admin site settings + # UI itself uses the Admin::SiteSettingsController#index endpoint, + # which also supports a `category` and `plugin` filter. + def index + params.require(:filter_names) + + render_json_dump( + site_settings: + SiteSetting.all_settings( + filter_names: params[:filter_names], + include_locale_setting: false, + include_hidden: true, + filter_allowed_hidden: ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS, + ), + ) + end +end diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 3773f6ead56..94f8d7faf76 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -5,25 +5,14 @@ class Admin::SiteSettingsController < Admin::AdminController render_json_error e.message, status: 422 end - ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS = %i[ - extended_site_description - about_banner_image - community_owner - ] - def index params.permit(:categories, :plugin) - params.permit(:filter_names, []) render_json_dump( site_settings: SiteSetting.all_settings( filter_categories: params[:categories], filter_plugin: params[:plugin], - filter_names: params[:filter_names], - include_locale_setting: params[:filter_names].blank?, - include_hidden: true, - filter_allowed_hidden: ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS, ), ) end diff --git a/config/routes.rb b/config/routes.rb index fa711559e71..c2e2c47365e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -387,6 +387,8 @@ Discourse::Application.routes.draw do end end namespace :config, constraints: StaffConstraint.new do + resources :site_settings, only: %i[index] + resources :flags, only: %i[index new create update destroy] do put "toggle" put "reorder/:direction" => "flags#reorder" diff --git a/spec/requests/admin/config/site_settings_controller_spec.rb b/spec/requests/admin/config/site_settings_controller_spec.rb new file mode 100644 index 00000000000..0a7d0fe019c --- /dev/null +++ b/spec/requests/admin/config/site_settings_controller_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +RSpec.describe Admin::SiteSettingsController do + fab!(:admin) + fab!(:user) + + describe "#index" do + context "when not logged in" do + it "returns 404" do + get "/admin/config/site_settings.json" + expect(response.status).to eq(404) + end + end + + context "when not admin" do + before { sign_in(user) } + + it "returns 404" do + get "/admin/config/site_settings.json" + expect(response.status).to eq(404) + end + end + + context "when logged in as admin" do + before { sign_in(admin) } + + it "returns 400 when no filter_names are provided" do + get "/admin/config/site_settings.json" + expect(response.status).to eq(400) + end + + it "includes only certain allowed hidden settings" do + get "/admin/config/site_settings.json", + params: { + filter_names: [ + Admin::Config::SiteSettingsController::ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS, + ], + } + expect( + response.parsed_body["site_settings"].find do |s| + s["setting"] == + Admin::Config::SiteSettingsController::ADMIN_CONFIG_AREA_ALLOWLISTED_HIDDEN_SETTINGS.first.to_s + end, + ).to be_present + get "/admin/config/site_settings.json", params: { filter_names: ["set_locale_from_cookie"] } + expect( + response.parsed_body["site_settings"].find do |s| + s["setting"] == "set_locale_from_cookie" + end, + ).to be_nil + end + + it "returns site settings by exact name" do + get "/admin/config/site_settings.json", + params: { + filter_names: %w[site_description enforce_second_factor], + } + expect(response.status).to eq(200) + expect(response.parsed_body["site_settings"].map { |s| s["setting"] }).to match_array( + %w[site_description enforce_second_factor], + ) + end + end + end +end diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index 6a4271132c2..64839adfc74 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -21,23 +21,14 @@ RSpec.describe Admin::SiteSettingsController do expect(locale.length).to eq(1) end - describe "the filter_names param" do - it "only returns settings that are specified in the filter_names param" do - get "/admin/site_settings.json", - params: { - filter_names: %w[title site_description notification_email], - } - - expect(response.status).to eq(200) - - json = response.parsed_body - expect(json["site_settings"].size).to eq(3) - expect(json["site_settings"].map { |s| s["setting"] }).to contain_exactly( - "title", - "site_description", - "notification_email", - ) - end + it "does not return hidden site settings" do + get "/admin/site_settings.json" + expect(response.status).to eq(200) + expect( + response.parsed_body["site_settings"].find do |s| + s["setting"] == "set_locale_from_cookie" + end, + ).to be_nil end end