[Hold for Payment on 16 June] Make permissions checking asynchronous by creating a withBetas HOC

[iwiznia]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Problem

The methods in permissions.js file should be asynchronous. They kind of already are if you look at the code, since data is coming from Onyx, but the methods themselves as synchronous, which does not align with how the data is loaded, so there are possible race conditions.

Solution

Change all places where permissions is used to instead subscribe to the ONYXKEYS.BETAS onyx key and call permissions methods passing the betas it got from onyx (see this comment)

Upwork posting - https://www.upwork.com/jobs/~01f29f2d2fa738d842

/cc @tgolen @marcaaron

[tgolen]

Yeah, I think this is a good move. I'd like to see those moved to an HOC or at the very least be made asynchronous. Without them being asynchronous, we are limited by the amount of checks we can do. For example, we can't use Environment.getEnvironment() because that is an async method.

While this is not currently a problem, I'd like to change this code so that when new betas are added, they do not follow this old sync pattern anymore.

[marcaaron]

Gonna try to rephrase this problem solution to make sure I understand...

Problem

• The beta list is fetched async then saved to storage before it updates the local list in memory.
• We also use a native module to determine if the app version is beta which happens async.
• Checks in Permissions.js need to wait for those things to happen before returning or else they may be inaccurate.

Solution

Make the Permissions methods async + create an HOC to delay rendering the wrapped component until after the async things are finished so we can check the betas via props?

Or was the HOC supposed to work differently...?

[iwiznia]

What do you mean by delay rendering? As in other HOCs, I think we would not delay it, right? If the betas data is not present, it would render the same as if you were not in any beta and when the data is available, it would re-render.

[marcaaron]

As in other HOCs, I think we would not delay it, right? If the betas data is not present, it would render the same as if you were not in any beta and when the data is available, it would re-render.

withOnyx delays the rendering of a component until the data is retrieved from AsyncStorage so the Onyx.connect() part of our problem is already solved. That leaves the API call and the native modules async data.

Delaying came to mind since I'd assume we would not want to initialize a view with all beta features disabled by default and then flash in the beta features as the various data becomes available. But if we don't care about that then maybe another idea is to just subscribe directly to the betas key via withOnyx and update it with an action like...

let isOnDev;

function updateBetas(betas = []) {
    if (isOnDev) {
        Onyx.set(ONYXKEYS.BETAS, CONST.BETAS.ALL);
        return;
    }
    
    Onyx.merge(ONYXKEYS.BETAS, betas);
}

getEnvironment()
    .then((environment) => {
        isOnDev = environment === CONST.ENVIRONMENT.DEV;
        updateBetas();
    });

Now any component that is connected to withOnyx() can just pass this.props.betas to a Permissions.js method to check that feature.

render() {
    if (canUseChronos(this.props.betas)) {
        return (...)
    };
}

export default withOnyx({
    betas: {key: ONYXKEYS.BETAS}
})(SomeComponent);

[SofiedeVreese]

I'm out of office today so removing my assignment and adding the AutoAssigner label back in for triaging.

[tgolen]

I think the delayed re-rendering might be a pre-optimization. I'm not sure that there is really a problem with that yet. However, I do kinda like the idea of just using withOnyx (not creating a new HOC) and then passing this.props.betas to the permission methods.

[iwiznia]

I kind of liked the HOC as it will abstract a common repeating pattern, but I am fine with subscribing to Onyx directly too.

[parasharrajat]

I prefer Onyx solution, a pretty straightforward thing and no need of HOC overhead.

[iwiznia]

@laurenreidexpensify updated the OP, we are ready to move this forward, can you post it to upwork?

[MelvinBot]

Triggered auto assignment to @timszot (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[laurenreidexpensify]

Upwork posting here https://www.upwork.com/jobs/~01f29f2d2fa738d842

[parasharrajat]

Proposal

1. We Will update all the methods in Permissions.js in a way that they take betas as a parameter.
   for example

function canUseAllBetas(betas) {
    return isDevelopment() || _.contains(betas, CONST.BETAS.ALL);
}

2. Now we will pass the betas at all places where we are calling these methods.
3. We will get the betas from Onyx by subscribing over the components as it makes the process synchronous.
   for ex:

withOnyx({
    betas: {key: ONYXKEYS.BETAS}
})(SomeComponent);

then updating the call as to Permissions.canUseAllBetas(this.props.betas) .

One of the location for change is https://github.com/Expensify/Expensify.cash/blob/f1cfb9eb102259a8f7f8662c86a432a09c23eb8e/src/pages/home/report/ReportActionCompose.js#L400

Special Case https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L336

Solution for this particular case.

1. Subscribe to ONYXKEYS.BETAS at page level(SearchPage).
2. Modify the getOptions to accept betas as optional Config Param

function getOptions(reports, personalDetails, draftComments, activeReportID, {
        betas = [],
        ....

which will be used at https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L336

3. Modify getSearchOptions to take another parameter betas.
   https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L365
4. pass down the betas from getSearchOptions to getOptions config key betas

[laurenreidexpensify]

@timszot these remain daily until we have a PR up and merged 👍🏽

[tgolen]

@parasharrajat I think that sounds like a good proposal, but please take a look at this usage: https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L336

It is going to be the most difficult one since the betas will have to be passed down through several layers. Can you please update your proposal to include this? Thanks!

[parasharrajat]

@tgolen
Better approach would be use the Onyx.connect method to pass the betas to getOptions but that goes against the desired behavior. We want to wait until betas is ready on Onyx so subscribing at component level helps but Onyx.connect will not.
So we can do the following:

1. Subscribe to ONYXKEYS.BETAS at page level(SearchPage).
2. Modify the getOptions to accept betas as optional Config Param

function getOptions(reports, personalDetails, draftComments, activeReportID, {
        betas = [],
        ....

which will be used at https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L336

3. Modify getSearchOptions to take another parameter betas.
   https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L365
4. pass down the betas from getSearchOptions to getOptions config key betas

[tgolen]

Yep, I think that's the approach we need to take with this! It's a good proposal now. Let's go with it. Green light!

…

On Tue, Jun 1, 2021 at 10:08 AM Rajat Parashar ***@***.***> wrote: Better approach would be use the Onyx.connect method to pass the betas to getOptions but that goes against the desired behavior. We want to wait until betas is ready on Onyx so subscribing at component level helps but Onyx.connect will not. So we can do the following: 1. Subscribe to ONYXKEYS.BETAS at page level(SearchPage). 2. Modify the getOptions to accept betas as optional Config Param function getOptions(reports, personalDetails, draftComments, activeReportID, { betas = [], .... which will be used at https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L336 1. Modify getSearchOptions to take another parameter betas. https://github.com/Expensify/Expensify.cash/blob/71c58ee8b17fce1503a8c9cabfcdc630b6fc5d2f/src/libs/OptionsListUtils.js#L365 2. pass down the betas from getSearchOptions to getOptions config key betas — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3194 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB5ATBG4XM3USEGGXJDTQUAWFANCNFSM45VGQQ4Q> .

[laurenreidexpensify]

@parasharrajat have hired you in Upwork 👍🏽

[laurenreidexpensify]

Payment has been issued in Upwork @parasharrajat 😃