[HOLD for payment 2024-07-24] [$250] [HIGH] [API] OpenWorkspaceView API call is made on pages that do not need it

[mountiny]

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:
Reproducible in staging?:
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @mountiny
Slack conversation:

Action Performed:

Break down in numbered steps

1. Go to settings
2. Navigate to workspaces
3. Select a workspace
4. Notice that OpenWorkspaceView API command is called

Expected Result:

Describe what you think should've happened

this command is used to load bank account data and it should be called in Workflows page and/ or some other pages where we might need the bank account information for the policy, paid or free (still since migration is not complete)

Actual Result:

Describe what actually happened

This is being called on pages where the bank account details are not needed

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c3a5ef10ba51590e
• Upwork Job ID: 1796465195576598528
• Last Price Increase: 2024-05-31
• Automatic offers:
• hoangzinh | Reviewer | 102543634
• abzokhattab | Contributor | 102543638

Issue Owner

Current Issue Owner: @greg-schroeder

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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.

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

either we need to refactor this code so the API call is not in the WorkspacePageWithSections component as then people will forget to pass the shouldSkipVBBA call - that would be ideal, or we make that param required and pass it everywhere

[abzokhattab]

Proposal

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

The problem we are trying to solve is that the OpenWorkspaceView API call is being made on pages that do not require it. This results in unnecessary API calls, which can lead to performance issues and inefficient use of resources.

What is the root cause of that problem?

The root cause of the problem is that the OpenWorkspaceView API call is embedded within the WorkspacePageWithSections component. This leads to the API call being executed regardless of whether the page actually requires bank account information, as the component does not conditionally skip the API call when it is not needed.

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

To solve this problem, we propose the following changes:

• Ensure that the parameter shouldSkipVBBACall is required and passed explicitly wherever the WorkspacePageWithSections component is used.
• Update all relevant pages and components to correctly utilize the shouldSkipVBBACall parameter, making sure that the OpenWorkspaceView API call is only made on pages where bank account details are actually needed.
• additionally we need add shouldSkipVBBACall to the WorkspaceProfilePage to prevent the current issue from occurring

Optionally:

To preserve the separation of concerns principle, we can create an intermediate component WorkspaceViewLoader which will handle the fetching and loading parts. This component will return a loading component if the data is still loading; otherwise, it will return the children components.

This component should be called within the WorkspacePageWithSections component.

Alternatively

To preserve the separation of concerns principle, we can:

1. Completely remove the shouldSkipVBBACall flag.
2. Create a new component, WorkspacePageWithSectionsAndDataLoaded, that handles the data fetching and loading parts. This component will then return WorkspacePageWithSections with the fetched data passed to it.
3. then in components that require VBB call, the new component should be used, otherwise use the WorkspacePageWithSections component

[truph01]

Proposal

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

Describe what actually happened

What is the root cause of that problem?

The OpenWorkspaceView is in the WorkspacePageWithSections component, we already have shouldSkipVBBACall prop in WorkspacePageWithSections to control whether API call is needed, but we forgot to pass it in some cases. And shouldSkipVBBACall will be false by default so if people forget about it, it will always call the API whenever WorkspacePageWithSections is used.

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

Keep shouldSkipVBBACall here optional but make it true by default, so people won't be able to "forget" it. If they want the call to happen, they will need to explicitly pass shouldSkipVBBACall as false, if it's omiited, the API call will not happen.

Then:

