From 6938cabac6b7d177962b74bc84df75b57b16d233 Mon Sep 17 00:00:00 2001 From: "cursor[bot]" <206951365+cursor[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:11:16 +0000 Subject: [PATCH 1/5] [MM-69646] Disallow MoveThreadsEnabled feature flag (fail server startup) (#37966) * [MM-69646] Disallow MoveThreadsEnabled feature flag Reject the MoveThreadsEnabled feature flag during config validation so the server fails to start while it is enabled. The feature is being retired in favor of Wrangler and will be removed later. Co-authored-by: mattermost-code * [MM-69646] Cover nil FeatureFlags guard in config validation test Co-authored-by: mattermost-code * Move MoveThreadsEnabled comment into isValid method body Keep isValid's doc comment generic since it will validate more flag combinations in the future, and place the MoveThreadsEnabled-specific rationale next to the actual flag check. * [MM-69646] Update TestMoveThread for retired MoveThreadsEnabled flag Config.IsValid now rejects enabling MoveThreadsEnabled, so the move-thread API stays disabled. Replace the enabled-path suite with assertions that the flag cannot be turned on and MoveThread returns 501. Co-authored-by: mattermost-code * [MM-69646] Stop forcing MoveThreadsEnabled in e2e environments E2E was setting MM_FEATUREFLAGS_MOVETHREADSENABLED=true, which now fails Config.IsValid and prevents the test server from starting. Remove the override and skip Cypress move-thread specs that require the retired flag. Co-authored-by: mattermost-code * [MM-69646] Skip TestMoveThread instead of asserting disabled flag Mirror the E2E describe.skip approach: retain the original TestMoveThread body and skip it at the top, since MoveThreadsEnabled is retired and rejected by Config.IsValid. * [MM-69646] Park cursor away from post dot menu in edit_file_attachment specs --------- Co-authored-by: Cursor Agent Co-authored-by: mattermost-code Co-authored-by: Mattermost Build Co-authored-by: Jesse Hallam --- e2e-tests/.ci/server.generate.sh | 1 - .../move_thread/move_thread_from_dm_spec.js | 4 ++- .../move_thread/move_thread_from_gm_spec.js | 4 ++- .../move_thread_from_private_channel_spec.js | 4 ++- .../move_thread_from_public_channel_spec.js | 4 ++- .../lib/src/containers/env_baseline.ts | 1 - .../edit_file_attachment.spec.ts | 32 +++++++++++++----- server/channels/api4/post_test.go | 4 +++ server/i18n/en.json | 4 +++ server/public/model/config.go | 6 ++++ server/public/model/config_test.go | 33 +++++++++++++++++++ server/public/model/feature_flags.go | 12 +++++++ 12 files changed, 94 insertions(+), 15 deletions(-) diff --git a/e2e-tests/.ci/server.generate.sh b/e2e-tests/.ci/server.generate.sh index a3ccd9bc92a5..537616e2d04e 100755 --- a/e2e-tests/.ci/server.generate.sh +++ b/e2e-tests/.ci/server.generate.sh @@ -64,7 +64,6 @@ services: MM_EMAILSETTINGS_SMTPSERVER: "localhost" MM_CLUSTERSETTINGS_READONLYCONFIG: "false" MM_SERVICEENVIRONMENT: "test" - MM_FEATUREFLAGS_MOVETHREADSENABLED: "true" MM_FEATUREFLAGS_CUSTOMPROFILEATTRIBUTES: "true" MM_FEATUREFLAGS_PERMISSIONPOLICIES: "true" MM_FEATUREFLAGS_TEAMMEMBERSHIPACCESSCONTROL: "true" diff --git a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_dm_spec.js b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_dm_spec.js index 592c5bfbbff7..2d65a7df1b0e 100644 --- a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_dm_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_dm_spec.js @@ -12,7 +12,9 @@ import * as TIMEOUTS from '@/fixtures/timeouts'; -describe('Move Thread', () => { +// Skipped: MoveThreadsEnabled is retired and rejected by Config.IsValid (MM-69646). +// These specs require the flag and cannot run while the server refuses to enable it. +describe.skip('Move Thread', () => { let user1; let user2; let testTeam; diff --git a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_gm_spec.js b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_gm_spec.js index 906042fe16b1..b761ea2e29ce 100644 --- a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_gm_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_gm_spec.js @@ -12,7 +12,9 @@ import * as TIMEOUTS from '@/fixtures/timeouts'; -describe('Move thread', () => { +// Skipped: MoveThreadsEnabled is retired and rejected by Config.IsValid (MM-69646). +// These specs require the flag and cannot run while the server refuses to enable it. +describe.skip('Move thread', () => { let user1; let user2; let user3; diff --git a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_private_channel_spec.js b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_private_channel_spec.js index 094783b0819d..fbff62361502 100644 --- a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_private_channel_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_private_channel_spec.js @@ -12,7 +12,9 @@ import * as TIMEOUTS from '@/fixtures/timeouts'; -describe('Move thread', () => { +// Skipped: MoveThreadsEnabled is retired and rejected by Config.IsValid (MM-69646). +// These specs require the flag and cannot run while the server refuses to enable it. +describe.skip('Move thread', () => { let user1; let testTeam; let privateChannel; diff --git a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_public_channel_spec.js b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_public_channel_spec.js index 42a64a563be9..50bf871cc91e 100644 --- a/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_public_channel_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/move_thread/move_thread_from_public_channel_spec.js @@ -10,7 +10,9 @@ // Stage: @prod // Group: @channels @enterprise @messaging -describe('Move Thread', () => { +// Skipped: MoveThreadsEnabled is retired and rejected by Config.IsValid (MM-69646). +// These specs require the flag and cannot run while the server refuses to enable it. +describe.skip('Move Thread', () => { let user1; let user2; let user3; diff --git a/e2e-tests/playwright/lib/src/containers/env_baseline.ts b/e2e-tests/playwright/lib/src/containers/env_baseline.ts index 1419149a980e..638fdad1c076 100644 --- a/e2e-tests/playwright/lib/src/containers/env_baseline.ts +++ b/e2e-tests/playwright/lib/src/containers/env_baseline.ts @@ -19,7 +19,6 @@ export const SERVER_ENV_BASELINE: Record = { // Feature flags this test suite needs on, off by default in the server MM_FEATUREFLAGS_ATTRIBUTEVALUEMASKING: 'true', MM_FEATUREFLAGS_ENABLEREMOTECLUSTERSERVICE: 'true', - MM_FEATUREFLAGS_MOVETHREADSENABLED: 'true', MM_FEATUREFLAGS_PERMISSIONPOLICIES: 'true', MM_FEATUREFLAGS_PROPERTYFIELDRANK: 'true', MM_FEATUREFLAGS_RECURRINGSCHEDULEDPOSTS: 'true', diff --git a/e2e-tests/playwright/specs/functional/channels/file_attachments/edit_file_attachment.spec.ts b/e2e-tests/playwright/specs/functional/channels/file_attachments/edit_file_attachment.spec.ts index 670d8bd0d703..375709990f45 100644 --- a/e2e-tests/playwright/specs/functional/channels/file_attachments/edit_file_attachment.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/file_attachments/edit_file_attachment.spec.ts @@ -13,7 +13,7 @@ test('MM-T5654_1 should be able to add attachments while editing a post', {tag: // # Initialize user and login const {user} = await pw.initSetup(); - const {channelsPage} = await pw.testBrowser.login(user); + const {channelsPage, page} = await pw.testBrowser.login(user); // # Navigate to channels page and post a message await channelsPage.goto(); @@ -28,6 +28,7 @@ test('MM-T5654_1 should be able to add attachments while editing a post', {tag: // # Open the dot menu and click edit await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); @@ -59,6 +60,7 @@ test('MM-T5654_2 should be able to add attachments while editing a threaded post // open the dot menu await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.replyMenuItem.click(); await channelsPage.sidebarRight.toBeVisible(); @@ -71,6 +73,7 @@ test('MM-T5654_2 should be able to add attachments while editing a threaded post await replyPost.hover(); await replyPost.postMenu.toBeVisible(); await replyPost.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.sidebarRight.postEdit.toBeVisible(); @@ -85,6 +88,7 @@ test('MM-T5654_2 should be able to add attachments while editing a threaded post await updatedReplyPost.hover(); await updatedReplyPost.postMenu.toBeVisible(); await updatedReplyPost.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.sidebarRight.postEdit.toBeVisible(); @@ -103,7 +107,7 @@ test('MM-T5654_2 should be able to add attachments while editing a threaded post await updatedReplyPost.hover(); await updatedReplyPost.postMenu.toBeVisible(); await updatedReplyPost.postMenu.clickOnDotMenu(); - await moveMouseToCenter(page); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.sidebarRight.postEdit.toBeVisible(); @@ -124,7 +128,7 @@ test('MM-T5654_3 should be able to edit post message originally containing files const originalMessage = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit'; const {user} = await pw.initSetup(); - const {channelsPage} = await pw.testBrowser.login(user); + const {channelsPage, page} = await pw.testBrowser.login(user); await channelsPage.goto(); await channelsPage.toBeVisible(); @@ -137,6 +141,7 @@ test('MM-T5654_3 should be able to edit post message originally containing files // open the dot menu await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.centerView.postEdit.toBeVisible(); @@ -152,7 +157,7 @@ test('MM-T5654_4 should be able to add files when editing a post', async ({pw}) const originalMessage = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit'; const {user} = await pw.initSetup(); - const {channelsPage} = await pw.testBrowser.login(user); + const {channelsPage, page} = await pw.testBrowser.login(user); await channelsPage.goto(); await channelsPage.toBeVisible(); @@ -165,6 +170,7 @@ test('MM-T5654_4 should be able to add files when editing a post', async ({pw}) // open the dot menu await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.centerView.postEdit.toBeVisible(); @@ -179,6 +185,7 @@ test('MM-T5654_4 should be able to add files when editing a post', async ({pw}) // now we'll add multiple files await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); await channelsPage.centerView.postEdit.toBeVisible(); @@ -213,7 +220,7 @@ test('MM-5654_5 should be able to remove attachments while editing a post', asyn await post.hover(); await post.postMenu.toBeVisible(); await post.postMenu.clickOnDotMenu(); - await moveMouseToCenter(page); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); @@ -233,7 +240,7 @@ test('MM-T5655_1 removing message content and files should delete the post', asy const originalMessage = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit'; const {user} = await pw.initSetup(); - const {channelsPage} = await pw.testBrowser.login(user); + const {channelsPage, page} = await pw.testBrowser.login(user); await channelsPage.goto(); await channelsPage.toBeVisible(); @@ -247,6 +254,7 @@ test('MM-T5655_1 removing message content and files should delete the post', asy await post.hover(); await post.postMenu.toBeVisible(); await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); @@ -283,7 +291,7 @@ test('MM-T5655_2 should be able to remove all files when editing a post', async await post.hover(); await post.postMenu.toBeVisible(); await post.postMenu.clickOnDotMenu(); - await moveMouseToCenter(page); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); @@ -306,7 +314,7 @@ test('MM-T5656_1 should be able to restore previously edited post version that c const newMessage = 'New Message'; const {user} = await pw.initSetup(); - const {channelsPage} = await pw.testBrowser.login(user); + const {channelsPage, page} = await pw.testBrowser.login(user); await channelsPage.goto(); await channelsPage.toBeVisible(); @@ -320,6 +328,7 @@ test('MM-T5656_1 should be able to restore previously edited post version that c await post.hover(); await post.postMenu.toBeVisible(); await post.postMenu.dotMenuButton.click(); + await moveMouseAway(page); await channelsPage.postDotMenu.toBeVisible(); await channelsPage.postDotMenu.editMenuItem.click(); @@ -350,6 +359,11 @@ test('MM-T5656_1 should be able to restore previously edited post version that c await restoredPost.toContainText('sample_text_file.txt'); }); -async function moveMouseToCenter(page: Page) { +/** + * Parks the cursor away from the post so it does not hover whatever menu item happens to render + * under it. A hovered submenu item (e.g. "Remind") opens a nested MUI popover, which marks the + * parent menu aria-hidden and makes getByRole('menu') unresolvable. + */ +async function moveMouseAway(page: Page) { await page.mouse.move(0, 0); } diff --git a/server/channels/api4/post_test.go b/server/channels/api4/post_test.go index 385fb1dfdd70..abe98131c7d8 100644 --- a/server/channels/api4/post_test.go +++ b/server/channels/api4/post_test.go @@ -996,6 +996,10 @@ func TestCreatePostWithOutgoingHook_no_content_type(t *testing.T) { } func TestMoveThread(t *testing.T) { + // Skipped: MoveThreadsEnabled is retired and rejected by Config.IsValid (MM-69646). + // This test requires the flag and cannot run while the server refuses to enable it. + t.Skip("MoveThreadsEnabled feature flag is retired (MM-69646)") + th := SetupEnterprise(t).InitBasic(t) // Enable MoveThreads feature flag diff --git a/server/i18n/en.json b/server/i18n/en.json index a0b96045a63a..811692751b0c 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -11962,6 +11962,10 @@ "id": "model.config.is_valid.extract_content_timeout.app_error", "translation": "Invalid content extraction timeout for file settings. Must be a whole number of seconds greater than or equal to zero." }, + { + "id": "model.config.is_valid.feature_flags.move_threads_enabled.app_error", + "translation": "The MoveThreadsEnabled feature flag is no longer supported and must be disabled." + }, { "id": "model.config.is_valid.file_driver.app_error", "translation": "Invalid driver name for file settings. Must be 'local', 'amazons3', or 'azureblob'." diff --git a/server/public/model/config.go b/server/public/model/config.go index 0a001809fc66..629883579465 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -4519,6 +4519,12 @@ func (o *Config) IsValid() *AppError { return appErr } + if o.FeatureFlags != nil { + if appErr := o.FeatureFlags.isValid(); appErr != nil { + return appErr + } + } + return nil } diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index 263eb99957a1..20c47ef673ee 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -116,6 +116,39 @@ func TestConfigIsValid(t *testing.T) { }) } +func TestFeatureFlagsIsValid(t *testing.T) { + t.Run("defaults are valid", func(t *testing.T) { + f := &FeatureFlags{} + f.SetDefaults() + require.Nil(t, f.isValid()) + }) + + t.Run("MoveThreadsEnabled is rejected", func(t *testing.T) { + f := &FeatureFlags{} + f.SetDefaults() + f.MoveThreadsEnabled = true + + appErr := f.isValid() + require.NotNil(t, appErr) + require.Equal(t, "model.config.is_valid.feature_flags.move_threads_enabled.app_error", appErr.Id) + }) +} + +func TestConfigIsValidMoveThreadsEnabled(t *testing.T) { + c := Config{} + c.SetDefaults() + require.Nil(t, c.IsValid()) + + c.FeatureFlags.MoveThreadsEnabled = true + appErr := c.IsValid() + require.NotNil(t, appErr) + require.Equal(t, "model.config.is_valid.feature_flags.move_threads_enabled.app_error", appErr.Id) + + // A nil FeatureFlags must not panic the validation chain. + c.FeatureFlags = nil + require.Nil(t, c.IsValid()) +} + func TestAccessControlSettingsIsValid(t *testing.T) { for name, test := range map[string]struct { AccessControlSettings AccessControlSettings diff --git a/server/public/model/feature_flags.go b/server/public/model/feature_flags.go index 14894f3630c7..94e849ef6759 100644 --- a/server/public/model/feature_flags.go +++ b/server/public/model/feature_flags.go @@ -4,6 +4,7 @@ package model import ( + "net/http" "reflect" "strconv" ) @@ -219,6 +220,17 @@ func (f *FeatureFlags) SetDefaults() { f.RecurringScheduledPosts = false } +// isValid rejects feature flag combinations that are no longer supported. +func (f *FeatureFlags) isValid() *AppError { + // MoveThreadsEnabled is being retired in favor of Wrangler, so the server + // refuses to start while it is enabled. + if f.MoveThreadsEnabled { + return NewAppError("FeatureFlags.IsValid", "model.config.is_valid.feature_flags.move_threads_enabled.app_error", nil, "", http.StatusBadRequest) + } + + return nil +} + // IsChannelPermissionPoliciesEnabled reports whether channel-scope // policies may carry permission-rule actions (file upload/download) // and whether the Channel Settings → Permissions Policy tab should From 44d12bef8037de88f50150a5795e0b183924e1d6 Mon Sep 17 00:00:00 2001 From: "cursor[bot]" <206951365+cursor[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:46:56 +0000 Subject: [PATCH 2/5] [MM-66243] Omit sanitized last_viewed_at/last_update_at instead of returning -1 for other users (#37505) * Omit sanitized channel member timestamps from JSON The channel member sanitization introduced in #33835 replaced other users' LastViewedAt and LastUpdateAt with -1, which clients decode as Dec 31 1969. Serialize the sanitized sentinel as an absent field instead so the API no longer returns an invalid timestamp for other users. Co-authored-by: mattermost-code * Add tests and API docs for omitted sanitized member timestamps Verify at the JSON layer that last_viewed_at and last_update_at are omitted for other users' memberships (across the channel and user endpoints) while remaining present for the requester, including a legitimate zero timestamp. Document the omission in the API spec. Co-authored-by: mattermost-code * Strengthen sanitized-timestamp test coverage Cover the NDJSON streaming branch of getChannelMembersForUser and the getChannelMembersForTeamForUser endpoint, assert the requester's own timestamps are valid (not the sentinel), use the sanitizedTimestamp constant, and note the ChannelMemberForExport marshaling footgun. Co-authored-by: mattermost-code * Marshal team data via a typed struct in ChannelMemberWithTeamData Co-authored-by: mattermost-code * Avoid shadowing err in ChannelMemberWithTeamData.MarshalJSON Co-authored-by: mattermost-code * Use omitzero tags to omit sanitized member timestamps Replace the custom ChannelMember/ChannelMemberWithTeamData MarshalJSON round-trip with the Go 1.24 omitzero tag on LastViewedAt/LastUpdateAt. SanitizeForCurrentUser now zeroes another user's timestamps so they are omitted from API responses, per reviewer feedback. * Give current user a real last_viewed_at in sanitization test With omitzero, a zero last_viewed_at is legitimately omitted. Have user2 post an unread message and the current user view the channel so the current-user assertions verify a genuine timestamp survives sanitization. * Use -1 sentinel for sanitized member timestamps with single-pass marshal A last_viewed_at of 0 legitimately means "never viewed", so it cannot double as the sanitization sentinel. Restore the -1 sentinel and omit it during serialization via shadowing pointer fields, avoiding the previous marshal/unmarshal/marshal round-trip. * Clarify ChannelMember.MarshalJSON doc comment per review feedback * Address PR feedback: 0 answered, 4 resolved, 0 declined - Simplify sanitizedTimestamp and SanitizeForCurrentUser doc comments per review - Document that new ChannelMemberWithTeamData fields must be added to MarshalJSON - Add round-trip test guarding against fields dropped by MarshalJSON * Address PR feedback: remove round-trip MarshalJSON test The round-trip test did not guard against forgetting to add a new field to MarshalJSON, since the same field would also be missing from the test. * Address PR feedback: assert legitimate zero last_update_at is serialized * Mark sanitized channel member timestamp fields as nullable in OpenAPI spec --------- Co-authored-by: Cursor Agent Co-authored-by: mattermost-code Co-authored-by: Mattermost Build --- api/v4/source/definitions.yaml | 12 +- server/channels/api4/channel_test.go | 180 +++++++++++++++------ server/public/model/channel_member.go | 72 ++++++++- server/public/model/channel_member_test.go | 130 ++++++++++++++- 4 files changed, 337 insertions(+), 57 deletions(-) diff --git a/api/v4/source/definitions.yaml b/api/v4/source/definitions.yaml index b5d512632e09..a4ff44c48304 100644 --- a/api/v4/source/definitions.yaml +++ b/api/v4/source/definitions.yaml @@ -243,9 +243,13 @@ components: roles: type: string last_viewed_at: - description: The time in milliseconds the channel was last viewed by the user + description: >- + The time in milliseconds the channel was last viewed by the user. + This field is omitted when the membership belongs to a user other + than the requester, as the value is private to that user. type: integer format: int64 + nullable: true msg_count: type: integer mention_count: @@ -253,9 +257,13 @@ components: notify_props: $ref: "#/components/schemas/ChannelNotifyProps" last_update_at: - description: The time in milliseconds the channel member was last updated + description: >- + The time in milliseconds the channel member was last updated. This + field is omitted when the membership belongs to a user other than + the requester, as the value is private to that user. type: integer format: int64 + nullable: true ChannelMemberWithTeamData: allOf: - $ref: "#/components/schemas/ChannelMember" diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index f88f1380102a..2be8d2ff74b1 100644 --- a/server/channels/api4/channel_test.go +++ b/server/channels/api4/channel_test.go @@ -8,6 +8,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "strings" "testing" @@ -7505,73 +7506,162 @@ func TestChannelMemberSanitization(t *testing.T) { _, _, err := client.AddChannelMember(context.Background(), channel.Id, user2.Id) require.NoError(t, err) - t.Run("getChannelMembers sanitizes LastViewedAt and LastUpdateAt for other users", func(t *testing.T) { - members, _, err := client.GetChannelMembers(context.Background(), channel.Id, 0, 60, "") + // Give the current user a real, non-zero last_viewed_at: user2 posts a + // message so the channel is unread for the current user, who then views it. + // This keeps the current-user assertions realistic; the requester's own + // timestamps are never sanitized, so a genuine 0 would still be serialized. + user2Client := th.CreateClient() + _, _, err = user2Client.Login(context.Background(), user2.Email, user2.Password) + require.NoError(t, err) + _, _, err = user2Client.CreatePost(context.Background(), &model.Post{ChannelId: channel.Id, Message: "unread message"}) + require.NoError(t, err) + + _, _, err = client.ViewChannel(context.Background(), user.Id, &model.ChannelView{ChannelId: channel.Id}) + require.NoError(t, err) + + // decodeRawMembers reads the raw JSON body of a channel member response so the + // test can assert whether the timestamp fields are present or omitted, which a + // typed model.ChannelMember cannot distinguish from a zero value. + decodeRawMembers := func(resp *http.Response, err error) []map[string]json.RawMessage { require.NoError(t, err) + defer resp.Body.Close() + + var raw json.RawMessage + require.NoError(t, json.NewDecoder(resp.Body).Decode(&raw)) + + var members []map[string]json.RawMessage + if decodeErr := json.Unmarshal(raw, &members); decodeErr != nil { + var single map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &single)) + members = []map[string]json.RawMessage{single} + } + return members + } + + // decodeNDJSONMembers reads a newline-delimited JSON stream, as returned by + // getChannelMembersForUser when page=-1. + decodeNDJSONMembers := func(resp *http.Response, err error) []map[string]json.RawMessage { + require.NoError(t, err) + defer resp.Body.Close() + + var members []map[string]json.RawMessage + decoder := json.NewDecoder(resp.Body) + for { + var member map[string]json.RawMessage + decodeErr := decoder.Decode(&member) + if decodeErr == io.EOF { + break + } + require.NoError(t, decodeErr) + members = append(members, member) + } + return members + } + + userIDOf := func(t *testing.T, member map[string]json.RawMessage) string { + t.Helper() + var id string + require.NoError(t, json.Unmarshal(member["user_id"], &id)) + return id + } + // assertTimestamps verifies that the current user's memberships expose valid + // timestamps while other users' timestamps are omitted entirely. + assertTimestamps := func(t *testing.T, members []map[string]json.RawMessage) { + t.Helper() for _, member := range members { - if member.UserId == user.Id { - // Current user should see their own timestamps - assert.NotEqual(t, int64(-1), member.LastViewedAt, "Current user should see their LastViewedAt") - assert.NotEqual(t, int64(-1), member.LastUpdateAt, "Current user should see their LastUpdateAt") + rawLastViewedAt, hasLastViewedAt := member["last_viewed_at"] + rawLastUpdateAt, hasLastUpdateAt := member["last_update_at"] + + if userIDOf(t, member) == user.Id { + require.True(t, hasLastViewedAt, "Current user should see their last_viewed_at") + require.True(t, hasLastUpdateAt, "Current user should see their last_update_at") + + var lastViewedAt, lastUpdateAt int64 + require.NoError(t, json.Unmarshal(rawLastViewedAt, &lastViewedAt)) + require.NoError(t, json.Unmarshal(rawLastUpdateAt, &lastUpdateAt)) + assert.GreaterOrEqual(t, lastViewedAt, int64(0), "Current user's last_viewed_at should be a valid timestamp, not the sentinel") + assert.GreaterOrEqual(t, lastUpdateAt, int64(0), "Current user's last_update_at should be a valid timestamp, not the sentinel") } else { - // Other users' timestamps should be sanitized - assert.Equal(t, int64(-1), member.LastViewedAt, "Other users' LastViewedAt should be sanitized") - assert.Equal(t, int64(-1), member.LastUpdateAt, "Other users' LastUpdateAt should be sanitized") + assert.False(t, hasLastViewedAt, "Other users' last_viewed_at should be omitted, not returned as an invalid value") + assert.False(t, hasLastUpdateAt, "Other users' last_update_at should be omitted, not returned as an invalid value") } } - }) - - t.Run("getChannelMember sanitizes LastViewedAt and LastUpdateAt for other users", func(t *testing.T) { - // Get other user's membership data - member, _, err := client.GetChannelMember(context.Background(), channel.Id, user2.Id, "") - require.NoError(t, err) + } - // Should be sanitized since it's not the current user - assert.Equal(t, int64(-1), member.LastViewedAt, "Other user's LastViewedAt should be sanitized") - assert.Equal(t, int64(-1), member.LastUpdateAt, "Other user's LastUpdateAt should be sanitized") + t.Run("getChannelMembers omits last_viewed_at and last_update_at for other users", func(t *testing.T) { + members := decodeRawMembers(client.DoAPIGet(context.Background(), "/channels/"+channel.Id+"/members?page=0&per_page=60", "")) + require.Len(t, members, 2) + assertTimestamps(t, members) + }) - // Get current user's membership data - currentMember, _, err := client.GetChannelMember(context.Background(), channel.Id, user.Id, "") - require.NoError(t, err) + t.Run("getChannelMember omits timestamps for other users but keeps them for the current user", func(t *testing.T) { + otherMembers := decodeRawMembers(client.DoAPIGet(context.Background(), "/channels/"+channel.Id+"/members/"+user2.Id, "")) + require.Len(t, otherMembers, 1) + _, hasLastViewedAt := otherMembers[0]["last_viewed_at"] + _, hasLastUpdateAt := otherMembers[0]["last_update_at"] + assert.False(t, hasLastViewedAt, "Other user's last_viewed_at should be omitted") + assert.False(t, hasLastUpdateAt, "Other user's last_update_at should be omitted") - // Should not be sanitized since it's the current user - assert.NotEqual(t, int64(-1), currentMember.LastViewedAt, "Current user should see their LastViewedAt") - assert.NotEqual(t, int64(-1), currentMember.LastUpdateAt, "Current user should see their LastUpdateAt") + currentMembers := decodeRawMembers(client.DoAPIGet(context.Background(), "/channels/"+channel.Id+"/members/"+user.Id, "")) + require.Len(t, currentMembers, 1) + assertTimestamps(t, currentMembers) }) - t.Run("getChannelMembersByIds sanitizes data appropriately", func(t *testing.T) { - userIds := []string{user.Id, user2.Id} - members, _, err := client.GetChannelMembersByIds(context.Background(), channel.Id, userIds) - require.NoError(t, err) + t.Run("getChannelMembersByIds omits timestamps for other users", func(t *testing.T) { + members := decodeRawMembers(client.DoAPIPostJSON(context.Background(), "/channels/"+channel.Id+"/members/ids", []string{user.Id, user2.Id})) require.Len(t, members, 2) + assertTimestamps(t, members) + }) + assertOtherUserMembersOmitted := func(t *testing.T, members []map[string]json.RawMessage, expectTeamData bool) { + t.Helper() + require.NotEmpty(t, members) for _, member := range members { - if member.UserId == user.Id { - // Current user should see their own timestamps - assert.NotEqual(t, int64(-1), member.LastViewedAt, "Current user should see their LastViewedAt") - assert.NotEqual(t, int64(-1), member.LastUpdateAt, "Current user should see their LastUpdateAt") - } else { - // Other users' timestamps should be sanitized - assert.Equal(t, int64(-1), member.LastViewedAt, "Other users' LastViewedAt should be sanitized") - assert.Equal(t, int64(-1), member.LastUpdateAt, "Other users' LastUpdateAt should be sanitized") + assert.Equal(t, user2.Id, userIDOf(t, member)) + assert.NotContains(t, member, "last_viewed_at", "Other user's last_viewed_at should be omitted") + assert.NotContains(t, member, "last_update_at", "Other user's last_update_at should be omitted") + if expectTeamData { + assert.Contains(t, member, "team_name", "Team data should still be present") } } + } + + t.Run("getChannelMembersForUser (paginated) omits timestamps for other users", func(t *testing.T) { + // Querying another user's channel members requires the edit_other_users + // permission, so use the system admin client. + members := decodeRawMembers(th.SystemAdminClient.DoAPIGet(context.Background(), "/users/"+user2.Id+"/channel_members?page=0", "")) + assertOtherUserMembersOmitted(t, members, true) + }) + + t.Run("getChannelMembersForUser (NDJSON stream) omits timestamps for other users", func(t *testing.T) { + // page=-1 switches the endpoint to the newline-delimited streaming path, + // which sanitizes members through a separate code path. + members := decodeNDJSONMembers(th.SystemAdminClient.DoAPIGet(context.Background(), "/users/"+user2.Id+"/channel_members?page=-1", "")) + assertOtherUserMembersOmitted(t, members, true) + }) + + t.Run("getChannelMembersForTeamForUser omits timestamps for other users", func(t *testing.T) { + // Querying another user's memberships requires manage_system, so use the + // system admin client. + members := decodeRawMembers(th.SystemAdminClient.DoAPIGet(context.Background(), "/users/"+user2.Id+"/teams/"+th.BasicTeam.Id+"/channels/members", "")) + assertOtherUserMembersOmitted(t, members, false) }) - t.Run("addChannelMember sanitizes returned member data", func(t *testing.T) { + t.Run("addChannelMember omits timestamps in the returned member data", func(t *testing.T) { newUser := th.CreateUser(t) th.LinkUserToTeam(t, newUser, th.BasicTeam) - // Add new user and check returned member data - returnedMember, _, err := client.AddChannelMember(context.Background(), channel.Id, newUser.Id) - require.NoError(t, err) + members := decodeRawMembers(client.DoAPIPostJSON(context.Background(), "/channels/"+channel.Id+"/members", map[string]string{"user_id": newUser.Id})) + require.Len(t, members, 1) + + assert.NotContains(t, members[0], "last_viewed_at", "Returned member last_viewed_at should be omitted") + assert.NotContains(t, members[0], "last_update_at", "Returned member last_update_at should be omitted") + assert.Equal(t, newUser.Id, userIDOf(t, members[0]), "UserId should be preserved") - // The returned member should be sanitized since it's not the current user - assert.Equal(t, int64(-1), returnedMember.LastViewedAt, "Returned member LastViewedAt should be sanitized") - assert.Equal(t, int64(-1), returnedMember.LastUpdateAt, "Returned member LastUpdateAt should be sanitized") - assert.Equal(t, newUser.Id, returnedMember.UserId, "UserId should be preserved") - assert.Equal(t, channel.Id, returnedMember.ChannelId, "ChannelId should be preserved") + var channelID string + require.NoError(t, json.Unmarshal(members[0]["channel_id"], &channelID)) + assert.Equal(t, channel.Id, channelID, "ChannelId should be preserved") }) } diff --git a/server/public/model/channel_member.go b/server/public/model/channel_member.go index fa3630e5e415..3a5992e83c92 100644 --- a/server/public/model/channel_member.go +++ b/server/public/model/channel_member.go @@ -4,6 +4,7 @@ package model import ( + "encoding/json" "fmt" "net/http" "strings" @@ -90,19 +91,53 @@ func (o *ChannelMember) Auditable() map[string]any { } } -// SanitizeForCurrentUser sanitizes channel member data based on whether -// it's the current user's own membership or another user's membership +// sanitizedTimestamp marks a LastViewedAt/LastUpdateAt field that belongs to +// another user and must be hidden. MarshalJSON omits any field holding this +// sentinel rather than serializing an invalid value +const sanitizedTimestamp int64 = -1 + +// SanitizeForCurrentUser hides another user's private timestamp fields by +// marking them with the sanitized sentinel, which MarshalJSON then omits from +// API responses. The requester's own values are left untouched. func (o *ChannelMember) SanitizeForCurrentUser(currentUserId string) { - // If this is not the current user's own membership, - // sanitize sensitive timestamp fields if o.UserId != currentUserId { - o.LastViewedAt = -1 - o.LastUpdateAt = -1 + o.LastViewedAt = sanitizedTimestamp + o.LastUpdateAt = sanitizedTimestamp + } +} + +// timestampOrNil returns nil for the sanitized sentinel so that the omitempty +// tag drops the field, and a pointer to the real value otherwise (including a +// legitimate 0). +func timestampOrNil(ts int64) *int64 { + if ts == sanitizedTimestamp { + return nil } + return &ts +} + +// MarshalJSON serializes the channel member in a single pass, omitting +// last_viewed_at and/or last_update_at when they hold the sanitized sentinel +// written by SanitizeForCurrentUser. The shadowing pointer fields allow for a +// direct marshal with the sanitized values removed if needed. +func (o ChannelMember) MarshalJSON() ([]byte, error) { + type alias ChannelMember + return json.Marshal(&struct { + *alias + LastViewedAt *int64 `json:"last_viewed_at,omitempty"` + LastUpdateAt *int64 `json:"last_update_at,omitempty"` + }{ + alias: (*alias)(&o), + LastViewedAt: timestampOrNil(o.LastViewedAt), + LastUpdateAt: timestampOrNil(o.LastUpdateAt), + }) } // ChannelMemberWithTeamData contains ChannelMember appended with extra team information // as well. +// +// Any new non-embedded field added here must also be added to MarshalJSON below, +// otherwise it will be silently dropped from the JSON output. type ChannelMemberWithTeamData struct { ChannelMember TeamDisplayName string `json:"team_display_name"` @@ -110,10 +145,35 @@ type ChannelMemberWithTeamData struct { TeamUpdateAt int64 `json:"team_update_at"` } +// MarshalJSON flattens the embedded ChannelMember together with the team fields +// in a single pass. It is required because ChannelMember's MarshalJSON would +// otherwise be promoted and drop the team fields entirely. +func (o ChannelMemberWithTeamData) MarshalJSON() ([]byte, error) { + type alias ChannelMember + return json.Marshal(&struct { + *alias + LastViewedAt *int64 `json:"last_viewed_at,omitempty"` + LastUpdateAt *int64 `json:"last_update_at,omitempty"` + TeamDisplayName string `json:"team_display_name"` + TeamName string `json:"team_name"` + TeamUpdateAt int64 `json:"team_update_at"` + }{ + alias: (*alias)(&o.ChannelMember), + LastViewedAt: timestampOrNil(o.LastViewedAt), + LastUpdateAt: timestampOrNil(o.LastUpdateAt), + TeamDisplayName: o.TeamDisplayName, + TeamName: o.TeamName, + TeamUpdateAt: o.TeamUpdateAt, + }) +} + type ChannelMembers []ChannelMember type ChannelMembersWithTeamData []ChannelMemberWithTeamData +// ChannelMemberForExport is only converted field-by-field for export and is +// never JSON-marshaled. If that changes, it must define its own MarshalJSON; +// otherwise ChannelMember's promoted MarshalJSON drops ChannelName and Username. type ChannelMemberForExport struct { ChannelMember ChannelName string diff --git a/server/public/model/channel_member_test.go b/server/public/model/channel_member_test.go index 26ad81228932..3fcb4013bf96 100644 --- a/server/public/model/channel_member_test.go +++ b/server/public/model/channel_member_test.go @@ -4,6 +4,7 @@ package model import ( + "encoding/json" "strings" "testing" @@ -93,8 +94,8 @@ func TestChannelMemberSanitizeForCurrentUser(t *testing.T) { member.SanitizeForCurrentUser(currentUserId) - assert.Equal(t, int64(-1), member.LastViewedAt, "LastViewedAt should be sanitized for other users") - assert.Equal(t, int64(-1), member.LastUpdateAt, "LastUpdateAt should be sanitized for other users") + assert.Equal(t, sanitizedTimestamp, member.LastViewedAt, "LastViewedAt should be marked sanitized for other users") + assert.Equal(t, sanitizedTimestamp, member.LastUpdateAt, "LastUpdateAt should be marked sanitized for other users") }) t.Run("should preserve other fields when sanitizing", func(t *testing.T) { @@ -120,8 +121,8 @@ func TestChannelMemberSanitizeForCurrentUser(t *testing.T) { member.SanitizeForCurrentUser(currentUserId) - assert.Equal(t, int64(-1), member.LastViewedAt, "LastViewedAt should be sanitized") - assert.Equal(t, int64(-1), member.LastUpdateAt, "LastUpdateAt should be sanitized") + assert.Equal(t, sanitizedTimestamp, member.LastViewedAt, "LastViewedAt should be marked sanitized") + assert.Equal(t, sanitizedTimestamp, member.LastUpdateAt, "LastUpdateAt should be marked sanitized") assert.Equal(t, originalRoles, member.Roles, "Roles should be preserved") assert.Equal(t, originalMsgCount, member.MsgCount, "MsgCount should be preserved") assert.Equal(t, originalMentionCount, member.MentionCount, "MentionCount should be preserved") @@ -129,3 +130,124 @@ func TestChannelMemberSanitizeForCurrentUser(t *testing.T) { assert.Equal(t, originalSchemeAdmin, member.SchemeAdmin, "SchemeAdmin should be preserved") }) } + +func TestChannelMemberMarshalJSON(t *testing.T) { + currentUserId := NewId() + otherUserId := NewId() + + newMember := func(userId string) ChannelMember { + return ChannelMember{ + ChannelId: NewId(), + UserId: userId, + Roles: "channel_user", + LastViewedAt: 1234567890000, + MsgCount: 100, + LastUpdateAt: 1234567890000, + NotifyProps: GetDefaultChannelNotifyProps(), + } + } + + decode := func(t *testing.T, member ChannelMember) map[string]any { + t.Helper() + data, err := json.Marshal(member) + require.NoError(t, err) + + fields := map[string]any{} + require.NoError(t, json.Unmarshal(data, &fields)) + return fields + } + + t.Run("keeps timestamps for the current user's own membership", func(t *testing.T) { + member := newMember(currentUserId) + member.SanitizeForCurrentUser(currentUserId) + + fields := decode(t, member) + assert.EqualValues(t, 1234567890000, fields["last_viewed_at"]) + assert.EqualValues(t, 1234567890000, fields["last_update_at"]) + }) + + t.Run("keeps a legitimate zero timestamp for the requester", func(t *testing.T) { + member := newMember(currentUserId) + member.LastViewedAt = 0 + member.LastUpdateAt = 0 + member.SanitizeForCurrentUser(currentUserId) + + fields := decode(t, member) + assert.Contains(t, fields, "last_viewed_at", "the requester's own last_viewed_at of 0 (never viewed) must be serialized") + assert.EqualValues(t, 0, fields["last_viewed_at"]) + assert.Contains(t, fields, "last_update_at", "the requester's own last_update_at of 0 must be serialized") + assert.EqualValues(t, 0, fields["last_update_at"]) + }) + + t.Run("omits sanitized timestamps for another user's membership", func(t *testing.T) { + member := newMember(otherUserId) + member.SanitizeForCurrentUser(currentUserId) + + fields := decode(t, member) + assert.NotContains(t, fields, "last_viewed_at", "sanitized last_viewed_at must be omitted") + assert.NotContains(t, fields, "last_update_at", "sanitized last_update_at must be omitted") + + assert.Equal(t, member.ChannelId, fields["channel_id"]) + assert.Equal(t, otherUserId, fields["user_id"]) + assert.Equal(t, "channel_user", fields["roles"]) + assert.EqualValues(t, 100, fields["msg_count"]) + assert.Contains(t, fields, "notify_props") + }) +} + +func TestChannelMemberWithTeamDataMarshalJSON(t *testing.T) { + currentUserId := NewId() + otherUserId := NewId() + + newMember := func(userId string) ChannelMemberWithTeamData { + return ChannelMemberWithTeamData{ + ChannelMember: ChannelMember{ + ChannelId: NewId(), + UserId: userId, + Roles: "channel_user", + LastViewedAt: 1234567890000, + LastUpdateAt: 1234567890000, + NotifyProps: GetDefaultChannelNotifyProps(), + }, + TeamDisplayName: "Test Team", + TeamName: "test-team", + TeamUpdateAt: 987654321, + } + } + + decode := func(t *testing.T, member ChannelMemberWithTeamData) map[string]any { + t.Helper() + data, err := json.Marshal(member) + require.NoError(t, err) + + fields := map[string]any{} + require.NoError(t, json.Unmarshal(data, &fields)) + return fields + } + + t.Run("preserves team data and timestamps for the current user", func(t *testing.T) { + member := newMember(currentUserId) + member.SanitizeForCurrentUser(currentUserId) + + fields := decode(t, member) + assert.EqualValues(t, 1234567890000, fields["last_viewed_at"]) + assert.EqualValues(t, 1234567890000, fields["last_update_at"]) + assert.Equal(t, "Test Team", fields["team_display_name"]) + assert.Equal(t, "test-team", fields["team_name"]) + assert.EqualValues(t, 987654321, fields["team_update_at"]) + }) + + t.Run("omits sanitized timestamps but keeps team data for another user", func(t *testing.T) { + member := newMember(otherUserId) + member.SanitizeForCurrentUser(currentUserId) + + fields := decode(t, member) + assert.NotContains(t, fields, "last_viewed_at", "sanitized last_viewed_at must be omitted") + assert.NotContains(t, fields, "last_update_at", "sanitized last_update_at must be omitted") + + assert.Equal(t, "Test Team", fields["team_display_name"]) + assert.Equal(t, "test-team", fields["team_name"]) + assert.EqualValues(t, 987654321, fields["team_update_at"]) + assert.Equal(t, otherUserId, fields["user_id"]) + }) +} From 5bd5b3b899f7ce2eb5d981015db2a37077746a8c Mon Sep 17 00:00:00 2001 From: Julien Tant <785518+JulienTant@users.noreply.github.com> Date: Tue, 18 Aug 2026 09:58:38 -0700 Subject: [PATCH 3/5] [MM-69865] Add Delete row action to Manage Attributes (#37875) * [MM-69865] Enable the Delete row action on Manage Attributes The kebab menu's Delete was stubbed as disabled + "Coming soon". Wire it to a confirmation modal and the existing DELETE property-field endpoint, dispatching PROPERTY_FIELD_DELETED so the row leaves via Redux rather than local component state. The modal closes immediately on confirm instead of freezing behind a spinner, so failures surface in an AlertBanner above the table rather than inside a modal that is already gone. A 409 (the server refuses while live linked dependents exist) gets its own message instead of the generic one -- the wording says "other attributes are still linked to it", matching what CountLinkedFields actually counts, not the AD/LDAP and SAML source links, which are plain attrs on the field itself. Delete stays disabled on plugin-owned rows, relabelled "Plugin-managed" so the reason is visible rather than the item silently doing nothing. E2E covers confirm, cancel, and the 409 branch against a real server response: the linked dependent is seeded through the API, which needs a non-template object type because IsValid rejects a template field carrying a linked_field_id. Co-Authored-By: Claude Opus 5 (1M context) * Resolve server-only plugin names instead of showing the raw plugin ID getPluginDisplayName only read state.plugins.plugins, which holds manifests for plugins that shipped a webapp bundle and registered themselves in the browser. A server-only plugin is never in that map, so every caller fell through to the fallback and rendered the bare ID -- "com.mattermost.gahelper" in the Manage Attributes Source column. Consult state.entities.admin.pluginStatuses as a second source before giving up on the ID. That map covers every installed plugin, server-only included, and is empty for non-admins, so the non-admin callers (user_settings_general, integrations/bots) are unaffected. Nothing on the Manage Attributes page loaded those statuses, so fetch them there -- once, and only when a plugin-owned row is actually present. The result is deliberately not awaited: a failure just leaves the column showing the ID it was already showing. Also fixes the same latent fallback on custom_profile_attributes, user_properties_values, and system_user_detail. Co-Authored-By: Claude Opus 5 (1M context) * Keep e2e attribute names under the 40-char Unique name cap Every test in the "create attribute" block built expectedName from a long prefix plus a 13-digit Date.now(), overshooting the Unique name input's Constants.MAX_CUSTOM_ATTRIBUTE_NAME_LENGTH (40). The derived slug truncated silently, so "playwright_created_attribute_<13 digits>" (42 chars) was asserted against the 40 chars actually rendered. The two linked-source tests only use expectedName for cleanup, so instead of failing they quietly leaked their seeded field onto the shared server on every run. Shorten the prefixes so the derived slug fits. The whole file is mode: 'serial', so these failures also skipped every test after them -- including the delete coverage at the end of the file. Co-Authored-By: Claude Opus 5 (1M context) * Announce the delete error banner to assistive tech The banner was mounted together with its message, and an alert inserted into the DOM at the same moment as its text is not reliably announced -- so a screen-reader user got silence when a delete they had just confirmed failed. Keep the region mounted and swap only its content, matching the reason attribute_external_source.tsx already keeps its own status region mounted rather than rendering it alongside the announcement. Co-Authored-By: Claude Opus 5 (1M context) * [MM-69865] Allow deleting a plugin-owned attribute once its plugin is uninstalled Delete was permanently disabled on plugin-owned rows, but the server only protects such a field while its source plugin is still installed: checkFieldDeleteAccess allows the delete once the plugin is gone, which is how an admin cleans up what an uninstalled plugin left behind. Reuse the User Attributes page's orphan check rather than growing a second one. isFieldOrphaned moves out of system_properties/orphaned_fields_utils.ts into utils/properties.ts, widened to PropertyField so both pages can use it, with the redux-aware hook in components/common/hooks/use_field_orphaned.ts. The hook unions admin.plugins and admin.pluginStatuses because the two attribute pages populate different slices, and a field should not read as orphaned merely because the page rendering it loaded only one of them. The delete confirmation now names the uninstalled plugin, since an admin has no other way to tell where the leftover attribute came from. Co-Authored-By: Claude Opus 5 (1M context) * Fix the delete-error scroll and the premature orphan check The error banner sits above the table, so a delete confirmed from a row further down left the failure off-screen. The existing scroll fired on the error state, which lands during the modal's fade-out: GenericModal passes restoreFocus, so react-bootstrap returns focus to the row's actions button on close, and focusing an off-screen element scrolls it back into view -- undoing the scroll every time. GenericModal also starts closing before it invokes handleConfirm, so the delete response can land either side of the fade and the error and the exit arrive in either order. Scroll only once both have landed, take focus on the banner without its own scroll, then scroll the console wrapper to the top. Focusing the banner also leaves keyboard and screen reader users at the error rather than back on a row button. Separately, gate the orphan check on the plugin inventory having settled. Both admin.plugins and admin.pluginStatuses start empty, and the fetch is dispatched from an effect, so an inventory that has not loaded is indistinguishable from one where nothing is installed -- which isFieldOrphaned reads as "every plugin-owned field is orphaned". A protected row briefly offered Delete behind a dialog wrongly claiming its plugin was uninstalled, which the server would then refuse anyway. Settled rather than resolved: a failed fetch still leaves the inventory as good as it will get, and staying false forever would strand genuine leftovers as undeletable. The same gap exists for the system_properties consumers of the hook, which never fetch the inventory at all; left alone here and noted on the hook. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../global_attributes.spec.ts | 190 ++++++++- .../global_attributes_helpers.ts | 40 ++ .../global_attribute_delete_modal.test.tsx | 68 +++ .../global_attribute_delete_modal.tsx | 106 +++++ .../global_attributes_table.test.tsx | 393 +++++++++++++++++- .../global_attributes_table.tsx | 148 ++++++- .../admin_console/global_attributes/utils.ts | 7 + .../orphaned_fields_utils.ts | 25 -- .../user_properties_table.tsx | 2 +- .../user_properties_values.tsx | 3 +- .../common/hooks/use_field_orphaned.ts | 43 ++ webapp/channels/src/i18n/en.json | 6 + webapp/channels/src/selectors/plugins.ts | 11 +- webapp/channels/src/utils/constants.tsx | 1 + webapp/channels/src/utils/properties.test.ts | 28 ++ webapp/channels/src/utils/properties.ts | 25 ++ 16 files changed, 1047 insertions(+), 49 deletions(-) create mode 100644 webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.test.tsx create mode 100644 webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.tsx delete mode 100644 webapp/channels/src/components/admin_console/system_properties/orphaned_fields_utils.ts create mode 100644 webapp/channels/src/components/common/hooks/use_field_orphaned.ts diff --git a/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes.spec.ts b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes.spec.ts index eed208526465..ee857932a289 100644 --- a/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes.spec.ts +++ b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes.spec.ts @@ -21,7 +21,9 @@ import { import { GLOBAL_ATTRIBUTES_ADMIN_PATH, createGlobalAttributeField, + createLinkedDependentField, deleteGlobalAttributeFieldIfExists, + deleteLinkedDependentField, requireGlobalAttributesEnabled, setGlobalAttributesFeatureFlag, } from './global_attributes_helpers'; @@ -436,9 +438,13 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () // one, so "E2E ..." would actually slugify to "e2_e_..." (verified against // slugifyForCEL directly), not the naively-expected "e2e_...". "Playwright" // has no internal case/digit boundary, so its derived slug is unambiguous. + // The prefix is kept short on purpose: the Unique name input is capped at + // Constants.MAX_CUSTOM_ATTRIBUTE_NAME_LENGTH (40), and a 13-digit Date.now() + // leaves only 27 characters for everything before it. A longer prefix derives + // a silently truncated slug that no longer matches the expectation below. const timestamp = Date.now(); - const displayName = `Playwright Created Attribute ${timestamp}`; - const expectedName = `playwright_created_attribute_${timestamp}`; + const displayName = `Playwright Attr ${timestamp}`; + const expectedName = `playwright_attr_${timestamp}`; try { // # Log in and open the Manage Attributes page @@ -681,8 +687,9 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); const timestamp = Date.now(); - const displayName = `Playwright Select Attribute ${timestamp}`; - const expectedName = `playwright_select_attribute_${timestamp}`; + // Short prefix: see the 40-char Unique name cap noted in the bare-Text test above + const displayName = `Playwright Select ${timestamp}`; + const expectedName = `playwright_select_${timestamp}`; try { const {systemConsolePage} = await pw.testBrowser.login(adminUser); @@ -736,8 +743,9 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); const timestamp = Date.now(); - const displayName = `Playwright Ranked Attribute ${timestamp}`; - const expectedName = `playwright_ranked_attribute_${timestamp}`; + // Short prefix: see the 40-char Unique name cap noted in the bare-Text test above + const displayName = `Playwright Ranked ${timestamp}`; + const expectedName = `playwright_ranked_${timestamp}`; try { const {systemConsolePage} = await pw.testBrowser.login(adminUser); @@ -822,8 +830,11 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); const timestamp = Date.now(); - const displayName = `Playwright LDAP Linked Attribute ${timestamp}`; - const expectedName = `playwright_ldap_linked_attribute_${timestamp}`; + // Short prefix: see the 40-char Unique name cap noted in the bare-Text test above. + // This one only uses expectedName for cleanup, so an over-long prefix leaked the + // created field onto the shared server instead of failing loudly. + const displayName = `Playwright Ldap ${timestamp}`; + const expectedName = `playwright_ldap_${timestamp}`; try { const {systemConsolePage} = await pw.testBrowser.login(adminUser); @@ -867,8 +878,9 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); const timestamp = Date.now(); - const displayName = `Playwright Dual Linked Attribute ${timestamp}`; - const expectedName = `playwright_dual_linked_attribute_${timestamp}`; + // Short prefix: see the 40-char Unique name cap noted in the bare-Text test above + const displayName = `Playwright Dual ${timestamp}`; + const expectedName = `playwright_dual_${timestamp}`; try { const {systemConsolePage} = await pw.testBrowser.login(adminUser); @@ -1011,4 +1023,162 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () ); }); }); + + test.describe('delete attribute', () => { + /** + * @objective Ensure the row kebab's Delete action removes the attribute end-to-end: + * the confirmation names the attribute, and confirming drops the row from the table. + */ + test('deletes an attribute from the row menu after confirming, and the row disappears', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_delete_${timestamp}`; + const displayName = `E2E Delete Attribute ${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, + }); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + const row = page.locator('tr', { + has: page.getByTestId('global-attribute-name').filter({hasText: displayName}), + }); + await expect(row).toBeVisible(); + + // # Open the row kebab and click Delete + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-delete`).click(); + + // * The confirmation names the specific attribute rather than prompting generically + await expect(page.getByRole('heading', {name: `Delete ${displayName} attribute`})).toBeVisible(); + + // # Confirm + await page.getByRole('button', {name: 'Delete', exact: true}).click(); + + // * The row is gone and no error banner appeared + await expect(row).toHaveCount(0); + await expect(page.getByTestId('global-attributes-delete-error')).toHaveCount(0); + + // * The delete really hit the server, not just the client store — a fresh + // page load still doesn't show it + await page.reload(); + await expect(page.getByTestId('global-attribute-name').filter({hasText: displayName})).toHaveCount(0); + } finally { + await deleteGlobalAttributeFieldIfExists(adminClient, name); + } + }); + + /** + * @objective Ensure cancelling the confirmation is a true no-op — no delete call fires + * and the attribute survives a reload. + */ + test('leaves the attribute in place when the confirmation is cancelled', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_cancel_${timestamp}`; + const displayName = `E2E Cancel Attribute ${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, + }); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + const row = page.locator('tr', { + has: page.getByTestId('global-attribute-name').filter({hasText: displayName}), + }); + await expect(row).toBeVisible(); + + // # Open the row kebab, click Delete, then back out + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-delete`).click(); + await expect(page.getByRole('heading', {name: `Delete ${displayName} attribute`})).toBeVisible(); + await page.getByRole('button', {name: 'Cancel'}).click(); + + // * The modal closed and the row survived + await expect(page.getByRole('heading', {name: `Delete ${displayName} attribute`})).toHaveCount(0); + await expect(row).toBeVisible(); + + // * Nothing was deleted server-side either + await page.reload(); + await expect(page.getByTestId('global-attribute-name').filter({hasText: displayName})).toBeVisible(); + } finally { + await deleteGlobalAttributeFieldIfExists(adminClient, name); + } + }); + + /** + * @objective Ensure a server-side 409 (the attribute still has live linked dependents) + * surfaces as the specific "still linked" banner above the table, not the generic error, + * and leaves the row intact. Exercised against a real 409 from the server rather than a + * stubbed rejection. + */ + test('shows the linked-dependents banner and keeps the row when the server refuses the delete', async ({ + pw, + }) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_linked_${timestamp}`; + const displayName = `E2E Linked Attribute ${timestamp}`; + const dependentName = `e2e_global_attribute_dependent_${timestamp}`; + + let dependentFieldId: string | undefined; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, + }); + + // # Point a dependent field at it, which is what makes the server refuse the delete + const dependent = await createLinkedDependentField(adminClient, dependentName, field.id, 'text'); + dependentFieldId = dependent.id; + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + const row = page.locator('tr', { + has: page.getByTestId('global-attribute-name').filter({hasText: displayName}), + }); + await expect(row).toBeVisible(); + + // # Try to delete it + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-delete`).click(); + await page.getByRole('button', {name: 'Delete', exact: true}).click(); + + // * The banner explains the blocking dependency instead of the generic failure + const banner = page.getByTestId('global-attributes-delete-error'); + await expect(banner).toBeVisible(); + await expect(banner).toContainText('other attributes are still linked to it'); + await expect(banner).not.toContainText('An error occurred while deleting this attribute'); + + // * The row survived the rejected delete + await expect(row).toBeVisible(); + + // # The banner is dismissible + await banner.getByRole('button', {name: 'Close'}).click(); + await expect(banner).toHaveCount(0); + } finally { + // Dependent first: the source delete stays blocked while it exists + if (dependentFieldId) { + await deleteLinkedDependentField(adminClient, dependentFieldId); + } + await deleteGlobalAttributeFieldIfExists(adminClient, name); + } + }); + }); }); diff --git a/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_helpers.ts b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_helpers.ts index 00d17059e46a..168580549a1a 100644 --- a/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_helpers.ts +++ b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_helpers.ts @@ -14,6 +14,12 @@ const PROPERTY_GROUP = 'access_control'; const OBJECT_TYPE = 'template'; const TARGET_TYPE = 'system'; +// Object type used to seed a field that *links to* a template field. It has to be +// anything but 'template': PropertyField.IsValid rejects a template field carrying a +// linked_field_id ("template fields cannot have a linked field"). 'user' matches the +// shape the store's own CountLinkedFields coverage uses. +const LINKED_OBJECT_TYPE = 'user'; + // Server clamps per_page to this max (see web.PerPageMaximum in server/channels/web/params.go). // Directory-mode search with no cursor sorts CreateAt ASC, so the default 60-item page only // returns the oldest fields — request the max to reduce the risk of missing newer ones. @@ -96,3 +102,37 @@ export async function createGlobalAttributeField( ...field, } as Parameters[2]); } + +/** + * Creates a field that links to `sourceFieldId`, which makes the server refuse to delete + * that source field: deletePropertyField counts live linked dependents and returns 409 + * `has_linked_dependents` when any exist (server/channels/app/properties/property_field.go). + * This is the only way to exercise the listing's 409 branch against a real server response. + */ +export async function createLinkedDependentField( + adminClient: Client4, + name: string, + sourceFieldId: string, + type: string, +) { + return adminClient.createPropertyField(PROPERTY_GROUP, LINKED_OBJECT_TYPE, { + name, + type, + target_type: TARGET_TYPE, + target_id: '', + linked_field_id: sourceFieldId, + } as unknown as Parameters[2]); +} + +/** + * Deletes a linked dependent field by id, ignoring failures (it may already be gone). + * Must run BEFORE deleting the field it points at — the source delete stays blocked + * with a 409 for as long as a live dependent exists. + */ +export async function deleteLinkedDependentField(adminClient: Client4, fieldId: string) { + try { + await adminClient.deletePropertyField(PROPERTY_GROUP, LINKED_OBJECT_TYPE, fieldId); + } catch { + // Already deleted, or routes unavailable; ignore. + } +} diff --git a/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.test.tsx b/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.test.tsx new file mode 100644 index 000000000000..b13603752f42 --- /dev/null +++ b/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.test.tsx @@ -0,0 +1,68 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import {renderWithContext, screen, userEvent} from 'tests/react_testing_utils'; + +import GlobalAttributeDeleteModal from './global_attribute_delete_modal'; + +describe('GlobalAttributeDeleteModal', () => { + const renderModal = (overrides: Partial> = {}) => { + const props = { + name: 'Department', + onConfirm: jest.fn(), + onExited: jest.fn(), + ...overrides, + }; + renderWithContext(); + return props; + }; + + it('names the attribute being deleted in the title, rather than a generic prompt', () => { + renderModal({name: 'Department'}); + + expect(screen.getByRole('heading', {name: /delete department attribute/i})).toBeInTheDocument(); + expect(screen.getByText(/permanently remove its definition/i)).toBeInTheDocument(); + }); + + it('invokes onConfirm when the Delete button is clicked', async () => { + const props = renderModal(); + + await userEvent.click(screen.getByRole('button', {name: /^delete$/i})); + + expect(props.onConfirm).toHaveBeenCalledTimes(1); + }); + + it('does not invoke onConfirm when the Cancel button is clicked', async () => { + const props = renderModal(); + + await userEvent.click(screen.getByRole('button', {name: /cancel/i})); + + expect(props.onConfirm).not.toHaveBeenCalled(); + }); + + describe('orphaned attribute', () => { + it('names the uninstalled plugin the attribute was left behind by', () => { + renderModal({isOrphaned: true, sourcePluginId: 'com.acme.plugin'}); + + expect(screen.getByText(/was created by the plugin "com\.acme\.plugin", which is no longer installed/i)).toBeInTheDocument(); + + // * The standard warning is kept alongside it rather than replaced — an + // orphaned attribute is just as permanently deleted as any other + expect(screen.getByText(/permanently remove its definition/i)).toBeInTheDocument(); + }); + + it('falls back to "unknown" when the source plugin id is missing', () => { + renderModal({isOrphaned: true}); + + expect(screen.getByText(/was created by the plugin "unknown"/i)).toBeInTheDocument(); + }); + + it('says nothing about plugins for an ordinary attribute', () => { + renderModal(); + + expect(screen.queryByText(/no longer installed/i)).not.toBeInTheDocument(); + }); + }); +}); diff --git a/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.tsx b/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.tsx new file mode 100644 index 000000000000..edac4752953d --- /dev/null +++ b/webapp/channels/src/components/admin_console/global_attributes/global_attribute_delete_modal.tsx @@ -0,0 +1,106 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {FormattedMessage, useIntl} from 'react-intl'; +import {useDispatch} from 'react-redux'; + +import {GenericModal} from '@mattermost/components'; + +import {openModal} from 'actions/views/modals'; + +import {ModalIdentifiers} from 'utils/constants'; + +type Props = { + name: string; + onConfirm: () => void; + onExited: () => void; + isOrphaned?: boolean; + sourcePluginId?: string; +}; + +// GenericModal only renders a Cancel button when handleCancel is supplied, and +// cancelling needs no side effect here beyond closing — same shape as +// user_properties_delete_modal. +const noop = () => {}; + +/** + * Opens the delete-confirmation modal for a Global Attribute. The modal is + * display-only: `onConfirm` fires the caller's own delete logic, which owns the + * API call and its error handling. Mirrors useUserPropertyFieldDelete / + * useBoardAttributeFieldDelete, but passes the callback in rather than resolving + * a Promise, since the caller's handler is async and reports its own failures. + * + * Pass `orphan` when the field's source plugin is no longer installed, so the + * confirmation can explain where the leftover attribute came from. It is an + * object rather than a bare flag so there is no way to declare a field orphaned + * without supplying the plugin it came from. + * + * `onExited` runs once the modal has finished closing, whether it was confirmed + * or cancelled. ModalController composes it with its own close handling, so the + * caller's callback runs after the dialog is actually gone -- which is when it is + * safe to move focus or scroll, since react-bootstrap restores focus to whatever + * opened the modal on the way out. + */ +export const useGlobalAttributeFieldDelete = () => { + const dispatch = useDispatch(); + + return (name: string, onConfirm: () => void, orphan?: {sourcePluginId?: string}, onExited?: () => void) => { + dispatch(openModal({ + modalId: ModalIdentifiers.GLOBAL_ATTRIBUTE_FIELD_DELETE, + dialogType: GlobalAttributeDeleteModal, + dialogProps: { + name, + onConfirm, + isOrphaned: Boolean(orphan), + sourcePluginId: orphan?.sourcePluginId, + onExited, + }, + })); + }; +}; + +function GlobalAttributeDeleteModal({name, onConfirm, onExited, isOrphaned = false, sourcePluginId}: Props) { + const {formatMessage} = useIntl(); + + const title = formatMessage({ + id: 'admin.global_attributes.confirm.delete.title', + defaultMessage: 'Delete {name} attribute', + }, {name}); + + const confirmButtonText = formatMessage({ + id: 'admin.system_properties.confirm.delete.button', + defaultMessage: 'Delete', + }); + + return ( + + {/* An uninstalled plugin leaves no manifest behind to resolve a display + name from, so the raw source_plugin_id is the only identifier we can + honestly show here. */} + {isOrphaned && ( +

+ +

+ )} + + + ); +} + +export default GlobalAttributeDeleteModal; diff --git a/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.test.tsx b/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.test.tsx index 14938ccd62cc..e1cbe40da12e 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.test.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.test.tsx @@ -1,9 +1,10 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {screen, waitFor} from '@testing-library/react'; +import {act, screen, waitFor, within} from '@testing-library/react'; import React from 'react'; +import {ClientError} from '@mattermost/client'; import {ChevronDownCircleOutlineIcon, FormatListBulletedIcon, MenuVariantIcon, PowerPlugOutlineIcon, SortAscendingIcon, SyncIcon} from '@mattermost/compass-icons/components'; import type {PropertyField} from '@mattermost/types/properties'; import type {DeepPartial} from '@mattermost/types/utilities'; @@ -15,6 +16,7 @@ import { CLASSIFICATIONS_TEMPLATE_FIELD_NAME, CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE, } from 'components/admin_console/classification_markings/utils'; +import ModalController from 'components/modal_controller'; import {renderWithContext, userEvent} from 'tests/react_testing_utils'; import {WindowSizes} from 'utils/constants'; @@ -47,6 +49,14 @@ function makeField(overrides: Partial = {}): PropertyField { } as PropertyField; } +function makeClientError(statusCode: number): ClientError { + return new ClientError('https://example.com', { + message: 'error', + status_code: statusCode, + url: 'https://example.com/api/v4/properties/groups/access_control/template/fields/field-1', + }); +} + function getBaseState(): DeepPartial { return { entities: { @@ -348,6 +358,50 @@ describe('GlobalAttributesTable', () => { expect(cell.querySelector('svg')).toBeInTheDocument(); }); + it('resolves a server-only plugin name from the admin plugin statuses rather than showing the raw plugin ID', async () => { + const getPluginStatuses = jest.spyOn(Client4, 'getPluginStatuses').mockResolvedValue([{ + plugin_id: 'com.mattermost.gahelper', + name: 'Global Attributes Helper', + description: '', + version: '1.0.0', + cluster_id: '', + plugin_path: '', + state: 1, + }]); + + // No entry in state.plugins: a server-only plugin ships no webapp bundle, + // so it never registers a client manifest. + getPropertyFields.mockResolvedValueOnce([makeField({ + attrs: {source_plugin_id: 'com.mattermost.gahelper', protected: true}, + })]).mockResolvedValue([]); + + renderWithContext(, getBaseState()); + + await waitFor(() => { + expect(getPluginStatuses).toHaveBeenCalled(); + }); + + const cell = await screen.findByTestId('global-attribute-source'); + expect(cell).toHaveTextContent('Global Attributes Helper'); + expect(cell).not.toHaveTextContent('com.mattermost.gahelper'); + + getPluginStatuses.mockRestore(); + }); + + it('does not fetch plugin statuses when no row is plugin-owned', async () => { + const getPluginStatuses = jest.spyOn(Client4, 'getPluginStatuses').mockResolvedValue([]); + + getPropertyFields.mockResolvedValueOnce([makeField({attrs: {ldap: 'someAttribute'}})]).mockResolvedValue([]); + + renderWithContext(, getBaseState()); + + await screen.findByTestId('global-attribute-source'); + + expect(getPluginStatuses).not.toHaveBeenCalled(); + + getPluginStatuses.mockRestore(); + }); + it('shows AD/LDAP when attrs.ldap is set', async () => { getPropertyFields.mockResolvedValueOnce([makeField({attrs: {ldap: 'someAttribute'}})]).mockResolvedValue([]); @@ -403,7 +457,7 @@ describe('GlobalAttributesTable', () => { }); describe('Actions column', () => { - it('opens the menu with Edit/Duplicate/Delete rendered visibly disabled, not a silent no-op', async () => { + it('opens the menu with Edit/Duplicate still visibly disabled and Delete enabled', async () => { getPropertyFields.mockResolvedValueOnce([makeField()]).mockResolvedValue([]); renderWithContext(, getBaseState()); @@ -427,12 +481,341 @@ describe('GlobalAttributesTable', () => { expect(edit!).toHaveAttribute('aria-disabled', 'true'); expect(duplicate!).toHaveAttribute('aria-disabled', 'true'); - expect(del!).toHaveAttribute('aria-disabled', 'true'); - // * Each disabled item explains why, rather than silently doing nothing + // * Each still-stubbed item explains why, rather than silently doing nothing expect(edit!).toHaveTextContent('Coming soon'); expect(duplicate!).toHaveTextContent('Coming soon'); - expect(del!).toHaveTextContent('Coming soon'); + + // * Delete is live now, so it carries neither the disabled state nor the stub label + expect(del!).not.toHaveAttribute('aria-disabled', 'true'); + expect(del!).not.toHaveTextContent('Coming soon'); + }); + }); + + describe('Delete action', () => { + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField'); + + beforeEach(() => { + deletePropertyField.mockReset(); + }); + + // The class here is not decoration: the System Console scrolls in + // .admin-console__wrapper, and that is the ancestor the table pulls back to + // the top when a delete fails. jsdom implements no scrolling at all, so the + // method is stubbed to record the call. + function renderTable(fields: PropertyField[], state: DeepPartial = getBaseState()) { + getPropertyFields.mockResolvedValueOnce(fields).mockResolvedValue([]); + + renderWithContext( +
+ + +
, + state, + ); + + const scrollTo = jest.fn(); + Object.assign(document.querySelector('.admin-console__wrapper')!, {scrollTo}); + + return {scrollTo}; + } + + // A plugin-owned row is server-protected only while its plugin is still + // installed, so these tests have to state which plugins the admin console + // believes are installed. Without this the row reads as orphaned. + function getStateWithInstalledPlugin(pluginId: string): DeepPartial { + const state = getBaseState(); + state.entities!.admin = { + pluginStatuses: {[pluginId]: {id: pluginId}}, + } as EntitiesPartial['admin']; + return state; + } + + const PLUGIN_ID = 'com.acme.plugin'; + + function makePluginOwnedField() { + return makeField({attrs: {display_name: 'Department', source_plugin_id: PLUGIN_ID, protected: true}}); + } + + async function openDeleteModal(fieldId = 'field-1') { + await userEvent.click(await screen.findByTestId(`global-attribute-actions-${fieldId}`)); + + const del = screen.getAllByRole('menuitem').find((el) => el.textContent?.includes('Delete attribute')); + await userEvent.click(del!); + } + + it('names the attribute in the confirmation modal instead of deleting straight from the menu', async () => { + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + + expect(await screen.findByRole('heading', {name: /delete department attribute/i})).toBeInTheDocument(); + + // * Opening the modal alone must not have fired the destructive call + expect(deletePropertyField).not.toHaveBeenCalled(); + }); + + it('leaves the row and the API untouched when the modal is cancelled', async () => { + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /cancel/i})); + + expect(deletePropertyField).not.toHaveBeenCalled(); + expect(screen.getByTestId('global-attribute-name')).toHaveTextContent('Department'); + }); + + it('deletes via the access_control/template scope and drops the row on success', async () => { + deletePropertyField.mockResolvedValue({status: 'OK'}); + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + await waitFor(() => { + expect(deletePropertyField).toHaveBeenCalledWith('access_control', 'template', 'field-1'); + }); + + // * The row is gone because the reducer removed the field, not because the + // component hid it locally — the last-attribute empty state proves the store changed + expect(await screen.findByTestId('global-attributes-empty')).toBeInTheDocument(); + }); + + it('surfaces a generic banner above the table and keeps the row when the delete fails', async () => { + deletePropertyField.mockRejectedValue(makeClientError(500)); + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + const banner = await screen.findByTestId('global-attributes-delete-error'); + expect(banner).toHaveTextContent('An error occurred while deleting this attribute. Please try again.'); + + // * The row survives a failed delete + expect(screen.getByTestId('global-attribute-name')).toHaveTextContent('Department'); + }); + + it('explains the blocking dependency rather than showing the generic error on a 409', async () => { + deletePropertyField.mockRejectedValue(makeClientError(409)); + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + const banner = await screen.findByTestId('global-attributes-delete-error'); + expect(banner).toHaveTextContent(/other attributes are still linked to it/i); + expect(banner).not.toHaveTextContent('An error occurred while deleting this attribute'); + }); + + it('dismisses the error banner without re-running the delete', async () => { + deletePropertyField.mockRejectedValue(makeClientError(500)); + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + const banner = await screen.findByTestId('global-attributes-delete-error'); + + // The modal aria-hides the page behind it, so wait for it to tear down before + // reaching for the banner's own dismiss control by role + await waitFor(() => { + expect(screen.queryByText(/permanently remove its definition/i)).not.toBeInTheDocument(); + }); + + await userEvent.click(within(banner).getByRole('button', {name: /close/i})); + + expect(screen.queryByTestId('global-attributes-delete-error')).not.toBeInTheDocument(); + expect(deletePropertyField).toHaveBeenCalledTimes(1); + }); + + it('keeps the error live region mounted so the banner is announced when it appears', async () => { + deletePropertyField.mockRejectedValue(makeClientError(500)); + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + // * The region exists before any error, so the banner arriving is a content + // change inside a live region rather than a newly-inserted region — the + // latter is not reliably announced + const liveRegion = await screen.findByRole('alert'); + expect(liveRegion).toBeEmptyDOMElement(); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + // * The node captured before the error now carries the message. A region + // remounted alongside its content would have left this reference detached + // and empty, so this also proves the region persisted. + await waitFor(() => { + expect(liveRegion).toHaveTextContent('An error occurred while deleting this attribute'); + }); + expect(liveRegion).toBeInTheDocument(); + }); + + it('scrolls the page back to the top and takes focus once a failed delete has closed the modal', async () => { + deletePropertyField.mockRejectedValue(makeClientError(500)); + const {scrollTo} = renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + const liveRegion = await screen.findByRole('alert'); + + await waitFor(() => { + expect(scrollTo).toHaveBeenCalledWith({top: 0}); + }); + + // * Focus lands on the banner rather than being restored to the row's + // actions button, which is what would otherwise scroll the page away + // from the error again + expect(liveRegion).toHaveFocus(); + }); + + it('still scrolls to the error when the delete outlasts the modal close animation', async () => { + // GenericModal starts closing before it calls handleConfirm, so a slow + // request can land after the modal is already gone — the reverse of the + // usual order, and the case a plain onExited hook would miss + let failDelete: (error: unknown) => void = () => {}; + deletePropertyField.mockImplementation(() => new Promise((_resolve, reject) => { + failDelete = reject; + })); + + const {scrollTo} = renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + await waitFor(() => { + expect(screen.queryByText(/permanently remove its definition/i)).not.toBeInTheDocument(); + }); + expect(scrollTo).not.toHaveBeenCalled(); + + await act(async () => { + failDelete(makeClientError(500)); + }); + + await waitFor(() => { + expect(scrollTo).toHaveBeenCalledWith({top: 0}); + }); + }); + + it('leaves the scroll position alone when the delete succeeds', async () => { + deletePropertyField.mockResolvedValue({status: 'OK'}); + const {scrollTo} = renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + expect(await screen.findByTestId('global-attributes-empty')).toBeInTheDocument(); + expect(scrollTo).not.toHaveBeenCalled(); + }); + + it('leaves the scroll position alone when the modal is cancelled', async () => { + const {scrollTo} = renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /cancel/i})); + + await waitFor(() => { + expect(screen.queryByText(/permanently remove its definition/i)).not.toBeInTheDocument(); + }); + expect(scrollTo).not.toHaveBeenCalled(); + }); + + it('keeps scrolling to the error on a second failed delete', async () => { + deletePropertyField.mockRejectedValue(makeClientError(500)); + const {scrollTo} = renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + await waitFor(() => { + expect(scrollTo).toHaveBeenCalledTimes(1); + }); + + await openDeleteModal(); + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + // * The behaviour is per-attempt rather than one-shot. Note this does not + // pin down *when* the second scroll fires: jsdom completes the modal's + // fade before the rejection lands, so the ordering the component re-arms + // for is not reproducible here. + await waitFor(() => { + expect(scrollTo).toHaveBeenCalledTimes(2); + }); + }); + + it('keeps Delete disabled with a reason on a plugin-owned row while the plugin is installed', async () => { + renderTable([makePluginOwnedField()], getStateWithInstalledPlugin(PLUGIN_ID)); + + await userEvent.click(await screen.findByTestId('global-attribute-actions-field-1')); + + const del = screen.getAllByRole('menuitem').find((el) => el.textContent?.includes('Delete attribute')); + expect(del!).toHaveAttribute('aria-disabled', 'true'); + expect(del!).toHaveTextContent('Plugin-managed'); + + // pointerEventsCheck: 0 forces the click past the disabled item's + // `pointer-events: none`, proving no handler is wired underneath the styling + await userEvent.click(del!, {pointerEventsCheck: 0}); + + // * No modal, no API call — the disabled item is inert, not just styled as disabled + expect(screen.queryByRole('heading', {name: /delete department attribute/i})).not.toBeInTheDocument(); + expect(deletePropertyField).not.toHaveBeenCalled(); + }); + + it('re-enables Delete on a plugin-owned row once the plugin is uninstalled, so the leftover can be cleaned up', async () => { + deletePropertyField.mockResolvedValue({status: 'OK'}); + + // No plugin statuses at all: the source plugin is gone, which is what the + // server itself keys the delete allowance off (checkFieldDeleteAccess) + renderTable([makePluginOwnedField()]); + + await userEvent.click(await screen.findByTestId('global-attribute-actions-field-1')); + + const del = screen.getAllByRole('menuitem').find((el) => el.textContent?.includes('Delete attribute')); + expect(del!).not.toHaveAttribute('aria-disabled', 'true'); + expect(del!).not.toHaveTextContent('Plugin-managed'); + + await userEvent.click(del!); + + // * The confirmation names the plugin the leftover came from, since an + // uninstalled plugin is otherwise invisible to the admin + expect(await screen.findByRole('heading', {name: /delete department attribute/i})).toBeInTheDocument(); + expect(screen.getByText(/was created by the plugin "com\.acme\.plugin", which is no longer installed/i)).toBeInTheDocument(); + + await userEvent.click(await screen.findByRole('button', {name: /^delete$/i})); + + await waitFor(() => { + expect(deletePropertyField).toHaveBeenCalledWith('access_control', 'template', 'field-1'); + }); + }); + + it('treats a plugin-owned row as protected while the plugin inventory is still in flight', async () => { + // An inventory that has not arrived looks byte-for-byte like a server with + // the plugin uninstalled, so only the settled fetch tells the two apart. + // This one never settles, pinning the row in the not-yet-known state; the + // 're-enables Delete' test above covers the settled side. + const getPluginStatuses = jest.spyOn(Client4, 'getPluginStatuses'). + mockImplementation(() => new Promise(() => {})); + + renderTable([makePluginOwnedField()]); + + await userEvent.click(await screen.findByTestId('global-attribute-actions-field-1')); + + const del = (await screen.findAllByRole('menuitem')).find((el) => el.textContent?.includes('Delete attribute')); + + // * Without the gate the empty inventory reads as "plugin gone", offering + // Delete behind a dialog that wrongly says the plugin was uninstalled + expect(del!).toHaveAttribute('aria-disabled', 'true'); + expect(del!).toHaveTextContent('Plugin-managed'); + + getPluginStatuses.mockRestore(); + }); + + it('omits the plugin explanation for an ordinary attribute', async () => { + renderTable([makeField({attrs: {display_name: 'Department'}})]); + + await openDeleteModal(); + + expect(await screen.findByText(/permanently remove its definition/i)).toBeInTheDocument(); + expect(screen.queryByText(/no longer installed/i)).not.toBeInTheDocument(); }); }); diff --git a/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.tsx b/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.tsx index a02eb1df7155..90f8943384d3 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/global_attributes_table.tsx @@ -4,18 +4,21 @@ import {createColumnHelper, getCoreRowModel, useReactTable, type ColumnDef} from '@tanstack/react-table'; import classNames from 'classnames'; import type {ComponentType} from 'react'; -import React, {useEffect, useMemo, useState} from 'react'; +import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import type {MessageDescriptor} from 'react-intl'; import {FormattedMessage, defineMessages, useIntl} from 'react-intl'; import {useDispatch, useSelector} from 'react-redux'; import {Link} from 'react-router-dom'; +import type {ClientError} from '@mattermost/client'; import {ChevronDownCircleOutlineIcon, ContentCopyIcon, DotsHorizontalIcon, FormatListBulletedIcon, MenuVariantIcon, OpenInNewIcon, PencilOutlineIcon, PowerPlugOutlineIcon, SortAscendingIcon, SyncIcon, TrashCanOutlineIcon} from '@mattermost/compass-icons/components'; import type IconProps from '@mattermost/compass-icons/components/props'; import {WithTooltip} from '@mattermost/shared/components/tooltip'; import type {FieldType, PropertyField, PropertyFieldOption} from '@mattermost/types/properties'; import {supportsOptions} from '@mattermost/types/properties'; +import PropertyTypes from 'mattermost-redux/action_types/properties'; +import {getPluginStatuses} from 'mattermost-redux/actions/admin'; import {fetchPropertyFields} from 'mattermost-redux/actions/properties'; import {getConfig as getAdminConfig} from 'mattermost-redux/selectors/entities/admin'; import {getLicense} from 'mattermost-redux/selectors/entities/general'; @@ -29,6 +32,8 @@ import { CLASSIFICATIONS_TEMPLATE_FIELD_NAME, CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE, } from 'components/admin_console/classification_markings/utils'; +import AlertBanner from 'components/alert_banner'; +import {useIsFieldOrphaned} from 'components/common/hooks/use_field_orphaned'; import LoadingScreen from 'components/loading_screen'; import * as Menu from 'components/menu'; @@ -37,6 +42,8 @@ import {LicenseSkus} from 'utils/constants'; import type {GlobalState} from 'types/store'; import {GLOBAL_ATTRIBUTES_GROUP_NAME, GLOBAL_ATTRIBUTES_OBJECT_TYPE, GLOBAL_ATTRIBUTES_TARGET_TYPE} from './constants'; +import {useGlobalAttributeFieldDelete} from './global_attribute_delete_modal'; +import {deleteAttributeField} from './utils'; import {it} from '../admin_definition_helpers'; import {AdminConsoleListTable} from '../list_table'; @@ -204,10 +211,42 @@ function AttributeCell({field, isClassificationRow}: ClassificationAwareCellProp ); } -function ActionsCell({field, isClassificationRow, isMobileView}: ClassificationAwareCellProps & {isMobileView: boolean}) { +type ActionsCellProps = ClassificationAwareCellProps & { + isMobileView: boolean; + pluginInventoryLoaded: boolean; + onDeleteError: (message: string | null) => void; + onDeleteModalExited: () => void; +}; + +function ActionsCell({field, isClassificationRow, isMobileView, pluginInventoryLoaded, onDeleteError, onDeleteModalExited}: ActionsCellProps) { const {formatMessage} = useIntl(); + const dispatch = useDispatch(); + const promptDelete = useGlobalAttributeFieldDelete(); const menuId = `global-attribute-actions-${field.id}`; + // A plugin-owned field is server-protected only while its plugin is installed, + // so the item stays disabled with a reason rather than offering a dead action. + // Once the plugin is uninstalled the server allows the delete (see + // checkFieldDeleteAccess in server/channels/app/properties/access_control.go) — + // that is how an admin cleans up what the plugin left behind. + // Not short-circuited into the hook call, which has to run unconditionally. + const fieldLooksOrphaned = useIsFieldOrphaned(field); + const isOrphaned = pluginInventoryLoaded && fieldLooksOrphaned; + const isPluginManaged = getSourceKind(field) === 'plugin' && !isOrphaned; + + const handleConfirmed = useCallback(async () => { + onDeleteError(null); + + try { + await deleteAttributeField(field.id); + dispatch({type: PropertyTypes.PROPERTY_FIELD_DELETED, data: {fieldId: field.id}}); + } catch (error) { + onDeleteError(formatMessage( + (error as ClientError)?.status_code === 409 ? actionsLabels.deleteErrorHasDependents : actionsLabels.deleteErrorGeneric, + )); + } + }, [dispatch, field.id, formatMessage, onDeleteError]); + if (isClassificationRow) { const classificationLinkLabel = formatMessage(actionsLabels.classificationLink); @@ -273,13 +312,19 @@ function ActionsCell({field, isClassificationRow, isMobileView}: ClassificationA /> } + onClick={isPluginManaged ? undefined : () => promptDelete( + getDisplayName(field), + handleConfirmed, + isOrphaned ? {sourcePluginId: field.attrs?.source_plugin_id as string | undefined} : undefined, + onDeleteModalExited, + )} labels={( <> - + {isPluginManaged && } )} /> @@ -292,6 +337,9 @@ export default function GlobalAttributesTable() { const [loaded, setLoaded] = useState(false); const [loadError, setLoadError] = useState(false); + const [deleteError, setDeleteError] = useState(null); + const [deleteModalExited, setDeleteModalExited] = useState(false); + const bannerRef = useRef(null); const groupId = useSelector((state: GlobalState) => getPropertyGroupByName(state, GLOBAL_ATTRIBUTES_GROUP_NAME)?.id ?? '', @@ -333,6 +381,64 @@ export default function GlobalAttributesTable() { }; }, [dispatch]); + // The Source column resolves plugin-owned rows to a plugin display name, but + // server-only plugins are absent from the webapp manifest registry — their names + // live in the admin plugin statuses, which nothing else on this page loads. + // Fetched once, and only when a plugin-owned row is actually present. + const hasPluginOwnedFields = useMemo(() => fields.some((field) => Boolean(field.attrs?.source_plugin_id)), [fields]); + const pluginStatusesRequested = useRef(false); + + // Whether the plugin inventory is known yet. This gates the orphan check + // rather than the Source column, which degrades harmlessly to the plugin ID: + // an inventory that has not arrived is indistinguishable from one where + // nothing is installed, and isFieldOrphaned reads the latter as "every + // plugin-owned field is orphaned". Acting on that would briefly offer Delete + // on a still-protected field, behind a dialog wrongly claiming the plugin was + // uninstalled -- and the server would then refuse it anyway. Settled rather + // than resolved: a failed fetch still leaves the inventory as good as it will + // get, and staying false forever would strand genuine leftovers as + // undeletable. + const [pluginInventoryLoaded, setPluginInventoryLoaded] = useState(false); + + useEffect(() => { + if (!hasPluginOwnedFields || pluginStatusesRequested.current) { + return; + } + + pluginStatusesRequested.current = true; + dispatch(getPluginStatuses()).finally(() => setPluginInventoryLoaded(true)); + }, [dispatch, hasPluginOwnedFields]); + + const handleDeleteModalExited = useCallback(() => setDeleteModalExited(true), []); + + // The banner sits above the table, so a delete triggered from a row further + // down can land off-screen. Two things make the timing here load-bearing: + // + // GenericModal passes restoreFocus, so on close react-bootstrap returns focus + // to the row's actions button -- far down the list -- and focusing an + // off-screen element scrolls it back into view. Scrolling before that happens + // is simply undone, so the page has to settle first. + // + // GenericModal also starts closing *before* it invokes handleConfirm, and the + // delete request may finish either side of the modal's fade. So the error and + // the exit arrive in either order; act only once both have landed. + useEffect(() => { + if (!deleteError || !deleteModalExited) { + return; + } + + // Disarmed so the next delete waits for its own modal to close rather + // than acting on this attempt's stale exit. + setDeleteModalExited(false); + + // Focus without its own scroll, then scroll deliberately: the banner ends + // up owning focus for keyboard and screen reader users -- who would + // otherwise be returned to a row button and have to hunt for the error -- + // and the browser never scrolls anywhere we did not ask it to. + bannerRef.current?.focus?.({preventScroll: true}); + bannerRef.current?.closest('.admin-console__wrapper')?.scrollTo?.({top: 0}); + }, [deleteError, deleteModalExited]); + const rows = useMemo( () => [...fields].sort((a, b) => getDisplayName(a).localeCompare(getDisplayName(b))), [fields], @@ -409,12 +515,15 @@ export default function GlobalAttributesTable() { field={row.original} isClassificationRow={isClassificationRow(row.original)} isMobileView={isMobileView} + pluginInventoryLoaded={pluginInventoryLoaded} + onDeleteError={setDeleteError} + onDeleteModalExited={handleDeleteModalExited} /> ), enableHiding: false, }), ]; - }, [groupId, classificationMarkingsReachable, isMobileView]); + }, [groupId, classificationMarkingsReachable, isMobileView, pluginInventoryLoaded, handleDeleteModalExited]); const table = useReactTable({ data: rows, @@ -456,6 +565,26 @@ export default function GlobalAttributesTable() { return (
+ {/* Kept mounted with only its content swapped in -- an alert inserted at the + same moment as its text is not reliably announced (same reason + attribute_external_source.tsx keeps its status region mounted). */} +
+ {deleteError && ( + setDeleteError(null)} + /> + )} +
table={table}/>
); @@ -509,6 +638,15 @@ export const actionsLabels = defineMessages({ duplicate: {id: 'admin.global_attributes.table.actions.duplicate', defaultMessage: 'Duplicate attribute'}, delete: {id: 'admin.global_attributes.table.actions.delete', defaultMessage: 'Delete attribute'}, comingSoon: {id: 'admin.global_attributes.table.actions.coming_soon', defaultMessage: 'Coming soon'}, + pluginManaged: {id: 'admin.global_attributes.table.actions.plugin_managed', defaultMessage: 'Plugin-managed'}, + deleteErrorHasDependents: { + id: 'admin.global_attributes.confirm.delete.error.has_dependents', + defaultMessage: "This attribute can't be deleted because other attributes are still linked to it. Remove those links first, then try again.", + }, + deleteErrorGeneric: { + id: 'admin.global_attributes.confirm.delete.error.generic', + defaultMessage: 'An error occurred while deleting this attribute. Please try again.', + }, classificationLink: { id: 'admin.global_attributes.table.actions.classification_link', defaultMessage: 'Open Classification Markings', diff --git a/webapp/channels/src/components/admin_console/global_attributes/utils.ts b/webapp/channels/src/components/admin_console/global_attributes/utils.ts index 4f1ca8c95887..4877445a851b 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/utils.ts +++ b/webapp/channels/src/components/admin_console/global_attributes/utils.ts @@ -57,3 +57,10 @@ export function createAttributeField( }, }); } + +// Deletes a template field from the access_control group. The server returns +// 409 when the field still has active linked dependents (CountLinkedFields > 0); +// callers are expected to surface that case distinctly. +export function deleteAttributeField(fieldId: string): Promise { + return Client4.deletePropertyField(GLOBAL_ATTRIBUTES_GROUP_NAME, GLOBAL_ATTRIBUTES_OBJECT_TYPE, fieldId); +} diff --git a/webapp/channels/src/components/admin_console/system_properties/orphaned_fields_utils.ts b/webapp/channels/src/components/admin_console/system_properties/orphaned_fields_utils.ts deleted file mode 100644 index e063818859c3..000000000000 --- a/webapp/channels/src/components/admin_console/system_properties/orphaned_fields_utils.ts +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import {useSelector} from 'react-redux'; - -import type {UserPropertyField} from '@mattermost/types/properties_user'; - -import type {GlobalState} from 'types/store'; - -export function isFieldOrphaned( - field: UserPropertyField, - installedPlugins: Record, -): boolean { - const sourcePluginId = field.attrs?.source_plugin_id; - const isProtected = Boolean(field.attrs?.protected); - - // Field is orphaned if it's protected, has a source plugin ID, - // but that plugin isn't installed - return isProtected && Boolean(sourcePluginId) && !installedPlugins[sourcePluginId as string]; -} - -export function useIsFieldOrphaned(field: UserPropertyField): boolean { - const installedPlugins = useSelector((state: GlobalState) => state.entities.admin.plugins ?? {}); - return isFieldOrphaned(field, installedPlugins); -} diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx index ca970d31d423..1936b0e52da5 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_table.tsx @@ -14,13 +14,13 @@ import {type UserPropertyField} from '@mattermost/types/properties_user'; import {collectionToArray} from '@mattermost/types/utilities'; import AlertBanner from 'components/alert_banner'; +import {useIsFieldOrphaned} from 'components/common/hooks/use_field_orphaned'; import LoadingScreen from 'components/loading_screen'; import Constants from 'utils/constants'; import {CPA_FIELD_NAME_RESERVED_WORDS, filterCELIdentifier, slugifyForCEL} from 'utils/properties'; import {BorderlessInput, LinkButton} from './controls'; -import {useIsFieldOrphaned} from './orphaned_fields_utils'; import type {SectionHook} from './section_utils'; import DotMenu from './user_properties_dot_menu'; import OrphanedFieldDeleteButton from './user_properties_orphaned_delete_button'; diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx index 55a5d67bf5a5..0f4c12d74afc 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx @@ -16,13 +16,14 @@ import {type UserPropertyField} from '@mattermost/types/properties_user'; import {getPluginDisplayName} from 'selectors/plugins'; +import {useIsFieldOrphaned} from 'components/common/hooks/use_field_orphaned'; + import Constants from 'utils/constants'; import {isKeyPressed} from 'utils/keyboard'; import type {GlobalState} from 'types/store'; import {DangerText} from './controls'; -import {useIsFieldOrphaned} from './orphaned_fields_utils'; import './user_properties_values.scss'; import {useAttributeLinkModal} from './user_properties_dot_menu'; import UserPropertyRankValues from './user_properties_rank_values'; diff --git a/webapp/channels/src/components/common/hooks/use_field_orphaned.ts b/webapp/channels/src/components/common/hooks/use_field_orphaned.ts new file mode 100644 index 000000000000..e72949777709 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/use_field_orphaned.ts @@ -0,0 +1,43 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {useMemo} from 'react'; +import {useSelector} from 'react-redux'; + +import type {PropertyField} from '@mattermost/types/properties'; + +import {isFieldOrphaned} from 'utils/properties'; + +import type {GlobalState} from 'types/store'; + +/** + * Ids of every plugin the admin console knows to be installed. + * + * Two slices are unioned because the attribute pages populate different ones: + * `admin.plugins` is filled by the admin sidebar's getPlugins() on mount, while + * `admin.pluginStatuses` is filled by the per-page getPluginStatuses() fetch + * that resolves server-only plugins. Taking the union keeps a field from + * reading as orphaned just because the page rendering it loaded only one. + * + * Both slices start as `{}`, so an inventory that has not loaded yet is + * indistinguishable from one where nothing is installed -- and the latter is a + * real state, since uninstalling the last plugin is exactly when leftovers need + * cleaning up. Neither this hook nor useIsFieldOrphaned can therefore tell + * "not known yet" from "nothing installed"; callers that act on the result must + * gate it on their own fetch having settled (GlobalAttributesTable does this via + * pluginInventoryLoaded). + */ +export function useInstalledPluginIds(): ReadonlySet { + const plugins = useSelector((state: GlobalState) => state.entities.admin.plugins); + const pluginStatuses = useSelector((state: GlobalState) => state.entities.admin.pluginStatuses); + + return useMemo( + () => new Set([...Object.keys(plugins ?? {}), ...Object.keys(pluginStatuses ?? {})]), + [plugins, pluginStatuses], + ); +} + +export function useIsFieldOrphaned(field: Pick): boolean { + const installedPluginIds = useInstalledPluginIds(); + return isFieldOrphaned(field, installedPluginIds); +} diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index e71415c704ee..9eb72db20536 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -1491,6 +1491,11 @@ "admin.global_attributes.attribute_details.unique_name.edit_aria_label": "Edit unique name", "admin.global_attributes.attribute_details.unique_name.helper_text": "Name is the internal identifier for policies and integrations. Display name is what admins and users see.", "admin.global_attributes.attribute_details.unique_name.prefix": "Unique name:", + "admin.global_attributes.confirm.delete.body": "Deleting this attribute will permanently remove its definition. This action cannot be undone.", + "admin.global_attributes.confirm.delete.error.generic": "An error occurred while deleting this attribute. Please try again.", + "admin.global_attributes.confirm.delete.error.has_dependents": "This attribute can't be deleted because other attributes are still linked to it. Remove those links first, then try again.", + "admin.global_attributes.confirm.delete.orphaned_body": "This attribute was created by the plugin \"{pluginId}\", which is no longer installed.", + "admin.global_attributes.confirm.delete.title": "Delete {name} attribute", "admin.global_attributes.new_attribute": "New attribute", "admin.global_attributes.subtitle": "Define an attribute once, then choose which resources can use it.", "admin.global_attributes.table.actions.classification_link": "Open Classification Markings", @@ -1499,6 +1504,7 @@ "admin.global_attributes.table.actions.duplicate": "Duplicate attribute", "admin.global_attributes.table.actions.edit": "Edit attribute", "admin.global_attributes.table.actions.menu_label": "Select an action", + "admin.global_attributes.table.actions.plugin_managed": "Plugin-managed", "admin.global_attributes.table.actions.tooltip": "More actions", "admin.global_attributes.table.applies_to": "Applies to", "admin.global_attributes.table.attribute": "Attribute", diff --git a/webapp/channels/src/selectors/plugins.ts b/webapp/channels/src/selectors/plugins.ts index b34c825c7686..5d6c031e17e0 100644 --- a/webapp/channels/src/selectors/plugins.ts +++ b/webapp/channels/src/selectors/plugins.ts @@ -179,6 +179,13 @@ export const getPluginDisplayName = (state: GlobalState, pluginId?: string): str if (!pluginId) { return 'unknown'; } - const plugins = state.plugins?.plugins ?? {}; - return plugins[pluginId]?.name || pluginId; + + // state.plugins.plugins only holds manifests for plugins that shipped a webapp + // bundle and registered themselves in the browser. Server-only plugins never + // appear there, so consult the admin plugin statuses (populated by + // getPluginStatuses) before falling back to the raw, user-hostile plugin ID. + const webappName = state.plugins?.plugins?.[pluginId]?.name; + const installedName = state.entities?.admin?.pluginStatuses?.[pluginId]?.name; + + return webappName || installedName || pluginId; }; diff --git a/webapp/channels/src/utils/constants.tsx b/webapp/channels/src/utils/constants.tsx index cc87d986593e..881bc9fd0a50 100644 --- a/webapp/channels/src/utils/constants.tsx +++ b/webapp/channels/src/utils/constants.tsx @@ -501,6 +501,7 @@ export const ModalIdentifiers = { USER_PROPERTY_FIELD_DELETE: 'user_property_field_delete', SESSION_ATTRIBUTE_DISABLE: 'session_attribute_disable', BOARD_ATTRIBUTE_FIELD_DELETE: 'board_attribute_field_delete', + GLOBAL_ATTRIBUTE_FIELD_DELETE: 'global_attribute_field_delete', ATTRIBUTE_MODAL_LDAP: 'attribute_modal_ldap', ATTRIBUTE_MODAL_SAML: 'attribute_modal_saml', RANKED_SCHEMA_MODAL: 'ranked_schema_modal', diff --git a/webapp/channels/src/utils/properties.test.ts b/webapp/channels/src/utils/properties.test.ts index 32786f5ff895..ccad40e6bf4b 100644 --- a/webapp/channels/src/utils/properties.test.ts +++ b/webapp/channels/src/utils/properties.test.ts @@ -6,6 +6,7 @@ import { CPA_FIELD_NAME_RESERVED_WORDS, filterCELIdentifier, getUserPropertyFieldLabel, + isFieldOrphaned, slugifyForCEL, validateCPAFieldName, } from './properties'; @@ -274,3 +275,30 @@ describe('filterCELIdentifier', () => { expect(filterCELIdentifier(input)).toBe(expected); }); }); + +describe('isFieldOrphaned', () => { + const installed = new Set(['com.acme.plugin']); + + it('reports a plugin-owned field whose plugin is gone', () => { + const field = {attrs: {source_plugin_id: 'com.acme.removed', protected: true}}; + expect(isFieldOrphaned(field, installed)).toBe(true); + }); + + it('does not report a plugin-owned field whose plugin is still installed', () => { + const field = {attrs: {source_plugin_id: 'com.acme.plugin', protected: true}}; + expect(isFieldOrphaned(field, installed)).toBe(false); + }); + + // An unprotected field is admin-managed regardless of where it came from, so it + // is never "orphaned" — it was always the admin's to delete. + it('does not report an unprotected field even when its plugin is gone', () => { + const field = {attrs: {source_plugin_id: 'com.acme.removed'}}; + expect(isFieldOrphaned(field, installed)).toBe(false); + }); + + it('does not report a field with no source plugin at all', () => { + expect(isFieldOrphaned({attrs: {protected: true}}, installed)).toBe(false); + expect(isFieldOrphaned({attrs: {}}, installed)).toBe(false); + expect(isFieldOrphaned({}, installed)).toBe(false); + }); +}); diff --git a/webapp/channels/src/utils/properties.ts b/webapp/channels/src/utils/properties.ts index a76d94b5c2ea..161c475c878d 100644 --- a/webapp/channels/src/utils/properties.ts +++ b/webapp/channels/src/utils/properties.ts @@ -1,6 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import type {PropertyField} from '@mattermost/types/properties'; import type {UserPropertyField} from '@mattermost/types/properties_user'; /** @@ -127,3 +128,27 @@ export function slugifyForCEL(name: string): string { slug = slug.replace(/_+/g, '_').replace(/_+$/, ''); return slug || '_copy'; } + +/** + * A plugin-owned attribute is "orphaned" once its source plugin is no longer + * installed. The server permits an admin to delete an orphaned field so the + * leftovers of an uninstalled plugin can be cleaned up, and refuses the delete + * while the plugin is still installed — see checkFieldDeleteAccess in + * server/channels/app/properties/access_control.go. + * + * An empty `installedPluginIds` means "nothing is installed", which reads every + * plugin-owned field as orphaned. Callers are responsible for having fetched the + * plugin list before acting on the result. + */ +export function isFieldOrphaned( + field: Pick, + installedPluginIds: ReadonlySet, +): boolean { + const sourcePluginId = field.attrs?.source_plugin_id as string | undefined; + + if (!sourcePluginId || !field.attrs?.protected) { + return false; + } + + return !installedPluginIds.has(sourcePluginId); +} From 78d120399f1b5e9474e9b453fdc909fabf65801a Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Tue, 18 Aug 2026 10:58:59 -0600 Subject: [PATCH 4/5] MM-68396: Remove deprecated dialog date/datetime fields for v12.0 (#37759) * Drop top-level min_date/max_date/time_interval and allow_manual_time_entry; require datetime_config (and manual_time_entry). Update docs, tests, and e2e fixtures accordingly. * update important-upgrade-notes.rst per Doc Impact Analysis --- .../plugins/interactive-dialogs/index.md | 8 +- .../upgrade/important-upgrade-notes.mdx | 4 + e2e-tests/cypress/utils/webhook_utils.js | 12 +- .../assets/webhook/utils/webhook_utils.js | 12 +- server/public/model/integration_action.go | 39 +-- .../public/model/integration_action_test.go | 229 ++++++++---------- .../apps_form/apps_form_component.test.tsx | 167 ++----------- .../apps_form/apps_form_component.tsx | 32 +-- .../apps_form_date_field.test.tsx | 18 +- .../apps_form_date_field.tsx | 6 +- .../apps_form_datetime_field.test.tsx | 42 ++-- .../apps_form_datetime_field.tsx | 16 +- .../src/utils/integration_utils.test.ts | 57 +---- .../src/utils/integration_utils.ts | 6 +- .../src/utils/apps_type_guard.test.ts | 49 ---- .../src/utils/dialog_conversion.test.ts | 158 ++---------- .../channels/src/utils/dialog_conversion.ts | 23 +- webapp/platform/types/src/apps.ts | 40 --- webapp/platform/types/src/integrations.ts | 12 - 19 files changed, 231 insertions(+), 699 deletions(-) diff --git a/docs/develop/integrate/plugins/interactive-dialogs/index.md b/docs/develop/integrate/plugins/interactive-dialogs/index.md index 9b2e6288cb29..4eb8b09bcac8 100644 --- a/docs/develop/integrate/plugins/interactive-dialogs/index.md +++ b/docs/develop/integrate/plugins/interactive-dialogs/index.md @@ -436,8 +436,6 @@ The full list of supported fields for `date` elements is included below: | `help_text` | String | (Optional) Help text displayed below the field. Maximum 150 characters. | | `optional` | Boolean | (Optional) Set to `true` if this form element is not required. Default is `false`. | | `datetime_config` | Object | (Optional) Nested date configuration object. See [datetime_config object](#datetime_config-object) for supported properties. | -| `min_date` | String | (Deprecated — use `datetime_config.min_date`.) Earliest selectable date. Supports ISO date format (YYYY-MM-DD) or relative formats (`today`, `tomorrow`, `+1d`, `-7d`, etc.). Full ISO datetime strings are accepted, but only the date part is parsed; timezone information is ignored. | -| `max_date` | String | (Deprecated — use `datetime_config.max_date`.) Latest selectable date. Supports ISO date format (YYYY-MM-DD) or relative formats (`today`, `+30d`, `+1y`, etc.). Full ISO datetime strings are accepted, but only the date part is parsed; timezone information is ignored. | #### Date field usage examples @@ -505,9 +503,6 @@ The full list of supported fields for `datetime` elements is included below: | `help_text` | String | (Optional) Help text displayed below the field. Maximum 150 characters. | | `optional` | Boolean | (Optional) Set to `true` if this form element is not required. Default is `false`. | | `datetime_config` | Object | (Optional) Nested datetime configuration object. See [datetime_config object](#datetime_config-object) for supported properties. | -| `min_date` | String | (Deprecated — use `datetime_config.min_date`.) Earliest selectable date. Supports ISO format or relative formats (`today`, `tomorrow`, `+1d`, `-7d`, etc.). | -| `max_date` | String | (Deprecated — use `datetime_config.max_date`.) Latest selectable date. Supports ISO format or relative formats (`today`, `+30d`, `+1y`, etc.). | -| `time_interval` | Integer | (Deprecated — use `datetime_config.time_interval`.) Time selection interval in minutes. Must be between 1 and 1440, and must be a divisor of 1440 to create evenly spaced intervals throughout the day. Common values: 15, 30, 60, 90, 120. Default is 60. | #### DateTime field usage examples @@ -570,9 +565,8 @@ The `datetime_config` object groups date/datetime configuration into a single ne | `time_interval` | Integer | `datetime` | 11.6 | (Optional) Time selection interval in minutes. Must be between 1 and 1440, and must be a divisor of 1440. Default is 60. | | `location_timezone` | String | `datetime` | 11.6 | (Optional) IANA timezone used to display and submit the time (e.g. `America/Denver`, `Asia/Tokyo`). When set, all users see the same wall-clock time regardless of their own timezone. Defaults to the viewing user's timezone. | | `manual_time_entry` | Boolean | `datetime` | 11.8 | (Optional) When `true`, users can type the time directly in addition to using the dropdown. Default is `false`. | -| `allow_manual_time_entry` | Boolean | `datetime` | 11.6 (deprecated in 11.8) | (Deprecated — use `manual_time_entry`.) When both are set, either enabling turns the feature on. | -**Backward compatibility (new in 11.8):** The top-level `min_date`, `max_date`, and `time_interval` fields on `date` and `datetime` elements are still accepted for existing integrations, but are deprecated in favor of `datetime_config`. When both are provided on the same element, values inside `datetime_config` take precedence over the legacy top-level values. +> **Breaking change (12.0):** The top-level `min_date`, `max_date`, and `time_interval` fields on `date`/`datetime` elements, and the `datetime_config.allow_manual_time_entry` field, have been removed. Integrations must send these values under `datetime_config` (using `manual_time_entry` instead of `allow_manual_time_entry`) or they will be silently ignored. #### Date and DateTime field specifications diff --git a/docs/main/administration-guide/upgrade/important-upgrade-notes.mdx b/docs/main/administration-guide/upgrade/important-upgrade-notes.mdx index 2c3cef926f64..219da7ae1709 100644 --- a/docs/main/administration-guide/upgrade/important-upgrade-notes.mdx +++ b/docs/main/administration-guide/upgrade/important-upgrade-notes.mdx @@ -21,6 +21,10 @@ We recommend reviewing the [additional upgrade notes](#additional-upgrade-notes) +v12.0 +

Mattermost v12.0 removes the deprecated interactive dialog date/datetime fields. Top-level min_date, max_date, and time_interval on dialog elements and app fields, and datetime_config.allow_manual_time_entry, are no longer accepted. Integrations must migrate to datetime_config (using manual_time_entry instead of allow_manual_time_entry) before upgrading to v12.0. Legacy keys are silently ignored, so date constraints and manual time entry will not apply until payloads are updated. See the interactive dialogs documentation for details.

+ + v11.9

Mattermost v11.9 changes how redirect URI allowlist patterns are matched for OAuth Dynamic Client Registration (DCR). Patterns are now evaluated per URL component (scheme, host, path, and query) rather than as a whole-string glob. As a result, a pattern such as https://\*.example.com/\*\* no longer matches redirect URIs that include a query string (for example, https://app.example.com/callback?tenant=foo); redirect URIs without a query string continue to match as expected.

Admins using DCR with redirect URIs that include query strings must update their allowlist. To allow redirect URIs both with and without a query string, add two separate entries:

  • https://\*.example.com/\*\* — matches redirect URIs with no query string.
  • https://\*.example.com/\*\*?\*\* — matches redirect URIs with any query string.

A pattern that includes a query component (such as ?\*\*) only matches URIs that also carry a query string; it will not match URIs without one. Both entries are required to cover both cases.

diff --git a/e2e-tests/cypress/utils/webhook_utils.js b/e2e-tests/cypress/utils/webhook_utils.js index 9543c7e6dced..e21c4fd991af 100644 --- a/e2e-tests/cypress/utils/webhook_utils.js +++ b/e2e-tests/cypress/utils/webhook_utils.js @@ -437,7 +437,9 @@ function getBasicDateTimeDialog(triggerId, webhookBaseUrl) { placeholder: 'Select date and time', help_text: 'Select the date and time for your meeting', optional: false, - time_interval: 60, + datetime_config: { + time_interval: 60, + }, }, ], submit_label: 'Submit', @@ -465,7 +467,9 @@ function getMinDateConstraintDialog(triggerId, webhookBaseUrl) { placeholder: 'Select a future date', help_text: 'Must be today or later', optional: true, - min_date: 'today', + datetime_config: { + min_date: 'today', + }, }, ], submit_label: 'Submit', @@ -493,7 +497,9 @@ function getCustomIntervalDialog(triggerId, webhookBaseUrl) { placeholder: 'Select time (30min intervals)', help_text: 'Time picker with 30-minute intervals', optional: true, - time_interval: 30, + datetime_config: { + time_interval: 30, + }, }, ], submit_label: 'Submit', diff --git a/e2e-tests/playwright/lib/src/containers/assets/webhook/utils/webhook_utils.js b/e2e-tests/playwright/lib/src/containers/assets/webhook/utils/webhook_utils.js index 0925d6aa3140..f31bc5685b3f 100644 --- a/e2e-tests/playwright/lib/src/containers/assets/webhook/utils/webhook_utils.js +++ b/e2e-tests/playwright/lib/src/containers/assets/webhook/utils/webhook_utils.js @@ -567,7 +567,9 @@ function getBasicDateTimeDialog(triggerId, webhookBaseUrl) { placeholder: 'Select date and time', help_text: 'Select the date and time for your meeting', optional: false, - time_interval: 60, + datetime_config: { + time_interval: 60, + }, }, ], submit_label: 'Submit', @@ -595,7 +597,9 @@ function getMinDateConstraintDialog(triggerId, webhookBaseUrl) { placeholder: 'Select a future date', help_text: 'Must be today or later', optional: true, - min_date: 'today', + datetime_config: { + min_date: 'today', + }, }, ], submit_label: 'Submit', @@ -623,7 +627,9 @@ function getCustomIntervalDialog(triggerId, webhookBaseUrl) { placeholder: 'Select time (30min intervals)', help_text: 'Time picker with 30-minute intervals', optional: true, - time_interval: 30, + datetime_config: { + time_interval: 30, + }, }, ], submit_label: 'Submit', diff --git a/server/public/model/integration_action.go b/server/public/model/integration_action.go index 05e16b16489a..9602cb2370c2 100644 --- a/server/public/model/integration_action.go +++ b/server/public/model/integration_action.go @@ -467,9 +467,6 @@ type DialogDateTimeConfig struct { LocationTimezone string `json:"location_timezone,omitempty"` // ManualTimeEntry: Allow manual text entry for time instead of dropdown ManualTimeEntry bool `json:"manual_time_entry,omitempty"` - // Deprecated: Use ManualTimeEntry instead. Kept for backward compatibility; - // when both are provided, either field being true enables manual time entry. - AllowManualTimeEntry bool `json:"allow_manual_time_entry,omitempty"` } type DialogElement struct { @@ -492,46 +489,18 @@ type DialogElement struct { // Date/datetime field configuration DateTimeConfig *DialogDateTimeConfig `json:"datetime_config,omitempty"` - // Deprecated: Use DateTimeConfig.MinDate instead. Kept for backward compatibility; - // if DateTimeConfig is provided, its MinDate takes precedence. - MinDate string `json:"min_date,omitempty"` - // Deprecated: Use DateTimeConfig.MaxDate instead. Kept for backward compatibility; - // if DateTimeConfig is provided, its MaxDate takes precedence. - MaxDate string `json:"max_date,omitempty"` - // Deprecated: Use DateTimeConfig.TimeInterval instead. Kept for backward compatibility; - // if DateTimeConfig is provided, its TimeInterval takes precedence. - TimeInterval int `json:"time_interval,omitempty"` // Action button configuration (type "action_button") ActionButton *DialogActionButton `json:"action_button,omitempty"` } -// EffectiveDateTimeConfig returns the resolved date/datetime configuration by -// merging DateTimeConfig over the deprecated top-level fields (MinDate, MaxDate, -// TimeInterval). DateTimeConfig values take precedence when set. +// EffectiveDateTimeConfig returns the resolved date/datetime configuration, +// treating a nil DateTimeConfig as the zero value. func (e *DialogElement) EffectiveDateTimeConfig() DialogDateTimeConfig { - cfg := DialogDateTimeConfig{ - MinDate: e.MinDate, - MaxDate: e.MaxDate, - TimeInterval: e.TimeInterval, - } if e.DateTimeConfig != nil { - if e.DateTimeConfig.MinDate != "" { - cfg.MinDate = e.DateTimeConfig.MinDate - } - if e.DateTimeConfig.MaxDate != "" { - cfg.MaxDate = e.DateTimeConfig.MaxDate - } - if e.DateTimeConfig.TimeInterval != 0 { - cfg.TimeInterval = e.DateTimeConfig.TimeInterval - } - cfg.LocationTimezone = e.DateTimeConfig.LocationTimezone - // ManualTimeEntry is OR'd with the deprecated AllowManualTimeEntry. Booleans can't - // distinguish explicit-false from not-set across JSON (omitempty drops the zero value), - // so either field being true must enable the feature during the deprecation window. - cfg.ManualTimeEntry = e.DateTimeConfig.ManualTimeEntry || e.DateTimeConfig.AllowManualTimeEntry + return *e.DateTimeConfig } - return cfg + return DialogDateTimeConfig{} } type DialogActionButton struct { diff --git a/server/public/model/integration_action_test.go b/server/public/model/integration_action_test.go index 8b1526ef7501..592850b17edf 100644 --- a/server/public/model/integration_action_test.go +++ b/server/public/model/integration_action_test.go @@ -1391,9 +1391,11 @@ func TestDialogElementDateTimeValidation(t *testing.T) { DisplayName: "Test Date", Name: "test_date", Type: "date", - MinDate: "2025-01-01", - MaxDate: "2025-12-31", - Optional: false, + DateTimeConfig: &DialogDateTimeConfig{ + MinDate: "2025-01-01", + MaxDate: "2025-12-31", + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err) @@ -1401,13 +1403,15 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should validate DialogElement with datetime type and time properties", func(t *testing.T) { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - MinDate: "2025-01-01T00:00:00Z", - MaxDate: "2025-12-31T23:59:59Z", - TimeInterval: 30, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + MinDate: "2025-01-01T00:00:00Z", + MaxDate: "2025-12-31T23:59:59Z", + TimeInterval: 30, + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err) @@ -1415,13 +1419,15 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should validate DialogElement with datetime type and relative min/max", func(t *testing.T) { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - MinDate: "+2H", - MaxDate: "+7d", - TimeInterval: 30, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + MinDate: "+2H", + MaxDate: "+7d", + TimeInterval: 30, + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err) @@ -1429,13 +1435,15 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should accept datetime DialogElement with date-only min/max for backward compatibility", func(t *testing.T) { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - MinDate: "2025-01-01", - MaxDate: "2025-12-31", - TimeInterval: 30, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + MinDate: "2025-01-01", + MaxDate: "2025-12-31", + TimeInterval: 30, + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err) @@ -1446,8 +1454,10 @@ func TestDialogElementDateTimeValidation(t *testing.T) { DisplayName: "Test Date", Name: "test_date", Type: "date", - MinDate: "invalid-date", - Optional: false, + DateTimeConfig: &DialogDateTimeConfig{ + MinDate: "invalid-date", + }, + Optional: false, } err := element.IsValid() assert.Error(t, err) @@ -1456,11 +1466,13 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should reject DialogElement with invalid time_interval", func(t *testing.T) { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: -1, // Invalid - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: -1, // Invalid + }, + Optional: false, } err := element.IsValid() assert.Error(t, err) @@ -1469,11 +1481,13 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should reject DialogElement with time_interval that is not a divisor of 1440", func(t *testing.T) { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: 729, // Invalid - not a divisor of 1440 - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: 729, // Invalid - not a divisor of 1440 + }, + Optional: false, } err := element.IsValid() assert.Error(t, err) @@ -1485,11 +1499,13 @@ func TestDialogElementDateTimeValidation(t *testing.T) { for _, interval := range validIntervals { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: interval, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: interval, + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err, "time_interval %d should be valid", interval) @@ -1501,11 +1517,13 @@ func TestDialogElementDateTimeValidation(t *testing.T) { for _, interval := range invalidIntervals { element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: interval, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: interval, + }, + Optional: false, } err := element.IsValid() assert.Error(t, err, "time_interval %d should be invalid", interval) @@ -1516,22 +1534,26 @@ func TestDialogElementDateTimeValidation(t *testing.T) { t.Run("should use default time_interval of 60 minutes when zero", func(t *testing.T) { // Valid with explicit 60-minute interval element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: DefaultTimeIntervalMinutes, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: DefaultTimeIntervalMinutes, + }, + Optional: false, } err := element.IsValid() assert.NoError(t, err) // time_interval=0 means omitted — treated as default, should pass validation element = DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - TimeInterval: 0, - Optional: false, + DisplayName: "Test DateTime", + Name: "test_datetime", + Type: "datetime", + DateTimeConfig: &DialogDateTimeConfig{ + TimeInterval: 0, + }, + Optional: false, } err = element.IsValid() assert.NoError(t, err) @@ -1594,64 +1616,6 @@ func TestDialogElementDateTimeValidation(t *testing.T) { assert.Contains(t, err.Error(), "divisor of 1440") }) - t.Run("DateTimeConfig should take precedence over legacy fields", func(t *testing.T) { - element := DialogElement{ - DisplayName: "Test Date", - Name: "test_date", - Type: "date", - MinDate: "invalid-date", - DateTimeConfig: &DialogDateTimeConfig{ - MinDate: "2025-01-01", - }, - } - cfg := element.EffectiveDateTimeConfig() - assert.Equal(t, "2025-01-01", cfg.MinDate) - }) - - t.Run("legacy fields used when DateTimeConfig not provided", func(t *testing.T) { - element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - MinDate: "2025-01-01T00:00:00Z", - MaxDate: "2025-12-31T23:59:59Z", - TimeInterval: 30, - } - cfg := element.EffectiveDateTimeConfig() - assert.Equal(t, "2025-01-01T00:00:00Z", cfg.MinDate) - assert.Equal(t, "2025-12-31T23:59:59Z", cfg.MaxDate) - assert.Equal(t, 30, cfg.TimeInterval) - }) - - t.Run("ManualTimeEntry resolves via OR across new and deprecated fields", func(t *testing.T) { - cases := []struct { - name string - newField bool - oldField bool - expected bool - }{ - {"both false", false, false, false}, - {"only new true", true, false, true}, - {"only deprecated true", false, true, true}, - {"both true", true, true, true}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - element := DialogElement{ - DisplayName: "Test DateTime", - Name: "test_datetime", - Type: "datetime", - DateTimeConfig: &DialogDateTimeConfig{ - ManualTimeEntry: tc.newField, - AllowManualTimeEntry: tc.oldField, - }, - } - cfg := element.EffectiveDateTimeConfig() - assert.Equal(t, tc.expected, cfg.ManualTimeEntry) - }) - } - }) - t.Run("ManualTimeEntry marshals under manual_time_entry JSON key", func(t *testing.T) { cfg := DialogDateTimeConfig{ManualTimeEntry: true} b, err := json.Marshal(cfg) @@ -1659,22 +1623,31 @@ func TestDialogElementDateTimeValidation(t *testing.T) { assert.Contains(t, string(b), `"manual_time_entry":true`) }) - t.Run("deprecated allow_manual_time_entry payload still enables manual entry end-to-end", func(t *testing.T) { - // Simulate a legacy integrator sending only the deprecated field. - payload := []byte(`{"allow_manual_time_entry":true}`) + t.Run("removed legacy top-level fields are silently ignored on unmarshal", func(t *testing.T) { + // MM-68396: min_date, max_date, and time_interval are no longer DialogElement + // fields (moved to DateTimeConfig). This documents the intended breaking-change + // behavior for integrations still sending them at the top level: encoding/json + // drops unrecognized keys, so the element ends up with no date/datetime config + // and IsValid() no longer applies constraints derived from them. + payload := []byte(`{ + "display_name": "Test Date", + "name": "test_date", + "type": "date", + "min_date": "invalid-date", + "max_date": "2025-12-31" + }`) + var element DialogElement + require.NoError(t, json.Unmarshal(payload, &element)) + + assert.Nil(t, element.DateTimeConfig, "legacy top-level fields must not populate DateTimeConfig") + assert.NoError(t, element.IsValid(), "an invalid legacy min_date must no longer fail validation since the field is unrecognized") + }) + + t.Run("removed deprecated AllowManualTimeEntry is silently ignored on unmarshal", func(t *testing.T) { + payload := []byte(`{"allow_manual_time_entry": true}`) var cfg DialogDateTimeConfig require.NoError(t, json.Unmarshal(payload, &cfg)) - require.False(t, cfg.ManualTimeEntry, "new field should remain zero-value after unmarshal") - require.True(t, cfg.AllowManualTimeEntry, "deprecated field should unmarshal under its legacy tag") - - element := DialogElement{ - DisplayName: "Test", - Name: "t", - Type: "datetime", - DateTimeConfig: &cfg, - } - effective := element.EffectiveDateTimeConfig() - assert.True(t, effective.ManualTimeEntry, "deprecated field alone should enable manual entry after EffectiveDateTimeConfig") + assert.False(t, cfg.ManualTimeEntry, "the deprecated key must no longer populate ManualTimeEntry") }) } diff --git a/webapp/channels/src/components/apps_form/apps_form_component.test.tsx b/webapp/channels/src/components/apps_form/apps_form_component.test.tsx index f352bf78df04..e799a4221c2f 100644 --- a/webapp/channels/src/components/apps_form/apps_form_component.test.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_component.test.tsx @@ -1348,9 +1348,11 @@ describe('AppsFormComponent', () => { type: 'datetime', is_required: true, label: 'Meeting Time', - time_interval: 30, - min_date: 'today', - max_date: '+30d', + datetime_config: { + time_interval: 30, + min_date: 'today', + max_date: '+30d', + }, }, ], }; @@ -1374,8 +1376,10 @@ describe('AppsFormComponent', () => { { name: 'invalid_field', type: 'datetime', - time_interval: -1, // Invalid interval - min_date: 'invalid-date', // Invalid date format + datetime_config: { + time_interval: -1, // Invalid interval + min_date: 'invalid-date', // Invalid date format + }, label: 'Invalid Field', }, ], @@ -1429,7 +1433,7 @@ describe('AppsFormComponent', () => { { name: 'valid_datetime', type: 'datetime', - time_interval: interval, + datetime_config: {time_interval: interval}, label: `DateTime with ${interval}min interval`, }, ], @@ -1456,7 +1460,7 @@ describe('AppsFormComponent', () => { { name: 'invalid_datetime', type: 'datetime', - time_interval: interval, + datetime_config: {time_interval: interval}, label: `DateTime with ${interval}min interval`, }, ], @@ -1494,7 +1498,7 @@ describe('AppsFormComponent', () => { { name: 'out_of_range_datetime', type: 'datetime', - time_interval: interval, + datetime_config: {time_interval: interval}, label: `DateTime with ${interval}min interval`, }, ], @@ -1532,7 +1536,7 @@ describe('AppsFormComponent', () => { { name: 'non_numeric_datetime', type: 'datetime', - time_interval: interval as any, + datetime_config: {time_interval: interval as any}, label: `DateTime with ${interval} interval`, }, ], @@ -1570,13 +1574,13 @@ describe('AppsFormComponent', () => { { name: 'text_with_interval', type: 'text', - time_interval: 729, // Invalid but should be ignored for text fields + datetime_config: {time_interval: 729}, // Invalid but should be ignored for text fields label: 'Text Field', }, { name: 'date_with_interval', type: 'date', - time_interval: 729, // Invalid but should be ignored for date fields + datetime_config: {time_interval: 729}, // Invalid but should be ignored for date fields label: 'Date Field', }, ], @@ -1594,78 +1598,9 @@ describe('AppsFormComponent', () => { consoleSpy.mockRestore(); }); - - it('should validate min_date and max_date formats for date and datetime fields', () => { - const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - const formWithInvalidDates = { - ...baseProps.form, - fields: [ - { - name: 'invalid_dates', - type: 'datetime', - min_date: 'invalid-date-format', - max_date: '2025/01/01', // Wrong format - label: 'DateTime with Invalid Dates', - }, - ], - }; - - const props = { - ...baseProps, - form: formWithInvalidDates, - }; - - renderWithContext(); - - // Should log warnings for invalid date formats - expect(consoleSpy).toHaveBeenCalledWith( - 'AppForm field validation errors:', - expect.arrayContaining([ - expect.stringContaining('min_date "invalid-date-format" is not a valid date format'), - expect.stringContaining('max_date "2025/01/01" is not a valid date format'), - ]), - ); - - consoleSpy.mockRestore(); - }); - - it('should validate date range when min_date is after max_date', () => { - const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - const formWithInvalidDateRange = { - ...baseProps.form, - fields: [ - { - name: 'invalid_range', - type: 'date', - min_date: '2025-12-31', - max_date: '2025-01-01', // Before min_date - label: 'Date with Invalid Range', - }, - ], - }; - - const props = { - ...baseProps, - form: formWithInvalidDateRange, - }; - - renderWithContext(); - - // Should log warning for invalid date range - expect(consoleSpy).toHaveBeenCalledWith( - 'AppForm field validation errors:', - expect.arrayContaining([ - expect.stringContaining('min_date cannot be after max_date'), - ]), - ); - - consoleSpy.mockRestore(); - }); }); - describe('DateTime Field Validation - datetime_config precedence', () => { + describe('DateTime Field Validation - datetime_config', () => { it('should validate invalid datetime_config.time_interval', () => { const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); @@ -1753,75 +1688,5 @@ describe('AppsFormComponent', () => { consoleSpy.mockRestore(); }); - - it('should use datetime_config values over legacy fields for validation', () => { - const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - // Legacy values are valid, datetime_config values are invalid. - // If precedence works, validation should flag the datetime_config values. - const form = { - ...baseProps.form, - fields: [ - { - name: 'precedence_test', - type: 'datetime', - time_interval: 30, // Valid legacy - min_date: '2025-01-01', // Valid legacy - max_date: '2025-12-31', // Valid legacy - datetime_config: { - time_interval: 729, // Invalid - min_date: 'not-a-date', // Invalid - max_date: '2025/13/45', // Invalid - }, - label: 'Precedence Test', - }, - ], - }; - - renderWithContext(); - - expect(consoleSpy).toHaveBeenCalledWith( - 'AppForm field validation errors:', - expect.arrayContaining([ - expect.stringContaining('time_interval must be a divisor of 1440 (24 hours * 60 minutes) to create valid time intervals, got 729'), - expect.stringContaining('min_date "not-a-date" is not a valid date format'), - ]), - ); - - // Legacy values are valid, so they should NOT appear in error messages. - const errorCalls = consoleSpy.mock.calls.flat().flat(); - const errorStr = JSON.stringify(errorCalls); - expect(errorStr).not.toContain('got 30'); - expect(errorStr).not.toContain('"2025-01-01"'); - - consoleSpy.mockRestore(); - }); - - it('should fall back to legacy fields when datetime_config is absent', () => { - const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - const form = { - ...baseProps.form, - fields: [ - { - name: 'legacy_only', - type: 'datetime', - time_interval: 729, // Invalid legacy - label: 'Legacy Only', - }, - ], - }; - - renderWithContext(); - - expect(consoleSpy).toHaveBeenCalledWith( - 'AppForm field validation errors:', - expect.arrayContaining([ - expect.stringContaining('time_interval must be a divisor of 1440 (24 hours * 60 minutes) to create valid time intervals, got 729'), - ]), - ); - - consoleSpy.mockRestore(); - }); }); }); 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 1baf09b682f1..9448a53e639d 100644 --- a/webapp/channels/src/components/apps_form/apps_form_component.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_component.tsx @@ -93,10 +93,10 @@ const validateDateFieldValue = (fieldName: string, valueType: string, value: str const validateAppField = (field: AppField): string[] => { const errors: string[] = []; - // Resolve effective datetime values (datetime_config takes precedence over deprecated top-level fields) - const effectiveTimeInterval = field.datetime_config?.time_interval ?? field.time_interval; - const effectiveMinDate = field.datetime_config?.min_date ?? field.min_date; - const effectiveMaxDate = field.datetime_config?.max_date ?? field.max_date; + // Resolve effective datetime values + const effectiveTimeInterval = field.datetime_config?.time_interval; + const effectiveMinDate = field.datetime_config?.min_date; + const effectiveMaxDate = field.datetime_config?.max_date; // Validate time_interval for datetime fields (no mutation) if (field.type === AppFieldTypes.DATETIME && effectiveTimeInterval !== undefined) { @@ -167,10 +167,10 @@ const getSafeDateValue = (dateString: string): string => { const createSanitizedField = (field: AppField): AppField => { const sanitized = {...field}; - // Resolve effective datetime values (datetime_config takes precedence over deprecated top-level fields) - const effectiveInterval = field.datetime_config?.time_interval ?? field.time_interval; - const effectiveMin = field.datetime_config?.min_date ?? field.min_date; - const effectiveMax = field.datetime_config?.max_date ?? field.max_date; + // Resolve effective datetime values + const effectiveInterval = field.datetime_config?.time_interval; + const effectiveMin = field.datetime_config?.min_date; + const effectiveMax = field.datetime_config?.max_date; // Sanitize time_interval for datetime fields if (field.type === AppFieldTypes.DATETIME && effectiveInterval !== undefined) { @@ -179,7 +179,6 @@ const createSanitizedField = (field: AppField): AppField => { if (sanitized.datetime_config) { sanitized.datetime_config = {...sanitized.datetime_config, time_interval: sanitizedInterval}; } - sanitized.time_interval = sanitizedInterval; } // Sanitize date values for date fields only — datetime fields need the full pattern preserved @@ -189,14 +188,12 @@ const createSanitizedField = (field: AppField): AppField => { if (sanitized.datetime_config) { sanitized.datetime_config = {...sanitized.datetime_config, min_date: safeMin}; } - sanitized.min_date = safeMin; } if (effectiveMax) { const safeMax = getSafeDateValue(effectiveMax); if (sanitized.datetime_config) { sanitized.datetime_config = {...sanitized.datetime_config, max_date: safeMax}; } - sanitized.max_date = safeMax; } if (field.type === AppFieldTypes.DATE && field.value && typeof field.value === 'string') { sanitized.value = getSafeDateValue(field.value); @@ -236,8 +233,8 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => { // Set default to current time for required datetime fields const currentTime = timezone ? moment.tz(timezone) : moment(); - // Use sanitized time_interval (guaranteed to be valid; datetime_config takes precedence) - const timePickerInterval = field.datetime_config?.time_interval ?? field.time_interval ?? DEFAULT_TIME_INTERVAL_MINUTES; + // Use sanitized time_interval (guaranteed to be valid) + const timePickerInterval = field.datetime_config?.time_interval ?? DEFAULT_TIME_INTERVAL_MINUTES; // Round up to next time interval const minutesMod = currentTime.minutes() % timePickerInterval; @@ -245,9 +242,9 @@ const initFormValues = (form: AppForm, timezone?: string): AppFormValues => { currentTime.clone().seconds(0).milliseconds(0) : currentTime.clone().add(timePickerInterval - minutesMod, 'minutes').seconds(0).milliseconds(0); - // Clamp default to min_date/max_date bounds (datetime_config takes precedence) - const effectiveMin = field.datetime_config?.min_date ?? field.min_date; - const effectiveMax = field.datetime_config?.max_date ?? field.max_date; + // Clamp default to min_date/max_date bounds + const effectiveMin = field.datetime_config?.min_date; + const effectiveMax = field.datetime_config?.max_date; const minMoment = effectiveMin ? stringToMoment(effectiveMin, timezone) : null; const maxMoment = effectiveMax ? stringToMoment(effectiveMax, timezone) : null; if (minMoment && defaultMoment.isBefore(minMoment)) { @@ -857,9 +854,6 @@ function fieldsAsElements(fields?: AppField[]): DialogElement[] { subtype: f.subtype, optional: !f.is_required, datetime_config: f.datetime_config, - min_date: f.datetime_config?.min_date ?? f.min_date, - max_date: f.datetime_config?.max_date ?? f.max_date, - time_interval: f.datetime_config?.time_interval ?? f.time_interval, })) as DialogElement[]; } diff --git a/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.test.tsx b/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.test.tsx index 0dfad212b619..31e2d72ffaa8 100644 --- a/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.test.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.test.tsx @@ -114,8 +114,10 @@ describe('AppsFormDateField', () => { it('should render without errors even when date is outside range (validation is centralized)', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-10', - max_date: '2025-01-20', + datetime_config: { + min_date: '2025-01-10', + max_date: '2025-01-20', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-01'}); @@ -126,8 +128,10 @@ describe('AppsFormDateField', () => { it('should not show error for valid date within range', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-01', - max_date: '2025-01-31', + datetime_config: { + min_date: '2025-01-01', + max_date: '2025-01-31', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-15'}); expect(screen.queryByText(/error/i)).not.toBeInTheDocument(); @@ -145,8 +149,10 @@ describe('AppsFormDateField', () => { it('should handle date range constraints', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-01', - max_date: '2025-01-31', + datetime_config: { + min_date: '2025-01-01', + max_date: '2025-01-31', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-15'}); diff --git a/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.tsx b/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.tsx index 33880355aedf..22ed72ec3226 100644 --- a/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_date_field/apps_form_date_field.tsx @@ -58,9 +58,9 @@ const AppsFormDateField: React.FC = ({ setIsInteracting?.(isOpen); }, [setIsInteracting]); - // Resolve effective min/max dates (datetime_config takes precedence over deprecated top-level fields) - const effectiveMinDate = field.datetime_config?.min_date ?? field.min_date; - const effectiveMaxDate = field.datetime_config?.max_date ?? field.max_date; + // Resolve effective min/max dates + const effectiveMinDate = field.datetime_config?.min_date; + const effectiveMaxDate = field.datetime_config?.max_date; const disabledDays = useMemo(() => { const disabled = []; diff --git a/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.test.tsx b/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.test.tsx index 86e706b1fb6b..c12d6cdcdcb0 100644 --- a/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.test.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.test.tsx @@ -119,7 +119,7 @@ describe('AppsFormDateTimeField', () => { }); it('should use custom time_interval', () => { - const fieldWithInterval = {...defaultField, time_interval: 30}; + const fieldWithInterval = {...defaultField, datetime_config: {time_interval: 30}}; renderComponent({field: fieldWithInterval, value: '2025-01-15T14:30:00Z'}); // The time_interval is passed to DateTimeInput component @@ -138,8 +138,10 @@ describe('AppsFormDateTimeField', () => { it('should render without errors even when datetime is outside range (validation is centralized)', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-10', - max_date: '2025-01-20', + datetime_config: { + min_date: '2025-01-10', + max_date: '2025-01-20', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-01T14:30:00Z'}); @@ -150,8 +152,10 @@ describe('AppsFormDateTimeField', () => { it('should not show error for valid datetime within range', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-01', - max_date: '2025-01-31', + datetime_config: { + min_date: '2025-01-01', + max_date: '2025-01-31', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-15T14:30:00Z'}); expect(screen.queryByText(/error/i)).not.toBeInTheDocument(); @@ -160,8 +164,10 @@ describe('AppsFormDateTimeField', () => { it('should handle datetime range constraints', () => { const fieldWithRange = { ...defaultField, - min_date: '2025-01-01', - max_date: '2025-01-31', + datetime_config: { + min_date: '2025-01-01', + max_date: '2025-01-31', + }, }; renderComponent({field: fieldWithRange, value: '2025-01-15T14:30:00Z'}); @@ -190,7 +196,7 @@ describe('AppsFormDateTimeField', () => { }); it('should restrict past dates when min_date is today or future', () => { - const fieldWithMinDate = {...defaultField, min_date: 'today'}; + const fieldWithMinDate = {...defaultField, datetime_config: {min_date: 'today'}}; renderComponent({field: fieldWithMinDate, value: '2025-01-15T14:30:00Z'}); // DateTimeInput should receive allowPastDates=false @@ -198,7 +204,7 @@ describe('AppsFormDateTimeField', () => { }); it('should allow past dates when min_date is in the past', () => { - const fieldWithMinDate = {...defaultField, min_date: '-5d'}; + const fieldWithMinDate = {...defaultField, datetime_config: {min_date: '-5d'}}; renderComponent({field: fieldWithMinDate, value: '2025-01-15T14:30:00Z'}); // DateTimeInput should receive allowPastDates=true @@ -206,28 +212,16 @@ describe('AppsFormDateTimeField', () => { }); }); - describe('manualTimeEntry resolution (OR precedence)', () => { - it('is false when neither field is set', () => { + describe('manualTimeEntry resolution', () => { + it('is false when not set', () => { renderComponent(); expect(screen.getByTestId('datetime-input')).toHaveAttribute('data-manual-time-entry', 'false'); }); - it('is true when only the new manual_time_entry is set', () => { + it('is true when manual_time_entry is set', () => { const field = {...defaultField, datetime_config: {manual_time_entry: true}}; renderComponent({field}); expect(screen.getByTestId('datetime-input')).toHaveAttribute('data-manual-time-entry', 'true'); }); - - it('is true when only the deprecated allow_manual_time_entry is set', () => { - const field = {...defaultField, datetime_config: {allow_manual_time_entry: true}}; - renderComponent({field}); - expect(screen.getByTestId('datetime-input')).toHaveAttribute('data-manual-time-entry', 'true'); - }); - - it('is true when either field is true (OR semantics)', () => { - const field = {...defaultField, datetime_config: {manual_time_entry: false, allow_manual_time_entry: true}}; - renderComponent({field}); - expect(screen.getByTestId('datetime-input')).toHaveAttribute('data-manual-time-entry', 'true'); - }); }); }); diff --git a/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.tsx b/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.tsx index 9dfc196cd4a5..5c77eef45178 100644 --- a/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.tsx +++ b/webapp/channels/src/components/apps_form/apps_form_datetime_field/apps_form_datetime_field.tsx @@ -49,15 +49,11 @@ const AppsFormDateTimeField: React.FC = ({ }) => { const userTimezone = useSelector(getCurrentTimezone); - // Resolve datetime config (datetime_config takes precedence over deprecated top-level fields) + // Resolve datetime config const locationTimezone = field.datetime_config?.location_timezone; - const timePickerInterval = field.datetime_config?.time_interval ?? field.time_interval ?? DEFAULT_TIME_INTERVAL_MINUTES; + const timePickerInterval = field.datetime_config?.time_interval ?? DEFAULT_TIME_INTERVAL_MINUTES; - // manual_time_entry supersedes the deprecated allow_manual_time_entry. Either enabling - // it turns it on (booleans can't distinguish explicit-false from not-set across the wire). - // The OR covers direct Apps Framework bindings that may still carry the deprecated key; - // dialog-sourced AppFields are pre-normalized by dialog_conversion and only carry manual_time_entry. - const manualTimeEntry = Boolean(field.datetime_config?.manual_time_entry) || Boolean(field.datetime_config?.allow_manual_time_entry); + const manualTimeEntry = Boolean(field.datetime_config?.manual_time_entry); // Use location_timezone if specified, otherwise fall back to user's timezone const timezone = locationTimezone || userTimezone; @@ -87,9 +83,9 @@ const AppsFormDateTimeField: React.FC = ({ onChange(field.name, newValue); }, [field.name, onChange]); - // Resolve effective min/max dates (datetime_config takes precedence over deprecated top-level fields) - const effectiveMinDate = field.datetime_config?.min_date ?? field.min_date; - const effectiveMaxDate = field.datetime_config?.max_date ?? field.max_date; + // Resolve effective min/max dates + const effectiveMinDate = field.datetime_config?.min_date; + const effectiveMaxDate = field.datetime_config?.max_date; const {minDateTime, allowPastDates} = useMemo(() => { if (!effectiveMinDate) { diff --git a/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.test.ts b/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.test.ts index 50f01fd88793..f35fb109bd6f 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.test.ts @@ -149,7 +149,9 @@ describe('integration utils', () => { it('should return error when datetime is before min_date', () => { const element = TestHelper.getDialogElementMock({ type: 'datetime', - min_date: '2025-06-01T00:00:00Z', + datetime_config: { + min_date: '2025-06-01T00:00:00Z', + }, }); const error = checkDialogElementForError(element, '2025-05-15T12:00:00Z'); @@ -159,7 +161,9 @@ describe('integration utils', () => { it('should return error when datetime is after max_date', () => { const element = TestHelper.getDialogElementMock({ type: 'datetime', - max_date: '2025-06-01T00:00:00Z', + datetime_config: { + max_date: '2025-06-01T00:00:00Z', + }, }); const error = checkDialogElementForError(element, '2025-06-15T12:00:00Z'); @@ -169,8 +173,10 @@ describe('integration utils', () => { it('should return null when datetime is within min_date and max_date bounds', () => { const element = TestHelper.getDialogElementMock({ type: 'datetime', - min_date: '2025-01-01T00:00:00Z', - max_date: '2025-12-31T23:59:59Z', + datetime_config: { + min_date: '2025-01-01T00:00:00Z', + max_date: '2025-12-31T23:59:59Z', + }, }); expect(checkDialogElementForError(element, '2025-06-15T12:00:00Z')).toBeNull(); @@ -184,51 +190,14 @@ describe('integration utils', () => { it('should handle unresolvable min_date/max_date gracefully', () => { const element = TestHelper.getDialogElementMock({ type: 'datetime', - min_date: 'not-a-valid-format', - max_date: 'also-invalid', - }); - - // Should skip range check (resolveBoundToDate returns null) and pass - expect(checkDialogElementForError(element, '2025-06-15T12:00:00Z')).toBeNull(); - }); - - it('should use datetime_config.min_date over legacy min_date', () => { - const element = TestHelper.getDialogElementMock({ - type: 'datetime', - min_date: '2020-01-01T00:00:00Z', datetime_config: { - min_date: '2025-06-01T00:00:00Z', + min_date: 'not-a-valid-format', + max_date: 'also-invalid', }, }); - // datetime_config.min_date (2025-06-01) takes precedence over legacy (2020-01-01) - const error = checkDialogElementForError(element, '2025-05-15T12:00:00Z'); - expect(error?.id).toBe('interactive_dialog.error.before_min_date'); - }); - - it('should use datetime_config.max_date over legacy max_date', () => { - const element = TestHelper.getDialogElementMock({ - type: 'datetime', - max_date: '2030-12-31T23:59:59Z', - datetime_config: { - max_date: '2025-06-01T00:00:00Z', - }, - }); - - // datetime_config.max_date (2025-06-01) takes precedence over legacy (2030-12-31) - const error = checkDialogElementForError(element, '2025-06-15T12:00:00Z'); - expect(error?.id).toBe('interactive_dialog.error.after_max_date'); - }); - - it('should fall back to legacy fields when datetime_config not set', () => { - const element = TestHelper.getDialogElementMock({ - type: 'datetime', - min_date: '2025-06-01T00:00:00Z', - max_date: '2025-12-31T23:59:59Z', - }); - + // Should skip range check (resolveBoundToDate returns null) and pass expect(checkDialogElementForError(element, '2025-06-15T12:00:00Z')).toBeNull(); - expect(checkDialogElementForError(element, '2025-05-01T12:00:00Z')?.id).toBe('interactive_dialog.error.before_min_date'); }); }); }); diff --git a/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.ts b/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.ts index ef11b165873b..ce4705d1bf1f 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/utils/integration_utils.ts @@ -83,9 +83,9 @@ function validateDateTimeValue(value: string, elem: DialogElement): DialogError }); } - // Range validation against min_date / max_date (datetime_config takes precedence over legacy fields) - const effectiveMinDate = elem.datetime_config?.min_date ?? elem.min_date; - const effectiveMaxDate = elem.datetime_config?.max_date ?? elem.max_date; + // Range validation against min_date / max_date + const effectiveMinDate = elem.datetime_config?.min_date; + const effectiveMaxDate = elem.datetime_config?.max_date; if (effectiveMinDate) { const minDate = resolveBoundToDate(effectiveMinDate); if (minDate && parsedDate < minDate) { diff --git a/webapp/channels/src/utils/apps_type_guard.test.ts b/webapp/channels/src/utils/apps_type_guard.test.ts index 4cfcadebb2f9..9e61feef0b40 100644 --- a/webapp/channels/src/utils/apps_type_guard.test.ts +++ b/webapp/channels/src/utils/apps_type_guard.test.ts @@ -158,35 +158,6 @@ describe('isAppBinding — isAppField datetime_config validation', () => { }); }); - describe('datetime_config.allow_manual_time_entry (deprecated)', () => { - test('accepts boolean', () => { - const binding = bindingWithField({ - ...baseField, - datetime_config: {allow_manual_time_entry: true}, - }); - expect(isAppBinding(binding)).toBe(true); - }); - - test('rejects non-boolean value', () => { - const binding = bindingWithField({ - ...baseField, - datetime_config: {allow_manual_time_entry: 1}, - }); - expect(isAppBinding(binding)).toBe(false); - }); - - test('accepts both new and deprecated fields set simultaneously', () => { - const binding = bindingWithField({ - ...baseField, - datetime_config: { - manual_time_entry: true, - allow_manual_time_entry: false, - }, - }); - expect(isAppBinding(binding)).toBe(true); - }); - }); - describe('datetime_config shape', () => { test('accepts empty object', () => { const binding = bindingWithField({ @@ -212,24 +183,4 @@ describe('isAppBinding — isAppField datetime_config validation', () => { expect(isAppBinding(binding)).toBe(false); }); }); - - describe('interaction with deprecated top-level fields', () => { - test('accepts both datetime_config and legacy top-level min_date when valid', () => { - const binding = bindingWithField({ - ...baseField, - min_date: '2024-01-01', - datetime_config: {min_date: '2025-01-15'}, - }); - expect(isAppBinding(binding)).toBe(true); - }); - - test('rejects when datetime_config.min_date is invalid even if legacy min_date is valid', () => { - const binding = bindingWithField({ - ...baseField, - min_date: '2025-01-15', - datetime_config: {min_date: 'not-a-date'}, - }); - expect(isAppBinding(binding)).toBe(false); - }); - }); }); diff --git a/webapp/channels/src/utils/dialog_conversion.test.ts b/webapp/channels/src/utils/dialog_conversion.test.ts index c773f1e5c796..d62e6de432dd 100644 --- a/webapp/channels/src/utils/dialog_conversion.test.ts +++ b/webapp/channels/src/utils/dialog_conversion.test.ts @@ -1755,81 +1755,17 @@ describe('dialog_conversion', () => { }); describe('convertDialogToAppForm with date/datetime fields', () => { - it('should convert date field with min_date and max_date', () => { - const elements: DialogElement[] = [ - { - name: 'event_date', - type: 'date', - display_name: 'Event Date', - min_date: '2025-01-01', - max_date: '2025-12-31', - optional: false, - } as DialogElement, - ]; - - const {form} = convertDialogToAppForm( - elements, - 'Test Form', - undefined, - undefined, - undefined, - '', - '', - legacyOptions, - ); - - expect(form.fields).toHaveLength(1); - expect(form.fields?.[0]).toMatchObject({ - name: 'event_date', - type: 'date', - label: 'Event Date', - min_date: '2025-01-01', - max_date: '2025-12-31', - is_required: true, - }); - }); - - it('should convert datetime field with time_interval', () => { - const elements: DialogElement[] = [ - { - name: 'meeting_time', - type: 'datetime', - display_name: 'Meeting Time', - time_interval: 30, - optional: true, - } as DialogElement, - ]; - - const {form} = convertDialogToAppForm( - elements, - 'Test Form', - undefined, - undefined, - undefined, - '', - '', - legacyOptions, - ); - - expect(form.fields).toHaveLength(1); - expect(form.fields?.[0]).toMatchObject({ - name: 'meeting_time', - type: 'datetime', - label: 'Meeting Time', - time_interval: 30, - is_required: false, - }); - }); - - it('should convert datetime field with all date properties', () => { + it('should convert datetime field with all datetime_config properties', () => { const elements: DialogElement[] = [ { name: 'full_datetime', type: 'datetime', display_name: 'Full DateTime', - min_date: 'today', - max_date: '+30d', - time_interval: 15, + datetime_config: { + min_date: 'today', + max_date: '+30d', + time_interval: 15, + }, optional: false, } as DialogElement, ]; @@ -1850,10 +1786,12 @@ describe('dialog_conversion', () => { name: 'full_datetime', type: 'datetime', label: 'Full DateTime', + is_required: true, + }); + expect(form.fields?.[0]?.datetime_config).toMatchObject({ min_date: 'today', max_date: '+30d', time_interval: 15, - is_required: true, }); }); @@ -1887,8 +1825,6 @@ describe('dialog_conversion', () => { name: 'event_date', type: 'date', label: 'Event Date', - min_date: '2025-01-01', - max_date: '2025-12-31', is_required: true, }); expect(form.fields?.[0]?.datetime_config).toMatchObject({ @@ -1926,41 +1862,12 @@ describe('dialog_conversion', () => { name: 'meeting_time', type: 'datetime', label: 'Meeting Time', - time_interval: 30, is_required: false, }); expect(form.fields?.[0]?.datetime_config?.time_interval).toBe(30); }); - it('normalizes deprecated allow_manual_time_entry into manual_time_entry', () => { - const elements: DialogElement[] = [ - { - name: 'meeting_time', - type: 'datetime', - display_name: 'Meeting Time', - datetime_config: { - allow_manual_time_entry: true, - }, - optional: false, - } as DialogElement, - ]; - - const {form} = convertDialogToAppForm( - elements, - 'Test Form', - undefined, - undefined, - undefined, - '', - '', - legacyOptions, - ); - - expect(form.fields?.[0]?.datetime_config?.manual_time_entry).toBe(true); - expect(form.fields?.[0]?.datetime_config?.allow_manual_time_entry).toBeUndefined(); - }); - - it('preserves manual_time_entry when set directly', () => { + it('preserves manual_time_entry when set', () => { const elements: DialogElement[] = [ { name: 'meeting_time', @@ -1985,10 +1892,9 @@ describe('dialog_conversion', () => { ); expect(form.fields?.[0]?.datetime_config?.manual_time_entry).toBe(true); - expect(form.fields?.[0]?.datetime_config?.allow_manual_time_entry).toBeUndefined(); }); - it('omits manual_time_entry when neither source is true', () => { + it('omits manual_time_entry when not set', () => { const elements: DialogElement[] = [ { name: 'meeting_time', @@ -2013,40 +1919,6 @@ describe('dialog_conversion', () => { ); expect(form.fields?.[0]?.datetime_config?.manual_time_entry).toBeUndefined(); - expect(form.fields?.[0]?.datetime_config?.allow_manual_time_entry).toBeUndefined(); - }); - - it('datetime_config should take precedence over legacy fields', () => { - const elements: DialogElement[] = [ - { - name: 'event_date', - type: 'date', - display_name: 'Event Date', - min_date: '2024-01-01', - max_date: '2024-12-31', - datetime_config: { - min_date: '2025-06-01', - max_date: '2025-12-31', - }, - optional: false, - } as DialogElement, - ]; - - const {form} = convertDialogToAppForm( - elements, - 'Test Form', - undefined, - undefined, - undefined, - '', - '', - legacyOptions, - ); - - expect(form.fields?.[0]?.min_date).toBe('2025-06-01'); - expect(form.fields?.[0]?.max_date).toBe('2025-12-31'); - expect(form.fields?.[0]?.datetime_config?.min_date).toBe('2025-06-01'); - expect(form.fields?.[0]?.datetime_config?.max_date).toBe('2025-12-31'); }); it('should not add datetime-specific properties to date fields', () => { @@ -2055,7 +1927,9 @@ describe('dialog_conversion', () => { name: 'simple_date', type: 'date', display_name: 'Simple Date', - time_interval: 30, // Should be ignored for date fields + datetime_config: { + time_interval: 30, // Should be ignored for date fields + }, optional: false, } as DialogElement, ]; @@ -2071,9 +1945,7 @@ describe('dialog_conversion', () => { legacyOptions, ); - expect(form.fields?.[0]).not.toHaveProperty('time_interval'); - expect(form.fields?.[0]).not.toHaveProperty('min_date'); - expect(form.fields?.[0]).not.toHaveProperty('max_date'); + expect(form.fields?.[0]?.datetime_config?.time_interval).toBeUndefined(); }); }); diff --git a/webapp/channels/src/utils/dialog_conversion.ts b/webapp/channels/src/utils/dialog_conversion.ts index 7c911018e8c3..581a5d3d2555 100644 --- a/webapp/channels/src/utils/dialog_conversion.ts +++ b/webapp/channels/src/utils/dialog_conversion.ts @@ -479,20 +479,16 @@ export function convertElement(element: DialogElement, options: ConversionOption // Add date/datetime specific properties if (element.type === DialogElementTypes.DATE || element.type === DialogElementTypes.DATETIME) { - // Merge datetime_config over deprecated top-level fields (datetime_config takes precedence) - const minDate = element.datetime_config?.min_date ?? element.min_date; - const maxDate = element.datetime_config?.max_date ?? element.max_date; - const timeInterval = element.datetime_config?.time_interval ?? element.time_interval; + const minDate = element.datetime_config?.min_date; + const maxDate = element.datetime_config?.max_date; + const timeInterval = element.datetime_config?.time_interval; const mergedConfig: DateTimeConfig = {}; if (element.datetime_config?.location_timezone) { mergedConfig.location_timezone = element.datetime_config.location_timezone; } - // manual_time_entry supersedes the deprecated allow_manual_time_entry. OR-merge - // the two sources into a single normalized key so downstream consumers don't - // need to repeat the precedence logic. - if (element.datetime_config?.manual_time_entry || element.datetime_config?.allow_manual_time_entry) { + if (element.datetime_config?.manual_time_entry) { mergedConfig.manual_time_entry = true; } if (minDate !== undefined) { @@ -509,17 +505,6 @@ export function convertElement(element: DialogElement, options: ConversionOption appField.datetime_config = mergedConfig; } - // Also set deprecated top-level fields for backward compatibility with consumers - if (minDate !== undefined) { - appField.min_date = String(minDate); - } - if (maxDate !== undefined) { - appField.max_date = String(maxDate); - } - if (timeInterval !== undefined && element.type === DialogElementTypes.DATETIME) { - appField.time_interval = Number(timeInterval); - } - if (element.refresh !== undefined) { appField.refresh = element.refresh; } diff --git a/webapp/platform/types/src/apps.ts b/webapp/platform/types/src/apps.ts index 6a1327fd47a8..22424736f250 100644 --- a/webapp/platform/types/src/apps.ts +++ b/webapp/platform/types/src/apps.ts @@ -445,9 +445,6 @@ export type DateTimeConfig = { time_interval?: number; // Minutes between time options (default: 60) location_timezone?: string; // IANA timezone for display (e.g., "America/Denver", "Asia/Tokyo") manual_time_entry?: boolean; // Allow text entry for time - - /** @deprecated Use manual_time_entry instead. Kept for backward compatibility. */ - allow_manual_time_entry?: boolean; }; // This should go in mattermost-redux @@ -487,15 +484,6 @@ export type AppField = { // Date/datetime configuration datetime_config?: DateTimeConfig; - /** @deprecated Use datetime_config.min_date instead. Kept for backward compatibility. */ - min_date?: string; - - /** @deprecated Use datetime_config.max_date instead. Kept for backward compatibility. */ - max_date?: string; - - /** @deprecated Use datetime_config.time_interval instead. Kept for backward compatibility. */ - time_interval?: number; - // Action button props action_button_url?: string; action_button_context?: Record; @@ -619,34 +607,6 @@ function isAppField(v: unknown): v is AppField { if (field.datetime_config.manual_time_entry !== undefined && typeof field.datetime_config.manual_time_entry !== 'boolean') { return false; } - if (field.datetime_config.allow_manual_time_entry !== undefined && typeof field.datetime_config.allow_manual_time_entry !== 'boolean') { - return false; - } - } - - // Validate deprecated top-level fields (kept for backward compatibility) - if (field.min_date !== undefined) { - if (typeof field.min_date !== 'string') { - return false; - } - - if (!isValidDateString(field.min_date)) { - return false; - } - } - - if (field.max_date !== undefined) { - if (typeof field.max_date !== 'string') { - return false; - } - - if (!isValidDateString(field.max_date)) { - return false; - } - } - - if (field.time_interval !== undefined && typeof field.time_interval !== 'number') { - return false; } // Validate action button fields diff --git a/webapp/platform/types/src/integrations.ts b/webapp/platform/types/src/integrations.ts index e4d43612a412..a38425654a7a 100644 --- a/webapp/platform/types/src/integrations.ts +++ b/webapp/platform/types/src/integrations.ts @@ -207,20 +207,8 @@ export type DialogElement = { time_interval?: number; location_timezone?: string; manual_time_entry?: boolean; - - /** @deprecated Use manual_time_entry instead. Kept for backward compatibility. */ - allow_manual_time_entry?: boolean; }; - /** @deprecated Use datetime_config.min_date instead. Kept for backward compatibility. */ - min_date?: string; - - /** @deprecated Use datetime_config.max_date instead. Kept for backward compatibility. */ - max_date?: string; - - /** @deprecated Use datetime_config.time_interval instead. Kept for backward compatibility. */ - time_interval?: number; - // Action button configuration (type "action_button") action_button?: { url: string; From 0bff02c8148abbea87f260c28839ae2ab4da3aee Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 18 Aug 2026 16:32:00 -0300 Subject: [PATCH 5/5] Graduate user typing settings to Site Configuration > Posts (#38023) * [MM-57814][MM-57815] Graduate user typing settings to Site Configuration > Posts Move ServiceSettings.EnableUserTypingMessages and ServiceSettings.TimeBetweenUserTypingUpdatesMilliseconds out of System Console > Experimental > Features into the Performance & Limits section of System Console > Site Configuration > Posts, and reclassify their access tags from experimental_features to site_posts (preserving write_restrictable and cloud_restrictable). The two settings stay adjacent, and the timeout remains disabled while typing messages are off. The timeout label now states its unit, since "User Typing Timeout" alone did not convey milliseconds. The i18n ids move from admin.experimental.* to the Posts page's admin.posts.* convention; the "E.g.: 5000" placeholder previously shared with the experimental user status and profile fetching poll interval is now defined once per setting. No config keys, defaults, or runtime behavior change. * [MM-57814][MM-57815] Assert user typing settings are searchable under Posts Searching the System Console for "typing" now also matches Site Configuration > Posts, guarding the new location of the user typing settings. Experimental Features still matches on unrelated help text about typing a tilde to trigger channel autocomplete. * [MM-57814][MM-57815] Move user typing settings docs out of Experimental Document "Enable user typing messages" and "User typing timeout" in the Posts section of the site configuration settings guide, and drop them from the experimental configuration settings guide. --- .../experimental-configuration-settings.mdx | 40 ---------------- .../configure/site-configuration-settings.mdx | 46 +++++++++++++++++++ server/public/model/config.go | 4 +- .../admin_console/admin_definition.tsx | 42 ++++++++--------- webapp/channels/src/i18n/en.json | 11 +++-- .../src/utils/admin_console_index.test.tsx | 4 ++ 6 files changed, 79 insertions(+), 68 deletions(-) diff --git a/docs/main/administration-guide/configure/experimental-configuration-settings.mdx b/docs/main/administration-guide/configure/experimental-configuration-settings.mdx index 7a92c5ec81e5..92cf91175377 100644 --- a/docs/main/administration-guide/configure/experimental-configuration-settings.mdx +++ b/docs/main/administration-guide/configure/experimental-configuration-settings.mdx @@ -361,46 +361,6 @@ Set a default theme that applies to all new users on the system. -### Enable user typing messages - -This setting determines whether "user is typing..." messages are displayed below the message box when using Mattermost in a web browser or the desktop app. - - --- - - - - - -
This feature's config.json setting is "EnableUserTypingMessages": true with options true and false.
- - - -Disabling this experimental configuration setting in larger deployments may improve server performance in the following areas: - -- Reduced Server Load: Typing events generate additional websocket traffic. Disabling them can reduce the amount of data that needs to be handled by the server, improving the overall response time and decreasing server load. -- Lower Network Traffic: When typing events are enabled, every keystroke generates a network event. This can lead to a significant amount of network traffic, particularly in busy channels. Disabling these events reduces the amount of information transmitted over the network. -- Client Performance: On the client side, processing typing events requires resources. By not having to handle these events, the client can be more responsive and use less memory and CPU. - - - -### User typing timeout - -This setting defines how frequently "user is typing..." messages are updated, measured in milliseconds. - - --- - - - - - -
This feature's config.json setting is "TimeBetweenUserTypingUpdatesMilliseconds": 5000 with numerical input.
- ### User's status and profile fetching poll interval This setting configures the number of milliseconds to wait between fetching user statuses and profiles periodically. Set to `0` to disable. diff --git a/docs/main/administration-guide/configure/site-configuration-settings.mdx b/docs/main/administration-guide/configure/site-configuration-settings.mdx index 0a89cc5a5d23..3df838f38de4 100644 --- a/docs/main/administration-guide/configure/site-configuration-settings.mdx +++ b/docs/main/administration-guide/configure/site-configuration-settings.mdx @@ -1714,6 +1714,52 @@ While drafts can be very useful for maintaining work continuity, especially in c +### Enable user typing messages + + ++++ + + + + + + +

This setting determines whether "user is typing..." messages are displayed below the message box.

  • true: (Default) "User is typing..." messages are displayed below the message box.
  • false: "User is typing..." messages are not displayed.
  • System Config path: Site Configuration > Posts
  • config.json setting: ServiceSettings > EnableUserTypingMessages > true
  • Environment variable: MM_SERVICESETTINGS_ENABLEUSERTYPINGMESSAGES
+ + + +Disabling this configuration setting in larger deployments may improve server performance in the following areas: + +- Reduced Server Load: Typing events generate additional websocket traffic. Disabling them can reduce the amount of data that needs to be handled by the server, improving the overall response time and decreasing server load. +- Lower Network Traffic: When typing events are enabled, every keystroke generates a network event. This can lead to a significant amount of network traffic, particularly in busy channels. Disabling these events reduces the amount of information transmitted over the network. +- Client Performance: On the client side, processing typing events requires resources. By not having to handle these events, the client can be more responsive and use less memory and CPU. + + + +### User typing timeout + + ++++ + + + + + + +

The number of milliseconds to wait between emitting user typing websocket events, which determines how frequently "user is typing..." messages are updated.

Numerical input in milliseconds. Default is 5000. Minimum is 1000.

  • System Config path: Site Configuration > Posts
  • config.json setting: ServiceSettings > TimeBetweenUserTypingUpdatesMilliseconds > 5000
  • Environment variable: MM_SERVICESETTINGS_TIMEBETWEENUSERTYPINGUPDATESMILLISECONDS
+ + + +This setting only applies when **Enable user typing messages** is set to **true**. + + + ------------------------------------------------------------------------------------------------------------------------ ## Content flagging diff --git a/server/public/model/config.go b/server/public/model/config.go index 629883579465..8d8277da7112 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -435,12 +435,12 @@ type ServiceSettings struct { EnableCustomEmoji *bool `access:"site_emoji"` EnableEmojiPicker *bool `access:"site_emoji"` PostEditTimeLimit *int `access:"user_management_permissions"` - TimeBetweenUserTypingUpdatesMilliseconds *int64 `access:"experimental_features,write_restrictable,cloud_restrictable"` + TimeBetweenUserTypingUpdatesMilliseconds *int64 `access:"site_posts,write_restrictable,cloud_restrictable"` EnableCrossTeamSearch *bool `access:"write_restrictable,cloud_restrictable"` EnablePostSearch *bool `access:"write_restrictable,cloud_restrictable"` EnableFileSearch *bool `access:"write_restrictable"` MinimumHashtagLength *int `access:"environment_database,write_restrictable,cloud_restrictable"` - EnableUserTypingMessages *bool `access:"experimental_features,write_restrictable,cloud_restrictable"` + EnableUserTypingMessages *bool `access:"site_posts,write_restrictable,cloud_restrictable"` EnableChannelViewedMessages *bool `access:"experimental_features,write_restrictable,cloud_restrictable"` EnableUserStatuses *bool `access:"write_restrictable,cloud_restrictable"` ExperimentalEnableAuthenticationTransfer *bool `access:"experimental_features"` diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index 9861b2d3ae69..5e132d6860a7 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/webapp/channels/src/components/admin_console/admin_definition.tsx @@ -3814,6 +3814,26 @@ const AdminDefinition: AdminDefinitionType = { return new ValidationResult(true, ''); }, }, + { + type: 'bool', + key: 'ServiceSettings.EnableUserTypingMessages', + label: defineMessage({id: 'admin.posts.enableUserTypingMessages.title', defaultMessage: 'Enable User Typing Messages:'}), + help_text: defineMessage({id: 'admin.posts.enableUserTypingMessages.desc', defaultMessage: 'This setting determines whether "user is typing..." messages are displayed below the message box. Disabling the setting in larger deployments may improve server performance.'}), + help_text_markdown: false, + isDisabled: it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.SITE.POSTS)), + }, + { + type: 'number', + key: 'ServiceSettings.TimeBetweenUserTypingUpdatesMilliseconds', + label: defineMessage({id: 'admin.posts.timeBetweenUserTypingUpdatesMilliseconds.title', defaultMessage: 'User Typing Timeout (milliseconds):'}), + help_text: defineMessage({id: 'admin.posts.timeBetweenUserTypingUpdatesMilliseconds.desc', defaultMessage: 'The number of milliseconds to wait between emitting user typing websocket events.'}), + help_text_markdown: false, + placeholder: defineMessage({id: 'admin.posts.timeBetweenUserTypingUpdatesMilliseconds.example', defaultMessage: 'E.g.: "5000"'}), + isDisabled: it.any( + it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.SITE.POSTS)), + it.stateIsFalse('ServiceSettings.EnableUserTypingMessages'), + ), + }, ], }, ], @@ -6679,33 +6699,13 @@ const AdminDefinition: AdminDefinitionType = { help_text_markdown: false, isDisabled: it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.EXPERIMENTAL.FEATURES)), }, - { - type: 'bool', - key: 'ServiceSettings.EnableUserTypingMessages', - label: defineMessage({id: 'admin.experimental.enableUserTypingMessages.title', defaultMessage: 'Enable User Typing Messages:'}), - help_text: defineMessage({id: 'admin.experimental.enableUserTypingMessages.desc', defaultMessage: 'This setting determines whether "user is typing..." messages are displayed below the message box. Disabling the setting in larger deployments may improve server performance.'}), - help_text_markdown: false, - isDisabled: it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.EXPERIMENTAL.FEATURES)), - }, - { - type: 'number', - key: 'ServiceSettings.TimeBetweenUserTypingUpdatesMilliseconds', - label: defineMessage({id: 'admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.title', defaultMessage: 'User Typing Timeout:'}), - help_text: defineMessage({id: 'admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.desc', defaultMessage: 'The number of milliseconds to wait between emitting user typing websocket events.'}), - help_text_markdown: false, - placeholder: defineMessage({id: 'admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.example', defaultMessage: 'E.g.: "5000"'}), - isDisabled: it.any( - it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.EXPERIMENTAL.FEATURES)), - it.stateIsFalse('ServiceSettings.EnableUserTypingMessages'), - ), - }, { type: 'number', key: 'ExperimentalSettings.UsersStatusAndProfileFetchingPollIntervalMilliseconds', label: defineMessage({id: 'admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.title', defaultMessage: 'User\'s Status and Profile Fetching Poll Interval:'}), help_text: defineMessage({id: 'admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.desc', defaultMessage: 'The number of milliseconds to wait between fetching user statuses and profiles periodically.'}), help_text_markdown: false, - placeholder: defineMessage({id: 'admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.example', defaultMessage: 'E.g.: "5000"'}), + placeholder: defineMessage({id: 'admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.example', defaultMessage: 'E.g.: "5000"'}), isDisabled: it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.EXPERIMENTAL.FEATURES)), }, { diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 9eb72db20536..671634e64550 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -1338,8 +1338,6 @@ "admin.experimental.enableTutorial.title": "Enable Tutorial:", "admin.experimental.enableUserDeactivation.desc": "When true, users may deactivate their own account from **Settings > Advanced**. If a user deactivates their own account, they will get an email notification confirming they were deactivated. When false, users may not deactivate their own account.", "admin.experimental.enableUserDeactivation.title": "Enable Account Deactivation:", - "admin.experimental.enableUserTypingMessages.desc": "This setting determines whether \"user is typing...\" messages are displayed below the message box. Disabling the setting in larger deployments may improve server performance.", - "admin.experimental.enableUserTypingMessages.title": "Enable User Typing Messages:", "admin.experimental.enableWatermark.desc": "When true, authenticated mobile sessions will display a watermark overlay showing the username, domain, date (YYYY-MM-DD), and time (HH:mm) for data loss prevention (DLP) purposes.", "admin.experimental.enableWatermark.title": "Enable Mobile Watermark:", "admin.experimental.experimentalEnableAuthenticationTransfer.desc": "When true, users can change their sign-in method to any that is enabled on the server, either via their Profile or the APIs. When false, Users cannot change their sign-in method, regardless of which authentication options are enabled.", @@ -1371,12 +1369,10 @@ "admin.experimental.PermittedMoveThreadRoles.title": "Permitted Roles", "admin.experimental.threadAutoFollow.desc": "This setting must be enabled in order to enable Threaded Discussions. When enabled, threads a user starts, participates in, or is mentioned in are automatically followed. A new `Threads` table is added in the database that tracks threads and thread participants, and a `ThreadMembership` table tracks followed threads for each user and the read or unread state of each followed thread. When false, all backend operations to support Threaded Discussions are disabled.", "admin.experimental.threadAutoFollow.title": "Automatically Follow Threads", - "admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.desc": "The number of milliseconds to wait between emitting user typing websocket events.", - "admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.example": "E.g.: \"5000\"", - "admin.experimental.timeBetweenUserTypingUpdatesMilliseconds.title": "User Typing Timeout:", "admin.experimental.useChannelInEmailNotifications.desc": "When true, channel and team name appears in email notification subject lines. Useful for servers using only one team. When false, only team name appears in email notification subject line.", "admin.experimental.useChannelInEmailNotifications.title": "Use Channel Name in Email Notifications:", "admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.desc": "The number of milliseconds to wait between fetching user statuses and profiles periodically.", + "admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.example": "E.g.: \"5000\"", "admin.experimental.UsersStatusAndProfileFetchingPollIntervalMilliseconds.title": "User's Status and Profile Fetching Poll Interval:", "admin.experimental.userStatusAwayTimeout.desc": "This setting defines the number of seconds after which the user's status indicator changes to \"Away\", when they are away from Mattermost.", "admin.experimental.userStatusAwayTimeout.example": "E.g.: \"300\"", @@ -2839,6 +2835,8 @@ "admin.posts.burnOnRead.maximumTTL.7days": "7 days", "admin.posts.burnOnRead.maximumTTL.desc": "Sets the maximum duration that Burn-on-Read messages will be allowed to exist for after they are sent. The message will be deleted after the specified time after it is sent, even if it is not read by all recipients by then.", "admin.posts.burnOnRead.maximumTTL.title": "Maximum time to live for burn-on-read messages", + "admin.posts.enableUserTypingMessages.desc": "This setting determines whether \"user is typing...\" messages are displayed below the message box. Disabling the setting in larger deployments may improve server performance.", + "admin.posts.enableUserTypingMessages.title": "Enable User Typing Messages:", "admin.posts.persistentNotifications.desc": "When enabled, users can trigger repeating notifications for the recipients of urgent messages. Learn more about message priority and persistent notifications in our documentation.", "admin.posts.persistentNotifications.title": "Persistent Notifications", "admin.posts.persistentNotificationsGuests.desc": "Whether a guest is able to require persistent notifications. Learn more about message priority and persistent notifications in our documentation.", @@ -2866,6 +2864,9 @@ "admin.posts.sections.priority.title": "Priority & Urgent Notifications", "admin.posts.sections.threads.description": "Configure threaded discussions and auto-follow defaults.", "admin.posts.sections.threads.title": "Threads", + "admin.posts.timeBetweenUserTypingUpdatesMilliseconds.desc": "The number of milliseconds to wait between emitting user typing websocket events.", + "admin.posts.timeBetweenUserTypingUpdatesMilliseconds.example": "E.g.: \"5000\"", + "admin.posts.timeBetweenUserTypingUpdatesMilliseconds.title": "User Typing Timeout (milliseconds):", "admin.privacy.showEmailDescription": "When false, hides the email address of members from everyone except System Administrators and the System Roles with read/write access to Compliance, Billing, or User Management.", "admin.privacy.showEmailTitle": "Show Email Address:", "admin.privacy.showFullNameDescription": "When false, hides the full name of members from everyone except System Administrators. Username is shown in place of full name.", diff --git a/webapp/channels/src/utils/admin_console_index.test.tsx b/webapp/channels/src/utils/admin_console_index.test.tsx index 2928f05b8b10..4ed5db99530d 100644 --- a/webapp/channels/src/utils/admin_console_index.test.tsx +++ b/webapp/channels/src/utils/admin_console_index.test.tsx @@ -38,6 +38,10 @@ describe('AdminConsoleIndex.generateIndex', () => { 'site_config/customization', 'authentication/password', ]); + expect(idx.search('typing')).toEqual([ + 'experimental/features', + 'site_config/posts', + ]); expect(idx.search('caracteres')).toEqual([]); expect(idx.search('notexistingword')).toEqual([]); });