Trim whitespace when saving comma-separated System Console settings (#38042)

webapp/channels/src/components/admin_console/schema_admin_settings.test.tsx

@@ -12,7 +12,7 @@ import {defaultIntl} from 'tests/helpers/intl-test-helper';
 import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils';
 
 import {it} from './admin_definition_helpers';
-import SchemaAdminSettings, {SchemaAdminSettings as SchemaAdminSettingsClass} from './schema_admin_settings';
+import SchemaAdminSettings, {SchemaAdminSettings as SchemaAdminSettingsClass, getConfigFromState} from './schema_admin_settings';
 import type {ConsoleAccess, AdminDefinitionSubSectionSchema, AdminDefinitionSettingInput} from './types';
 import ValidationResult from './validation';
 
@@ -1119,3 +1119,66 @@ describe('components/admin_console/SchemaAdminSettings', () => {
         });
     });
 });
+
+describe('components/admin_console/SchemaAdminSettings/getConfigFromState', () => {
+    const buildSchema = (setting: AdminDefinitionSettingInput) => ({
+        id: 'ServiceSettings',
+        name: 'Service Settings',
+        settings: [setting],
+    } as unknown as AdminDefinitionSubSectionSchema);
+
+    const multipleSetting = {
+        type: 'text',
+        key: 'ServiceSettings.DCRRedirectURIAllowlist',
+        label: 'label',
+        multiple: true,
+    } as AdminDefinitionSettingInput;
+
+    test('should trim whitespace from a multiple text setting', () => {
+        const config = getConfigFromState(
+            {},
+            {'ServiceSettings.DCRRedirectURIAllowlist': ['https://one.example.com', ' https://two.example.com']},
+            buildSchema(multipleSetting),
+            () => false,
+        );
+
+        expect(config.ServiceSettings?.DCRRedirectURIAllowlist).toEqual(['https://one.example.com', 'https://two.example.com']);
+    });
+
+    test('should drop empty entries from a multiple text setting', () => {
+        const config = getConfigFromState(
+            {},
+            {'ServiceSettings.DCRRedirectURIAllowlist': ['https://one.example.com', '', '   ']},
+            buildSchema(multipleSetting),
+            () => false,
+        );
+
+        expect(config.ServiceSettings?.DCRRedirectURIAllowlist).toEqual(['https://one.example.com']);
+    });
+
+    test('should save an empty array for a blank multiple text setting', () => {
+        const config = getConfigFromState(
+            {},
+            {'ServiceSettings.DCRRedirectURIAllowlist': [' ']},
+            buildSchema(multipleSetting),
+            () => false,
+        );
+
+        expect(config.ServiceSettings?.DCRRedirectURIAllowlist).toEqual([]);
+    });
+
+    test('should preserve whitespace in a text setting that is not multiple', () => {
+        const config = getConfigFromState(
+            {},
+            {'ServiceSettings.SiteURL': ' https://example.com '},
+            buildSchema({
+                type: 'text',
+                key: 'ServiceSettings.SiteURL',
+                label: 'label',
+            } as AdminDefinitionSettingInput),
+            () => false,
+        );
+
+        expect(config.ServiceSettings?.SiteURL).toBe(' https://example.com ');
+    });
+});

webapp/channels/src/components/admin_console/schema_admin_settings.tsx

@@ -1563,6 +1563,12 @@ export const getSettingValue = (
         return setting.dynamic_value(state[setting.key], config, state);
     }
 
+    // A multiple text setting is typed as a comma-separated list, so discard the whitespace and
+    // empty entries that separating the values leaves behind before saving.
+    if (setting.type === Constants.SettingsTypes.TYPE_TEXT && setting.multiple && Array.isArray(state[setting.key])) {
+        return state[setting.key].map((value: string) => value.trim()).filter((value: string) => value !== '');
+    }
+
     return state[setting.key];
 };
 

webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/index.ts

@@ -12,7 +12,7 @@ import PremadeThemeChooser from './premade_theme_chooser';
 function mapStateToProps(state: GlobalState) {
     const config = getConfig(state);
 
-    const allowedThemes = (config.AllowedThemes && config.AllowedThemes.split(',')) || [];
+    const allowedThemes = config.AllowedThemes?.split(',').map((theme) => theme.trim()).filter((theme) => theme !== '') || [];
 
     return {
         allowedThemes,

webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.test.tsx

@@ -58,4 +58,27 @@ describe('components/user_settings/display/premade_theme_chooser', () => {
         expect(screen.getByText('Onyx')).toBeInTheDocument();
         expect(screen.queryByText('Denim')).not.toBeInTheDocument();
     });
+
+    test('ignores the whitespace surrounding the themes listed in ThemeSettings.AllowedThemes', () => {
+        renderChooser(' denim , onyx ');
+
+        expect(document.querySelectorAll('.premade-themes')).toHaveLength(2);
+        expect(screen.getByText('Denim')).toBeInTheDocument();
+        expect(screen.getByText('Onyx')).toBeInTheDocument();
+        expect(screen.queryByText('Sapphire')).not.toBeInTheDocument();
+    });
+
+    test('ignores the empty entries in ThemeSettings.AllowedThemes', () => {
+        renderChooser('onyx,,');
+
+        expect(document.querySelectorAll('.premade-themes')).toHaveLength(1);
+        expect(screen.getByText('Onyx')).toBeInTheDocument();
+        expect(screen.queryByText('Denim')).not.toBeInTheDocument();
+    });
+
+    test('renders every premade theme when ThemeSettings.AllowedThemes is only whitespace', () => {
+        renderChooser(' , ');
+
+        expect(document.querySelectorAll('.premade-themes')).toHaveLength(allThemeKeys.length);
+    });
 });

webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.tsx

@@ -19,11 +19,11 @@ type Props = {
 
 const PremadeThemeChooser = ({theme, updateTheme, allowedThemes = []}: Props) => {
     const premadeThemes = [];
-    const hasAllowedThemes = allowedThemes.length > 1 || (allowedThemes[0] && allowedThemes[0].trim().length > 0);
+    const hasAllowedThemes = allowedThemes.length > 0;
 
     for (const k in Preferences.THEMES) {
         if (Object.hasOwn(Preferences.THEMES, k)) {
-            if (hasAllowedThemes && allowedThemes.indexOf(k) < 0) {
+            if (hasAllowedThemes && !allowedThemes.includes(k)) {
                 continue;
             }