From 4d41365fbe3f013be7aa354b48260f873b12aa42 Mon Sep 17 00:00:00 2001 From: Konstantinos Pittas Date: Fri, 2 Jun 2023 13:30:33 +0300 Subject: [PATCH] [MM-52541] Mark files as deleted along with thread (#23226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * mark thread files as deleted * add missing check * improve query * Stopped rendering post preview if post is deleted * Fixed lint error * Fixed test * updated types * Removed deleted post from other post's embed data * Added tests * Apply suggestions from code review Co-authored-by: Daniel Espino García * lint fix --------- Co-authored-by: Konstantinos Pittas Co-authored-by: Harshil Sharma Co-authored-by: Mattermost Build Co-authored-by: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Co-authored-by: Daniel Espino García --- server/channels/store/sqlstore/post_store.go | 24 +++++ server/channels/store/storetest/post_store.go | 82 +++++++++++++++++ .../forward_post_modal/forward_post_modal.tsx | 1 - .../post_body_additional_content.tsx | 1 - .../post_message_preview.test.tsx.snap | 89 +++++++++++++++++++ .../post_view/post_message_preview/index.ts | 5 +- .../post_message_preview.test.tsx | 26 ++++++ .../post_message_preview.tsx | 2 + .../src/reducers/entities/posts.test.ts | 53 +++++++++++ .../src/reducers/entities/posts.ts | 37 +++++++- 10 files changed, 314 insertions(+), 6 deletions(-) diff --git a/server/channels/store/sqlstore/post_store.go b/server/channels/store/sqlstore/post_store.go index 61568bc7bd..59d7d7ac3d 100644 --- a/server/channels/store/sqlstore/post_store.go +++ b/server/channels/store/sqlstore/post_store.go @@ -2913,6 +2913,30 @@ func (s *SqlPostStore) deleteThread(transaction *sqlxTxWrapper, postId string, d return errors.Wrapf(err, "failed to mark thread for root post %s as deleted", postId) } + return s.deleteThreadFiles(transaction, postId, deleteAtTime) +} + +func (s *SqlPostStore) deleteThreadFiles(transaction *sqlxTxWrapper, postID string, deleteAtTime int64) error { + var query sq.UpdateBuilder + if s.DriverName() == model.DatabaseDriverPostgres { + query = s.getQueryBuilder().Update("FileInfo"). + Set("DeleteAt", deleteAtTime). + From("Posts") + } else { + query = s.getQueryBuilder().Update("FileInfo", "Posts"). + Set("FileInfo.DeleteAt", deleteAtTime) + } + + query = query.Where(sq.And{ + sq.Expr("FileInfo.PostId = Posts.Id"), + sq.Eq{"Posts.RootId": postID}, + }) + + _, err := transaction.ExecBuilder(query) + if err != nil { + return errors.Wrapf(err, "failed to mark files of thread post %s as deleted", postID) + } + return nil } diff --git a/server/channels/store/storetest/post_store.go b/server/channels/store/storetest/post_store.go index ed4715d562..e9da558e79 100644 --- a/server/channels/store/storetest/post_store.go +++ b/server/channels/store/storetest/post_store.go @@ -1303,6 +1303,88 @@ func testPostStoreDelete(t *testing.T, ss store.Store) { // last reply at should be 0 require.Equal(t, int64(0), thread.LastReplyAt) }) + + t.Run("thread with file attachments", func(t *testing.T) { + teamId := model.NewId() + channel, err := ss.Channel().Save(&model.Channel{ + TeamId: teamId, + DisplayName: "DisplayName1", + Name: "channel" + model.NewId(), + Type: model.ChannelTypeOpen, + }, -1) + require.NoError(t, err) + + // Create a root post + rootPost1, err := ss.Post().Save(&model.Post{ + ChannelId: channel.Id, + UserId: model.NewId(), + Message: NewTestId(), + }) + require.NoError(t, err) + + // Create another root post + rootPost2, err := ss.Post().Save(&model.Post{ + ChannelId: channel.Id, + UserId: model.NewId(), + Message: NewTestId(), + }) + require.NoError(t, err) + + // Reply to first root post with file attachments + replyPost1, err := ss.Post().Save(&model.Post{ + ChannelId: rootPost1.ChannelId, + UserId: model.NewId(), + Message: NewTestId(), + RootId: rootPost1.Id, + }) + require.NoError(t, err) + file11, err := ss.FileInfo().Save(&model.FileInfo{ + Id: model.NewId(), + PostId: replyPost1.Id, + CreatorId: replyPost1.UserId, + Path: "file1.txt", + }) + require.NoError(t, err) + file12, err := ss.FileInfo().Save(&model.FileInfo{ + Id: model.NewId(), + PostId: replyPost1.Id, + CreatorId: replyPost1.UserId, + Path: "file2.png", + }) + require.NoError(t, err) + + // Reply to second root post with file attachments + replyPost2, err := ss.Post().Save(&model.Post{ + ChannelId: rootPost2.ChannelId, + UserId: model.NewId(), + Message: NewTestId(), + RootId: rootPost2.Id, + }) + require.NoError(t, err) + file21, err := ss.FileInfo().Save(&model.FileInfo{ + Id: model.NewId(), + PostId: replyPost2.Id, + CreatorId: replyPost2.UserId, + Path: "file1.txt", + }) + require.NoError(t, err) + + // Delete the first root post + err = ss.Post().Delete(rootPost1.Id, model.GetMillis(), "") + require.NoError(t, err) + + // Verify the reply post's files are deleted + _, err = ss.FileInfo().Get(file11.Id) + require.Error(t, err, "Deleted id should have failed") + require.IsType(t, &store.ErrNotFound{}, err) + _, err = ss.FileInfo().Get(file12.Id) + require.Error(t, err, "Deleted id should have failed") + require.IsType(t, &store.ErrNotFound{}, err) + + // Verify the other reply post's files are NOT deleted + _, err = ss.FileInfo().Get(file21.Id) + require.NoError(t, err, "Not deleted id should have succeeded") + }) } func testPostStorePermDelete1Level(t *testing.T, ss store.Store) { diff --git a/webapp/channels/src/components/forward_post_modal/forward_post_modal.tsx b/webapp/channels/src/components/forward_post_modal/forward_post_modal.tsx index 8759fddac2..035594d1b1 100644 --- a/webapp/channels/src/components/forward_post_modal/forward_post_modal.tsx +++ b/webapp/channels/src/components/forward_post_modal/forward_post_modal.tsx @@ -291,7 +291,6 @@ const ForwardPostModal = ({onExited, post, actions}: Props) => { > ); diff --git a/webapp/channels/src/components/post_view/post_message_preview/__snapshots__/post_message_preview.test.tsx.snap b/webapp/channels/src/components/post_view/post_message_preview/__snapshots__/post_message_preview.test.tsx.snap index 5dd1a59e5d..c1ba45b470 100644 --- a/webapp/channels/src/components/post_view/post_message_preview/__snapshots__/post_message_preview.test.tsx.snap +++ b/webapp/channels/src/components/post_view/post_message_preview/__snapshots__/post_message_preview.test.tsx.snap @@ -668,3 +668,92 @@ exports[`PostMessagePreview should render correctly 1`] = ` `; exports[`PostMessagePreview should render without preview 1`] = `""`; + +exports[`PostMessagePreview show render without preview when preview posts becomes undefined after being defined 1`] = ` + +
+
+
+
+ + + +
+
+
+ +
+
+ +
+
+ +
+

+ +

+
+
+ +`; + +exports[`PostMessagePreview show render without preview when preview posts becomes undefined after being defined 2`] = `""`; diff --git a/webapp/channels/src/components/post_view/post_message_preview/index.ts b/webapp/channels/src/components/post_view/post_message_preview/index.ts index 12b1b894e9..a3348907f5 100644 --- a/webapp/channels/src/components/post_view/post_message_preview/index.ts +++ b/webapp/channels/src/components/post_view/post_message_preview/index.ts @@ -8,7 +8,7 @@ import {bindActionCreators, Dispatch} from 'redux'; import {GlobalState} from 'types/store'; import {GenericAction} from 'mattermost-redux/types/actions'; -import {Post, PostPreviewMetadata} from '@mattermost/types/posts'; +import {PostPreviewMetadata} from '@mattermost/types/posts'; import {makeGetChannel} from 'mattermost-redux/selectors/entities/channels'; import {get} from 'mattermost-redux/selectors/entities/preferences'; @@ -29,7 +29,6 @@ import PostMessagePreview from './post_message_preview'; export type OwnProps = { metadata: PostPreviewMetadata; - previewPost?: Post; preventClickAction?: boolean; previewFooterMessage?: string; } @@ -43,7 +42,7 @@ function makeMapStateToProps() { let user = null; let embedVisible = false; let channelDisplayName = ownProps.metadata.channel_display_name; - const previewPost = getPost(state, ownProps.metadata.post_id) || ownProps.previewPost; + const previewPost = getPost(state, ownProps.metadata.post_id); if (previewPost && previewPost.user_id) { user = getUser(state, previewPost.user_id); diff --git a/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.test.tsx b/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.test.tsx index 604b97217c..fabbb4f18e 100644 --- a/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.test.tsx +++ b/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.test.tsx @@ -65,6 +65,32 @@ describe('PostMessagePreview', () => { expect(wrapper).toMatchSnapshot(); }); + test('show render without preview when preview posts becomes undefined after being defined', () => { + const props = {...baseProps}; + let wrapper = shallow( + , + ); + + expect(wrapper).toMatchSnapshot(); + let permalink = wrapper.find('.permalink'); + expect(permalink.length).toBe(1); + + // now we'll set the preview post to undefined. This happens when the + // previewed post is deleted. + props.previewPost = undefined; + + wrapper = shallow( + , + ); + expect(wrapper).toMatchSnapshot(); + permalink = wrapper.find('.permalink'); + expect(permalink.length).toBe(0); + }); + test('should not render bot icon', () => { const postProps = { override_icon_url: 'https://fakeicon.com/image.jpg', diff --git a/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.tsx b/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.tsx index cd3195bc29..7a0ef47348 100644 --- a/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.tsx +++ b/webapp/channels/src/components/post_view/post_message_preview/post_message_preview.tsx @@ -25,8 +25,10 @@ import {Constants} from 'utils/constants'; import {General} from 'mattermost-redux/constants'; import {OwnProps} from './index'; +import {Post} from '@mattermost/types/posts'; export type Props = OwnProps & { + previewPost?: Post; currentTeamUrl: string; channelDisplayName?: string; user: UserProfile | null; diff --git a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.test.ts b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.test.ts index c56ba21e64..8529957f5f 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.test.ts @@ -317,6 +317,59 @@ describe('posts', () => { }); }); + it('should remove deleted post from other post embeds', () => { + const post1 = {id: 'post1', message: 'Post 1'}; + const post2 = { + id: 'post2', + message: 'Post 2', + metadata: { + embeds: [ + { + type: 'permalink', + data: { + post_id: 'post1', + }, + }, + ], + }, + }; + const post3 = { + id: 'post3', + message: 'Post 3', + metadata: { + embeds: [ + { + type: 'permalink', + data: { + post_id: 'post1', + }, + }, + { + type: 'permalink', + data: { + post_id: 'post2', + }, + }, + ], + }, + }; + + const state = deepFreeze({ + post1, + post2, + post3, + }); + + const nextState = reducers.handlePosts(state, { + type: PostTypes.POST_DELETED, + data: {id: 'post1'}, + }); + + expect(nextState).not.toBe(state); + expect(nextState.post2.metadata.embeds.length).toBe(0); + expect(nextState.post3.metadata.embeds.length).toBe(1); + }); + it('should not remove the rest of the thread when deleting a comment', () => { const state = deepFreeze({ post1: {id: 'post1'}, diff --git a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts index 8324f77040..3f176e1709 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/reducers/entities/posts.ts @@ -16,6 +16,8 @@ import { PostOrderBlock, MessageHistory, PostAcknowledgement, + PostEmbed, + PostPreviewMetadata, } from '@mattermost/types/posts'; import {UserProfile} from '@mattermost/types/users'; import {Reaction} from '@mattermost/types/reactions'; @@ -200,11 +202,44 @@ export function handlePosts(state: RelationOneToOne = {}, action: Ge }, }; - // Remove any of its comments for (const otherPost of Object.values(state)) { + // Remove any of its comments if (otherPost.root_id === post.id) { Reflect.deleteProperty(nextState, otherPost.id); } + + // a deleted post may exist in some other post's + // embeds when its link is mentioned in the post message. + // We need to remove the deleted post from post embeds of all posts + // to ensure the deleted post's contents cannot be retrieved from the store. + if (otherPost.metadata && otherPost.metadata.embeds && otherPost.metadata.embeds.length > 0) { + // This will become the post's new embeds array. + // We'll add everything other than the deleted post's embed here. + const newEmbeds: PostEmbed[] = []; + + for (const embed of otherPost.metadata.embeds) { + if (embed.type === 'permalink' && (embed.data as PostPreviewMetadata).post_id === post.id) { + // skip if the embed is the deleted post + continue; + } + + // include everything else + newEmbeds.push(embed); + } + + // if newEmbeds changed, update post's embeds + if (newEmbeds.length !== otherPost.metadata.embeds.length) { + // Since otherPost refers to the post from store, its frozen un immutable. + // That's why cloning it and modifying required parts here. + nextState[otherPost.id] = { + ...nextState[otherPost.id], + metadata: { + ...nextState[otherPost.id].metadata, + embeds: newEmbeds, + }, + }; + } + } } return nextState;