DEV: Use Rails to render site-settings for qunit tests (#37105)

Duplicating our site-setting parsing/serializing logic in a broccoli
plugin is not ideal. We already rely on the Rails server being available
during qunit runs, so serving the site-settings from there makes more
sense. This change will also help with the Vite migration work.

app/controllers/bootstrap_controller.rb

@@ -2,6 +2,19 @@
 
 class BootstrapController < ApplicationController
   skip_before_action :redirect_to_login_if_required, :check_xhr
+  protect_from_forgery except: :site_settings_for_tests
+
+  def site_settings_for_tests
+    site_settings_json = SiteSetting.client_settings_json_uncached(return_defaults: true)
+    theme_site_settings_json = SiteSetting.theme_site_settings_json_uncached(nil)
+
+    render plain: <<~JS, content_type: "application/javascript"
+      window.CLIENT_SITE_SETTINGS_WITH_DEFAULTS = {
+        ...#{site_settings_json},
+        ...#{theme_site_settings_json}
+      };
+    JS
+  end
 
   def plugin_css_for_tests
     targets = Discourse.find_plugin_css_assets(include_disabled: true, desktop_view: true)

app/views/qunit/theme.html.erb

@@ -15,7 +15,7 @@
       <%- Discourse.find_plugin_js_assets(include_disabled: true, include_admin_asset: true, only: @required_plugins).each do |file| %>
         <%= preload_script file %>
       <%- end %>
-      <%= preload_script "test-site-settings" %>
+      <%= preload_script_url "/bootstrap/site-settings-for-tests.js", type_module: true %>
       <%= theme_translations_lookup %>
       <%= theme_lookup("head_tag") %>
       <%= theme_tests %>

config/routes.rb

@@ -30,6 +30,7 @@ Discourse::Application.routes.draw do
     if Rails.env.local?
       get "/bootstrap/plugin-css-for-tests.css" => "bootstrap#plugin_css_for_tests"
       get "/bootstrap/core-css-for-tests.css" => "bootstrap#core_css_for_tests"
+      get "/bootstrap/site-settings-for-tests.js" => "bootstrap#site_settings_for_tests"
     end
 
     # This is not a valid production route and is causing routing errors to be raised in

frontend/discourse/ember-cli-build.js

@@ -3,7 +3,6 @@
 const EmberApp = require("ember-cli/lib/broccoli/ember-app");
 const path = require("path");
 const mergeTrees = require("broccoli-merge-trees");
-const { parsePluginClientSettings } = require("./lib/site-settings-plugin");
 const generateScriptsTree = require("./lib/scripts");
 const funnel = require("broccoli-funnel");
 const DeprecationSilencer = require("deprecation-silencer");
@@ -50,7 +49,6 @@ function compatModulesFor(name) {
 
 module.exports = function (defaults) {
   const discourseRoot = path.resolve("../..");
-  const vendorJs = discourseRoot + "/vendor/assets/javascripts/";
 
   // Silence deprecations which we are aware of - see `lib/deprecation-silencer.js`
   DeprecationSilencer.silence(console, "warn");
@@ -138,7 +136,6 @@ module.exports = function (defaults) {
   }
 
   let extraPublicTrees = [
-    parsePluginClientSettings(discourseRoot, vendorJs, app),
     funnel(`${discourseRoot}/public/javascripts`, { destDir: "javascripts" }),
     applyTerser(generateScriptsTree(app)),
     pluginTrees,

frontend/discourse/lib/site-settings-plugin.js

@@ -1,96 +0,0 @@
-const Plugin = require("broccoli-plugin");
-const Yaml = require("js-yaml");
-const fs = require("fs");
-const concat = require("broccoli-concat");
-const mergeTrees = require("broccoli-merge-trees");
-const deepmerge = require("deepmerge");
-const glob = require("glob");
-const { shouldLoadPlugins } = require("discourse-plugins");
-
-let built = false;
-
-class SiteSettingsPlugin extends Plugin {
-  constructor(inputNodes, inputFile, options) {
-    super(inputNodes, {
-      ...options,
-      persistentOutput: true,
-    });
-  }
-
-  build() {
-    if (built) {
-      return;
-    }
-
-    let parsed = {};
-
-    this.inputPaths.forEach((path) => {
-      let inputFile;
-      if (path.includes("plugins")) {
-        inputFile = "settings.yml";
-      } else {
-        inputFile = "site_settings.yml";
-      }
-      const file = path + "/" + inputFile;
-      let yaml;
-      try {
-        yaml = fs.readFileSync(file, { encoding: "UTF-8" });
-      } catch {
-        // the plugin does not have a config file, go to the next file
-        return;
-      }
-      const loaded = Yaml.load(yaml, { json: true });
-      parsed = deepmerge(parsed, loaded);
-    });
-
-    let clientSettings = {};
-    // eslint-disable-next-line no-unused-vars
-    for (const [category, settings] of Object.entries(parsed)) {
-      for (const [setting, details] of Object.entries(settings)) {
-        if (details.client) {
-          clientSettings[setting] = details.default;
-        }
-      }
-    }
-    const contents = `var CLIENT_SITE_SETTINGS_WITH_DEFAULTS  = ${JSON.stringify(
-      clientSettings
-    )}`;
-
-    fs.writeFileSync(`${this.outputPath}/` + "settings_out.js", contents);
-    built = true;
-  }
-}
-
-module.exports = function siteSettingsPlugin(...params) {
-  return new SiteSettingsPlugin(...params);
-};
-
-module.exports.parsePluginClientSettings = function (
-  discourseRoot,
-  vendorJs,
-  app
-) {
-  let settings = [discourseRoot + "/config"];
-
-  if (shouldLoadPlugins()) {
-    const pluginInfos = app.project
-      .findAddonByName("discourse-plugins")
-      .pluginInfos();
-    pluginInfos.forEach(({ hasConfig, configDirectory }) => {
-      if (hasConfig) {
-        settings = settings.concat(glob.sync(configDirectory));
-      }
-    });
-  }
-
-  const loadedSettings = new SiteSettingsPlugin(settings, "site_settings.yml");
-
-  return concat(mergeTrees([loadedSettings]), {
-    inputFiles: [],
-    headerFiles: [],
-    footerFiles: [],
-    outputFile: `assets/test-site-settings.js`,
-  });
-};
-
-module.exports.SiteSettingsPlugin = SiteSettingsPlugin;

frontend/discourse/tests/helpers/site-settings.js

@@ -13,11 +13,9 @@ const CLIENT_SETTING_TEST_OVERRIDES = {
   anon_polling_interval: 30000,
 };
 
-// Note, CLIENT_SITE_SETTINGS_WITH_DEFAULTS is generated by the site-settings-plugin,
-// writing to test-site-settings.js via the ember-cli-build pipeline.
+// window.CLIENT_SITE_SETTINGS_WITH_DEFAULTS is injected by `/bootstrap/site-settings-for-tests.js`
 const ORIGINAL_CLIENT_SITE_SETTINGS = {
-  // eslint-disable-next-line no-undef
-  ...CLIENT_SITE_SETTINGS_WITH_DEFAULTS,
+  ...window.CLIENT_SITE_SETTINGS_WITH_DEFAULTS,
   ...CLIENT_SETTING_TEST_OVERRIDES,
 };
 

frontend/discourse/tests/index.html

@@ -53,11 +53,11 @@
 
     <script src="{{rootURL}}assets/discourse.js"></script>
 
-    <script src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/main.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/mf.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/admin.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/wizard.js" data-embroider-ignore></script>
-    <script src="{{rootURL}}assets/test-site-settings.js" data-embroider-ignore></script>
+    <script type="module" src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/main.js" data-embroider-ignore></script>
+    <script type="module" src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/mf.js" data-embroider-ignore></script>
+    <script type="module" src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/admin.js" data-embroider-ignore></script>
+    <script type="module" src="{{rootURL}}extra-locales/0000000000000000000000000000000000000000/en/wizard.js" data-embroider-ignore></script>
+    <script type="module" src="{{rootURL}}bootstrap/site-settings-for-tests.js" data-embroider-ignore></script>
 
     <template id="dynamic-test-js">
       {{content-for "test-plugin-css"}}
@@ -72,7 +72,7 @@
     </discourse-dynamic-test-js>
 
     <!-- This script takes the <template>, filters plugin assets as required, then appends to discourse-dynamic-test-js -->
-    <script src="{{rootURL}}assets/scripts/discourse-test-load-dynamic-js.js" data-embroider-ignore></script>
+    <script defer src="{{rootURL}}assets/scripts/discourse-test-load-dynamic-js.js" data-embroider-ignore></script>
 
     <link rel="stylesheet" href="{{rootURL}}stylesheets/qunit-custom.css" data-embroider-ignore />
   </body>

lib/site_setting_extension.rb

@@ -242,7 +242,7 @@ module SiteSettingExtension
     ""
   end
 
-  def client_settings_json_uncached
+  def client_settings_json_uncached(return_defaults: false)
     uncached_json =
       @client_settings.filter_map do |name|
         # Themeable site settings require a theme ID, which we do not always
@@ -251,7 +251,9 @@ module SiteSettingExtension
         next if themeable[name]
 
         value =
-          if deprecated_settings.include?(name.to_s)
+          if return_defaults
+            SiteSetting.defaults[name]
+          elsif deprecated_settings.include?(name.to_s)
             public_send(name, warn: false)
           else
             public_send(name)
@@ -260,7 +262,7 @@ module SiteSettingExtension
         type = type_supervisor.get_type(name)
         if type == :upload
           value = value.to_s
-        elsif type == :uploaded_image_list
+        elsif type == :uploaded_image_list && value.present?
           value = value.map(&:to_s).join("|")
         end