From d25326764f8a503b100131d3a5173a483e654c22 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Wed, 2 Apr 2025 11:45:00 -0400 Subject: [PATCH] MM-62710 Ensure CommentedOn message is never missing due to unloaded data (#30572) * MM-62710 Ensure CommentedOn message is never missing due to unloaded data There's more information in the Jira ticket on how this occurs, but the short version is that there's a way for either the root post and/or the root post's author to be missing which previously caused the "Commented On" line not to be rendered for a comment with CRT disabled. That caused the post to appear as if it was part of the wrong thread. Now, the CommentedOn component is always mounted when for the first non-consecutive reply to a thread, and it knows how to load that missing data itself. I also took the opportunity to implement basic `useUser` and `usePost` hooks which will fetch the user/post if needed. They're both implemented using a shared `makeUseEntity` which should be usable for other entities (objects stored in Redux state and fetched using a Redux action). These also both use actions that use `DelayedDataLoader` to batch and debounce requests for that data as well! Assuming this works well, I'm hoping we can start using this pattern elsewhere in the app. * Update tests and remove unneeded mapStateToProps * Address feedback * Fix playwright tests * revert Fix playwright tests --------- Co-authored-by: yasserfaraazkhan --- .../src/components/common/hooks/useEntity.ts | 39 ++ .../components/common/hooks/usePost.test.ts | 235 ++++++++++ .../src/components/common/hooks/usePost.ts | 15 + .../components/common/hooks/useUser.test.ts | 234 ++++++++++ .../src/components/common/hooks/useUser.ts | 15 + .../src/components/post/index.test.tsx | 176 ++++++++ .../src/components/post/post_component.tsx | 5 +- .../__snapshots__/commented_on.test.tsx.snap | 220 ---------- .../commented_on/commented_on.test.tsx | 400 ++++++++++++------ .../post_view/commented_on/commented_on.tsx | 51 +-- .../post_view/commented_on/index.ts | 33 +- webapp/channels/src/i18n/en.json | 1 + .../mattermost-redux/src/actions/posts.ts | 21 +- .../src/tests/react_testing_utils.tsx | 15 +- 14 files changed, 1038 insertions(+), 422 deletions(-) create mode 100644 webapp/channels/src/components/common/hooks/useEntity.ts create mode 100644 webapp/channels/src/components/common/hooks/usePost.test.ts create mode 100644 webapp/channels/src/components/common/hooks/usePost.ts create mode 100644 webapp/channels/src/components/common/hooks/useUser.test.ts create mode 100644 webapp/channels/src/components/common/hooks/useUser.ts create mode 100644 webapp/channels/src/components/post/index.test.tsx delete mode 100644 webapp/channels/src/components/post_view/commented_on/__snapshots__/commented_on.test.tsx.snap 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[]) {