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

sk409 · 2023-05-13T06:30:53Z

Summary

Queue sending message when files are being uploaded so that users can send next message.

Migrated from: mattermost/mattermost-webapp#11370

Ticket Link

Fixes #21264
Jira: https://mattermost.atlassian.net/browse/MM-37606

Related Pull Requests

No related PRs.

Screenshots

Release Note

Message can be uploaded when files are being uploaded.

mattermost-build · 2023-05-13T06:31:03Z

Hello @sk409,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

mattermost-build · 2023-05-13T12:56:26Z

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

hmhealey

Thanks again for resubmitting that PR! I continue to appreciate that you're putting in all the work to deal with out moving stuff around here.

My review of this is still taking some time since I want to be extra thorough because this is a very important part of the app, so while I still have some stuff I wanted to check out, I wanted to submit a partial review of the code and avoid being completely silent on it. I'm hoping to have some more time later this week to continue on my review, but I'll try to be responsive on my current comments in the mean time

hmhealey · 2023-05-23T19:25:21Z

webapp/channels/src/actions/file_actions.ts

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


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

hmhealey · 2023-05-23T19:26:50Z

webapp/channels/src/actions/file_actions.ts

+            return;
+        }
+
+        const pendingPost = Object.values(state.entities.posts.posts).find((post) => post.pending_post_id === pendingPostId);


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

hmhealey · 2023-05-23T19:38:50Z

webapp/channels/src/packages/mattermost-redux/src/selectors/entities/files.ts

+        return [];
+    }
+    return state.entities.files.filePreviews[pendingPostId] ?? [];


This selector isn't memoized, so we can't return new array literals from it. Either we can turn it into a selector factory which we could memoize with createSelector or I'd suggest having this return undefined and making the components that use it be able to handle the undefined value using defaultProps or something similar

hmhealey · 2023-05-23T20:09:43Z

webapp/channels/src/packages/mattermost-redux/src/utils/post_utils.ts

