Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Fixed a bug where user status is not displayed on first load of a product #9288

Closed
wants to merge 8 commits into from

Conversation

harshilsharma63
Copy link
Member

Summary

Currently, when you open Boards or Playbooks from Channels, your status is always displayed as offline in the global header.
This PR fixes that.

Ticket Link

mattermost/focalboard#1481

Related Pull Requests

None

Release Note

Fixes a bug where the user's status was always as offline on opening a product from Channels.

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

How is the user's status currently loaded by the web app when viewing Channels? Are we loading it explicitly, or is it just being loaded by something else? While this is needed when viewing Boards or Playbooks, I want to make sure we're not duplicating requests made on load in Channels since we already make too many requests on load there

actions/user_actions.jsx Outdated Show resolved Hide resolved
components/status_dropdown/status_dropdown.tsx Outdated Show resolved Hide resolved
@hmhealey
Copy link
Member

hmhealey commented Nov 1, 2021

Funnily enough, I just found the code that loads the status currently, and it's kind of out of the way since it's loaded by the ResetStatusModal calling the autoResetStatus action defined just below the new one you added.

Instead of doing that since it's rather out of the way, we should add status loading to getMeAndConfig that's called before the app renders to begin with. We could remove the extra call to load the status from autoResetStatus then to avoid loading the current user's status twice when viewing Channels then.

@harshilsharma63
Copy link
Member Author

Funnily enough, I just found the code that loads the status currently, and it's kind of out of the way since it's loaded by the ResetStatusModal calling the autoResetStatus action defined just below the new one you added.

Instead of doing that since it's rather out of the way, we should add status loading to getMeAndConfig that's called before the app renders to begin with. We could remove the extra call to load the status from autoResetStatus then to avoid loading the current user's status twice when viewing Channels then.

Done.

@@ -249,6 +249,7 @@ export function loadMe(): ActionFunc {
const user = getState().entities.users.profiles[currentUserId];
if (currentUserId) {
Client4.setUserId(currentUserId);
await dispatch(getStatus(currentUserId));
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to wait for this to complete? Since we aren't able to fire off this request while we're loading everything else, perhaps we could just dispatch it without awaiting on it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do need to wait for this. This is because the ResetStatusModal component fetches the status only on the component mount (and not updated) and if the status is not found in the store, sets it to offline. Not awaiting here causes the status to always be displayed as offline.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can do that since it'll make the first load longer. I think we either need to load the status asynchronously while the other stuff loads (like in the Promise.all above) or we could make the modal handle the unloaded state properly

packages/mattermost-redux/src/store/initial_state.ts Outdated Show resolved Hide resolved
@hmhealey hmhealey added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Nov 15, 2021
@chenilim
Copy link
Member

@harshilsharma63, what's the next step on this PR? Is there anything that can be done in the short term (e.g. refresh status after 200ms in Boards), if there are blocking issues on the full fix?

@angeloskyratzakos angeloskyratzakos removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 5, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@hmhealey
Copy link
Member

Removing my review request on this to clear out some notifications. Let me know when this is ready for my review again

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants