[2.x] fix: keep a saved falsy setting in settings modals too (#4901)

#4782 fixed this in `AdminPage` but the identical line in `SettingsModal`
survived, so the bug still reached every settings *modal*: settings live in a
string column, a saved `false` arrives as an empty string, and choosing the
fallback on falsiness discarded it and reseeded the default. A boolean setting
whose default is truthy could not be switched off — it came back on with every
reload.

`SettingsModal` is public API, imported by extensions as
`flarum/admin/components/SettingsModal`, and core's custom header, footer and
CSS modals extend it, as does flarum/audit.

The existing test file now covers both classes side by side rather than only
the one the original issue named, which is what would have caught this the
first time. Its docblock is corrected too: it claimed a saved `'0'` was also
discarded, but the string is truthy in JavaScript, unlike the number — the
empty string is the only value a setting can hold that JavaScript reads as
absent.

Author: imorland

framework/core/js/src/admin/components/SettingsModal.tsx

@@ -38,7 +38,12 @@ export default abstract class SettingsModal<CustomAttrs extends ISettingsModalAt
   }
 
   setting(key: string, fallback: string = ''): Stream<SettingValue> {
-    this.settings[key] = this.settings[key] || Stream(app.data.settings[key] || fallback);
+    // `??`, not `||`: settings live in a string column, so a saved `false`
+    // arrives as an empty string. Falling back on falsiness discarded it and
+    // reseeded the default, which meant a setting whose default is truthy could
+    // never be switched off — it came back on with every reload. Only the
+    // absence of a saved value should reach for the fallback.
+    this.settings[key] = this.settings[key] || Stream(app.data.settings[key] ?? fallback);
 
     return this.settings[key];
   }

framework/core/js/tests/integration/admin/components/AdminPageSetting.test.ts

@@ -1,5 +1,6 @@
 import bootstrapAdmin from '@flarum/jest-config/src/bootstrap/admin';
 import AdminPage from '../../../../src/admin/components/AdminPage';
+import SettingsModal from '../../../../src/admin/components/SettingsModal';
 import { app } from '../../../../src/admin';
 
 beforeAll(() => bootstrapAdmin());
@@ -8,12 +9,17 @@ beforeAll(() => bootstrapAdmin());
  * `setting(key, fallback)` seeds a stream from the saved value, falling back to
  * the given default when there is nothing saved.
  *
- * Settings are stored in a string column, so a saved `false` comes back as `''`
- * and a saved `0` as `'0'` — both falsy in JavaScript. Choosing the fallback on
- * falsiness therefore discarded values an administrator had deliberately saved,
- * and a setting whose default is truthy could never be turned off: it reverted
- * on every reload (flarum/framework#4781). Only the *absence* of a saved value
- * should reach for the default.
+ * Settings are stored in a string column, so a saved `false` comes back as an
+ * empty string — the one value a setting can hold that JavaScript reads as
+ * absent. Choosing the fallback on falsiness therefore discarded a value the
+ * administrator had deliberately saved, and a setting whose default is truthy
+ * could never be turned off: it reverted on every reload
+ * (flarum/framework#4781). Only the *absence* of a saved value should reach for
+ * the default.
+ *
+ * Two classes expose this, and both are public API: `AdminPage` for settings
+ * pages, and `SettingsModal` for settings modals. They are covered together
+ * because the bug was fixed in the first while surviving in the second.
  */
 describe('AdminPage.setting()', () => {
   class TestPage extends AdminPage {
@@ -65,3 +71,48 @@ describe('AdminPage.setting()', () => {
     expect(seeded('0', '10')).toBe('0');
   });
 });
+
+/**
+ * The same contract, on the class settings *modals* are built from. `flarum/audit`
+ * and core's own custom header, footer and CSS modals all extend it, and it is
+ * part of the public API extensions import as
+ * `flarum/admin/components/SettingsModal`.
+ */
+describe('SettingsModal.setting()', () => {
+  class TestModal extends SettingsModal {
+    title() {
+      return 'Test';
+    }
+
+    form() {
+      return null;
+    }
+  }
+
+  const seeded = (saved: string | undefined, fallback: string): unknown => {
+    const key = 'test.modal.setting';
+
+    if (saved === undefined) {
+      delete app.data.settings[key];
+    } else {
+      app.data.settings[key] = saved;
+    }
+
+    const modal = new TestModal();
+    (modal as any).settings = {};
+
+    return modal.setting(key, fallback)();
+  };
+
+  test('a saved value is used', () => {
+    expect(seeded('1', '')).toBe('1');
+  });
+
+  test('the fallback is used when nothing is saved', () => {
+    expect(seeded(undefined, 'the-default')).toBe('the-default');
+  });
+
+  test('a saved empty string is kept rather than falling back', () => {
+    expect(seeded('', '1')).toBe('');
+  });
+});