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-11-18] [$250] Workspace -App redirects to workspace editor with error when returning from not here page #46646

Closed
2 of 6 tasks
izarutskaya opened this issue Aug 1, 2024 · 76 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 1, 2024

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.15-4
Reproducible in staging?: Y
Reproducible in production?: Y
Found when executing PR : #46479
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Paste Fix back button overlaps with status bar in workspace not found page #46479
    in any chat.
  3. Tap on the link.
  4. Swipe back to return to previous page (not app back button).
  5. Might need to repeat Step 3 and 4 one more time to see the workspace editor.

Expected Result:

In Step 4, app will not return to workspace editor as the workspace link is invalid.

Actual Result:

In Step 4, app returns to workspace editor with the error "Invalid Policy ID".

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

Bug6558699_1722468815726.ScreenRecording_08-01-2024_07-24-08_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015e953d4b48a8b7a8
  • Upwork Job ID: 1821050583313225827
  • Last Price Increase: 2024-09-11
  • Automatic offers:
    • shubham1206agra | Contributor | 103516762
    • dominictb | Contributor | 103942180
Issue OwnerCurrent Issue Owner: @stephanieelliott
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@stephanieelliott
Copy link
Contributor

This seems like this may be a regression from #46183 -- posted there to have the PR author take a look.

@melvin-bot melvin-bot bot removed the Overdue label Aug 6, 2024
@bernhardoj
Copy link
Contributor

This issue is present before the PR.

@stephanieelliott
Copy link
Contributor

Thanks @bernhardoj -- we'll carry on and treat this as a standalone issue then.

@stephanieelliott stephanieelliott added the External Added to denote the issue can be worked on by a contributor label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

@melvin-bot melvin-bot bot changed the title Workspace -App redirects to workspace editor with error when returning from not here page [$250] Workspace -App redirects to workspace editor with error when returning from not here page Aug 7, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

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

@dominictb
Copy link
Contributor

Proposal

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

In Step 4, app returns to workspace editor with the error "Invalid Policy ID".

What is the root cause of that problem?

In

if (!isAtLeastOneInState(state, SCREENS.WORKSPACE.INITIAL)) {
, we always push the WORKSPACE.INITIAL page into the route if it does not exist yet and we're accessing a workspace page.

So even when the workspace does not exist/is not accessible, we still do that, so when the user swipe from the not found more feature page, they will see the WORKSPACE.INITIAL page with the error.

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

In

if (!isAtLeastOneInState(state, SCREENS.WORKSPACE.INITIAL)) {
, only push the WORKSPACE.INITIAL page into the route if the workspace is valid and accessible.

The condition for "workspace is valid and accessible" that is checked by AccessOrNotFoundWrapper is here

So we can add to this

const policy = PolicyUtils.getPolicy(workspaceCentralPane?.params?.policyID);
const isPolicyNotAccessible = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id;
if (isPolicyNotAccessible) {
    return;
}

What alternative solutions did you explore? (Optional)

There could be a chance that when deeplinking, the above condition is evaluated before the policy data is loaded. To prevent false positive in that case we can add a check for isLoadingReportData like we did in

if (isLoadingReportData && isEmpty(policy) && isEmpty(policyDraft)) {
, so we only do not push workspace initial page to the route if the policy is not accessible and isLoadingReportData is false.

Then in AccessOrNotFoundWrapper if we detect that the policy is not found, we should dispatch to remove the workspace initial page from the navigation state. Pseudo-code:

navigationRef.dispatch((state) => {
  const routes = state.routes?.filter(item => item.name !== SCREENS.WORKSPACE.INITIAL);

  return CommonActions.reset({
    ...state,
    routes,
    index: routes.length < state.routes.length ? state.index - 1 : state.index,
  });
});

@shubham1206agra
Copy link
Contributor

Taking over here on request here. https://expensify.slack.com/archives/C02NK2DQWUX/p1723190106955179

@dominictb Can you post a test branch here?

@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2024
@dominictb
Copy link
Contributor

@shubham1206agra Of course, the test branch is https://github.com/dominictb/epsf-app/tree/fix/46646-test

Looking forward to your review 🙏

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Aug 12, 2024

@dominictb Your proposal works, but it looks unstable, as you have mentioned already. Your alternative solution cannot be adopted as we should not use navigationRef.dispatch in the component.

Copy link

melvin-bot bot commented Aug 12, 2024

@stephanieelliott, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot removed Help Wanted Apply this label when an issue is open to proposals by contributors Overdue labels Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

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

@stephanieelliott
Copy link
Contributor

Hey @dominictb seems like you were investigating a few things, what's the latest on this one?

@wildan-m
Copy link
Contributor

wildan-m commented Oct 9, 2024

@adamgrzybowski what's your opinion about my proposal?

#46646 (comment)

@dominictb
Copy link
Contributor

I'll provide update today.

@lakchote
Copy link
Contributor

I'll provide update today.

Could you please give the current state of the situation? Thanks!

@dominictb
Copy link
Contributor

I fixed the issue and posted an update in the PR.

@stephanieelliott
Copy link
Contributor

PR is still under review #49226

@stephanieelliott
Copy link
Contributor

Hey @dominictb last comment on the PR says that you're doing some testing -- can you give an update on where we are with that?

@dominictb
Copy link
Contributor

dominictb commented Oct 22, 2024

I already fixed the C+ found issue on large screen but discovered several other issues in deep linking scenario. There's an unsynchronization of data between Onyx.connect in utils file and useOnyx in components, so the solution might not work in some EDGE cases. Trying to fix them.

@stephanieelliott
Copy link
Contributor

PR is under active review

@stephanieelliott
Copy link
Contributor

Hey @shubham1206agra the PR is blocked on you, can you review?

@lakchote
Copy link
Contributor

lakchote commented Nov 6, 2024

friendly bump @shubham1206agra as the PR has been ready for review for a few days already.

@shubham1206agra
Copy link
Contributor

Sorry for the delay
I will do the review today

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 11, 2024
@melvin-bot melvin-bot bot changed the title [$250] Workspace -App redirects to workspace editor with error when returning from not here page [HOLD for payment 2024-11-18] [$250] Workspace -App redirects to workspace editor with error when returning from not here page Nov 11, 2024
Copy link

melvin-bot bot commented Nov 11, 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 Nov 11, 2024
Copy link

melvin-bot bot commented Nov 11, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.59-3 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-11-18. 🎊

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

Copy link

melvin-bot bot commented Nov 11, 2024

@shubham1206agra / @dominictb @stephanieelliott The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Nov 18, 2024
@stephanieelliott
Copy link
Contributor

stephanieelliott commented Nov 19, 2024

Summarizing payment on this issue:

Upwork job is here: https://www.upwork.com/jobs/~021859354157979620903

@melvin-bot melvin-bot bot removed the Overdue label Nov 19, 2024
@shubham1206agra
Copy link
Contributor

@stephanieelliott I think you need to pay me, not @lakchote (internal employee)

@lakchote
Copy link
Contributor

@stephanieelliott I think you need to pay me, not @lakchote (internal employee)

Exactly, @stephanieelliott please pay @shubham1206agra as I'm an internal employee

@stephanieelliott
Copy link
Contributor

Oh my gosh @lakchote, I'm sorry -- I know you are an internal employee 🤦‍♀️. I extended this to the wrong person, @shubham1206agra offer was extended to you https://www.upwork.com/nx/wm/offer/104996565

@stephanieelliott
Copy link
Contributor

All paid up!

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Polish
Development

No branches or pull requests

10 participants