diff --git a/.github/workflows/e2e-tests-cypress-template.yml b/.github/workflows/e2e-tests-cypress-template.yml index 5822d06757da..a7da8b7bc7ca 100644 --- a/.github/workflows/e2e-tests-cypress-template.yml +++ b/.github/workflows/e2e-tests-cypress-template.yml @@ -377,7 +377,7 @@ jobs: - name: ci/run-summary id: summary continue-on-error: true - uses: mattermost/mattermost-test-system-io/.github/actions/test-system-io-summary@1631d8fcea24f4545a0b3b7f77e41c2fe0be4418 # 2026-07-28 + uses: mattermost/mattermost-test-system-io/.github/actions/test-system-io-summary@e2d5032cfa71a3ad11b0975b135cd221564f2787 # 2026-08-06 with: use-staging: ${{ vars.E2E_USE_STAGING_TEST_IO_URL != 'false' }} composite-identity: ${{ needs.prepare-run.outputs.composite-identity-json }} diff --git a/.github/workflows/e2e-tests-playwright-template.yml b/.github/workflows/e2e-tests-playwright-template.yml index a95787282c89..19d391bcbbed 100644 --- a/.github/workflows/e2e-tests-playwright-template.yml +++ b/.github/workflows/e2e-tests-playwright-template.yml @@ -362,7 +362,7 @@ jobs: - name: ci/run-summary id: summary continue-on-error: true - uses: mattermost/mattermost-test-system-io/.github/actions/test-system-io-summary@1631d8fcea24f4545a0b3b7f77e41c2fe0be4418 # 2026-07-28 + uses: mattermost/mattermost-test-system-io/.github/actions/test-system-io-summary@e2d5032cfa71a3ad11b0975b135cd221564f2787 # 2026-08-06 with: use-staging: ${{ vars.E2E_USE_STAGING_TEST_IO_URL != 'false' }} composite-identity: ${{ needs.prepare-run.outputs.composite-identity-json }} diff --git a/.github/workflows/e2e-tests-playwright.yml b/.github/workflows/e2e-tests-playwright.yml index 2f6b7bec4c7a..5eb64a97573f 100644 --- a/.github/workflows/e2e-tests-playwright.yml +++ b/.github/workflows/e2e-tests-playwright.yml @@ -177,7 +177,7 @@ jobs: pull-requests: write uses: ./.github/workflows/e2e-tests-playwright-template.yml with: - workers: 15 + workers: 20 enabled_docker_services: "postgres inbucket" commit_sha: ${{ inputs.commit_sha }} branch: ${{ needs.generate-build-variables.outputs.branch }} diff --git a/server/channels/api4/system.go b/server/channels/api4/system.go index 15c26b055abf..0746035689b9 100644 --- a/server/channels/api4/system.go +++ b/server/channels/api4/system.go @@ -367,6 +367,11 @@ func queryLogs(c *Context, w http.ResponseWriter, r *http.Request) { return } + if appErr := logFilter.IsValid(); appErr != nil { + c.Err = appErr + return + } + logs, appErr := c.App.QueryLogs(c.AppContext, c.Params.Page, c.Params.LogsPerPage, logFilter) if appErr != nil { c.Err = appErr diff --git a/server/channels/api4/system_test.go b/server/channels/api4/system_test.go index 4bcd37cbf8bb..e7e475308458 100644 --- a/server/channels/api4/system_test.go +++ b/server/channels/api4/system_test.go @@ -488,6 +488,88 @@ func TestGetLogs(t *testing.T) { CheckUnauthorizedStatus(t, resp) } +func TestQueryLogs(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t) + + testID := model.NewId() + expectedMessages := make([]string, 0, 5) + for i := range 5 { + message := fmt.Sprintf("querylogs_verify_%s_%d", testID, i) + expectedMessages = append(expectedMessages, message) + th.TestLogger.Info(message) + } + require.NoError(t, th.TestLogger.Flush(), "failed to flush log") + + containsAllMessages := func(combined string) bool { + for _, expected := range expectedMessages { + if !strings.Contains(combined, expected) { + return false + } + } + return true + } + + // waitForFilteredLogs queries with the filter, asserting a successful response, and + // polls until the expected messages appear (log availability after Flush is + // asynchronous). It returns the per-node lines from the last response. + waitForFilteredLogs := func(t *testing.T, filter *model.LogFilter) map[string][]json.RawMessage { + t.Helper() + var nodes map[string][]json.RawMessage + require.Eventually(t, func() bool { + decoded, _, err := th.SystemAdminClient.QueryLogs(context.Background(), 0, 200, filter) + if err != nil { + return false + } + + var combined strings.Builder + for _, lines := range decoded { + for _, line := range lines { + combined.Write(line) + } + } + if !containsAllMessages(combined.String()) { + return false + } + nodes = decoded + return true + }, 5*time.Second, 25*time.Millisecond, "expected logged messages to be returned") + return nodes + } + + t.Run("empty bounds return unfiltered logs", func(t *testing.T) { + nodes := waitForFilteredLogs(t, &model.LogFilter{DateFrom: "", DateTo: ""}) + require.Contains(t, nodes, "default") + }) + + t.Run("valid bounds are accepted and still return matching logs", func(t *testing.T) { + filter := &model.LogFilter{ + DateFrom: "2000-01-01 00:00:00.000 +00:00", + DateTo: "2100-01-01 00:00:00.000 +00:00", + } + nodes := waitForFilteredLogs(t, filter) + require.Contains(t, nodes, "default") + }) + + t.Run("malformed date_from is rejected with 400", func(t *testing.T) { + _, resp, err := th.SystemAdminClient.QueryLogs(context.Background(), 0, 200, &model.LogFilter{DateFrom: "not-a-date", DateTo: ""}) + CheckErrorID(t, err, "model.log_filter.is_valid.date_from.app_error") + CheckBadRequestStatus(t, resp) + }) + + t.Run("malformed date_to is rejected with 400", func(t *testing.T) { + _, resp, err := th.SystemAdminClient.QueryLogs(context.Background(), 0, 200, &model.LogFilter{DateFrom: "", DateTo: "also-not-a-date"}) + CheckErrorID(t, err, "model.log_filter.is_valid.date_to.app_error") + CheckBadRequestStatus(t, resp) + }) + + t.Run("non-admin is forbidden", func(t *testing.T) { + _, resp, err := th.Client.QueryLogs(context.Background(), 0, 200, &model.LogFilter{}) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) +} + func TestDownloadLogs(t *testing.T) { mainHelper.Parallel(t) th := Setup(t) diff --git a/server/channels/app/platform/log.go b/server/channels/app/platform/log.go index c69aab4ae8c1..9db08864ecdf 100644 --- a/server/channels/app/platform/log.go +++ b/server/channels/app/platform/log.go @@ -326,16 +326,17 @@ func isLogFilteredByDate(rctx request.CTX, logFilter *model.LogFilter, entry *mo return false } - dateFrom, err := time.Parse("2006-01-02 15:04:05.999 -07:00", logFilter.DateFrom) + // Keep parsing aligned with LogFilter.IsValid via the shared layout constant. + dateFrom, err := time.Parse(model.LogFilterDateLayout, logFilter.DateFrom) if err != nil { dateFrom = time.Time{} } - dateTo, err := time.Parse("2006-01-02 15:04:05.999 -07:00", logFilter.DateTo) + dateTo, err := time.Parse(model.LogFilterDateLayout, logFilter.DateTo) if err != nil { dateTo = time.Now() } - timestamp, err := time.Parse("2006-01-02 15:04:05.999 -07:00", entry.Timestamp) + timestamp, err := time.Parse(model.LogFilterDateLayout, entry.Timestamp) if err != nil { rctx.Logger().Debug("Cannot parse timestamp, skipping") return false diff --git a/server/i18n/en.json b/server/i18n/en.json index 811692751b0c..194cb16b7f03 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -12702,6 +12702,14 @@ "id": "model.link_metadata.is_valid.url_length.app_error", "translation": "Length of link metadata URL is {{ .Length }} characters long, which exceeds the maximum limit of {{ .MaxLength }} characters." }, + { + "id": "model.log_filter.is_valid.date_from.app_error", + "translation": "Invalid date_from filter. Expected the format \"{{.Layout}}\"." + }, + { + "id": "model.log_filter.is_valid.date_to.app_error", + "translation": "Invalid date_to filter. Expected the format \"{{.Layout}}\"." + }, { "id": "model.member.is_valid.channel.app_error", "translation": "Channel name is not valid" diff --git a/server/public/model/client4.go b/server/public/model/client4.go index f54b5dada2b7..e3192b297675 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -5464,6 +5464,20 @@ func (c *Client4) GetLogs(ctx context.Context, page, perPage int) ([]string, *Re return DecodeJSONFromResponse[[]string](r) } +// QueryLogs returns a page of logs, filtered by the given LogFilter, keyed by node id. +func (c *Client4) QueryLogs(ctx context.Context, page, perPage int, filter *LogFilter) (map[string][]json.RawMessage, *Response, error) { + values := url.Values{} + values.Set("page", strconv.Itoa(page)) + values.Set("logs_per_page", strconv.Itoa(perPage)) + + r, err := c.doAPIPostJSONWithQuery(ctx, c.logsRoute().Join("query"), values, filter) + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + return DecodeJSONFromResponse[map[string][]json.RawMessage](r) +} + // Download logs as mattermost.log file func (c *Client4) DownloadLogs(ctx context.Context) ([]byte, *Response, error) { r, err := c.doAPIGet(ctx, c.logsRoute().Join("download"), "") diff --git a/server/public/model/system.go b/server/public/model/system.go index f61c6012cdab..77bb646d5601 100644 --- a/server/public/model/system.go +++ b/server/public/model/system.go @@ -5,6 +5,8 @@ package model import ( "math/big" + "net/http" + "time" ) const ( @@ -90,6 +92,10 @@ type AppliedMigration struct { Name string `json:"name"` } +// LogFilterDateLayout is the timestamp layout expected for the DateFrom and +// DateTo bounds of a LogFilter. +const LogFilterDateLayout = "2006-01-02 15:04:05.999 -07:00" + type LogFilter struct { ServerNames []string `json:"server_names"` LogLevels []string `json:"log_levels"` @@ -97,6 +103,28 @@ type LogFilter struct { DateTo string `json:"date_to"` } +// IsValid validates the log filter. Empty date bounds mean "unbounded", so only +// a non-empty value that cannot be parsed with LogFilterDateLayout is rejected. +func (f *LogFilter) IsValid() *AppError { + if f == nil { + return nil + } + + if f.DateFrom != "" { + if _, err := time.Parse(LogFilterDateLayout, f.DateFrom); err != nil { + return NewAppError("LogFilter.IsValid", "model.log_filter.is_valid.date_from.app_error", map[string]any{"Layout": LogFilterDateLayout}, "", http.StatusBadRequest).Wrap(err) + } + } + + if f.DateTo != "" { + if _, err := time.Parse(LogFilterDateLayout, f.DateTo); err != nil { + return NewAppError("LogFilter.IsValid", "model.log_filter.is_valid.date_to.app_error", map[string]any{"Layout": LogFilterDateLayout}, "", http.StatusBadRequest).Wrap(err) + } + } + + return nil +} + type LogEntry struct { Timestamp string Level string diff --git a/server/public/model/system_test.go b/server/public/model/system_test.go new file mode 100644 index 000000000000..07818effe224 --- /dev/null +++ b/server/public/model/system_test.go @@ -0,0 +1,86 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLogFilterIsValid(t *testing.T) { + validDate := "2020-01-02 15:04:05.000 +00:00" + + testCases := []struct { + name string + filter *LogFilter + expectedID string + }{ + { + name: "nil filter is valid", + filter: nil, + expectedID: "", + }, + { + name: "empty bounds mean unbounded", + filter: &LogFilter{DateFrom: "", DateTo: ""}, + expectedID: "", + }, + { + name: "valid bounds", + filter: &LogFilter{DateFrom: validDate, DateTo: validDate}, + expectedID: "", + }, + { + name: "valid from with empty to", + filter: &LogFilter{DateFrom: validDate, DateTo: ""}, + expectedID: "", + }, + { + name: "valid to with empty from", + filter: &LogFilter{DateFrom: "", DateTo: validDate}, + expectedID: "", + }, + { + name: "malformed from is rejected", + filter: &LogFilter{DateFrom: "not-a-date", DateTo: ""}, + expectedID: "model.log_filter.is_valid.date_from.app_error", + }, + { + name: "malformed to is rejected", + filter: &LogFilter{DateFrom: "", DateTo: "also-not-a-date"}, + expectedID: "model.log_filter.is_valid.date_to.app_error", + }, + { + name: "malformed to is rejected with valid from", + filter: &LogFilter{DateFrom: validDate, DateTo: "also-not-a-date"}, + expectedID: "model.log_filter.is_valid.date_to.app_error", + }, + { + name: "from is checked before to", + filter: &LogFilter{DateFrom: "not-a-date", DateTo: "also-not-a-date"}, + expectedID: "model.log_filter.is_valid.date_from.app_error", + }, + { + name: "wrong layout is rejected", + filter: &LogFilter{DateFrom: "2020-01-02T15:04:05Z", DateTo: ""}, + expectedID: "model.log_filter.is_valid.date_from.app_error", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + appErr := tc.filter.IsValid() + if tc.expectedID == "" { + require.Nil(t, appErr) + return + } + + require.NotNil(t, appErr) + require.Equal(t, tc.expectedID, appErr.Id) + require.Equal(t, http.StatusBadRequest, appErr.StatusCode) + }) + } +} diff --git a/webapp/channels/src/components/admin_console/custom_plugin_settings/index.test.tsx b/webapp/channels/src/components/admin_console/custom_plugin_settings/index.test.tsx index 4dc4a6eb7dfa..f4523cf3e4c1 100644 --- a/webapp/channels/src/components/admin_console/custom_plugin_settings/index.test.tsx +++ b/webapp/channels/src/components/admin_console/custom_plugin_settings/index.test.tsx @@ -103,6 +103,154 @@ describe('custom plugin sections and settings', () => { expect(screen.getByText('This is the footer')).toBeInTheDocument(); }); + it('renders top-level settings together with sections', () => { + const state = { + ...baseState, + entities: { + admin: { + plugins: { + testplugin: { + ...plugin, + settings_schema: { + ...plugin.settings_schema, + settings: [{ + key: 'topLevelSetting', + display_name: 'Top-level Setting', + type: 'text' as const, + help_text: 'Top-level setting help text', + placeholder: '', + default: '', + }], + sections: [{ + key: 'section1', + title: 'Section 1', + settings: [{ + key: 'sectionSetting', + display_name: 'Section Setting', + type: 'text' as const, + help_text: 'Section setting help text', + placeholder: '', + default: '', + }], + }], + }, + }, + }, + }, + }, + }; + + renderWithContext( + , + state, + ); + + expect(screen.getByText('Top-level Setting')).toBeInTheDocument(); + expect(screen.getByText('Section 1')).toBeInTheDocument(); + expect(screen.getByText('Section Setting')).toBeInTheDocument(); + }); + + it('renders warnings for top-level custom settings when plugin activation failed', () => { + const state = { + ...baseState, + entities: { + admin: { + plugins: { + testplugin: { + ...plugin, + active: false, + settings_schema: { + ...plugin.settings_schema, + settings: [ + { + key: 'customSetting1', + display_name: 'Custom Setting 1', + type: 'custom' as const, + help_text: '', + }, + { + key: 'customSetting2', + display_name: 'Custom Setting 2', + type: 'custom' as const, + help_text: '', + }, + ], + sections: [{ + key: 'section1', + title: 'Section 1', + settings: [], + }], + }, + }, + }, + }, + }, + }; + + renderWithContext( + , + state, + ); + + expect(screen.getAllByText('In order to view this setting, enable the plugin and click Save.')).toHaveLength(2); + expect(screen.getByText('Section 1')).toBeInTheDocument(); + }); + + it('renders top-level settings when sections is empty', () => { + const state = { + ...baseState, + entities: { + admin: { + plugins: { + testplugin: { + ...plugin, + settings_schema: { + ...plugin.settings_schema, + settings: [{ + key: 'topLevelSetting', + display_name: 'Top-level Setting', + type: 'text' as const, + help_text: 'Top-level setting help text', + placeholder: '', + default: '', + }], + sections: [], + }, + }, + }, + }, + }, + }; + + renderWithContext( + , + state, + ); + + expect(screen.getByText('Top-level Setting')).toBeInTheDocument(); + }); + it('renders plugin metadata with distinct display name and id', () => { const pluginId = 'com.mattermost.fl3xx'; const pluginName = 'FL3XX'; diff --git a/webapp/channels/src/components/admin_console/custom_plugin_settings/index.ts b/webapp/channels/src/components/admin_console/custom_plugin_settings/index.ts index dc5bed1ea424..776780daa1d6 100644 --- a/webapp/channels/src/components/admin_console/custom_plugin_settings/index.ts +++ b/webapp/channels/src/components/admin_console/custom_plugin_settings/index.ts @@ -62,7 +62,10 @@ function makeGetPluginSchema() { type = Constants.SettingsTypes.TYPE_BANNER; displayName = defineMessage({id: 'admin.plugin.customSetting.pluginDisabledWarning', defaultMessage: 'In order to view this setting, enable the plugin and click Save.'}); bannerType = 'warning'; - isDisabled = it.any(it.stateIsTrue(pluginEnabledConfigKey), it.not(it.userHasWritePermissionOnResource('plugins'))); + isDisabled = it.any( + it.all(Boolean(plugin.active), it.stateIsTrue(pluginEnabledConfigKey)), + it.not(it.userHasWritePermissionOnResource('plugins')), + ); } const isHidden = () => { @@ -82,7 +85,7 @@ function makeGetPluginSchema() { banner_type: bannerType, component, showTitle: customComponents[key] ? customComponents[key].options.showTitle : false, - } as Partial; + } as AdminDefinitionSetting; }); }; @@ -90,7 +93,7 @@ function makeGetPluginSchema() { return sections.map((section) => { const key = section.key.toLowerCase(); let component; - let settings: Array> = []; + let settings: AdminDefinitionSetting[] = []; if (section.custom) { if (customSections[key]) { component = customSections[key]?.component; @@ -126,10 +129,11 @@ function makeGetPluginSchema() { }; let sections: AdminDefinitionConfigSchemaSection[] = []; - let settings: Array> = []; - if (plugin.settings_schema && plugin.settings_schema.sections) { + let settings: AdminDefinitionSetting[] = []; + if (plugin.settings_schema?.sections?.length) { sections = parsePluginSettingSections(plugin.settings_schema.sections); - } else if (plugin.settings_schema && plugin.settings_schema.settings) { + } + if (plugin.settings_schema?.settings?.length) { settings = parsePluginSettings(plugin.settings_schema.settings); } @@ -150,24 +154,30 @@ function makeGetPluginSchema() { sections = [{ key: pluginEnabledConfigKey + '.Section', - header: plugin.settings_schema?.header, - footer: plugin.settings_schema?.footer, - settings: [pluginEnableSetting, warningBanner], + settings: [pluginEnableSetting, ...settings, warningBanner], }]; + settings = []; } else if (sections.length > 0) { // Have a separate section on top with the plugin enable/disable setting. sections.unshift({ key: pluginEnabledConfigKey + '.Section', - header: plugin.settings_schema?.header, - footer: plugin.settings_schema?.footer, - settings: [pluginEnableSetting], + settings: [pluginEnableSetting, ...settings], }); + settings = []; } else { // Otherwise we retain existing behaviour and add the setting in front. settings.unshift(pluginEnableSetting); } } + if (sections.length > 0 && settings.length > 0) { + sections.unshift({ + key: pluginEnabledConfigKey + '.Section', + settings, + }); + settings = []; + } + const checkDisableSetting = (s: Partial) => { if (s.isDisabled) { s.isDisabled = it.any(s.isDisabled, it.not(it.userHasWritePermissionOnResource('plugins'))); diff --git a/webapp/channels/src/components/admin_console/plugin_management/plugin_management.test.tsx b/webapp/channels/src/components/admin_console/plugin_management/plugin_management.test.tsx index 112e424ada24..d1503891f02b 100644 --- a/webapp/channels/src/components/admin_console/plugin_management/plugin_management.test.tsx +++ b/webapp/channels/src/components/admin_console/plugin_management/plugin_management.test.tsx @@ -9,7 +9,7 @@ import PluginState from 'mattermost-redux/constants/plugins'; import {PluginManagement} from 'components/admin_console/plugin_management/plugin_management'; import {defaultIntl} from 'tests/helpers/intl-test-helper'; -import {renderWithContext} from 'tests/react_testing_utils'; +import {renderWithContext, screen} from 'tests/react_testing_utils'; describe('components/PluginManagement', () => { const defaultProps = { @@ -576,4 +576,36 @@ describe('components/PluginManagement', () => { }); expect(container).toMatchSnapshot(); }); + + test('should show the settings link for a plugin with sections only', () => { + const props = { + ...defaultProps, + pluginStatuses: { + plugin_0: defaultProps.pluginStatuses.plugin_0, + }, + plugins: { + plugin_0: { + ...defaultProps.plugins.plugin_0, + settings_schema: { + sections: [{ + key: 'section', + settings: [], + }], + }, + }, + }, + }; + const ref = React.createRef>(); + renderWithContext( + , + ); + act(() => { + ref.current!.setState({loading: false} as any); + }); + + expect(screen.getByText('Settings')).toBeInTheDocument(); + }); }); diff --git a/webapp/channels/src/components/admin_console/plugin_management/plugin_management.tsx b/webapp/channels/src/components/admin_console/plugin_management/plugin_management.tsx index 1f39c4264ead..4bf791cb8937 100644 --- a/webapp/channels/src/components/admin_console/plugin_management/plugin_management.tsx +++ b/webapp/channels/src/components/admin_console/plugin_management/plugin_management.tsx @@ -174,6 +174,7 @@ type PluginStatus = { header: string; footer: string; settings?: unknown[]; + sections?: unknown[]; }; }; @@ -997,7 +998,12 @@ export class PluginManagement extends OLDAdminSettings { }); pluginsList = plugins.map((pluginStatus: PluginStatus) => { const p = this.props.plugins[pluginStatus.id]; - const hasSettings = Boolean(p && p.settings_schema && (p.settings_schema.header || p.settings_schema.footer || (p.settings_schema.settings && p.settings_schema.settings.length > 0))); + const hasSettings = Boolean(p?.settings_schema && ( + p.settings_schema.header || + p.settings_schema.footer || + p.settings_schema.settings?.length || + p.settings_schema.sections?.length + )); return ( { expect(screen.getByText('Test')).toBeInTheDocument(); }); + test('should render a component-only section without settings', () => { + renderWithContext( +

