diff --git a/webapp/channels/src/components/common/hooks/useEntity.ts b/webapp/channels/src/components/common/hooks/useEntity.ts new file mode 100644 index 0000000000..b6c68e3d2b --- /dev/null +++ b/webapp/channels/src/components/common/hooks/useEntity.ts @@ -0,0 +1,39 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {useEffect} from 'react'; +import {useDispatch, useSelector} from 'react-redux'; +import type {Action} from 'redux'; +import type {ThunkAction} from 'redux-thunk'; + +import type {GlobalState} from 'types/store'; + +export type UseDataOptions = { + name: string; + + fetch: (identifier: Identifier) => Action | ThunkAction; + selector: (state: State, identifier: Identifier) => Entity | undefined; +} + +export function makeUseEntity(options: UseDataOptions) { + function useEntity(identifier: Identifier): Entity | undefined { + const dispatch = useDispatch(); + + const entity = useSelector((state: State) => { + return identifier ? options.selector(state, identifier) : undefined; + }); + + const entityLoaded = Boolean(entity); + useEffect(() => { + if (!entityLoaded && identifier) { + dispatch(options.fetch(identifier)); + } + }, [dispatch, entityLoaded, identifier]); + + return entity; + } + + Object.defineProperty(useEntity, 'name', {value: options.name, writable: false}); + + return useEntity; +} diff --git a/webapp/channels/src/components/common/hooks/usePost.test.ts b/webapp/channels/src/components/common/hooks/usePost.test.ts new file mode 100644 index 0000000000..78b50150cc --- /dev/null +++ b/webapp/channels/src/components/common/hooks/usePost.test.ts @@ -0,0 +1,235 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import nock from 'nock'; +import * as ReactRedux from 'react-redux'; + +import {Client4} from 'mattermost-redux/client'; + +import {renderHookWithContext} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +import {usePost} from './usePost'; + +describe('usePost', () => { + const post1 = TestHelper.getPostMock({id: 'post1'}); + const post2 = TestHelper.getPostMock({id: 'post2'}); + + describe('with fake dispatch', () => { + const dispatchMock = jest.fn(); + + beforeAll(() => { + jest.spyOn(ReactRedux, 'useDispatch').mockImplementation(() => dispatchMock); + }); + + afterAll(() => { + jest.restoreAllMocks(); + }); + + test("should return the post if it's already in the store", () => { + const {result} = renderHookWithContext( + () => usePost('post1'), + { + entities: { + posts: { + posts: { + post1, + }, + }, + }, + }, + ); + + expect(result.current).toBe(post1); + expect(dispatchMock).not.toHaveBeenCalled(); + }); + + test("should fetch the post if it's not in the store", () => { + const {result} = renderHookWithContext( + () => usePost('post1'), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + }); + + test('should only attempt to fetch the post once regardless of how many times the hook is used', () => { + const {result, rerender} = renderHookWithContext( + () => usePost('post1'), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + for (let i = 0; i < 10; i++) { + rerender(); + } + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + }); + + test('should attempt to fetch different posts if the post ID changes', () => { + let postId = 'post1'; + const {result, rerender} = renderHookWithContext( + () => usePost(postId), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + postId = 'post2'; + rerender(); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(2); + }); + + test("should only attempt to fetch each post once when they aren't loaded", () => { + let postId = 'post1'; + const {result, replaceStoreState, rerender} = renderHookWithContext( + () => usePost(postId), + ); + + // Initial state without post1 loaded + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + // Simulate the response to loading post1 + replaceStoreState({ + entities: { + posts: { + posts: { + post1, + }, + }, + }, + }); + + expect(result.current).toBe(post1); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + // Switch to post2 + postId = 'post2'; + + rerender(); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(2); + + // Simulate the response to loading post2 + replaceStoreState({ + entities: { + posts: { + posts: { + post1, + post2, + }, + }, + }, + }); + + expect(result.current).toBe(post2); + expect(dispatchMock).toHaveBeenCalledTimes(2); + + // Switch back to post1 which has already been loaded + postId = 'post1'; + + rerender(); + + expect(result.current).toBe(post1); + expect(dispatchMock).toHaveBeenCalledTimes(2); + }); + + test("shouldn't attempt to load anything when given an empty post ID", () => { + const {result} = renderHookWithContext( + () => usePost(''), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(0); + }); + }); + + describe('with real dispatch', () => { + beforeAll(() => { + Client4.setUrl('http://localhost:8065'); + }); + + test("should only attempt to fetch each post once when they aren't loaded", async () => { + const post1Mock = nock(Client4.getBaseRoute()). + post('/posts/ids', [post1.id]). + once(). + reply(200, [post1]); + const post2Mock = nock(Client4.getBaseRoute()). + post('/posts/ids', [post2.id]). + once(). + reply(200, [post2]); + + let postId = 'post1'; + const {result, rerender, waitForNextUpdate} = renderHookWithContext( + () => usePost(postId), + ); + + // Initial state without post1 loaded + expect(result.current).toEqual(undefined); + expect(post1Mock.isDone()).toBe(false); + expect(post2Mock.isDone()).toBe(false); + + // Wait for the response with post1 + + await waitForNextUpdate(); + + expect(post1Mock.isDone()).toBe(true); + expect(post2Mock.isDone()).toBe(false); + expect(result.current).toEqual(post1); + + // Switch to post2 + postId = 'post2'; + rerender(); + + expect(result.current).toEqual(undefined); + + // Wait for the response with post2 + await waitForNextUpdate(); + + expect(post1Mock.isDone()).toBe(true); + expect(post2Mock.isDone()).toBe(true); + expect(result.current).toEqual(post2); + + // Switch back to post1 which has already been loaded + postId = 'post1'; + rerender(); + + expect(result.current).toEqual(post1); + + // We know there's no second call because nock is set to only mock the first request for each post + }); + + test('should batch multiple requests to fetch posts', async () => { + const mock = nock(Client4.getBaseRoute()). + post('/posts/ids', [post1.id, post2.id]). + once(). + reply(200, [post1, post2]); + + const {result, waitForNextUpdate} = renderHookWithContext( + () => { + return [ + usePost('post1'), + usePost('post2'), + ]; + }, + ); + + // Initial state without post1 loaded + expect(result.current).toEqual([undefined, undefined]); + expect(mock.isDone()).toBe(false); + + // Wait for the response + await waitForNextUpdate(); + + expect(result.current).toEqual([post1, post2]); + expect(mock.isDone()).toBe(true); + }); + }); +}); diff --git a/webapp/channels/src/components/common/hooks/usePost.ts b/webapp/channels/src/components/common/hooks/usePost.ts new file mode 100644 index 0000000000..dd76044248 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/usePost.ts @@ -0,0 +1,15 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import type {Post} from '@mattermost/types/posts'; + +import {getPostsByIdsBatched} from 'mattermost-redux/actions/posts'; +import {getPost} from 'mattermost-redux/selectors/entities/posts'; + +import {makeUseEntity} from './useEntity'; + +export const usePost = makeUseEntity({ + name: 'usePost', + fetch: (postId: string) => getPostsByIdsBatched([postId]), + selector: getPost, +}); diff --git a/webapp/channels/src/components/common/hooks/useUser.test.ts b/webapp/channels/src/components/common/hooks/useUser.test.ts new file mode 100644 index 0000000000..803579d206 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/useUser.test.ts @@ -0,0 +1,234 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import nock from 'nock'; +import * as ReactRedux from 'react-redux'; + +import {Client4} from 'mattermost-redux/client'; + +import {renderHookWithContext} from 'tests/react_testing_utils'; +import {TestHelper} from 'utils/test_helper'; + +import {useUser} from './useUser'; + +describe('useUser', () => { + const user1 = TestHelper.getUserMock({id: 'user1'}); + const user2 = TestHelper.getUserMock({id: 'user2'}); + + describe('useUser with fake dispatch', () => { + const dispatchMock = jest.fn(); + + beforeAll(() => { + jest.spyOn(ReactRedux, 'useDispatch').mockImplementation(() => dispatchMock); + }); + + afterAll(() => { + jest.restoreAllMocks(); + }); + + test("should return the user if they're already in the store", () => { + const {result} = renderHookWithContext( + () => useUser('user1'), + { + entities: { + users: { + profiles: { + user1, + }, + }, + }, + }, + ); + + expect(result.current).toBe(user1); + expect(dispatchMock).not.toHaveBeenCalled(); + }); + + test("should fetch the user if they're not in the store", () => { + const {result} = renderHookWithContext( + () => useUser('user1'), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + }); + + test('should only attempt to fetch the user once regardless of how many times the hook is used', () => { + const {result, rerender} = renderHookWithContext( + () => useUser('user1'), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + for (let i = 0; i < 10; i++) { + rerender(); + } + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + }); + + test('should attempt to fetch different users if the user changes', () => { + let userId = 'user1'; + const {result, rerender} = renderHookWithContext( + () => useUser(userId), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + userId = 'user2'; + rerender(); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(2); + }); + + test("should only attempt to fetch each user once when they aren't loaded", () => { + let userId = 'user1'; + const {result, replaceStoreState, rerender} = renderHookWithContext( + () => useUser(userId), + ); + + // Initial state without user1 loaded + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + // Simulate the response to loading user1 + replaceStoreState({ + entities: { + users: { + profiles: { + user1, + }, + }, + }, + }); + + expect(result.current).toBe(user1); + expect(dispatchMock).toHaveBeenCalledTimes(1); + + // Switch to user2 + userId = 'user2'; + + rerender(); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(2); + + // Simulate the response to loading user2 + replaceStoreState({ + entities: { + users: { + profiles: { + user1, + user2, + }, + }, + }, + }); + + expect(result.current).toBe(user2); + expect(dispatchMock).toHaveBeenCalledTimes(2); + + // Switch back to user1 which has already been loaded + userId = 'user1'; + + rerender(); + + expect(result.current).toBe(user1); + expect(dispatchMock).toHaveBeenCalledTimes(2); + }); + + test("shouldn't attempt to load anything when given an empty user ID", () => { + const {result} = renderHookWithContext( + () => useUser(''), + ); + + expect(result.current).toBe(undefined); + expect(dispatchMock).toHaveBeenCalledTimes(0); + }); + }); + + describe('with real dispatch', () => { + beforeAll(() => { + Client4.setUrl('http://localhost:8065'); + }); + + test("should only attempt to fetch each user once when they aren't loaded", async () => { + const user1Mock = nock(Client4.getBaseRoute()). + post('/users/ids', [user1.id]). + once(). + reply(200, [user1]); + const user2Mock = nock(Client4.getBaseRoute()). + post('/users/ids', [user2.id]). + once(). + reply(200, [user2]); + + let userId = 'user1'; + const {result, rerender, waitForNextUpdate} = renderHookWithContext( + () => useUser(userId), + ); + + // Initial state without user1 loaded + expect(result.current).toEqual(undefined); + expect(user1Mock.isDone()).toBe(false); + expect(user2Mock.isDone()).toBe(false); + + // Wait for the response with user1 + await waitForNextUpdate(); + + expect(user1Mock.isDone()).toBe(true); + expect(user2Mock.isDone()).toBe(false); + expect(result.current).toEqual(user1); + + // Switch to user2 + userId = 'user2'; + rerender(); + + expect(result.current).toEqual(undefined); + + // Wait for the response with user2 + await waitForNextUpdate(); + + expect(user1Mock.isDone()).toBe(true); + expect(user2Mock.isDone()).toBe(true); + expect(result.current).toEqual(user2); + + // Switch back to user1 which has already been loaded + userId = 'user1'; + rerender(); + + expect(result.current).toEqual(user1); + + // We know there's no second call because nock is set to only mock the first request for each user + }); + + test('should batch multiple requests to fetch users', async () => { + const mock = nock(Client4.getBaseRoute()). + post('/users/ids', [user1.id, user2.id]). + once(). + reply(200, [user1, user2]); + + const {result, waitForNextUpdate} = renderHookWithContext( + () => { + return [ + useUser('user1'), + useUser('user2'), + ]; + }, + ); + + // Initial state without user1 loaded + expect(result.current).toEqual([undefined, undefined]); + expect(mock.isDone()).toBe(false); + + // Wait for the response + await waitForNextUpdate(); + + expect(result.current).toEqual([user1, user2]); + expect(mock.isDone()).toBe(true); + }); + }); +}); diff --git a/webapp/channels/src/components/common/hooks/useUser.ts b/webapp/channels/src/components/common/hooks/useUser.ts new file mode 100644 index 0000000000..46815b8c59 --- /dev/null +++ b/webapp/channels/src/components/common/hooks/useUser.ts @@ -0,0 +1,15 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import type {UserProfile} from '@mattermost/types/users'; + +import {getMissingProfilesByIds} from 'mattermost-redux/actions/users'; +import {getUser} from 'mattermost-redux/selectors/entities/users'; + +import {makeUseEntity} from './useEntity'; + +export const useUser = makeUseEntity({ + name: 'useUser', + fetch: (userId) => getMissingProfilesByIds([userId]), + selector: (state, userId) => getUser(state, userId), +}); diff --git a/webapp/channels/src/components/post/index.test.tsx b/webapp/channels/src/components/post/index.test.tsx new file mode 100644 index 0000000000..afca784741 --- /dev/null +++ b/webapp/channels/src/components/post/index.test.tsx @@ -0,0 +1,176 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import nock from 'nock'; +import React from 'react'; + +import {CollapsedThreads} from '@mattermost/types/config'; + +import {Client4} from 'mattermost-redux/client'; + +import {renderWithContext, screen, waitFor} from 'tests/react_testing_utils'; +import {Locations} from 'utils/constants'; +import {TestHelper} from 'utils/test_helper'; + +import ConnectedPostComponent from './index'; + +describe('PostComponent', () => { + beforeAll(() => { + Client4.setUrl('http://localhost:8065'); + }); + + test('MM-62710 should attempt to load missing root post when CRT is disabled', async () => { + const team1 = TestHelper.getTeamMock({id: 'team1'}); + const channel1 = TestHelper.getChannelMock({id: 'channel1', team_id: team1.id}); + + const currentUser = TestHelper.getUserMock({id: 'currentUser', username: 'current_user'}); + const otherUser = TestHelper.getUserMock({id: 'otherUser', username: 'other_user'}); + + const post1 = TestHelper.getPostMock({ + id: 'post1', + user_id: otherUser.id, + channel_id: channel1.id, + create_at: 1000, + message: 'This is the root post that will need to be loaded', + }); + const post2 = TestHelper.getPostMock({ + id: 'post2', + user_id: currentUser.id, + channel_id: channel1.id, + create_at: 1001, + message: 'This is a different root post', + }); + const post3 = TestHelper.getPostMock({ + id: 'post3', + user_id: currentUser.id, + channel_id: channel1.id, + root_id: post1.id, + create_at: 1002, + message: 'This is the test post', + }); + + const postsMock = nock(Client4.getBaseRoute()). + post('/posts/ids', [post1.id]). + reply(200, [post1]); + const usersMock = nock(Client4.getBaseRoute()). + post('/users/ids', [otherUser.id]). + reply(200, [otherUser]); + + renderWithContext( + , + { + entities: { + channels: { + channels: { + channel1, + }, + }, + posts: { + posts: { + post2, + }, + }, + }, + }, + ); + + expect(screen.getByText('This is the test post')).toBeInTheDocument(); + + // The Commented on line will be missing until the root post and user are loaded + expect(screen.queryByText('other_user')).not.toBeInTheDocument(); + expect(screen.queryByText('This is the root post that will need to be loaded')).not.toBeInTheDocument(); + + // The required user and post will be loaded async + await waitFor(() => { + expect(screen.queryByText('other_user')).toBeInTheDocument(); + expect(screen.queryByText('This is the root post that will need to be loaded')).toBeInTheDocument(); + }); + + expect(usersMock.isDone()).toBe(true); + expect(postsMock.isDone()).toBe(true); + }); + + test('should not attempt to load missing root post when CRT is enabled', async () => { + const team1 = TestHelper.getTeamMock({id: 'team1'}); + const channel1 = TestHelper.getChannelMock({id: 'channel1', team_id: team1.id}); + + const currentUser = TestHelper.getUserMock({id: 'currentUser', username: 'current_user'}); + const otherUser = TestHelper.getUserMock({id: 'otherUser', username: 'other_user'}); + + const post1 = TestHelper.getPostMock({ + id: 'post1', + user_id: otherUser.id, + channel_id: channel1.id, + create_at: 1000, + message: 'This is the root post that will need to be loaded', + }); + const post2 = TestHelper.getPostMock({ + id: 'post2', + user_id: currentUser.id, + channel_id: channel1.id, + create_at: 1001, + message: 'This is a different root post', + }); + const post3 = TestHelper.getPostMock({ + id: 'post3', + user_id: currentUser.id, + channel_id: channel1.id, + root_id: post1.id, + create_at: 1002, + message: 'This is the test post', + }); + + const postsMock = nock(Client4.getBaseRoute()). + post('/posts/ids', [post1.id]). + reply(200, [post1]); + const usersMock = nock(Client4.getBaseRoute()). + post('/users/ids', [otherUser.id]). + reply(200, [otherUser]); + + renderWithContext( + , + { + entities: { + channels: { + channels: { + channel1, + }, + }, + general: { + config: { + CollapsedThreads: CollapsedThreads.ALWAYS_ON, + }, + }, + posts: { + posts: { + post2, + }, + }, + }, + }, + ); + + expect(screen.getByText('This is the test post')).toBeInTheDocument(); + + // The Commented on line will be missing until the root post and user are loaded + expect(screen.queryByText('other_user')).not.toBeInTheDocument(); + expect(screen.queryByText('This is the root post that will need to be loaded')).not.toBeInTheDocument(); + + // The required user and post will be loaded async + await waitFor(() => { + expect(screen.queryByText('other_user')).toBeInTheDocument(); + expect(screen.queryByText('This is the root post that will need to be loaded')).toBeInTheDocument(); + }); + + expect(usersMock.isDone()).toBe(true); + expect(postsMock.isDone()).toBe(true); + }); +}); diff --git a/webapp/channels/src/components/post/post_component.tsx b/webapp/channels/src/components/post/post_component.tsx index 9872d08af8..867c7c6e55 100644 --- a/webapp/channels/src/components/post/post_component.tsx +++ b/webapp/channels/src/components/post/post_component.tsx @@ -402,12 +402,11 @@ function PostComponent(props: Props) { const postClass = classNames('post__body', {'post--edited': PostUtils.isEdited(post), 'search-item-snippet': isSearchResultItem}); let comment; - if (props.isFirstReply && props.parentPost && props.parentPostUser && post.type !== Constants.PostTypes.EPHEMERAL) { + if (props.isFirstReply && post.type !== Constants.PostTypes.EPHEMERAL) { comment = ( ); } diff --git a/webapp/channels/src/components/post_view/commented_on/__snapshots__/commented_on.test.tsx.snap b/webapp/channels/src/components/post_view/commented_on/__snapshots__/commented_on.test.tsx.snap deleted file mode 100644 index 45d39c8de1..0000000000 --- a/webapp/channels/src/components/post_view/commented_on/__snapshots__/commented_on.test.tsx.snap +++ /dev/null @@ -1,220 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`components/post_view/CommentedOn should match snapshot 1`] = ` -
- - - - , - } - } - /> - - text message - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshot 2`] = ` -
- - - - , - } - } - /> - - text message - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshot 3`] = ` -
- - - - , - } - } - /> - - - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshots for post with props.fallback as message 1`] = ` -
- - - - , - } - } - /> - - This is fallback message - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshots for post with props.pretext as message 1`] = ` -
- - - - , - } - } - /> - - This is a pretext - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshots for post with props.text as message 1`] = ` -
- - - - , - } - } - /> - - This is a text - - -
-`; - -exports[`components/post_view/CommentedOn should match snapshots for post with props.title as message 1`] = ` -
- - - - , - } - } - /> - - This is a title - - -
-`; diff --git a/webapp/channels/src/components/post_view/commented_on/commented_on.test.tsx b/webapp/channels/src/components/post_view/commented_on/commented_on.test.tsx index ff816fb9dd..4dbd705453 100644 --- a/webapp/channels/src/components/post_view/commented_on/commented_on.test.tsx +++ b/webapp/channels/src/components/post_view/commented_on/commented_on.test.tsx @@ -1,173 +1,293 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {shallow} from 'enzyme'; import React from 'react'; import CommentedOn from 'components/post_view/commented_on/commented_on'; -import CommentedOnFilesMessage from 'components/post_view/commented_on_files_message'; +import {renderWithContext, screen} from 'tests/react_testing_utils'; import {TestHelper} from 'utils/test_helper'; describe('components/post_view/CommentedOn', () => { - const baseProps = { - displayName: 'user_displayName', - enablePostUsernameOverride: false, - onCommentClick: jest.fn(), - post: TestHelper.getPostMock({ - id: 'post_id', - message: 'text message', - props: { - from_webhook: 'true', - override_username: 'override_username', - }, - update_at: 10, - edit_at: 20, - delete_at: 30, - channel_id: 'channel_id', - root_id: 'root_id', - original_id: 'original_id', - hashtags: 'hashtags', - pending_post_id: 'pending_post_id', - reply_count: 1, - }), - }; - - test('should match snapshot', () => { - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); - - wrapper.setProps({enablePostUsernameOverride: true}); - expect(wrapper).toMatchSnapshot(); - expect(wrapper.find(CommentedOnFilesMessage).exists()).toBe(false); - - const newPost = { - id: 'post_id', - message: '', - file_ids: ['file_id_1', 'file_id_2'], - }; - wrapper.setProps({post: newPost, enablePostUsernameOverride: false}); - expect(wrapper).toMatchSnapshot(); - expect(wrapper.find(CommentedOnFilesMessage).exists()).toBe(true); + const user1 = TestHelper.getUserMock({ + id: 'user1', + }); + const post1 = TestHelper.getPostMock({ + id: 'post1', + user_id: user1.id, + message: 'text message', }); - test('should match snapshots for post with props.pretext as message', () => { - const newPost = { - id: 'post_id', - message: '', - props: { - from_webhook: 'true', - override_username: 'override_username', - attachments: [{ - pretext: 'This is a pretext', - }], + test("should render the root post's message and author", () => { + renderWithContext( + , + { + entities: { + posts: { + posts: { + post1, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, }, - }; - const newProps = { - ...baseProps, - post: { - ...baseProps.post, - ...newPost, - }, - enablePostUsernameOverride: true, - }; + ); - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText(textInChildren("Commented on some-user's message: text message"))).toBeInTheDocument(); }); - test('should match snapshots for post with props.title as message', () => { - const newPost = { - id: 'post_id', - message: '', - props: { - from_webhook: 'true', - override_username: 'override_username', - attachments: [{ - pretext: '', - title: 'This is a title', - }], + test("should render a placeholder name when the post's author isn't loaded", () => { + renderWithContext( + , + { + entities: { + posts: { + posts: { + post1, + }, + }, + }, }, - }; - const newProps = { - ...baseProps, - post: { - ...baseProps.post, - ...newPost, - }, - enablePostUsernameOverride: true, - }; + ); - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText(textInChildren("Commented on Someone's message: text message"))).toBeInTheDocument(); }); - test('should match snapshots for post with props.text as message', () => { - const newPost = { - id: 'post_id', - message: '', - props: { - from_webhook: 'true', - override_username: 'override_username', - attachments: [{ - pretext: '', - title: '', - text: 'This is a text', - }], - }, - }; + test("should render a placeholder when the post isn't loaded", () => { + renderWithContext( + , + ); - const newProps = { - ...baseProps, - post: { - ...baseProps.post, - ...newPost, - }, - enablePostUsernameOverride: true, - }; - - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText(textInChildren("Commented on Someone's message: Loading…"))).toBeInTheDocument(); }); - test('should match snapshots for post with props.fallback as message', () => { - const newPost = { - id: 'post_id', - message: '', - props: { - from_webhook: 'true', - override_username: 'override_username', - attachments: [{ - pretext: '', - title: '', - text: '', - fallback: 'This is fallback message', - }], - }, - }; + test("should render the root post's file attachments when it has no message", () => { + const file1 = TestHelper.getFileInfoMock({id: 'file1', create_at: 1000, name: 'image.png'}); + const file2 = TestHelper.getFileInfoMock({id: 'file2', create_at: 1001, name: 'contract.doc'}); - const newProps = { - ...baseProps, - post: { - ...baseProps.post, - ...newPost, + renderWithContext( + , + { + entities: { + files: { + fileIdsByPostId: { + post1: [file1.id, file2.id], + }, + files: { + file1, + file2, + }, + }, + posts: { + posts: { + [post1.id]: { + ...post1, + message: '', + file_ids: [file1.id, file2.id], + }, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, }, - enablePostUsernameOverride: true, - }; + ); - const wrapper = shallow(); - expect(wrapper).toMatchSnapshot(); + expect(screen.getByText(textInChildren("Commented on some-user's message: image.png plus 1 other file"))).toBeInTheDocument(); + }); + + test("should render the root post's props.pretext as message", () => { + renderWithContext( + , + { + entities: { + general: { + config: { + EnablePostUsernameOverride: 'true', + }, + }, + posts: { + posts: { + [post1.id]: { + ...post1, + message: '', + props: { + from_webhook: 'true', + override_username: 'override_username', + attachments: [{ + pretext: 'This is a pretext', + }], + }, + }, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, + }, + ); + + // This incorrectly uses the post author's name due to MM-63564 + expect(screen.getByText(textInChildren("Commented on some-user's message: This is a pretext"))).toBeInTheDocument(); + }); + + test("should render the root post's props.title as message", () => { + renderWithContext( + , + { + entities: { + general: { + config: { + EnablePostUsernameOverride: 'true', + }, + }, + posts: { + posts: { + [post1.id]: { + ...post1, + message: '', + props: { + from_webhook: 'true', + override_username: 'override_username', + attachments: [{ + title: 'This is a title', + }], + }, + }, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, + }, + ); + + // This incorrectly uses the post author's name due to MM-63564 + expect(screen.getByText(textInChildren("Commented on some-user's message: This is a title"))).toBeInTheDocument(); + }); + + test("should render the root post's props.text as message", () => { + renderWithContext( + , + { + entities: { + general: { + config: { + EnablePostUsernameOverride: 'true', + }, + }, + posts: { + posts: { + [post1.id]: { + ...post1, + message: '', + props: { + from_webhook: 'true', + override_username: 'override_username', + attachments: [{ + text: 'This is a text', + }], + }, + }, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, + }, + ); + + // This incorrectly uses the post author's name due to MM-63564 + expect(screen.getByText(textInChildren("Commented on some-user's message: This is a text"))).toBeInTheDocument(); + }); + + test("should render the root post's props.fallback as message", () => { + renderWithContext( + , + { + entities: { + general: { + config: { + EnablePostUsernameOverride: 'true', + }, + }, + posts: { + posts: { + [post1.id]: { + ...post1, + message: '', + props: { + from_webhook: 'true', + override_username: 'override_username', + attachments: [{ + fallback: 'This is fallback message', + }], + }, + }, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, + }, + ); + + // This incorrectly uses the post author's name due to MM-63564 + expect(screen.getByText(textInChildren("Commented on some-user's message: This is fallback message"))).toBeInTheDocument(); }); test('should call onCommentClick on click of text message', () => { - const wrapper = shallow(); + const onCommentClick = jest.fn(); + renderWithContext( + , + { + entities: { + posts: { + posts: { + post1, + }, + }, + users: { + profiles: { + user1, + }, + }, + }, + }, + ); - wrapper.find('a').first().simulate('click'); - expect(baseProps.onCommentClick).toHaveBeenCalledTimes(1); - }); + screen.getByText('text message').click(); - test('Should trigger search with override_username', () => { - const wrapper = shallow(); - wrapper.setProps({enablePostUsernameOverride: true}); + expect(onCommentClick).toHaveBeenCalledTimes(1); }); }); + +function textInChildren(matchedText: string) { + return (content: string, element: Element | null) => { + const hasText = element?.textContent === matchedText; + const childHasText = element && Array.from(element?.children).some((child) => child?.textContent === matchedText); + return hasText && !childHasText; + }; +} diff --git a/webapp/channels/src/components/post_view/commented_on/commented_on.tsx b/webapp/channels/src/components/post_view/commented_on/commented_on.tsx index 037862d72c..cb66fcbd64 100644 --- a/webapp/channels/src/components/post_view/commented_on/commented_on.tsx +++ b/webapp/channels/src/components/post_view/commented_on/commented_on.tsx @@ -5,9 +5,9 @@ import React, {memo} from 'react'; import {FormattedMessage} from 'react-intl'; import {isMessageAttachmentArray} from '@mattermost/types/message_attachments'; -import type {Post} from '@mattermost/types/posts'; -import type {UserProfile as UserProfileType} from '@mattermost/types/users'; +import {usePost} from 'components/common/hooks/usePost'; +import {useUser} from 'components/common/hooks/useUser'; import CommentedOnFilesMessage from 'components/post_view/commented_on_files_message'; import UserProfile from 'components/user_profile'; @@ -15,36 +15,37 @@ import {stripMarkdown} from 'utils/markdown'; import * as Utils from 'utils/utils'; type Props = { - enablePostUsernameOverride?: boolean; - parentPostUser?: UserProfileType; onCommentClick?: React.EventHandler; - post: Post; + rootId: string; }; -function CommentedOn({post, parentPostUser, onCommentClick}: Props) { - const makeCommentedOnMessage = () => { - let message: React.ReactNode = ''; - if (post.message) { - message = Utils.replaceHtmlEntities(post.message); - } else if (post.file_ids && post.file_ids.length > 0) { - message = ( - - ); - } else if (isMessageAttachmentArray(post.props?.attachments) && post.props.attachments.length > 0) { - const attachment = post.props.attachments[0]; - const webhookMessage = attachment.pretext || attachment.title || attachment.text || attachment.fallback || ''; - message = Utils.replaceHtmlEntities(webhookMessage); - } +function CommentedOn({onCommentClick, rootId}: Props) { + const rootPost = usePost(rootId); + const rootPostUser = useUser(rootPost?.user_id ?? ''); - return message; - }; - - const message = makeCommentedOnMessage(); - const parentPostUserId = parentPostUser?.id ?? ''; + let message: React.ReactNode = ''; + if (!rootPost) { + message = ( + + ); + } else if (rootPost.message) { + message = Utils.replaceHtmlEntities(rootPost.message); + } else if (rootPost.file_ids && rootPost.file_ids.length > 0) { + message = ( + + ); + } else if (isMessageAttachmentArray(rootPost.props?.attachments) && rootPost.props.attachments.length > 0) { + const attachment = rootPost.props.attachments[0]; + const webhookMessage = attachment.pretext || attachment.title || attachment.text || attachment.fallback || ''; + message = Utils.replaceHtmlEntities(webhookMessage); + } const parentUserProfile = ( ); diff --git a/webapp/channels/src/components/post_view/commented_on/index.ts b/webapp/channels/src/components/post_view/commented_on/index.ts index 738c38f43b..4d9eeea242 100644 --- a/webapp/channels/src/components/post_view/commented_on/index.ts +++ b/webapp/channels/src/components/post_view/commented_on/index.ts @@ -1,37 +1,6 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {connect} from 'react-redux'; - -import type {Post} from '@mattermost/types/posts'; - -import {getConfig} from 'mattermost-redux/selectors/entities/general'; -import {getUser} from 'mattermost-redux/selectors/entities/users'; - -import {getDisplayNameByUser} from 'utils/utils'; - -import type {GlobalState} from 'types/store'; - import CommentedOn from './commented_on'; -type Props = { - post: Post; -} - -function mapStateToProps(state: GlobalState, ownProps: Props) { - let displayName = ''; - if (ownProps.post) { - const user = getUser(state, ownProps.post.user_id); - displayName = getDisplayNameByUser(state, user); - } - - const config = getConfig(state); - const enablePostUsernameOverride = config.EnablePostUsernameOverride === 'true'; - - return { - displayName, - enablePostUsernameOverride, - }; -} - -export default connect(mapStateToProps)(CommentedOn); +export default CommentedOn; diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 97f694f7e1..517720c990 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -4684,6 +4684,7 @@ "post_body.check_for_out_of_channel_mentions.message.one": "did not get notified by this mention because they are not in the channel. Would you like to ", "post_body.check_for_out_of_channel_mentions.others": "{numOthers} others", "post_body.commentedOn": "Commented on {name}'s message: ", + "post_body.commentedOn.loadingMessage": "Loading…", "post_body.deleted": "(message deleted)", "post_body.plusMore": " plus {count, number} other {count, plural, one {file} other {files}}", "post_delete.notPosted": "Comment could not be posted", diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts index 60b48aba70..2d41c90558 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts @@ -35,6 +35,7 @@ import * as PostSelectors from 'mattermost-redux/selectors/entities/posts'; import {getUnreadScrollPositionPreference, isCollapsedThreadsEnabled} from 'mattermost-redux/selectors/entities/preferences'; import {getCurrentUserId, getUsersByUsername} from 'mattermost-redux/selectors/entities/users'; import type {ActionResult, DispatchFunc, GetStateFunc, ActionFunc, ActionFuncAsync, ThunkActionFunc} from 'mattermost-redux/types/actions'; +import {DelayedDataLoader} from 'mattermost-redux/utils/data_loader'; import {isCombinedUserActivityPost} from 'mattermost-redux/utils/post_list'; import {logError, LogErrorBarMode} from './errors'; @@ -164,7 +165,6 @@ export function getPost(postId: string): ActionFuncAsync { } dispatch(receivedPost(post, crtEnabled)); - dispatch(batchFetchStatusesProfilesGroupsFromPosts([post])); return {data: post}; }; @@ -1035,6 +1035,25 @@ export function getPostsByIds(ids: string[]): ActionFuncAsync { }; } +export function getPostsByIdsBatched(postIds: string[]): ActionFuncAsync { + const maxBatchSize = 100; + const wait = 100; + + return async (dispatch, getState, {loaders}: any) => { + if (!loaders.postsByIdsLoader) { + loaders.postsByIdsLoader = new DelayedDataLoader({ + fetchBatch: (postIds) => dispatch(getPostsByIds(postIds)), + maxBatchSize, + wait, + }); + } + + loaders.postsByIdsLoader.queue(postIds); + + return {data: true}; + }; +} + export function getPostEditHistory(postId: string) { return bindClientFunc({ clientFunc: Client4.getPostEditHistory, diff --git a/webapp/channels/src/tests/react_testing_utils.tsx b/webapp/channels/src/tests/react_testing_utils.tsx index dfb3334917..559d0b2ba8 100644 --- a/webapp/channels/src/tests/react_testing_utils.tsx +++ b/webapp/channels/src/tests/react_testing_utils.tsx @@ -115,12 +115,25 @@ export const renderHookWithContext = ( }; replaceGlobalStore(() => renderState.store); - return renderHook(callback, { + const results = renderHook(callback, { wrapper: ({children}) => { // Every time this is called, these values should be updated from `renderState` return {children}; }, }); + + return { + ...results, + + /** + * Rerenders the component after replacing the entire store state with the provided one. + */ + replaceStoreState: (newInitialState: DeepPartial) => { + renderState.store = configureOrMockStore(newInitialState, renderState.options.useMockedStore, partialOptions?.pluginReducers); + + results.rerender(); + }, + }; }; function configureOrMockStore(initialState: DeepPartial, useMockedStore: boolean, extraReducersKeys?: string[]) {