diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index 0684d62c6ec..445b46ae1fa 100644 --- a/server/channels/api4/channel.go +++ b/server/channels/api4/channel.go @@ -2910,7 +2910,11 @@ func getChannelAccessControlAttributes(c *Context, w http.ResponseWriter, r *htt return } - attributes, err := c.App.GetAccessControlPolicyAttributes(c.AppContext, c.Params.ChannelId, "*") + // Channel banners care about the membership rule — the attributes that + // determine who can be in the channel. Since the v0.3 migration stores the + // action as "membership" rather than "*", ask for it explicitly; the + // wildcard fallback in GetRule still covers older policies that kept "*". + attributes, err := c.App.GetAccessControlPolicyAttributes(c.AppContext, c.Params.ChannelId, model.AccessControlPolicyActionMembership) if err != nil { c.Err = err return diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss index df3acf11195..d793b81c6e8 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.scss @@ -40,6 +40,11 @@ &__policy-banner { .TagGroup { margin-top: 12px; + + // Tighten vertical spacing when tags wrap onto multiple rows + // (TagGroup defaults to gap: 8px which is too airy in this + // banner). Column spacing is preserved. + row-gap: 0; } } } diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx index 67aac2b79a8..62ed4b7bfec 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.test.tsx @@ -502,9 +502,9 @@ describe('components/channel_invite_modal', () => { // Modal renders in a portal, so query from document instead of container expect(document.querySelector('.AlertBanner')).not.toBeNull(); - // Check that the attribute tags are shown - expect(screen.getByText('tag1')).toBeInTheDocument(); - expect(screen.getByText('tag2')).toBeInTheDocument(); + // Check that the attribute tags are shown in "Attribute: value" form. + expect(screen.getByText('Attribute1: tag1')).toBeInTheDocument(); + expect(screen.getByText('Attribute1: tag2')).toBeInTheDocument(); }); test('should not show AlertBanner when policy_enforced is false', () => { diff --git a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx index a20029404a7..0bdd450eddc 100644 --- a/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx +++ b/webapp/channels/src/components/channel_invite_modal/channel_invite_modal.tsx @@ -33,6 +33,7 @@ import GuestTag from 'components/widgets/tag/guest_tag'; import TagGroup from 'components/widgets/tag/tag_group'; import Constants, {ModalIdentifiers} from 'utils/constants'; +import {formatAttributeName} from 'utils/format_attribute_name'; import {sortUsersAndGroups} from 'utils/utils'; import GroupOption from './group_option'; @@ -112,14 +113,29 @@ const ChannelInviteModalComponent = (props: Props) => { props.channel.policy_enforced, ); - // Helper function to format attribute names for tooltips - const formatAttributeName = (name: string): string => { - // Convert snake_case or camelCase to Title Case with spaces - return name. - replace(/_/g, ' '). - replace(/([A-Z])/g, ' $1'). - replace(/\w\S*/g, (txt) => txt.charAt(0).toUpperCase() + txt.substring(1).toLowerCase()); - }; + // Memoise the rendered access-control tags so they don't re-render on + // every keystroke in the invite text box. + const accessControlTags = useMemo(() => { + if (structuredAttributes.length === 0) { + return null; + } + return ( + + {structuredAttributes.flatMap((attribute) => + attribute.values.map((value) => { + const attributeLabel = formatAttributeName(attribute.name); + return ( + + ); + }), + )} + + ); + }, [structuredAttributes]); // Helper function to add a user or group to the selected list const addValue = useCallback((value: UserProfileValue | GroupValue) => { @@ -667,19 +683,7 @@ const ChannelInviteModalComponent = (props: Props) => { /> )} > - {structuredAttributes.length > 0 && ( - - {structuredAttributes.flatMap((attribute) => - attribute.values.map((value) => ( - - )), - )} - - )} + {accessControlTags} )} diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss index bcd8f25472d..838a589ac40 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.scss @@ -34,6 +34,11 @@ .TagGroup { margin-top: 12px; + + // Tighten vertical spacing when tags wrap onto multiple rows + // (TagGroup defaults to gap: 8px which is too airy in this + // banner). Column spacing is preserved. + row-gap: 0; } } } diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx index d7b6f5f5640..8b580440f49 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx @@ -225,7 +225,9 @@ describe('channel_members_rhs/channel_members_rhs', () => { ); expect(screen.getByText('Channel access is restricted by user attributes')).toBeInTheDocument(); - expect(screen.getByText('tag1')).toBeInTheDocument(); - expect(screen.getByText('tag2')).toBeInTheDocument(); + + // Each tag is rendered as "Attribute: value" for readability. + expect(screen.getByText('Attribute1: tag1')).toBeInTheDocument(); + expect(screen.getByText('Attribute1: tag2')).toBeInTheDocument(); }); }); diff --git a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx index 22fc87e4108..15aa6dcaa0c 100644 --- a/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx +++ b/webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx @@ -2,7 +2,7 @@ // See LICENSE.txt for license information. import debounce from 'lodash/debounce'; -import React, {useCallback, useEffect, useState} from 'react'; +import React, {useCallback, useEffect, useMemo, useState} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {useHistory} from 'react-router-dom'; @@ -20,6 +20,7 @@ import AlertTag from 'components/widgets/tag/alert_tag'; import TagGroup from 'components/widgets/tag/tag_group'; import Constants, {ModalIdentifiers} from 'utils/constants'; +import {formatAttributeName} from 'utils/format_attribute_name'; import type {ModalData} from 'types/actions'; @@ -84,14 +85,29 @@ export default function ChannelMembersRHS({ channel.policy_enforced, ); - // Helper function to format attribute names for tooltips - const formatAttributeName = (name: string): string => { - // Convert snake_case or camelCase to Title Case with spaces - return name. - replace(/_/g, ' '). - replace(/([A-Z])/g, ' $1'). - replace(/\w\S*/g, (txt) => txt.charAt(0).toUpperCase() + txt.substring(1).toLowerCase()); - }; + // Memoise the rendered access-control tags so they don't re-render on + // every unrelated state change in the centre channel. + const accessControlTags = useMemo(() => { + if (structuredAttributes.length === 0) { + return null; + } + return ( + + {structuredAttributes.flatMap((attribute) => + attribute.values.map((value) => { + const attributeLabel = formatAttributeName(attribute.name); + return ( + + ); + }), + )} + + ); + }, [structuredAttributes]); const searching = searchTerms !== ''; @@ -245,19 +261,7 @@ export default function ChannelMembersRHS({ defaultMessage: 'Channel access is restricted by user attributes', })} > - {structuredAttributes.length > 0 && ( - - {structuredAttributes.flatMap((attribute) => - attribute.values.map((value) => ( - - )), - )} - - )} + {accessControlTags} {loading && {'Loading...'}} diff --git a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx index 35c4113e578..c32d0807151 100644 --- a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx +++ b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.test.tsx @@ -7,7 +7,7 @@ import {Provider} from 'react-redux'; import configureStore from 'redux-mock-store'; import {thunk} from 'redux-thunk'; -import {useAccessControlAttributes, EntityType} from './useAccessControlAttributes'; +import {invalidateAccessControlAttributesCache, useAccessControlAttributes, EntityType} from './useAccessControlAttributes'; // Mock the getChannelAccessControlAttributes action jest.mock('mattermost-redux/actions/channels', () => { @@ -202,4 +202,31 @@ describe('useAccessControlAttributes', () => { // The action should have been called again expect(getChannelAccessControlAttributes).toHaveBeenCalledWith('channel-1'); }); + + test('invalidateAccessControlAttributesCache forces a refresh on next read', async () => { + // Prime the cache with a first fetch. + const {result: result1} = renderHook(() => useAccessControlAttributes(EntityType.Channel, 'channel-1', true), {wrapper}); + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + expect(result1.current.structuredAttributes).toEqual([ + {name: 'department', values: ['engineering', 'marketing']}, + {name: 'location', values: ['remote']}, + ]); + + const getChannelAccessControlAttributes = require('mattermost-redux/actions/channels').getChannelAccessControlAttributes; + getChannelAccessControlAttributes.mockClear(); + + // After invalidating the cache the next mount should hit the action again. + invalidateAccessControlAttributesCache(EntityType.Channel, 'channel-1'); + + const {result: result2} = renderHook(() => useAccessControlAttributes(EntityType.Channel, 'channel-1', true), {wrapper}); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + + expect(getChannelAccessControlAttributes).toHaveBeenCalledWith('channel-1'); + expect(result2.current.structuredAttributes).toEqual(result1.current.structuredAttributes); + }); }); diff --git a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts index 1b21e86694f..d2c83392d41 100644 --- a/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts +++ b/webapp/channels/src/components/common/hooks/useAccessControlAttributes.ts @@ -15,22 +15,74 @@ export enum EntityType { // more entity types will be added here in the future } -// Module-level cache for access control attributes -// The cache stores processed data with a timestamp to implement a TTL (time-to-live) +// ProcessedAttributes is the shape that components consume: a structured +// dictionary of attribute -> values (used for the policy banners) plus a flat +// tag array (kept for backwards compatibility with consumers that only need +// the values without their owning attribute). +type ProcessedAttributes = { + attributeTags: string[]; + structuredAttributes: AccessControlAttribute[]; +}; + +// Module-level cache for access control attributes. The cache stores already +// processed data with a timestamp to implement a short TTL. const attributesCache: Record = {}; -// Cache TTL in milliseconds (5 minutes) +// Cache TTL in milliseconds (5 minutes). const CACHE_TTL = 5 * 60 * 1000; // Array of supported entity types for validation const SUPPORTED_ENTITY_TYPES = Object.values(EntityType); +const EMPTY_PROCESSED: ProcessedAttributes = { + attributeTags: [], + structuredAttributes: [], +}; + +// processAttributeData converts the server response (a dictionary keyed by +// attribute name with arrays of literal values) into the structured form +// consumed by the UI. The dictionary preserves both the attribute name and +// the values associated with it so each tag can be displayed alongside its +// originating attribute. +function processAttributeData(data: Record | undefined | null): ProcessedAttributes { + if (!data) { + return EMPTY_PROCESSED; + } + + const attributeTags: string[] = []; + const structuredAttributes: AccessControlAttribute[] = []; + + // Format: { "attributeName": ["value1", "value2"], "anotherAttribute": ["value3"] } + for (const [name, values] of Object.entries(data)) { + if (!Array.isArray(values)) { + continue; + } + + structuredAttributes.push({name, values: [...values]}); + + for (const value of values) { + if (value !== undefined && value !== null) { + attributeTags.push(value); + } + } + } + + return {attributeTags, structuredAttributes}; +} + +/** + * Invalidates the cached attributes for a single entity. Components that + * mutate the underlying access policy should call this to ensure the next + * read fetches fresh data instead of serving up to CACHE_TTL milliseconds of + * stale state. + */ +export function invalidateAccessControlAttributesCache(entityType: EntityType, entityId: string): void { + delete attributesCache[`${entityType}:${entityId}`]; +} + /** * A hook for fetching access control attributes for an entity * @@ -50,35 +102,9 @@ export const useAccessControlAttributes = ( const [error, setError] = useState(null); const dispatch = useDispatch(); - // Helper function to process attribute data and extract tags - const processAttributeData = useCallback((data: Record | undefined) => { - if (!data) { - setAttributeTags([]); - setStructuredAttributes([]); - return; - } - - const tags: string[] = []; - const attributes: AccessControlAttribute[] = []; - - // Extract values from all properties in the response - // Format: { "attributeName": ["value1", "value2"], "anotherAttribute": ["value3"] } - Object.entries(data).forEach(([name, values]) => { - // Add to structured format - if (Array.isArray(values)) { - attributes.push({name, values: [...values]}); - - // Add to flat tags (existing behavior) - values.forEach((value) => { - if (value !== undefined && value !== null) { - tags.push(value); - } - }); - } - }); - - setAttributeTags(tags); - setStructuredAttributes(attributes); + const applyProcessed = useCallback((processed: ProcessedAttributes) => { + setAttributeTags(processed.attributeTags); + setStructuredAttributes(processed.structuredAttributes); }, []); const fetchAttributes = useCallback(async (forceRefresh = false) => { @@ -86,32 +112,25 @@ export const useAccessControlAttributes = ( return; } - // Set loading state at the beginning setLoading(true); setError(null); try { - // Validate entity type first if (!SUPPORTED_ENTITY_TYPES.includes(entityType)) { throw new Error(`Unsupported entity type: ${entityType}`); } - // Check cache first (unless forceRefresh is true) const cacheKey = `${entityType}:${entityId}`; const cachedEntry = attributesCache[cacheKey]; const now = Date.now(); - // Use cache if it exists and is not too old and forceRefresh is false - // But still set loading to false to trigger a state update for tests + // Serve from cache when fresh and the caller didn't force a refresh. if (!forceRefresh && cachedEntry && (now - cachedEntry.timestamp < CACHE_TTL)) { - // Use the cached processed data directly instead of reprocessing - setAttributeTags(cachedEntry.processedData.attributeTags); - setStructuredAttributes(cachedEntry.processedData.structuredAttributes); + applyProcessed(cachedEntry.processedData); setLoading(false); return; } - // Handle different entity types let result; switch (entityType) { case EntityType.Channel: @@ -122,56 +141,21 @@ export const useAccessControlAttributes = ( throw new Error(`Unsupported entity type: ${entityType}`); } - // Check for error in the result if (result.error) { throw result.error; } - const data = result.data; - // Process the data and store it in cache - if (data) { - const processedTags: string[] = []; - const processedAttributes: AccessControlAttribute[] = []; - - // Process the data once - Object.entries(data).forEach(([name, values]) => { - if (Array.isArray(values)) { - processedAttributes.push({name, values: [...values]}); - values.forEach((value) => { - if (value !== undefined && value !== null) { - processedTags.push(value); - } - }); - } - }); - - // Store only the processed data - attributesCache[cacheKey] = { - processedData: { - attributeTags: processedTags, - structuredAttributes: processedAttributes, - }, - timestamp: now, - }; - - // Set state - setAttributeTags(processedTags); - setStructuredAttributes(processedAttributes); - } else { - // Handle the case where data is undefined or null - setAttributeTags([]); - setStructuredAttributes([]); - } + const processed = processAttributeData(result.data); + attributesCache[cacheKey] = {processedData: processed, timestamp: now}; + applyProcessed(processed); } catch (err) { setError(err as Error); - setAttributeTags([]); - setStructuredAttributes([]); + applyProcessed(EMPTY_PROCESSED); } finally { setLoading(false); } - }, [entityType, entityId, hasAccessControl, processAttributeData]); + }, [entityType, entityId, hasAccessControl, dispatch, applyProcessed]); - // Fetch attributes when the component mounts or when dependencies change useEffect(() => { fetchAttributes(); }, [fetchAttributes]); diff --git a/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap b/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap index 8b17f0a3de0..f0b8fb0305d 100644 --- a/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap +++ b/webapp/channels/src/components/sidebar/sidebar_channel/sidebar_channel_menu/__snapshots__/sidebar_channel_menu.test.tsx.snap @@ -32,55 +32,7 @@ exports[`components/sidebar/sidebar_channel/sidebar_channel_menu should match sn + /> + />