[$250] iOS/Android WS features -Feature is not enabled after enabling it and upgrading workspace to Control

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.69-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

1. Launch hybrid or ND app.
2. Create a Collect workspace.
3. Go to More features.
4. Enable Rules.
5. Tap Upgrade.
6. Tap Got it, thanks.

Expected Result:

Rules feature will be enabled after enabling it and upgrading workspace to Control plan (production behavior).

Actual Result:

Rules feature is not enabled after enabling it and upgrading workspace to Control plan. User has to enable it again.

Workaround:

Unknown

Platforms:

• [x] Android: Standalone
• [x] Android: HybridApp
• [ ] Android: mWeb Chrome
• [x] iOS: Standalone
• [x] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/351ee99c-bbd0-44e4-945c-3b9986ab1bd2

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021863284829899625527
• Upwork Job ID: 1863284829899625527
• Last Price Increase: 2024-12-01
• Automatic offers:
• CyberAndrii | Contributor | 105222820

Issue Owner

Current Issue Owner: @Ollyws

[melvin-bot[bot]]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot[bot]]

Triggered auto assignment to @carlosmiceli (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot[bot]]

💬 A slack conversation has been started in #expensify-open-source

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~021863284829899625527

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

[mountiny]

Demoting as it does not block the user from moving ahead

[shubham1206agra]

Proposal

Please re-state the problem that we are trying to solve in this issue

iOS/Android WS features -Feature is not enabled after enabling it and upgrading workspace to Control.

What is the root cause of that problem?

After https://github.com/Expensify/App/pull/49937, the navigation blur listener placed here

https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx#L159

Does not work. Due to this, https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx#L101 never gets triggered, and therefore, feature does not get enabled.

What changes do you think we should make in order to solve the problem?

Change

https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx#L158-L167

to

useEffect(() => {
    if (!isUpgraded || !canPerformUpgrade || policy?.isPendingUpgrade) {
        return;
    }
    confirmUpgrade();
}, [isUpgraded, canPerformUpgrade, confirmUpgrade, policy?.isPendingUpgrade]);

to remove reliance on navigation listener

What alternative solutions did you explore? (Optional)

[Ollyws]

@shubham1206agra's proposal LGTM. 🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @carlosmiceli is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[andriivitiv]

Edited by proposal-police: This proposal was edited at 2024-12-02 13:03:05 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

iOS/Android WS features - Feature is not enabled after enabling it and upgrading workspace to Control

What is the root cause of that problem?

After PR https://github.com/Expensify/App/pull/49937, navigator's blur events stopped working on native platforms.

https://github.com/Expensify/App/blob/3cc88f5a83a979c5e588e01065083a3d714e69da/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx#L158-L167

The same thing happens on other pages, eg. the educational tooltip is not getting hidden

https://github.com/Expensify/App/blob/5b884db725809af2fe9a2172fe240822e99b67c7/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L321-L326

What changes do you think we should make in order to solve the problem?

We can use useFocusEffect and put the logic from the event handler into its cleanup function

useFocusEffect(
        useCallback(() => {
            return () => {
                if (!isUpgraded || !canPerformUpgrade) {
                    return;
                }
                confirmUpgrade();
            };
        }, [isUpgraded, canPerformUpgrade, navigation])
    );

useFocusEffect will call the cleanup function either on the blur event or from its internal useEffect cleanup func (whatever will be first).

[andriivitiv]

useEffect(() => {
    if (!isUpgraded || !canPerformUpgrade || policy?.isPendingUpgrade) {
        return;
    }
    confirmUpgrade();
}, [isUpgraded, canPerformUpgrade, confirmUpgrade, policy?.isPendingUpgrade]);

@shubham1206agra with your solution the confirmUpgrade func

1. will be called on component mount, and not unmount
2. will be called on every render which will cause many requests to be sent to the BE

[shubham1206agra]

useEffect(() => {
    if (!isUpgraded || !canPerformUpgrade || policy?.isPendingUpgrade) {
        return;
    }
    confirmUpgrade();
}, [isUpgraded, canPerformUpgrade, confirmUpgrade, policy?.isPendingUpgrade]);

@shubham1206agra with your solution the confirmUpgrade func

1. will be called on component mount, and not unmount

Not really problematic.

2. will be called on every render which will cause many requests to be sent to the BE

Nope, since these properties will not change many times

[andriivitiv]

Nope, since these properties will not change many times

Try running on web and watch the network tab

[kirillzyusko]

My feeling tells me that we should spend some time and investigate why blur is not working (instead of re-implementing a particular code that was depending on this event). Potentially other screens that depends on blur effect also can be broken, so we should fix a root cause (in my understanding)

[saifelance]

Check Feature Enablement Logic:

Review the Policy.upgradeToCorporate function to ensure that it also enables the feature post-upgrade: Policy.upgradeToCorporate(policy.id, feature.name);

Server-Side Confirmation:

Verify the server API response for the upgradeToCorporate call. Ensure it includes both the upgraded plan and the enabled feature state.

