Updates post text processing to fetch nonexisting custom groups (#24292)

* Updates getProfilesAndStatusesForPosts to fetch nonexisting custom groups

* Fix linter

* Rename method

* Adds an action test

* Using a set on loadedProfiles instead of an array
This commit is contained in:
Miguel de la Cruz 2023-08-21 17:20:49 +02:00 committed by GitHub
parent dd73c2af0f
commit be68398c7f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 114 additions and 59 deletions

View File

@ -67,7 +67,7 @@ global.Date.now = jest.fn(() => POST_CREATED_TIME);
jest.mock('mattermost-redux/actions/posts', () => ({
getPostThread: (...args: any) => ({type: 'MOCK_GET_POST_THREAD', args}),
getProfilesAndStatusesForPosts: (...args: any) => ({type: 'MOCK_GET_PROFILES_AND_STATUSES_FOR_POSTS', args}),
getMentionsAndStatusesForPosts: (...args: any) => ({type: 'MOCK_GET_MENTIONS_AND_STATUSES_FOR_POSTS', args}),
}));
jest.mock('mattermost-redux/actions/search', () => ({

View File

@ -53,7 +53,7 @@ import {
getCustomEmojiForReaction,
getPosts,
getPostThread,
getProfilesAndStatusesForPosts,
getMentionsAndStatusesForPosts,
getThreadsForPosts,
postDeleted,
receivedNewPost,
@ -686,7 +686,7 @@ export function handleNewPostEvent(msg) {
myDispatch(handleNewPost(post, msg));
getProfilesAndStatusesForPosts([post], myDispatch, myGetState);
getMentionsAndStatusesForPosts([post], myDispatch, myGetState);
// Since status updates aren't real time, assume another user is online if they have posted and:
// 1. The user hasn't set their status manually to something that isn't online
@ -725,7 +725,7 @@ export function handleNewPostEvents(queue) {
myDispatch(getThreadsForPosts(posts));
// And any other data needed for them
getProfilesAndStatusesForPosts(posts, myDispatch, myGetState);
getMentionsAndStatusesForPosts(posts, myDispatch, myGetState);
};
}
@ -741,7 +741,7 @@ export function handlePostEditEvent(msg) {
const crtEnabled = isCollapsedThreadsEnabled(getState());
dispatch(receivedPost(post, crtEnabled));
getProfilesAndStatusesForPosts([post], dispatch, getState);
getMentionsAndStatusesForPosts([post], dispatch, getState);
}
async function handlePostDeleteEvent(msg) {

View File

@ -2,7 +2,7 @@
// See LICENSE.txt for license information.
import {
getProfilesAndStatusesForPosts,
getMentionsAndStatusesForPosts,
getThreadsForPosts,
receivedNewPost,
} from 'mattermost-redux/actions/posts';
@ -44,7 +44,7 @@ import {
jest.mock('mattermost-redux/actions/posts', () => ({
...jest.requireActual('mattermost-redux/actions/posts'),
getThreadsForPosts: jest.fn(() => ({type: 'GET_THREADS_FOR_POSTS'})),
getProfilesAndStatusesForPosts: jest.fn(),
getMentionsAndStatusesForPosts: jest.fn(),
}));
jest.mock('mattermost-redux/actions/groups', () => ({
@ -491,7 +491,7 @@ describe('handleNewPostEvent', () => {
};
testStore.dispatch(handleNewPostEvent(msg));
expect(getProfilesAndStatusesForPosts).toHaveBeenCalledWith([post], expect.anything(), expect.anything());
expect(getMentionsAndStatusesForPosts).toHaveBeenCalledWith([post], expect.anything(), expect.anything());
expect(handleNewPost).toHaveBeenCalledWith(post, msg);
});
@ -623,7 +623,7 @@ describe('handleNewPostEvents', () => {
},
]);
expect(getThreadsForPosts).toHaveBeenCalledWith(posts);
expect(getProfilesAndStatusesForPosts).toHaveBeenCalledWith(posts, expect.anything(), expect.anything());
expect(getMentionsAndStatusesForPosts).toHaveBeenCalledWith(posts, expect.anything(), expect.anything());
});
});

View File

@ -61,7 +61,7 @@ export function initializeTeam(team: Team): ActionFunc<Team, ServerError> {
if (team.group_constrained && license.LDAPGroups === 'true') {
dispatch(getAllGroupsAssociatedToTeam(team.id, true));
} else {
dispatch(getGroups(false, 0, 60, true));
dispatch(getGroups('', false, 0, 60, true));
}
}

View File

@ -275,7 +275,7 @@ describe('Actions.Groups', () => {
get('/groups?filter_allow_reference=true&page=0&per_page=0').
reply(200, response1.groups);
await Actions.getGroups(true, 0, 0)(store.dispatch, store.getState);
await Actions.getGroups('', true, 0, 0)(store.dispatch, store.getState);
const state = store.getState();
@ -876,4 +876,3 @@ describe('Actions.Groups', () => {
expect(JSON.stringify(response) === JSON.stringify(stats[groupID])).toBeTruthy();
});
});

View File

@ -156,14 +156,15 @@ export function getGroup(id: string, includeMemberCount = false): ActionFunc {
});
}
export function getGroups(filterAllowReference = false, page = 0, perPage = 10, includeMemberCount = false): ActionFunc {
export function getGroups(q = '', filterAllowReference = false, page = 0, perPage = 10, includeMemberCount = false): ActionFunc {
return bindClientFunc({
clientFunc: async (param1, param2, param3, param4) => {
const result = await Client4.getGroups(param1, param2, param3, param4);
clientFunc: async (param1, param2, param3, param4, param5) => {
const result = await Client4.getGroups(param1, param2, param3, param4, param5);
return result;
},
onSuccess: [GroupTypes.RECEIVED_GROUPS],
params: [
q,
filterAllowReference,
page,
perPage,

View File

@ -603,65 +603,80 @@ describe('Actions.Posts', () => {
},
},
},
groups: {
groups: [
{
id: '1',
name: 'zzz',
},
],
},
},
} as unknown as GlobalState;
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: 'aaa'}),
])).toEqual(
new Set(),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@aaa'}),
])).toEqual(
new Set(),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
TestHelper.getPostMock({message: '@aaa @bbb @ccc'}),
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@zzz'}),
])).toEqual(
new Set(),
);
expect(
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@aaa @bbb @ccc @zzz'}),
])).toEqual(
new Set(['bbb', 'ccc']),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@bbb. @ccc.ddd'}),
])).toEqual(
new Set(['bbb.', 'bbb', 'ccc.ddd']),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@bbb- @ccc-ddd'}),
])).toEqual(
new Set(['bbb-', 'bbb', 'ccc-ddd']),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@bbb_ @ccc_ddd'}),
])).toEqual(
new Set(['bbb_', 'ccc_ddd']),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '(@bbb/@ccc) ddd@eee'}),
])).toEqual(
new Set(['bbb', 'ccc']),
);
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({
message: '@aaa @bbb',
props: {
attachments: [
{text: '@ccc @ddd'},
{text: '@ccc @ddd @zzz'},
{pretext: '@eee @fff', text: '@ggg'},
],
},
@ -673,7 +688,7 @@ describe('Actions.Posts', () => {
// should never try to request usernames matching special mentions
expect(
Actions.getNeededAtMentionedUsernames(state, [
Actions.getNeededAtMentionedUsernamesAndGroups(state, [
TestHelper.getPostMock({message: '@all'}),
TestHelper.getPostMock({message: '@here'}),
TestHelper.getPostMock({message: '@channel'}),
@ -1512,7 +1527,7 @@ describe('Actions.Posts', () => {
expect(index === 2).toBeTruthy();
});
describe('getProfilesAndStatusesForPosts', () => {
describe('getMentionsAndStatusesForPosts', () => {
describe('different values for posts argument', () => {
// Mock the state to prevent any followup requests since we aren't testing those
const currentUserId = 'user';
@ -1536,13 +1551,13 @@ describe('Actions.Posts', () => {
})) as unknown as GetStateFunc;
it('null', async () => {
await Actions.getProfilesAndStatusesForPosts(null as any, dispatch as any, getState);
await Actions.getMentionsAndStatusesForPosts(null as any, dispatch as any, getState);
});
it('array of posts', async () => {
const posts = [post];
await Actions.getProfilesAndStatusesForPosts(posts, dispatch as any, getState);
await Actions.getMentionsAndStatusesForPosts(posts, dispatch as any, getState);
});
it('object map of posts', async () => {
@ -1550,7 +1565,7 @@ describe('Actions.Posts', () => {
[post.id]: post,
};
await Actions.getProfilesAndStatusesForPosts(posts, dispatch as any, getState);
await Actions.getMentionsAndStatusesForPosts(posts, dispatch as any, getState);
});
});
});

View File

@ -5,6 +5,7 @@ import {AnyAction} from 'redux';
import {batchActions} from 'redux-batched-actions';
import {UserProfile} from '@mattermost/types/users';
import {Group} from '@mattermost/types/groups';
import {Reaction} from '@mattermost/types/reactions';
import {Post, PostList, PostAcknowledgement} from '@mattermost/types/posts';
import {GlobalState} from '@mattermost/types/store';
@ -21,12 +22,14 @@ import {getCurrentChannelId, getMyChannelMember as getMyChannelMemberSelector} f
import {getCustomEmojisByName as selectCustomEmojisByName} from 'mattermost-redux/selectors/entities/emojis';
import * as PostSelectors from 'mattermost-redux/selectors/entities/posts';
import {getCurrentUserId, getUsersByUsername} from 'mattermost-redux/selectors/entities/users';
import {getAllGroupsByName} from 'mattermost-redux/selectors/entities/groups';
import {getUnreadScrollPositionPreference, isCollapsedThreadsEnabled} from 'mattermost-redux/selectors/entities/preferences';
import {isCombinedUserActivityPost} from 'mattermost-redux/utils/post_list';
import {General, Preferences, Posts} from 'mattermost-redux/constants';
import {getGroups} from 'mattermost-redux/actions/groups';
import {getProfilesByIds, getProfilesByUsernames, getStatusesByIds} from 'mattermost-redux/actions/users';
import {
deletePreferences,
@ -146,7 +149,7 @@ export function getPost(postId: string) {
try {
post = await Client4.getPost(postId);
getProfilesAndStatusesForPosts([post], dispatch, getState);
getMentionsAndStatusesForPosts([post], dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch({type: PostTypes.GET_POSTS_FAILURE, error});
@ -767,7 +770,7 @@ export function getPostThread(rootId: string, fetchThreads = true) {
let posts;
try {
posts = await getPaginatedPostThread(rootId, {fetchThreads, collapsedThreads: collapsedThreadsEnabled});
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch({type: PostTypes.GET_POST_THREAD_FAILURE, error});
@ -808,7 +811,7 @@ export function getNewestPostThread(rootId: string) {
let posts;
try {
posts = await getPaginatedPostThread(rootId, options);
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch({type: PostTypes.GET_POST_THREAD_FAILURE, error});
@ -834,7 +837,7 @@ export function getPosts(channelId: string, page = 0, perPage = Posts.POST_CHUNK
const collapsedThreadsEnabled = isCollapsedThreadsEnabled(getState());
try {
posts = await Client4.getPosts(channelId, page, perPage, fetchThreads, collapsedThreadsEnabled, collapsedThreadsExtended);
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -864,7 +867,7 @@ export function getPostsUnread(channelId: string, fetchThreads = true, collapsed
recentPosts = await Client4.getPosts(channelId, 0, Posts.POST_CHUNK_SIZE / 2, fetchThreads, collapsedThreadsEnabled, collapsedThreadsExtended);
}
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -897,7 +900,7 @@ export function getPostsSince(channelId: string, since: number, fetchThreads = t
try {
const collapsedThreadsEnabled = isCollapsedThreadsEnabled(getState());
posts = await Client4.getPostsSince(channelId, since, fetchThreads, collapsedThreadsEnabled, collapsedThreadsExtended);
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -922,7 +925,7 @@ export function getPostsBefore(channelId: string, postId: string, page = 0, perP
try {
const collapsedThreadsEnabled = isCollapsedThreadsEnabled(getState());
posts = await Client4.getPostsBefore(channelId, postId, page, perPage, fetchThreads, collapsedThreadsEnabled, collapsedThreadsExtended);
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -944,7 +947,7 @@ export function getPostsAfter(channelId: string, postId: string, page = 0, perPa
try {
const collapsedThreadsEnabled = isCollapsedThreadsEnabled(getState());
posts = await Client4.getPostsAfter(channelId, postId, page, perPage, fetchThreads, collapsedThreadsEnabled, collapsedThreadsExtended);
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch(logError(error));
@ -996,7 +999,7 @@ export function getPostsAround(channelId: string, postId: string, perPage = Post
first_inaccessible_post_time: Math.max(before.first_inaccessible_post_time, after.first_inaccessible_post_time, thread.first_inaccessible_post_time) || 0,
};
getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
dispatch(batchActions([
receivedPosts(posts),
@ -1038,8 +1041,8 @@ export function getThreadsForPosts(posts: Post[], fetchThreads = true) {
};
}
// Note that getProfilesAndStatusesForPosts can take either an array of posts or a map of ids to posts
export function getProfilesAndStatusesForPosts(postsArrayOrMap: Post[]|PostList['posts'], dispatch: DispatchFunc, getState: GetStateFunc) {
// Note that getMentionsAndStatusesForPosts can take either an array of posts or a map of ids to posts
export async function getMentionsAndStatusesForPosts(postsArrayOrMap: Post[]|PostList['posts'], dispatch: DispatchFunc, getState: GetStateFunc) {
if (!postsArrayOrMap) {
// Some API methods return {error} for no results
return Promise.resolve();
@ -1112,10 +1115,19 @@ export function getProfilesAndStatusesForPosts(postsArrayOrMap: Post[]|PostList[
}
// Profiles of users mentioned in the posts
const usernamesToLoad = getNeededAtMentionedUsernames(state, postsArray);
const usernamesAndGroupsToLoad = getNeededAtMentionedUsernamesAndGroups(state, postsArray);
if (usernamesToLoad.size > 0) {
promises.push(getProfilesByUsernames(Array.from(usernamesToLoad))(dispatch, getState));
if (usernamesAndGroupsToLoad.size > 0) {
// We need to load the profiles synchronously to filter them
// out of the groups to check
const getProfilesPromise = getProfilesByUsernames(Array.from(usernamesAndGroupsToLoad))(dispatch, getState);
promises.push(getProfilesPromise);
const {data} = await getProfilesPromise as ActionResult<UserProfile[]>;
const loadedProfiles = new Set<string>((data || []).map((p) => p.username));
const groupsToCheck = Array.from(usernamesAndGroupsToLoad).filter((name) => !loadedProfiles.has(name));
groupsToCheck.forEach((name) => promises.push(getGroups(name)(dispatch, getState)));
}
return Promise.all(promises);
@ -1150,12 +1162,13 @@ export function getPostEditHistory(postId: string) {
});
}
export function getNeededAtMentionedUsernames(state: GlobalState, posts: Post[]): Set<string> {
export function getNeededAtMentionedUsernamesAndGroups(state: GlobalState, posts: Post[]): Set<string> {
let usersByUsername: Record<string, UserProfile>; // Populate this lazily since it's relatively expensive
let groupsByName: Record<string, Group>;
const usernamesToLoad = new Set<string>();
const usernamesAndGroupsToLoad = new Set<string>();
function findNeededUsernames(text?: string) {
function findNeededUsernamesAndGroups(text?: string) {
if (!text || !text.includes('@')) {
return;
}
@ -1164,6 +1177,10 @@ export function getNeededAtMentionedUsernames(state: GlobalState, posts: Post[])
usersByUsername = getUsersByUsername(state);
}
if (!groupsByName) {
groupsByName = getAllGroupsByName(state);
}
const pattern = /\B@(([a-z0-9_.-]*[a-z0-9_])[.-]*)/gi;
let match;
@ -1179,25 +1196,30 @@ export function getNeededAtMentionedUsernames(state: GlobalState, posts: Post[])
continue;
}
if (groupsByName[match[1]] || groupsByName[match[2]]) {
// We have the group, go to the next match
continue;
}
// If there's no trailing punctuation, this will only add 1 item to the set
usernamesToLoad.add(match[1]);
usernamesToLoad.add(match[2]);
usernamesAndGroupsToLoad.add(match[1]);
usernamesAndGroupsToLoad.add(match[2]);
}
}
for (const post of posts) {
// These correspond to the fields searched by getMentionsEnabledFields on the server
findNeededUsernames(post.message);
findNeededUsernamesAndGroups(post.message);
if (post.props?.attachments) {
for (const attachment of post.props.attachments) {
findNeededUsernames(attachment.pretext);
findNeededUsernames(attachment.text);
findNeededUsernamesAndGroups(attachment.pretext);
findNeededUsernamesAndGroups(attachment.text);
}
}
}
return usernamesToLoad;
return usernamesAndGroupsToLoad;
}
export type ExtendedPost = Post & { system_post_ids?: string[] };

View File

@ -19,7 +19,7 @@ import {SearchParameter} from '@mattermost/types/search';
import {getChannelAndMyMember, getChannelMembers} from './channels';
import {forceLogoutIfNecessary} from './helpers';
import {logError} from './errors';
import {getProfilesAndStatusesForPosts, receivedPosts} from './posts';
import {getMentionsAndStatusesForPosts, receivedPosts} from './posts';
import {receivedFiles} from './files';
const WEBAPP_SEARCH_PER_PAGE = 20;
@ -82,7 +82,7 @@ export function searchPostsWithParams(teamId: string, params: SearchParameter):
try {
posts = await Client4.searchPostsWithParams(teamId, params);
const profilesAndStatuses = getProfilesAndStatusesForPosts(posts.posts, dispatch, getState);
const profilesAndStatuses = getMentionsAndStatusesForPosts(posts.posts, dispatch, getState);
const missingChannels = dispatch(getMissingChannelsFromPosts(posts.posts));
const arr: [Promise<any>, Promise<any>] = [profilesAndStatuses, missingChannels];
await Promise.all(arr);
@ -213,7 +213,7 @@ export function getFlaggedPosts(): ActionFunc {
try {
posts = await Client4.getFlaggedPosts(userId);
await Promise.all([getProfilesAndStatusesForPosts(posts.posts, dispatch, getState) as any, dispatch(getMissingChannelsFromPosts(posts.posts)) as any]);
await Promise.all([getMentionsAndStatusesForPosts(posts.posts, dispatch, getState) as any, dispatch(getMissingChannelsFromPosts(posts.posts)) as any]);
} catch (error) {
forceLogoutIfNecessary(error, dispatch, getState);
dispatch({type: SearchTypes.SEARCH_FLAGGED_POSTS_FAILURE, error});
@ -244,7 +244,7 @@ export function getPinnedPosts(channelId: string): ActionFunc {
try {
result = await Client4.getPinnedPosts(channelId);
const profilesAndStatuses = getProfilesAndStatusesForPosts(result.posts, dispatch, getState);
const profilesAndStatuses = getMentionsAndStatusesForPosts(result.posts, dispatch, getState);
const missingChannels = dispatch(getMissingChannelsFromPosts(result.posts));
const arr: [Promise<any>, Promise<any>] = [profilesAndStatuses, missingChannels];
await Promise.all(arr);

View File

@ -296,7 +296,7 @@ export function getProfilesByIds(userIds: string[], options?: any): ActionFunc {
};
}
export function getProfilesByUsernames(usernames: string[]): ActionFunc {
export function getProfilesByUsernames(usernames: string[]): ActionFunc<UserProfile[]> {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
let profiles;

View File

@ -33,6 +33,23 @@ export function getAllGroups(state: GlobalState) {
return state.entities.groups.groups;
}
export const getAllGroupsByName: (state: GlobalState) => Record<string, Group> = createSelector(
'getAllGroupsByName',
getAllGroups,
(groups) => {
const groupsByName: Record<string, Group> = {};
for (const id in groups) {
if (groups.hasOwnProperty(id)) {
const group = groups[id];
groupsByName[group.name] = group;
}
}
return groupsByName;
},
);
export function getMyGroupIds(state: GlobalState) {
return state.entities.groups.myGroups;
}

View File

@ -3490,8 +3490,9 @@ export default class Client4 {
);
};
getGroups = (filterAllowReference = false, page = 0, perPage = 10, includeMemberCount = false, hasFilterMember = false) => {
getGroups = (q = '', filterAllowReference = false, page = 0, perPage = 10, includeMemberCount = false, hasFilterMember = false) => {
const qs: any = {
q,
filter_allow_reference: filterAllowReference,
page,
per_page: perPage,