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

[$250] Not Found page shows up when enabling Company Cards on offline #57119

Open
1 of 8 tasks
m-natarajan opened this issue Feb 19, 2025 · 11 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue

Comments

@m-natarajan
Copy link

m-natarajan commented Feb 19, 2025

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.1.1-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @c3024
Slack conversation (hyperlinked to channel name): #Expensify bugs

Action Performed:

  1. Create a workspace.
  2. Switch “Force offline” on from Cmd + D menu.
  3. Go to workspace settings > More features > enable Company Cards.
  4. Disable “Company Cards”.
  5. Enable "Company Cards" again.
  6. Navigate to the "Company Cards" page.
  7. Go online from the Cmd + D menu.

Expected Result:

The Company Cards page should continue showing up as it was before.

Actual Result:

The Company Cards page changes to Not Found page.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence
Screen.Recording.2025-02-19.at.6.49.28.PM.mov
Recording.998.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021892309795912003900
  • Upwork Job ID: 1892309795912003900
  • Last Price Increase: 2025-02-19
Issue OwnerCurrent Issue Owner: @roryabraham
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 19, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

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

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Feb 19, 2025

I'm not able to reproduce this - asking QA to test. https://expensify.slack.com/archives/C9YU7BX5M/p1739993078008289

@kavimuru
Copy link

Still reproducible

Recording.38.mp4

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Feb 19, 2025
@melvin-bot melvin-bot bot changed the title Not Found page shows up when enabling Company Cards on offline [$250] Not Found page shows up when enabling Company Cards on offline Feb 19, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 19, 2025
Copy link

melvin-bot bot commented Feb 19, 2025

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Feb 20, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-20 05:58:39 UTC.

Proposal

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

What is the root cause of that problem?

When offline we have three requests in the network queue [API1, API2, API3] if we enable -> disable -> enable a feature. The API1 and API3 are the same.

When we get a request to process here by processNextPersistedRequest function, we use shift function that also change the current array and now the persistedRequests is API2, API3 after the first API is called.

ongoingRequest = persistedRequests.shift() ?? null;

After the API is complete, we call endPersistedRequestAndRemoveFromQueue to remove the API from the queue

endPersistedRequestAndRemoveFromQueue(requestToProcess);

Because the API1 and API3 are the same and the API1 is removed from processNextPersistedRequest function above, the first index is the index of the API3 then it's removed here and now the persistedRequests has only API2.

const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove));

So we only called two EnablePolicyCompanyCards APIs, the first is to enable this feature, and the second is to disable this feature so the not found page appears because this feature is disabled.

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

This starts happening after this PR. We have some options to fix it

  1. Don't use shift method here and replace it with at(0)
ongoingRequest = persistedRequests.at(0) ?? null;

ongoingRequest = persistedRequests.shift() ?? null;

  1. Remove the logic filter out the removed request in endRequestAndRemoveFromQueue and only update ongoing request to null in onyx

const requests = [...persistedRequests];
const index = requests.findIndex((persistedRequest) => isEqual(persistedRequest, requestToRemove));
if (index !== -1) {
requests.splice(index, 1);
}

  1. For each request, we can add a unique field like created time or random number so if it's removed from the queue, other requests will not be removed unexpectedly

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Update test for PersistedRequests with the case three APIs above and verify that all of them are processed

What alternative solutions did you explore? (Optional)

We can add checkAndFixConflictingRequest to the enable feature API to remove the APIs if we have another one. For example, when we call enable company card API with enable is true, we will check if in the persistedRequests exists another enable company card API with enable is false (Of course we need to check the policy param is the same) or vice versa, we will remove both of them.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@Christinadobrzyn
Copy link
Contributor

Awesome, @suneox can you take a peek at @nkdengineer's proposal to see if it will work?

@suneox
Copy link
Contributor

suneox commented Feb 20, 2025

The alternative solution from @nkdengineer looks better than his primary proposal. In this case, the API toggle feature should be called once with the latest request instead of calling all toggle APIs for each feature. The solution using RequestConflictResolver from his alternative proposal LGTM, and we need to apply it for all feature on the More Features page.

🎀 👀 🎀 C+ reviewed

Note

We can add more optional params into RequestConflictResolver to automatically resolveSingleRequest.

Copy link

melvin-bot bot commented Feb 20, 2025

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

@nkdengineer
Copy link
Contributor

@roryabraham Please help to review this #57119 (comment)

@melvin-bot melvin-bot bot added the Overdue label Feb 24, 2025
Copy link

melvin-bot bot commented Feb 24, 2025

@suneox, @roryabraham, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue
Projects
Status: No status
Development

No branches or pull requests

6 participants