Feature Retention on Upgrade:

If Policy.enablePolicyReportFields is the method to enable features:

Policy.enablePolicyReportFields(policyID, true, true);

Update Flow for Confirm Upgrade: Modify the confirmUpgrade method to include enabling the feature if it isn’t automatically retained:

const confirmUpgrade = useCallback(() => {
    if (!feature || !policyID) {
        return;
    }
    Policy.enablePolicyReportFields(policyID, true, true); // Ensure feature is explicitly enabled
    return route.params.backTo
        ? Navigation.navigate(route.params.backTo)
        : Navigation.goBack();
}, [feature, policyID, route.params.backTo]);

Suggested Code Update

src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx

const confirmUpgrade = useCallback(() => {
    if (!feature || !policyID) {
        return;
    }

    // Explicitly enable the feature after upgrade
    Policy.enablePolicyReportFields(policyID, true, true);

    // Navigate back or to the appropriate screen
    return route.params.backTo
        ? Navigation.navigate(route.params.backTo)
        : Navigation.goBack();
}, [feature, policyID, route.params.backTo]);

useEffect(() => {
    const unsubscribeListener = navigation.addListener('blur', () => {
        if (!isUpgraded) {
            return;
        }
        confirmUpgrade();
    });

    return unsubscribeListener;
}, [isUpgraded, confirmUpgrade, navigation]);

State Debugging:

console.log('Policy:', policy);
console.log('Feature:', feature);

[mountiny]

This is coming from the native stack pr so taking over to manage

@kirillzyusko i agree that would be the best, can your team look into that please?

[kirillzyusko]

@mountiny yes, sure - feel free to assign me on this issue 👍

[kirillzyusko]

It looks like events, such as onDisappear/onWillDisappear will not be dispatched if screen gets dismissed programmatically (i. e. by calling navigation.goBack()) - this is how native-stack works and it's a known problem.

A potential solution I've been thinking of is adding blur event on unmount to useFocusEvents hook (in @react-navigation/core), but:

• we need to apply an additional patch (which most likely will not be merged upstream);
• in JS stack (when all events dispatched as expected) we may receive 2 blur events, which may introduce some breaking changes (on web, since we use plain stack on web).

After a conversation with @WoLewicki we came to conclusion that it would be safer to re-write this particular screen/code fragment to some more reliable hooks/events.

The solution proposed by @CyberAndrii in https://github.com/Expensify/App/issues/53360#issuecomment-2511440512 looks pretty good for me, because this hook will be executed on blur and unmount. The only thing that we need to check is that web will work without breaking change.

@mountiny since @CyberAndrii suggested the fix that should introduce as less breaking changes as possible I think it would make sense to assign this task to him? What do you think?

[shubham1206agra]

@kirillzyusko Why does useFocusEffect work then? Since it also uses a blur event.

[kirillzyusko]

@shubham1206agra it executes cleanup on unmount as well, if blur event hasn't been dispatched.

[shubham1206agra]

@kirillzyusko Then how is my proposal different other than the bad placement of useEffect?

I mean, I would have figured it out where to put the callback. It was not my first thought to put it on cleanup instead of first change since I haven't tested that extensively.

[kirillzyusko]

@shubham1206agra in your solution you are running side-effect inside useEffect (not on unmount). Because of that your solution (as pointed out by @CyberAndrii) will pollute a server with unnecessary requests.

The solution provided by @CyberAndrii is more reliable, because:

• we still depend on navigation events;
• in case if a view is unmounted without an event we will run a cleanup effect anyway.

I don't get what you meant by "bad placement"... Originally you were trying to run the effect while the component was mounted and you still insisted that your solution is correct even when @CyberAndrii pointed out potential problems with your solution 🤷‍♂️

And for me running something inside the effect and running it only on unmount sounds like two different proposals.

[shubham1206agra]

you still insisted that your solution is correct

No lol, my solution is not perfect.

[kirillzyusko]

No lol, my solution is not perfect.

What I wanted to say is that a proposal from @CyberAndrii from my perspective will introduce less potential problems and that's why it's more preferable for me.

The final decision is up to Expensify team which proposal to choose. I'm not a C+ reviewer so I only gave a detailed answer on what happens internally and why the approach with fixing it inside native-stack/navigation can be hard. And if we decide to re-write this code - we need to choose from suggested proposals. And I explained why proposal from @CyberAndrii looks more perspective from my point of view.

[shubham1206agra]

@kirillzyusko Is there no way to fix blur event? Since re-writing every blur event might not be possible.

[kirillzyusko]

@shubham1206agra we have only 7 places in the app where we use blur event. Here is my conversation with @WoLewicki about the problem:

[melvin-bot[bot]]

📣 @CyberAndrii 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

Thanks for the break down @kirillzyusko I have assigned @CyberAndrii Could you please work on applying this fix to all places where we rely on the blur event?

@kirillzyusko would be great if you could also review the solution and help in case it will be needed.

[kirillzyusko]

Sure @mountiny - will be glad to do that if needed 😊