Mattermost-server: [MM-12734] Migrate email-related actions from user_actions.jsx to redux

Created on 25 Jan 2019 · 11Comments · Source: mattermost/mattermost-server

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]

verifyEmail
resetPassword
resendVerification
sendPasswordReset

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

// actions/foo_actions.js
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>;
\}
\}

AreTechnical Debt Medium Help Wanted TecReactJS TecRedux

Source

esethna

All 11 comments

I will start working on this ticket.

ajeethkannan on 11 Feb 2019

🎉1

Thanks @ajeethkannan. :+1: Let me know, if you have any question.

hanzei on 11 Feb 2019

@hanzei I am a new contributor. I have set up the dev environment in my local machine. I will get back to for questions :)

ajeethkannan on 11 Feb 2019

👍1

Hi,

I am trying to write unit tests for password_reset_form.jsx to test if resetUserPassword (an action in redux) in handlePasswordReset. I am not able to assign or mock a value to const password = ReactDOM.findDOMNode(this.refs.password).value;

Here is the complete code of the function handlePasswordReset
`handlePasswordReset = (e) => {

    e.preventDefault();
    const password = ReactDOM.findDOMNode(this.refs.password).value;
    if (!password || password.length < Constants.MIN_PASSWORD_LENGTH) {
        this.setState({
            error: (
                <FormattedMessage
                    id='password_form.error'
                    defaultMessage='Please enter at least {chars} characters.'
                    values={{
                        chars: Constants.MIN_PASSWORD_LENGTH,
                    }}
                />
            ),
        });
        return;
    }

    this.setState({
        error: null,
    });

    this.props.actions.resetUserPassword(
        (new URLSearchParams(this.props.location.search)).get('token'),
        password
    ).then(({data, error}) => {
        if (data) {
            browserHistory.push('/login?extra=' + Constants.PASSWORD_CHANGE);
            this.setState({error: null});
        } else if (error) {
            this.props.onError({id: error.server_error_id, ...error});
        }
    });
}`

And, this is the unit test that I am trying to achive
`

test("should have called handlePasswordReset", async () => {
        const props = {
            ...baseProps, 
            actions: {
                ...baseProps.actions
            },
        };
        const wrapper = mount(
            <PasswordResetForm {...props}/>
        );
        wrapper.find("password").value = "123456";
        await wrapper.instance().handlePasswordReset({preventDefault: jest.fn()});
        expect(baseProps.actions.resetUserPassword).toHaveBeenCalled();
        wrapper.unmount();
    });`

ajeethkannan on 26 Feb 2019

@ajeethkannan Thanks for the question. I will defer it to one of our JS devs.

@saturninoabril Can you please take a look at the above question?

hanzei on 26 Feb 2019

👍1

@ajeethkannan Thank you for helping out.

You may assign value to password ref by:

wrapper.ref('password').value = '123456';

Also on separate item, you may use a helper function mountWithIntl since it's using react-intl under the hood.

Test could be something like below (with comments on what is good to have):

import {mountWithIntl} from 'tests/helpers/intl-test-helper.jsx';
...
    test('should have called handlePasswordReset', async () => {
        const wrapper = mountWithIntl(
            <PasswordResetForm {...baseProps}/>
        );

        wrapper.ref('password').value = '123456';

        await wrapper.instance().handlePasswordReset({preventDefault: jest.fn()});
        expect(baseProps.actions.resetUserPassword).toHaveBeenCalled();
        // NOTE: better to test with ".toHaveBeenCalledTimes(1)" than ".toHaveBeenCalled()"
        // NOTE: resetUserPassword should test ".toBeCalledWith" with what args

        // when "resetUserPassword" returned "data", should test error state and if "browserHistory.push" is called and with what args
        // when "resetUserPassword" returned "error", should test if "props.onError" is called and with what args
    });