[MM-12743] Migrate loadProfilesAndTeamMembers of user actions into redux-style #2017

saturninoabril · 2018-11-07T14:24:26Z

Summary

This PR migrates the following user_actions into redux-style:

loadProfilesAndTeamMembers
loadProfilesAndTeamMembersAndChannelMembers
loadTeamMembersForProfilesList
loadTeamMembersAndChannelMembersForProfilesList
loadChannelMembersForProfilesList

and removed the following since it's not being used elsewhere:

loadChannelMembersForProfilesMap

Affected Area:

Viewing team members at "Main Menu > View Members"
Viewing system users at "System Console > Users"
And paging and searching to those pages/modal

@DHaussermann Above were tested and looks good.

Ticket Link

Jira ticket: MM-12743

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)

saturninoabril · 2018-11-07T14:29:49Z

tests/redux/actions/user_actions.test.js

+ actualActions = testStore.getActions();
+ expect(actualActions[0].args).toEqual(expectedActions[0].args);
+ expect(actualActions[0].type).toEqual(expectedActions[0].type);
+ });


Unfortunately, can't do much testing as I'm not able to spy on below codeblock. Maybe, it's something to think about on how to do it.

if (data) { loadTeamMembersForProfilesList(data, newTeamId, success); doDispatch(loadStatusesForProfilesList(data)); }

hmhealey · 2018-11-08T19:30:01Z

actions/user_actions.jsx

- const {data} = await UserActions.getProfilesInTeam(teamId, page, perPage)(dispatch, getState);
- loadTeamMembersForProfilesList(data, teamId, success);
- dispatch(loadStatusesForProfilesList(data));
+export function loadProfilesAndTeamMembers(page, perPage, teamId = getCurrentTeamId(getState()), success) {


Can you remove the default argument here? That's using the global state which is going to get confusing.

Oh, sorry about this, I thought I removed it.

hmhealey · 2018-11-08T19:31:39Z

actions/user_actions.jsx

@@ -40,10 +40,15 @@ export async function switchFromLdapToEmail(email, password, token, ldapPassword
 }
 }

