diff --git a/.github/codecov.yml b/.github/codecov.yml index cbc4185fa81..36d71df8cf2 100644 --- a/.github/codecov.yml +++ b/.github/codecov.yml @@ -1,17 +1,42 @@ -comment: - layout: "condensed_header, condensed_files, condensed_footer" - behavior: default - require_changes: "uncovered_patch" # only post comment if the patch has uncovered lines - hide_project_coverage: true # only show coverage on the git diff +codecov: + require_ci_to_pass: false + # Wait for all coverage uploads (4 server shards + 1 webapp) before + # computing status. Without this, Codecov may report partial coverage + # from the first shard to finish, showing a misleading drop on the PR. + notify: + after_n_builds: 5 + coverage: status: - changes: false - patch: false project: default: - threshold: 1.0 -codecov: - notify: - after_n_builds: 2 # Server and webapp at this point -ignore: - - ^store/storetest.* + target: auto + threshold: 1% + informational: true + patch: + default: + target: 50% + informational: true + + # Exclude generated code, mocks, and test infrastructure from reporting. + # Go compiles these into the test binary, so they appear in cover.out, + # but they aren't production code and inflate the denominator. + ignore: + - "server/**/retrylayer/**" + - "server/**/timerlayer/**" + - "server/**/*_serial_gen.go" + - "server/**/mocks/**" + - "server/**/storetest/**" + - "server/**/plugintest/**" + - "server/**/searchtest/**" + +flags: + server: + after_n_builds: 4 # 4 server test shards + webapp: + after_n_builds: 1 # 1 merged webapp upload + +comment: + layout: "condensed_header,diff,flags" + behavior: default + require_changes: true diff --git a/.github/workflows/server-ci.yml b/.github/workflows/server-ci.yml index 8c9f3980708..e0efe7c27db 100644 --- a/.github/workflows/server-ci.yml +++ b/.github/workflows/server-ci.yml @@ -208,6 +208,7 @@ jobs: logsartifact: postgres-binary-server-test-logs go-version: ${{ needs.go.outputs.version }} fips-enabled: false + fullyparallel: false # -- Sharded into 4 parallel runners for ~88% wall-time improvement -- test-postgres-normal: name: Postgres (shard ${{ matrix.shard }}) @@ -270,22 +271,27 @@ jobs: artifact-name: postgres-server-fips-test-logs test-coverage: - name: Generate Test Coverage - # Disabled: Running out of memory and causing spurious failures. - # Old condition: ${{ github.event_name != 'pull_request' || !startsWith(github.event.pull_request.base.ref, 'release-') }} - if: false + name: "Coverage (shard ${{ matrix.shard }})" + if: ${{ github.event_name != 'pull_request' || !startsWith(github.event.pull_request.base.ref, 'release-') }} needs: go + strategy: + fail-fast: false + matrix: + shard: [0, 1, 2, 3] uses: ./.github/workflows/server-test-template.yml secrets: inherit with: - name: Generate Test Coverage + name: "Coverage (shard ${{ matrix.shard }})" datasource: postgres://mmuser:mostest@postgres:5432/mattermost_test?sslmode=disable&connect_timeout=10 drivername: postgres - logsartifact: coverage-server-test-logs + logsartifact: "coverage-server-test-logs-shard-${{ matrix.shard }}" fullyparallel: true allow-failure: true enablecoverage: true go-version: ${{ needs.go.outputs.version }} + fips-enabled: false + shard-index: ${{ matrix.shard }} + shard-total: 4 test-mmctl: name: Run mmctl tests needs: go diff --git a/.github/workflows/server-test-template.yml b/.github/workflows/server-test-template.yml index 2e67e79aa2a..3d0f9abd7f4 100644 --- a/.github/workflows/server-test-template.yml +++ b/.github/workflows/server-test-template.yml @@ -222,6 +222,7 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} disable_search: true files: server/cover.out + flags: server - name: Stop docker compose run: | diff --git a/.github/workflows/webapp-ci.yml b/.github/workflows/webapp-ci.yml index 65fa9ab9f8d..869413d7e58 100644 --- a/.github/workflows/webapp-ci.yml +++ b/.github/workflows/webapp-ci.yml @@ -223,6 +223,7 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} disable_search: true files: ./webapp/channels/coverage/merged/lcov.info + flags: webapp build: needs: check-lint diff --git a/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts b/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts index 64234aecb66..24d8a049b89 100644 --- a/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/auth_sso/authentication_4_spec.ts @@ -209,12 +209,8 @@ describe('Authentication', () => { // # Go to front page cy.visit('/login'); - // * Assert that create account button is visible - cy.findByText('Don\'t have an account?', {timeout: TIMEOUTS.ONE_MIN}).should('be.visible').click(); - - // * Verify redirection to access problem page since account creation is disabled - cy.url().should('include', '/access_problem'); - cy.findByText('Contact your workspace admin'); + // * Assert that create account button is not visible + cy.findByText('Don\'t have an account?', {timeout: TIMEOUTS.ONE_MIN}).should('not.exist'); // # Go to sign up with email page cy.visit('/signup_user_complete'); diff --git a/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js b/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js index db4a64eebb9..83011c91130 100644 --- a/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/signin_authentication/login_close_server_spec.js @@ -36,8 +36,7 @@ describe('Login page with close server', () => { // Restore backed up settings cy.apiAdminLogin().apiUpdateConfig(oldSettings); }); - it('MM-47222 Should verify access problem page can be reached', () => { - cy.findByText('Don\'t have an account?').should('be.visible').click(); - cy.findByText('Contact your workspace admin').should('be.visible'); + it('MM-47222 Should verify signup link not visible', () => { + cy.findByText('Don\'t have an account?').should('not.exist'); }); }); diff --git a/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js b/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js index d956613d9b8..8873927b318 100644 --- a/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/system_console/ui_and_api/customization_spec.js @@ -49,10 +49,28 @@ describe('Customization', () => { // # Save setting saveSetting(); - // # Verify that after page reload image exist cy.reload(); cy.findByTestId('CustomBrandImage').should('be.visible').within(() => { + // * Verify that after page reload image exist cy.get('img').should('have.attr', 'src').and('include', '/api/v4/brand/image?t='); + + // * Verify that there's an option to delete the image. + cy.findByTestId('remove-image__btn').should('be.visible'); + + // # delete the image + cy.findByTestId('remove-image__btn').click(); + }); + + // # Save setting + saveSetting(); + + cy.reload(); + cy.findByTestId('CustomBrandImage').should('be.visible').within(() => { + // * Verify that after page reload, the image doesn't exist. + cy.findByAltText('brand image').should('not.exist'); + + // * Verify there's no option to delete the image. + cy.findByTestId('remove-image__btn').should('not.exist'); }); }); diff --git a/server/Makefile b/server/Makefile index d345fc31f19..d125a89dbd0 100644 --- a/server/Makefile +++ b/server/Makefile @@ -165,7 +165,7 @@ PLUGIN_PACKAGES += mattermost-plugin-playbooks-v2.8.0 PLUGIN_PACKAGES += mattermost-plugin-servicenow-v2.4.0 PLUGIN_PACKAGES += mattermost-plugin-zoom-v1.12.0 PLUGIN_PACKAGES += mattermost-plugin-agents-v1.7.2 -PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.2 +PLUGIN_PACKAGES += mattermost-plugin-boards-v9.2.4 PLUGIN_PACKAGES += mattermost-plugin-user-survey-v1.1.1 PLUGIN_PACKAGES += mattermost-plugin-mscalendar-v1.6.0 PLUGIN_PACKAGES += mattermost-plugin-msteams-meetings-v2.4.1 diff --git a/server/channels/api4/team.go b/server/channels/api4/team.go index 4d731c7e8b8..8676c26cce3 100644 --- a/server/channels/api4/team.go +++ b/server/channels/api4/team.go @@ -657,6 +657,10 @@ func getTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { return } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + team.SanitizeRoleData(c.AppContext.Session().UserId) + } + if err := json.NewEncoder(w).Encode(team); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) } @@ -695,6 +699,13 @@ func getTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembers", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -734,6 +745,13 @@ func getTeamMembersForUser(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + for _, m := range members { + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), m.TeamId, model.PermissionManageTeamRoles) { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersForUser", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -777,6 +795,13 @@ func getTeamMembersByIds(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range members { + m.SanitizeRoleData(currentUserId) + } + } + js, err := json.Marshal(members) if err != nil { c.Err = model.NewAppError("getTeamMembersByIds", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) @@ -883,6 +908,10 @@ func addTeamMember(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventObjectType("team_member") // TODO verify this is the final state. should it be the team instead? auditRec.Success() + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + tm.SanitizeRoleData(c.AppContext.Session().UserId) + } + w.WriteHeader(http.StatusCreated) if err := json.NewEncoder(w).Encode(tm); err != nil { c.Logger.Warn("Error while writing response", mlog.Err(err)) @@ -1037,6 +1066,15 @@ func addTeamMembers(c *Context, w http.ResponseWriter, r *http.Request) { return } + currentUserId := c.AppContext.Session().UserId + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionManageTeamRoles) { + for _, m := range membersWithErrors { + if m.Member != nil { + m.Member.SanitizeRoleData(currentUserId) + } + } + } + var ( js []byte err error diff --git a/server/channels/api4/team_test.go b/server/channels/api4/team_test.go index 4070366030c..06305a34363 100644 --- a/server/channels/api4/team_test.go +++ b/server/channels/api4/team_test.go @@ -4609,3 +4609,247 @@ func TestInvalidateAllEmailInvites(t *testing.T) { CheckOKStatus(t, res) }) } + +func setupTeamWithAdminAndMember(t *testing.T, th *TestHelper) *model.Client4 { + t.Helper() + th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam) + require.Nil(t, th.App.Srv().InvalidateAllCaches()) + teamAdminClient := th.CreateClient() + _, _, err := teamAdminClient.Login(context.Background(), th.BasicUser2.Email, th.BasicUser2.Password) + require.NoError(t, err) + return teamAdminClient +} + +func assertRoleDataSanitized(t *testing.T, m *model.TeamMember) { + t.Helper() + assert.Empty(t, m.Roles) + assert.Empty(t, m.ExplicitRoles) + assert.False(t, m.SchemeAdmin) + assert.False(t, m.SchemeGuest) + assert.False(t, m.SchemeUser) + assert.Equal(t, int64(-1), m.DeleteAt) +} + +func TestGetTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId != th.BasicUser.Id { + assertRoleDataSanitized(t, m) + } + } + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "current user not found in members") + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "target user not found in members") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembers(context.Background(), th.BasicTeam.Id, 0, 100, "") + require.NoError(t, err) + + for _, m := range members { + if m.UserId == th.BasicUser2.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "team admin not found in members") + }) +} + +func TestGetTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assertRoleDataSanitized(t, member) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + member, _, err := th.Client.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + member, _, err := teamAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + member, _, err := th.SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + assert.True(t, member.SchemeAdmin) + }) +} + +func TestGetTeamMembersByIdsRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("non-admin cannot see role data of others", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) + + t.Run("non-admin sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("team admin sees full role data for other user", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersByIds(context.Background(), th.BasicTeam.Id, []string{th.BasicUser2.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeAdmin) + }) +} + +func TestAddTeamMemberRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding user sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser(t) + tm, _, err := teamAdminClient.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assert.True(t, tm.SchemeUser) + }) + + t.Run("non-admin adding user sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser(t) + tm, _, err := th.Client.AddTeamMember(context.Background(), th.BasicTeam.Id, newUser.Id) + require.NoError(t, err) + assertRoleDataSanitized(t, tm) + }) +} + +func TestAddTeamMembersRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("team admin adding users sees full role data in response", func(t *testing.T) { + newUser := th.CreateUser(t) + members, _, err := teamAdminClient.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assert.True(t, members[0].SchemeUser) + }) + + t.Run("non-admin adding users sees sanitized role data in response", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionAddUserToTeam.Id, model.TeamUserRoleId) + + newUser := th.CreateUser(t) + members, _, err := th.Client.AddTeamMembers(context.Background(), th.BasicTeam.Id, []string{newUser.Id}) + require.NoError(t, err) + require.Len(t, members, 1) + assertRoleDataSanitized(t, members[0]) + }) +} + +func TestGetTeamMembersForUserRoleDataSanitization(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + teamAdminClient := setupTeamWithAdminAndMember(t, th) + + t.Run("user sees own role data", func(t *testing.T) { + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assert.True(t, m.SchemeUser) + } + }) + + t.Run("non-admin cannot see role data of another user", func(t *testing.T) { + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + th.AddPermissionToRole(t, model.PermissionReadOtherUsersTeams.Id, model.SystemUserRoleId) + + members, _, err := th.Client.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + assertRoleDataSanitized(t, m) + } + }) + + t.Run("team admin sees full role data for other user in managed team", func(t *testing.T) { + members, _, err := teamAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeUser) + return + } + } + require.Fail(t, "basic team membership not found") + }) + + t.Run("system admin sees full role data", func(t *testing.T) { + members, _, err := th.SystemAdminClient.GetTeamMembersForUser(context.Background(), th.BasicUser2.Id, "") + require.NoError(t, err) + require.NotEmpty(t, members) + for _, m := range members { + if m.TeamId == th.BasicTeam.Id { + assert.True(t, m.SchemeAdmin) + return + } + } + require.Fail(t, "basic team membership not found") + }) +} diff --git a/server/public/model/team_member.go b/server/public/model/team_member.go index 53cf25f072b..59c73936794 100644 --- a/server/public/model/team_member.go +++ b/server/public/model/team_member.go @@ -142,3 +142,14 @@ func (o *TeamMember) PreUpdate() { func (o *TeamMember) GetRoles() []string { return strings.Fields(o.Roles) } + +func (o *TeamMember) SanitizeRoleData(currentUserId string) { + if o.UserId != currentUserId { + o.Roles = "" + o.ExplicitRoles = "" + o.SchemeAdmin = false + o.SchemeGuest = false + o.SchemeUser = false + o.DeleteAt = -1 + } +} diff --git a/server/scripts/run-shard-tests.sh b/server/scripts/run-shard-tests.sh index 57c55ab059d..688e3f2019e 100755 --- a/server/scripts/run-shard-tests.sh +++ b/server/scripts/run-shard-tests.sh @@ -108,6 +108,20 @@ fi cat gotestsum-*.json > gotestsum.json 2>/dev/null || true +# ── Merge coverage profiles within this shard (if coverage is enabled) ── +# A single shard may run multiple gotestsum invocations (light packages + +# heavy package splits), each producing its own cover-N.out. This merges +# them into one cover.out per shard. The cross-shard merge (combining all +# shards into a single report) is handled by Codecov's after_n_builds. +if [[ "${ENABLE_COVERAGE:-false}" == "true" ]] && ls cover-*.out 1>/dev/null 2>&1; then + echo "Merging coverage profiles..." + { + head -1 cover-0.out # "mode: atomic" header + tail -q -n +2 cover-*.out # data lines from all files + } > cover.out + echo "Merged $(ls cover-*.out | wc -l) coverage files into cover.out" +fi + if [[ $FAILURES -gt 0 ]]; then echo "Shard complete: $RUN_IDX gotestsum runs, $FAILURES failed" exit 1 diff --git a/webapp/channels/src/components/access_problem/access_problem.scss b/webapp/channels/src/components/access_problem/access_problem.scss deleted file mode 100644 index 7386478f691..00000000000 --- a/webapp/channels/src/components/access_problem/access_problem.scss +++ /dev/null @@ -1,40 +0,0 @@ -.header-footer-route .header-footer-route-container { - display: flex; - justify-content: space-between; -} - -.AccessProblem { - &__body { - display: flex; - flex-direction: column; - align-items: center; - justify-content: center; - } - - &__title { - display: flex; - align-items: center; - margin: 32px 0 16px 0; - color: var(--portal-denim); - font-family: 'Metropolis'; - font-size: 22px; - font-style: normal; - font-weight: 600; - line-height: 28px; - text-align: center; - } - - &__description { - display: flex; - max-width: 640px; - align-items: center; - padding: 0 40px; - color: var(--portal-denim); - font-family: 'Open Sans'; - font-size: 14px; - font-style: normal; - font-weight: 400; - line-height: 20px; - text-align: center; - } -} diff --git a/webapp/channels/src/components/access_problem/index.tsx b/webapp/channels/src/components/access_problem/index.tsx deleted file mode 100644 index 05429c7fbe0..00000000000 --- a/webapp/channels/src/components/access_problem/index.tsx +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import React, {useCallback, useEffect} from 'react'; -import {useIntl} from 'react-intl'; -import {useHistory} from 'react-router-dom'; - -import AccessProblemSVG from 'components/common/svg_images_components/access_problem_svg'; -import type {CustomizeHeaderType} from 'components/header_footer_route/header_footer_route'; - -import './access_problem.scss'; - -type AccessProblemProps = { - onCustomizeHeader?: CustomizeHeaderType; -} - -const AccessProblem = ({ - onCustomizeHeader, -}: AccessProblemProps) => { - const {formatMessage} = useIntl(); - const history = useHistory(); - - const handleHeaderBackButtonOnClick = useCallback(() => { - history.goBack(); - }, [history]); - - useEffect(() => { - if (onCustomizeHeader) { - onCustomizeHeader({ - onBackButtonClick: handleHeaderBackButtonOnClick, - }); - } - }, [onCustomizeHeader, handleHeaderBackButtonOnClick]); - - return ( -
- -
- {formatMessage({id: 'login.contact_admin.title', defaultMessage: 'Contact your workspace admin'})} -
-
- {formatMessage({id: 'login.contact_admin.detail', defaultMessage: "To access your team's workspace, contact your workspace admin. If you've been invited already, check your email inbox for a Mattermost workspace invite."})} -
-
- ); -}; - -export default AccessProblem; diff --git a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx index 6f65ef09781..93ea64e1841 100644 --- a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx +++ b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.test.tsx @@ -1,30 +1,18 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import nock from 'nock'; import React from 'react'; -import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; +import {Client4} from 'mattermost-redux/client'; import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; import BrandImageSetting from './brand_image_setting'; -// Real implementations are async (await dispatch(...)); mocks must return Promises so handleSave can await them. -jest.mock('actions/admin_actions.jsx', () => ({ - ...jest.requireActual('actions/admin_actions.jsx'), - uploadBrandImage: jest.fn(async () => {}), - deleteBrandImage: jest.fn(async () => {}), -})); +Client4.setUrl('http://localhost:8065'); describe('components/admin_console/brand_image_setting', () => { - beforeEach(() => { - jest.spyOn(global, 'fetch').mockResolvedValue({status: 404} as Response); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - const baseProps = { disabled: false, setSaveNeeded: jest.fn(), @@ -32,71 +20,59 @@ describe('components/admin_console/brand_image_setting', () => { unRegisterSaveAction: jest.fn(), }; - test('should have called deleteBrandImage or uploadBrandImage on save depending on component state', async () => { - let saveAction: (() => Promise) | undefined; - const registerSaveAction = jest.fn((fn: () => Promise) => { - saveAction = fn; - }); + const deleteButtonTestId = 'remove-image__btn'; - const {container, unmount} = renderWithContext( - , - ); + let scope: nock.Scope; - // Wait for componentDidMount fetch to resolve - await waitFor(() => { - expect(registerSaveAction).toHaveBeenCalled(); - }); - expect(saveAction).toBeDefined(); + beforeAll(() => { + scope = nock(Client4.getBaseRoute()).persist().get('/brand/image').query(true).reply(200); + }); - // Simulate selecting a file via the file input to set brandImage - const file = new File(['brand_image_file'], 'brand.png', {type: 'image/png'}); - const fileInput = container.querySelector('input[type="file"]'); - expect(fileInput).toBeInTheDocument(); - await userEvent.upload(fileInput as HTMLInputElement, file); + afterAll(() => { + nock.cleanAll(); + }); - // Now call save - should call uploadBrandImage - await saveAction!(); - expect(deleteBrandImage).toHaveBeenCalledTimes(0); - expect(uploadBrandImage).toHaveBeenCalledTimes(1); + test('should register and unregister save handler when mounted and unmounted respectively', () => { + const {unmount} = renderWithContext(); + + expect(baseProps.registerSaveAction).toHaveBeenCalledTimes(1); - // To test deleteBrandImage path, unmount then re-mount with fetch returning 200 unmount(); - jest.clearAllMocks(); - (global.fetch as jest.Mock).mockResolvedValueOnce({status: 200} as Response); - let saveAction2: (() => Promise) | undefined; - const registerSaveAction2 = jest.fn((fn: () => Promise) => { - saveAction2 = fn; - }); + expect(baseProps.unRegisterSaveAction).toHaveBeenCalledTimes(1); + }); - renderWithContext( - , - ); + test('should show delete button if brand image exists', async () => { + renderWithContext(); - await waitFor(() => { - expect(registerSaveAction2).toHaveBeenCalled(); - }); - expect(saveAction2).toBeDefined(); + await waitFor(() => expect(scope.isDone()).toBe(true)); + + expect(screen.getByTestId(deleteButtonTestId)).toBeVisible(); + }); + + test('should hide delete button if the setting is disabled', async () => { + const props = {...baseProps, disabled: true}; + + renderWithContext(); + + await waitFor(() => expect(screen.queryByTestId(deleteButtonTestId)).toBe(null)); + }); + + test('should call setSaveNeeded when a brand image is uploaded', async () => { + renderWithContext(); + + await userEvent.upload(screen.getByTestId('file__upload-input'), new File(['brand_image_file'], 'brand_image_file.png', {type: 'image/png'})); + + expect(baseProps.setSaveNeeded).toHaveBeenCalledTimes(1); + }); + + test('should call setSaveNeeded when the delete button is pressed', async () => { + renderWithContext(); + + const deleteButton = await screen.findByTestId(deleteButtonTestId); - // Wait for the brand image to be detected and delete button to appear - await waitFor(() => { - expect(screen.getByText('×')).toBeInTheDocument(); - }); - const deleteButton = screen.getByText('×').closest('button')!; await userEvent.click(deleteButton); - await waitFor(() => { - expect(screen.getByText('No brand image uploaded')).toBeInTheDocument(); - }); - - await saveAction2!(); - expect(deleteBrandImage).toHaveBeenCalledTimes(1); - expect(uploadBrandImage).toHaveBeenCalledTimes(0); + expect(baseProps.setSaveNeeded).toHaveBeenCalledTimes(1); }); }); diff --git a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx index f058e9bbc0c..7a155f07304 100644 --- a/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx +++ b/webapp/channels/src/components/admin_console/brand_image_setting/brand_image_setting.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {memo, useCallback, useEffect, useRef, useState} from 'react'; import {FormattedMessage} from 'react-intl'; import {Client4} from 'mattermost-redux/client'; @@ -9,6 +9,7 @@ import {Client4} from 'mattermost-redux/client'; import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; import SettingSet from 'components/admin_console/setting_set'; +import useDidUpdate from 'components/common/hooks/useDidUpdate'; import FormError from 'components/form_error'; import WithTooltip from 'components/with_tooltip'; @@ -44,242 +45,224 @@ type Props = { unRegisterSaveAction: (saveAction: () => Promise) => void; }; -type State = { - deleteBrandImage: boolean; - brandImage?: Blob; - brandImageExists: boolean; - brandImageTimestamp: number; - error: string; -}; +const BrandImageSetting = ({ + id, + disabled, + setSaveNeeded, + registerSaveAction, + unRegisterSaveAction, +}: Props) => { + const imageRef = useRef(null); + const fileInputRef = useRef(null); -export default class BrandImageSetting extends React.PureComponent { - private imageRef: React.RefObject; - private fileInputRef: React.RefObject; + const [brandImage, setBrandImage] = useState(); + const [shouldDeleteBrandImage, setShouldDeleteBrandImage] = useState(false); + const [brandImageExists, setBrandImageExists] = useState(false); + const [brandImageTimestamp, setBrandImageTimestamp] = useState(Date.now()); - constructor(props: Props) { - super(props); + const [errorFromState, setErrorFromState] = useState(''); - this.state = { - deleteBrandImage: false, - brandImageExists: false, - brandImageTimestamp: Date.now(), - error: '', - }; + const handleSave = useCallback(async () => { + setErrorFromState(''); - this.imageRef = React.createRef(); - this.fileInputRef = React.createRef(); - } + let error; + if (shouldDeleteBrandImage) { + await deleteBrandImage( + () => { + setShouldDeleteBrandImage(false); + setBrandImageExists(false); + setBrandImage(undefined); + }, + (err: Error) => { + error = err; + setErrorFromState(err.message); + }, + ); + } else if (brandImage) { + await uploadBrandImage( + brandImage, + () => { + setBrandImageExists(true); + setBrandImage(undefined); + setBrandImageTimestamp(Date.now()); + }, + (err: Error) => { + error = err; + setErrorFromState(err.message); + }, + ); + } + return {error}; + }, [brandImage, shouldDeleteBrandImage]); - componentDidMount() { + useEffect(() => { fetch( - Client4.getBrandImageUrl(String(this.state.brandImageTimestamp)), + Client4.getBrandImageUrl(String(Date.now())), ).then((resp) => { if (resp.status === HTTP_STATUS_OK) { - this.setState({brandImageExists: true}); + setBrandImageExists(true); } else { - this.setState({brandImageExists: false}); + setBrandImageExists(false); } }).catch((err) => { console.error(`unable to retrieve brand image: ${err}`); //eslint-disable-line no-console - this.setState({brandImageExists: false}); + setBrandImageExists(false); }); + }, []); - this.props.registerSaveAction(this.handleSave); - } + useEffect(() => { + registerSaveAction(handleSave); - componentWillUnmount() { - this.props.unRegisterSaveAction(this.handleSave); - } + return () => { + unRegisterSaveAction(handleSave); + }; + }, [handleSave, registerSaveAction, unRegisterSaveAction]); - componentDidUpdate() { - if (this.imageRef.current) { + useDidUpdate(() => { + if (imageRef.current) { const reader = new FileReader(); - const img = this.imageRef.current; + const img = imageRef.current; reader.onload = (e) => { const src = - e.target?.result instanceof ArrayBuffer ? e.target?.result.toString() : e.target?.result; + e.target?.result instanceof ArrayBuffer ? e.target?.result.toString() : e.target?.result; if (src) { img.setAttribute('src', src); } }; - if (this.state.brandImage) { - reader.readAsDataURL(this.state.brandImage); + if (brandImage) { + reader.readAsDataURL(brandImage); } } - } + }, [brandImage]); - handleSelectClick = () => { - this.fileInputRef.current?.click(); - }; + const handleSelectClick = useCallback(() => { + fileInputRef.current?.click(); + }, []); - handleImageChange = () => { - if (!this.fileInputRef.current) { + const handleImageChange = useCallback(() => { + if (!fileInputRef.current) { return; } - const element = this.fileInputRef.current; + const element = fileInputRef.current; if (element.files && element.files.length > 0) { - this.props.setSaveNeeded(); - this.setState({ - brandImage: element.files[0], - deleteBrandImage: false, - }); + setSaveNeeded(); + setBrandImage(element.files[0]); + setShouldDeleteBrandImage(false); } - }; + }, [setSaveNeeded]); - handleDeleteButtonPressed = () => { - this.setState({ - deleteBrandImage: true, - brandImage: undefined, - brandImageExists: false, - }); - this.props.setSaveNeeded(); - }; + const handleDeleteButtonPressed = useCallback(() => { + setShouldDeleteBrandImage(true); + setBrandImage(undefined); + setBrandImageExists(false); - handleSave = async () => { - this.setState({ - error: '', - }); + setSaveNeeded(); + }, [setSaveNeeded]); - let error; - if (this.state.deleteBrandImage) { - await deleteBrandImage( - () => { - this.setState({ - deleteBrandImage: false, - brandImageExists: false, - brandImage: undefined, - }); - }, - (err: Error) => { - error = err; - this.setState({ - error: err.message, - }); - }, - ); - } else if (this.state.brandImage) { - await uploadBrandImage( - this.state.brandImage, - () => { - this.setState({ - brandImageExists: true, - brandImage: undefined, - brandImageTimestamp: Date.now(), - }); - }, - (err: Error) => { - error = err; - this.setState({ - error: err.message, - }); - }, - ); - } - return {error}; - }; - - render() { - let img = null; - if (this.state.brandImage) { - img = ( -
- brand image -
- ); - } else if (this.state.brandImageExists) { - let overlay; - if (!this.props.disabled) { - overlay = ( - - )} - isVertical={false} - > - - - ); - } - img = ( -
- brand image - {overlay} -
- ); - } else { - img = ( -

- -

- ); - } - - return ( - - } - label={ - - } - setByEnv={false} - > -
-
{img}
-
-
+ let img = null; + if (brandImage) { + img = ( +
+ brand image +
+ ); + } else if (brandImageExists) { + let overlay; + if (!disabled) { + overlay = ( + + )} + isVertical={false} + > - -
- - + + ); + } + img = ( +
+ brand image + {overlay} +
+ ); + } else { + img = ( +

+ +

); } -} + + return ( + + } + label={ + + } + setByEnv={false} + > +
+
{img}
+
+
+ + +
+ + + ); +}; + +export default memo(BrandImageSetting); diff --git a/webapp/channels/src/components/apps_form/apps_form_component.tsx b/webapp/channels/src/components/apps_form/apps_form_component.tsx index 39efda5be43..27228735ec7 100644 --- a/webapp/channels/src/components/apps_form/apps_form_component.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_component.tsx @@ -237,6 +237,12 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => { }; export class AppsForm extends React.PureComponent { + // Cache sanitized fields to preserve object identity across renders. + // Without this, createSanitizedField() returns a new object every render, + // causing AppsFormSelectField to remount its AsyncSelect (via refreshNonce), + // which re-triggers dynamic select lookups on every keystroke in any field. + private sanitizedFieldCache = new Map(); + constructor(props: Props) { super(props); @@ -271,6 +277,14 @@ export class AppsForm extends React.PureComponent { return null; } + componentDidUpdate(prevProps: Props) { + // Clear sanitized field cache when the form changes (e.g., refresh or multistep) + // so stale entries don't accumulate. + if (prevProps.form !== this.props.form) { + this.sanitizedFieldCache.clear(); + } + } + updateErrors = (elements: DialogElement[], fieldErrors?: {[x: string]: string}, formError?: string): boolean => { let hasErrors = false; const state = {} as State; @@ -657,8 +671,13 @@ export class AppsForm extends React.PureComponent { } return fields.filter((f) => f.name !== form.submit_buttons).map((originalField, index) => { - // Use sanitized field for safe usage in components - const field = createSanitizedField(originalField); + // Use cached sanitized field to preserve object identity across renders. + // This prevents AsyncSelect remounts that trigger spurious lookup calls. + let field = this.sanitizedFieldCache.get(originalField); + if (!field) { + field = createSanitizedField(originalField); + this.sanitizedFieldCache.set(originalField, field); + } return ( { ); } case AppFieldTypes.RADIO: { - const radioValue = value as string; + // Radio values may be stored as AppSelectOption objects (from initial default) + // or plain strings (after user interaction via RadioSetting.onChange) + const radioValue = isAppSelectOption(value) ? value.value : (value as string) ?? ''; return ( { const mockCall = MockAppsFormContainer.mock.calls[0][0]; const submitAdapter = mockCall.actions.doAppSubmit; - // Test with null value for required field - should not crash + // Test with null value for required field - should not crash. + // processFormValues normalizes null to '' in accumulatedValues, + // so the validation sees an empty string (not null) and does not + // produce a required-field error. const result = await submitAdapter({ values: { 'text-field': 'valid', - 'required-field': null, // Missing required field + 'required-field': null, // Cleared field — normalized to '' }, }); - // Should complete successfully (null values are simply skipped) + // Should complete successfully (null values are normalized to empty strings) expect(result.data?.type).toBe('ok'); - expect(mockConsole.warn).toHaveBeenCalledWith( - '[InteractiveDialogAdapter]', - 'Form submission validation errors', - expect.objectContaining({ - errorCount: expect.any(Number), - errors: expect.arrayContaining([ - expect.objectContaining({ - field: expect.stringContaining('required-field'), - message: expect.any(String), - }), - ]), - }), - ); }); }); @@ -2305,5 +2295,131 @@ describe('components/interactive_dialog/InteractiveDialogAdapter', () => { data: {items: []}, }); }); + + test('should include selected_field in refresh submission', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'category', + type: 'select', + display_name: 'Category', + help_text: '', + placeholder: '', + default: '', + optional: false, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'Option A', value: 'a'}, + {text: 'Option B', value: 'b'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // Trigger refresh with selected_field set + await refreshHandler({ + selected_field: 'category', + values: {category: 'a'}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + url: '/plugins/myplugin/refresh', + submission: expect.objectContaining({ + selected_field: 'category', + category: 'a', + }), + }), + ); + }); + + test('should send empty string for cleared select field values in refresh', async () => { + const mockSubmitDialog = jest.fn().mockResolvedValue({data: {}}); + + const refreshSelectElement: DialogElement = { + name: 'priority', + type: 'select', + display_name: 'Priority', + help_text: '', + placeholder: '', + default: '', + optional: true, + max_length: 0, + min_length: 0, + subtype: '', + data_source: '', + options: [ + {text: 'High', value: 'high'}, + {text: 'Low', value: 'low'}, + ], + }; + + const props = { + ...baseProps, + sourceUrl: '/plugins/myplugin/refresh', + elements: [refreshSelectElement], + actions: { + submitInteractiveDialog: mockSubmitDialog, + lookupInteractiveDialog: jest.fn().mockResolvedValue({data: {items: []}}), + }, + }; + + const {getByTestId} = renderWithContext( + , + ); + + await waitFor(() => { + expect(getByTestId('apps-form-container')).toBeInTheDocument(); + }); + + // Get the refresh handler from the MockAppsFormContainer + const mockCall = MockAppsFormContainer.mock.calls[0][0]; + const refreshHandler = mockCall.actions.doAppFetchForm; + + // First refresh sets a value so it gets accumulated + await refreshHandler({ + selected_field: 'priority', + values: {priority: 'high'}, + }); + + mockSubmitDialog.mockClear(); + + // Second refresh clears the field (null simulates a cleared select) + await refreshHandler({ + selected_field: 'priority', + values: {priority: null}, + }); + + expect(mockSubmitDialog).toHaveBeenCalledWith( + expect.objectContaining({ + submission: expect.objectContaining({ + priority: '', + }), + }), + ); + }); }); }); diff --git a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx index 78839f01f6e..813f2f81013 100644 --- a/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx +++ b/webapp/channels/src/components/dialog_router/interactive_dialog_adapter.tsx @@ -169,13 +169,12 @@ class InteractiveDialogAdapter extends React.PureComponent { * Common logic for processing form values - used by both submit and refresh */ private processFormValues = (currentValues: Record): void => { - // Normalize current values to extract primitive values from select objects - const normalizedCurrentValues = extractPrimitiveValues(currentValues); - - // Accumulate values: merge current with existing accumulated values + // Normalize current values to extract primitive values from select objects. + // clearEmptyFields=true ensures cleared fields emit '' or [] so they + // overwrite any previously accumulated value for that key. this.accumulatedValues = { - ...this.accumulatedValues, // Previous steps' values (including other pages) - ...normalizedCurrentValues, // Current form normalized values + ...this.accumulatedValues, + ...extractPrimitiveValues(currentValues, true), }; }; @@ -533,8 +532,15 @@ class InteractiveDialogAdapter extends React.PureComponent { const currentValues = call.values || {}; this.processFormValues(currentValues); - // For refresh, send all accumulated normalized values - const refreshPayload = this.accumulatedValues; + // For refresh, use a shallow copy so that adding selected_field + // does not permanently contaminate this.accumulatedValues + const refreshPayload = {...this.accumulatedValues}; + + // Include the changed field name in the submission so the plugin + // knows which field triggered the refresh + if (call.selected_field) { + refreshPayload.selected_field = call.selected_field; + } const refreshSubmission: DialogSubmission = { url: this.props.sourceUrl, diff --git a/webapp/channels/src/components/login/login.test.tsx b/webapp/channels/src/components/login/login.test.tsx index 2f901fb0890..fc87d76a110 100644 --- a/webapp/channels/src/components/login/login.test.tsx +++ b/webapp/channels/src/components/login/login.test.tsx @@ -408,6 +408,25 @@ describe('components/login/Login', () => { expect(history.push).toHaveBeenCalledWith(redirectPath); }); + it('should not show dont have an account when open server is disabled', () => { + const state = mergeObjects(baseState, { + entities: { + general: { + config: { + EnableOpenServer: 'false', + }, + }, + }, + }); + + renderWithContext( + , + state, + ); + + expect(screen.queryByText('Don\'t have an account')).not.toBeInTheDocument(); + }); + describe('EnableGuestMagicLink', () => { it('should show password field when EnableGuestMagicLink is false', async () => { const state = mergeObjects(baseState, { diff --git a/webapp/channels/src/components/login/login.tsx b/webapp/channels/src/components/login/login.tsx index f44c9185e01..4466cac1ca9 100644 --- a/webapp/channels/src/components/login/login.tsx +++ b/webapp/channels/src/components/login/login.tsx @@ -437,23 +437,18 @@ const Login = ({onCustomizeHeader}: LoginProps) => { }, [sessionExpired, formatMessage, onDismissSessionExpired, extraParam, siteName, searchParam]); const getAlternateLink = useCallback(() => { + if (!showSignup) { + return undefined; + } + const linkLabel = formatMessage({ id: 'login.noAccount', defaultMessage: 'Don\'t have an account?', }); - if (showSignup) { - return ( - - ); - } return ( ); diff --git a/webapp/channels/src/components/root/root.tsx b/webapp/channels/src/components/root/root.tsx index 38c71e840f0..f7db6c6cb3f 100644 --- a/webapp/channels/src/components/root/root.tsx +++ b/webapp/channels/src/components/root/root.tsx @@ -50,7 +50,6 @@ const MobileViewWatcher = makeAsyncComponent('MobileViewWatcher', lazy(() => imp const WindowSizeObserver = makeAsyncComponent('WindowSizeObserver', lazy(() => import('components/window_size_observer/WindowSizeObserver'))); const ErrorPage = makeAsyncComponent('ErrorPage', lazy(() => import('components/error_page'))); const Login = makeAsyncComponent('LoginController', lazy(() => import('components/login/login'))); -const AccessProblem = makeAsyncComponent('AccessProblem', lazy(() => import('components/access_problem'))); const PasswordResetSendLink = makeAsyncComponent('PasswordResedSendLink', lazy(() => import('components/password_reset_send_link'))); const PasswordResetForm = makeAsyncComponent('PasswordResetForm', lazy(() => import('components/password_reset_form'))); const Signup = makeAsyncComponent('SignupController', lazy(() => import('components/signup/signup'))); @@ -320,10 +319,6 @@ export default class Root extends React.PureComponent { path={'/login'} component={Login} /> - { } as DialogElement; const result = getDefaultValue(element); - expect(result).toEqual({ - label: 'Option 1', - value: 'option1', - }); + + // Radio defaults are plain strings (not {label, value} objects) + // because RadioSetting.onChange returns e.target.value (a string) + expect(result).toBe('option1'); + }); + + it('should return null for radio default that does not match any option', () => { + const element = { + type: 'radio', + default: 'stale_value', + options: [ + {text: 'Option 1', value: 'option1'}, + {text: 'Option 2', value: 'option2'}, + ], + } as DialogElement; + + const result = getDefaultValue(element); + expect(result).toBeNull(); }); it('should handle dynamic select defaults', () => { @@ -973,6 +988,48 @@ describe('dialog_conversion', () => { expect(form.fields?.[0].refresh).toBe(true); }); + it('should handle refresh property for date and datetime fields', () => { + const elements: DialogElement[] = [ + { + name: 'refreshable_date', + type: 'date', + display_name: 'Refreshable Date', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'refreshable_datetime', + type: 'datetime', + display_name: 'Refreshable Datetime', + optional: false, + refresh: true, + } as DialogElement, + { + name: 'non_refreshable_date', + type: 'date', + display_name: 'Non-Refreshable Date', + optional: false, + } as DialogElement, + ]; + + const {form, errors} = convertDialogToAppForm( + elements, + 'Test Dialog', + undefined, + undefined, + undefined, + 'http://example.com/source', + '', + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(form.fields).toHaveLength(3); + expect(form.fields?.[0].refresh).toBe(true); + expect(form.fields?.[1].refresh).toBe(true); + expect(form.fields?.[2].refresh).toBeUndefined(); + }); + it('should include state in submit and source AppCall objects', () => { const elements: DialogElement[] = [ { @@ -1234,6 +1291,48 @@ describe('dialog_conversion', () => { }); }); + it('should extract value from radio field stored as AppSelectOption object', () => { + const values = { + radio_object: {label: 'Option A', value: 'optA'}, + radio_string: 'optB', + } as unknown as AppFormValues; + + const elements: DialogElement[] = [ + { + name: 'radio_object', + type: 'radio', + display_name: 'Radio Object Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + { + name: 'radio_string', + type: 'radio', + display_name: 'Radio String Field', + optional: false, + options: [ + {text: 'Option A', value: 'optA'}, + {text: 'Option B', value: 'optB'}, + ], + } as DialogElement, + ]; + + const {submission, errors} = convertAppFormValuesToDialogSubmission( + values, + elements, + legacyOptions, + ); + + expect(errors).toHaveLength(0); + expect(submission).toEqual({ + radio_object: 'optA', + radio_string: 'optB', + }); + }); + it('should handle textarea field values', () => { const values = { textarea_field: 'Long text content', @@ -1682,4 +1781,154 @@ describe('dialog_conversion', () => { }); }); }); + + describe('extractPrimitiveValues', () => { + it('should extract value from a single select option', () => { + const result = extractPrimitiveValues({ + color: {label: 'Red', value: 'red'}, + }); + expect(result).toEqual({color: 'red'}); + }); + + it('should extract values from a multiselect array', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + it('should pass through primitive strings', () => { + const result = extractPrimitiveValues({ + name: 'hello', + }); + expect(result).toEqual({name: 'hello'}); + }); + + it('should pass through booleans', () => { + const result = extractPrimitiveValues({ + enabled: true, + disabled: false, + }); + expect(result).toEqual({enabled: true, disabled: false}); + }); + + it('should skip null, undefined, empty string, and values', () => { + const result = extractPrimitiveValues({ + a: null, + b: undefined, + c: '', + d: '', + e: 'keep', + }); + expect(result).toEqual({e: 'keep'}); + }); + + it('should skip select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }); + expect(result).toEqual({}); + }); + + it('should skip empty multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [], + }); + expect(result).toEqual({}); + }); + + it('should pass through primitive string arrays', () => { + const result = extractPrimitiveValues({ + dates: ['2026-01-01', '2026-01-15'], + }); + expect(result).toEqual({dates: ['2026-01-01', '2026-01-15']}); + }); + + it('should handle mixed arrays of select options and primitives', () => { + const result = extractPrimitiveValues({ + items: [ + {label: 'Red', value: 'red'}, + 'already-extracted', + ], + }); + expect(result).toEqual({items: ['red', 'already-extracted']}); + }); + + it('should filter out meaningless values from multiselect arrays', () => { + const result = extractPrimitiveValues({ + colors: [ + {label: 'Red', value: 'red'}, + {label: 'Empty', value: ''}, + {label: 'Blue', value: 'blue'}, + ], + }); + expect(result).toEqual({colors: ['red', 'blue']}); + }); + + describe('with clearEmptyFields=true', () => { + it('should emit empty string for null values', () => { + const result = extractPrimitiveValues({a: null}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for undefined values', () => { + const result = extractPrimitiveValues({a: undefined}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for empty string values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty string for values', () => { + const result = extractPrimitiveValues({a: ''}, true); + expect(result).toEqual({a: ''}); + }); + + it('should emit empty array for empty multiselect arrays', () => { + const result = extractPrimitiveValues({colors: []}, true); + expect(result).toEqual({colors: []}); + }); + + it('should emit empty string for select option with empty value', () => { + const result = extractPrimitiveValues({ + color: {label: '', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should emit empty string for select option with value', () => { + const result = extractPrimitiveValues({ + color: {label: 'None', value: ''}, + }, true); + expect(result).toEqual({color: ''}); + }); + + it('should still extract meaningful values normally', () => { + const result = extractPrimitiveValues({ + name: 'hello', + color: {label: 'Red', value: 'red'}, + cleared: null, + emptied: [], + }, true); + expect(result).toEqual({ + name: 'hello', + color: 'red', + cleared: '', + emptied: [], + }); + }); + }); + }); }); diff --git a/webapp/channels/src/utils/dialog_conversion.ts b/webapp/channels/src/utils/dialog_conversion.ts index 345aca40757..63e708970fb 100644 --- a/webapp/channels/src/utils/dialog_conversion.ts +++ b/webapp/channels/src/utils/dialog_conversion.ts @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {AppForm, AppField, AppFormValue, AppSelectOption, AppFormValues} from '@mattermost/types/apps'; +import {isAppSelectOption, type AppForm, type AppField, type AppFormValue, type AppSelectOption, type AppFormValues} from '@mattermost/types/apps'; import type {DialogElement} from '@mattermost/types/integrations'; import {AppFieldTypes} from 'mattermost-redux/constants/apps'; @@ -253,10 +253,19 @@ export function getDefaultValue(element: DialogElement): AppFormValue { return boolString === 'true' || boolString === '1' || boolString === 'yes'; } - case DialogElementTypes.SELECT: case DialogElementTypes.RADIO: { + // Radio values are always plain strings (RadioSetting.onChange returns e.target.value). + // Normalize to string from the start so the value shape never changes after user interaction. + if (element.options && element.default) { + const match = element.options.find((opt) => opt.value === element.default); + return match ? match.value : null; + } + return element.default ? String(element.default) : null; + } + + case DialogElementTypes.SELECT: { // Handle dynamic selects that use data_source instead of static options - if (element.type === 'select' && element.data_source === 'dynamic' && element.default) { + if (element.data_source === 'dynamic' && element.default) { if (element.multiselect) { const values = Array.isArray(element.default) ? element.default : @@ -274,7 +283,7 @@ export function getDefaultValue(element: DialogElement): AppFormValue { if (element.options && element.default) { // Handle multiselect defaults (comma-separated values) - if (element.type === 'select' && element.multiselect) { + if (element.multiselect) { const defaultValues = Array.isArray(element.default) ? element.default : String(element.default).split(',').map((val) => val.trim()); const defaultOptions = defaultValues.map((value) => { @@ -439,6 +448,13 @@ export function convertElement(element: DialogElement, options: ConversionOption } } + // Add refresh support for bool fields + if (element.type === DialogElementTypes.BOOL) { + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } + } + // Add date/datetime specific properties if (element.type === DialogElementTypes.DATE || element.type === DialogElementTypes.DATETIME) { // Use datetime_config if provided @@ -456,6 +472,10 @@ export function convertElement(element: DialogElement, options: ConversionOption if (element.time_interval !== undefined && element.type === DialogElementTypes.DATETIME) { appField.time_interval = Number(element.time_interval); } + + if (element.refresh !== undefined) { + appField.refresh = element.refresh; + } } return {field: appField, errors}; @@ -567,42 +587,34 @@ export function convertDialogToAppForm( /** * Extract primitive values from form field objects for storage/submission * Converts select option objects {label: "Text", value: "val"} to primitive "val" - * Filters out null, undefined, empty, and "" values + * Filters out null, undefined, empty, and "" values unless clearEmptyFields is true, + * in which case empty/null fields are emitted as '' or [] so they can overwrite prior values. */ -export function extractPrimitiveValues(values: Record): Record { - const normalized: Record = {}; - - Object.entries(values).forEach(([key, value]) => { - // Skip null, undefined, empty string, and "" values - if (value === null || value === undefined || value === '' || value === '') { - return; - } +export function extractPrimitiveValues(values: Record, clearEmptyFields = false): Record { + const isMeaningful = (v: any): v is string | boolean => v != null && v !== '' && v !== ''; + return Object.entries(values).reduce>((acc, [key, value]) => { if (Array.isArray(value)) { - // Handle multiselect arrays - extract values from each option object - const extractedValues = value. - filter((item) => item && typeof item === 'object' && 'value' in item). - map((item) => item.value). - filter((val) => val !== null && val !== undefined && val !== '' && val !== ''); + const extracted = value. + map((item) => (isAppSelectOption(item) ? item.value : item)). + filter(isMeaningful); - if (extractedValues.length > 0) { - normalized[key] = extractedValues; + if (extracted.length > 0 || clearEmptyFields) { + acc[key] = extracted; } - } else if (value && typeof value === 'object' && 'value' in value) { - // Extract value from single select option object {label: "...", value: "..."} - const extractedValue = value.value; - - // Only store if the extracted value is meaningful - if (extractedValue !== null && extractedValue !== undefined && extractedValue !== '' && extractedValue !== '') { - normalized[key] = extractedValue; + } else if (isAppSelectOption(value)) { + if (isMeaningful(value.value)) { + acc[key] = value.value; + } else if (clearEmptyFields) { + acc[key] = ''; } - } else { - // Keep primitive values as-is (but skip empty/nil values) - normalized[key] = value; + } else if (isMeaningful(value)) { + acc[key] = value; + } else if (clearEmptyFields) { + acc[key] = ''; } - }); - - return normalized; + return acc; + }, {}); } /** @@ -693,7 +705,13 @@ export function convertAppFormValuesToDialogSubmission( break; case DialogElementTypes.RADIO: - submission[element.name] = String(value); + // Radio values are normally plain strings, but accept {label, value} + // objects for backwards compatibility with older code paths. + if (isAppSelectOption(value)) { + submission[element.name] = value.value; + } else { + submission[element.name] = String(value); + } break; case DialogElementTypes.SELECT: diff --git a/webapp/platform/types/src/apps.ts b/webapp/platform/types/src/apps.ts index 2b61571f0fe..d7b5f069f18 100644 --- a/webapp/platform/types/src/apps.ts +++ b/webapp/platform/types/src/apps.ts @@ -417,7 +417,7 @@ export type AppSelectOption = { icon_data?: string; }; -function isAppSelectOption(v: unknown): v is AppSelectOption { +export function isAppSelectOption(v: unknown): v is AppSelectOption { if (typeof v !== 'object' || v === null) { return false; }