Migrate do_verify_email to Redux

[avasconcelos114]

Summary

Converts the do_verify_email.jsx component to utilize Redux, and adds a few component tests

Ticket Link

mattermost/mattermost#7784

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)

[jwilander]

Overall looks pretty good, got a few comments though

[jwilander]

Super minor, but it looks like serverError is only used in the else if, mind moving the declaration inside there?

[jwilander]

Is the selector really needed here? Also it's not returning any props so won't you be losing the location prop? Let me know if I'm missing something

[avasconcelos114]

You're right about the selector not being needed. What I was wondering about this particular component though, is that it seems to only be called through a link in a verification mail, rather than by a parent component (likely then getting its location prop through parameters given in the link).

It seems like the mapStateToProps function is required by connect(), so I wasn't sure what was the best course of action for a component that only needed the mapDispatchToProps.
Would the following be the right way to do it then?

function mapStateToProps(state, ownProps) {
    return ownProps;
}

[jwilander]

Is this correct? It looks like token should be nested in query per this access above this.props.location.query.token

[saturninoabril]

Since this won't subscribe to the store, it can be like:

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

[saturninoabril]

I suggests not to import this and would recommend only doing the mockup of props.verifyUserEmail.

Would prefer to have tests as well for:

• on verifyUserEmail success (with state and browserHistory.push assertions) similar to this - https://github.com/mattermost/mattermost-webapp/blob/master/tests/components/integrations/edit_oauth_app.test.jsx#L139
• on verifyUserEmail error (with state assertions) similar to this - https://github.com/mattermost/mattermost-webapp/blob/master/tests/components/integrations/edit_oauth_app.test.jsx#L165

[saturninoabril]

This setTimeout can be avoided here by:

• adding async like test('should run verifyUserEmail action & display error message', async () => {
• adding mockup to verifyUserEmail (e.g. requiredProps.actions.verifyUserEmail = jest.genMockFunction().mockImplementation(...))

Please see EditOAuthApp component test for your reference.

[saturninoabril]

Would recommend to add values here (e.g. {token: 'token', email: '[email protected]'}), so we could do assertions like:

• expect(requiredProps.actions.verifyUserEmail).toHaveBeenCalledWith('token');
• expect(browserHistory.push).toHaveBeenCalledWith('/login?extra=verified&email=test%40test.com');

[jwilander]

@avasconcelos114, let us know if you have any questions or need any help :)

[avasconcelos114]

EDIT: Oh god my commit was a mess after I rebased to the newest master, any guidance on how not to make this mistake again would also be greatly appreciated 😞
Yeah sadly I need help again :(

I figured it's best to make another commit given that my issue is with various parts of the do_verify_email.test.jsx file.

As (apparently) Promises with no reject will soon be deprecated (I keep seeing warning messages about it), I've made a few changes to the original mock function seen in edit_oauth_app.test.jsx. (The problems occur even when I use the exact same code as edit_oauth_app.test.jsx, but I'm just explaining why it looks different).

The issue is that I'm getting errors in the last two tests that involve the aforementioned Promises, yet I can't seem to find the cause of it. Running npm test do_verify_email.test.jsx gives me the following errors:

● components/DoVerifyEmail › should reject verifyUserEmail action & display error message

TypeError: Cannot read property 'data' of undefined

● components/DoVerifyEmail › should resolve verifyUserEmail & push value to browserHistory
Failed: [object Object]

Been looking over everything and just can't seem to find what's causing these, some guidance would be very much appreciated :(

[saturninoabril]

I guess you forgot below. That might also be one of the reason why your test (patterned to EditOAuthApp) is not working properly.

        actions: PropTypes.shape({
            verifyUserEmail: PropTypes.func.isRequired
        }).isRequired

[saturninoabril]

Don't know why but this is not playing nicely with the test. It works fine though with below code but I'm 0/5 if it's a good pattern:

        const verify = await verifyUserEmail(this.props.location.query.token);

       if (!verify) {
            // something wrong happened
        }else if (verify.data) {
            ...
        } else if (verify.error) {
            ...
        }

[avasconcelos114]

Everything else you mentioned worked really well. Thank you for the help.

but regarding this part, I tried to see if maybe there was a better pattern to use, and the best I could come up with ended up being:

const verify = await verifyUserEmail(this.props.location.query.token);

        if (verify && verify.data) {
            browserHistory.push('/login?extra=verified&email=' + encodeURIComponent(this.props.location.query.email));
        } else if (verify && verify.error) {
            const serverError = (
                <FormattedMessage
                    id='signup_user_completed.invalid_invite'
                    defaultMessage='The invite link was invalid.  Please speak with your Administrator to receive an invitation.'
                />
            );
            this.setState({
                verifyStatus: 'failure',
                serverError
            });
        }

Which isn't too different than what you showed.

I'll keep trying to find a possibly better pattern for this, let me know what you think

[saturninoabril]

Cool, I'm good with it.

[saturninoabril]

Should add snapshots test for

• when state.verifyStatus is 'pending' and,
• when state.serverError is not null.

[saturninoabril]

Should add:

    global.window.mm_config = {};
    
    beforeEach(() => {
        global.window.mm_config.SiteName = 'Mattermost';
    });

    afterEach(() => {
        global.window.mm_config = {};
    });

[saturninoabril]

Once action is added at DoVerifyEmail, below should work

        requiredProps.actions.verifyUserEmail = jest.genMockFunction().mockImplementation(
            () => {
                return new Promise((resolve) => {
                    process.nextTick(() => resolve(requestError));
                });
            }
        );

Then, process.nextTick(() => resolve(requestSuccess)); to next test.

[saturninoabril]

All looks good to me now except for last minor change request.

Not sure what happened to your branch and I'm not sure how to fix it. Consider this approve from me once your branch is cleared. Thanks!

[saturninoabril]

Sorry for this last bit from me. I don't think this is needed here since it's already setup here: https://github.com/mattermost/mattermost-webapp/blob/master/tests/setup.js

[avasconcelos114]

Alrighty, so just to confirm, before getting this branch approved I need to:

1. Remove import Adapter from 'enzyme-adapter-react-16'; as it's already been setup in a previous merge
2. Clear up my branch

By clearing up my branch you mean that mess of commits/files changed right? I'm not quite sure what happened, they somehow got included as if they were my commits every time I rebased to the newest master, a bit frustrating but I'll look for ways to fix it!

[avasconcelos114]

Haven't been having much luck with finding a solution for clearing up all those commits and modified files...

Wondering if starting a new branch/pull request would provide a 'cleaner' solution to fixing this up.

[saturninoabril]

I temporarily changed the base branch then backed again to master, and it looked like the commits are now cleaner.

[saturninoabril]

@avasconcelos114 Sorry for the inconvenience but could you give this a rebase to latest master (with fix to our build system)? Thanks!