-
Notifications
You must be signed in to change notification settings - Fork 7k
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.
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
[MM-56904] Reduce the number of api requests made to fetch user information for GMs on page load #27149
base: master
Are you sure you want to change the base?
Conversation
@hmhealey @ZubairImtiaz3 Let me know if you would like someone from the xyz team to review the server code |
Yes @BenCookie95 that would be good to have someone from XYZ team to have a look at server code |
server/channels/jobs/delete_dms_preferences_migration/delete_dms_preferences_migration.go
Outdated
Show resolved
Hide resolved
Great find by the way if I didn't say that previously. It's always nice to go to improve something and then find out that we already have an API for it 😀 |
} | ||
|
||
await queue.onEmpty(); | ||
if (channelUsersToLoad.length > 0) { | ||
await dispatch(UserActions.getProfilesInGroupChannels(channelUsersToLoad)); |
There was a problem hiding this comment.
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 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we need to ensure the users exist in the redux store before we call loadCustomEmojisForCustomStatusesByUserIds
. That custom emoji's function checks the custom status for all those user IDs
@agnivade just looking for a set of eyes over the job I added |
AND (SUBSTRING( | ||
CONCAT('000000000000000', value), | ||
LENGTH(value) + 1, | ||
15 | ||
) > '000000000000040' | ||
OR SUBSTRING( | ||
CONCAT('000000000000000', value), | ||
LENGTH(value) + 1, | ||
15 | ||
) < '000000000000001' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very hard to read. And there are no comments, which isn't making things easier. And everything is hardcoded with no parameterization.
- I would suggest to use squirrel to build up the query. Use constants like
model.PreferenceCategorySidebarSettings
instead of hardcoded strings and pass them as parameters in the query. - Add some comments to explain what is being achieved here.
Summary
Make one api request to fetch user information for all visible GMs. This endpoint was added 5 years ago for the DMs modal but it's perfect for fetching our user information on page load too.
Previously we would make a request for each GM visible in your sidebar. This could be as many as 40 extra requests on page load depending on on your preference for
limit_visible_dms_gms
. The default value for this is 40.I also removed the option to set
limit_visible_dms_gms
to any value above 40. Previously we would allow a user to setall
for this which could prove troublesome for this new endpoint. I added a new job to clean up any preferences with a value greater than 40.Ticket Link
https://mattermost.atlassian.net/browse/MM-56904
Screenshots
Release Note