DEV: Move config area site setting fetch into new controller (#28136)

Followup 4aea12fdcb

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.

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",

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

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

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"

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

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