Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MM-37606 Queue sending message after file uploading finish #23382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+919 −299
Open

MM-37606 Queue sending message after file uploading finish #23382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 50 additions & 3 deletions webapp/channels/src/actions/file_actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,19 @@

import {batchActions} from 'redux-batched-actions';

import {FileInfo} from '@mattermost/types/files';
import {FileInfo, FilePreviewInfo, FileUploadResponse} from '@mattermost/types/files';
import {ServerError} from '@mattermost/types/errors';

import {FileTypes} from 'mattermost-redux/action_types';
import {getLogErrorAction} from 'mattermost-redux/actions/errors';
import {forceLogoutIfNecessary} from 'mattermost-redux/actions/helpers';
import {DispatchFunc, GetStateFunc} from 'mattermost-redux/types/actions';
import {Client4} from 'mattermost-redux/client';
import {FilePreviewInfo} from 'components/file_preview/file_preview';

import {localizeMessage} from 'utils/utils';

import {postRemoved, storePendingPostByClientId, storePost} from 'mattermost-redux/actions/posts';

export interface UploadFile {
file: File;
name: string;
Expand All @@ -23,7 +24,7 @@ export interface UploadFile {
channelId: string;
clientId: string;
onProgress: (filePreviewInfo: FilePreviewInfo) => void;
onSuccess: (data: any, channelId: string, rootId: string) => void;
onSuccess: (data: FileUploadResponse, channelId: string, rootId: string) => void;
onError: (err: string | ServerError, clientId: string, channelId: string, rootId: string) => void;
}

Expand Down Expand Up @@ -59,6 +60,10 @@ export function uploadFile({file, name, type, rootId, channelId, clientId, onPro
percent,
type,
} as FilePreviewInfo;
dispatch({
type: FileTypes.UPDATE_FILE_UPLOAD_PROGRESS,
data: filePreviewInfo,
});
onProgress(filePreviewInfo);
};
}
Expand Down Expand Up @@ -86,6 +91,8 @@ export function uploadFile({file, name, type, rootId, channelId, clientId, onPro
},
]));

dispatch(storePendingPostByClientId(clientId));

