Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-05-03] [$500] [Workflows] Workspace - "Hmm it's not here" error appears briefly when enabling workflows #39443

Closed
6 tasks done
lanitochka17 opened this issue Apr 2, 2024 · 64 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 2, 2024

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


Version Number: 1.4.59.0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4468674&group_by=cases:section_id&group_order=asc&group_id=283225
Issue reported by: Applause - Internal Team

Action Performed:

  1. Log in with a new expensifail account
  2. Create a workspace
  3. Navigate to Workspace settings - More features
  4. Enable workflows

Expected Result:

I shouldn't be getting any errors

Actual Result:

"Hmm it's not here" error appears briefly when enabling workflows for the first time

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6435811_1712077387579.bandicam_2024-04-02_18-58-32-923.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f589bc2448420016
  • Upwork Job ID: 1775643123069042688
  • Last Price Increase: 2024-04-10
  • Automatic offers:
    • hungvu193 | Reviewer | 0
    • ZhenjaHorbach | Contributor | 0
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@lanitochka17
Copy link
Author

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 3, 2024

Proposal

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

"Hmm it's not here" error appears briefly when enabling workflows

What is the root cause of that problem?

The main problem with issue is that when we create a new workspace we are waiting for a response from the server
And if during this period of time we enable PolicyWorkflows we will add enablePolicyWorkflows to the queue

But after enabling PolicyWorkflows we will navigate to the PolicyWorkflows screen and make request to get current state of policy which is still disabled

As result we will have here is list of event

  1. Create workspace ( PolicyWorkflows is disabled) (API.write)
  2. Press on the radio button ( PolicyWorkflows is enabled) (API.write) - still waiting first event
  3. Navigate to the PolicyWorkflows screen
  4. Get answer from create workspace ( PolicyWorkflows is disabled) (At this moment we have the issue)
  5. Get answer from changing state of PolicyWorkflows(PolicyWorkflows is enabled)
Screenshot 2024-04-03 at 11 35 24

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

To fix this issue we can update our FeatureEnabledAccessOrNotFoundComponent and depending on pendingFields(which we will change after press on radio button) we will update our policyFeature to prevent unnecessary changes while we are waiting for a response from the server

function FeatureEnabledAccessOrNotFoundComponent(props: FeatureEnabledAccessOrNotFoundComponentProps) {
const isPolicyIDInRoute = !!props.policyID?.length;
const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id);
const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName);
useEffect(() => {
if (!isPolicyIDInRoute || !isEmptyObject(props.policy)) {
// If the workspace is not required or is already loaded, we don't need to call the API
return;
}
Policy.openWorkspace(props.policyID, []);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isPolicyIDInRoute, props.policyID]);

    const isPolicyIDInRoute = !!props.policyID?.length;
    const [isPolicyFeatureEnabled, setIsPolicyFeatureEnabled] = useState(PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName));
    const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id);
    const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !isPolicyFeatureEnabled;
    const {isOffline} = useNetwork();

    useEffect(() => {
        if (!isPolicyIDInRoute || !isEmptyObject(props.policy)) {
            // If the workspace is not required or is already loaded, we don't need to call the API
            return;
        }

        Policy.openWorkspace(props.policyID, []);
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [isPolicyIDInRoute, props.policyID]);


    useEffect(() => {
        if (props.policy?.pendingFields?.[props.featureName] === 'update' && !PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName) && !isOffline) {
            return;
        }
        setIsPolicyFeatureEnabled(PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName));
    }, [props.policy?.pendingFields?.[props.featureName], PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName)]);

Here we add useState and useEffect. And only when props.policy?.pendingFields?.[props.featureName] will be null we will update isPolicyFeatureEnabled to be sure that we receive only current data from the server

What alternative solutions did you explore? (Optional)

As alternative we can show isLoading while we are waiting answer from server

In case of offline mode we will not use pendingFields
Since I mentioned that the issue is related to the server response
In this case, displaying content will work as expected

const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id);

    const shouldShowFullScreenLoadingIndicator =
        (props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id)) ||
        (props.policy?.pendingFields?.[props.featureName] === 'update' && !isOffline);

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

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

@melvin-bot melvin-bot bot changed the title Workspace - "Hmm it's not here" error appears briefly when enabling workflows [$500] Workspace - "Hmm it's not here" error appears briefly when enabling workflows Apr 3, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

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

@hungvu193
Copy link
Contributor

