Mattermost-server: [Help Wanted] [MM-12738] Migrate OAuth app actions from user_actions.jsx and admin_actions.jsx to redux

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Part of [https://mattermost.atlassian.net/browse/MM-12581]

• getAuthorizedApps
• allowOAuth2
• deauthorizeOauthApp

---

Since switching to redux we've been moving all the component actions into the redux stores. There are still a bunch of older actions that need to be migrated. The key change that needs to be made is getting rid of functions (primarily located in the actions directory of the web app code) are directly accessing and dispatching redux actions using the global store instance. Often these functions will be imported by a component and used directly.

If needed convert the action to be a redux action, take a look if the action already exists in mattermost-redux and prefer the use of the library one unless you need to combine it with other actions into one view action.

Then update all the components that makes use of the action and finally add/update/delete the appropriate unit tests

_______

Example: you may have a component and activity such as

import * as FooActions from 'mattermost-redux/actions/foo';
import store from 'stores/redux_store';
export function doSomething() { 
  return store.dispatch(FooActions.doSomething()); 
}

// components/my_button.js
import React from 'react';
import {doSomething} from 'actions/foo_actions';
export default class MyButton extends React.PureComponent {
  render() {
      return <button onClick={doSomething}>'Click me!'}</button>
  }
}

We should never be importing the global instance of the store directly. Instead, its dispatch method should be accessed through a mapDispatchToProps function passed to react-redux's connect function. Generally, this will also remove the need to have a file like actions/foo_actions.js or at least remove a significant amount of code from it. In this example, the correct code will look like

// components/index.js
import {connect} from 'react-redux';
import {bindActionCreators} from 'redux';
import {doSomething} from 'mattermost-redux/actions/foo';
import MyButton from './my_button';
function mapDispatchToProps(dispatch) {
    return {
        actions: bindActionCreators({ doSomething, }),
    }
}

export default connect(null, mapDispatchToProps)(MyButton);

// components/my_button.js
import PropTypes from 'prop-types';
import React from 'react';
export default class MyButton extends React.PureComponent {
  static propTypes = {
      actions: PropTypes.shape({ doSomething: PropTypes.func, }),
  }
  render() {
      return <button onClick={this.props.actions.doSomething}>{'Click me!'}</button>;
  }
}

[MirlanMaksv]

Let me help with this issue

[hanzei]

Great, thanks @MirlanMaksv!

[MirlanMaksv]

If I understand correctly, above listed functions are just wrappers for making requests, the only problem I see is that they are used directly by components. So I need a clarification here, do we need to pass wrapper functions or bound Client4 methods to components as props?

[hanzei]

@sudheerDev Can you help with the question please? ^

[sudheerDev]

@MirlanMaksv Hey, I updated the code formatting in the description above to make it more readable. Have another look at it to see if it helps.

So I need a clarification here, do we need to pass wrapper functions or bound Client4

You need to pass wrapper functions to the component through props. if you look at getAuthorizedApps function we have a getState().entities.users.currentUserId where we are using global getState and we want to remove these global instances

so it would be something like

export function loadChannelMembersForProfilesList(profiles, channelId) {
    return async (doDispatch, doGetState) => {
       // use doGetState().entities.users.currentUserId
   }
}

Don't hesitate to mention me if you have any questions going forward.

[MirlanMaksv]

Thank you @sudheerDev, one more clarification do we pass them as action props, I mean like this

// components/user_settings/security/index.js
function mapDispatchToProps(dispatch) {
    return {
        actions: bindActionCreators({
            getMe,
            deauthorizeOAuthApp,
            getAuthorizedApps
        }, dispatch),
    };
}

?
But for me they do not seem to be actions, as they are not dispatching anything

[sudheerDev]

@mikroskeem Yes, they should be like action props.

But for me they do not seem to be actions, as they are not dispatching anything.

Thats true. We should be adding these actions to mattermost-redux similar to

https://github.com/mattermost/mattermost-redux/blob/a8d240b0d464d1e467a04bb9bacac710342e9c8a/src/actions/users.js#L57-L64 which uses https://github.com/mattermost/mattermost-redux/blob/a8d240b0d464d1e467a04bb9bacac710342e9c8a/src/actions/helpers.js#L66

so the dispatch we pass though the action prop is used in the helper function.

We should not be calling the client4 func directly as we don't have necessary checks such as logout if needed when user is not authorised. That is something we should be changing here with the other changes.

FYI: You can reach me on community instance with the same handle @sudheerDev in case you have something to run by

[mikroskeem]

Autocompletion is nice, but you still have to be careful ;)

---- On Thu, 14 Feb 2019 08:28:27 +0000 [email protected] wrote ----

@mikroskeem Yes, they should be like action props.

But for me they do not seem to be actions, as they are not dispatching anything.