onSuccess(response, channelId, rootId);
} else if (xhr.status >= 400 && xhr.readyState === 4) {
const errorResponse = JSON.parse(xhr.response);
Expand Down Expand Up @@ -141,6 +148,46 @@ export function uploadFile({file, name, type, rootId, channelId, clientId, onPro

xhr.send(formData);

dispatch({
type: FileTypes.START_UPLOADING_FILE,
clientId,
data: xhr,
});

return xhr;
};
}

export function cancelUploadingFile(clientId: string) {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
const state = getState();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind inlining these calls to getState instead of saving it as a variable? It shouldn't matter here too much, but because dispatching an action changes the state, state is out of date by the time we use it again on line 180, so I think it's a good practice to not reuse the old version of state there

const pendingPostEntry = Object.entries(state.entities.files.filePreviews).find((entry) => entry[1].some((filePreview) => filePreview.clientId === clientId));
if (!pendingPostEntry) {
return;
}
const pendingPostId = pendingPostEntry[0];

dispatch({
type: FileTypes.UPLOAD_FILES_CANCEL,
clientId,
});

// If this is the last uploading file, immediately send the message or delete it.
if (pendingPostEntry[1].length !== 1) {
return;
}

const pendingPost = Object.values(state.entities.posts.posts).find((post) => post.pending_post_id === pendingPostId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just use the getPost selector here? I'm pretty sure pending posts have their ID set to their pending post ID before they've been created

if (!pendingPost) {
return;
}

if (pendingPost.message.length === 0) {
// If this post doesn't have message, then delete it.
await dispatch(postRemoved(pendingPost));
} else {
// otherwise send it.
await dispatch(storePost(pendingPost, []));
}
};
}
17 changes: 11 additions & 6 deletions webapp/channels/src/actions/post_actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// See LICENSE.txt for license information.

import {Post} from '@mattermost/types/posts';
import {FileInfo} from '@mattermost/types/files';
import {FileInfo, FilePreviewInfo} from '@mattermost/types/files';

import {GlobalState} from 'types/store';
import {ChannelTypes, SearchTypes} from 'mattermost-redux/action_types';
Expand Down Expand Up @@ -344,6 +344,8 @@ describe('Actions.Posts', () => {
const newReply = {id: 'reply_post_id', channel_id: 'current_channel_id', message: 'new message', root_id: 'new_post_id'} as Post;
const files: FileInfo[] = [];

const filePreviews: FilePreviewInfo[] = [];

const immediateExpectedState = [{
args: [newPost, files],
type: 'MOCK_CREATE_POST_IMMEDIATELY',
Expand All @@ -358,28 +360,29 @@ describe('Actions.Posts', () => {
const finalExpectedState = [
...immediateExpectedState,
{
args: [newReply, files],
args: [newReply, files, filePreviews],
type: 'MOCK_CREATE_POST',
}, {
args: ['comment_draft_new_post_id', null],
type: 'MOCK_SET_GLOBAL_ITEM',
},
];

await testStore.dispatch(Actions.createPost(newReply, files));
await testStore.dispatch(Actions.createPost(newReply, files, filePreviews));
expect(testStore.getActions()).toEqual(finalExpectedState);
});

test('with single shorthand emoji', async () => {
const testStore = mockStore(initialState);
const newPost = {id: 'new_post_id', channel_id: 'current_channel_id', message: 'new message :+1:'} as Post;
const files: FileInfo[] = [];
const filePreviews: FilePreviewInfo[] = [];

const immediateExpectedState = [{
args: ['+1'],
type: 'MOCK_ADD_RECENT_EMOJI',
}, {
args: [newPost, files],
args: [newPost, files, filePreviews],
type: 'MOCK_CREATE_POST',
}, {
args: ['draft_current_channel_id', null],
Expand All @@ -394,12 +397,13 @@ describe('Actions.Posts', () => {
const testStore = mockStore(initialState);
const newPost = {id: 'new_post_id', channel_id: 'current_channel_id', message: 'new message :cake:'} as Post;
const files: FileInfo[] = [];
const filePreviews: FilePreviewInfo[] = [];

const immediateExpectedState = [{
args: ['cake'],
type: 'MOCK_ADD_RECENT_EMOJI',
}, {
args: [newPost, files],
args: [newPost, files, filePreviews],
type: 'MOCK_CREATE_POST',
}, {
args: ['draft_current_channel_id', null],
Expand All @@ -414,6 +418,7 @@ describe('Actions.Posts', () => {
const testStore = mockStore(initialState);
const newPost = {id: 'new_post_id', channel_id: 'current_channel_id', message: 'new message :cake: :+1:'} as Post;
const files: FileInfo[] = [];
const filePreviews: FilePreviewInfo[] = [];

const immediateExpectedState = [{
args: ['cake'],
Expand All @@ -422,7 +427,7 @@ describe('Actions.Posts', () => {
args: ['+1'],
type: 'MOCK_ADD_RECENT_EMOJI',
}, {
args: [newPost, files],
args: [newPost, files, filePreviews],
type: 'MOCK_CREATE_POST',
}, {
args: ['draft_current_channel_id', null],
Expand Down
14 changes: 8 additions & 6 deletions webapp/channels/src/actions/post_actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import {Post} from '@mattermost/types/posts';
import {GroupChannel} from '@mattermost/types/groups';
import {FileInfo} from '@mattermost/types/files';
import {FileInfo, FilePreviewInfo} from '@mattermost/types/files';

import {SearchTypes} from 'mattermost-redux/action_types';
import {getMyChannelMember} from 'mattermost-redux/actions/channels';
Expand All @@ -14,7 +14,7 @@ import * as PostActions from 'mattermost-redux/actions/posts';
import * as PostSelectors from 'mattermost-redux/selectors/entities/posts';
import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users';
import {getCurrentTeamId} from 'mattermost-redux/selectors/entities/teams';
import {canEditPost, comparePosts} from 'mattermost-redux/utils/post_utils';
import {canEditPost, comparePosts, isPostDangling} from 'mattermost-redux/utils/post_utils';
import {DispatchFunc, GetStateFunc} from 'mattermost-redux/types/actions';

import {addRecentEmoji} from 'actions/emoji_actions';
Expand Down Expand Up @@ -97,8 +97,8 @@ export function unflagPost(postId: string) {
};
}

export function createPost(post: Post, files: FileInfo[]) {
return async (dispatch: DispatchFunc) => {
export function createPost(post: Post, files: FileInfo[], filePreviews: FilePreviewInfo[] = []) {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
// parse message and emit emoji event
const emojis = matchEmoticons(post.message);
if (emojis) {
Expand All @@ -112,11 +112,13 @@ export function createPost(post: Post, files: FileInfo[]) {
if (UserAgent.isIosClassic()) {
result = await dispatch(PostActions.createPostImmediately(post, files));
} else {
result = await dispatch(PostActions.createPost(post, files));
result = await dispatch(PostActions.createPost(post, files, filePreviews));
}

if (post.root_id) {
dispatch(storeCommentDraft(post.root_id, null));
if (!isPostDangling(getState(), post)) {
dispatch(storeCommentDraft(post.root_id, null));
}
} else {
dispatch(storeDraft(post.channel_id, null));
}
Expand Down
10 changes: 5 additions & 5 deletions webapp/channels/src/actions/views/create_comment.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ describe('rhs view actions', () => {
value: {
message: '',
fileInfos: [],
uploadsInProgress: [],
uploadsProgressPercent: {},
channelId,
rootId,
},
Expand Down Expand Up @@ -183,9 +183,9 @@ describe('rhs view actions', () => {
// make sure callback is a function which clears uploadsInProgress
expect(typeof callback).toBe('function');

const draft = {message: 'test msg', channelId, rootId, uploadsInProgress: [3, 4], fileInfos: [{id: 1}, {id: 2}]};
const draft = {message: 'test msg', channelId, rootId, uploadsProgressPercent: {3: undefined, 4: undefined}, fileInfos: [{id: 1}, {id: 2}]};

expect(callback(null, draft)).toEqual({...draft, uploadsInProgress: []});
expect(callback(null, draft)).toEqual({...draft, uploadsProgressPercent: {}});

const testStore = mockStore(initialState);

Expand Down Expand Up @@ -249,7 +249,7 @@ describe('rhs view actions', () => {

const testStore = mockStore(initialState);

testStore.dispatch(updateCommentDraft(rootId, {message: 'test message', channelId, rootId, fileInfos: [], uploadsInProgress: []}));
testStore.dispatch(updateCommentDraft(rootId, {message: 'test message', channelId, rootId, fileInfos: [], uploadsProgressPercent: {}}));

expect(store.getActions()).toEqual(
expect.arrayContaining(testStore.getActions()),
Expand All @@ -260,7 +260,7 @@ describe('rhs view actions', () => {
});

describe('submitPost', () => {
const draft = {message: '', channelId, rootId, fileInfos: []};
const draft = {message: '', channelId, rootId, fileInfos: [], uploadsProgressPercent: {}};

const post = {
file_ids: [],
Expand Down
11 changes: 8 additions & 3 deletions webapp/channels/src/actions/views/create_comment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,15 @@ import {Constants, StoragePrefixes} from 'utils/constants';
import type {PostDraft} from 'types/store/draft';
import type {GlobalState} from 'types/store';
import type {DispatchFunc, GetStateFunc} from 'mattermost-redux/types/actions';
import {FilePreviewInfo} from '@mattermost/types/files';

export function clearCommentDraftUploads() {
return actionOnGlobalItemsWithPrefix(StoragePrefixes.COMMENT_DRAFT, (_key: string, draft: PostDraft) => {
if (!draft || !draft.uploadsInProgress || draft.uploadsInProgress.length === 0) {
if (!draft || !draft.uploadsProgressPercent || Object.keys(draft.uploadsProgressPercent).length === 0) {
return draft;
}

return {...draft, uploadsInProgress: []};
return {...draft, uploadsProgressPercent: {}};
});
}

Expand Down Expand Up @@ -102,7 +103,11 @@ export function submitPost(channelId: string, rootId: string, draft: PostDraft)

post = hookResult.data;

return dispatch(PostActions.createPost(post, draft.fileInfos));
return dispatch(PostActions.createPost(
post,
draft.fileInfos,
Object.values(draft.uploadsProgressPercent).filter((filePreviewInfo): filePreviewInfo is FilePreviewInfo => filePreviewInfo !== undefined),
));
};
}

Expand Down
2 changes: 1 addition & 1 deletion webapp/channels/src/actions/views/drafts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export function transformServerDraft(draft: ServerDraft): Draft {
message: draft.message,
fileInfos,
props: draft.props,
uploadsInProgress: [],
uploadsProgressPercent: {},
channelId: draft.channel_id,
rootId: draft.root_id,
createAt: draft.create_at,
Expand Down