From 260d0cda822beded4517a191bce5f6504fa3c3f1 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 19 Aug 2026 20:28:51 -0300 Subject: [PATCH] Trim whitespace when saving comma-separated System Console settings (#38042) --- .../schema_admin_settings.test.tsx | 65 ++++++++++++++++++- .../admin_console/schema_admin_settings.tsx | 6 ++ .../premade_theme_chooser/index.ts | 2 +- .../premade_theme_chooser.test.tsx | 23 +++++++ .../premade_theme_chooser.tsx | 4 +- 5 files changed, 96 insertions(+), 4 deletions(-) diff --git a/webapp/channels/src/components/admin_console/schema_admin_settings.test.tsx b/webapp/channels/src/components/admin_console/schema_admin_settings.test.tsx index 419e55da2fb..5ab12b6b26d 100644 --- a/webapp/channels/src/components/admin_console/schema_admin_settings.test.tsx +++ b/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 '); + }); +}); diff --git a/webapp/channels/src/components/admin_console/schema_admin_settings.tsx b/webapp/channels/src/components/admin_console/schema_admin_settings.tsx index 171a14dbdd2..bd817489586 100644 --- a/webapp/channels/src/components/admin_console/schema_admin_settings.tsx +++ b/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]; }; diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/index.ts b/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/index.ts index fed2d34661d..22f1f02b43f 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/index.ts +++ b/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, diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.test.tsx b/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.test.tsx index 38e5432c633..773a6382d23 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.test.tsx +++ b/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); + }); }); diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.tsx b/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.tsx index e90c0ad0644..51a6c28daa8 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.tsx +++ b/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; }