hungvu193 commented Apr 4, 2024

Hey! How can I re-enable workflow once I enabled it?

Screen.Recording.2024-04-04.at.10.05.49.mov

@ZhenjaHorbach
Copy link
Contributor

Hey! How can I re-enable workflow once I enabled it?

Screen.Recording.2024-04-04.at.10.05.49.mov

Do you mean to open more features ?
And then turn workflows off and on ?)

@hungvu193
Copy link
Contributor

Hey! How can I re-enable workflow once I enabled it?
Screen.Recording.2024-04-04.at.10.05.49.mov

Do you mean to open more features ? And then turn workflows off and on ?)

Oh god 🤦 , silly me. Thank you.

@hungvu193
Copy link
Contributor

I'm not able to reproduce on latest main, can you still reproduce? @ZhenjaHorbach

Screen.Recording.2024-04-04.at.14.37.18.mov

@ZhenjaHorbach
Copy link
Contributor

I'm not able to reproduce on latest main, can you still reproduce? @ZhenjaHorbach

Screen.Recording.2024-04-04.at.14.37.18.mov

This bug is reproduced immediately after creating a workspace

@hungvu193
Copy link
Contributor

Like this right?

Screen.Recording.2024-04-04.at.14.46.26.mov

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 4, 2024

Like this right?

Screen.Recording.2024-04-04.at.14.46.26.mov

Almost )
When the workspace was created.
Turn on any feature and wait on this screen until "Hmm it's not here" appears

Screen.Recording.2024-04-04.at.09.49.56.mov

@hungvu193
Copy link
Contributor

I believe I could reproduce this issue without creating a new workspace while reviewing your proposal, however I can't reproduce it now. Can you give it a try and let me know? Thank you 😄

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 4, 2024

I believe I could reproduce this issue without creating a new workspace while reviewing your proposal, however I can't reproduce it now. Can you give it a try and let me know? Thank you 😄

I understood what you were talking about)
Yes )
If we reload the page and then turn the feature off and on, then we will see "Hmm it's not here"
Apparently, not all cases in my proposal were verified
But I updated the proposition )

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 4, 2024

Proposal

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

  • Workspace - "Hmm it's not here" error appears briefly when enabling workflows

What is the root cause of that problem?

  • Given these below steps:
  1. We click to create new workspace, API CreateWorkspace is called. It will set areWorkflowsEnabled: false in optimisticData (If done, it will also return areWorkflowsEnabled: false)

  2. When the above API is in progress, open "More feature" screen then toggle "Workflows" feature, API EnablePolicyWorkflows is called. It will set areWorkflowsEnabled: true in optimisticData (If done, it will return areWorkflowsEnabled: true).

  3. In step 2, if after we set areWorkflowsEnabled: true in optimisticData, then API in step 1 is successful, the areWorkflowsEnabled will be false finally. So, the shouldShowNotFoundPage in here will be true, and the Not here page is displayed.

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

  • Based on the RCA that the optimistic data areWorkflowsEnabled: true in step 2 is overridden by the server response areWorkflowsEnabled: false in step 1, I think we should modify the function enabledPolicyWorkflows to introduce a new key in policy, named areWorkflowsEnabledTemp, it stores the same data as the areWorkflowsEnabled, but cannot be overridden. So along with the areWorkflowsEnabled key, we can also use areWorkflowsEnabledTemp to check whether the workflows feature is enabled or not. We can do like below:
  1. Update this:
    areWorkflowsEnabled: enabled,

    to:
                    areWorkflowsEnabled: enabled,
                    areWorkflowsEnabledTemp: enabled,

and this:

