From bfd6f5aa0153abc16698c582f8061594ed4015ee Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 30 Oct 2023 16:03:43 -0400 Subject: [PATCH] Use bulk emoji API for loading recent emojis (#25043) --- webapp/channels/src/actions/emoji_actions.js | 22 +- .../src/actions/emoji_actions.mock.test.js | 75 ----- .../actions/emoji_actions_load_recent.test.ts | 258 ++++++++++++++++++ .../mattermost-redux/src/actions/emojis.ts | 32 ++- .../src/utils/emoji_utils.test.ts | 64 +---- .../mattermost-redux/src/utils/emoji_utils.ts | 23 +- 6 files changed, 302 insertions(+), 172 deletions(-) delete mode 100644 webapp/channels/src/actions/emoji_actions.mock.test.js create mode 100644 webapp/channels/src/actions/emoji_actions_load_recent.test.ts diff --git a/webapp/channels/src/actions/emoji_actions.js b/webapp/channels/src/actions/emoji_actions.js index 0e38996d68415..bc3299a88f388 100644 --- a/webapp/channels/src/actions/emoji_actions.js +++ b/webapp/channels/src/actions/emoji_actions.js @@ -3,8 +3,7 @@ import * as EmojiActions from 'mattermost-redux/actions/emojis'; import {savePreferences} from 'mattermost-redux/actions/preferences'; -import {getCustomEmojisByName} from 'mattermost-redux/selectors/entities/emojis'; -import {getConfig} from 'mattermost-redux/selectors/entities/general'; +import {getCustomEmojisByName as selectCustomEmojisByName, getCustomEmojisEnabled} from 'mattermost-redux/selectors/entities/emojis'; import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users'; import {getEmojiMap, getRecentEmojisData, getRecentEmojisNames, isCustomEmojiEnabled} from 'selectors/emojis'; @@ -15,23 +14,16 @@ import Constants, {ActionTypes, Preferences} from 'utils/constants'; import {EmojiIndicesByAlias} from 'utils/emoji'; export function loadRecentlyUsedCustomEmojis() { - return async (dispatch, getState) => { + return (dispatch, getState) => { const state = getState(); - const config = getConfig(state); - if (config.EnableCustomEmoji !== 'true') { + if (!getCustomEmojisEnabled(state)) { return {data: true}; } - const recentEmojis = getRecentEmojisNames(state); - const emojiMap = getEmojiMap(state); - const missingEmojis = recentEmojis.filter((name) => !emojiMap.has(name)); - - missingEmojis.forEach((name) => { - dispatch(EmojiActions.getCustomEmojiByName(name)); - }); + const recentEmojiNames = getRecentEmojisNames(state); - return {data: true}; + return dispatch(EmojiActions.getCustomEmojisByName(recentEmojiNames)); }; } @@ -153,7 +145,7 @@ export function loadCustomEmojisIfNeeded(emojis) { } const systemEmojis = EmojiIndicesByAlias; - const customEmojisByName = getCustomEmojisByName(state); + const customEmojisByName = selectCustomEmojisByName(state); const nonExistentCustomEmoji = state.entities.emojis.nonExistentEmoji; const emojisToLoad = []; @@ -180,7 +172,7 @@ export function loadCustomEmojisIfNeeded(emojis) { emojisToLoad.push(emoji); }); - return dispatch(EmojiActions.getCustomEmojisByName(emojisToLoad)); + return dispatch(EmojiActions.selectCustomEmojisByName(emojisToLoad)); }; } diff --git a/webapp/channels/src/actions/emoji_actions.mock.test.js b/webapp/channels/src/actions/emoji_actions.mock.test.js deleted file mode 100644 index 8563ddecd5859..0000000000000 --- a/webapp/channels/src/actions/emoji_actions.mock.test.js +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -import * as Actions from 'actions/emoji_actions'; -import {getEmojiMap, getRecentEmojisNames} from 'selectors/emojis'; - -import mockStore from 'tests/test_store'; - -const initialState = { - entities: { - general: { - config: {EnableCustomEmoji: 'true'}, - }, - }, -}; - -jest.mock('selectors/emojis', () => ({ - getEmojiMap: jest.fn(), - getRecentEmojisNames: jest.fn(), -})); - -jest.mock('mattermost-redux/actions/emojis', () => ({ - getCustomEmojiByName: (...args) => ({type: 'MOCK_GET_CUSTOM_EMOJI_BY_NAME', args}), -})); - -describe('loadRecentlyUsedCustomEmojis', () => { - let testStore; - beforeEach(async () => { - testStore = await mockStore(initialState); - }); - - test('Get only emojis missing on the map', async () => { - getEmojiMap.mockImplementation(() => { - return new Map([['emoji1', {}], ['emoji3', {}], ['emoji4', {}]]); - }); - getRecentEmojisNames.mockImplementation(() => { - return ['emoji1', 'emoji2', 'emoji3', 'emoji5']; - }); - - const expectedActions = [ - {type: 'MOCK_GET_CUSTOM_EMOJI_BY_NAME', args: ['emoji2']}, - {type: 'MOCK_GET_CUSTOM_EMOJI_BY_NAME', args: ['emoji5']}, - ]; - - testStore.dispatch(Actions.loadRecentlyUsedCustomEmojis()); - expect(testStore.getActions()).toEqual(expectedActions); - }); - - test('Does not get any emojis if none is missing on the map', async () => { - getEmojiMap.mockImplementation(() => { - return new Map([['emoji1', {}], ['emoji3', {}], ['emoji4', {}]]); - }); - getRecentEmojisNames.mockImplementation(() => { - return ['emoji1', 'emoji3']; - }); - const expectedActions = []; - - testStore.dispatch(Actions.loadRecentlyUsedCustomEmojis()); - expect(testStore.getActions()).toEqual(expectedActions); - }); - - test('Does not get any emojis if they are not enabled', async () => { - testStore = await mockStore({entities: {general: {config: {EnableCustomEmoji: 'false'}}}}); - getEmojiMap.mockImplementation(() => { - return new Map([['emoji1', {}], ['emoji3', {}], ['emoji4', {}]]); - }); - getRecentEmojisNames.mockImplementation(() => { - return ['emoji1', 'emoji2', 'emoji3', 'emoji5']; - }); - const expectedActions = []; - - testStore.dispatch(Actions.loadRecentlyUsedCustomEmojis()); - expect(testStore.getActions()).toEqual(expectedActions); - }); -}); diff --git a/webapp/channels/src/actions/emoji_actions_load_recent.test.ts b/webapp/channels/src/actions/emoji_actions_load_recent.test.ts new file mode 100644 index 0000000000000..869c7f901ca09 --- /dev/null +++ b/webapp/channels/src/actions/emoji_actions_load_recent.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import nock from 'nock'; +import {BATCH} from 'redux-batched-actions'; + +import type {DeepPartial} from '@mattermost/types/utilities'; + +import {EmojiTypes} from 'mattermost-redux/action_types'; +import {setSystemEmojis} from 'mattermost-redux/actions/emojis'; +import {Client4} from 'mattermost-redux/client'; + +import * as Actions from 'actions/emoji_actions'; + +import mergeObjects from 'packages/mattermost-redux/test/merge_objects'; +import mockStore from 'tests/test_store'; +import {Preferences} from 'utils/constants'; +import {EmojiIndicesByAlias} from 'utils/emoji'; +import {TestHelper} from 'utils/test_helper'; + +import type {GlobalState} from 'types/store'; + +Client4.setUrl('http://localhost:8065'); + +describe('loadRecentlyUsedCustomEmojis', () => { + const currentUserId = 'currentUserId'; + + const emoji1 = TestHelper.getCustomEmojiMock({name: 'emoji1', id: 'emojiId1'}); + const emoji2 = TestHelper.getCustomEmojiMock({name: 'emoji2', id: 'emojiId2'}); + const loadedEmoji = TestHelper.getCustomEmojiMock({name: 'loaded', id: 'loadedId'}); + + const initialState: DeepPartial = { + entities: { + emojis: { + customEmoji: { + [loadedEmoji.id]: loadedEmoji, + }, + nonExistentEmoji: new Set(), + }, + general: { + config: { + EnableCustomEmoji: 'true', + }, + }, + users: { + currentUserId, + }, + }, + }; + + setSystemEmojis(new Set(EmojiIndicesByAlias.keys())); + + test('should get requested emojis', async () => { + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: emoji1.name, usageCount: 3}, + {name: emoji2.name, usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + const store = mockStore(state); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1', 'emoji2']). + reply(200, [emoji1, emoji2]); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([ + { + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data: [emoji1, emoji2], + }, + ]); + }); + + test('should not request emojis which are already loaded', async () => { + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: emoji1.name, usageCount: 3}, + {name: loadedEmoji.name, usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + const store = mockStore(state); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1']). + reply(200, [emoji1]); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([ + { + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data: [emoji1], + }, + ]); + }); + + test('should not make a request if all recentl emojis are loaded', async () => { + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: loadedEmoji.name, usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + const store = mockStore(state); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([]); + }); + + test('should properly store any names of nonexistent emojis', async () => { + const fakeEmojiName = 'fake-emoji-name'; + + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: emoji1.name, usageCount: 3}, + {name: fakeEmojiName, usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + const store = mockStore(state); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1', fakeEmojiName]). + reply(200, [emoji1]); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([ + { + type: BATCH, + meta: { + batch: true, + }, + payload: [ + { + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data: [emoji1], + }, + { + type: EmojiTypes.CUSTOM_EMOJI_DOES_NOT_EXIST, + data: fakeEmojiName, + }, + ], + }, + ]); + }); + + test('should not request an emoji that we know does not exist', async () => { + const fakeEmojiName = 'fake-emoji-name'; + + // Add nonExistentEmoji separately because mergeObjects only works with plain objects + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: emoji1.name, usageCount: 3}, + {name: fakeEmojiName, usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + state.entities.emojis.nonExistentEmoji = new Set([fakeEmojiName]); + const store = mockStore(state); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1']). + reply(200, [emoji1]); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([ + { + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data: [emoji1], + }, + ]); + }); + + test('should not request a system emoji', async () => { + const state = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.RECENT_EMOJIS, + name: currentUserId, + value: JSON.stringify([ + {name: emoji1.name, usageCount: 3}, + {name: 'taco', usageCount: 5}, + ]), + }, + ]), + }, + }, + }); + const store = mockStore(state); + + nock(Client4.getBaseRoute()). + post('/emoji/names', ['emoji1']). + reply(200, [emoji1]); + + await store.dispatch(Actions.loadRecentlyUsedCustomEmojis()); + + expect(store.getActions()).toEqual([ + { + type: EmojiTypes.RECEIVED_CUSTOM_EMOJIS, + data: [emoji1], + }, + ]); + }); +}); diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts index 140eb29adcd8d..32a1c8b323ef8 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/emojis.ts @@ -5,12 +5,13 @@ import type {AnyAction} from 'redux'; import {batchActions} from 'redux-batched-actions'; import type {CustomEmoji} from '@mattermost/types/emojis'; +import type {GlobalState} from '@mattermost/types/store'; import {EmojiTypes} from 'mattermost-redux/action_types'; import {Client4} from 'mattermost-redux/client'; import {getCustomEmojisByName as selectCustomEmojisByName} from 'mattermost-redux/selectors/entities/emojis'; import type {GetStateFunc, DispatchFunc, ActionFunc} from 'mattermost-redux/types/actions'; -import {parseNeededCustomEmojisFromText} from 'mattermost-redux/utils/emoji_utils'; +import {parseEmojiNamesFromText} from 'mattermost-redux/utils/emoji_utils'; import {logError} from './errors'; import {bindClientFunc, forceLogoutIfNecessary} from './helpers'; @@ -73,7 +74,9 @@ export function getCustomEmojiByName(name: string): ActionFunc { export function getCustomEmojisByName(names: string[]): ActionFunc { return async (dispatch: DispatchFunc, getState: GetStateFunc) => { - if (!names || names.length === 0) { + const neededNames = filterNeededCustomEmojis(getState(), names); + + if (neededNames.length === 0) { return {data: true}; } @@ -82,7 +85,7 @@ export function getCustomEmojisByName(names: string[]): ActionFunc { const batches = []; for (let i = 0; i < names.length; i += batchSize) { - batches.push(names.slice(i, i + batchSize)); + batches.push(neededNames.slice(i, i + batchSize)); } let results; @@ -102,10 +105,10 @@ export function getCustomEmojisByName(names: string[]): ActionFunc { data, }]; - if (data.length !== names.length) { + if (data.length !== neededNames.length) { const foundNames = new Set(data.map((emoji) => emoji.name)); - for (const name of names) { + for (const name of neededNames) { if (foundNames.has(name)) { continue; } @@ -121,19 +124,22 @@ export function getCustomEmojisByName(names: string[]): ActionFunc { }; } +function filterNeededCustomEmojis(state: GlobalState, names: string[]) { + const nonExistentEmoji = state.entities.emojis.nonExistentEmoji; + const customEmojisByName = selectCustomEmojisByName(state); + + return names.filter((name) => { + return !systemEmojis.has(name) && !nonExistentEmoji.has(name) && !customEmojisByName.has(name); + }); +} + export function getCustomEmojisInText(text: string): ActionFunc { - return async (dispatch: DispatchFunc, getState: GetStateFunc) => { + return (dispatch: DispatchFunc) => { if (!text) { return {data: true}; } - const state = getState(); - const nonExistentEmoji = state.entities.emojis.nonExistentEmoji; - const customEmojisByName = selectCustomEmojisByName(state); - - const emojisToLoad = parseNeededCustomEmojisFromText(text, systemEmojis, customEmojisByName, nonExistentEmoji); - - return getCustomEmojisByName(Array.from(emojisToLoad))(dispatch, getState); + return dispatch(getCustomEmojisByName(parseEmojiNamesFromText(text))); }; } diff --git a/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.test.ts b/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.test.ts index f74378962cae0..e333ac68c223e 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.test.ts @@ -6,75 +6,39 @@ import * as EmojiUtils from 'mattermost-redux/utils/emoji_utils'; import TestHelper from '../../test/test_helper'; describe('EmojiUtils', () => { - describe('parseNeededCustomEmojisFromText', () => { - it('no emojis', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( + describe('parseEmojiNamesFromText', () => { + test('should return empty array for text without emojis', () => { + const actual = EmojiUtils.parseEmojiNamesFromText( 'This has no emojis', - new Set(), - new Map(), - new Set(), ); - const expected = new Set([]); + const expected: string[] = []; expect(actual).toEqual(expected); }); - it('some emojis', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( + test('should return anything that looks like an emoji', () => { + const actual = EmojiUtils.parseEmojiNamesFromText( ':this: :is_all: :emo-jis: :123:', - new Set(), - new Map(), - new Set(), ); - const expected = new Set(['this', 'is_all', 'emo-jis', '123']); + const expected = ['this', 'is_all', 'emo-jis', '123']; expect(actual).toEqual(expected); }); - it('text surrounding emojis', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( + test('should correctly handle text surrounding emojis', () => { + const actual = EmojiUtils.parseEmojiNamesFromText( ':this:/:is_all: (:emo-jis:) surrounding:123:text:456:asdf', - new Set(), - new Map(), - new Set(), ); - const expected = new Set(['this', 'is_all', 'emo-jis', '123', '456']); + const expected = ['this', 'is_all', 'emo-jis', '123', '456']; expect(actual).toEqual(expected); }); - it('system emojis', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( - ':this: :is_all: :emo-jis: :123:', - new Set(['this', '123']), - new Map(), - new Set(), - ); - const expected = new Set(['is_all', 'emo-jis']); - - expect(actual).toEqual(expected); - }); - - it('custom emojis', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( - ':this: :is_all: :emo-jis: :123:', - new Set(), - new Map([['is_all', TestHelper.getCustomEmojiMock({name: 'is_all'})], ['emo-jis', TestHelper.getCustomEmojiMock({name: 'emo-jis'})]]), - new Set(), - ); - const expected = new Set(['this', '123']); - - expect(actual).toEqual(expected); - }); - - it('emojis that we\'ve already tried to load', () => { - const actual = EmojiUtils.parseNeededCustomEmojisFromText( - ':this: :is_all: :emo-jis: :123:', - new Set(), - new Map(), - new Set(['this', 'emo-jis']), + test('should not return duplicates', () => { + const actual = EmojiUtils.parseEmojiNamesFromText( + ':emoji: :emoji: :emoji: :emoji:', ); - const expected = new Set(['is_all', '123']); + const expected = ['emoji']; expect(actual).toEqual(expected); }); diff --git a/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.ts b/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.ts index 7415219959ee3..dbbeabbc7525d 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/utils/emoji_utils.ts @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {Emoji, SystemEmoji, CustomEmoji} from '@mattermost/types/emojis'; +import type {Emoji, SystemEmoji} from '@mattermost/types/emojis'; import {Client4} from 'mattermost-redux/client'; @@ -29,9 +29,9 @@ export function getEmojiImageUrl(emoji: Emoji): string { return Client4.getEmojiRoute(emoji.id) + '/image'; } -export function parseNeededCustomEmojisFromText(text: string, systemEmojis: Set, customEmojisByName: Map, nonExistentEmoji: Set): Set { +export function parseEmojiNamesFromText(text: string): string[] { if (!text.includes(':')) { - return new Set(); + return []; } const pattern = /:([A-Za-z0-9_-]+):/gi; @@ -42,23 +42,8 @@ export function parseNeededCustomEmojisFromText(text: string, systemEmojis: Set< continue; } - if (systemEmojis.has(match[1])) { - // It's a system emoji, go the next match - continue; - } - - if (nonExistentEmoji.has(match[1])) { - // We've previously confirmed this is not a custom emoji - continue; - } - - if (customEmojisByName.has(match[1])) { - // We have the emoji, go to the next match - continue; - } - customEmojis.add(match[1]); } - return customEmojis; + return Array.from(customEmojis); }