+    return post.id === post.pending_post_id && post.file_client_ids !== undefined && post.file_client_ids.length !== 0;
+}
+
+export function isPostDangling(state: GlobalState, post: Post) {


If I'm reading this right, is a "dangling post" another name for being a reply to a pending post? If so, is that needed for this ticket, or can we remove that part of the PR?

Queuing posts with ongoing file uploads is already pretty complicated, and being able to reply to a pending post feels significantly more complex than that. I don't think Thread objects have any logic yet to support being in a pending state similar to posts

@hmhealey

If I'm reading this right, is a "dangling post" another name for being a reply to a pending post?

Yes, that's right.

If so, is that needed for this ticket, or can we remove that part of the PR?

I implemented this to fix the following issue.
mattermost/mattermost-webapp#11370 (comment)

I think we need this feature since an alert appears when a user replies to a pending post.
Can we ignore such a case?

Ah, I see. I was under the impression that we didn't allow you to open the RHS to a pending post, but maybe that's not actually the case since, unless there's a lot of lag, it's hard to open the RHS to reply to a post before it's created currently. It'll definitely be easier now if we hold the post while we wait for attachments to upload.

Perhaps, we can either prevent the user from opening a pending post in the RHS or just replying to it? It's not ideal UX, but I feel like it'd be much easier to prevent that case from being possible. That way, we can also focus this effort on just queuing posts with pending file attachments without worrying about solving the existing edge cases caused by being able to open pending posts in the RHS.

I'll see if anyone else here has any ideas, but how would that sound to you?

hmhealey · 2023-05-23T20:16:44Z

webapp/channels/src/packages/mattermost-redux/src/utils/post_utils.ts

    const aIsPendingOrFailed = isPostPendingOrFailed(a);
    const bIsPendingOrFailed = isPostPendingOrFailed(b);
-    if (aIsPendingOrFailed && !bIsPendingOrFailed) {
+    if (!aIsUploadingFile && aIsPendingOrFailed && !bIsPendingOrFailed) {


Is this supposed to make queued posts with uploads always appear after regular pending posts in the channel? If so, would you mind separating this case into a separate if-else block? I usually like doing that because it makes it easy to see the priority of different fields when you're comparing two items

@hmhealey
I added this condition to sort posts with uploads by their create_at.
Should I separate if-else block in this case?

Yeah, I'm thinking of just breaking that up so that it's like

if (aIsUploadingFile && !bIsUploadingFile) { // I might have gotten these cases backwards to what you were intending return -1; } else if (!aIsUploadingFile && bIsUploadingFile) { return 1; } if (aIsPendingOrFailed && !bIsPendingOrFailed) { return -1; } else if (!aIsPendingOrFailed && bIsPendingOrFailed) { return 1; } if (a.create_at > b.create_at) { return -1; } else if (a.create_at < b.create_at) { return 1; } return 0;

I find that pattern just makes it a bit easier to read since it spreads out the different cases pretty clearly

hmhealey · 2023-05-23T20:59:24Z

webapp/channels/src/packages/mattermost-redux/src/reducers/entities/files.ts

+        const entries = Object.entries(nextState);
+        for (let entryIndex = 0; entryIndex < entries.length; entryIndex++) {
+            const entry = entries[entryIndex];


I think you might be able to rewrite this as a for-of loop and use some destructuring to make it a bit easier to follow since it's not particularly clear what entry is. I'm thinking something like

for (const [id, uploadsInProgress] of Object.entries(nextState)) { // ... }

hmhealey · 2023-05-23T21:04:06Z

webapp/channels/src/packages/mattermost-redux/src/reducers/entities/files.ts

+        }
+        return {
+            ...state,
+            [entry[0]]: entry[1].filter((f) => f.clientId !== action.clientId),


A bit more nitpicking here, but would you mind redeclaring entry[0] and entry[1] as something like id and uploadsInProgress to make it more obvious what's going on here?

hmhealey · 2023-05-23T21:04:16Z

webapp/channels/src/packages/mattermost-redux/src/reducers/entities/files.ts

+    }
+}
+
+function fileUploadRequests(state: Record<string, XMLHttpRequest> = {}, action: GenericAction) {


Is this still needed? I can't find where it's being used

hmhealey · 2023-05-23T21:11:59Z

webapp/channels/src/packages/mattermost-redux/src/reducers/entities/files.ts

+        return nextState;
+    }
+
+    case FileTypes.REMOVE_FILE_PREVIEWS: {


Instead of needing a separate action for this, could we actually listen for a RECEIVED_NEW_POST here? If that post's pending_post_id is set, we can see if that ID is a key in state and then, if that's found, remove it

hmhealey · 2023-05-23T21:18:10Z

webapp/channels/src/packages/mattermost-redux/src/action_types/files.ts

+    START_UPLOADING_FILE: null,
+    UPDATE_FILE_UPLOAD_PROGRESS: null,
+    REMOVE_FILE_PREVIEWS: null,


More nitpicking, but would you mind renaming these to be written more like STARTED_FILE_UPLOAD or CANCELED_FILE_UPLOAD to be a bit more consistent with all of our RECEIVED_* actions?

mm-cloud-bot · 2023-05-24T12:50:50Z

Creating a new SpinWick test server using Mattermost Cloud.

mm-cloud-bot · 2023-05-24T13:20:52Z

Enterprise Edition Image not available in the 30 minutes timeframe, checking the Team Edition Image and if available will use that.

yasserfaraazkhan · 2023-05-24T14:50:39Z

hi @sk409 , can you help in resolving conflicts on this PR ?

sk409 · 2023-05-28T11:58:57Z

@hmhealey

My review of this is still taking some time since...

I've got it.

mattermost-build · 2023-06-09T01:01:31Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

larkox · 2023-08-08T11:48:50Z

@sk409 Are you still working on this? Could you resolve the merge conflicts and the comments?

sk409 · 2023-08-09T17:12:46Z

@larkox
I'm still working on this issue, but haven't had much time lately.
I'm thinking of completing it by the end of this month, but if it seems like it will take longer than that, please unassign me.

larkox · 2023-08-10T08:43:58Z

@sk409 no rush at all. Take your time. Just checking in case I had to mark it again as up for grabs 😄

mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 13, 2023

mattermost-build added the Contributor label May 13, 2023

sk409 mentioned this pull request May 13, 2023

MM-37606: Queue sending message after file upload finishes mattermost/mattermost-webapp#11370

Closed

M-ZubairAhmed requested a review from hmhealey May 13, 2023 12:56

M-ZubairAhmed added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 13, 2023

M-ZubairAhmed requested review from larkox and calebroseland May 13, 2023 12:59

MM-37606 Queue sending message after file uploading finish

f60f2ea

sk409 force-pushed the MM-37606_Queue-sending-message-after-file-upload-finish branch from f1967d9 to f60f2ea Compare May 21, 2023 06:26

hmhealey requested changes May 23, 2023

View reviewed changes

marianunez added the Setup Cloud Test Server Setup an on-prem test server label May 24, 2023

marianunez requested a review from yasserfaraazkhan May 24, 2023 12:50

mattermost-build added the Lifecycle/1:stale label Jun 9, 2023

calebroseland removed their request for review July 11, 2023 16:15

yasserfaraazkhan removed their request for review July 25, 2023 07:16

yasserfaraazkhan removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sk409 commented May 13, 2023

mattermost-build commented May 13, 2023

mattermost-build commented May 13, 2023

hmhealey left a comment

hmhealey May 23, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

sk409 May 28, 2023

hmhealey May 29, 2023 •

edited

hmhealey May 23, 2023

sk409 May 28, 2023

hmhealey May 29, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

hmhealey May 23, 2023

mm-cloud-bot commented May 24, 2023

mm-cloud-bot commented May 24, 2023

yasserfaraazkhan commented May 24, 2023

sk409 commented May 28, 2023 •

edited

mattermost-build commented Jun 9, 2023

larkox commented Aug 8, 2023

sk409 commented Aug 9, 2023

larkox commented Aug 10, 2023

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

Are you sure you want to change the base?

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

Conversation

sk409 commented May 13, 2023

Summary

Ticket Link

Related Pull Requests

Screenshots

Release Note

mattermost-build commented May 13, 2023

mattermost-build commented May 13, 2023

hmhealey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmhealey May 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm-cloud-bot commented May 24, 2023

mm-cloud-bot commented May 24, 2023

yasserfaraazkhan commented May 24, 2023

sk409 commented May 28, 2023 • edited

mattermost-build commented Jun 9, 2023

larkox commented Aug 8, 2023

sk409 commented Aug 9, 2023

larkox commented Aug 10, 2023

hmhealey May 29, 2023 •

edited

sk409 commented May 28, 2023 •

edited