diff --git a/.github/workflows/i18n-ci-template.yml b/.github/workflows/i18n-ci-template.yml index a4b6743b97a5..9c518bd76948 100644 --- a/.github/workflows/i18n-ci-template.yml +++ b/.github/workflows/i18n-ci-template.yml @@ -7,11 +7,14 @@ on: permissions: contents: read +# Translations used to arrive exclusively through Weblate, so this job failed +# any PR that touched a non-English file unless it came from the weblate +# account. Translations may now be authored in-repo and land in the same PR as +# the English string, so the change is reported instead of rejected. jobs: check-files: - name: Check only English translation files changed + name: Report non-English translation file changes runs-on: ubuntu-22.04 - if: github.event.pull_request.user.login != 'weblate' # Allow weblate to modify non-English steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -31,5 +34,4 @@ jobs: - name: Check changed files if: steps.changed-files.outputs.any_changed == 'true' run: | - echo "::error title=Non-English i18n files changed::Only PRs from weblate should modify non-English translation files." - exit 1 + echo "::warning title=Non-English i18n files changed::This PR modifies non-English translation files directly. Ensure the changes are intentional and will not be overwritten by Weblate." diff --git a/e2e-tests/playwright/specs/functional/channels/sidebar_left/dm_gm_profiles_on_load.spec.ts b/e2e-tests/playwright/specs/functional/channels/sidebar_left/dm_gm_profiles_on_load.spec.ts new file mode 100644 index 000000000000..55b3d437ae7b --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/sidebar_left/dm_gm_profiles_on_load.spec.ts @@ -0,0 +1,163 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import type {PlaywrightExtended} from '@mattermost/playwright-lib'; +import {expect, test} from '@mattermost/playwright-lib'; + +// GET /api/v4/users/me/channels and GET /api/v4/users/me/teams/{team_id}/channels, the two requests +// that populate the sidebar's channels. Anchored so the sibling /channels/members and +// /channels/categories routes are left alone. +const CHANNEL_LIST_PATH = /^\/api\/v4\/users\/me\/(channels|teams\/[^/]+\/channels)$/; + +// GET /api/v4/users/{user_id}/channel_members, the request that populates the user's channel +// memberships. The DM half of loadProfilesForSidebar reads getMyChannels, which filters the channels +// through these, so it needs them as much as it needs the channels themselves. +const CHANNEL_MEMBERS_PATH = /^\/api\/v4\/users\/[^/]+\/channel_members$/; + +// GET /api/v4/users, the paged "load some profiles" request. +const USER_PAGE_PATH = /^\/api\/v4\/users$/; + +const REQUEST_DELAY_MS = 4000; + +type Scenario = Awaited>; + +/** + * Sets up a favorited DM and a GM whose members appear nowhere else in the loaded state, then logs + * in without navigating anywhere yet. + */ +async function setupSidebarProfiles(pw: PlaywrightExtended) { + const {adminClient, user, userClient, team} = await pw.initSetup(); + + // # Create the teammates for the DM and the GM. They are deliberately left out of the + // channel the test lands on, so that nothing else pulls their profiles into the store. + const [dmUser, gmUser1, gmUser2] = await adminClient.createUsers(team.id, 3, 'sidebar-profile'); + + const dmChannel = await userClient.createDirectChannel([user.id, dmUser.id]); + const gmChannel = await userClient.createGroupChannel([user.id, gmUser1.id, gmUser2.id]); + + // # Land on a channel containing only the test user, so the current channel's member + // profiles can't mask a missing DM or GM profile. + const landingChannel = await adminClient.createPublicChannel(team.id, 'Sidebar Profiles'); + await adminClient.addToChannel(user.id, landingChannel.id); + + // # Make both conversations visible in the sidebar + await userClient.savePreferences(user.id, [ + {user_id: user.id, category: 'direct_channel_show', name: dmUser.id, value: 'true'}, + {user_id: user.id, category: 'group_channel_show', name: gmChannel.id, value: 'true'}, + ]); + + // # Move the DM into Favorites. A DM whose teammate profile is missing is filtered out of + // the Direct Messages category, so only another category surfaces the blank row. + const {categories} = await userClient.getChannelCategories(user.id, team.id); + const favorites = categories.find((category) => category.type === 'favorites'); + const directMessages = categories.find((category) => category.type === 'direct_messages'); + if (!favorites || !directMessages) { + throw new Error('Expected the default Favorites and Direct Messages categories to exist'); + } + await userClient.updateChannelCategories(user.id, team.id, [ + {...favorites, channel_ids: [dmChannel.id]}, + { + ...directMessages, + channel_ids: directMessages.channel_ids.filter((channelId) => channelId !== dmChannel.id), + }, + ]); + + const {channelsPage, page} = await pw.testBrowser.login(user); + + // # Empty the first page of users, which on a server this small would otherwise return + // every teammate and load the DM profile by accident. + await page.route( + (url) => USER_PAGE_PATH.test(url.pathname), + (route) => route.fulfill({status: 200, contentType: 'application/json', body: '[]'}), + ); + + return {channelsPage, page, team, landingChannel, dmUser, dmChannel, gmChannel}; +} + +/** + * Delays the requests matching the given path so the sidebar categories always win the race. + */ +async function delayRequests({page}: Scenario, path: RegExp) { + await page.route( + (url) => path.test(url.pathname), + async (route) => { + await new Promise((resolve) => setTimeout(resolve, REQUEST_DELAY_MS)); + await route.continue(); + }, + ); +} + +/** + * Loads the landing channel. Nothing in the sidebar is clicked afterwards, because navigating to a + * channel reloads these profiles and would hide the bug. + */ +async function gotoLandingChannel({channelsPage, team, landingChannel}: Scenario) { + await channelsPage.goto(team.name, landingChannel.name); + await channelsPage.toBeVisible(); +} + +/** + * @objective Verify that direct and group message channels in the sidebar render the teammate's + * name and the member count when the channel list arrives after the sidebar categories. + * + * @precondition + * Two conditions that occur naturally on a busy account are forced here so the test is + * deterministic on a small server: + * + * 1. The channel list requests are delayed so the sidebar categories always resolve first. That + * ordering is what leaves the DM and GM profiles unloaded, and it happens on its own once an + * account has enough channels for the channel list to be the slower of the two. + * 2. The first page of GET /api/v4/users is emptied. On a server with a handful of users that one + * request happens to return every teammate and hides the missing DM profile; on a real server + * the teammates are nowhere near the first page. + */ +test( + 'renders DM and GM sidebar rows when the channel list arrives after the categories', + {tag: '@sidebar_left'}, + async ({pw}) => { + const scenario = await setupSidebarProfiles(pw); + + // # Delay the channel list so the sidebar categories always win the race + await delayRequests(scenario, CHANNEL_LIST_PATH); + + await gotoLandingChannel(scenario); + + const {sidebarLeft} = scenario.channelsPage; + + // * Verify the group message shows how many other members it has instead of 0 + await expect(sidebarLeft.memberCountBadge(scenario.gmChannel.name)).toHaveText('2'); + + // * Verify the favorited DM shows the teammate's username instead of rendering an empty row + await expect(sidebarLeft.item(scenario.dmChannel.name)).toContainText(scenario.dmUser.username); + }, +); + +/** + * @objective Verify that direct and group message channels in the sidebar render the teammate's + * name and the member count when the channel memberships arrive after the sidebar categories. + * + * @precondition + * Same as the channel list ordering above, except the memberships are the slow request. This is the + * separate half of the dependency: the channels can all be present and the DM profiles still go + * unloaded, because getMyChannels filters the channels through the memberships. + */ +test( + 'renders DM and GM sidebar rows when the channel memberships arrive after the categories', + {tag: '@sidebar_left'}, + async ({pw}) => { + const scenario = await setupSidebarProfiles(pw); + + // # Delay the channel memberships so the sidebar categories always win the race + await delayRequests(scenario, CHANNEL_MEMBERS_PATH); + + await gotoLandingChannel(scenario); + + const {sidebarLeft} = scenario.channelsPage; + + // * Verify the group message shows how many other members it has instead of 0 + await expect(sidebarLeft.memberCountBadge(scenario.gmChannel.name)).toHaveText('2'); + + // * Verify the favorited DM shows the teammate's username instead of rendering an empty row + await expect(sidebarLeft.item(scenario.dmChannel.name)).toContainText(scenario.dmUser.username); + }, +); 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_form.spec.ts similarity index 69% rename from e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes.spec.ts rename to e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_form.spec.ts index c3a7e959c35b..4ed202431978 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_form.spec.ts @@ -2,48 +2,36 @@ // See LICENSE.txt for license information. /** - * System Console — Global Attributes access gate and attribute listing. - * Visibility is gated by the GlobalAttributes feature flag AND an Enterprise-tier license. - * Once past the gate, the page lists every access_control/template property field on the server. + * System Console — Global Attributes create/edit form (Definition, options, external + * source, Applies-to). Assumes the GlobalAttributes flag is already on — flag-off + * coverage lives in global_attributes_listing.spec.ts so this file never turns it off. * * Local runs: upload or use a license with SkuShortName `enterprise`, `entry`, or `advanced`. - * Professional-only licenses hide this admin route (React Router redirects away). */ import {expect, test, getAdminClient} from '@mattermost/playwright-lib'; -import { - CLASSIFICATION_MARKINGS_ADMIN_PATH, - deleteClassificationMarkingsFieldIfExists, - setClassificationMarkingsFeatureFlag, -} from '../site_configuration/classification_markings_helpers'; - import { GLOBAL_ATTRIBUTES_ADMIN_PATH, createGlobalAttributeField, createLinkedDependentField, deleteAppliesToAttributeAndLinkedFieldsIfExists, deleteGlobalAttributeFieldIfExists, - deleteLinkedDependentField, fetchLinkedFieldsForTemplate, requireGlobalAttributesEnabled, setGlobalAttributesFeatureFlag, } from './global_attributes_helpers'; -test.describe('System Console - Global Attributes', {tag: '@system_console'}, () => { - // All tests here toggle the same server-wide GlobalAttributes config flag, so the whole - // file (both nested describes below) must stay serial — parallelizing across them would - // race one test's flag-on against another's flag-off on the shared server. +test.describe('System Console - Global Attributes form', {tag: '@system_console'}, () => { + // Serial so create/edit/applies-to tests on the shared server do not overlap mid-save. test.describe.configure({mode: 'serial'}); let originalFlagValue: boolean | undefined; - let originalClassificationFlagValue: boolean | undefined; test.beforeAll(async () => { const {adminClient} = await getAdminClient(); const {FeatureFlags} = await adminClient.getConfig(); originalFlagValue = FeatureFlags.GlobalAttributes === true; - originalClassificationFlagValue = FeatureFlags.ClassificationMarkings === true; }); test.afterAll(async () => { @@ -51,377 +39,6 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () if (adminClient && originalFlagValue !== undefined) { await setGlobalAttributesFeatureFlag(adminClient, originalFlagValue); } - if (adminClient && originalClassificationFlagValue !== undefined) { - await setClassificationMarkingsFeatureFlag(adminClient, originalClassificationFlagValue); - } - }); - - test.describe('access gate', () => { - /** - * @objective Ensure the Manage Attributes admin route is unavailable when the feature flag is off. - */ - test('feature flag off hides Manage Attributes regardless of license', async ({pw}) => { - const {adminUser, adminClient} = await getAdminClient(); - - if (!adminUser || !adminClient) { - throw new Error('Failed to get admin user'); - } - - // # Turn off GlobalAttributes in server config - await setGlobalAttributesFeatureFlag(adminClient, false); - const {FeatureFlags} = await adminClient.getConfig(); - test.skip( - FeatureFlags.GlobalAttributes === true, - 'GlobalAttributes stays enabled (e.g. MM_FEATUREFLAGS or split-key overrides); cannot assert flag-off in this environment.', - ); - - // # Navigate directly to the Manage Attributes path - const {systemConsolePage} = await pw.testBrowser.login(adminUser); - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - // * User is redirected away from the hidden route (no Route registered) - await expect(systemConsolePage.page).not.toHaveURL(/manage_attributes/); - // * Manage Attributes menu entry is not shown in the sidebar - await expect( - systemConsolePage.page.getByTestId('admin-sidebar').getByText('Manage Attributes'), - ).not.toBeVisible(); - }); - - /** - * @objective Ensure the Manage Attributes page is reachable and shows its page frame - * once the feature flag is on and the license meets the Enterprise tier. - */ - test('feature flag on with Enterprise+ license shows the page frame', async ({pw}) => { - const {adminUser} = await requireGlobalAttributesEnabled(pw); - - // # Log in and open the Manage Attributes URL - const {systemConsolePage} = await pw.testBrowser.login(adminUser); - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - // * URL stays on the Manage Attributes section - await expect(systemConsolePage.page).toHaveURL(/manage_attributes/); - // * Sidebar menu entry and page heading are both visible ("Manage Attributes" - // renders in both places, so each is asserted within its own scope) - await expect( - systemConsolePage.page.getByTestId('admin-sidebar').getByText('Manage Attributes'), - ).toBeVisible(); - await expect( - systemConsolePage.page.getByTestId('admin-console-header').getByText('Manage Attributes'), - ).toBeVisible(); - // * Page frame's static subtitle is present (renders regardless of fetch state) - await expect( - systemConsolePage.page.getByText('Define an attribute once, then choose which resources can use it.'), - ).toBeVisible(); - }); - }); - - test.describe('listing', () => { - /** - * @objective Ensure a real access_control/template attribute renders in the table with - * its display name, type icon+label, source, and options count — across every field - * type the ticket's Type column has a mapping for (Text/Select/Multiselect/Ranked), - * plus one unmapped type (date) to prove the fallback also holds end-to-end. - */ - test('renders one seeded field per type with correct Attribute/Type/Source/Options values', async ({pw}) => { - const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); - - const timestamp = Date.now(); - - // Covers the "Managed here" Source branch (all of these) alongside every Type/Options - // combination reachable through the admin API — the plugin+protected Source branch is - // unit-test-only since the admin API blocks source_plugin_id/protected from non-plugin - // callers, and rank uses type: 'rank' directly (the same type Classification Markings' - // own saveCreateField creates), not the select-based seeding some older e2e helpers use. - // displayName embeds the same per-run timestamp as name — required so the row locator - // below can't collide with another concurrently-running browser project's seeded row - // (this suite's projects share one server/worker pool; see playwright.config.ts). - const seeds = [ - { - name: `e2e_global_attribute_text_${timestamp}`, - displayName: `E2E Text Attribute ${timestamp}`, - type: 'text', - attrs: {}, - expectedType: 'Text', - expectedOptions: 'Free Text', - }, - { - name: `e2e_global_attribute_select_${timestamp}`, - displayName: `E2E Select Attribute ${timestamp}`, - type: 'select', - attrs: { - options: [ - {id: '', name: 'Option A'}, - {id: '', name: 'Option B'}, - ], - }, - expectedType: 'Select', - expectedOptions: '2 options', - }, - { - name: `e2e_global_attribute_multiselect_${timestamp}`, - displayName: `E2E Multiselect Attribute ${timestamp}`, - type: 'multiselect', - attrs: { - options: [ - {id: '', name: 'Option A'}, - {id: '', name: 'Option B'}, - {id: '', name: 'Option C'}, - ], - }, - expectedType: 'Multiselect', - expectedOptions: '3 options', - }, - { - name: `e2e_global_attribute_rank_${timestamp}`, - displayName: `E2E Ranked Attribute ${timestamp}`, - type: 'rank', - attrs: { - options: [ - {id: '', name: 'Low', rank: 1}, - {id: '', name: 'High', rank: 2}, - ], - }, - expectedType: 'Ranked', - expectedOptions: '2 options', - }, - { - name: `e2e_global_attribute_date_${timestamp}`, - displayName: `E2E Date Attribute ${timestamp}`, - type: 'date', - attrs: {}, - expectedType: 'Other', - expectedOptions: 'Free Text', - }, - ] as const; - - try { - // # Seed every field inside the try block — if creation fails partway - // through (e.g. field 3 of 5), the finally below still cleans up whatever - // was already created instead of leaving it orphaned on the shared server. - for (const seed of seeds) { - await createGlobalAttributeField(adminClient, seed.name, { - type: seed.type, - attrs: {display_name: seed.displayName, ...seed.attrs}, - }); - } - - // # Log in and open the Manage Attributes page once every field is seeded - const {systemConsolePage} = await pw.testBrowser.login(adminUser); - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - for (const seed of seeds) { - // Re-rooted `has` probe on the name cell's testid+text, mirroring the - // established row-lookup pattern (see board_attributes.ts's rowByName). - const row = systemConsolePage.page.locator('tr', { - has: systemConsolePage.page - .getByTestId('global-attribute-name') - .filter({hasText: seed.displayName}), - }); - - // * Each seeded field's display name, type, source, and options render correctly - await expect(row.getByTestId('global-attribute-name')).toHaveText(seed.displayName); - await expect(row.getByTestId('global-attribute-type')).toContainText(seed.expectedType); - // * A leading icon renders alongside the label, including for the - // unmapped "date" type (fallback icon, not a blank cell) - await expect(row.getByTestId('global-attribute-type').locator('svg')).toBeVisible(); - await expect(row.getByTestId('global-attribute-source')).toContainText('Managed here'); - // * "Managed here" is the one source kind reachable through the admin - // API (plugin/ldap/saml require server-side attrs the API blocks from - // non-plugin callers — those icon mappings are unit-test-only) and it - // renders with no leading icon, unlike plugin/ldap/saml sources - await expect(row.getByTestId('global-attribute-source').locator('svg')).toHaveCount(0); - await expect(row.getByTestId('global-attribute-options')).toContainText(seed.expectedOptions); - } - } finally { - // # Clean up regardless of assertion outcome, so reruns start from a clean slate - for (const seed of seeds) { - await deleteGlobalAttributeFieldIfExists(adminClient, seed.name); - } - } - }); - - /** - * @objective Ensure the table sorts by the same value shown in the Attribute column - * (display_name, falling back to name) rather than by the hidden internal name. - */ - test('sorts rows by the displayed Attribute value, not the internal field name', async ({pw}) => { - const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); - - const timestamp = Date.now(); - - // Internal names sort z-then-a; display names (what the table actually shows and - // must sort by) sort a-then-z. If the table sorted by the internal name instead, - // these rows would render in the opposite order. - const firstName = `zzz_e2e_sort_${timestamp}`; - const secondName = `aaa_e2e_sort_${timestamp}`; - const firstDisplayName = `Aardvark E2E Sort Attribute ${timestamp}`; - const secondDisplayName = `Zebra E2E Sort Attribute ${timestamp}`; - - // No display_name set — the rendered (and sorted-by) value falls back to this - // internal name directly. Chosen to alphabetically land between the two display - // names above, so its position in the rendered order proves the fallback value - // participates in sorting correctly alongside explicit display names, not just - // in isolation (the unit tests already cover the fallback alone). - const thirdName = `mmm_e2e_sort_fallback_${timestamp}`; - - try { - // # Seed all three fields inside the try block — if creation fails - // partway through, the finally below still cleans up whatever was - // already created instead of leaving it orphaned on the shared server. - await createGlobalAttributeField(adminClient, firstName, { - type: 'text', - attrs: {display_name: firstDisplayName}, - }); - await createGlobalAttributeField(adminClient, secondName, { - type: 'text', - attrs: {display_name: secondDisplayName}, - }); - await createGlobalAttributeField(adminClient, thirdName, { - type: 'text', - attrs: {}, - }); - - const {systemConsolePage} = await pw.testBrowser.login(adminUser); - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - await systemConsolePage.page.getByTestId('global-attribute-name').getByText(firstDisplayName).waitFor(); - - // * The fallback field renders under its internal name, since no display_name was set - await expect( - systemConsolePage.page.getByTestId('global-attribute-name').getByText(thirdName), - ).toBeVisible(); - - // * Rendered order follows the displayed Attribute value (Aardvark, then the - // fallback name, then Zebra) — not the internal name (which would put "aaa_..." - // first if sorted that way) - const names = await systemConsolePage.page.getByTestId('global-attribute-name').allTextContents(); - const firstIndex = names.indexOf(firstDisplayName); - const thirdIndex = names.indexOf(thirdName); - const secondIndex = names.indexOf(secondDisplayName); - expect(firstIndex).toBeGreaterThanOrEqual(0); - expect(thirdIndex).toBeGreaterThanOrEqual(0); - expect(secondIndex).toBeGreaterThanOrEqual(0); - expect(firstIndex).toBeLessThan(thirdIndex); - expect(thirdIndex).toBeLessThan(secondIndex); - } finally { - await deleteGlobalAttributeFieldIfExists(adminClient, firstName); - await deleteGlobalAttributeFieldIfExists(adminClient, secondName); - await deleteGlobalAttributeFieldIfExists(adminClient, thirdName); - } - }); - - /** - * @objective Ensure a real Classification Markings field (name/object_type/group_id - * matching production's saveCreateField) renders the read-only subtitle and an - * open-in-new link to its own admin page instead of the ordinary dot-menu, and that an - * unrelated field — including one that shares the same 'rank' type — is entirely unaffected. - */ - test( - 'renders the Classification Markings row as a read-only open-in-new link, leaving an unrelated rank field unaffected', - {tag: ['@system_console', '@classification_markings']}, - async ({pw}) => { - const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); - - // # The link's destination page is gated by its own independent feature flag - // (ClassificationMarkings), separate from the GlobalAttributes flag gating this - // listing page — both must be on for the link to render. - // Tagged @classification_markings like every other spec that touches this same - // shared server-wide field/flag (classification_markings.spec.ts, - // global_classification_banner.spec.ts) — those specs are NOT otherwise - // concurrency-guarded against each other; the tag is this suite's existing - // (if informal) convention for grouping tests that share this exact resource. - await setClassificationMarkingsFeatureFlag(adminClient, true); - await pw.skipIfFeatureFlagNotSet('ClassificationMarkings', true); - - const timestamp = Date.now(); - const classificationDisplayName = `E2E Classification Attribute ${timestamp}`; - const unrelatedRankName = `e2e_unrelated_rank_${timestamp}`; - const unrelatedRankDisplayName = `E2E Unrelated Ranked Attribute ${timestamp}`; - - try { - // # Clean slate first via the classification-aware helper (not the generic - // deleteGlobalAttributeFieldIfExists): it also removes any linked system/channel - // field first, avoiding server-side deletion-protection errors that leave a - // stale template field behind if another classification spec ran previously. - await deleteClassificationMarkingsFieldIfExists(adminClient); - - // # Seed the real classification field: name 'classification', type 'rank', - // matching classification_markings/utils/index.ts's saveCreateField exactly — - // not the select-based shape some older, test-only e2e helpers use. - await createGlobalAttributeField(adminClient, 'classification', { - type: 'rank', - attrs: { - display_name: classificationDisplayName, - options: [ - {id: '', name: 'Unclassified', rank: 1}, - {id: '', name: 'Secret', rank: 2}, - ], - }, - }); - - // # Seed an unrelated field that shares the same 'rank' type, to prove the - // predicate keys on name/object_type/group_id, not on type. - await createGlobalAttributeField(adminClient, unrelatedRankName, { - type: 'rank', - attrs: { - display_name: unrelatedRankDisplayName, - options: [ - {id: '', name: 'Low', rank: 1}, - {id: '', name: 'High', rank: 2}, - ], - }, - }); - - const {systemConsolePage} = await pw.testBrowser.login(adminUser); - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - const classificationRow = systemConsolePage.page.locator('tr', { - has: systemConsolePage.page - .getByTestId('global-attribute-name') - .filter({hasText: classificationDisplayName}), - }); - await classificationRow.waitFor(); - - // * The read-only subtitle renders under the attribute name - await expect(classificationRow.getByText('Read-only')).toBeVisible(); - - // * The Source column identifies this row's true source, not the generic - // "Managed here" every other native field gets - await expect(classificationRow.getByTestId('global-attribute-source')).toContainText( - 'Classification Markings', - ); - - // * The rightmost cell is an open-in-new link to the Classification Markings - // admin page, not the dot-menu action trigger - const openInNewLink = classificationRow.getByRole('link', {name: 'Open Classification Markings'}); - await expect(openInNewLink).toBeVisible(); - await expect(openInNewLink).toHaveAttribute('href', CLASSIFICATION_MARKINGS_ADMIN_PATH); - await expect(classificationRow.getByRole('button', {name: 'More actions'})).toHaveCount(0); - - // # Clicking the link actually navigates to the Classification Markings page - await openInNewLink.click(); - // * Navigation lands on the Classification Markings admin page - await expect(systemConsolePage.page).toHaveURL(new RegExp(CLASSIFICATION_MARKINGS_ADMIN_PATH)); - - await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - - // * An unrelated 'rank'-type field is unaffected: ordinary dot-menu, no subtitle - const unrelatedRow = systemConsolePage.page.locator('tr', { - has: systemConsolePage.page - .getByTestId('global-attribute-name') - .filter({hasText: unrelatedRankDisplayName}), - }); - await unrelatedRow.waitFor(); - await expect(unrelatedRow.getByRole('button', {name: 'More actions'})).toBeVisible(); - await expect(unrelatedRow.getByText('Read-only')).toHaveCount(0); - await expect(unrelatedRow.getByRole('link', {name: 'Open Classification Markings'})).toHaveCount(0); - await expect(unrelatedRow.getByTestId('global-attribute-source')).toContainText('Managed here'); - } finally { - await deleteClassificationMarkingsFieldIfExists(adminClient); - await deleteGlobalAttributeFieldIfExists(adminClient, unrelatedRankName); - } - }, - ); }); test.describe('create attribute', () => { @@ -1370,17 +987,18 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () }); }); - test.describe('delete attribute', () => { + test.describe('edit 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. + * @objective Opening Edit on a managed attribute prefills the Definition form and Save + * PATCHes the existing template rather than creating a second one. */ - test('deletes an attribute from the row menu after confirming, and the row disappears', async ({pw}) => { + test('opens an existing attribute, PATCHes a display-name change, and shows it in the list', 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}`; + const name = `e2e_global_attribute_edit_${timestamp}`; + const displayName = `Playwright Edit ${timestamp}`; + const updatedDisplayName = `${displayName} Updated`; try { const field = await createGlobalAttributeField(adminClient, name, { @@ -1392,138 +1010,376 @@ test.describe('System Console - Global Attributes', {tag: '@system_console'}, () 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 page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await expect(page).toHaveURL(new RegExp(`attribute_details/${field.id}$`)); + await expect(page.getByRole('heading', {name: 'Edit attribute'})).toBeVisible(); + await expect(page.getByTestId('attributeDisplayNameInput')).toHaveValue(displayName); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); + + await page.getByTestId('attributeDisplayNameInput').fill(updatedDisplayName); + await page.getByTestId('saveSetting').click(); + + await expect(page).toHaveURL(new RegExp(`${GLOBAL_ATTRIBUTES_ADMIN_PATH}$`)); + await expect( + page.getByTestId('global-attribute-name').filter({hasText: updatedDisplayName}), + ).toBeVisible(); + } finally { + await deleteGlobalAttributeFieldIfExists(adminClient, name); + } + }); + + /** + * @objective Regression test: a no-op Edit/Done round-trip on the Unique name must not + * unpin it, even when the loaded name happens to equal the current auto-slug of the + * Display name -- Done's "did the committed value change from the auto-slug" check + * previously mistook that coincidence for "not manually set" and re-enabled live + * derivation, so a later Display name edit silently changed the persisted Unique name. + */ + test('does not unpin Unique name after a no-op Edit/Done round-trip, even when the loaded name matches the current auto-slug', async ({ + pw, + }) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const displayName = `Playwright Pin ${timestamp}`; + + // Matches the auto-slug of displayName exactly -- the coincidence that triggered + // the regression (see slugifyForCEL: lowercase, spaces to underscores). + const name = `playwright_pin_${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, }); - await expect(row).toBeVisible(); - // # Open the row kebab and click Delete + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); - await page.locator(`#global-attribute-actions-${field.id}-delete`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); - // * The confirmation names the specific attribute rather than prompting generically - await expect(page.getByRole('heading', {name: `Delete ${displayName} attribute`})).toBeVisible(); + // # No-op Edit/Done round-trip -- open Edit, change nothing, click Done + await page.getByTestId('attributeNameEditLink').click(); + await page.getByTestId('attributeNameEditLink').click(); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); - // # Confirm - await page.getByRole('button', {name: 'Delete', exact: true}).click(); + // * Changing Display name afterward must not re-slug the pinned Unique name + await page.getByTestId('attributeDisplayNameInput').fill(`${displayName} Updated`); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); + } finally { + await deleteGlobalAttributeFieldIfExists(adminClient, name); + } + }); + + /** + * @objective Same regression, via the Escape-only variant of a no-op edit session -- + * confirms Escape (which never touches the pin) was never the problem, only Done was. + */ + test('does not unpin Unique name after a no-op Edit/Escape round-trip', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const displayName = `Playwright Pin Escape ${timestamp}`; + const name = `playwright_pin_escape_${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); + + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); - // * The row is gone and no error banner appeared - await expect(row).toHaveCount(0); - await expect(page.getByTestId('global-attributes-delete-error')).toHaveCount(0); + await page.getByTestId('attributeNameEditLink').click(); + await page.getByTestId('attributeNameInput').press('Escape'); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); - // * 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); + await page.getByTestId('attributeDisplayNameInput').fill(`${displayName} Updated`); + await expect(page.getByTestId('attributeUniqueNameValue')).toHaveText(name); } 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. + * @objective Type is locked while a persisted Applies-to resource is on the form. + * Remove is local until Save, which DELETEs the linked field and leaves the template. */ - test('leaves the attribute in place when the confirmation is cancelled', async ({pw}) => { + test('locks Type while a resource is applied, and Save deletes a removed linked field', 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}`; + const name = `e2e_global_attribute_edit_remove_${timestamp}`; + const displayName = `Playwright Remove ${timestamp}`; try { const field = await createGlobalAttributeField(adminClient, name, { type: 'text', attrs: {display_name: displayName}, }); + await createLinkedDependentField(adminClient, name, field.id, 'text', 'user'); 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 page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await expect(page.getByTestId('attributeAppliesToRow-user')).toBeVisible(); + await expect(page.getByTestId('attributeTypeMenuButton')).toBeDisabled(); + + await page.getByTestId('attributeAppliesToRow-user-toggle').click(); + await page.getByTestId('attributeAppliesToRow-user-remove').click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); + await expect(page.getByTestId('attributeTypeMenuButton')).toBeEnabled(); + + await page.getByTestId('saveSetting').click(); + + // # Removing a resource with stored values warns before deleting it + await expect(page.getByRole('dialog')).toBeVisible(); + await page.getByRole('button', {name: 'Remove and save', exact: true}).click(); + + await expect(page).toHaveURL(new RegExp(`${GLOBAL_ATTRIBUTES_ADMIN_PATH}$`)); + + const remaining = await fetchLinkedFieldsForTemplate(adminClient, field.id); + expect(remaining).toHaveLength(0); + const templates = await adminClient.getPropertyFields( + 'access_control', + 'template', + 'system', + undefined, + { + perPage: 200, + }, + ); + expect(templates.some((template) => template.id === field.id && template.delete_at === 0)).toBe(true); + } finally { + await deleteAppliesToAttributeAndLinkedFieldsIfExists(adminClient, name); + } + }); + + /** + * @objective Removing then re-adding the same resource before Save must not delete the + * persisted linked field (and therefore must not wipe stored values). + */ + test('does not delete a linked field that is removed then re-added before Save', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_edit_readd_${timestamp}`; + const displayName = `Playwright Readd ${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, }); - await expect(row).toBeVisible(); + const linked = await createLinkedDependentField(adminClient, name, field.id, 'text', 'user'); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - // # 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(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await page.getByTestId('attributeAppliesToRow-user-toggle').click(); + await page.getByTestId('attributeAppliesToRow-user-remove').click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); - // * The modal closed and the row survived - await expect(page.getByRole('heading', {name: `Delete ${displayName} attribute`})).toHaveCount(0); - await expect(row).toBeVisible(); + await page.getByTestId('attributeAppliesToAddResourceButtonHeader').click(); + await page.getByRole('menuitem', {name: 'Users'}).click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toBeVisible(); - // * Nothing was deleted server-side either - await page.reload(); - await expect(page.getByTestId('global-attribute-name').filter({hasText: displayName})).toBeVisible(); + await page.getByTestId('saveSetting').click(); + await expect(page).toHaveURL(new RegExp(`${GLOBAL_ATTRIBUTES_ADMIN_PATH}$`)); + + const remaining = await fetchLinkedFieldsForTemplate(adminClient, field.id); + expect(remaining).toHaveLength(1); + expect(remaining[0].id).toBe(linked.id); } finally { - await deleteGlobalAttributeFieldIfExists(adminClient, name); + await deleteAppliesToAttributeAndLinkedFieldsIfExists(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. + * @objective Locks the Unique name while any Applies-to resource is persisted -- renaming + * would leave those linked fields on the old identifier, since the server does not + * propagate a template rename onto them. */ - test('shows the linked-dependents banner and keeps the row when the server refuses the delete', async ({ + test('locks Unique name editing while a resource is applied, and unlocks only once Save persists its removal', 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; + const name = `e2e_global_attribute_edit_name_lock_${timestamp}`; + const displayName = `Playwright Name Lock ${timestamp}`; try { const field = await createGlobalAttributeField(adminClient, name, { type: 'text', attrs: {display_name: displayName}, }); + await createLinkedDependentField(adminClient, name, field.id, 'text', 'user'); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - // # 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; + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await expect(page.getByTestId('attributeAppliesToRow-user')).toBeVisible(); + await expect(page.getByTestId('attributeNameEditLink')).toBeDisabled(); + + // # A pending local removal does not unlock the Name -- the dependent field is + // still live on the server until Save actually removes it + await page.getByTestId('attributeAppliesToRow-user-toggle').click(); + await page.getByTestId('attributeAppliesToRow-user-remove').click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); + await expect(page.getByTestId('attributeNameEditLink')).toBeDisabled(); + + await page.getByTestId('saveSetting').click(); + await expect(page.getByRole('dialog')).toBeVisible(); + await page.getByRole('button', {name: 'Remove and save', exact: true}).click(); + await expect(page).toHaveURL(new RegExp(`${GLOBAL_ATTRIBUTES_ADMIN_PATH}$`)); + + // * Only once the removal is actually persisted does re-opening the attribute + // show the Name as editable + await page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); + await expect(page.getByTestId('attributeNameEditLink')).toBeEnabled(); + } finally { + await deleteAppliesToAttributeAndLinkedFieldsIfExists(adminClient, name); + } + }); + + /** + * @objective Declining the remove-applies-to warning aborts the save entirely -- no PATCH, + * no DELETE, and the admin stays on the form with the persisted linked field untouched. + */ + test('cancelling the remove-applies-to warning leaves the linked field untouched', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_edit_cancel_remove_${timestamp}`; + const displayName = `Playwright Cancel Remove ${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, + }); + const linked = await createLinkedDependentField(adminClient, name, field.id, 'text', 'user'); 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 page.getByTestId(`global-attribute-actions-${field.id}`).click(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await page.getByTestId('attributeAppliesToRow-user-toggle').click(); + await page.getByTestId('attributeAppliesToRow-user-remove').click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); + + await page.getByTestId('saveSetting').click(); + await expect(page.getByRole('dialog')).toBeVisible(); + await page.getByRole('button', {name: 'Cancel', exact: true}).click(); + + // * Still on the form, Save re-clickable, nothing sent to the server + await expect(page).toHaveURL(new RegExp(`attribute_details/${field.id}$`)); + await expect(page.getByTestId('saveSetting')).toBeEnabled(); + + const remaining = await fetchLinkedFieldsForTemplate(adminClient, field.id); + expect(remaining).toHaveLength(1); + expect(remaining[0].id).toBe(linked.id); + } finally { + await deleteAppliesToAttributeAndLinkedFieldsIfExists(adminClient, name); + } + }); + + /** + * @objective Regression guard for the ordering bug: when the type is not changing, a + * failed PATCH (e.g. a name conflict) must abort the save before the confirmed removal's + * DELETE ever runs -- otherwise the linked field's stored values are lost even though + * Save reported failure. + */ + test('does not delete a confirmed-removed linked field when the PATCH fails and type is unchanged', async ({ + pw, + }) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + const name = `e2e_global_attribute_edit_patch_fail_${timestamp}`; + const displayName = `Playwright Patch Fail ${timestamp}`; + + try { + const field = await createGlobalAttributeField(adminClient, name, { + type: 'text', + attrs: {display_name: displayName}, }); - await expect(row).toBeVisible(); + const linked = await createLinkedDependentField(adminClient, name, field.id, 'text', 'user'); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const {page} = systemConsolePage; + await page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); - // # 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(); + await page.locator(`#global-attribute-actions-${field.id}-edit`).click(); + + await page.getByTestId('attributeAppliesToRow-user-toggle').click(); + await page.getByTestId('attributeAppliesToRow-user-remove').click(); + await expect(page.getByTestId('attributeAppliesToRow-user')).toHaveCount(0); + + // # Force the template PATCH to fail for a reason unrelated to type (e.g. a + // name conflict), without touching the DELETE/POST linked-field requests + await page.route( + `**/api/v4/properties/groups/access_control/template/fields/${field.id}`, + async (route) => { + if (route.request().method() === 'PATCH') { + await route.fulfill({ + status: 400, + contentType: 'application/json', + body: JSON.stringify({ + id: 'app.property_field.update.name_conflict.app_error', + message: 'forced name conflict', + }), + }); + } else { + await route.continue(); + } + }, + ); - // * 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'); + await page.getByTestId('saveSetting').click(); + await expect(page.getByRole('dialog')).toBeVisible(); + await page.getByRole('button', {name: 'Remove and save', exact: true}).click(); - // * The row survived the rejected delete - await expect(row).toBeVisible(); + // * Save reports failure and stays on the form + await expect(page.getByTestId('attributeSaveError')).toBeVisible(); + await expect(page).toHaveURL(new RegExp(`attribute_details/${field.id}$`)); - // # The banner is dismissible - await banner.getByRole('button', {name: 'Close'}).click(); - await expect(banner).toHaveCount(0); + // * The linked field survives -- PATCH ran (and failed) before any DELETE could + const remaining = await fetchLinkedFieldsForTemplate(adminClient, field.id); + expect(remaining).toHaveLength(1); + expect(remaining[0].id).toBe(linked.id); } finally { - // Dependent first: the source delete stays blocked while it exists - if (dependentFieldId) { - await deleteLinkedDependentField(adminClient, dependentFieldId); - } - await deleteGlobalAttributeFieldIfExists(adminClient, name); + await deleteAppliesToAttributeAndLinkedFieldsIfExists(adminClient, name); } }); }); diff --git a/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_listing.spec.ts b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_listing.spec.ts new file mode 100644 index 000000000000..9d322c3c146d --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/global_attributes/global_attributes_listing.spec.ts @@ -0,0 +1,584 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +/** + * System Console — Global Attributes access gate and attribute listing. + * Visibility is gated by the GlobalAttributes feature flag AND an Enterprise-tier license. + * Once past the gate, the page lists every access_control/template property field on the server. + * + * Local runs: upload or use a license with SkuShortName `enterprise`, `entry`, or `advanced`. + * Professional-only licenses hide this admin route (React Router redirects away). + * + * Flag-off access-gate tests live only in this file. The form spec assumes the flag is on + * and must not turn it off — both files share a server, and default PW_WORKERS is 1. + */ + +import {expect, test, getAdminClient} from '@mattermost/playwright-lib'; + +import { + CLASSIFICATION_MARKINGS_ADMIN_PATH, + deleteClassificationMarkingsFieldIfExists, + setClassificationMarkingsFeatureFlag, +} from '../site_configuration/classification_markings_helpers'; + +import { + GLOBAL_ATTRIBUTES_ADMIN_PATH, + createGlobalAttributeField, + createLinkedDependentField, + deleteGlobalAttributeFieldIfExists, + deleteLinkedDependentField, + requireGlobalAttributesEnabled, + setGlobalAttributesFeatureFlag, +} from './global_attributes_helpers'; + +test.describe('System Console - Global Attributes listing', {tag: '@system_console'}, () => { + // Access-gate tests toggle the server-wide GlobalAttributes flag. Stay serial so a + // flag-off assertion cannot race a later listing/delete test in this file. + test.describe.configure({mode: 'serial'}); + + let originalFlagValue: boolean | undefined; + let originalClassificationFlagValue: boolean | undefined; + + test.beforeAll(async () => { + const {adminClient} = await getAdminClient(); + const {FeatureFlags} = await adminClient.getConfig(); + originalFlagValue = FeatureFlags.GlobalAttributes === true; + originalClassificationFlagValue = FeatureFlags.ClassificationMarkings === true; + }); + + test.afterAll(async () => { + const {adminClient} = await getAdminClient(); + if (adminClient && originalFlagValue !== undefined) { + await setGlobalAttributesFeatureFlag(adminClient, originalFlagValue); + } + if (adminClient && originalClassificationFlagValue !== undefined) { + await setClassificationMarkingsFeatureFlag(adminClient, originalClassificationFlagValue); + } + }); + + test.describe('access gate', () => { + /** + * @objective Ensure the Manage Attributes admin route is unavailable when the feature flag is off. + */ + test('feature flag off hides Manage Attributes regardless of license', async ({pw}) => { + const {adminUser, adminClient} = await getAdminClient(); + + if (!adminUser || !adminClient) { + throw new Error('Failed to get admin user'); + } + + // # Turn off GlobalAttributes in server config + await setGlobalAttributesFeatureFlag(adminClient, false); + const {FeatureFlags} = await adminClient.getConfig(); + test.skip( + FeatureFlags.GlobalAttributes === true, + 'GlobalAttributes stays enabled (e.g. MM_FEATUREFLAGS or split-key overrides); cannot assert flag-off in this environment.', + ); + + // # Navigate directly to the Manage Attributes path + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + // * User is redirected away from the hidden route (no Route registered) + await expect(systemConsolePage.page).not.toHaveURL(/manage_attributes/); + // * Manage Attributes menu entry is not shown in the sidebar + await expect( + systemConsolePage.page.getByTestId('admin-sidebar').getByText('Manage Attributes'), + ).not.toBeVisible(); + }); + + /** + * @objective Ensure the Manage Attributes page is reachable and shows its page frame + * once the feature flag is on and the license meets the Enterprise tier. + */ + test('feature flag on with Enterprise+ license shows the page frame', async ({pw}) => { + const {adminUser} = await requireGlobalAttributesEnabled(pw); + + // # Log in and open the Manage Attributes URL + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + // * URL stays on the Manage Attributes section + await expect(systemConsolePage.page).toHaveURL(/manage_attributes/); + // * Sidebar menu entry and page heading are both visible ("Manage Attributes" + // renders in both places, so each is asserted within its own scope) + await expect( + systemConsolePage.page.getByTestId('admin-sidebar').getByText('Manage Attributes'), + ).toBeVisible(); + await expect( + systemConsolePage.page.getByTestId('admin-console-header').getByText('Manage Attributes'), + ).toBeVisible(); + // * Page frame's static subtitle is present (renders regardless of fetch state) + await expect( + systemConsolePage.page.getByText('Define an attribute once, then choose which resources can use it.'), + ).toBeVisible(); + }); + }); + + test.describe('listing', () => { + /** + * @objective Ensure a real access_control/template attribute renders in the table with + * its display name, type icon+label, source, and options count — across every field + * type the ticket's Type column has a mapping for (Text/Select/Multiselect/Ranked), + * plus one unmapped type (date) to prove the fallback also holds end-to-end. + */ + test('renders one seeded field per type with correct Attribute/Type/Source/Options values', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + + // Covers the "Managed here" Source branch (all of these) alongside every Type/Options + // combination reachable through the admin API — the plugin+protected Source branch is + // unit-test-only since the admin API blocks source_plugin_id/protected from non-plugin + // callers, and rank uses type: 'rank' directly (the same type Classification Markings' + // own saveCreateField creates), not the select-based seeding some older e2e helpers use. + // displayName embeds the same per-run timestamp as name — required so the row locator + // below can't collide with another concurrently-running browser project's seeded row + // (this suite's projects share one server/worker pool; see playwright.config.ts). + const seeds = [ + { + name: `e2e_global_attribute_text_${timestamp}`, + displayName: `E2E Text Attribute ${timestamp}`, + type: 'text', + attrs: {}, + expectedType: 'Text', + expectedOptions: 'Free Text', + }, + { + name: `e2e_global_attribute_select_${timestamp}`, + displayName: `E2E Select Attribute ${timestamp}`, + type: 'select', + attrs: { + options: [ + {id: '', name: 'Option A'}, + {id: '', name: 'Option B'}, + ], + }, + expectedType: 'Select', + expectedOptions: '2 options', + }, + { + name: `e2e_global_attribute_multiselect_${timestamp}`, + displayName: `E2E Multiselect Attribute ${timestamp}`, + type: 'multiselect', + attrs: { + options: [ + {id: '', name: 'Option A'}, + {id: '', name: 'Option B'}, + {id: '', name: 'Option C'}, + ], + }, + expectedType: 'Multiselect', + expectedOptions: '3 options', + }, + { + name: `e2e_global_attribute_rank_${timestamp}`, + displayName: `E2E Ranked Attribute ${timestamp}`, + type: 'rank', + attrs: { + options: [ + {id: '', name: 'Low', rank: 1}, + {id: '', name: 'High', rank: 2}, + ], + }, + expectedType: 'Ranked', + expectedOptions: '2 options', + }, + { + name: `e2e_global_attribute_date_${timestamp}`, + displayName: `E2E Date Attribute ${timestamp}`, + type: 'date', + attrs: {}, + expectedType: 'Other', + expectedOptions: 'Free Text', + }, + ] as const; + + try { + // # Seed every field inside the try block — if creation fails partway + // through (e.g. field 3 of 5), the finally below still cleans up whatever + // was already created instead of leaving it orphaned on the shared server. + for (const seed of seeds) { + await createGlobalAttributeField(adminClient, seed.name, { + type: seed.type, + attrs: {display_name: seed.displayName, ...seed.attrs}, + }); + } + + // # Log in and open the Manage Attributes page once every field is seeded + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + for (const seed of seeds) { + // Re-rooted `has` probe on the name cell's testid+text, mirroring the + // established row-lookup pattern (see board_attributes.ts's rowByName). + const row = systemConsolePage.page.locator('tr', { + has: systemConsolePage.page + .getByTestId('global-attribute-name') + .filter({hasText: seed.displayName}), + }); + + // * Each seeded field's display name, type, source, and options render correctly + await expect(row.getByTestId('global-attribute-name')).toHaveText(seed.displayName); + await expect(row.getByTestId('global-attribute-type')).toContainText(seed.expectedType); + // * A leading icon renders alongside the label, including for the + // unmapped "date" type (fallback icon, not a blank cell) + await expect(row.getByTestId('global-attribute-type').locator('svg')).toBeVisible(); + await expect(row.getByTestId('global-attribute-source')).toContainText('Managed here'); + // * "Managed here" is the one source kind reachable through the admin + // API (plugin/ldap/saml require server-side attrs the API blocks from + // non-plugin callers — those icon mappings are unit-test-only) and it + // renders with no leading icon, unlike plugin/ldap/saml sources + await expect(row.getByTestId('global-attribute-source').locator('svg')).toHaveCount(0); + await expect(row.getByTestId('global-attribute-options')).toContainText(seed.expectedOptions); + } + } finally { + // # Clean up regardless of assertion outcome, so reruns start from a clean slate + for (const seed of seeds) { + await deleteGlobalAttributeFieldIfExists(adminClient, seed.name); + } + } + }); + + /** + * @objective Ensure the table sorts by the same value shown in the Attribute column + * (display_name, falling back to name) rather than by the hidden internal name. + */ + test('sorts rows by the displayed Attribute value, not the internal field name', async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + const timestamp = Date.now(); + + // Internal names sort z-then-a; display names (what the table actually shows and + // must sort by) sort a-then-z. If the table sorted by the internal name instead, + // these rows would render in the opposite order. + const firstName = `zzz_e2e_sort_${timestamp}`; + const secondName = `aaa_e2e_sort_${timestamp}`; + const firstDisplayName = `Aardvark E2E Sort Attribute ${timestamp}`; + const secondDisplayName = `Zebra E2E Sort Attribute ${timestamp}`; + + // No display_name set — the rendered (and sorted-by) value falls back to this + // internal name directly. Chosen to alphabetically land between the two display + // names above, so its position in the rendered order proves the fallback value + // participates in sorting correctly alongside explicit display names, not just + // in isolation (the unit tests already cover the fallback alone). + const thirdName = `mmm_e2e_sort_fallback_${timestamp}`; + + try { + // # Seed all three fields inside the try block — if creation fails + // partway through, the finally below still cleans up whatever was + // already created instead of leaving it orphaned on the shared server. + await createGlobalAttributeField(adminClient, firstName, { + type: 'text', + attrs: {display_name: firstDisplayName}, + }); + await createGlobalAttributeField(adminClient, secondName, { + type: 'text', + attrs: {display_name: secondDisplayName}, + }); + await createGlobalAttributeField(adminClient, thirdName, { + type: 'text', + attrs: {}, + }); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + await systemConsolePage.page.getByTestId('global-attribute-name').getByText(firstDisplayName).waitFor(); + + // * The fallback field renders under its internal name, since no display_name was set + await expect( + systemConsolePage.page.getByTestId('global-attribute-name').getByText(thirdName), + ).toBeVisible(); + + // * Rendered order follows the displayed Attribute value (Aardvark, then the + // fallback name, then Zebra) — not the internal name (which would put "aaa_..." + // first if sorted that way) + const names = await systemConsolePage.page.getByTestId('global-attribute-name').allTextContents(); + const firstIndex = names.indexOf(firstDisplayName); + const thirdIndex = names.indexOf(thirdName); + const secondIndex = names.indexOf(secondDisplayName); + expect(firstIndex).toBeGreaterThanOrEqual(0); + expect(thirdIndex).toBeGreaterThanOrEqual(0); + expect(secondIndex).toBeGreaterThanOrEqual(0); + expect(firstIndex).toBeLessThan(thirdIndex); + expect(thirdIndex).toBeLessThan(secondIndex); + } finally { + await deleteGlobalAttributeFieldIfExists(adminClient, firstName); + await deleteGlobalAttributeFieldIfExists(adminClient, secondName); + await deleteGlobalAttributeFieldIfExists(adminClient, thirdName); + } + }); + + /** + * @objective Ensure a real Classification Markings field (name/object_type/group_id + * matching production's saveCreateField) renders the read-only subtitle and an + * open-in-new link to its own admin page instead of the ordinary dot-menu, and that an + * unrelated field — including one that shares the same 'rank' type — is entirely unaffected. + */ + test( + 'renders the Classification Markings row as a read-only open-in-new link, leaving an unrelated rank field unaffected', + {tag: ['@system_console', '@classification_markings']}, + async ({pw}) => { + const {adminUser, adminClient} = await requireGlobalAttributesEnabled(pw); + + // # The link's destination page is gated by its own independent feature flag + // (ClassificationMarkings), separate from the GlobalAttributes flag gating this + // listing page — both must be on for the link to render. + // Tagged @classification_markings like every other spec that touches this same + // shared server-wide field/flag (classification_markings.spec.ts, + // global_classification_banner.spec.ts) — those specs are NOT otherwise + // concurrency-guarded against each other; the tag is this suite's existing + // (if informal) convention for grouping tests that share this exact resource. + await setClassificationMarkingsFeatureFlag(adminClient, true); + await pw.skipIfFeatureFlagNotSet('ClassificationMarkings', true); + + const timestamp = Date.now(); + const classificationDisplayName = `E2E Classification Attribute ${timestamp}`; + const unrelatedRankName = `e2e_unrelated_rank_${timestamp}`; + const unrelatedRankDisplayName = `E2E Unrelated Ranked Attribute ${timestamp}`; + + try { + // # Clean slate first via the classification-aware helper (not the generic + // deleteGlobalAttributeFieldIfExists): it also removes any linked system/channel + // field first, avoiding server-side deletion-protection errors that leave a + // stale template field behind if another classification spec ran previously. + await deleteClassificationMarkingsFieldIfExists(adminClient); + + // # Seed the real classification field: name 'classification', type 'rank', + // matching classification_markings/utils/index.ts's saveCreateField exactly — + // not the select-based shape some older, test-only e2e helpers use. + await createGlobalAttributeField(adminClient, 'classification', { + type: 'rank', + attrs: { + display_name: classificationDisplayName, + options: [ + {id: '', name: 'Unclassified', rank: 1}, + {id: '', name: 'Secret', rank: 2}, + ], + }, + }); + + // # Seed an unrelated field that shares the same 'rank' type, to prove the + // predicate keys on name/object_type/group_id, not on type. + await createGlobalAttributeField(adminClient, unrelatedRankName, { + type: 'rank', + attrs: { + display_name: unrelatedRankDisplayName, + options: [ + {id: '', name: 'Low', rank: 1}, + {id: '', name: 'High', rank: 2}, + ], + }, + }); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + const classificationRow = systemConsolePage.page.locator('tr', { + has: systemConsolePage.page + .getByTestId('global-attribute-name') + .filter({hasText: classificationDisplayName}), + }); + await classificationRow.waitFor(); + + // * The read-only subtitle renders under the attribute name + await expect(classificationRow.getByText('Read-only')).toBeVisible(); + + // * The Source column identifies this row's true source, not the generic + // "Managed here" every other native field gets + await expect(classificationRow.getByTestId('global-attribute-source')).toContainText( + 'Classification Markings', + ); + + // * The rightmost cell is an open-in-new link to the Classification Markings + // admin page, not the dot-menu action trigger + const openInNewLink = classificationRow.getByRole('link', {name: 'Open Classification Markings'}); + await expect(openInNewLink).toBeVisible(); + await expect(openInNewLink).toHaveAttribute('href', CLASSIFICATION_MARKINGS_ADMIN_PATH); + await expect(classificationRow.getByRole('button', {name: 'More actions'})).toHaveCount(0); + + // # Clicking the link actually navigates to the Classification Markings page + await openInNewLink.click(); + // * Navigation lands on the Classification Markings admin page + await expect(systemConsolePage.page).toHaveURL(new RegExp(CLASSIFICATION_MARKINGS_ADMIN_PATH)); + + await systemConsolePage.page.goto(GLOBAL_ATTRIBUTES_ADMIN_PATH); + + // * An unrelated 'rank'-type field is unaffected: ordinary dot-menu, no subtitle + const unrelatedRow = systemConsolePage.page.locator('tr', { + has: systemConsolePage.page + .getByTestId('global-attribute-name') + .filter({hasText: unrelatedRankDisplayName}), + }); + await unrelatedRow.waitFor(); + await expect(unrelatedRow.getByRole('button', {name: 'More actions'})).toBeVisible(); + await expect(unrelatedRow.getByText('Read-only')).toHaveCount(0); + await expect(unrelatedRow.getByRole('link', {name: 'Open Classification Markings'})).toHaveCount(0); + await expect(unrelatedRow.getByTestId('global-attribute-source')).toContainText('Managed here'); + } finally { + await deleteClassificationMarkingsFieldIfExists(adminClient); + await deleteGlobalAttributeFieldIfExists(adminClient, unrelatedRankName); + } + }, + ); + }); + + 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/webapp/channels/package.json b/webapp/channels/package.json index b53b255ec243..c28638aebfb7 100644 --- a/webapp/channels/package.json +++ b/webapp/channels/package.json @@ -75,7 +75,7 @@ "prop-types": "15.8.1", "react": "18.2.0", "react-beautiful-dnd": "13.1.1", - "react-bootstrap": "github:mattermost/react-bootstrap#c17701564a6f240a14419d94369936c380f917e5", + "react-bootstrap": "github:mattermost/react-bootstrap#d693ec8082954f0e6d3e826ca6161979b29001cd", "react-color": "2.19.3", "react-day-picker": "8.3.6", "react-dom": "18.2.0", diff --git a/webapp/channels/src/components/actions_menu/__snapshots__/actions_menu.test.tsx.snap b/webapp/channels/src/components/actions_menu/__snapshots__/actions_menu.test.tsx.snap index f32a7be10b1a..ad0eb73b71a0 100644 --- a/webapp/channels/src/components/actions_menu/__snapshots__/actions_menu.test.tsx.snap +++ b/webapp/channels/src/components/actions_menu/__snapshots__/actions_menu.test.tsx.snap @@ -18,32 +18,34 @@ exports[`components/actions_menu/ActionsMenu has actions - marketplace disabled class="icon icon-apps" /> -
- + + Some text + + + + +
@@ -67,66 +69,68 @@ exports[`components/actions_menu/ActionsMenu has actions - marketplace enabled a class="icon icon-apps" /> -
- + + + +
diff --git a/webapp/channels/src/components/add_groups_to_channel_modal/add_groups_to_channel_modal.tsx b/webapp/channels/src/components/add_groups_to_channel_modal/add_groups_to_channel_modal.tsx index d0419e02a017..80030629fce1 100644 --- a/webapp/channels/src/components/add_groups_to_channel_modal/add_groups_to_channel_modal.tsx +++ b/webapp/channels/src/components/add_groups_to_channel_modal/add_groups_to_channel_modal.tsx @@ -188,7 +188,7 @@ export class AddGroupsToChannelModal extends React.PureComponent { return (
onAdd(option)} onMouseMove={() => (onMouseMove ? onMouseMove(option) : undefined)} diff --git a/webapp/channels/src/components/add_groups_to_team_modal/add_groups_to_team_modal.tsx b/webapp/channels/src/components/add_groups_to_team_modal/add_groups_to_team_modal.tsx index ba8964fa444f..46d1c489dda0 100644 --- a/webapp/channels/src/components/add_groups_to_team_modal/add_groups_to_team_modal.tsx +++ b/webapp/channels/src/components/add_groups_to_team_modal/add_groups_to_team_modal.tsx @@ -200,7 +200,7 @@ export class AddGroupsToTeamModal extends React.PureComponent { return (
onAdd(option)} onMouseMove={() => onMouseMove(option)} diff --git a/webapp/channels/src/components/add_users_to_team_modal/add_users_to_team_modal.tsx b/webapp/channels/src/components/add_users_to_team_modal/add_users_to_team_modal.tsx index 16ba32fd04f7..40e379babcc0 100644 --- a/webapp/channels/src/components/add_users_to_team_modal/add_users_to_team_modal.tsx +++ b/webapp/channels/src/components/add_users_to_team_modal/add_users_to_team_modal.tsx @@ -176,7 +176,7 @@ export class AddUsersToTeamModal extends React.PureComponent { return (
onAdd(option)} diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index a15ddb3b77d9..acc04a91dcf8 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/webapp/channels/src/components/admin_console/admin_definition.tsx @@ -718,6 +718,18 @@ const AdminDefinition: AdminDefinitionType = { component: BoardAttributes, }, }, + global_attribute_details_edit: { + url: `system_attributes/manage_attributes/attribute_details/:field_id(${ID_PATH_PATTERN})`, + isHidden: it.not(it.all( + it.minLicenseTier(LicenseSkus.Enterprise), + it.configIsTrue('FeatureFlags', 'GlobalAttributes'), + )), + isDisabled: it.not(it.isSystemAdmin), + schema: { + id: 'GlobalAttributeDetails', + component: AttributeDetails, + }, + }, global_attribute_details: { url: 'system_attributes/manage_attributes/attribute_details', isHidden: it.not(it.all( @@ -778,6 +790,7 @@ const AdminDefinition: AdminDefinitionType = { { type: 'bool', key: 'AccessControlSettings.EnableAccessControlAuditLogging', + isHidden: true, // TODO: Remove when the PR#37771 is merged label: defineMessage({id: 'admin.accesscontrol.enableAuditLogging.title', defaultMessage: 'Enable audit logging for access control decisions'}), help_text: defineMessage({id: 'admin.accesscontrol.enableAuditLogging.desc', defaultMessage: 'When enabled, attribute-based access control policy decisions are written to the server audit log. Requires server audit logging to be active.'}), disabled_help_text: defineMessage({id: 'admin.accesscontrol.enableAuditLogging.disabled', defaultMessage: 'When enabled, attribute-based access control policy decisions are written to the server audit log. This setting requires attribute-based access control to be enabled and server audit logging to be active (enable file audit logging or configure an advanced audit logging target).'}), diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.scss b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.scss index d8b1a517cc73..10343104f934 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.scss +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.scss @@ -46,6 +46,11 @@ } } +.AttributeDetails__typeLockWrap { + display: inline-flex; + max-width: 100%; +} + .AttributeDetails__blockTitle { margin: 0; color: var(--center-channel-color); diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.test.tsx b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.test.tsx index 158f8faf5bec..566c3598deed 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.test.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.test.tsx @@ -1,7 +1,9 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import {createMemoryHistory} from 'history'; import React from 'react'; +import {Route} from 'react-router-dom'; import {ClientError} from '@mattermost/client'; import type {PropertyField} from '@mattermost/types/properties'; @@ -752,6 +754,18 @@ describe('AttributeDetails', () => { expect(screen.getByTestId('attributeTypeMenuButton')).toHaveTextContent('Select'); }); + it('shows a dedicated tooltip for the external-source type lock, not the accessible name', async () => { + renderComponent(); + await linkViaMenu(/AD\/LDAP/, 'employeeID'); + + expect(screen.getByTestId('attributeTypeMenuButton')).toHaveAccessibleName('Type: Text. Locked while linked to an external source.'); + + await userEvent.hover(screen.getByTestId('attributeTypeLockWrap')); + const tooltip = await screen.findByRole('tooltip', {}, {timeout: 1000}); + expect(tooltip).toHaveTextContent('Type cannot be changed while this attribute is linked to an external source.'); + expect(tooltip).not.toHaveTextContent('Type: Text.'); + }); + it('re-saving a linked chip with the unchanged value is a no-op -- no extra dirty-marking dispatch', async () => { renderComponent(); @@ -974,6 +988,18 @@ describe('AttributeDetails', () => { await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); }); + it('does not lock Type while unsaved Applies-to resources sit on a create form', async () => { + renderComponent(); + await addResource('Users', 'user'); + + expect(screen.getByTestId('attributeTypeMenuButton')).not.toBeDisabled(); + expect(screen.queryByTestId('attributeTypeLockWrap')).not.toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('attributeTypeMenuButton')); + await userEvent.click(screen.getByRole('menuitemradio', {name: 'Select'})); + expect(screen.getByTestId('attributeTypeMenuButton')).toHaveTextContent('Select'); + }); + it('calls markDirty (navigation-blocked, error cleared) when a resource is added', async () => { jest.spyOn(Client4, 'createPropertyField').mockRejectedValue(makeClientError('app.property_field.create.name_conflict.app_error', 'already exists')); @@ -1087,4 +1113,453 @@ describe('AttributeDetails', () => { expect(mockSetNavigationBlocked).not.toHaveBeenCalledWith(false); }); }); + + describe('edit attribute', () => { + const FIELD_ID = 'abcdefghijklmnopqrstuvwxyz'; + + const addResource = async (label: string, resourceType: string) => { + await userEvent.click(screen.getByTestId('attributeAppliesToAddResourceButtonHeader')); + await userEvent.click(screen.getByRole('menuitem', {name: label})); + await waitFor(() => expect(screen.getByTestId(`attributeAppliesToRow-${resourceType}`)).toBeInTheDocument()); + }; + + function makeTemplate(overrides: Partial = {}): PropertyField { + return { + id: FIELD_ID, + name: 'department', + type: 'text', + group_id: 'accesscontrolgroupuuid001', + object_type: 'template', + target_id: '', + target_type: 'system', + create_at: 1, + update_at: 1, + delete_at: 0, + created_by: '', + updated_by: '', + attrs: {display_name: 'Department'}, + ...overrides, + } as PropertyField; + } + + function makeLinked(objectType: 'user' | 'channel' | 'post', id: string): PropertyField { + return { + id, + name: 'department', + type: 'text', + group_id: 'accesscontrolgroupuuid001', + object_type: objectType, + target_id: '', + target_type: 'system', + linked_field_id: FIELD_ID, + create_at: 1, + update_at: 1, + delete_at: 0, + created_by: '', + updated_by: '', + attrs: {display_name: 'Department'}, + } as PropertyField; + } + + function mockLoadedField(field: PropertyField, linked: PropertyField[] = []) { + jest.spyOn(Client4, 'getPropertyFields').mockImplementation((_group, objectType) => { + if (objectType === 'template') { + return Promise.resolve([field]); + } + return Promise.resolve(linked.filter((linkedField) => linkedField.object_type === objectType)); + }); + } + + const renderEdit = () => renderWithContext( +
+ + + + +
, + {}, + { + history: createMemoryHistory({ + initialEntries: [`/admin_console/system_attributes/manage_attributes/attribute_details/${FIELD_ID}`], + }), + }, + ); + + const waitForForm = () => waitFor(() => expect(screen.getByTestId('attributeDetails')).toBeInTheDocument()); + + it('prefills Definition and Applies-to from the loaded field without marking the page dirty', async () => { + mockLoadedField(makeTemplate({ + type: 'select', + attrs: { + display_name: 'Department', + options: [{id: 'opt-1', name: 'Engineering'}], + ldap: 'dept', + }, + }), [makeLinked('user', 'user-field')]); + + renderEdit(); + await waitForForm(); + + expect(screen.getByRole('heading', {name: 'Edit attribute'})).toBeInTheDocument(); + expect(screen.getByTestId('attributeDisplayNameInput')).toHaveValue('Department'); + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + expect(screen.getByTestId('attributeTypeMenuButton')).toHaveTextContent('Select'); + expect(screen.getByTestId('attributeAppliesToRow-user')).toBeInTheDocument(); + expect(screen.getByTestId('saveSetting')).toBeDisabled(); + expect(mockSetNavigationBlocked).not.toHaveBeenCalled(); + }); + + it('does not move focus to an Applies-to row when loading a field that already applies to every resource type', async () => { + mockLoadedField(makeTemplate(), [ + makeLinked('user', 'user-field'), + makeLinked('channel', 'channel-field'), + makeLinked('post', 'post-field'), + ]); + + renderEdit(); + await waitForForm(); + + await waitFor(() => expect(screen.getByTestId('attributeDisplayNameInput')).toHaveFocus()); + expect(screen.getByTestId('attributeAppliesToRow-post-toggle')).not.toHaveFocus(); + }); + + it('does not re-slug Unique name when Display name changes', async () => { + mockLoadedField(makeTemplate()); + renderEdit(); + await waitForForm(); + + await userEvent.clear(screen.getByTestId('attributeDisplayNameInput')); + await userEvent.type(screen.getByTestId('attributeDisplayNameInput'), 'New Label'); + + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + }); + + it('does not unpin Unique name after a no-op Edit/Done round-trip, even when the loaded name matches the current auto-slug', async () => { + // 'department' is the auto-slug of 'Department' -- the loaded name and + // the auto-derived one coincide, which is exactly what triggered the + // regression: Done's "did the committed value change from the + // auto-slug" heuristic must not unpin an edit-mode name just because + // of that coincidence. + mockLoadedField(makeTemplate()); + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeNameEditLink')); + await userEvent.click(screen.getByTestId('attributeNameEditLink')); + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + + await userEvent.clear(screen.getByTestId('attributeDisplayNameInput')); + await userEvent.type(screen.getByTestId('attributeDisplayNameInput'), 'Cost Centre'); + + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + }); + + it('does not unpin Unique name after a no-op Edit/Escape round-trip', async () => { + mockLoadedField(makeTemplate()); + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeNameEditLink')); + await userEvent.keyboard('{Escape}'); + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + + await userEvent.clear(screen.getByTestId('attributeDisplayNameInput')); + await userEvent.type(screen.getByTestId('attributeDisplayNameInput'), 'Cost Centre'); + + expect(screen.getByTestId('attributeUniqueNameValue')).toHaveTextContent('department'); + }); + + it('skips name validation while Unique name is unchanged, then validates once it changes', async () => { + mockLoadedField(makeTemplate({name: 'for', attrs: {display_name: 'For'}})); + renderEdit(); + await waitForForm(); + + expect(screen.queryByTestId('attributeUniqueNameError')).not.toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('attributeNameEditLink')); + const nameInput = screen.getByTestId('attributeNameInput'); + await userEvent.clear(nameInput); + await userEvent.type(nameInput, 'if'); + + expect(screen.getByTestId('attributeUniqueNameError')).toHaveTextContent('reserved word'); + }); + + it('PATCHes the existing field and does not POST a new template', async () => { + mockLoadedField(makeTemplate()); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate()); + const createPropertyField = jest.spyOn(Client4, 'createPropertyField'); + + renderEdit(); + await waitForForm(); + + await userEvent.type(screen.getByTestId('attributeDisplayNameInput'), ' 2'); + await userEvent.click(screen.getByTestId('saveSetting')); + + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/manage_attributes')); + expect(createPropertyField).not.toHaveBeenCalled(); + expect(patchPropertyField).toHaveBeenCalledWith('access_control', 'template', FIELD_ID, expect.objectContaining({ + type: 'text', + attrs: expect.objectContaining({display_name: 'Department 2'}), + })); + expect(patchPropertyField.mock.calls[0][3]).not.toHaveProperty('name'); + }); + + it('keeps existing option IDs on PATCH and sends an empty id for newly added options', async () => { + mockLoadedField(makeTemplate({ + type: 'select', + attrs: { + display_name: 'Department', + options: [{id: 'opt-1', name: 'Engineering'}], + }, + })); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate({type: 'select'})); + + renderEdit(); + await waitForForm(); + + await userEvent.type(screen.getByTestId('attributeOptionsValues__addInput'), 'Sales{Enter}'); + await userEvent.click(screen.getByTestId('saveSetting')); + + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); + expect(patchPropertyField).toHaveBeenCalledWith('access_control', 'template', FIELD_ID, expect.objectContaining({ + attrs: expect.objectContaining({ + options: [{id: 'opt-1', name: 'Engineering'}, {id: '', name: 'Sales'}], + }), + })); + }); + + it('locks Type while a pending Applies-to resource is on the form, and unlocks after local remove-all', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + renderEdit(); + await waitForForm(); + + expect(screen.getByTestId('attributeTypeMenuButton')).toBeDisabled(); + expect(screen.getByTestId('attributeTypeLockWrap')).toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + expect(screen.getByTestId('attributeTypeMenuButton')).not.toBeDisabled(); + expect(screen.queryByTestId('attributeTypeLockWrap')).not.toBeInTheDocument(); + }); + + it('locks adding an external source while a pending Applies-to resource is on the form, so linking cannot silently change the type', async () => { + mockLoadedField(makeTemplate({type: 'select', attrs: {display_name: 'Department', options: [{id: 'opt-1', name: 'Engineering'}]}}), [makeLinked('user', 'user-field')]); + renderEdit(); + await waitForForm(); + + expect(screen.getByTestId('attributeExternalSourceTrigger')).toBeDisabled(); + expect(screen.getByTestId('attributeExternalSourceTriggerLockWrap')).toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + expect(screen.getByTestId('attributeExternalSourceTrigger')).not.toBeDisabled(); + expect(screen.queryByTestId('attributeExternalSourceTriggerLockWrap')).not.toBeInTheDocument(); + }); + + it('locks Name editing while the attribute is currently applied to a resource', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + renderEdit(); + await waitForForm(); + + expect(screen.getByTestId('attributeNameEditLink')).toBeDisabled(); + expect(screen.getByTestId('attributeNameEditLinkLockWrap')).toBeInTheDocument(); + }); + + it('does not DELETE a persisted resource until Save, then DELETEs only removed types', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field'), makeLinked('channel', 'channel-field')]); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField').mockResolvedValue({status: 'OK'}); + jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate()); + const createPropertyField = jest.spyOn(Client4, 'createPropertyField'); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + expect(deletePropertyField).not.toHaveBeenCalled(); + + await userEvent.click(screen.getByTestId('saveSetting')); + await userEvent.click(await screen.findByRole('button', {name: /remove and save/i})); + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); + + expect(deletePropertyField).toHaveBeenCalledTimes(1); + expect(deletePropertyField).toHaveBeenCalledWith('access_control', 'user', 'user-field'); + expect(createPropertyField).not.toHaveBeenCalled(); + }); + + it('aborts the save without deleting anything when the remove-applies-to confirmation is declined', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField').mockResolvedValue({status: 'OK'}); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField'); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + + await userEvent.click(screen.getByTestId('saveSetting')); + await userEvent.click(await screen.findByRole('button', {name: /^cancel$/i})); + + expect(deletePropertyField).not.toHaveBeenCalled(); + expect(patchPropertyField).not.toHaveBeenCalled(); + expect(mockHistoryPush).not.toHaveBeenCalled(); + expect(screen.getByTestId('saveSetting')).toBeEnabled(); + }); + + it('POSTs only newly added resource types and skips types that were already persisted', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate()); + const createPropertyField = jest.spyOn(Client4, 'createPropertyField').mockResolvedValue({id: 'channel-field'} as PropertyField); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField'); + + renderEdit(); + await waitForForm(); + await addResource('Channels', 'channel'); + await userEvent.click(screen.getByTestId('saveSetting')); + + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); + expect(createPropertyField).toHaveBeenCalledTimes(1); + expect(createPropertyField).toHaveBeenCalledWith('access_control', 'channel', expect.objectContaining({ + linked_field_id: FIELD_ID, + })); + expect(deletePropertyField).not.toHaveBeenCalled(); + }); + + it('issues neither DELETE nor POST when a persisted resource is removed then re-added before Save', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate()); + const createPropertyField = jest.spyOn(Client4, 'createPropertyField'); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField'); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + await addResource('Users', 'user'); + + await userEvent.click(screen.getByTestId('saveSetting')); + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); + + expect(deletePropertyField).not.toHaveBeenCalled(); + expect(createPropertyField).not.toHaveBeenCalled(); + }); + + it('DELETEs removed resources before PATCHing when type also changes', async () => { + const callOrder: string[] = []; + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField').mockImplementation(async () => { + callOrder.push('delete'); + return {status: 'OK'}; + }); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockImplementation(async () => { + callOrder.push('patch'); + return makeTemplate({type: 'select'}); + }); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.getByTestId('attributeTypeMenuButton')).not.toBeDisabled()); + + await userEvent.click(screen.getByTestId('attributeTypeMenuButton')); + await userEvent.click(screen.getByRole('menuitemradio', {name: 'Select'})); + await userEvent.type(screen.getByTestId('attributeOptionsValues__addInput'), 'Engineering{Enter}'); + await userEvent.click(screen.getByTestId('saveSetting')); + await userEvent.click(await screen.findByRole('button', {name: /remove and save/i})); + + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalled()); + expect(deletePropertyField).toHaveBeenCalled(); + expect(patchPropertyField).toHaveBeenCalled(); + expect(callOrder).toEqual(['delete', 'patch']); + }); + + it('PATCHes before DELETEing when the type has not changed, and does not delete on PATCH failure', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + const callOrder: string[] = []; + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField').mockImplementation(async () => { + callOrder.push('delete'); + return {status: 'OK'}; + }); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockImplementation(async () => { + callOrder.push('patch'); + throw makeClientError('app.property_field.update.name_conflict.app_error'); + }); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + + await userEvent.click(screen.getByTestId('saveSetting')); + await userEvent.click(await screen.findByRole('button', {name: /remove and save/i})); + + expect(await screen.findByTestId('attributeSaveError')).toBeInTheDocument(); + expect(patchPropertyField).toHaveBeenCalledTimes(1); + expect(deletePropertyField).not.toHaveBeenCalled(); + expect(callOrder).toEqual(['patch']); + expect(mockHistoryPush).not.toHaveBeenCalled(); + }); + + it('reports a partial-save error, not "nothing else was saved", when the PATCH succeeds but the DELETE fails', async () => { + mockLoadedField(makeTemplate(), [makeLinked('user', 'user-field')]); + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue(makeTemplate()); + const deletePropertyField = jest.spyOn(Client4, 'deletePropertyField').mockRejectedValue(new Error('delete failed')); + + renderEdit(); + await waitForForm(); + + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-toggle')); + await userEvent.click(screen.getByTestId('attributeAppliesToRow-user-remove')); + await waitFor(() => expect(screen.queryByTestId('attributeAppliesToRow-user')).not.toBeInTheDocument()); + + await userEvent.click(screen.getByTestId('saveSetting')); + await userEvent.click(await screen.findByRole('button', {name: /remove and save/i})); + + expect(patchPropertyField).toHaveBeenCalledTimes(1); + expect(deletePropertyField).toHaveBeenCalledTimes(1); + expect(mockHistoryPush).not.toHaveBeenCalled(); + + const banner = await screen.findByTestId('attributeSaveError'); + expect(banner).toHaveTextContent('The attribute was saved'); + expect(banner).not.toHaveTextContent('Nothing else was saved'); + }); + + it('redirects to the listing when the field is plugin-owned', async () => { + mockLoadedField(makeTemplate({ + attrs: {display_name: 'Plugin field', source_plugin_id: 'com.example.plugin', protected: true}, + })); + + renderEdit(); + + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/manage_attributes')); + expect(screen.queryByTestId('attributeDetails')).not.toBeInTheDocument(); + }); + + it('redirects to the listing when the field is Classification Markings', async () => { + mockLoadedField(makeTemplate({name: 'classification'})); + renderEdit(); + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/manage_attributes')); + expect(screen.queryByTestId('attributeDetails')).not.toBeInTheDocument(); + }); + + it('redirects to the listing when the field is missing', async () => { + jest.spyOn(Client4, 'getPropertyFields').mockResolvedValue([]); + renderEdit(); + await waitFor(() => expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/manage_attributes')); + expect(screen.queryByTestId('attributeDetails')).not.toBeInTheDocument(); + }); + }); }); diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.tsx b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.tsx index fc68f7441205..2646dabf0b2d 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_details.tsx @@ -6,9 +6,11 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import type {IntlShape} from 'react-intl'; import {defineMessages, FormattedMessage, useIntl} from 'react-intl'; import {useDispatch} from 'react-redux'; +import {useParams} from 'react-router-dom'; import type {ClientError} from '@mattermost/client'; import {buttonClassNames} from '@mattermost/shared/components/button'; +import {WithTooltip} from '@mattermost/shared/components/tooltip'; import type {PropertyField, PropertyFieldOption} from '@mattermost/types/properties'; import {supportsOptions} from '@mattermost/types/properties'; @@ -17,6 +19,7 @@ import {setNavigationBlocked} from 'actions/admin_actions'; import BlockableLink from 'components/admin_console/blockable_link'; import {findRankCollision, isValidRank} from 'components/admin_console/system_properties/rank_utils'; import Card from 'components/card/card'; +import LoadingScreen from 'components/loading_screen'; import * as Menu from 'components/menu'; import SaveButton from 'components/save_button'; import AdminHeader from 'components/widgets/admin_console/admin_header'; @@ -34,15 +37,26 @@ import AttributeExternalSource from './attribute_external_source'; import type {ExternalSource} from './attribute_external_source'; import AttributeOptionsRankValues from './attribute_options_rank_values'; import AttributeOptionsValues from './attribute_options_values'; +import {useConfirmRemoveAppliesTo} from './attribute_remove_applies_to_warning_modal'; -import {getTypeIcon, getTypeLabel, typeLabels} from '../global_attributes_table'; +import {GLOBAL_ATTRIBUTES_LIST_ROUTE} from '../constants'; +import {getSourceKind, getTypeIcon, getTypeLabel, isClassificationMarkingsField, typeLabels} from '../global_attributes_table'; import type {AttributeFieldType} from '../utils'; -import {createAttributeField, createLinkedAttributeField, deleteAttributeField, deleteLinkedAttributeField} from '../utils'; +import { + createAttributeField, + createLinkedAttributeField, + deleteAttributeField, + deleteLinkedAttributeField, + fetchAttributeField, + fetchLinkedFieldsForTemplate, + linkedFieldsByResourceType, + updateAttributeField, +} from '../utils'; import './attribute_details.scss'; const ALL_TYPES: AttributeFieldType[] = ['text', 'select', 'multiselect', 'rank']; -const LIST_ROUTE = '/admin_console/system_attributes/manage_attributes'; +const LIST_ROUTE = GLOBAL_ATTRIBUTES_LIST_ROUTE; // Whether any option in `options` has a name equal to another option's name. // Used defensively by canSave -- both options editors already block this @@ -66,9 +80,22 @@ function hasValidRanks(options: PropertyFieldOption[]): boolean { return options.every((option, index) => isValidRank(option.rank) && !findRankCollision(options, option.rank as number, index)); } -// Single source of truth for this page's own route, so the "New attribute" -// button in global_attributes.tsx doesn't hardcode a second copy of this path. -export const ATTRIBUTE_DETAILS_ROUTE = `${LIST_ROUTE}/attribute_details`; +function isAttributeFieldType(value: string): value is AttributeFieldType { + return (ALL_TYPES as string[]).includes(value); +} + +function optionsFromField(field: PropertyField): PropertyFieldOption[] { + const raw = field.attrs?.options; + if (!Array.isArray(raw)) { + return []; + } + return raw.filter((option): option is PropertyFieldOption => ( + typeof option === 'object' && + option !== null && + typeof (option as PropertyFieldOption).id === 'string' && + typeof (option as PropertyFieldOption).name === 'string' + )); +} // Mirrors CPA's own computeAutoFillSlug guard (user_properties_table.tsx): a // derived slug of '_copy' (slugifyForCEL's empty-input sentinel) means there is @@ -90,7 +117,11 @@ type ErrorKind = 'limit_reached' | 'invalid_options' | 'generic' | + 'type_change_with_dependents' | 'applies_to_failed' | + 'applies_to_remove_failed' | + 'applies_to_remove_partial_save' | + 'applies_to_partial_save' | 'applies_to_rollback_failed' | 'applies_to_name_conflict' | 'applies_to_limit_reached'; @@ -99,6 +130,7 @@ function errorKindFromError(error: unknown): ErrorKind { const serverErrorId = (error as ClientError | undefined)?.server_error_id; switch (serverErrorId) { case 'app.property_field.create.name_conflict.app_error': + case 'app.property_field.update.name_conflict.app_error': return 'name_conflict'; case 'model.cpa_field.name.invalid_charset.app_error': return 'invalid_charset'; @@ -109,6 +141,8 @@ function errorKindFromError(error: unknown): ErrorKind { return 'limit_reached'; case 'app.property_field.invalid_attrs.app_error': return 'invalid_options'; + case 'app.property_field.update.type_change_with_dependents.app_error': + return 'type_change_with_dependents'; default: return 'generic'; } @@ -228,7 +262,11 @@ type Props = { function AttributeDetails({disabled = false}: Props): JSX.Element { const dispatch = useDispatch(); const {formatMessage} = useIntl(); + const {field_id: fieldId} = useParams<{field_id?: string}>(); + const isEditMode = Boolean(fieldId); + const [loading, setLoading] = useState(isEditMode); + const [isDirty, setIsDirty] = useState(false); const [displayName, setDisplayName] = useState(''); const [manualName, setManualName] = useState(''); @@ -268,6 +306,20 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { // Posts order (that fixed order only governs the picker's own offer list). const [appliesTo, setAppliesTo] = useState([]); + // Loaded linked fields, keyed by resource type. Add/Remove only mutate + // appliesTo; Save diffs against this snapshot. Updated in place after each + // successful DELETE/POST so a retry does not repeat a completed step. + const persistedLinkedFieldsRef = useRef>>({}); + const originalNameRef = useRef(''); + + // Compared against the live fieldType at Save time to pick which order + // DELETE/PATCH run in (see handleSave) -- the server rejects a type-changing + // PATCH while linked fields of the old type still exist + // (type_change_with_dependents), so that case must delete first, but doing + // so unconditionally would delete linked values on a PATCH failure that has + // nothing to do with type (e.g. a name conflict). + const originalFieldTypeRef = useRef('text'); + const [saving, setSaving] = useState(false); const [errorKind, setErrorKind] = useState(null); @@ -293,9 +345,66 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { isMountedRef.current = false; }, []); + useEffect(() => { + if (!fieldId) { + return undefined; + } + + let cancelled = false; + + const load = async () => { + try { + const field = await fetchAttributeField(fieldId); + if (cancelled) { + return; + } + if ( + !field || + getSourceKind(field) === 'plugin' || + isClassificationMarkingsField(field, field.group_id) + ) { + getHistory().push(LIST_ROUTE); + return; + } + + const linkedFields = await fetchLinkedFieldsForTemplate(fieldId); + if (cancelled) { + return; + } + + const linkedByType = linkedFieldsByResourceType(linkedFields); + persistedLinkedFieldsRef.current = linkedByType; + originalNameRef.current = field.name; + const loadedFieldType = isAttributeFieldType(field.type) ? field.type : 'text'; + originalFieldTypeRef.current = loadedFieldType; + + setDisplayName((field.attrs?.display_name as string | undefined) || ''); + setManualName(field.name); + setIsNameManuallyEdited(true); + setFieldType(loadedFieldType); + setOptions(optionsFromField(field)); + setLdapAttr(typeof field.attrs?.ldap === 'string' ? field.attrs.ldap : ''); + setSamlAttr(typeof field.attrs?.saml === 'string' ? field.attrs.saml : ''); + setAppliesTo(ALL_RESOURCE_TYPES.filter((type) => Boolean(linkedByType[type]))); + setLoading(false); + } catch { + if (!cancelled) { + getHistory().push(LIST_ROUTE); + } + } + }; + + load(); + + return () => { + cancelled = true; + }; + }, [fieldId]); + const autoSlugDisplay = useMemo(() => computeAutoSlugDisplay(displayName), [displayName]); const currentName = (isEditingName || isNameManuallyEdited) ? manualName : (autoSlugDisplay ?? ''); - const nameValidationError = currentName ? validateCPAFieldName(currentName) : null; + const nameUnchanged = isEditMode && currentName === originalNameRef.current; + const nameValidationError = (!nameUnchanged && currentName) ? validateCPAFieldName(currentName) : null; // Kept as a primitive rather than passing nameValidationError itself into // handleDoneClick's dep array -- the error object is rebuilt on every @@ -326,6 +435,7 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { } const markDirty = useCallback(() => { + setIsDirty(true); dispatch(setNavigationBlocked(true)); setErrorKind(null); setServerErrorMessage(null); @@ -366,8 +476,22 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { // just-added row's own toggle keeps focus on a real, newly-rendered // element instead. Looked up by data-testid (not id) since the row // doesn't otherwise need a stable element id. + // + // Create starts hydrated. Edit skips the first populated commit so + // loading all three resource types does not look like "just added the + // last type" and steal autoFocus from Display name. const prevAppliesToLengthRef = useRef(appliesTo.length); + const appliesToHydratedRef = useRef(!isEditMode); useEffect(() => { + if (loading) { + prevAppliesToLengthRef.current = appliesTo.length; + return; + } + if (!appliesToHydratedRef.current) { + appliesToHydratedRef.current = true; + prevAppliesToLengthRef.current = appliesTo.length; + return; + } const prevLength = prevAppliesToLengthRef.current; if (appliesTo.length < prevLength) { document.getElementById(ATTRIBUTE_APPLIES_TO_ADD_HEADER_TRIGGER_ID)?.focus(); @@ -376,7 +500,7 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { document.querySelector(`[data-testid="attributeAppliesToRow-${lastAddedType}-toggle"]`)?.focus(); } prevAppliesToLengthRef.current = appliesTo.length; - }, [appliesTo]); + }, [appliesTo, loading]); // Switching into Rank from any other type (re)assigns rank = index + 1 to // every current option, overwriting any stale rank values -- mirrors CPA's @@ -457,10 +581,18 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { setIsNameManuallyEdited(false); } } else { - setIsNameManuallyEdited(manualName !== (autoSlugDisplay ?? '')); + // In create mode, a committed value that happens to match the + // current auto-slug keeps live derivation on (see the "no-op + // keeps derivation" tests). In edit mode this must never flip + // isNameManuallyEdited to false: the loaded Name is a persisted + // identifier, not a derivation, and Done on an unchanged/no-op + // edit session must not silently unpin it just because it + // happens to coincide with what the current Display name would + // slugify to (see the "does not re-slug on edit" regression). + setIsNameManuallyEdited(isEditMode || manualName !== (autoSlugDisplay ?? '')); } setIsEditingName(false); - }, [hasNameError, manualName, isNameManuallyEdited, autoSlugDisplay]); + }, [hasNameError, manualName, isNameManuallyEdited, autoSlugDisplay, isEditMode]); const handleNameChange = useCallback((e: React.ChangeEvent) => { setManualName(filterCELIdentifier(e.target.value)); @@ -487,8 +619,19 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { }, [handleDoneClick, handleCancelEdit]); const hasExternalSource = Boolean(ldapAttr || samlAttr); + const typeLockedByAppliesTo = isEditMode && appliesTo.length > 0; + const typeLocked = hasExternalSource || typeLockedByAppliesTo; + const typeChanged = isEditMode && fieldType !== originalFieldTypeRef.current; const typeSupportsOptions = supportsOptions({type: fieldType} as PropertyField); + // Unique name is the identifier policies and integrations bind to, and the + // server does not copy it onto linked fields. Renaming while any resource + // application is persisted would leave those fields on the old identifier, + // so the rename is locked until they are removed and saved. Keyed off the + // persisted snapshot, not appliesTo, so a pending local remove doesn't + // unlock a rename the dependents wouldn't receive. + const nameLockedByAppliesTo = isEditMode && Object.keys(persistedLinkedFieldsRef.current).length > 0; + // Defensive re-check, not the primary guard: both options editors already // block duplicate names and invalid/duplicate ranks interactively at // add/rename time (see attribute_options_values.tsx / @@ -514,7 +657,9 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { return null; }, [typeSupportsOptions, options, fieldType]); - const canSave = !disabled && Boolean(displayName.trim()) && Boolean(currentName) && !nameValidationError && !saving && optionsIssue === null; + const canSave = !disabled && Boolean(displayName.trim()) && Boolean(currentName) && !nameValidationError && !saving && optionsIssue === null && (!isEditMode || isDirty); + + const confirmRemoveAppliesTo = useConfirmRemoveAppliesTo(); // Applies the fully-settled outcome of a save attempt -- the ONLY place in // handleSave that reads isMountedRef, checked once after the entire @@ -547,6 +692,105 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { setServerErrorMessage(null); setFailedResourceTypes(null); + if (isEditMode && fieldId) { + const persisted = persistedLinkedFieldsRef.current; + const toDelete = (Object.keys(persisted) as ResourceObjectType[]).filter((type) => !appliesTo.includes(type)); + const toCreate = appliesTo.filter((type) => !persisted[type]); + + // Removing a resource deletes every value stored under its linked + // field -- unbounded and irreversible -- so confirm before doing + // anything else. Declining aborts the whole save; nothing has been + // deleted or patched yet at this point. + if (toDelete.length > 0 && !(await confirmRemoveAppliesTo(toDelete))) { + setSaving(false); + return; + } + + // Deletes toDelete's linked fields, stopping (and reporting) at the + // first failure. Called from one of two positions below depending on + // typeChanged, never both -- errorKind differs by position because + // the two failures are not the same: before PATCH, nothing else has + // been saved yet; after PATCH has already succeeded, the template + // update is not lost, only the removal is. + const deleteRemovedLinkedFields = async (errorKind: 'applies_to_remove_failed' | 'applies_to_remove_partial_save'): Promise => { + for (const type of toDelete) { + const existing = persisted[type]; + if (!existing) { + continue; + } + try { + // eslint-disable-next-line no-await-in-loop + await deleteLinkedAttributeField(type, existing.id); + delete persistedLinkedFieldsRef.current[type]; + } catch { + finalizeSave({ + success: false, + errorKind, + serverErrorMessage: null, + failedResourceTypes: [type], + }); + return false; + } + } + return true; + }; + + // A type-changing PATCH 409s server-side while linked fields of the + // old type still exist (type_change_with_dependents), so that case + // requires DELETE before PATCH. But applying that order + // unconditionally means a PATCH failure that has nothing to do with + // type (e.g. a name conflict) would still have already deleted every + // linked value -- irreversibly, for a save that just failed. So when + // the type isn't changing, PATCH runs first and the delete only + // happens once it's known to succeed. + if (typeChanged && !(await deleteRemovedLinkedFields('applies_to_remove_failed'))) { + return; + } + + try { + await updateAttributeField(fieldId, { + ...(nameUnchanged ? {} : {name: currentName}), + type: fieldType, + displayName, + options, + ldapAttr, + samlAttr, + }); + } catch (error) { + finalizeSave({ + success: false, + errorKind: errorKindFromError(error), + serverErrorMessage: (error as ClientError | undefined)?.message ?? null, + failedResourceTypes: null, + }); + return; + } + + if (!typeChanged && !(await deleteRemovedLinkedFields('applies_to_remove_partial_save'))) { + return; + } + + for (const type of toCreate) { + try { + // eslint-disable-next-line no-await-in-loop + const linkedField = await createLinkedAttributeField(type, currentName, fieldType, displayName, fieldId); + persistedLinkedFieldsRef.current[type] = linkedField; + } catch (error) { + const cpaErrorKind = type === 'user' ? appliesToErrorKindFromError(error) : null; + finalizeSave({ + success: false, + errorKind: cpaErrorKind ?? 'applies_to_partial_save', + serverErrorMessage: null, + failedResourceTypes: [type], + }); + return; + } + } + + finalizeSave({success: true}); + return; + } + let templateField: PropertyField; try { templateField = await createAttributeField(displayName, currentName, fieldType, options, {ldapAttr, samlAttr}); @@ -578,9 +822,64 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { } finalizeSave(outcome); - }, [canSave, displayName, currentName, fieldType, options, ldapAttr, samlAttr, appliesTo, finalizeSave]); + }, [canSave, isEditMode, fieldId, nameUnchanged, displayName, currentName, fieldType, typeChanged, options, ldapAttr, samlAttr, appliesTo, finalizeSave, confirmRemoveAppliesTo]); const TypeIcon = getTypeIcon(fieldType); + const typeLockTooltip = hasExternalSource ? formatMessage(messages.typeLockedExternalSourceTooltip) : formatMessage(messages.typeLockedAppliesToTooltip); + let typeButtonAriaLabel = formatMessage(messages.typeFieldAriaLabel, {value: formatMessage(getTypeLabel(fieldType))}); + if (hasExternalSource) { + typeButtonAriaLabel = formatMessage(messages.typeFieldLockedAriaLabel); + } else if (typeLockedByAppliesTo) { + typeButtonAriaLabel = formatMessage(messages.typeFieldLockedAppliesToAriaLabel, {value: formatMessage(getTypeLabel(fieldType))}); + } + const typeMenu = ( + + + + + + {!typeLocked && ( + + )} + + ), + dataTestId: 'attributeTypeMenuButton', + }} + menu={{ + id: 'attribute-type-menu', + 'aria-label': formatMessage(messages.typeMenuAriaLabel), + }} + > + {ALL_TYPES.map((optionFieldType) => { + const ItemIcon = getTypeIcon(optionFieldType); + const isCurrentType = optionFieldType === fieldType; + + return ( + handleTypeChange(optionFieldType)} + leadingElement={} + labels={} + /> + ); + })} + + ); + + if (loading) { + return ; + } // The two applies_to_* kinds below that interpolate resource names need // their own copy path -- they can't go through the flat @@ -594,8 +893,8 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { // has no notion of "User Attribute" to say, since that framing is // specific to this feature's CPA-namespace overlap. let errorContent: React.ReactNode = null; - if (errorKind === 'applies_to_failed') { - errorContent = formatMessage(errorMessages.applies_to_failed, {resources: resourceTypeListLabel(failedResourceTypes ?? [], formatMessage)}); + if (errorKind === 'applies_to_failed' || errorKind === 'applies_to_remove_failed' || errorKind === 'applies_to_remove_partial_save' || errorKind === 'applies_to_partial_save') { + errorContent = formatMessage(errorMessages[errorKind], {resources: resourceTypeListLabel(failedResourceTypes ?? [], formatMessage)}); } else if (errorKind === 'applies_to_rollback_failed') { const resources = resourceTypeListLabel(failedResourceTypes ?? [], formatMessage); errorContent = resources ? formatMessage(errorMessages.applies_to_rollback_failed, { @@ -624,7 +923,7 @@ function AttributeDetails({disabled = false}: Props): JSX.Element {
)} - + {(() => { + const editLinkButton = ( + + ); + return nameLockedByAppliesTo ? ( + + + {editLinkButton} + + + ) : editLinkButton; + })()} {nameValidationError && (
- - - - - - {!hasExternalSource && ( - - )} - - ), - dataTestId: 'attributeTypeMenuButton', - }} - menu={{ - id: 'attribute-type-menu', - 'aria-label': formatMessage(messages.typeMenuAriaLabel), - }} - > - {ALL_TYPES.map((optionFieldType) => { - const ItemIcon = getTypeIcon(optionFieldType); - const isCurrentType = optionFieldType === fieldType; - - return ( - handleTypeChange(optionFieldType)} - leadingElement={} - labels={} - /> - ); - })} - + {typeLocked ? ( + + + {typeMenu} + + + ) : typeMenu}
@@ -859,6 +1137,7 @@ function AttributeDetails({disabled = false}: Props): JSX.Element { fieldType={fieldType} onLink={handleLink} disabled={saving || disabled} + disableAdding={typeLockedByAppliesTo} />
@@ -907,6 +1186,7 @@ export default AttributeDetails; const messages = defineMessages({ backLink: {id: 'admin.global_attributes.attribute_details.back_link', defaultMessage: 'Back to Manage Attributes'}, title: {id: 'admin.global_attributes.attribute_details.title', defaultMessage: 'New attribute'}, + editTitle: {id: 'admin.global_attributes.attribute_details.edit_title', defaultMessage: 'Edit attribute'}, subtitle: {id: 'admin.global_attributes.attribute_details.subtitle', defaultMessage: 'Add a display name, choose a type, and pick where it applies.'}, definitionTitle: {id: 'admin.global_attributes.attribute_details.definition.title', defaultMessage: 'Definition'}, definitionSubtitle: {id: 'admin.global_attributes.attribute_details.definition.subtitle', defaultMessage: 'Display name, type, and options.'}, @@ -917,6 +1197,14 @@ const messages = defineMessages({ editLinkAriaLabel: {id: 'admin.global_attributes.attribute_details.unique_name.edit_aria_label', defaultMessage: 'Edit unique name'}, doneLink: {id: 'admin.global_attributes.attribute_details.unique_name.done', defaultMessage: 'Done'}, doneLinkAriaLabel: {id: 'admin.global_attributes.attribute_details.unique_name.done_aria_label', defaultMessage: 'Done editing unique name'}, + nameLockedAppliesToAriaLabel: { + id: 'admin.global_attributes.attribute_details.unique_name.locked_applies_to_aria_label', + defaultMessage: 'Edit unique name. Locked while this attribute applies to a resource.', + }, + nameLockedAppliesToTooltip: { + id: 'admin.global_attributes.attribute_details.unique_name.locked_applies_to_tooltip', + defaultMessage: 'Name cannot be changed while this attribute applies to a resource. Remove and save first.', + }, helperText: { id: 'admin.global_attributes.attribute_details.unique_name.helper_text', defaultMessage: 'Name is the internal identifier for policies and integrations. Display name is what admins and users see.', @@ -929,6 +1217,18 @@ const messages = defineMessages({ typeMenuAriaLabel: {id: 'admin.global_attributes.attribute_details.type.menu_label', defaultMessage: 'Select type'}, typeFieldAriaLabel: {id: 'admin.global_attributes.attribute_details.type.field_aria_label', defaultMessage: 'Type: {value}'}, typeFieldLockedAriaLabel: {id: 'admin.global_attributes.attribute_details.type.field_locked_aria_label', defaultMessage: 'Type: Text. Locked while linked to an external source.'}, + typeFieldLockedAppliesToAriaLabel: { + id: 'admin.global_attributes.attribute_details.type.field_locked_applies_to_aria_label', + defaultMessage: 'Type: {value}. Locked while this attribute applies to a resource.', + }, + typeLockedAppliesToTooltip: { + id: 'admin.global_attributes.attribute_details.type.locked_applies_to_tooltip', + defaultMessage: 'Type cannot be changed while this attribute applies to a resource.', + }, + typeLockedExternalSourceTooltip: { + id: 'admin.global_attributes.attribute_details.type.locked_external_source_tooltip', + defaultMessage: 'Type cannot be changed while this attribute is linked to an external source.', + }, optionsLabel: {id: 'admin.global_attributes.attribute_details.options.label', defaultMessage: 'Options'}, optionsHelp: { id: 'admin.global_attributes.attribute_details.options.help', @@ -986,10 +1286,26 @@ const errorMessages = defineMessages({ id: 'admin.global_attributes.attribute_details.save_error.generic', defaultMessage: 'Something went wrong while saving this attribute. Please try again.', }, + type_change_with_dependents: { + id: 'admin.global_attributes.attribute_details.save_error.type_change_with_dependents', + defaultMessage: "This attribute's type can't be changed while it applies to a resource. Remove those resources first, then try again.", + }, applies_to_failed: { id: 'admin.global_attributes.attribute_details.save_error.applies_to_failed', defaultMessage: "Couldn't apply this attribute to {resources}. Nothing was saved — please try again.", }, + applies_to_remove_failed: { + id: 'admin.global_attributes.attribute_details.save_error.applies_to_remove_failed', + defaultMessage: "Couldn't remove this attribute from {resources}. Nothing else was saved — please try again.", + }, + applies_to_remove_partial_save: { + id: 'admin.global_attributes.attribute_details.save_error.applies_to_remove_partial_save', + defaultMessage: "The attribute was saved, but couldn't be removed from {resources}. Please try again.", + }, + applies_to_partial_save: { + id: 'admin.global_attributes.attribute_details.save_error.applies_to_partial_save', + defaultMessage: 'The attribute was saved, but couldn\'t be applied to {resources}. Please try again.', + }, applies_to_rollback_failed: { id: 'admin.global_attributes.attribute_details.save_error.applies_to_rollback_failed', defaultMessage: '"{name}" may have been partially created for {resources}. A retry under the same name will likely fail until those are cleaned up.', diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.test.tsx b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.test.tsx index fb7b06786957..ac730b451c9a 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.test.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.test.tsx @@ -43,6 +43,15 @@ describe('AttributeExternalSource', () => { expect(screen.getByRole('menuitem', {name: /^SAML/})).toBeInTheDocument(); }); + it('disables the add-source trigger when disableAdding is set, without disabling an existing chip\'s edit/remove actions', () => { + renderComponent({ldapAttr: 'department', disableAdding: true}); + + expect(screen.getByTestId('attributeExternalSourceTrigger')).toBeDisabled(); + expect(screen.getByTestId('attributeExternalSourceTriggerLockWrap')).toBeInTheDocument(); + expect(screen.getByTestId('attributeExternalSourceChip-ldap-edit')).not.toBeDisabled(); + expect(screen.getByTestId('attributeExternalSourceChip-ldap-remove')).not.toBeDisabled(); + }); + it('renders a chip for a linked source prefixed by Synced with, and offers only the remaining source', async () => { renderComponent({ldapAttr: 'department'}); diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.tsx b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.tsx index 3db10c1fabbd..8dfdc15a7c24 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/attribute_external_source.tsx @@ -9,6 +9,7 @@ import {components} from 'react-select'; import {PencilOutlineIcon, RefreshIcon, SyncIcon} from '@mattermost/compass-icons/components'; import {buttonClassNames} from '@mattermost/shared/components/button'; +import {WithTooltip} from '@mattermost/shared/components/tooltip'; import {openModal} from 'actions/views/modals'; @@ -34,13 +35,21 @@ type Props = { fieldType: AttributeFieldType; onLink: (source: ExternalSource, value: string) => void; disabled?: boolean; + + // Linking a new source forces fieldType to 'text' (see attribute_details.tsx's + // handleLink) -- while this attribute is applied to a resource, that would + // change its type out from under the server's type_change_with_dependents + // guard the same way the Type menu itself is locked for. Only gates the + // "add" trigger below: editing or removing an already-linked source never + // touches fieldType, so those stay enabled. + disableAdding?: boolean; }; function sourceValue(source: ExternalSource, ldapAttr: string, samlAttr: string): string { return source === 'ldap' ? ldapAttr : samlAttr; } -function AttributeExternalSource({ldapAttr, samlAttr, fieldType, onLink, disabled = false}: Props): JSX.Element { +function AttributeExternalSource({ldapAttr, samlAttr, fieldType, onLink, disabled = false, disableAdding = false}: Props): JSX.Element { const {formatMessage} = useIntl(); const dispatch = useDispatch(); @@ -156,41 +165,52 @@ function AttributeExternalSource({ldapAttr, samlAttr, fieldType, onLink, disable
)} {unlinkedSources.length > 0 && ( - - - - - - ), - dataTestId: 'attributeExternalSourceTrigger', - }} - menu={{ - id: 'attribute-external-source-menu', - 'aria-label': formatMessage(messages.triggerLabel), - }} - > - {unlinkedSources.map((source) => ( - } - onClick={() => openLinkModal(source)} - labels={( - <> - - - - )} - /> - ))} - + (() => { + const trigger = ( + + + + + + ), + dataTestId: 'attributeExternalSourceTrigger', + }} + menu={{ + id: 'attribute-external-source-menu', + 'aria-label': formatMessage(messages.triggerLabel), + }} + > + {unlinkedSources.map((source) => ( + } + onClick={() => openLinkModal(source)} + labels={( + <> + + + + )} + /> + ))} + + ); + return disableAdding ? ( + + + {trigger} + + + ) : trigger; + })() )} void; + onCancel: () => void; + onExited: () => void; +}; + +/** + * Confirms removing Applies-to resources from an existing attribute before + * Save runs. Removing a resource deletes its linked field, and with it every + * value any User/Channel/Post has stored under it -- an irreversible + * deletion. There is no client-facing endpoint to count how many values + * would be lost (see the plan's Decisions table), so the copy can only name + * what is at risk, not how many. + * + * Resolves the returned promise with false on both explicit Cancel and any + * other dismissal (backdrop click, Esc) -- handleSave treats "not confirmed" + * as "don't delete anything" either way, so there is no third outcome to + * distinguish. + */ +export const useConfirmRemoveAppliesTo = () => { + const dispatch = useDispatch(); + + return (resourceTypes: ResourceObjectType[]): Promise => { + return new Promise((resolve) => { + let resolved = false; + const resolveOnce = (value: boolean) => { + if (!resolved) { + resolved = true; + resolve(value); + } + }; + + dispatch(openModal({ + modalId: ModalIdentifiers.GLOBAL_ATTRIBUTE_REMOVE_APPLIES_TO, + dialogType: ConfirmRemoveAppliesToModal, + dialogProps: { + resourceTypes, + onConfirm: () => resolveOnce(true), + onCancel: () => resolveOnce(false), + onExited: () => resolveOnce(false), + }, + })); + }); + }; +}; + +function ConfirmRemoveAppliesToModal({resourceTypes, onConfirm, onCancel, onExited}: Props) { + const {formatMessage} = useIntl(); + + const resources = resourceTypes.map((type) => formatMessage(resourceTypeLabels[type])).join(', '); + + const title = formatMessage({ + id: 'admin.global_attributes.confirm.remove_applies_to.title', + defaultMessage: 'Remove {resources}?', + }, {resources}); + + const confirmButtonText = formatMessage({ + id: 'admin.global_attributes.confirm.remove_applies_to.button', + defaultMessage: 'Remove and save', + }); + + return ( + + + + ); +} + +export default ConfirmRemoveAppliesToModal; diff --git a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/index.ts b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/index.ts index 9734c6dbb892..8a1958b63983 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/attribute_details/index.ts +++ b/webapp/channels/src/components/admin_console/global_attributes/attribute_details/index.ts @@ -1,4 +1,5 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -export {default, ATTRIBUTE_DETAILS_ROUTE} from './attribute_details'; +export {default} from './attribute_details'; +export {ATTRIBUTE_DETAILS_ROUTE, attributeDetailsRoute} from '../constants'; diff --git a/webapp/channels/src/components/admin_console/global_attributes/constants.ts b/webapp/channels/src/components/admin_console/global_attributes/constants.ts index a6f8e085bd36..faeff31b2c4f 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/constants.ts +++ b/webapp/channels/src/components/admin_console/global_attributes/constants.ts @@ -4,3 +4,13 @@ export const GLOBAL_ATTRIBUTES_GROUP_NAME = 'access_control'; export const GLOBAL_ATTRIBUTES_OBJECT_TYPE = 'template'; export const GLOBAL_ATTRIBUTES_TARGET_TYPE = 'system'; + +// Kept here (not on attribute_details.tsx) so the listing table can build an +// edit URL without importing the details page, which already imports type +// helpers from the table. +export const GLOBAL_ATTRIBUTES_LIST_ROUTE = '/admin_console/system_attributes/manage_attributes'; +export const ATTRIBUTE_DETAILS_ROUTE = `${GLOBAL_ATTRIBUTES_LIST_ROUTE}/attribute_details`; + +export function attributeDetailsRoute(fieldId: string): string { + return `${ATTRIBUTE_DETAILS_ROUTE}/${fieldId}`; +} diff --git a/webapp/channels/src/components/admin_console/global_attributes/global_attributes.tsx b/webapp/channels/src/components/admin_console/global_attributes/global_attributes.tsx index f0a1e41b913a..2b1d3a46d052 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/global_attributes.tsx +++ b/webapp/channels/src/components/admin_console/global_attributes/global_attributes.tsx @@ -11,7 +11,7 @@ import AdminHeader from 'components/widgets/admin_console/admin_header'; import {getHistory} from 'utils/browser_history'; -import {ATTRIBUTE_DETAILS_ROUTE} from './attribute_details'; +import {ATTRIBUTE_DETAILS_ROUTE} from './constants'; import GlobalAttributesTable from './global_attributes_table'; import './global_attributes.scss'; 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 e1cbe40da12e..e569cef4ce88 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 @@ -25,6 +25,13 @@ import type {GlobalState} from 'types/store'; import GlobalAttributesTable, {getDisplayName, getSourceIcon, getSourceKind, getTypeIcon, isClassificationMarkingsField} from './global_attributes_table'; +const mockHistoryPush = jest.fn(); +jest.mock('utils/browser_history', () => ({ + getHistory: () => ({ + push: mockHistoryPush, + }), +})); + // The server keys every field under a real group UUID that differs from the // group name ('access_control'); fixtures mirror that so the resolve-by-name // path is exercised, matching the pattern used by the session_attributes tests. @@ -112,6 +119,7 @@ describe('GlobalAttributesTable', () => { beforeEach(() => { getPropertyFields.mockReset(); + mockHistoryPush.mockReset(); }); it('shows the loading state before fields resolve', async () => { @@ -457,7 +465,7 @@ describe('GlobalAttributesTable', () => { }); describe('Actions column', () => { - it('opens the menu with Edit/Duplicate still visibly disabled and Delete enabled', async () => { + it('opens the menu with Edit enabled for a managed field, Duplicate still stubbed, and Delete enabled', async () => { getPropertyFields.mockResolvedValueOnce([makeField()]).mockResolvedValue([]); renderWithContext(, getBaseState()); @@ -479,16 +487,38 @@ describe('GlobalAttributesTable', () => { expect(duplicate).toBeDefined(); expect(del).toBeDefined(); - expect(edit!).toHaveAttribute('aria-disabled', 'true'); + expect(edit!).not.toHaveAttribute('aria-disabled', 'true'); + expect(edit!).not.toHaveTextContent('Coming soon'); expect(duplicate!).toHaveAttribute('aria-disabled', 'true'); - - // * Each still-stubbed item explains why, rather than silently doing nothing - expect(edit!).toHaveTextContent('Coming soon'); expect(duplicate!).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'); + + await userEvent.click(edit!); + await waitFor(() => { + expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/manage_attributes/attribute_details/field-1'); + }); + }); + + it('keeps Edit as Coming soon on a plugin-owned row', async () => { + getPropertyFields.mockResolvedValueOnce([makeField({ + attrs: {source_plugin_id: 'com.example.plugin', protected: true}, + })]).mockResolvedValue([]); + + const state = getBaseState(); + state.entities!.admin = { + pluginStatuses: {'com.example.plugin': {id: 'com.example.plugin'}}, + } as EntitiesPartial['admin']; + + renderWithContext(, state); + + await userEvent.click(await screen.findByTestId('global-attribute-actions-field-1')); + + const edit = screen.getAllByRole('menuitem').find((el) => el.textContent?.includes('Edit attribute')); + expect(edit).toHaveAttribute('aria-disabled', 'true'); + expect(edit).toHaveTextContent('Coming soon'); }); }); 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 90f8943384d3..6bcba5681bdd 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 @@ -37,11 +37,12 @@ import {useIsFieldOrphaned} from 'components/common/hooks/use_field_orphaned'; import LoadingScreen from 'components/loading_screen'; import * as Menu from 'components/menu'; +import {getHistory} from 'utils/browser_history'; 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 {attributeDetailsRoute, 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'; @@ -231,8 +232,9 @@ function ActionsCell({field, isClassificationRow, isMobileView, pluginInventoryL // 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 isPluginOwned = getSourceKind(field) === 'plugin'; const isOrphaned = pluginInventoryLoaded && fieldLooksOrphaned; - const isPluginManaged = getSourceKind(field) === 'plugin' && !isOrphaned; + const isPluginManaged = isPluginOwned && !isOrphaned; const handleConfirmed = useCallback(async () => { onDeleteError(null); @@ -290,14 +292,19 @@ function ActionsCell({field, isClassificationRow, isMobileView, pluginInventoryL > } - labels={( - <> - - - - )} + onClick={isPluginOwned ? undefined : () => getHistory().push(attributeDetailsRoute(field.id))} + labels={ + isPluginOwned ? ( + <> + + + + ) : ( + + ) + } /> { describe('createAttributeField', () => { @@ -243,4 +252,122 @@ describe('global_attributes/utils', () => { expect(deletePropertyField).toHaveBeenCalledWith('access_control', 'post', 'field-id'); }); }); + + describe('updateAttributeField', () => { + beforeEach(() => { + jest.restoreAllMocks(); + }); + + it('PATCHes the template and keeps option ids, sending null ldap/saml to unlink', async () => { + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue({} as PropertyField); + + await updateAttributeField('field-id', { + name: 'renamed', + type: 'select', + displayName: 'Renamed', + options: [{id: 'opt-1', name: 'Engineering'}, {id: '', name: 'Sales'}], + ldapAttr: '', + samlAttr: '', + }); + + expect(patchPropertyField).toHaveBeenCalledWith('access_control', 'template', 'field-id', { + name: 'renamed', + type: 'select', + attrs: { + display_name: 'Renamed', + options: [{id: 'opt-1', name: 'Engineering'}, {id: '', name: 'Sales'}], + ldap: null, + saml: null, + }, + }); + }); + + it('omits name when it is not in the patch, and sends options: null for text', async () => { + const patchPropertyField = jest.spyOn(Client4, 'patchPropertyField').mockResolvedValue({} as PropertyField); + + await updateAttributeField('field-id', { + type: 'text', + displayName: 'Cost center', + options: [{id: 'opt-1', name: ' leftover '}], + ldapAttr: 'department', + samlAttr: '', + }); + + expect(patchPropertyField).toHaveBeenCalledWith('access_control', 'template', 'field-id', { + type: 'text', + attrs: { + display_name: 'Cost center', + options: null, + ldap: 'department', + saml: null, + }, + }); + }); + }); + + describe('fetchAttributeField', () => { + beforeEach(() => { + jest.restoreAllMocks(); + }); + + it('returns the matching live template field and ignores deleted ones', async () => { + const live = {id: 'field-1', delete_at: 0} as PropertyField; + jest.spyOn(Client4, 'getPropertyFields').mockResolvedValue([ + {id: 'field-1', delete_at: 1} as PropertyField, + live, + ]); + + await expect(fetchAttributeField('field-1')).resolves.toBe(live); + }); + + it('returns undefined when the id is not in the page', async () => { + jest.spyOn(Client4, 'getPropertyFields').mockResolvedValue([{id: 'other', delete_at: 0} as PropertyField]); + + await expect(fetchAttributeField('field-1')).resolves.toBeUndefined(); + }); + }); + + describe('fetchLinkedFieldsForTemplate', () => { + beforeEach(() => { + jest.restoreAllMocks(); + }); + + it('queries user, channel, and post and keeps fields pointing at the template', async () => { + const getPropertyFields = jest.spyOn(Client4, 'getPropertyFields').mockImplementation((_group, objectType) => { + if (objectType === 'user') { + return Promise.resolve([ + {id: 'u1', object_type: 'user', linked_field_id: 'template-id', delete_at: 0} as PropertyField, + {id: 'u2', object_type: 'user', linked_field_id: 'other', delete_at: 0} as PropertyField, + ]); + } + if (objectType === 'channel') { + return Promise.resolve([ + {id: 'c1', object_type: 'channel', linked_field_id: 'template-id', delete_at: 0} as PropertyField, + ]); + } + return Promise.resolve([]); + }); + + const fields = await fetchLinkedFieldsForTemplate('template-id'); + + expect(getPropertyFields).toHaveBeenCalledWith('access_control', 'user', 'system', undefined, expect.objectContaining({perPage: 200})); + expect(getPropertyFields).toHaveBeenCalledWith('access_control', 'channel', 'system', undefined, expect.objectContaining({perPage: 200})); + expect(getPropertyFields).toHaveBeenCalledWith('access_control', 'post', 'system', undefined, expect.objectContaining({perPage: 200})); + expect(fields.map((field) => field.id)).toEqual(['u1', 'c1']); + }); + }); + + describe('linkedFieldsByResourceType', () => { + it('indexes the first live field per resource object type', () => { + const byType = linkedFieldsByResourceType([ + {id: 'u1', object_type: 'user'} as PropertyField, + {id: 'u2', object_type: 'user'} as PropertyField, + {id: 'c1', object_type: 'channel'} as PropertyField, + ]); + + expect(byType.user?.id).toBe('u1'); + expect(byType.channel?.id).toBe('c1'); + expect(byType.post).toBeUndefined(); + }); + }); }); 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 d3f976dd7360..ab2d202f4a1b 100644 --- a/webapp/channels/src/components/admin_console/global_attributes/utils.ts +++ b/webapp/channels/src/components/admin_console/global_attributes/utils.ts @@ -5,11 +5,17 @@ import type {PropertyField, PropertyFieldOption} from '@mattermost/types/propert import {Client4} from 'mattermost-redux/client'; +import {ALL_RESOURCE_TYPES} from './attribute_details/attribute_applies_to_constants'; import type {ResourceObjectType} from './attribute_details/attribute_applies_to_constants'; import {GLOBAL_ATTRIBUTES_GROUP_NAME, GLOBAL_ATTRIBUTES_OBJECT_TYPE, GLOBAL_ATTRIBUTES_TARGET_TYPE} from './constants'; export type AttributeFieldType = 'text' | 'select' | 'multiselect' | 'rank'; +// Server clamps per_page to this max (web.PerPageMaximum). Directory-mode +// listing with no cursor sorts CreateAt ASC, so a default 60-item page can +// miss a freshly created field. +const MAX_PROPERTY_FIELDS_PER_PAGE = 200; + // Builds the attrs.options payload for the given type: {id: '', name} for // Select/Multiselect (id is a required string on PropertyFieldOption, but the // server always generates the real one -- EnsureOptionIDs / @@ -29,6 +35,84 @@ function buildOptionsAttr(fieldType: AttributeFieldType, options: PropertyFieldO } } +// Patch keeps existing option IDs so stored values stay attached. New options +// still send an empty id for the server to mint. Text sends null so mergeAttrs +// drops a leftover options key when switching away from Select/Multiselect/Rank. +function buildPatchOptionsAttr(fieldType: AttributeFieldType, options: PropertyFieldOption[]): PropertyFieldOption[] | null { + switch (fieldType) { + case 'select': + case 'multiselect': + return options.map(({id, name}) => ({id: id || '', name})); + case 'rank': + return options.map(({id, name, rank}) => ({id: id || '', name, rank})); + default: + return null; + } +} + +async function listPropertyFields(objectType: string): Promise { + const fields: PropertyField[] = []; + let cursorId: string | undefined; + let cursorCreateAt: number | undefined; + + while (true) { + // eslint-disable-next-line no-await-in-loop + const page = await Client4.getPropertyFields( + GLOBAL_ATTRIBUTES_GROUP_NAME, + objectType, + GLOBAL_ATTRIBUTES_TARGET_TYPE, + undefined, + {perPage: MAX_PROPERTY_FIELDS_PER_PAGE, cursorId, cursorCreateAt}, + ); + fields.push(...page); + if (page.length === 0) { + break; + } + const last = page[page.length - 1]; + cursorId = last.id; + cursorCreateAt = last.create_at; + if (page.length < MAX_PROPERTY_FIELDS_PER_PAGE) { + break; + } + } + + return fields; +} + +// There is no GET-by-id property-fields HTTP handler. List template fields and +// find the one whose id matches. Returns undefined when it isn't in the group. +export async function fetchAttributeField(fieldId: string): Promise { + const fields = await listPropertyFields(GLOBAL_ATTRIBUTES_OBJECT_TYPE); + return fields.find((field) => field.id === fieldId && field.delete_at === 0); +} + +function isResourceObjectType(value: string): value is ResourceObjectType { + return (ALL_RESOURCE_TYPES as string[]).includes(value); +} + +// Lists user/channel/post fields and keeps those pointing at the template. +// There is no cross-object-type listing endpoint. +export async function fetchLinkedFieldsForTemplate(templateFieldId: string): Promise { + const pages = await Promise.all( + ALL_RESOURCE_TYPES.map((objectType) => listPropertyFields(objectType)), + ); + return pages.flat().filter((field) => ( + field.linked_field_id === templateFieldId && + field.delete_at === 0 && + isResourceObjectType(field.object_type) + )); +} + +export function linkedFieldsByResourceType(fields: PropertyField[]): Partial> { + const byType: Partial> = {}; + for (const field of fields) { + if (isResourceObjectType(field.object_type) && !byType[field.object_type]) { + byType[field.object_type] = field; + } + } + return byType; +} + // Creates a template field in the access_control group. target_type/target_id // are set explicitly because CanonicalizeSystemObjectField only auto-corrects // ObjectType=system fields, not ObjectType=template ones. @@ -59,6 +143,35 @@ export function createAttributeField( }); } +export type UpdateAttributeFieldPatch = { + name?: string; + type: AttributeFieldType; + displayName: string; + options: PropertyFieldOption[]; + ldapAttr: string; + samlAttr: string; +}; + +// PATCHes a template field. Attrs are merge-patched (mergeAttrs=true on the +// server): ldap/saml send null to unlink, and Text sends options: null so a +// leftover options array is dropped. name is omitted when unchanged so the +// server skips uniqueness re-validation. +export function updateAttributeField( + fieldId: string, + patch: UpdateAttributeFieldPatch, +): Promise { + return Client4.patchPropertyField(GLOBAL_ATTRIBUTES_GROUP_NAME, GLOBAL_ATTRIBUTES_OBJECT_TYPE, fieldId, { + ...(patch.name === undefined ? {} : {name: patch.name}), + type: patch.type as PropertyField['type'], + attrs: { + display_name: patch.displayName.trim() || undefined, + options: buildPatchOptionsAttr(patch.type, patch.options), + ldap: patch.ldapAttr || null, + saml: patch.samlAttr || null, + }, + }); +} + // 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 (or, for a save-time diff --git a/webapp/channels/src/components/admin_console/permission_schemes_settings/permission_system_scheme_settings/permission_system_scheme_settings.tsx b/webapp/channels/src/components/admin_console/permission_schemes_settings/permission_system_scheme_settings/permission_system_scheme_settings.tsx index 49f19105aa7d..77021deb24a9 100644 --- a/webapp/channels/src/components/admin_console/permission_schemes_settings/permission_system_scheme_settings/permission_system_scheme_settings.tsx +++ b/webapp/channels/src/components/admin_console/permission_schemes_settings/permission_system_scheme_settings/permission_system_scheme_settings.tsx @@ -118,9 +118,11 @@ export class PermissionSystemSchemeSettings extends React.PureComponent nextProps.roles[roleName])) { - this.loadRolesIntoState(nextProps); + componentDidUpdate() { + // The roles requested on mount arrive asynchronously, so keep waiting for them until + // they're all in props. Guarded by `loaded` so the setState can't loop. + if (!this.state.loaded && this.rolesNeeded.every((roleName) => this.props.roles[roleName])) { + this.loadRolesIntoState(this.props); } } diff --git a/webapp/channels/src/components/admin_console/system_roles/system_role/add_users_to_role_modal/add_users_to_role_modal.tsx b/webapp/channels/src/components/admin_console/system_roles/system_role/add_users_to_role_modal/add_users_to_role_modal.tsx index 19deb42f54b8..4858e8e8b006 100644 --- a/webapp/channels/src/components/admin_console/system_roles/system_role/add_users_to_role_modal/add_users_to_role_modal.tsx +++ b/webapp/channels/src/components/admin_console/system_roles/system_role/add_users_to_role_modal/add_users_to_role_modal.tsx @@ -120,7 +120,6 @@ export class AddUsersToRoleModal extends React.PureComponent { return (
onAdd(option)} onMouseMove={() => onMouseMove(option)} diff --git a/webapp/channels/src/components/advanced_text_editor/formatting_bar/formatting_bar.tsx b/webapp/channels/src/components/advanced_text_editor/formatting_bar/formatting_bar.tsx index 1ac883c566a3..2d7652132e5b 100644 --- a/webapp/channels/src/components/advanced_text_editor/formatting_bar/formatting_bar.tsx +++ b/webapp/channels/src/components/advanced_text_editor/formatting_bar/formatting_bar.tsx @@ -233,12 +233,13 @@ const FormattingBar = forwardRef((props const {formatMessage} = useIntl(); const HiddenControlsButtonAriaLabel = formatMessage({id: 'accessibility.button.hidden_controls_button', defaultMessage: 'show hidden formatting options'}); - const {x, y, strategy, update, context, refs: {setReference, setFloating}} = useFloating({ + const {x, y, strategy, update, context, refs} = useFloating({ open: showHiddenControls, onOpenChange: setShowHiddenControls, placement: 'top', middleware: [offset({mainAxis: 4})], }); + const {setReference, setFloating} = refs; const click = useClick(context); const {getReferenceProps: getClickReferenceProps, getFloatingProps: getClickFloatingProps} = useInteractions([ @@ -363,6 +364,7 @@ const FormattingBar = forwardRef((props timeout={250} classNames='scale' in={showHiddenControls} + nodeRef={refs.floating} unmountOnExit={true} > (null); + const nodeRef = useRef(null); useEffect(() => { if (ref === null || ref.current === null) { @@ -47,11 +48,13 @@ const LineLimiterBase = ({children, maxLines, lineHeight, moreText, lessText, er return ( <>
diff --git a/webapp/channels/src/components/channel_selector_modal/channel_selector_modal.tsx b/webapp/channels/src/components/channel_selector_modal/channel_selector_modal.tsx index 02d3051b79cf..46fe3172500c 100644 --- a/webapp/channels/src/components/channel_selector_modal/channel_selector_modal.tsx +++ b/webapp/channels/src/components/channel_selector_modal/channel_selector_modal.tsx @@ -204,7 +204,7 @@ export class ChannelSelectorModal extends React.PureComponent { return (
onAdd(option)} onMouseMove={() => onMouseMove(option)} diff --git a/webapp/channels/src/components/common/auto_height_switcher.tsx b/webapp/channels/src/components/common/auto_height_switcher.tsx index b965387b2b33..416b0c18467f 100644 --- a/webapp/channels/src/components/common/auto_height_switcher.tsx +++ b/webapp/channels/src/components/common/auto_height_switcher.tsx @@ -96,6 +96,7 @@ const AutoHeightSwitcher = ({showSlot, onTransitionEnd, slot1 = null, slot2 = nu return ( { setHeight(prevHeight.current ?? childRef.current!.offsetHeight); @@ -105,12 +106,12 @@ const AutoHeightSwitcher = ({showSlot, onTransitionEnd, slot1 = null, slot2 = nu onEntering={() => { setHeight(childRef.current!.offsetHeight); }} - onEntered={(node: HTMLElement) => { + onEntered={() => { prevHeight.current = childRef.current!.offsetHeight; setHeight('auto'); setOverflow('visible'); setAnimate(false); - onTransitionEnd?.(node); + onTransitionEnd?.(wrapperRef.current ?? undefined); }} >
; prefetchRequestStatus: Record; - // Whether or not the categories in the sidebar have been loaded for the current team + // Whether or not the data the sidebar renders from has been loaded: the categories for the + // current team, plus the user's channels and channel memberships sidebarLoaded: boolean; unreadChannels: Channel[]; diff --git a/webapp/channels/src/components/data_prefetch/index.test.tsx b/webapp/channels/src/components/data_prefetch/index.test.tsx new file mode 100644 index 000000000000..6e770df66847 --- /dev/null +++ b/webapp/channels/src/components/data_prefetch/index.test.tsx @@ -0,0 +1,149 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import type {DeepPartial} from '@mattermost/types/utilities'; + +import {CategoryTypes} from 'mattermost-redux/constants/channel_categories'; + +import {loadProfilesForSidebar} from 'actions/user_actions'; + +import {renderWithContext, runPostRenderAct} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +import type {GlobalState} from 'types/store'; + +import DataPrefetch from './index'; + +const mockQueue: Array<() => Promise> = []; + +jest.mock('p-queue', () => class PQueueMock { + add = (o: () => Promise) => mockQueue.push(o); + clear = () => mockQueue.splice(0, mockQueue.length); +}); + +jest.mock('actions/user_actions', () => ({ + loadProfilesForSidebar: jest.fn(() => Promise.resolve({})), +})); + +// The sidebar's DM and GM profiles are loaded from three requests that resolve in any order, and +// loadProfilesForSidebar silently loads nothing if it runs before all three have landed. These +// cover both of the orderings that leave it short. +describe('components/data_prefetch (connected)', () => { + const currentUser = TestHelper.getUserMock({id: 'current_user_id'}); + const currentTeam = TestHelper.getTeamMock({id: 'current_team_id'}); + const channel = TestHelper.getChannelMock({id: 'channel_id', team_id: currentTeam.id}); + const category = TestHelper.getCategoryMock({ + id: 'category_id', + team_id: currentTeam.id, + user_id: currentUser.id, + type: CategoryTypes.FAVORITES, + channel_ids: [channel.id], + }); + + const nothingLoaded: DeepPartial = { + entities: { + users: { + currentUserId: currentUser.id, + profiles: {[currentUser.id]: currentUser}, + }, + teams: { + currentTeamId: currentTeam.id, + teams: {[currentTeam.id]: currentTeam}, + }, + channels: { + channels: {[channel.id]: channel}, + }, + }, + }; + + const categoriesLoaded: DeepPartial = { + entities: { + channelCategories: { + byId: {[category.id]: category}, + orderByTeam: {[currentTeam.id]: [category.id]}, + }, + }, + }; + + const initChannelsLoaded: DeepPartial = { + entities: { + channels: { + channelsInTeam: {[currentTeam.id]: new Set([channel.id])}, + }, + }, + views: { + channelSidebar: { + initChannelsLoaded: true, + }, + }, + }; + + const initChannelMembershipsLoaded: DeepPartial = { + entities: { + channels: { + myMembers: { + [channel.id]: TestHelper.getChannelMembershipMock({ + channel_id: channel.id, + user_id: currentUser.id, + }), + }, + }, + }, + views: { + channelSidebar: { + initChannelMembershipsLoaded: true, + }, + }, + }; + + beforeEach(() => { + mockQueue.splice(0, mockQueue.length); + jest.clearAllMocks(); + }); + + // One case per request arriving last, so that dropping any one of the three checks fails a test + // rather than leaving a silently blank sidebar row. + test.each([ + ['categories', [initChannelsLoaded, initChannelMembershipsLoaded, categoriesLoaded]], + ['channels', [categoriesLoaded, initChannelMembershipsLoaded, initChannelsLoaded]], + ['memberships', [categoriesLoaded, initChannelsLoaded, initChannelMembershipsLoaded]], + ] as Array<[string, Array>]>)( + 'should not load profiles for the sidebar until the %s arrive last', + async (_, [first, second, last]) => { + const {updateStoreState} = renderWithContext(, nothingLoaded); + + updateStoreState(first); + await runPostRenderAct(); + + expect(loadProfilesForSidebar).not.toHaveBeenCalled(); + + updateStoreState(second); + await runPostRenderAct(); + + expect(loadProfilesForSidebar).not.toHaveBeenCalled(); + + updateStoreState(last); + await runPostRenderAct(); + + expect(loadProfilesForSidebar).toHaveBeenCalledTimes(1); + }, + ); + + test('should only load profiles for the sidebar once', async () => { + const {updateStoreState} = renderWithContext(, nothingLoaded); + + updateStoreState(categoriesLoaded); + updateStoreState(initChannelsLoaded); + updateStoreState(initChannelMembershipsLoaded); + await runPostRenderAct(); + + expect(loadProfilesForSidebar).toHaveBeenCalledTimes(1); + + updateStoreState({}); + await runPostRenderAct(); + + expect(loadProfilesForSidebar).toHaveBeenCalledTimes(1); + }); +}); diff --git a/webapp/channels/src/components/data_prefetch/index.ts b/webapp/channels/src/components/data_prefetch/index.ts index eeb0bb34c089..c0df12d77baa 100644 --- a/webapp/channels/src/components/data_prefetch/index.ts +++ b/webapp/channels/src/components/data_prefetch/index.ts @@ -17,8 +17,18 @@ import type {GlobalState} from 'types/store'; import {prefetchQueue} from './actions'; import DataPrefetch from './data_prefetch'; +// This gates loadProfilesForSidebar, so it has to cover everything that action reads: its GM half +// reads getDisplayedChannels, which is driven by the categories, and its DM half reads +// getMyChannels, which needs both the channels and the memberships. It reads the store once when +// called and never retries, so opening this gate before all three have arrived silently loads no +// profiles at all for the rest of the session. +// +// The completion flags are important: the current channel can populate one channel or membership +// before the corresponding bulk request completes, which is not enough for loadProfilesForSidebar. function isSidebarLoaded(state: GlobalState) { - return getCategoriesForCurrentTeam(state).length > 0; + return getCategoriesForCurrentTeam(state).length > 0 && + state.views.channelSidebar.initChannelsLoaded && + state.views.channelSidebar.initChannelMembershipsLoaded; } function mapStateToProps(state: GlobalState) { diff --git a/webapp/channels/src/components/emoji_picker/components/emoji_picker_skin.tsx b/webapp/channels/src/components/emoji_picker/components/emoji_picker_skin.tsx index b264b75f48da..9bc6591d4fc2 100644 --- a/webapp/channels/src/components/emoji_picker/components/emoji_picker_skin.tsx +++ b/webapp/channels/src/components/emoji_picker/components/emoji_picker_skin.tsx @@ -83,6 +83,8 @@ type State = { }; export class EmojiPickerSkin extends React.PureComponent { + private nodeRef = React.createRef(); + constructor(props: Props) { super(props); @@ -204,11 +206,15 @@ export class EmojiPickerSkin extends React.PureComponent { return ( -
+
@@ -311,166 +313,116 @@ exports[`components/global/product_switcher should match snapshot with a registe /> -
- + + + +
+
@@ -541,72 +545,73 @@ exports[`components/global/product_switcher should match snapshot with product s /> -
- + + Playbooks + + + +
+
@@ -778,72 +784,73 @@ exports[`components/global/product_switcher should render once when there are no /> -
- + + Playbooks + + + +
+
@@ -976,72 +984,73 @@ exports[`components/global/product_switcher should render the correct amount of /> -
- + + Playbooks + + + +
+
diff --git a/webapp/channels/src/components/info_toast/info_toast.tsx b/webapp/channels/src/components/info_toast/info_toast.tsx index 6c972b855d5d..35ff71ab01fd 100644 --- a/webapp/channels/src/components/info_toast/info_toast.tsx +++ b/webapp/channels/src/components/info_toast/info_toast.tsx @@ -2,7 +2,7 @@ // See LICENSE.txt for license information. import classNames from 'classnames'; -import React, {useEffect, useCallback} from 'react'; +import React, {useEffect, useCallback, useRef} from 'react'; import {useIntl} from 'react-intl'; import {CSSTransition} from 'react-transition-group'; @@ -25,6 +25,7 @@ type Props = { function InfoToast({content, onExited, className, position = DEFAULT_POSITION}: Props): JSX.Element { const {formatMessage} = useIntl(); + const nodeRef = useRef(null); // Validate position and fallback to default if invalid const validatedPosition = VALID_POSITIONS.includes(position) ? position : DEFAULT_POSITION; @@ -51,13 +52,17 @@ function InfoToast({content, onExited, className, position = DEFAULT_POSITION}: return ( -
+
{content.icon} {content.message} {content.undo && ( diff --git a/webapp/channels/src/components/mobile_sidebar_right/mobile_sidebar_right.tsx b/webapp/channels/src/components/mobile_sidebar_right/mobile_sidebar_right.tsx index c7665ee0fd5e..0e7054ab594a 100644 --- a/webapp/channels/src/components/mobile_sidebar_right/mobile_sidebar_right.tsx +++ b/webapp/channels/src/components/mobile_sidebar_right/mobile_sidebar_right.tsx @@ -22,6 +22,7 @@ const MobileRightDrawer = ({ }: Props) => { const usageDeltas = useGetUsageDeltas(); const sidebarRef = useRef(null); + const drawerRef = useRef(null); useEffect(() => { if (isOpen && sidebarRef.current) { @@ -46,15 +47,18 @@ const MobileRightDrawer = ({
- +
+ +
diff --git a/webapp/channels/src/components/multiselect/multiselect_list.tsx b/webapp/channels/src/components/multiselect/multiselect_list.tsx index 954e9d9fc5b2..fffa481cec4e 100644 --- a/webapp/channels/src/components/multiselect/multiselect_list.tsx +++ b/webapp/channels/src/components/multiselect/multiselect_list.tsx @@ -139,7 +139,7 @@ export default class MultiSelectList extends React.PureComponen return (
add(option)} diff --git a/webapp/channels/src/components/onboarding_tasklist/onboarding_tasklist_completed.tsx b/webapp/channels/src/components/onboarding_tasklist/onboarding_tasklist_completed.tsx index f3bcae1ce6dc..31984ac470ac 100644 --- a/webapp/channels/src/components/onboarding_tasklist/onboarding_tasklist_completed.tsx +++ b/webapp/channels/src/components/onboarding_tasklist/onboarding_tasklist_completed.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useEffect} from 'react'; +import React, {useEffect, useRef} from 'react'; import {FormattedMessage} from 'react-intl'; import {useSelector, useDispatch} from 'react-redux'; import {CSSTransition} from 'react-transition-group'; @@ -130,6 +130,7 @@ const Completed = (props: Props): JSX.Element => { const {dismissAction} = props; const dispatch = useDispatch(); + const wrapperRef = useRef(null); useEffect(() => { dispatch(getPrevTrialLicense()); @@ -155,10 +156,11 @@ const Completed = (props: Props): JSX.Element => { <> - + {'completed(null); useLayoutEffect(() => { setReference(trigger); @@ -94,9 +95,11 @@ export const TaskListPopover = ({ timeout={150} classNames='fade' in={isVisible} + nodeRef={overlayRef} unmountOnExit={true} > diff --git a/webapp/channels/src/components/preparing_workspace/invite_members.tsx b/webapp/channels/src/components/preparing_workspace/invite_members.tsx index 0c3c48f266c7..b8e1d0f7daaf 100644 --- a/webapp/channels/src/components/preparing_workspace/invite_members.tsx +++ b/webapp/channels/src/components/preparing_workspace/invite_members.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useState, useMemo, useEffect} from 'react'; +import React, {useState, useMemo, useEffect, useRef} from 'react'; import {FormattedMessage, defineMessages, useIntl} from 'react-intl'; import {CSSTransition} from 'react-transition-group'; @@ -41,6 +41,7 @@ const InviteMembers = (props: Props) => { const [showSkipButton, setShowSkipButton] = useState(false); const {formatMessage} = useIntl(); + const nodeRef = useRef(null); let className = 'InviteMembers-body'; if (props.className) { className += ' ' + props.className; @@ -204,12 +205,16 @@ const InviteMembers = (props: Props) => { return ( -
+
(null); + const fullscreenRef = useRef(null); useEffect(() => { if (hasEntered) { @@ -58,7 +60,10 @@ function LaunchingWorkspace(props: Props) { bodyClass += ' LaunchingWorkspace-body--non-fullscreen'; } const body = ( -
+
{ const dispatch = useDispatch(); const [triedNext, setTriedNext] = useState(false); + const nodeRef = useRef(null); const inputRef = useRef(); const validation = teamNameToUrl(props.organization || ''); const teamApiError = useRef(null); @@ -122,14 +123,19 @@ const Organization = (props: Props) => { className += ' ' + props.className; } return ( + -
+
diff --git a/webapp/channels/src/components/preparing_workspace/plugins.tsx b/webapp/channels/src/components/preparing_workspace/plugins.tsx index 4a621e94d071..700dad74c82c 100644 --- a/webapp/channels/src/components/preparing_workspace/plugins.tsx +++ b/webapp/channels/src/components/preparing_workspace/plugins.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {useRef} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {CSSTransition} from 'react-transition-group'; @@ -32,6 +32,7 @@ type Props = PreparingWorkspacePageProps & { }; const Plugins = (props: Props) => { const {formatMessage} = useIntl(); + const nodeRef = useRef(null); let className = 'Plugins-body'; if (props.className) { @@ -55,12 +56,16 @@ const Plugins = (props: Props) => { return ( -
+
{ +export const Progress = React.forwardRef((props, ref) => { // exclude transitioning out as a progress step const numSteps = props.stepOrder.length - 1; if (numSteps < 2) { @@ -36,21 +36,33 @@ export const Progress = (props: Props) => { ); }); - return (
-
{dots}
-
); -}; + return ( +
+
{dots}
+
+ ); +}); +Progress.displayName = 'Progress'; export default function TransitionedProgress(props: Props) { + const nodeRef = useRef(null); + return ( - + ); } diff --git a/webapp/channels/src/components/root/performance_reporter_controller.tsx b/webapp/channels/src/components/root/performance_reporter_controller.tsx index 26c6584548d8..ca89a2e5a334 100644 --- a/webapp/channels/src/components/root/performance_reporter_controller.tsx +++ b/webapp/channels/src/components/root/performance_reporter_controller.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {useEffect, useRef} from 'react'; +import {useEffect} from 'react'; import {useStore} from 'react-redux'; import {Client4} from 'mattermost-redux/client'; @@ -14,16 +14,12 @@ import type {GlobalState} from 'types/store'; export default function PerformanceReporterController() { const store = useStore(); - const reporter = useRef(); - useEffect(() => { - reporter.current = new PerformanceReporter(Client4, store, DesktopAppAPI); - reporter.current.observe(); + const reporter = new PerformanceReporter(Client4, store, DesktopAppAPI); + reporter.observe(); - // There's no way to clean up web-vitals, so continue to assume that this component won't ever be unmounted return () => { - // eslint-disable-next-line no-console - console.error('PerformanceReporterController - Component unmounted or store changed'); + reporter.disconnect(); }; }, [store]); diff --git a/webapp/channels/src/components/team_selector_modal/team_selector_modal.tsx b/webapp/channels/src/components/team_selector_modal/team_selector_modal.tsx index cfaf49b502a5..276a2260be63 100644 --- a/webapp/channels/src/components/team_selector_modal/team_selector_modal.tsx +++ b/webapp/channels/src/components/team_selector_modal/team_selector_modal.tsx @@ -168,7 +168,7 @@ export class TeamSelectorModal extends React.PureComponent { return (
onAdd(option)} onMouseMove={() => onMouseMove(option)} diff --git a/webapp/channels/src/components/widgets/menu/menu_wrapper_animation.tsx b/webapp/channels/src/components/widgets/menu/menu_wrapper_animation.tsx index 05ca9a48eb98..1f6c5543040d 100644 --- a/webapp/channels/src/components/widgets/menu/menu_wrapper_animation.tsx +++ b/webapp/channels/src/components/widgets/menu/menu_wrapper_animation.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {useRef} from 'react'; import {CSSTransition} from 'react-transition-group'; import {isMobile} from './is_mobile_view_hack'; @@ -17,6 +17,12 @@ type Props = { * @deprecated Use the "webapp/channels/src/components/menu" instead. */ export default function MenuWrapperAnimation(props: Props) { + // The children are arbitrary, so there's no element of our own to hang a ref on. Without a + // nodeRef, CSSTransition falls back to findDOMNode, which React 18 warns about in StrictMode and + // React 19 removes. The wrapper is a static box, so the menu inside it still positions itself + // against .MenuWrapper. + const nodeRef = useRef(null); + if (isMobile()) { if (props.show) { return props.children; @@ -28,6 +34,7 @@ export default function MenuWrapperAnimation(props: Props) { return ( - {props.children} +
+ {props.children} +
); } diff --git a/webapp/channels/src/components/widgets/modals/full_screen_modal.tsx b/webapp/channels/src/components/widgets/modals/full_screen_modal.tsx index 0e1a12f67857..b9c4cd809e4d 100644 --- a/webapp/channels/src/components/widgets/modals/full_screen_modal.tsx +++ b/webapp/channels/src/components/widgets/modals/full_screen_modal.tsx @@ -74,6 +74,7 @@ class FullScreenModal extends React.PureComponent { return ( { + test('tracks completion of the bulk channel and membership requests', () => { + expect(Reducers.initChannelsLoaded(false, {type: ChannelTypes.INIT_CHANNELS_LOADED})).toBe(true); + expect(Reducers.initChannelMembershipsLoaded(false, {type: ChannelTypes.INIT_CHANNEL_MEMBERSHIPS_LOADED})).toBe(true); + }); + + test('ignores unrelated channel and membership responses', () => { + expect(Reducers.initChannelsLoaded(false, {type: ChannelTypes.RECEIVED_CHANNELS})).toBe(false); + expect(Reducers.initChannelMembershipsLoaded(false, {type: ChannelTypes.RECEIVED_MY_CHANNEL_MEMBERS})).toBe(false); + }); + + test('resets completion state when switching teams or logging out', () => { + expect(Reducers.initChannelsLoaded(true, {type: TeamTypes.SELECT_TEAM})).toBe(false); + expect(Reducers.initChannelMembershipsLoaded(true, {type: TeamTypes.SELECT_TEAM})).toBe(false); + expect(Reducers.initChannelsLoaded(true, {type: UserTypes.LOGOUT_SUCCESS})).toBe(false); + expect(Reducers.initChannelMembershipsLoaded(true, {type: UserTypes.LOGOUT_SUCCESS})).toBe(false); + }); +}); + describe('multiSelectedChannelIds', () => { test('should select single channel when it does not exist in the list', () => { const initialState = [ diff --git a/webapp/channels/src/reducers/views/channel_sidebar.ts b/webapp/channels/src/reducers/views/channel_sidebar.ts index c132f5581192..650b4c324300 100644 --- a/webapp/channels/src/reducers/views/channel_sidebar.ts +++ b/webapp/channels/src/reducers/views/channel_sidebar.ts @@ -5,7 +5,7 @@ import {combineReducers} from 'redux'; import type {ChannelCategory} from '@mattermost/types/channel_categories'; -import {ChannelCategoryTypes, UserTypes} from 'mattermost-redux/action_types'; +import {ChannelCategoryTypes, ChannelTypes, TeamTypes, UserTypes} from 'mattermost-redux/action_types'; import {removeItem} from 'mattermost-redux/utils/array_utils'; import {ActionTypes} from 'utils/constants'; @@ -24,6 +24,30 @@ export function unreadFilterEnabled(state = false, action: MMAction) { } } +export function initChannelsLoaded(state = false, action: MMAction) { + switch (action.type) { + case ChannelTypes.INIT_CHANNELS_LOADED: + return true; + case TeamTypes.SELECT_TEAM: + case UserTypes.LOGOUT_SUCCESS: + return false; + default: + return state; + } +} + +export function initChannelMembershipsLoaded(state = false, action: MMAction) { + switch (action.type) { + case ChannelTypes.INIT_CHANNEL_MEMBERSHIPS_LOADED: + return true; + case TeamTypes.SELECT_TEAM: + case UserTypes.LOGOUT_SUCCESS: + return false; + default: + return state; + } +} + export function draggingState(state: DraggingState = {}, action: MMAction): DraggingState { switch (action.type) { case ActionTypes.SIDEBAR_DRAGGING_SET_STATE: @@ -130,6 +154,8 @@ export function lastSelectedChannel(state = '', action: MMAction): string { export default combineReducers({ unreadFilterEnabled, + initChannelsLoaded, + initChannelMembershipsLoaded, draggingState, newCategoryIds, multiSelectedChannelIds, diff --git a/webapp/channels/src/types/store/views.ts b/webapp/channels/src/types/store/views.ts index eddbe4600589..f28e829ddcdf 100644 --- a/webapp/channels/src/types/store/views.ts +++ b/webapp/channels/src/types/store/views.ts @@ -200,6 +200,8 @@ export type ViewsState = { newCategoryIds: string[]; multiSelectedChannelIds: string[]; lastSelectedChannel: string; + initChannelsLoaded: boolean; + initChannelMembershipsLoaded: boolean; }; addChannelCtaDropdown: { diff --git a/webapp/channels/src/utils/constants.tsx b/webapp/channels/src/utils/constants.tsx index 881bc9fd0a50..abee6402036d 100644 --- a/webapp/channels/src/utils/constants.tsx +++ b/webapp/channels/src/utils/constants.tsx @@ -502,6 +502,7 @@ export const ModalIdentifiers = { SESSION_ATTRIBUTE_DISABLE: 'session_attribute_disable', BOARD_ATTRIBUTE_FIELD_DELETE: 'board_attribute_field_delete', GLOBAL_ATTRIBUTE_FIELD_DELETE: 'global_attribute_field_delete', + GLOBAL_ATTRIBUTE_REMOVE_APPLIES_TO: 'global_attribute_remove_applies_to', 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/performance_telemetry/reporter.test.ts b/webapp/channels/src/utils/performance_telemetry/reporter.test.ts index dc7639f09516..1a16ce8634c2 100644 --- a/webapp/channels/src/utils/performance_telemetry/reporter.test.ts +++ b/webapp/channels/src/utils/performance_telemetry/reporter.test.ts @@ -357,6 +357,24 @@ describe.skip('PerformanceReporter', () => { }); }); +describe('PerformanceReporter.disconnect', () => { + test('should stop reporting web vitals once disconnected', () => { + const {reporter, sendBeacon} = newTestReporter(); + reporter.observe(); + + const onCLSCallback = (onCLS as jest.Mock).mock.calls.at(-1)[0]; + + reporter.disconnect(); + + // web-vitals has no way to unregister a callback, so a reporter left behind by an unmounted + // component keeps being handed metrics after it has been disconnected. + onCLSCallback({name: 'CLS', value: 100}); + reporter.maybeSendReport(); + + expect(sendBeacon).not.toHaveBeenCalled(); + }); +}); + describe('PerformanceReporter.sendReport content-type', () => { const sampleReport = { version: '0.1.0' as const, @@ -420,8 +438,6 @@ class TestPerformanceReporter extends PerformanceReporter { public reportPeriodBase = 10; public reportPeriodJitter = 0; - public disconnect = super.disconnect; - public handleObservations = jest.fn(super.handleObservations); public maybeSendReport = jest.fn(super.maybeSendReport); diff --git a/webapp/channels/src/utils/performance_telemetry/reporter.ts b/webapp/channels/src/utils/performance_telemetry/reporter.ts index cb0e8b67017c..e08c0109430f 100644 --- a/webapp/channels/src/utils/performance_telemetry/reporter.ts +++ b/webapp/channels/src/utils/performance_telemetry/reporter.ts @@ -80,6 +80,7 @@ export default class PerformanceReporter { private observer: PerformanceObserver; private reportTimeout: number | undefined; + private disconnected = false; // These values are protected instead of private so that they can be modified by unit tests protected reportPeriodBase = 60 * 1000; @@ -170,9 +171,12 @@ export default class PerformanceReporter { } /** - * This method is for testing only because we can't clean up the callbacks registered with web-vitals. + * web-vitals has no way to unregister a callback, so a disconnected reporter has to keep + * ignoring the metrics it still receives rather than stop receiving them. */ - protected disconnect() { + public disconnect() { + this.disconnected = true; + removeEventListener('visibilitychange', this.handleVisibilityChange); clearTimeout(this.reportTimeout); @@ -181,6 +185,7 @@ export default class PerformanceReporter { this.observer.disconnect(); this.desktopOffListener?.(); + this.desktopOffListener = undefined; } protected handleObservations(list: PerformanceObserverEntryList) { @@ -244,6 +249,10 @@ export default class PerformanceReporter { } private handleWebVital(metric: Metric) { + if (this.disconnected) { + return; + } + let labels: Record | undefined; if (isLCPMetric(metric)) { diff --git a/webapp/package-lock.json b/webapp/package-lock.json index f41a0b0bb4ca..08a32ca124df 100644 --- a/webapp/package-lock.json +++ b/webapp/package-lock.json @@ -123,7 +123,7 @@ "prop-types": "15.8.1", "react": "18.2.0", "react-beautiful-dnd": "13.1.1", - "react-bootstrap": "github:mattermost/react-bootstrap#c17701564a6f240a14419d94369936c380f917e5", + "react-bootstrap": "github:mattermost/react-bootstrap#d693ec8082954f0e6d3e826ca6161979b29001cd", "react-color": "2.19.3", "react-day-picker": "8.3.6", "react-dom": "18.2.0", @@ -23556,8 +23556,8 @@ }, "node_modules/react-bootstrap": { "version": "0.32.4", - "resolved": "git+ssh://git@github.com/mattermost/react-bootstrap.git#c17701564a6f240a14419d94369936c380f917e5", - "integrity": "sha512-FOdO69AE6NY0gAjCb5FRSPpmh+6+kUEp83Ss/eu8jtuHk3tB/JygupZgAeSPALNAo2svdg3DfnNo2VQd2+J0rw==", + "resolved": "git+ssh://git@github.com/mattermost/react-bootstrap.git#d693ec8082954f0e6d3e826ca6161979b29001cd", + "integrity": "sha512-cfde04u5rHWxflLLbjktlUZDgqkTGcti/7RPH7ch7OR0pT4msXcxml9nR3ClM5fbiOnlkJLxIVYjfS9+K3JWkQ==", "hasInstallScript": true, "license": "MIT", "dependencies": { @@ -28672,7 +28672,7 @@ "version": "11.11.0", "dependencies": { "@tippyjs/react": "4.2.6", - "react-bootstrap": "github:mattermost/react-bootstrap#c17701564a6f240a14419d94369936c380f917e5", + "react-bootstrap": "github:mattermost/react-bootstrap#d693ec8082954f0e6d3e826ca6161979b29001cd", "styled-components": "5.3.7" }, "devDependencies": { diff --git a/webapp/platform/components/package.json b/webapp/platform/components/package.json index 8f29643f35d9..36a58dfe8fe8 100644 --- a/webapp/platform/components/package.json +++ b/webapp/platform/components/package.json @@ -51,7 +51,7 @@ }, "dependencies": { "@tippyjs/react": "4.2.6", - "react-bootstrap": "github:mattermost/react-bootstrap#c17701564a6f240a14419d94369936c380f917e5", + "react-bootstrap": "github:mattermost/react-bootstrap#d693ec8082954f0e6d3e826ca6161979b29001cd", "styled-components": "5.3.7" } } diff --git a/webapp/platform/components/rollup.config.js b/webapp/platform/components/rollup.config.js index 7be1d950a1fd..6315d6f213f3 100644 --- a/webapp/platform/components/rollup.config.js +++ b/webapp/platform/components/rollup.config.js @@ -13,6 +13,10 @@ const externals = [ ...Object.keys(packagejson.peerDependencies || {}), 'lodash/throttle', 'react', + + // react-dom is only a devDependency here, so without this it gets bundled into dist and the + // web app ends up running two copies of the renderer. + 'react-dom', 'mattermost-redux', 'reselect', ];