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 2025-02-05] [$250] Workspace - User can navigate to deleted workspace editor while offline #54582

Closed
1 of 8 tasks
vincdargento opened this issue Dec 26, 2024 · 37 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

@vincdargento
Copy link

vincdargento commented Dec 26, 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: v9.0.78-4
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team

Action Performed:

  1. Create a workspace
  2. Navigate to the workspace and enable workflows
  3. Go to connect bank account and copy the URL
  4. Go offline
  5. Delete the workspace
  6. Send the copied URL in any chat
  7. Click on it to navigate to the editor
  8. Change any value while offline

Expected Result:

User should not be able to navigate to workspace editor after the workspace has been deleted (as in PR #27743)

Actual Result:

User is able to navigate to workspace editor in a deleted workspace while offline

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021873785256425821183
  • Upwork Job ID: 1873785256425821183
  • Last Price Increase: 2025-01-13
  • Automatic offers:
    • rayane-djouah | Reviewer | 105681465
    • FitseTLT | Contributor | 105681467
Issue OwnerCurrent Issue Owner: @slafortune
@vincdargento vincdargento added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 26, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Dec 26, 2024

Edited by proposal-police: This proposal was edited at 2024-12-26 19:04:03 UTC.

Proposal

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

User is able to navigate to workspace editor in a deleted workspace while offline

What is the root cause of that problem?

The condition to display not found page now doesn't check the policy is pending delete

if (!isLoading && (isEmptyObject(policy) || !PolicyUtils.isPolicyAdmin(policy))) {
return (
<ScreenWrapper testID={ReimbursementAccountPage.displayName}>
<FullPageNotFoundView

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

  1. We should display not found page if the policy is pending delete
if (!isLoading && (isEmptyObject(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy))) {

if (!isLoading && (isEmptyObject(policy) || !PolicyUtils.isPolicyAdmin(policy))) {
return (
<ScreenWrapper testID={ReimbursementAccountPage.displayName}>
<FullPageNotFoundView

  1. Optional: If we want to fix other workspace pages, we should implement the shouldShow logic here to return true if the policy is pending delete. We should add this logic back PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy) that will prevent not found page display briefly after we delete the workspace
return (!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) || (!shouldShowPolicy && !prevShouldShowPolicy) || (PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy));

const shouldShow = useMemo(() => {
// If the policy object doesn't exist or contains only error data, we shouldn't display it.
if (((isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
return true;
}

Or we can pass isOffline param as false here then this function will return false if the policy is pending delete

const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, false, currentUserLogin), [policy, currentUserLogin]);
const prevShouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(prevPolicy, false, currentUserLogin), [prevPolicy, currentUserLogin]);

const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);

const prevShouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(prevPolicy, isOffline, currentUserLogin), [prevPolicy, isOffline, currentUserLogin]);

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

What alternative solutions did you explore? (Optional)

NA

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.

@twilight2294
Copy link
Contributor

Proposal

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

User can navigate to deleted workspace editor while offline

What is the root cause of that problem?

The actual cause is that we allow user to access the workspace via deep linking even when the policy is pending the delete action:

const shouldShow = useMemo(() => {
// If the policy object doesn't exist or contains only error data, we shouldn't display it.
if (((isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
return true;
}

This causes the pages to show when the pending action is delete

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

We need to add a extra check in shouldShow such that it will block the whole screen of the policy when the action is pending delete too:

    const shouldShow = useMemo(() => {
        // If the policy object doesn't exist or contains only error data, we shouldn't display it.
        if (((isEmptyObject(policy) || PolicyUtils.isPendingDeletePolicy(policy) ||(Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
            return true;
        }

const shouldShow = useMemo(() => {
// If the policy object doesn't exist or contains only error data, we shouldn't display it.
if (((isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
return true;
}

We also need to update shouldShowPolicy because we need to display the not found text, this will be set to false:

subtitleKey={shouldShowPolicy ? 'workspace.common.notAuthorized' : undefined}

Note that the whole policy options need to be blocked here and not just the bank option as in delete case, the more features/ categories show a not found page:

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

N/A this is a UI bug

What alternative solutions did you explore? (Optional)

@nkdengineer
Copy link
Contributor

Updated proposal to fix other places if we want to fix this here.

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 26, 2024

Edited by proposal-police: This proposal was edited at 2024-12-26 22:40:30 UTC.

Proposal

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

Workspace - User can navigate to deleted workspace editor while offline

What is the root cause of that problem?

We already show not found page for pending delete for both WorkspaceInitialPage

const shouldShowNotFoundPage = isEmptyObject(policy) || (!shouldShowPolicy && !prevShouldShowPolicy);

and WorkspacePageWithSections
return (!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) || (!shouldShowPolicy && !prevShouldShowPolicy);

but that only happens in online mode b/c shouldShowPolicy returns true for pending delete workspaces for offline case

App/src/libs/PolicyUtils.ts

Lines 218 to 223 in 12e0941

function shouldShowPolicy(policy: OnyxEntry<Policy>, isOffline: boolean, currentUserLogin: string | undefined): boolean {
return (
!!policy?.isJoinRequestPending ||
(!!policy &&
policy?.type !== CONST.POLICY.TYPE.PERSONAL &&
(isOffline || policy?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || Object.keys(policy.errors ?? {}).length > 0) &&

this behaviour is correct for WorkspaceListPage here because we want to show the pending delete workspace in offline mode but not in these places.

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

We should first rename isOffline param of shouldShowPolicy to shouldShowPendingDeleteWorkspace then we pass false for the shouldShowPolicy we are using in

const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);
const prevShouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(prevPolicy, isOffline, currentUserLogin), [prevPolicy, isOffline, currentUserLogin]);

and additionally here to display not found for the case of worspace inital setting page
const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);
const prevShouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(prevPolicy, isOffline, currentUserLogin), [prevPolicy, isOffline, currentUserLogin]);

And additionally for WorkspacePageWithSections there is a problem of offline indicator showing on the top of the page for small screen (because we shouldForceFullScreen that will make the view fixed position and it distorts the normal display of the offline indicator on the bottom of the screen) so as we don't show the wide screen offline indicator when not found page is displayed here we can make it consistent with it and disable the small screen offline indicator when displaying not found page so add

            shouldShowOfflineIndicator={!shouldShow}

but if we want to show offline indicator when displaying not found page in WorkspacePageWithSections like we do in WorkspaceInitialPage we can make the shouldForceFullScreen here to apply only to large screens (!shouldUseNarrowLayout or !isSmallScreenWidth) because we don't need it for small screen and the offline indicator will display in the bottom correctly.
BTW there are other policy pages that are not currently displayed under WorkspacePageWithSections like PolicyAccountingPage, WorkspaceCategoriesPage, WorkspaceMoreFeaturesPage, WorkspaceTagsPage, WorkspaceTaxesPage, WorkspaceReportFieldsPage, WorkspacePerDiemPage, PolicyDistanceRatesPage and PolicyRulesPage, WorkspaceInvitePage, ReimbursementAccountPage and many others so we should make consistent fixes there too whether via WorkspacePageWithSections or applying a not found component (or AccessOrNotFoundWrapper) independently 👍

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

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

@slafortune slafortune added the External Added to denote the issue can be worked on by a contributor label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

@melvin-bot melvin-bot bot changed the title Workspace - User can navigate to deleted workspace editor while offline [$250] Workspace - User can navigate to deleted workspace editor while offline Dec 30, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

Copy link

melvin-bot bot commented Jan 3, 2025

@slafortune, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Jan 3, 2025
@rayane-d
Copy link
Contributor

rayane-d commented Jan 3, 2025

Will review today

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2025
Copy link

melvin-bot bot commented Jan 6, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Jan 6, 2025
@rayane-d
Copy link
Contributor

rayane-d commented Jan 6, 2025

Hi all,

I believe we need to address this issue across all workspace pages (e.g., WorkspaceProfilePage, WorkspaceCategoriesPage, etc.).

Many workspace pages utilize the AccessOrNotFoundWrapper component, which includes logic for displaying a "not found" page when certain conditions are met:

const isPolicyNotAccessible = !PolicyUtils.isPolicyAccessible(policy);
const shouldShowNotFoundPage = (!isMoneyRequest && !isFromGlobalCreate && isPolicyNotAccessible) || !isPageAccessible || !isPolicyFeatureEnabled || shouldBeBlocked;

To handle workspaces that are pending deletion, I propose modifying PolicyUtils.isPolicyAccessible to return false for such cases.

Additionally, could someone clarify the distinction between PolicyUtils.shouldShowPolicy and PolicyUtils.isPolicyAccessible? It would also be helpful to discuss how we can standardize and correct their usage throughout the codebase.

@melvin-bot melvin-bot bot removed the Overdue label Jan 6, 2025
@FitseTLT
Copy link
Contributor

FitseTLT commented Jan 6, 2025

@rayane-djouah Yep that's what I suggested in my proposal we can indeed use AccessOrNotFoundWrapper with the details to be thought on PR phase.

shouldShowPolicy: we should use it to determine if the workspace can be shown in the workspace list page so we show pending deleted policies in offline mode and if the policy has errors or If it is join request pending and so on.

isPolicyAccessible: we are using it to check the policy is not empty has proper id and has more than one keys or has no errors so we can incorporate the new logic of disallowing pending delete policies in it. We use it in AccessOrNotFoundWrapper and to determine if we should push the workspace initial page on the nav state here

if (!isAtLeastOneInState(state, SCREENS.WORKSPACE.INITIAL)) {
if (isNarrowLayout && !isLoadingReportData && !isPolicyAccessible) {
return;
}

So they have different purposes and regarding not found page for non-accessible policies isPolicyAccessible is the most appropriate function as shouldShowPolicy allows pending request workspaces so we can update isPolicyAccessible logic to exclude pending delete workspaces and use it in AccessOrNotFoundWrapper to cover a lot of workspace pages and also in other pages like WorkspacePageWithSections.tsx as needed. We can also consider a logic similar to getActivePolicies by adding more checks like the existence of role for the user on policy and other manadatory props like name on the policy to make it more strict. But we can work on the detail in the PR phase 👍

@nkdengineer
Copy link
Contributor

nkdengineer commented Jan 7, 2025

@rayane-djouah AccessOrNotFoundWrapper is initialized here and seems to build the condition for isPolicyAccessible function instead of using the existing logic shouldShowPolicy function. That is an inconsistency we also need to fix and we can handle this in the PR phrase. Besides some workspace pages like WorkspaceMembersPage don't use AccessOrNotFoundWrapper so we need to fix both AccessOrNotFoundWrapper and WorkspacePageWithSections, other pages like ReimbursementAccountPage that doesn't use AccessOrNotFoundWrapper or WorkspacePageWithSections we can fix case by case by using the same check.

Copy link

melvin-bot bot commented Jan 9, 2025

@slafortune @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Jan 9, 2025
@FitseTLT
Copy link
Contributor

PR will be ready tomorrow

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 21, 2025
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 29, 2025
@melvin-bot melvin-bot bot changed the title [$250] Workspace - User can navigate to deleted workspace editor while offline [HOLD for payment 2025-02-05] [$250] Workspace - User can navigate to deleted workspace editor while offline Jan 29, 2025
Copy link

melvin-bot bot commented Jan 29, 2025

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 29, 2025
Copy link

melvin-bot bot commented Jan 29, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.90-6 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 2025-02-05. 🎊

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

Copy link

melvin-bot bot commented Jan 29, 2025

@rayane-djouah @slafortune @rayane-djouah 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]

@rayane-d
Copy link
Contributor

rayane-d commented Feb 3, 2025

BugZero Checklist:

  • Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:

Regression Test Proposal


#### Test:

1. Create a workspace
2. Navigate to the workspace and enable workflows
3. Go to connect bank account and copy the URL
4. Go offline
5. Delete the workspace
6. Send the copied URL in any chat
7. Click on it to navigate to the editor
8. Verify that the not found page is displayed
9. Navigate to other workspace setting pages for the deleted workspace and Verify that not found page is displayed


Do we agree 👍 or 👎

Copy link

melvin-bot bot commented Feb 3, 2025

📣 @rayane-d! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 4, 2025
@dylanexpensify dylanexpensify moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Feb 4, 2025
@melvin-bot melvin-bot bot added the Overdue label Feb 6, 2025
@slafortune
Copy link
Contributor

@rayane-d can you please accept the offer - https://www.upwork.com/nx/wm/offer/105681465

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2025
@rayane-d
Copy link
Contributor

rayane-d commented Feb 6, 2025

@slafortune - My eligibility for NewDot payments began on December 28, 2024, and I was assigned to this issue on December 30 #54582 (comment). Please provide a payment summary for me; I will request on NewDot

@slafortune
Copy link
Contributor

OH! I looked at those dates backwards! Thanks for calling it out.

@slafortune
Copy link
Contributor

@FitseTLT Role Contributor - was paid $250 via Upwork
@rayane-djouah Role C+ - requires payment via NewDot - owed $250

@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Feb 6, 2025
@slafortune
Copy link
Contributor

Upworks offer withdrawn

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
Status: Done
Development

No branches or pull requests

7 participants