• In places that have shouldSkipVBBACall as true (like in here), remove it because it's already true by default
• In places that right now do not have shouldSkipVBBACall (means it's intended to be false), evaluate if that user flow needs the VBBA data, if yes, pass shouldSkipVBBACall as false, if no, do nothing. Eg. from my checking, this WorkspaceProfilePage won't need the VBBA data and should not have the shouldSkipVBBACall prop, there will be similar pages like that. Meanwhile there will be pages that need the VBBA data and need the shouldSkipVBBACall={false}

What alternative solutions did you explore? (Optional)

[truph01]

people will forget to pass the shouldSkipVBBA call - that would be ideal, or we make that param required and pass it everywhere

@mountiny I think we can make shouldSkipVBBACall true by default, so people won't be able to "forget" it. If they want the call to happen, they will need to explicitly pass shouldSkipVBBACall as false, if it's omiited, the API call will not happen.

[truph01]

Proposal updated to add the above suggestion as alternative solution

[dragnoir]

Proposal

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

OpenWorkspaceView API call is made on pages that do not need it

What is the root cause of that problem?

inside WorkspacePageWithSections the default value for shouldSkipVBBACall is false, so the VBBACall will not be skipped if we forget the prop.

App/src/pages/workspace/WorkspacePageWithSections.tsx

Line 109 in e915d36

shouldSkipVBBACall = false,

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

on the WorkspacePageWithSections set the default shouldSkipVBBACall to true. So by default, the call will be skipped.

App/src/pages/workspace/WorkspacePageWithSections.tsx

Line 109 in e915d36

shouldSkipVBBACall = false,

There's no issue if anyone forget the prop shouldSkipVBBACall

Now we need to make sure the other pages have set shouldSkipVBBACall={false] when we need bank details

Like for

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Line 291 in e915d36

shouldSkipVBBACall

We need to set shouldSkipVBBACall to false

What alternative solutions did you explore?

[truph01]

Proposal updated to mention the specific case of Workflows page and what we should do with it.

@mountiny I see you mentioned in the OP

this command is used to load bank account data and it should be called in Workflows page

But I think:

the better solution here is to add the VBBA data to the OpenPolicyWorkflowsPage API response (if not already) which the Workflows page already called. This will help us maintain 1:1:1 rule.

@mountiny What do you think?

[abzokhattab]

Proposal Updated

added an optional improvement and an alternative solution

[truph01]

@hoangzinh @mountiny Just so there's no confusion when viewing proposals, @abzokhattab's proposal was posted before me but have these 2 points which are incorrect:

• Refactor the WorkspacePageWithSections component to remove the OpenWorkspaceView API call. (The API call is important and should stay)
• Introduce a conditional parameter (e.g., shouldSkipVBBA) that determines whether the OpenWorkspaceView API call should be made. (the prop already exists so there's no need to add a new one)

Which were later removed/updated after I posted mine

[hoangzinh]

Thanks for proposal everyone

• @dragnoir @truph01: your approach is making shouldSkipVBBA as required/default true. But I agreed with @mountiny that the ideal solution should be moving that API out of WorkspacePageWithSections component.
• @abzokhattab's alternative solution is the only proposal that follows this approach "refactor this code so the API call is not in the WorkspacePageWithSections component". But I think, instead of a new wrapper component, we can consider HOC, i.e withReimbursementAccount or withWorkspaceViewData.

---

Based on above points, I think we can go with @abzokhattab's proposal #42900 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[jasperhuangg]

Thanks for the investigation. Agree that @abzokhattab's alternative proposal is the only one that follows the pattern prescribed by Vit. Assigned!

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @abzokhattab 🎉 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 📖

[melvin-bot]

@hoangzinh @greg-schroeder @mountiny @truph01 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[truph01]

@greg-schroeder Sorry for the delay, this is a HIGH issue so could you help to apply the High Priority issue that was missed out here, as also done in this issue

TIA!

[mountiny]

I have opened a backend Pr to add the bank account information to the auth call, but seems like the OpenPolicyProfilePage call was reverted so we need to bring that back first

[greg-schroeder]

@mountiny sorry so does that mean this wasn't implemented as expected, or just an addendum to the original PR?

[greg-schroeder]

@truph01 I'll handle the High thing but I really need you to apply to the job

[truph01]

apply to the job

@greg-schroeder Done!

[greg-schroeder]

sent an offer

[truph01]

@greg-schroeder Accepted, thanks

[mountiny]

This was a required backend change to make it even better. We can soon proceed with the auth change as we have re-reverted the app pr

[mountiny]

The Auth PR was merged

[greg-schroeder]

On staging

[melvin-bot]

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

[melvin-bot]

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

• [NO QA] Stop call OpenWorkspaceView on workspace profile page #45318

If no regressions arise, payment will be issued on 2024-07-24. 🎊

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

• @hoangzinh requires payment automatic offer (Reviewer)
• @truph01 requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] 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.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[greg-schroeder]

This is for the backend PR, no contributors were involved in that one