{'Component-only section'}

, + }], + } as unknown as AdminDefinitionSubSectionSchema} + patchConfig={jest.fn()} + />, + ); + + expect(screen.getByText('Component-only section')).toBeInTheDocument(); + }); + test('should render header text with markdown links', () => { const headerText = 'This is [a link](!https://example.com) in the header'; const props = { @@ -336,6 +358,73 @@ describe('components/admin_console/SchemaAdminSettings', () => { expect(container.textContent).toContain('in the footer'); }); + test('should render a schema footer after all sections', () => { + const props = { + ...DefaultProps, + config, + environmentConfig, + schema: { + id: 'Config', + name: 'config', + header: 'Schema header', + footer: 'Schema footer', + sections: [{ + key: 'section', + title: 'Plugin section', + settings: [], + }], + } as AdminDefinitionSubSectionSchema, + patchConfig: jest.fn(), + }; + + const {container} = renderWithContext(); + const text = container.textContent || ''; + + expect(text.indexOf('Schema header')).toBeLessThan(text.indexOf('Plugin section')); + expect(text.indexOf('Plugin section')).toBeLessThan(text.indexOf('Schema footer')); + }); + + test('should render top-level settings and sections between the schema header and footer', () => { + const props = { + ...DefaultProps, + config, + environmentConfig, + schema: { + id: 'Config', + name: 'config', + header: 'Schema header', + footer: 'Schema footer', + settings: [{ + key: 'ServiceSettings.SiteURL', + label: 'Top-level Setting', + type: 'text' as const, + }], + sections: [{ + key: 'section', + title: 'Plugin section', + settings: [{ + key: 'ServiceSettings.ConnectionURL', + label: 'Section Setting', + type: 'text' as const, + }], + }], + } as AdminDefinitionSubSectionSchema, + patchConfig: jest.fn(), + }; + + const {container} = renderWithContext(); + const text = container.textContent || ''; + + expect(screen.getByText('Schema header')).toBeInTheDocument(); + expect(screen.getByText('Schema footer')).toBeInTheDocument(); + expect(screen.getByText('Top-level Setting')).toBeInTheDocument(); + expect(screen.getByText('Section Setting')).toBeInTheDocument(); + expect(text.indexOf('Schema header')).toBeLessThan(text.indexOf('Top-level Setting')); + expect(text.indexOf('Top-level Setting')).toBeLessThan(text.indexOf('Plugin section')); + expect(text.indexOf('Plugin section')).toBeLessThan(text.indexOf('Section Setting')); + expect(text.indexOf('Section Setting')).toBeLessThan(text.indexOf('Schema footer')); + }); + test('should render page not found', () => { const props = { ...DefaultProps, @@ -422,6 +511,102 @@ describe('components/admin_console/SchemaAdminSettings', () => { expect(mockValidate).toHaveBeenCalled(); }); + test('should persist permissions from sections in a mixed schema', async () => { + const permissionKey = 'Permissions.enableTeamCreation'; + const editRole = jest.fn().mockResolvedValue({}); + const roles = { + system_user: { + name: 'system_user', + permissions: [], + }, + }; + const mixedSchema = { + id: 'Config', + name: 'config', + settings: [{ + key: 'ServiceSettings.SiteURL', + label: 'Site URL', + type: 'text' as const, + }], + sections: [{ + key: 'permissions', + settings: [{ + key: permissionKey, + label: 'Enable Team Creation', + type: 'permission' as const, + permissions_mapping_name: 'enableTeamCreation' as const, + }], + }], + }; + const ref = React.createRef(); + renderWithContext( + , + ); + + act(() => { + ref.current!.setState({ + [permissionKey]: true, + saveNeeded: 'permissions', + } as any); + }); + await ref.current!.handleSubmit({preventDefault: jest.fn()} as any); + + expect(editRole).toHaveBeenCalledWith(expect.objectContaining({ + name: 'system_user', + permissions: ['create_team'], + })); + }); + + test('should validate top-level and section settings in a mixed schema', () => { + const topLevelValidate = jest.fn(() => new ValidationResult(true, '')); + const sectionValidate = jest.fn(() => new ValidationResult(false, 'Invalid section setting')); + const mixedSchema = { + id: 'Config', + name: 'config', + settings: [{ + key: 'ServiceSettings.SiteURL', + label: 'Site URL', + type: 'text' as const, + validate: topLevelValidate, + }], + sections: [{ + key: 'connection', + settings: [{ + key: 'ServiceSettings.ConnectionURL', + label: 'Connection URL', + type: 'text' as const, + validate: sectionValidate, + }], + }], + }; + const ref = React.createRef(); + renderWithContext( + , + ); + + expect(ref.current?.canSave()).toBe(false); + expect(topLevelValidate).toHaveBeenCalled(); + expect(sectionValidate).toHaveBeenCalled(); + }); + test('should handle changing text input values', async () => { // Use a simplified schema without username/jobstable fields to avoid async complications const simpleSchema = { 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 7a5457faf7e1..171a14dbdd2c 100644 --- a/webapp/channels/src/components/admin_console/schema_admin_settings.tsx +++ b/webapp/channels/src/components/admin_console/schema_admin_settings.tsx @@ -119,6 +119,23 @@ export function descriptorOrStringToString(text: string | MessageDescriptor | un return typeof text === 'string' ? text : intl.formatMessage(text, values); } +function getSchemaSettings(schema: AdminDefinitionSubSectionSchema | null): AdminDefinitionSetting[] { + if (!schema || !('settings' in schema || 'sections' in schema)) { + return []; + } + + const settings = 'settings' in schema && schema.settings ? [...schema.settings] : []; + if ('sections' in schema && schema.sections) { + schema.sections.forEach((section) => { + if (section.settings) { + settings.push(...section.settings); + } + }); + } + + return settings; +} + export class SchemaAdminSettings extends React.PureComponent { private isPlugin: boolean; private saveActions: Array<() => Promise<{error?: {message?: string}}>>; @@ -189,7 +206,7 @@ export class SchemaAdminSettings extends React.PureComponent>((acc, val) => { if (val.type === Constants.SettingsTypes.TYPE_PERMISSION) { acc[val.permissions_mapping_name] = this.state[val.key].toString(); @@ -233,13 +250,7 @@ export class SchemaAdminSettings extends React.PureComponent = {}; if (schema) { - let settings: AdminDefinitionSetting[] = []; - - if ('settings' in schema && schema.settings) { - settings = schema.settings; - } else if ('sections' in schema && schema.sections) { - schema.sections.map((section) => section.settings).forEach((sectionSettings) => settings.push(...sectionSettings)); - } + const settings = getSchemaSettings(schema); // Recursively collect settings from expandable settings const collectSettingsRecursively = (settingsArray: AdminDefinitionSetting[]): AdminDefinitionSetting[] => { @@ -1069,63 +1080,61 @@ export class SchemaAdminSettings extends React.PureComponent { const settingsList: React.ReactNode[] = []; - if (schema.settings) { - schema.settings.forEach((setting) => { + if (settings) { + settings.forEach((setting) => { if (this.buildSettingFunctions[setting.type] && !this.isHidden(setting)) { settingsList.push(this.buildSettingFunctions[setting.type](setting)); } }); } - let header; - if (schema.header) { - header = ( -
- -
- ); - } + return settingsList; + }; - let footer; - if (schema.footer) { - footer = ( -
- -
- ); - } + let header; + if ('header' in schema && schema.header) { + header = ( +
+ +
+ ); + } + let footer; + if ('footer' in schema && schema.footer) { + footer = ( +
+ +
+ ); + } + + const schemaSections = 'sections' in schema ? schema.sections : undefined; + if ('settings' in schema && schema.settings && !schemaSections) { return ( {header} - {settingsList} + {buildSettingsList(schema.settings)} {footer} ); - } else if ('sections' in schema && schema.sections) { + } else if (schemaSections) { const sections: React.ReactNode[] = []; - schema.sections.forEach((section) => { + schemaSections.forEach((section) => { if (this.isSectionHidden(section)) { return; } - const settingsList: React.ReactNode[] = []; - if (section.settings) { - section.settings.forEach((setting) => { - if (this.buildSettingFunctions[setting.type] && !this.isHidden(setting)) { - settingsList.push(this.buildSettingFunctions[setting.type](setting)); - } - }); - } + const settingsList = buildSettingsList(section.settings); if (section.component) { const CustomComponent = section.component; @@ -1228,7 +1237,14 @@ export class SchemaAdminSettings extends React.PureComponent + {header} + {'settings' in schema && schema.settings && schema.settings.length > 0 && ( + + {buildSettingsList(schema.settings)} + + )} {sections} + {footer} ); } @@ -1312,11 +1328,7 @@ export class SchemaAdminSettings extends React.PureComponent { - if (!this.props.schema || !('settings' in this.props.schema) || !this.props.schema.settings) { - return true; - } - - for (const setting of this.props.schema.settings) { + for (const setting of getSchemaSettings(this.props.schema)) { // Some settings are actually not settings (banner) // and don't have a key, skip those ones if (!('key' in setting) || !setting.key) { @@ -1561,13 +1573,7 @@ export const getConfigFromState = ( isDisabled: (setting: AdminDefinitionSetting) => boolean, ) => { if (schema) { - let settings: AdminDefinitionSetting[] = []; - - if ('settings' in schema && schema.settings) { - settings = schema.settings; - } else if ('sections' in schema && schema.sections) { - schema.sections.map((section) => section.settings).forEach((sectionSettings) => settings.push(...sectionSettings)); - } + const settings = getSchemaSettings(schema); // Recursively collect settings from expandable settings const collectSettingsRecursively = (settingsArray: AdminDefinitionSetting[]): AdminDefinitionSetting[] => { diff --git a/webapp/channels/src/utils/admin_console_plugin_index.test.ts b/webapp/channels/src/utils/admin_console_plugin_index.test.ts index 85af5addfdda..8fa44f27ff01 100644 --- a/webapp/channels/src/utils/admin_console_plugin_index.test.ts +++ b/webapp/channels/src/utils/admin_console_plugin_index.test.ts @@ -67,4 +67,42 @@ describe('AdminConsolePluginsIndex.getPluginEntries', () => { expect(entries['plugin_plugin-without-settings']).toContain('Enable Plugin: '); expect(entries['plugin_plugin-without-settings']).toContain('PluginSettings.PluginStates.plugin-without-settings.Enable'); }); + + it('should index plugin section content and nested settings', () => { + const plugin = { + ...samplePlugin4, + settings_schema: { + ...samplePlugin4.settings_schema, + sections: [{ + key: 'connection', + title: 'Connection Settings', + subtitle: 'Configure the service connection', + header: 'Read the [connection guide](https://example.com/header)', + footer: 'Restart after [saving](https://example.com/footer)', + settings: [{ + key: 'ServiceURL', + display_name: 'Service URL', + type: 'text', + help_text: 'Address of the [external service](https://example.com/service)', + placeholder: '', + default: '', + }], + }], + }, + }; + + const entries = getPluginEntries({[plugin.id]: plugin}, intl)[`plugin_${plugin.id}`]; + + expect(entries).toEqual(expect.arrayContaining([ + 'connection', + 'Connection Settings', + 'Configure the service connection', + 'Read the connection guide', + 'Restart after saving', + 'ServiceURL', + 'Service URL', + 'Address of the external service', + ])); + expect(entries.join(' ')).not.toContain('https://example.com'); + }); }); diff --git a/webapp/channels/src/utils/admin_console_plugin_index.ts b/webapp/channels/src/utils/admin_console_plugin_index.ts index d7f77fd35924..1049da909899 100644 --- a/webapp/channels/src/utils/admin_console_plugin_index.ts +++ b/webapp/channels/src/utils/admin_console_plugin_index.ts @@ -34,6 +34,21 @@ function extractTextsFromPlugin(plugin: PluginRedux, intl: IntlShape) { texts.push(...settingsTexts); } } + + if (plugin.settings_schema.sections) { + for (const section of plugin.settings_schema.sections) { + pushString(texts, section.key, intl); + pushString(texts, section.title, intl); + pushString(texts, section.subtitle, intl); + pushString(texts, section.header, intl, true); + pushString(texts, section.footer, intl, true); + + for (const setting of section.settings || []) { + const settingsTexts = extractTextFromSetting(setting as Partial, intl); + texts.push(...settingsTexts); + } + } + } } return texts; }