From c21b61bef237f50b8b20ff8aeb5ee7b4a3eeceaf Mon Sep 17 00:00:00 2001 From: Farhan Munshi <3207297+fmunshi@users.noreply.github.com> Date: Wed, 26 Aug 2020 13:00:32 -0400 Subject: [PATCH] [MM-27132] Handle group mention keys for group synced channels and teams (#4726) * Handle group synced channel highlighting better * Add tests * Dont check if teamRoleFound if no teamId given for haveIChannelPermission * Pass team id for channel mentions and group mentions checks * Actually make a selector * Check for falsey teamId instead of undefined * Remove true && * Iterate over object values instead of for in --- app/components/at_mention/index.js | 4 +- .../autocomplete/at_mention/index.js | 3 +- app/components/post_body/index.js | 3 +- .../post_draft/draft_input/index.js | 2 + app/mm-redux/selectors/entities/groups.ts | 79 ++++--- app/mm-redux/selectors/entities/roles.ts | 10 +- .../selectors/entities/search.test.js | 201 +++++++++--------- app/mm-redux/selectors/entities/search.ts | 42 ++-- 8 files changed, 181 insertions(+), 163 deletions(-) diff --git a/app/components/at_mention/index.js b/app/components/at_mention/index.js index bfa8d48f6..ba6887d32 100644 --- a/app/components/at_mention/index.js +++ b/app/components/at_mention/index.js @@ -9,7 +9,7 @@ import {getAllUserMentionKeys} from '@mm-redux/selectors/entities/search'; import {getTeammateNameDisplaySetting, getTheme} from '@mm-redux/selectors/entities/preferences'; -import {getGroupsByName} from '@mm-redux/selectors/entities/groups'; +import {getAllGroupsForReferenceByName} from '@mm-redux/selectors/entities/groups'; import AtMention from './at_mention'; @@ -19,7 +19,7 @@ function mapStateToProps(state, ownProps) { usersByUsername: getUsersByUsername(state), mentionKeys: ownProps.mentionKeys || getAllUserMentionKeys(state), teammateNameDisplay: getTeammateNameDisplaySetting(state), - groupsByName: getGroupsByName(state), + groupsByName: getAllGroupsForReferenceByName(state), }; } diff --git a/app/components/autocomplete/at_mention/index.js b/app/components/autocomplete/at_mention/index.js index 73cacc8c0..1bfc69d08 100644 --- a/app/components/autocomplete/at_mention/index.js +++ b/app/components/autocomplete/at_mention/index.js @@ -37,6 +37,7 @@ function mapStateToProps(state, ownProps) { state, { channel: currentChannelId, + team: currentTeamId, permission: Permissions.USE_CHANNEL_MENTIONS, default: true, }, @@ -57,7 +58,7 @@ function mapStateToProps(state, ownProps) { outChannel = filterMembersNotInChannel(state, matchTerm); } - if (hasLicense && isMinimumServerVersion(state.entities.general.serverVersion, 5, 24)) { + if (haveIChannelPermission(state, {channel: currentChannelId, team: currentTeamId, permission: Permissions.USE_GROUP_MENTIONS, default: true}) && hasLicense && isMinimumServerVersion(state.entities.general.serverVersion, 5, 24)) { if (matchTerm) { groups = searchAssociatedGroupsForReferenceLocal(state, matchTerm, currentTeamId, currentChannelId); } else { diff --git a/app/components/post_body/index.js b/app/components/post_body/index.js index 508e617ef..28ca0f0f1 100644 --- a/app/components/post_body/index.js +++ b/app/components/post_body/index.js @@ -33,6 +33,7 @@ const POST_TIMEOUT = 20000; export function makeMapStateToProps() { const memoizeHasEmojisOnly = memoizeResult((message, customEmojis) => hasEmojisOnly(message, customEmojis)); const getReactionsForPost = makeGetReactionsForPost(); + const getMentionKeysForPost = makeGetMentionKeysForPost(); return (state, ownProps) => { const post = ownProps.post; @@ -104,7 +105,7 @@ export function makeMapStateToProps() { isEmojiOnly, shouldRenderJumboEmoji, theme: getTheme(state), - mentionKeys: makeGetMentionKeysForPost(state, postProps?.disable_group_highlight, postProps?.mentionHighlightDisabled), + mentionKeys: getMentionKeysForPost(state, channel, postProps?.disable_group_highlight, postProps?.mentionHighlightDisabled), canDelete, ...getDimensions(state), }; diff --git a/app/components/post_draft/draft_input/index.js b/app/components/post_draft/draft_input/index.js index ca2d76d76..253ddaa9a 100644 --- a/app/components/post_draft/draft_input/index.js +++ b/app/components/post_draft/draft_input/index.js @@ -47,6 +47,7 @@ export function mapStateToProps(state, ownProps) { state, { channel: channel.id, + team: channel.team_id, permission: Permissions.USE_CHANNEL_MENTIONS, default: true, }, @@ -60,6 +61,7 @@ export function mapStateToProps(state, ownProps) { channel: channel.id, team: channel.team_id, permission: Permissions.USE_GROUP_MENTIONS, + default: true, }, ); diff --git a/app/mm-redux/selectors/entities/groups.ts b/app/mm-redux/selectors/entities/groups.ts index e46beb8aa..fc2964520 100644 --- a/app/mm-redux/selectors/entities/groups.ts +++ b/app/mm-redux/selectors/entities/groups.ts @@ -2,15 +2,13 @@ // See LICENSE.txt for license information. import * as reselect from 'reselect'; import {GlobalState} from '@mm-redux/types/store'; -import {Dictionary} from '@mm-redux/types/utilities'; +import {Dictionary, NameMappedObjects} from '@mm-redux/types/utilities'; import {Group} from '@mm-redux/types/groups'; import {filterGroupsMatchingTerm} from '@mm-redux/utils/group_utils'; import {getCurrentUserLocale} from '@mm-redux/selectors/entities/i18n'; import {getChannel} from '@mm-redux/selectors/entities/channels'; import {UserMentionKey} from '@mm-redux/selectors/entities/users'; -import {haveIChannelPermission} from '@mm-redux/selectors/entities/roles'; import {getTeam} from '@mm-redux/selectors/entities/teams'; -import {Permissions} from '@mm-redux/constants'; const emptyList: any[] = []; const emptySyncables = { @@ -73,15 +71,6 @@ export function getAssociatedGroupsForReference(state: GlobalState, teamId: stri const channel = getChannel(state, channelId); const locale = getCurrentUserLocale(state); - if (!haveIChannelPermission(state, { - permission: Permissions.USE_GROUP_MENTIONS, - channel: channelId, - team: teamId, - default: true, - })) { - return emptyList; - } - let groupsForReference = []; if (team && team.group_constrained && channel && channel.group_constrained) { const groupsFromChannel = getGroupsAssociatedToChannelForReference(state, channelId); @@ -173,24 +162,8 @@ export const getAllAssociatedGroupsForReference = reselect.createSelector( }, ); -export const getMyAllowReferencedGroups = reselect.createSelector( - getMyGroups, - (myGroups) => { - return Object.values(myGroups).filter((group) => group.allow_reference && group.delete_at === 0); - }, -); - -export const getCurrentUserGroupMentionKeys = reselect.createSelector( - getMyAllowReferencedGroups, - (groups: Array) => { - const keys: UserMentionKey[] = []; - groups.forEach((group) => keys.push({key: `@${group.name}`})); - return keys; - }, -); - -export const getGroupsByName = reselect.createSelector( - getAllGroups, +export const getAssociatedGroupsByName: (state: GlobalState, teamID: string, channelId: string) => NameMappedObjects = reselect.createSelector( + getAssociatedGroupsForReference, (groups) => { const groupsByName: Dictionary = {}; @@ -201,3 +174,49 @@ export const getGroupsByName = reselect.createSelector( return groupsByName; }, ); + +export const getAllGroupsForReferenceByName: (state: GlobalState) => NameMappedObjects = reselect.createSelector( + getAllAssociatedGroupsForReference, + (groups) => { + const groupsByName: Dictionary = {}; + + Object.values(groups).forEach((group) => { + groupsByName[group.name] = group; + }); + + return groupsByName; + }, +); + +export const getMyAllowReferencedGroups = reselect.createSelector( + getMyGroups, + (myGroups) => { + return Object.values(myGroups).filter((group) => group.allow_reference && group.delete_at === 0); + }, +); + +export const getMyGroupMentionKeys: (state: GlobalState) => UserMentionKey[] = reselect.createSelector( + getMyAllowReferencedGroups, + (groups: Array) => { + const keys: UserMentionKey[] = []; + groups.forEach((group) => keys.push({key: `@${group.name}`})); + return keys; + }, +); + +export const getMyGroupsAssociatedToChannelForReference: (state: GlobalState, teamId: string, channelId: string) => Group[] = reselect.createSelector( + getMyGroups, + getAssociatedGroupsByName, + (myGroups, groups) => { + return Object.values(myGroups).filter((group) => group.allow_reference && group.delete_at === 0 && groups[group.name]); + }, +); + +export const getMyGroupMentionKeysForChannel: (state: GlobalState, teamId: string, channelId: string) => UserMentionKey[] = reselect.createSelector( + getMyGroupsAssociatedToChannelForReference, + (groups: Array) => { + const keys: UserMentionKey[] = []; + groups.forEach((group) => keys.push({key: `@${group.name}`})); + return keys; + }, +); diff --git a/app/mm-redux/selectors/entities/roles.ts b/app/mm-redux/selectors/entities/roles.ts index 72c9f8995..78e7ba2f5 100644 --- a/app/mm-redux/selectors/entities/roles.ts +++ b/app/mm-redux/selectors/entities/roles.ts @@ -104,7 +104,7 @@ export const getMyCurrentChannelPermissions = reselect.createSelector( for (const permission of roles[roleName].permissions) { permissions.add(permission); } - roleFound = true && teamRoleFound; + roleFound = teamRoleFound; } } } @@ -144,8 +144,10 @@ export const getMyChannelPermissions = reselect.createSelector( getMyChannelRoles, getRoles, getMyTeamPermissions, - (state, options: PermissionsOptions) => options.channel, - (myChannelRoles, roles, {permissions: teamPermissions, roleFound: teamRoleFound}, channelId) => { + (state, options: PermissionsOptions) => options, + (myChannelRoles, roles, {permissions: teamPermissions, roleFound: teamRoleFound}, options: PermissionsOptions) => { + const channelId = options.channel; + const teamId = options.team; const permissions = new Set(); let roleFound = false; if (myChannelRoles[channelId!]) { @@ -154,7 +156,7 @@ export const getMyChannelPermissions = reselect.createSelector( for (const permission of roles[roleName].permissions) { permissions.add(permission); } - roleFound = true && teamRoleFound; + roleFound = teamRoleFound || !teamId; } } } diff --git a/app/mm-redux/selectors/entities/search.test.js b/app/mm-redux/selectors/entities/search.test.js index 5312508ed..5dfcc6280 100644 --- a/app/mm-redux/selectors/entities/search.test.js +++ b/app/mm-redux/selectors/entities/search.test.js @@ -65,38 +65,104 @@ describe('Selectors.Search', () => { assert.deepEqual(Selectors.getAllUserMentionKeys(state), [{key: 'First', caseSensitive: true}, {key: '@user'}, {key: '@I-AM-THE-BEST!'}, {key: '@Do-you-love-me?'}]); }); - describe('makeGetMentionKeysForPost', () => { - it('should return all mentionKeys', () => { - const postProps = { - disable_group_highlight: false, - mentionHighlightDisabled: false, - }; - const state = { - entities: { - users: { - currentUserId: 'a123', - profiles: { - a123: { - username: 'a123', - notify_props: { - channel: 'true', - }, - }, - }, - }, - groups: { - myGroups: { - developers: { - id: 123, - name: 'developers', - allow_reference: true, - delete_at: 0, + describe('getMentionKeysForPost', () => { + const group = { + id: 123, + name: 'developers', + allow_reference: true, + delete_at: 0, + }; + + const group2 = { + id: 1234, + name: 'developers2', + allow_reference: true, + delete_at: 0, + }; + + const channel1 = {...TestHelper.fakeChannelWithId(team1.id), group_constrained: true}; + const channel2 = {...TestHelper.fakeChannelWithId(team1.id), group_constrained: false}; + + const state = { + entities: { + users: { + currentUserId: 'a123', + profiles: { + a123: { + username: 'a123', + notify_props: { + channel: 'true', }, }, }, }, + groups: { + syncables: {}, + members: {}, + groups: { + [group.name]: group, + [group2.name]: group2, + }, + myGroups: { + [group.name]: group, + [group2.name]: group2, + }, + }, + teams: { + teams: { + [team1.id]: team1, + }, + groupsAssociatedToTeam: { + [team1.id]: {ids: []}, + }, + }, + channels: { + channels: { + [channel1.id]: channel1, + [channel2.id]: channel2, + }, + groupsAssociatedToChannel: { + [channel1.id]: {ids: [group.id]}, + [channel2.id]: {ids: []}, + }, + }, + general: { + config: {}, + }, + preferences: { + myPreferences: {}, + }, + }, + }; + + const getMentionKeysForPost = Selectors.makeGetMentionKeysForPost(); + + it('should return all mentionKeys for post if null channel given', () => { + const postProps = { + disable_group_highlight: false, + mentionHighlightDisabled: false, }; - const results = Selectors.makeGetMentionKeysForPost(state, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const results = getMentionKeysForPost(state, null, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const expected = [{key: '@channel'}, {key: '@all'}, {key: '@here'}, {key: '@a123'}, {key: '@developers'}, {key: '@developers2'}]; + assert.deepEqual(results, expected); + }); + + it('should return all mentionKeys for post made in non group constrained channel', () => { + const postProps = { + disable_group_highlight: false, + mentionHighlightDisabled: false, + }; + const results = getMentionKeysForPost(state, channel2, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const expected = [{key: '@channel'}, {key: '@all'}, {key: '@here'}, {key: '@a123'}, {key: '@developers'}, {key: '@developers2'}]; + assert.deepEqual(results, expected); + }); + + it('should return mentionKeys for post made in group constrained channel', () => { + const postProps = { + disable_group_highlight: false, + mentionHighlightDisabled: false, + }; + const results = getMentionKeysForPost(state, channel1, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); const expected = [{key: '@channel'}, {key: '@all'}, {key: '@here'}, {key: '@a123'}, {key: '@developers'}]; assert.deepEqual(results, expected); }); @@ -106,32 +172,7 @@ describe('Selectors.Search', () => { disable_group_highlight: true, mentionHighlightDisabled: false, }; - const state = { - entities: { - users: { - currentUserId: 'a123', - profiles: { - a123: { - username: 'a123', - notify_props: { - channel: 'true', - }, - }, - }, - }, - groups: { - myGroups: { - developers: { - id: 123, - name: 'developers', - allow_reference: true, - delete_at: 0, - }, - }, - }, - }, - }; - const results = Selectors.makeGetMentionKeysForPost(state, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const results = getMentionKeysForPost(state, channel1, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); const expected = [{key: '@channel'}, {key: '@all'}, {key: '@here'}, {key: '@a123'}]; assert.deepEqual(results, expected); }); @@ -141,32 +182,7 @@ describe('Selectors.Search', () => { disable_group_highlight: false, mentionHighlightDisabled: true, }; - const state = { - entities: { - users: { - currentUserId: 'a123', - profiles: { - a123: { - username: 'a123', - notify_props: { - channel: 'true', - }, - }, - }, - }, - groups: { - myGroups: { - developers: { - id: 123, - name: 'developers', - allow_reference: true, - delete_at: 0, - }, - }, - }, - }, - }; - const results = Selectors.makeGetMentionKeysForPost(state, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const results = getMentionKeysForPost(state, channel1, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); const expected = [{key: '@a123'}, {key: '@developers'}]; assert.deepEqual(results, expected); }); @@ -176,32 +192,7 @@ describe('Selectors.Search', () => { disable_group_highlight: true, mentionHighlightDisabled: true, }; - const state = { - entities: { - users: { - currentUserId: 'a123', - profiles: { - a123: { - username: 'a123', - notify_props: { - channel: 'true', - }, - }, - }, - }, - groups: { - myGroups: { - developers: { - id: 123, - name: 'developers', - allow_reference: true, - delete_at: 0, - }, - }, - }, - }, - }; - const results = Selectors.makeGetMentionKeysForPost(state, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); + const results = getMentionKeysForPost(state, channel1, postProps.disable_group_highlight, postProps.mentionHighlightDisabled); const expected = [{key: '@a123'}]; assert.deepEqual(results, expected); }); diff --git a/app/mm-redux/selectors/entities/search.ts b/app/mm-redux/selectors/entities/search.ts index 92356b394..48ae82811 100644 --- a/app/mm-redux/selectors/entities/search.ts +++ b/app/mm-redux/selectors/entities/search.ts @@ -3,9 +3,10 @@ import * as reselect from 'reselect'; import {GlobalState} from '@mm-redux/types/store'; +import {Channel} from '@mm-redux/types/channels'; import {UserMentionKey} from './users'; import {getCurrentUserMentionKeys} from '@mm-redux/selectors/entities/users'; -import {getCurrentUserGroupMentionKeys} from '@mm-redux/selectors/entities/groups'; +import {getMyGroupMentionKeys, getMyGroupMentionKeysForChannel} from '@mm-redux/selectors/entities/groups'; import {getCurrentTeamId} from '@mm-redux/selectors/entities/teams'; @@ -19,29 +20,30 @@ export const getCurrentSearchForCurrentTeam = reselect.createSelector( export const getAllUserMentionKeys: (state: GlobalState) => UserMentionKey[] = reselect.createSelector( getCurrentUserMentionKeys, - (state: GlobalState) => getCurrentUserGroupMentionKeys(state), + (state: GlobalState) => getMyGroupMentionKeys(state), (userMentionKeys, groupMentionKeys) => { return userMentionKeys.concat(groupMentionKeys); }, ); -export const makeGetMentionKeysForPost: (state: GlobalState, disableGroupHighlight: boolean, mentionHighlightDisabled: boolean) => UserMentionKey[] = reselect.createSelector( - getAllUserMentionKeys, - getCurrentUserMentionKeys, - (state: GlobalState, disableGroupHighlight: boolean) => disableGroupHighlight, - (state: GlobalState, disableGroupHighlight: boolean, mentionHighlightDisabled: boolean) => mentionHighlightDisabled, - (allMentionKeys, mentionKeysWithoutGroups, disableGroupHighlight = false, mentionHighlightDisabled = false) => { - let mentionKeys = allMentionKeys; - if (disableGroupHighlight) { - mentionKeys = mentionKeysWithoutGroups; - } +export function makeGetMentionKeysForPost(): (state: GlobalState, channel: Channel, disableGroupHighlight: boolean, mentionHighlightDisabled: boolean) => UserMentionKey[] { + return reselect.createSelector( + getCurrentUserMentionKeys, + (state: GlobalState, channel: Channel) => (channel?.id ? getMyGroupMentionKeysForChannel(state, channel?.team_id, channel?.id) : getMyGroupMentionKeys(state)), + (state: GlobalState, channel: Channel, disableGroupHighlight: boolean) => disableGroupHighlight, + (state: GlobalState, channel: Channel, disableGroupHighlight: boolean, mentionHighlightDisabled: boolean) => mentionHighlightDisabled, + (mentionKeysWithoutGroups, groupMentionKeys, disableGroupHighlight = false, mentionHighlightDisabled = false) => { + let mentionKeys = mentionKeysWithoutGroups; + if (!disableGroupHighlight) { + mentionKeys = mentionKeys.concat(groupMentionKeys); + } - if (mentionHighlightDisabled) { - const CHANNEL_MENTIONS = ['@all', '@channel', '@here']; - mentionKeys = mentionKeys.filter((value) => !CHANNEL_MENTIONS.includes(value.key)); - } - - return mentionKeys; - }, -); + if (mentionHighlightDisabled) { + const CHANNEL_MENTIONS = ['@all', '@channel', '@here']; + mentionKeys = mentionKeys.filter((value) => !CHANNEL_MENTIONS.includes(value.key)); + } + return mentionKeys; + }, + ); +}