MM-24549 Fix for skipping user info calls for hidden GM's #5398
Conversation
|
/update-branch |
actions/user_actions.test.js
Outdated
|
|
||
| import * as UserActions from 'actions/user_actions'; | ||
|
|
||
| const mockStore = configureStore([thunk]); | ||
| const mockChannelsObj1 = [{id: 'gmChannel', type: 'G1'}]; |
There was a problem hiding this comment.
If these are supposed to be gm channels, shouldn't those types be using General.GM_CHANNEL? G1 and G2 aren't valid channel types.
There was a problem hiding this comment.
It was a mock so it wouldn't have mattered all that much. That wasn't great in the way I had to hack in the first place. I spent quite a lot of time trying to figure out how to pass out of scope variable to mock and there is no way to do that so, I had to resort to writing to variables and checking for that. Honestly, the whole test case is odd. I couldn't think of a better way to do it.
There was a problem hiding this comment.
I'm confused. We're mocking the constants? This test file is a mess, but I didn't know it was that bad.
| @@ -21,6 +26,19 @@ jest.mock('mattermost-redux/actions/users', () => { | |||
| }; | |||
| }); | |||
|
|
|||
| jest.mock('mattermost-redux/selectors/entities/channel_categories', () => { | |||
There was a problem hiding this comment.
In particular, why mock these selectors?
There was a problem hiding this comment.
If we are calling the selectors then isn't that a good enough test? Selectors are supposed to filter and they have tests to cover that.
There was a problem hiding this comment.
The main thing I'm concerned about is that the tests confirm that the code call the selectors, but it doesn't confirm that calling the selectors actually prevents the extra requests. In the end, they both cover the same thing assuming that the selectors work as expected, so while I personally think that testing for the bug would be better, doing it this way is probably fine as well, so 0/5 on whether or not we change it.
|
Just noting that I've seen this, but am removing myself from review as Devin and Harrison have this. Thanks @sudheerDev! |
There was a problem hiding this comment.
Thank you @sudheerDev
Unable to test on the PR server as system closed GMs are not present (GMs closed after 7 days with no messages)
QA will test on daily after merge.
|
Test server destroyed |
Summary
This PR fixes the issue with making unnecessary calls or members info in GM channels when hidden in LHS. This only skip calls if experimental sidebar settings are turned on
Ticket Link
https://mattermost.atlassian.net/browse/MM-24549