From b1e745894b720dc27df600a5bbd00cfc424f03ed Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Fri, 8 Dec 2023 10:35:15 -0500 Subject: [PATCH] MM-55468 Ensure custom status emojis exist (#25501) * MM-55468 Ensure custom status emojis exist * Fix plugin API unit test * Print underlying error as detailed error message * Convert CustomStatusModal tests to React Testing Library and improve a11y * Don't suggest custom statuses with non-existent emojis * Silence test error by providing fake translation strings --- server/channels/app/emoji.go | 14 ++ server/channels/app/plugin_api_test.go | 2 +- server/channels/app/status.go | 7 + server/channels/app/status_test.go | 93 ++++++++++- server/i18n/en.json | 4 + server/public/model/emoji.go | 4 +- webapp/channels/src/actions/emoji_actions.js | 31 ++++ .../custom_status_modal.test.tsx.snap | 77 --------- .../custom_status_modal.test.tsx | 158 +++++++++++++++--- .../custom_status/custom_status_modal.tsx | 7 +- .../src/components/emoji/render_emoji.tsx | 2 +- .../src/selectors/views/custom_status.ts | 20 ++- .../src/tests/react_testing_utils.tsx | 3 + webapp/channels/src/utils/test_helper.ts | 13 +- 14 files changed, 320 insertions(+), 115 deletions(-) delete mode 100644 webapp/channels/src/components/custom_status/__snapshots__/custom_status_modal.test.tsx.snap diff --git a/server/channels/app/emoji.go b/server/channels/app/emoji.go index abc0ddade1..10123be66f 100644 --- a/server/channels/app/emoji.go +++ b/server/channels/app/emoji.go @@ -366,3 +366,17 @@ func (a *App) deleteReactionsForEmoji(rctx request.CTX, emojiName string) { rctx.Logger().Warn("Unable to delete reactions when deleting emoji", mlog.String("emoji_name", emojiName), mlog.Err(err)) } } + +func (a *App) confirmEmojiExists(c request.CTX, emojiName string) *model.AppError { + if model.IsSystemEmojiName(emojiName) { + return nil + } + + err := model.IsValidEmojiName(emojiName) + if err != nil { + return err + } + + _, err = a.GetEmojiByName(c, emojiName) + return err +} diff --git a/server/channels/app/plugin_api_test.go b/server/channels/app/plugin_api_test.go index f19e31e7cc..82ecd72419 100644 --- a/server/channels/app/plugin_api_test.go +++ b/server/channels/app/plugin_api_test.go @@ -513,7 +513,7 @@ func TestPluginAPIUserCustomStatus(t *testing.T) { defer th.App.PermanentDeleteUser(th.Context, user1) custom := &model.CustomStatus{ - Emoji: ":tada:", + Emoji: "tada", Text: "honk", } diff --git a/server/channels/app/status.go b/server/channels/app/status.go index f5129f3601..180b22acf7 100644 --- a/server/channels/app/status.go +++ b/server/channels/app/status.go @@ -77,6 +77,13 @@ func (a *App) SetCustomStatus(c request.CTX, userID string, cs *model.CustomStat return model.NewAppError("SetCustomStatus", "api.custom_status.set_custom_statuses.update.app_error", nil, "", http.StatusBadRequest) } + // Ensure the emoji exists before saving the custom status even if it's deleted afterwards + if cs.Emoji != "" { + if err := a.confirmEmojiExists(c, cs.Emoji); err != nil { + return model.NewAppError("SetCustomStatus", "api.custom_status.set_custom_statuses.emoji_not_found", nil, err.Error(), http.StatusBadRequest) + } + } + user, err := a.GetUser(userID) if err != nil { return err diff --git a/server/channels/app/status_test.go b/server/channels/app/status_test.go index 4a0f054979..1dcc6f5596 100644 --- a/server/channels/app/status_test.go +++ b/server/channels/app/status_test.go @@ -6,6 +6,7 @@ package app import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -22,7 +23,7 @@ func TestCustomStatus(t *testing.T) { user := th.BasicUser cs := &model.CustomStatus{ - Emoji: ":smile:", + Emoji: "smile", Text: "honk!", } @@ -91,7 +92,7 @@ func TestCustomStatusErrors(t *testing.T) { require.NoError(t, err) cs := &model.CustomStatus{ - Emoji: ":smile:", + Emoji: "smile", Text: "honk!", } @@ -108,3 +109,91 @@ func TestCustomStatusErrors(t *testing.T) { }) } } + +func TestSetCustomStatus(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableCustomEmoji = true + }) + + emoji := th.CreateEmoji() + + for _, testCase := range []struct { + Name string + Input *model.CustomStatus + ExpectsError bool + }{ + { + Name: "should be able to set custom status with text and emoji", + Input: &model.CustomStatus{ + Emoji: "smile", + Text: "honk!", + }, + ExpectsError: false, + }, + { + Name: "should be able to set custom status with only text", + Input: &model.CustomStatus{ + Text: "honk!", + }, + ExpectsError: false, + }, + { + Name: "should be able to set custom status with just a system emoji", + Input: &model.CustomStatus{ + Emoji: "smile", + }, + ExpectsError: false, + }, + { + Name: "should be able to set custom status with just a custom emoji", + Input: &model.CustomStatus{ + Emoji: emoji.Name, + }, + ExpectsError: false, + }, + { + Name: "should not be able to set custom status without text or emoji", + Input: &model.CustomStatus{}, + ExpectsError: true, + }, + { + Name: "should not be able to set custom status with a non-existent emoji name", + Input: &model.CustomStatus{ + Emoji: "somethingthatdoesntexist", + }, + ExpectsError: true, + }, + { + Name: "should not be able to set custom status with an invalid emoji name", + Input: &model.CustomStatus{ + Emoji: "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz", + Text: "honk!", + }, + ExpectsError: true, + }, + } { + t.Run(testCase.Name, func(t *testing.T) { + err := th.App.SetCustomStatus(th.Context, th.BasicUser.Id, testCase.Input) + defer th.App.RemoveCustomStatus(th.Context, th.BasicUser.Id) + + if testCase.ExpectsError { + require.NotNil(t, err) + } else { + require.Nil(t, err) + } + + customStatus, err := th.App.GetCustomStatus(th.BasicUser.Id) + + require.Nil(t, err) + + if testCase.ExpectsError { + assert.NotEqual(t, testCase.Input, customStatus) + } else { + assert.Equal(t, testCase.Input, customStatus) + } + }) + } +} diff --git a/server/i18n/en.json b/server/i18n/en.json index a20573e589..5b46c55c8c 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -1717,6 +1717,10 @@ "id": "api.custom_status.recent_custom_statuses.delete.app_error", "translation": "Failed to delete the recent status. Please try adding the status first or contact your system administrator for details." }, + { + "id": "api.custom_status.set_custom_statuses.emoji_not_found", + "translation": "Failed to update the custom status. An emoji with the given name does not exist." + }, { "id": "api.custom_status.set_custom_statuses.update.app_error", "translation": "Failed to update the custom status. Please add either emoji or custom text status or both." diff --git a/server/public/model/emoji.go b/server/public/model/emoji.go index 9b12dbdafe..9ec2cd1e2f 100644 --- a/server/public/model/emoji.go +++ b/server/public/model/emoji.go @@ -36,7 +36,7 @@ func (emoji *Emoji) Auditable() map[string]interface{} { } } -func inSystemEmoji(emojiName string) bool { +func IsSystemEmojiName(emojiName string) bool { _, ok := SystemEmojis[emojiName] return ok } @@ -92,7 +92,7 @@ func IsValidEmojiName(name string) *AppError { if name == "" || len(name) > EmojiNameMaxLength || !IsValidAlphaNumHyphenUnderscorePlus(name) { return NewAppError("Emoji.IsValid", "model.emoji.name.app_error", nil, "", http.StatusBadRequest) } - if inSystemEmoji(name) { + if IsSystemEmojiName(name) { return NewAppError("Emoji.IsValid", "model.emoji.system_emoji_name.app_error", nil, "", http.StatusBadRequest) } diff --git a/webapp/channels/src/actions/emoji_actions.js b/webapp/channels/src/actions/emoji_actions.js index 94e56246cd..553cd74281 100644 --- a/webapp/channels/src/actions/emoji_actions.js +++ b/webapp/channels/src/actions/emoji_actions.js @@ -3,7 +3,9 @@ import * as EmojiActions from 'mattermost-redux/actions/emojis'; import {savePreferences} from 'mattermost-redux/actions/preferences'; +import {Preferences as ReduxPreferences} from 'mattermost-redux/constants'; import {getCustomEmojisByName as selectCustomEmojisByName, getCustomEmojisEnabled} from 'mattermost-redux/selectors/entities/emojis'; +import {get} from 'mattermost-redux/selectors/entities/preferences'; import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users'; import {getEmojiMap, getRecentEmojisData, getRecentEmojisNames, isCustomEmojiEnabled} from 'selectors/emojis'; @@ -132,6 +134,35 @@ export function loadCustomEmojisForCustomStatusesByUserIds(userIds) { }; } +export function loadCustomEmojisForRecentCustomStatuses() { + return (dispatch, getState) => { + const state = getState(); + const customEmojiEnabled = isCustomEmojiEnabled(state); + const customStatusEnabled = isCustomStatusEnabled(state); + if (!customEmojiEnabled || !customStatusEnabled) { + return {data: false}; + } + + const recentCustomStatusesValue = get(state, ReduxPreferences.CATEGORY_CUSTOM_STATUS, ReduxPreferences.NAME_RECENT_CUSTOM_STATUSES); + if (!recentCustomStatusesValue) { + return {data: false}; + } + + const recentCustomStatuses = JSON.parse(recentCustomStatusesValue); + const emojisToLoad = new Set(); + + for (const customStatus of recentCustomStatuses) { + if (!customStatus || !customStatus.emoji) { + continue; + } + + emojisToLoad.add(customStatus.emoji); + } + + return dispatch(loadCustomEmojisIfNeeded(Array.from(emojisToLoad))); + }; +} + export function loadCustomEmojisIfNeeded(emojis) { return (dispatch, getState) => { if (!emojis || emojis.length === 0) { diff --git a/webapp/channels/src/components/custom_status/__snapshots__/custom_status_modal.test.tsx.snap b/webapp/channels/src/components/custom_status/__snapshots__/custom_status_modal.test.tsx.snap deleted file mode 100644 index 89594bb071..0000000000 --- a/webapp/channels/src/components/custom_status/__snapshots__/custom_status_modal.test.tsx.snap +++ /dev/null @@ -1,77 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`components/custom_status/custom_status_modal should match snapshot 1`] = ` - - - -`; - -exports[`components/custom_status/custom_status_modal should match snapshot when user has custom status set 1`] = ` - - - -`; diff --git a/webapp/channels/src/components/custom_status/custom_status_modal.test.tsx b/webapp/channels/src/components/custom_status/custom_status_modal.test.tsx index f3a0f47a52..80cbe72f5d 100644 --- a/webapp/channels/src/components/custom_status/custom_status_modal.test.tsx +++ b/webapp/channels/src/components/custom_status/custom_status_modal.test.tsx @@ -1,46 +1,156 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {shallow} from 'enzyme'; import React from 'react'; -import {Provider} from 'react-redux'; +import type {AutoSizerProps} from 'react-virtualized-auto-sizer'; -import * as StatusSelectors from 'selectors/views/custom_status'; +import type {DeepPartial} from '@mattermost/types/utilities'; -import mockStore from 'tests/test_store'; +import {Preferences} from 'mattermost-redux/constants'; + +import mergeObjects from 'packages/mattermost-redux/test/merge_objects'; +import {act, renderWithContext, screen, userEvent} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +import type {GlobalState} from 'types/store'; import CustomStatusModal from './custom_status_modal'; -jest.mock('selectors/views/custom_status'); +jest.mock('react-virtualized-auto-sizer', () => (props: AutoSizerProps) => props.children({height: 100, width: 100})); +jest.mock('images/img_trans.gif', () => 'img_trans.gif'); -describe('components/custom_status/custom_status_modal', () => { - const store = mockStore({}); +describe('CustomStatusModal', () => { const baseProps = { onExited: jest.fn(), }; - it('should match snapshot', () => { - const wrapper = shallow( - - - , + const initialState: DeepPartial = { + entities: { + general: { + config: { + EnableCustomEmoji: 'true', + EnableCustomUserStatuses: 'true', + }, + }, + }, + }; + + // The emoji picker renders emoji categories without passing a defaultMessage, and we don't pass translation strings + // into the provider by default, so we need to pass something for this string to silence errors from FormatJS. + const renderOptions = { + intlMessages: { + 'emoji_picker.smileys-emotion': 'Smileys & Emotions', + }, + }; + + test('should render suggested statuses until the user starts typing', () => { + renderWithContext( + , + initialState, + renderOptions, ); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText('SUGGESTIONS')).toBeInTheDocument(); + expect(screen.getByText('Out for lunch')).toBeInTheDocument(); + expect(screen.getByLabelText(':hamburger:')).toBeInTheDocument(); + + userEvent.type(screen.getByPlaceholderText('Set a status'), 'Test status, please ignore'); + + expect(screen.queryByText('SUGGESTIONS')).not.toBeInTheDocument(); + expect(screen.queryByText('Out for lunch')).not.toBeInTheDocument(); + expect(screen.queryByLabelText(':hamburger:')).not.toBeInTheDocument(); }); - it('should match snapshot when user has custom status set', () => { - const customStatus = { - emoji: 'speech_balloon', - text: 'speaking', - }; - (StatusSelectors.makeGetCustomStatus as jest.Mock).mockReturnValue(() => customStatus); - const wrapper = shallow( - - - , + test('should render suggested statuses until the user selects an emoji', () => { + renderWithContext( + , + initialState, + renderOptions, ); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText('SUGGESTIONS')).toBeInTheDocument(); + expect(screen.getByLabelText(':hamburger:')).toBeInTheDocument(); + expect(screen.getByText('Out for lunch')).toBeInTheDocument(); + + userEvent.click(screen.getByLabelText('select an emoji')); + act(() => userEvent.click(screen.getByLabelText('grinning emoji'))); + + expect(screen.queryByText('SUGGESTIONS')).not.toBeInTheDocument(); + expect(screen.queryByText('Out for lunch')).not.toBeInTheDocument(); + expect(screen.queryByLabelText(':hamburger:')).not.toBeInTheDocument(); + }); + + test('should render recently used statuses as suggestions', () => { + const testState = mergeObjects(initialState, { + entities: { + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.CATEGORY_CUSTOM_STATUS, + name: Preferences.NAME_RECENT_CUSTOM_STATUSES, + value: JSON.stringify([ + TestHelper.getCustomStatusMock({emoji: 'taco', text: 'Eating a taco'}), + ]), + }, + ]), + }, + }, + }); + + renderWithContext( + , + testState, + renderOptions, + ); + + expect(screen.getByText('SUGGESTIONS')).toBeInTheDocument(); + expect(screen.getByText('Eating a taco')).toBeInTheDocument(); + expect(screen.getByLabelText(':taco:')).toBeInTheDocument(); + }); + + test('should render recently used statuses with custom emojis which exist', () => { + const existentEmoji = TestHelper.getCustomEmojiMock({name: 'existent'}); + + const testState = mergeObjects(initialState, { + entities: { + emojis: { + customEmoji: { + [existentEmoji.id]: existentEmoji, + }, + }, + preferences: { + myPreferences: TestHelper.getPreferencesMock([ + { + category: Preferences.CATEGORY_CUSTOM_STATUS, + name: Preferences.NAME_RECENT_CUSTOM_STATUSES, + value: JSON.stringify([ + TestHelper.getCustomStatusMock({emoji: 'existent', text: 'Existing'}), + TestHelper.getCustomStatusMock({emoji: 'nonexistent', text: 'Not existing'}), + ]), + }, + ]), + }, + }, + }); + + renderWithContext( + , + testState, + renderOptions, + ); + + expect(screen.getByText('SUGGESTIONS')).toBeInTheDocument(); + expect(screen.getByText('Existing')).toBeInTheDocument(); + expect(screen.getByLabelText(':existent:')).toBeInTheDocument(); + expect(screen.queryByText('Not existing')).not.toBeInTheDocument(); + expect(screen.queryByLabelText(':nonexistent:')).not.toBeInTheDocument(); }); }); diff --git a/webapp/channels/src/components/custom_status/custom_status_modal.tsx b/webapp/channels/src/components/custom_status/custom_status_modal.tsx index c1bd7537a7..9f568950f9 100644 --- a/webapp/channels/src/components/custom_status/custom_status_modal.tsx +++ b/webapp/channels/src/components/custom_status/custom_status_modal.tsx @@ -19,7 +19,7 @@ import {setCustomStatus, unsetCustomStatus, removeRecentCustomStatus} from 'matt import {Preferences} from 'mattermost-redux/constants'; import {getCurrentTimezone} from 'mattermost-redux/selectors/entities/timezone'; -import {loadCustomEmojisIfNeeded} from 'actions/emoji_actions'; +import {loadCustomEmojisForRecentCustomStatuses} from 'actions/emoji_actions'; import {closeModal} from 'actions/views/modals'; import {makeGetCustomStatus, getRecentCustomStatuses, showStatusDropdownPulsatingDot, isCustomStatusExpired} from 'selectors/views/custom_status'; @@ -153,9 +153,7 @@ const CustomStatusModal: React.FC = (props: Props) => { }; const loadCustomEmojisForRecentStatuses = () => { - const emojisToLoad = new Set(); - recentCustomStatuses.forEach((customStatus: UserCustomStatus) => emojisToLoad.add(customStatus.emoji)); - dispatch(loadCustomEmojisIfNeeded(Array.from(emojisToLoad))); + dispatch(loadCustomEmojisForRecentCustomStatuses()); }; const handleStatusExpired = () => { @@ -427,6 +425,7 @@ const CustomStatusModal: React.FC = (props: Props) => { type='button' onClick={toggleEmojiPicker} ref={emojiButtonRef} + aria-label={formatMessage({id: 'emoji_picker.emojiPicker.button.ariaLabel', defaultMessage: 'select an emoji'})} className={classNames('emoji-picker__container', 'StatusModal__emoji-button', { 'StatusModal__emoji-button--active': showEmojiPicker, })} diff --git a/webapp/channels/src/components/emoji/render_emoji.tsx b/webapp/channels/src/components/emoji/render_emoji.tsx index 065f3b8d49..c9558c2d09 100644 --- a/webapp/channels/src/components/emoji/render_emoji.tsx +++ b/webapp/channels/src/components/emoji/render_emoji.tsx @@ -34,7 +34,7 @@ const RenderEmoji = ({emojiName, emojiStyle, size, onClick}: ComponentProps) => UserCustomStatus[] = createSelector( 'getRecentCustomStatuses', (state: GlobalState) => get(state, Preferences.CATEGORY_CUSTOM_STATUS, Preferences.NAME_RECENT_CUSTOM_STATUSES), - (value) => { - return value ? JSON.parse(value) : []; + getEmojiMap, + (value, emojiMap) => { + if (!value) { + return []; + } + + let recentCustomStatuses: UserCustomStatus[] = JSON.parse(value); + recentCustomStatuses = recentCustomStatuses.filter((customStatus) => emojiMap.has(customStatus.emoji)); + + return recentCustomStatuses; }, ); diff --git a/webapp/channels/src/tests/react_testing_utils.tsx b/webapp/channels/src/tests/react_testing_utils.tsx index 47915988b7..e986179800 100644 --- a/webapp/channels/src/tests/react_testing_utils.tsx +++ b/webapp/channels/src/tests/react_testing_utils.tsx @@ -23,6 +23,7 @@ export * from '@testing-library/react'; export {userEvent}; export type FullContextOptions = { + intlMessages?: Record; locale?: string; useMockedStore?: boolean; } @@ -33,6 +34,7 @@ export const renderWithContext = ( partialOptions?: FullContextOptions, ) => { const options = { + intlMessages: partialOptions?.intlMessages, locale: partialOptions?.locale ?? 'en', useMockedStore: partialOptions?.useMockedStore ?? false, }; @@ -55,6 +57,7 @@ export const renderWithContext = ( {props.children} diff --git a/webapp/channels/src/utils/test_helper.ts b/webapp/channels/src/utils/test_helper.ts index b00820530f..ccca026651 100644 --- a/webapp/channels/src/utils/test_helper.ts +++ b/webapp/channels/src/utils/test_helper.ts @@ -17,7 +17,8 @@ import type {Reaction} from '@mattermost/types/reactions'; import type {Role} from '@mattermost/types/roles'; import type {Session} from '@mattermost/types/sessions'; import type {Team, TeamMembership} from '@mattermost/types/teams'; -import type {UserProfile, UserAccessToken} from '@mattermost/types/users'; +import {CustomStatusDuration} from '@mattermost/types/users'; +import type {UserProfile, UserAccessToken, UserCustomStatus} from '@mattermost/types/users'; import {CategoryTypes} from 'mattermost-redux/constants/channel_categories'; import {getPreferenceKey} from 'mattermost-redux/utils/preference_utils'; @@ -81,6 +82,16 @@ export class TestHelper { return Object.assign({}, defaultUser, override); } + public static getCustomStatusMock(override?: Partial): UserCustomStatus { + const defaultCustomStatus: UserCustomStatus = { + emoji: 'neutral_face', + text: 'text', + duration: CustomStatusDuration.DONT_CLEAR, + }; + + return Object.assign({}, defaultCustomStatus, override); + } + public static getUserAccessTokenMock(override?: Partial): UserAccessToken { const defaultUserAccessToken: UserAccessToken = { id: 'token_id',