From 2a896f8420fbd7a71ba146688f4cc63ecba8bcdf Mon Sep 17 00:00:00 2001 From: Ben Cooke Date: Thu, 16 Nov 2023 11:24:20 -0500 Subject: [PATCH] [MM-54933] Add the ability to @ mention custom groups in group constrained teams and channels (#24987) * add the ability to @ mention custom groups in group constrained teams --------- Co-authored-by: Mattermost Build --- server/channels/app/notification.go | 12 ++++++ server/channels/app/notification_test.go | 39 +++++++++++++++---- .../src/selectors/entities/groups.test.ts | 38 +++++++++++++++++- .../src/selectors/entities/groups.ts | 19 +++++++-- 4 files changed, 95 insertions(+), 13 deletions(-) diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 065db7f63b..f1d72c8858 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -1151,6 +1151,18 @@ func (a *App) getGroupsAllowedForReferenceInChannel(channel *model.Channel, team groupsMap[group.Id] = &group.Group } } + + opts.Source = model.GroupSourceCustom + var customgroups []*model.Group + customgroups, err = a.Srv().Store().Group().GetGroups(0, 0, opts, nil) + if err != nil { + return nil, errors.Wrap(err, "unable to get custom groups") + } + for _, group := range customgroups { + if group.Name != nil { + groupsMap[group.Id] = group + } + } return groupsMap, nil } diff --git a/server/channels/app/notification_test.go b/server/channels/app/notification_test.go index cad0e63021..e41c3cfed6 100644 --- a/server/channels/app/notification_test.go +++ b/server/channels/app/notification_test.go @@ -2571,6 +2571,25 @@ func TestGetGroupsAllowedForReferenceInChannel(t *testing.T) { group2, err = th.App.UpdateGroup(group2) require.Nil(t, err) + // Create a custom group + customGroupId := model.NewId() + customGroup, err := th.App.CreateGroup(&model.Group{ + DisplayName: customGroupId, + Name: model.NewString("name" + customGroupId), + Source: model.GroupSourceCustom, + Description: "description_" + customGroupId, + AllowReference: true, + RemoteId: nil, + }) + assert.Nil(t, err) + + u1 := th.BasicUser + _, err = th.App.UpsertGroupMember(customGroup.Id, u1.Id) + assert.Nil(t, err) + + customGroup, err = th.App.GetGroup(customGroup.Id, &model.GetGroupOpts{IncludeMemberCount: true}, nil) + assert.Nil(t, err) + // Sync first group to constrained channel constrainedChannel := th.CreateChannel(th.Context, th.BasicTeam) constrainedChannel.GroupConstrained = model.NewBool(true) @@ -2583,15 +2602,16 @@ func TestGetGroupsAllowedForReferenceInChannel(t *testing.T) { }) require.Nil(t, err) - t.Run("should return only groups synced to channel if channel is group constrained", func(t *testing.T) { + t.Run("should return only groups synced to channel and custom groups if channel is group constrained", func(t *testing.T) { groupsMap, nErr := th.App.getGroupsAllowedForReferenceInChannel(constrainedChannel, team) require.NoError(t, nErr) - require.Len(t, groupsMap, 1) + require.Len(t, groupsMap, 2) require.Nil(t, groupsMap[group2.Id]) require.Equal(t, groupsMap[group1.Id], group1) + require.Equal(t, groupsMap[customGroup.Id], customGroup) }) - // Create a third group not synced with a team or channel + // Create a third ldap group not synced with a team or channel group3 := th.CreateGroup() group3.AllowReference = true group3, err = th.App.UpdateGroup(group3) @@ -2608,22 +2628,24 @@ func TestGetGroupsAllowedForReferenceInChannel(t *testing.T) { }) require.Nil(t, err) - t.Run("should return union of groups synced to team and any channels if team is group constrained", func(t *testing.T) { + t.Run("should return union of custom groups, groups synced to team and any channels if team is group constrained", func(t *testing.T) { groupsMap, nErr := th.App.getGroupsAllowedForReferenceInChannel(channel, team) require.NoError(t, nErr) - require.Len(t, groupsMap, 2) + require.Len(t, groupsMap, 3) require.Nil(t, groupsMap[group3.Id]) require.Equal(t, groupsMap[group2.Id], group2) require.Equal(t, groupsMap[group1.Id], group1) + require.Equal(t, groupsMap[customGroup.Id], customGroup) }) - t.Run("should return only subset of groups synced to channel for group constrained channel when team is also group constrained", func(t *testing.T) { + t.Run("should return only subset of custom groups and groups synced to channel for group constrained channel when team is also group constrained", func(t *testing.T) { groupsMap, nErr := th.App.getGroupsAllowedForReferenceInChannel(constrainedChannel, team) require.NoError(t, nErr) - require.Len(t, groupsMap, 1) + require.Len(t, groupsMap, 2) require.Nil(t, groupsMap[group3.Id]) require.Nil(t, groupsMap[group2.Id]) require.Equal(t, groupsMap[group1.Id], group1) + require.Equal(t, groupsMap[customGroup.Id], customGroup) }) team.GroupConstrained = model.NewBool(false) @@ -2633,10 +2655,11 @@ func TestGetGroupsAllowedForReferenceInChannel(t *testing.T) { t.Run("should return all groups when team and channel are not group constrained", func(t *testing.T) { groupsMap, nErr := th.App.getGroupsAllowedForReferenceInChannel(channel, team) require.NoError(t, nErr) - require.Len(t, groupsMap, 3) + require.Len(t, groupsMap, 4) require.Equal(t, groupsMap[group1.Id], group1) require.Equal(t, groupsMap[group2.Id], group2) require.Equal(t, groupsMap[group3.Id], group3) + require.Equal(t, groupsMap[customGroup.Id], customGroup) }) } diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.test.ts b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.test.ts index c07374d2a1..a75ed0430f 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.test.ts @@ -121,7 +121,7 @@ describe('Selectors.Groups', () => { }, teams: { teams: { - [teamID]: {group_constrained: false}, + [teamID]: {group_constrained: true, id: teamID}, }, groupsAssociatedToTeam: { [teamID]: {ids: teamAssociatedGroupIDs}, @@ -129,7 +129,7 @@ describe('Selectors.Groups', () => { }, channels: { channels: { - [channelID]: {team_id: teamID, id: channelID}, + [channelID]: {team_id: teamID, id: channelID, group_constrained: true}, }, groupsAssociatedToChannel: { [channelID]: {ids: channelAssociatedGroupIDs}, @@ -242,4 +242,38 @@ describe('Selectors.Groups', () => { ]; expect(Selectors.getMyGroupMentionKeysForChannel(testState, teamID, channelID)).toEqual(expected); }); + + it('getAssociatedGroupsForReference team constrained', () => { + const expected = [ + group5, + group1, + ]; + expect(Selectors.getAssociatedGroupsForReference(testState, teamID, '')).toEqual(expected); + }); + + it('getAssociatedGroupsForReference channel constrained', () => { + const expected = [ + group5, + group4, + ]; + expect(Selectors.getAssociatedGroupsForReference(testState, '', channelID)).toEqual(expected); + }); + + it('getAssociatedGroupsForReference team and channel constrained', () => { + const expected = [ + group4, + group1, + group5, + ]; + expect(Selectors.getAssociatedGroupsForReference(testState, teamID, channelID)).toEqual(expected); + }); + + it('getAssociatedGroupsForReference no constraints', () => { + const expected = [ + group1, + group4, + group5, + ]; + expect(Selectors.getAssociatedGroupsForReference(testState, '', '')).toEqual(expected); + }); }); diff --git a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.ts b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.ts index 2e4abde32b..476f1b3d21 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/selectors/entities/groups.ts @@ -125,11 +125,16 @@ export function getAssociatedGroupsForReference(state: GlobalState, teamId: stri if (team && team.group_constrained && channel && channel.group_constrained) { const groupsFromChannel = getGroupsAssociatedToChannelForReference(state, channelId); const groupsFromTeam = getGroupsAssociatedToTeamForReference(state, teamId); - groupsForReference = groupsFromChannel.concat(groupsFromTeam.filter((item) => groupsFromChannel.indexOf(item) < 0)); + const customGroups = getAllCustomGroups(state); + groupsForReference = groupsFromChannel.concat(groupsFromTeam.filter((item) => groupsFromChannel.indexOf(item) < 0), customGroups); } else if (team && team.group_constrained) { - groupsForReference = getGroupsAssociatedToTeamForReference(state, teamId); + const customGroups = getAllCustomGroups(state); + const groupsFromTeam = getGroupsAssociatedToTeamForReference(state, teamId); + groupsForReference = [...customGroups, ...groupsFromTeam]; } else if (channel && channel.group_constrained) { - groupsForReference = getGroupsAssociatedToChannelForReference(state, channelId); + const customGroups = getAllCustomGroups(state); + const groupsFromChannel = getGroupsAssociatedToChannelForReference(state, channelId); + groupsForReference = [...customGroups, ...groupsFromChannel]; } else { groupsForReference = getAllAssociatedGroupsForReference(state, false); } @@ -259,6 +264,14 @@ export const getAllGroupsForReferenceByName: (state: GlobalState) => Record Group[] = createSelector( + 'getAllCustomGroups', + getAllGroups, + (groups) => { + return Object.entries(groups).filter((entry) => (entry[1].allow_reference && entry[1].delete_at === 0 && entry[1].source === GroupSource.Custom)).map((entry) => entry[1]); + }, +); + export const makeGetMyAllowReferencedGroups = () => { return createSelector( 'makeGetMyAllowReferencedGroups',