MM-13348: Fix TOS reappears on next page refresh

[chetanyakan]

Summary

loadMe method expects clientConfig to be loaded in advance. The value from clientConfig is used to load terms of service status and custom emoji conditionally.

Ticket Link

https://mattermost.atlassian.net/browse/MM-13348

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
• Touches critical sections of the codebase (custom terms of service)

[lieut-data]

@chetanyakan, is it safe to dispatch getLicenseConfig asynchronously?

[chetanyakan]

@lieut-data I am not sure. Could you elaborate on why it would not be safe?

[sudheerDev]

This will slow down the initiation of the app as we are waiting for getClientConfig response. Does it not work if we pass the key value from config to the react component it requires for conditional updates?

[chetanyakan]

One alternative is to get terms of service and custom emoji always regardless of client config value in mattermost-redux.

[chetanyakan]

@lieut-data @sudheerDev Would it make more sense to make this change instead?

Also, I am not sure about the working of custom emoji. So, could you help me understand if changing that would be required.

[chetanyakan]

@sudheerDev This API call in loadMe of mattermost-redux is called only when config.EnableCustomTermsOfService === 'true'.

But since the config is not loaded, the condition will always evaluate to false and the user's terms-of-service status is not fetched on page refresh. This is why user needs to re-accept terms of service on every page refresh.

So, either we need config before API call for loadMe or we don't use config inside loadMe.

[sudheerDev]

Got it!. Thanks. Make sense to exclude them from loadMe and call those two api calls here after these promises are resolved IMO.

@lieut-data The config call does not take a long time to return around 200ms on pre-release. I am not sure if i am reading too much into this.

[chetanyakan]

@lieut-data @sudheerDev
It makes sense to mention the above mentioned behavior was changed in this PR.

Here, getClientConfig was called before loadMe and getLicenseConfig.
And here, after refactoring, getClientConfig, loadMe and getLicenseConfig are called parallelly.

I couldn't figure out whether this intentional or changed unintentionally while refactoring.

[lieut-data]

I suspect the parallelization was done intentionally. I really like @sudheerDev's idea of conditionally doing the TOS fetch here instead of necessarily within loadMe, but I wouldn't be surprised if that opens up other issues, so it might be easier to defer.

We're investigating ways to fix this login/startup pipeline, but for now I'd propose that we at least continue to parallelize getClientConfig and getLicenseConfig, await on Promise.all, and then await on UserActions.loadMe(), finally returning all the promises together. That should at least preserve the parallelization of loading config and license.

[chetanyakan]

1. We shouldn't fetch TOS details here in the web app compared to redux as it would also be needed for the mobile app.
2. Updated to parallelize getClientConfig and getLicenseConfig.

[sudheerDev]

LGTM

[lieut-data]

If there is no MMUSERID cookie, loadMeResponse will be undefined, whereas previously it wasn't returned at all. Since we're creating a new array on line 26 anyway, I'd just drop the const on resolvedPromises and push as before.

[chetanyakan]

@lieut-data Good catch. Updated.

[lieut-data]

Perfect! And right, no need to drop const since we’re not reassigning. Always trips me up :)