-export async function loadProfilesAndTeamMembers(page, perPage, teamId = getCurrentTeamId(getState()), success) {


Can you remove the success callback and have callers use .then(success) instead?

Since we're passing it to loadTeamMembersForProfilesList and you're not changing that function right now, we can get around that for now by explicitly returning an explicit Promise instead of using an async function for the body of the action like

function doSomething() { return (dispatch, getState) => { return new Promise((resolve) => { loadTeamMembersForProfilesList(resolve); }); }; );

Thanks @hmhealey! I managed to remove success and error callbacks (although error callback is really not being used elsewhere). Could you please take a look again?

I'm not really surprised. We probably just added success and error for consistency with all the other actions

If it is working properly, I'm ok with it, but looks like this is not exactly the same behavior under the hood. Here, the promise is resolved when the getProfilesInTeam is success, in the previous implementation it the success call was made after the loadTeamMembersForProfilesList.

It's working properly after testing several cases, and yes, it's a bit different since it only dispatches loadTeamMembersForProfilesList and loadStatusesForProfilesList after success call in getProfilesInTeam which I believe makes sense.
The loadProfilesAndTeamMembers returns {data: true} which can be consumed when a callback is necessary.

saturninoabril · 2018-11-12T17:29:07Z

actions/user_actions.jsx

- }
-}
-
-export function loadChannelMembersForProfilesMap(profiles, channelId = getCurrentChannelId(getState()), success, error) {


loadChannelMembersForProfilesMap is not being used elsewhere.

jespino · 2018-11-16T11:09:42Z

actions/user_actions.jsx

+ const channelIdParam = channelId || getCurrentChannelId(state);
+ const {data} = await doDispatch(UserActions.getProfilesInChannel(channelIdParam, page, perPage));
+ if (data) {
+ doDispatch(loadTeamMembersForProfilesList(data, teamIdParam)).then(() => {


You can use await there (I think) instead of .then.

jespino · 2018-11-16T13:01:05Z

actions/user_actions.jsx


- loadTeamMembersForProfiles(list, teamId, success, error);
+ doDispatch(getTeamMembersByIds(teamIdParam, userIdsToLoad));


Same here, the .then is called before the getTeamMembersByIds returns, with the other implementation is called after it returns. It can be solved adding an await here.

You're right. I'll add it.

jespino · 2018-11-16T13:04:40Z

actions/user_actions.jsx

+ const state = doGetState();
+ const teamIdParam = teamId || getCurrentTeamId(state);
+ const channelIdParam = channelId || getCurrentChannelId(state);
+ doDispatch(loadTeamMembersForProfilesList(profiles, teamIdParam)).then(() => {


We can use await here, I think.

jespino · 2018-11-16T13:06:20Z

actions/user_actions.jsx

- success(result.data);
- }
- });
+ doDispatch(getChannelMembersByIds(channelIdParam, list));


If we don't await here, we are returning on .then before the getChannelMembersById action is run.

jespino · 2018-11-16T13:08:20Z

components/admin_console/system_users/system_users.jsx

@@ -116,7 +115,12 @@ export default class SystemUsers extends React.Component {
 } else if (teamId === SearchUserTeamFilter.NO_TEAM) {
 loadProfilesWithoutTeam(0, Constants.PROFILE_CHUNK_SIZE, this.loadComplete);
 } else {
- loadProfilesAndTeamMembers(0, Constants.PROFILE_CHUNK_SIZE, teamId, this.loadComplete);
+ this.props.actions.loadProfilesAndTeamMembers(0, Constants.PROFILE_CHUNK_SIZE, teamId).then(({data}) => {


If you going to use async you should use await here, if not, you can leave the function without async and handle here the promise. I prefer the former option.

Right, I totally missed that. I'll use the async/await.

jespino · 2018-11-16T13:09:49Z

components/admin_console/system_users/system_users.jsx

 // Paging isn't supported while searching

 if (this.props.teamId === SearchUserTeamFilter.ALL_USERS) {
 loadProfiles(page + 1, USERS_PER_PAGE, this.loadComplete);
 } else if (this.props.teamId === SearchUserTeamFilter.NO_TEAM) {
 loadProfilesWithoutTeam(page + 1, USERS_PER_PAGE, this.loadComplete);
 } else {
- loadProfilesAndTeamMembers(page + 1, USERS_PER_PAGE, this.props.teamId, this.loadComplete);
+ this.props.actions.loadProfilesAndTeamMembers(page + 1, USERS_PER_PAGE, this.props.teamId).then(({data}) => {


Maybe, if this not propage the async to other functions, makes sense to keep this function consistent with the loadDataForTeam function. I mean, set the function as async and use await instead of .then.

Sure, changing since I'm 0/5 with that and yours make sense 💯

jespino

In general looks good to me, but I see some inconsistencies and subtle changes of behavior in the async requests. Maybe I'm wrong, but I prefer to clarify it before we merge :)

saturninoabril · 2018-11-23T15:08:28Z

Thanks @jespino for your feedbacks, please take a look again.

saturninoabril added the 2: Dev Review Requires review by a core commiter label Nov 7, 2018

saturninoabril added this to the v5.6.0 milestone Nov 7, 2018

saturninoabril requested review from jespino and hmhealey November 7, 2018 14:24

saturninoabril commented Nov 7, 2018

View reviewed changes

hmhealey suggested changes Nov 8, 2018

View reviewed changes

saturninoabril commented Nov 12, 2018

View reviewed changes

hmhealey approved these changes Nov 13, 2018

View reviewed changes

jespino reviewed Nov 16, 2018

View reviewed changes

jespino suggested changes Nov 16, 2018

View reviewed changes

saturninoabril added 3 commits November 23, 2018 22:35

migrate loadProfilesAndTeamMembers of user actions into redux-style

0664167

updated per comment and also migrated other user_actions

591ebc3

update per comment

078704e

jespino approved these changes Nov 23, 2018

View reviewed changes

saturninoabril merged commit b3edb1f into mattermost:master Nov 23, 2018

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Nov 23, 2018

saturninoabril deleted the MM-12743-loadProfilesAndTeamMembers branch November 24, 2018 00:46

lindy65 added Tests/Done Release tests have been written and removed 2: Dev Review Requires review by a core commiter labels Dec 4, 2018

JtheBAB mentioned this pull request Jan 15, 2019

rebase (#1) #2272

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-12743] Migrate loadProfilesAndTeamMembers of user actions into redux-style #2017

[MM-12743] Migrate loadProfilesAndTeamMembers of user actions into redux-style #2017

saturninoabril commented Nov 7, 2018 •

edited

Loading

saturninoabril Nov 7, 2018

hmhealey Nov 8, 2018

saturninoabril Nov 12, 2018

hmhealey Nov 8, 2018

hmhealey Nov 8, 2018

saturninoabril Nov 12, 2018

hmhealey Nov 13, 2018

jespino Nov 16, 2018

saturninoabril Nov 23, 2018 •

edited

Loading

saturninoabril Nov 12, 2018

jespino Nov 16, 2018

jespino Nov 16, 2018

saturninoabril Nov 23, 2018

jespino Nov 16, 2018

jespino Nov 16, 2018

jespino Nov 16, 2018

saturninoabril Nov 23, 2018

jespino Nov 16, 2018

saturninoabril Nov 23, 2018

jespino left a comment

saturninoabril commented Nov 23, 2018


		loadTeamMembersForProfiles(list, teamId, success, error);
		doDispatch(getTeamMembersByIds(teamIdParam, userIdsToLoad));

[MM-12743] Migrate loadProfilesAndTeamMembers of user actions into redux-style #2017

[MM-12743] Migrate loadProfilesAndTeamMembers of user actions into redux-style #2017

Conversation

saturninoabril commented Nov 7, 2018 • edited Loading

Summary

Ticket Link

Checklist

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

saturninoabril Nov 23, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jespino left a comment

Choose a reason for hiding this comment

saturninoabril commented Nov 23, 2018

saturninoabril commented Nov 7, 2018 •

edited

Loading

saturninoabril Nov 23, 2018 •

edited

Loading