value: {

to:

                   areWorkflowsEnabledTemp: null,

and this:

areWorkflowsEnabled: !enabled,

to:

                    areWorkflowsEnabled: !enabled,
                    areWorkflowsEnabledTemp: null,
  • Then, to fix the bug "Hmm It's not here appears briefly", just need to update this:
    return Boolean(policy?.[featureName]);

    to:
    return Boolean(policy?.[featureName]) || Boolean(policy?.[`${featureName}Temp`]);
  • Then, we need to fix other components that encounter the same issue. For example, we can see that along with the page""Hmm It's not here appears briefly", the "Workflows" option in WorkspaceInitialPage is also disappeared. To fix it, just need to update this:
    if (policy?.areWorkflowsEnabled) {

    to:
    if (PolicyUtils.isPolicyFeatureEnabled(policy, 'areWorkflowsEnabled')) {

What alternative solutions did you explore? (Optional)

  • Generally, if the user already accessed the page, we do not display the not here page anymore. It will prevent the not here page from appearing briefly. Below is the detail.
  • We can update this to:
  const wasPageVisible = useRef(false);
    const shouldShowNotFoundPage = !wasPageVisible.current && (isEmptyObject(props.policy) || !props.policy?.id || !PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName));

  • Then update this to:
    wasPageVisible.current = true;
    return typeof props.children === 'function' ? props.children(props) : props.children;
  • We need to handle additional case: we open workflows page in device A then turn off this feature in device B. In this case, we just need to navigate user to more-feature page. Do it by updating this to:
    if(!PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName)){
        Navigation.navigate(ROUTES.WORKSPACE_MORE_FEATURES.getRoute(props.policyID))
    }
  • With this solution, there is no "Hmm It not here" page display for a while.
  • Also, we need to apply the similar logic to other components that will display based on condition isPolicyFeatureEnabled, for example, the "Workflows" button in WorkspaceInitialPage

@hungvu193
Copy link
Contributor

hungvu193 commented Apr 5, 2024

@ZhenjaHorbach's alternative solution looks good to me.
Thanks @nkdengineer, I think using persistedRequests is a little over-engineering for this case since we only needed pendingFields itself.

🎀👀🎀 C+ reviewed

@hungvu193

This comment was marked as duplicate.

Copy link

melvin-bot bot commented Apr 5, 2024

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

@nkdengineer
Copy link
Contributor

@hungvu193 In the read API, for example OpenMoreFeaturePage, we do not have pendingFields. So the bug still appears if we just check if the policy has pendingField or not

@hungvu193
Copy link
Contributor

@hungvu193 In the read API, for example OpenMoreFeaturePage, we do not have pendingFields. So the bug still appears if we just check if the policy has pendingField or not

Can you reproduce it? I tested it with all the features today and I can't seem to reproduce with pendingFields solution.

@nkdengineer
Copy link
Contributor

nkdengineer commented Apr 5, 2024

@hungvu193

  • You are correct, I am unable to reproduce this bug anymore.
  • I updated my alternative proposal to fix this issue "Workspace - "Hmm it's not here" error appears briefly when enabling workflows". With the solution using "pendingField" and "persistentRequest", the infinite loading indicator will be displayed in offline mode

@hungvu193
Copy link
Contributor

hungvu193 commented Apr 5, 2024

Again I can reproduce the issue while I was reviewing 2 proposals, however I can't reproduce now 😂 , so It will take more times to review this alternative proposal

One note for @ZhenjaHorbach, your solution worked but it will always show loading indicator even when we enabled the feature in offline mode.

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 5, 2024

Again I can reproduce the issue while I was reviewing 2 proposals, however I can't reproduce now 😂 , so It will make more times to review this alternative proposal

One note for @ZhenjaHorbach, your solution worked but it will always show loading indicator even when we enabled the feature in offline mode.

Updated proposition )

@nkdengineer
Copy link
Contributor

@ZhenjaHorbach Yeah. Please cover this case in your PR. Anyway, your proposal looks good

@trjExpensify trjExpensify changed the title [$500] Workspace - "Hmm it's not here" error appears briefly when enabling workflows [$500] [Workflows] Workspace - "Hmm it's not here" error appears briefly when enabling workflows Apr 11, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 14, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Apr 20, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 26, 2024
@melvin-bot melvin-bot bot changed the title [$500] [Workflows] Workspace - "Hmm it's not here" error appears briefly when enabling workflows [HOLD for payment 2024-05-03] [$500] [Workflows] Workspace - "Hmm it's not here" error appears briefly when enabling workflows Apr 26, 2024
Copy link

melvin-bot bot commented Apr 26, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 26, 2024
Copy link

melvin-bot bot commented Apr 26, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.66-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-05-03. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Apr 26, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@hungvu193] The PR that introduced the bug has been identified. Link to the PR:
  • [@hungvu193] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@hungvu193] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@hungvu193] Determine if we should create a regression test for this bug.
  • [@hungvu193] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@puneetlath
Copy link
Contributor

@hungvu193 friendly reminder on the checklist so that we can pay in a couple days.

@hungvu193
Copy link
Contributor

This is new feature and we're still developing it so I don't think we need regression test for this one.

@puneetlath
Copy link
Contributor

All paid. Thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

5 participants