Thats true. We should be adding these actions to mattermost-redux similar to

https://github.com/mattermost/mattermost-redux/blob/a8d240b0d464d1e467a04bb9bacac710342e9c8a/src/actions/users.js#L57-L64 which uses https://github.com/mattermost/mattermost-redux/blob/a8d240b0d464d1e467a04bb9bacac710342e9c8a/src/actions/helpers.js#L66

so the dispatch we pass though the action prop is used in the helper function.

We should not be calling the client4 func directly as we don't have necessary checks such as logout if needed when user is not authorised. That is something we should be changing here with the other changes.

FYI: You can reach me on community instance with the same handle @sudheerDev in case you have something to run by

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[enahum]

@sudheerDev why should this be added to mattermost-redux, it seems to me that they should be part of the webapp as is very client specific. Another thing is that it should use the reducers to store the data, the submitted PR is doing nothing with that data https://github.com/mattermost/mattermost-redux/pull/776

[sudheerDev]

@enahum It is an API call at the end of the day and so, It is not client specific. We should have all API call actions in mattermost-redux as it can be used by any client or plugin.

To give an example of why we should have it in redux

If we do end up eventually having this feature in mobile for feature parity we would need these and we then would be migrating this logic.

Another thing is that it should use the reducers to store the data

I don't see a reason for that It just adds complexity to the state and logic in the component as the data can be stale(These events wont be updated by socket actions) and IMHO not worth the effort in fixing the UX around unsynced data or old data being populated from the store.

[enahum]

@sudheerDev I agree that your logic makes sense and that we should have one source of truth and that all API call actions should be in redux. Now if you only need an API call that's why the Client exists, it does not require an action. YES the reality is that doing it this way it will work but it does not mean that it should be done this way, to me is definitely a misuse of redux.

The definition of an action according to the redux documentation

Actions are payloads of information that send data from your application to your store. They are the only source of information for the store. You send them to the store using store.dispatch().

You can also read the definition on the advanced guide, but what it has in common is the alteration of the store state.

An example of a pure action.

function userLoggedIn() {
  return {
    type: USER_LOGGED_IN
  };
}

The reality is that we use thunk to make actions that require some async calls before they dispatch data to the reducers and that is why we use them that way, is not just an API call.

Happy to discuss this more, IMHO this PR is using the action in the wrong way. Not because is convenient means that is the way to do it.

[hmhealey]

While I agree that we might be abusing thunk since this has no effect on the store, I think it makes sense to keep these calls as actions because:

1. It makes it so that these calls can be connected to components the same way as other API-related actions, and if we make it affect the redux state in the future, it'll be trivial to just modify the action to use dispatch without having to change everywhere that it's called.
2. At least one of the calls uses getState, so this lets us bind it to the store in clean manner.
3. I think we should be minimizing direct access to the global Client4 instance, so having that call as part of an action makes the most sense to me since that's where Client4 calls usually go. I still want to get rid of the global instance eventually, and I think having it come from redux somehow will be the key to doing that.

It does feel a bit odd, and I'm not a huge fan of having those actions in the mattermost-redux repo since I can't imagine the mobile app will ever need them, but I can't think of a significant enough reason to change that.

[sudheerDev]

It does feel a bit odd, and I'm not a huge fan of having those actions in the mattermost-redux repo since I can't imagine the mobile app will ever need them

@hmhealey I understand where this comes from but reason that we might not or won't be needing in mobile does not seem right to me given the direction we are headed with mm-redux as an API for plugins.
I am just trying to see if we can modify the thought of what goes into redux and what goes into specific repos.

I like the fact that we keep view stores and any actions related to UI in specific repos but considering this an action to make an API call i feel this should be part of redux. Community should have access to all API actions through redux at the least.

I found myself at an odd place recently when i was trying to use an action present in webapp for a plugin because it causes circular dependency and i ended up copying it. Cases around these are the reason why i was pushing it to redux.

[enahum]

Well it looks like everyone would like to be an action and I’m ok with that.

The mobile reason to me is not a reason at all but that is another topic.

This discussion at least was beneficial in terms of what are we using, how and why

[MirlanMaksv]

So the conclusion is to leave that API calls as actions and in the webapp? Correct me please if I got it wrong

[enahum]

Yea @MirlanMaksv we are going to leave it as actions!

[MirlanMaksv]

and stays in the web app repo?

[sudheerDev]

@MirlanMaksv Yup. You can close the redux PR. We can leave them in the webapp and use them as prop actions for components

Sorry for the trouble.

[MirlanMaksv]

Hey @sudheerDev that is not a trouble, couple of copy-pastes and everything is fine

[hanzei]

Fixed with https://github.com/mattermost/mattermost-webapp/pull/2387.

Thanks @MirlanMaksv! :tada: