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-12-30] [$250] Room - Deleted workspace appears in workspace list during room creation flow #53315

Closed
8 tasks done
IuliiaHerets opened this issue Nov 29, 2024 · 34 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

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 29, 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.68-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  • Account only has one workspace.
  • Invoices feature is enabled.
  1. Go to staging.new.expensify.com
  2. Go to FAB > Send invoice.
  3. Send an invoice to another user.
  4. As invoice receiver, open the invoice room and pay the invoice via Pay with business.
  5. Go back to the invoice sender.
  6. Delete the workspace.
  7. Create a new workspace.
  8. Go to FAB > Start chat > Room.
  9. Click Workspace.
  10. Note that the deleted workspace is still present in the workspace list.
  11. Select the deleted workspace and create a room.

Expected Result:

In Step 10, the deleted workspace should not appear in the workspace list in room creation flow.

Actual Result:

In Step 10, the deleted workspace appears in the workspace list in room creation flow.
When room is created from the deleted workspace, error shows up.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6679809_1732872707026.20241129_171506.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021863656321139042808
  • Upwork Job ID: 1863656321139042808
  • Last Price Increase: 2024-12-02
  • Automatic offers:
    • shubham1206agra | Contributor | 105188027
    • FitseTLT | Contributor | 105188077
Issue OwnerCurrent Issue Owner: @shubham1206agra
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 29, 2024
Copy link

melvin-bot bot commented Nov 29, 2024

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

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Nov 29, 2024

Proposal

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

Deleted workspace appears in workspace list during room creation flow

What is the root cause of that problem?

When we try to get active policies we call PolicyUtils.getActivePolicies(policies),

const workspaceOptions = useMemo(
() =>
PolicyUtils.getActivePolicies(policies)

getActivePolicieschecks if!!policy.name && !!policy.id` exists, and returns policies that have !!policy.name && !!policy.id
function getActivePolicies(policies: OnyxCollection<Policy> | null): Policy[] {
return Object.values(policies ?? {}).filter<Policy>(
(policy): policy is Policy => !!policy && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !!policy.name && !!policy.id,

A policy will have name, and id prop when it has an invoice report. Hence workspaceOptions will include the deleted policy

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

We should check if policy.type exists too since the prop is available for active workspaces. Or optionally we can use policy.ownerAccountID or policy.role here

...!!policy.name && !!policy.id && !!policy.type

What alternative solutions did you explore? (Optional)

we can use PolicyUtils.shouldShowPolicy to get active policies instead of PolicyUtils.getActivePolicies(policies) as we are doing in WorkspaceListpage

PolicyUtils.shouldShowPolicy(policy, !!isOffline, currentUserLogin)?.filter..

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 29, 2024

Proposal

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

Room - Workspace from the other user appears in workspace list during room creation flow
Note that the deleted workspace is not appearing in the room creation flow you can clearly see from the OP that the workspace appearing in room creation flow is Applause's Workspace 6 while the deleted workspace is Applause's Workspace so the workspace appearing is a workspace from the other user that the current user is not a member.

What is the root cause of that problem?

When a user A has sent an invoice to another user and the other user B paid the invoice but User A deletes this workspace the invoice will be transferred to another workspace of user B where user A is not a member and this workspace where user A is not a member is returned in the policies list from the BE and on room creation flow we are displaying it as we only check for pending delete, policy id and name existence

function getActivePolicies(policies: OnyxCollection<Policy> | null): Policy[] {
return Object.values(policies ?? {}).filter<Policy>(
(policy): policy is Policy => !!policy && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !!policy.name && !!policy.id,
);
}

So when we create a room with it the BE will return an error as the current user is not a member of the workspace
image

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

Here

function getActivePolicies(policies: OnyxCollection<Policy> | null): Policy[] {
return Object.values(policies ?? {}).filter<Policy>(
(policy): policy is Policy => !!policy && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !!policy.name && !!policy.id,
);
}

we need to check that the current user is a member of the workspace (by checking the existence of role for the current user) same as we are doing here

function getActivePolicies(policies: OnyxCollection<Policy> | null, currentUserLogin): Policy[] {
    return Object.values(policies ?? {}).filter<Policy>(
        (policy): policy is Policy =>
            !!policy && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !!policy.name && !!policy.id && !!getPolicyRole(policy, currentUserLogin),
    );
}

we can then pass the session?.email as the current user login but even if we don't pass it will check for policy.role which should exist

Note on the above proposal:

  1. the above proposal didn't identify the right problem and RCA
  2. we can use PolicyUtils.shouldShowPolicy to get active policies instead of PolicyUtils.getActivePolicies(policies) as we are doing in WorkspaceListpage

using shouldShowPolicy instead of getActivePolicies will allow pending deletion workspaces when offline to be included in the workspaces list as we normally use shouldShowPolicy to display in workspace list page where we display pending deletion workspaces when offline with strikethrough applied. 👍

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2024
@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Dec 2, 2024
@melvin-bot melvin-bot bot changed the title Room - Deleted workspace appears in workspace list during room creation flow [$250] Room - Deleted workspace appears in workspace list during room creation flow Dec 2, 2024
Copy link

melvin-bot bot commented Dec 2, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 2, 2024
Copy link

melvin-bot bot commented Dec 2, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Dec 2, 2024
@zanyrenney
Copy link
Contributor

hey there @rayane-djouah please could you review the proposals above if either are suitable?

Thank you!

@etCoderDysto
Copy link
Contributor

A kind reminder.

My proposal already suggests to check for policy.role since active policy will have a value of policy.role = user for a member and policy.role = admin for admin. getPolicyRole checks for policy.role and policy?.employeeList?.[currentUserLogin ?? '-1']?.role which doesn't exist in this case. Therefore there is no upside to using getPolicyRole.

App/src/libs/PolicyUtils.ts

Lines 197 to 199 in 473822d

function getPolicyRole(policy: OnyxInputOrEntry<Policy> | SearchPolicy, currentUserLogin: string | undefined) {
return policy?.role ?? policy?.employeeList?.[currentUserLogin ?? '-1']?.role;
}

The wrong workspace data
Screenshot 2024-12-02 at 10 31 23 at night

Member
Screenshot 2024-12-02 at 10 23 52 at night
Admin
Screenshot 2024-12-02 at 10 22 07 at night

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 2, 2024

You have identified neither the correct problem nor the correct RCA therefore you haven't backed your solution linking to the correct RCA of the problem. You were just randomly giving several policy props as an option without any logical explanation so it is not acceptable and the reviewer will give the proper review so let's just wait for that.

@etCoderDysto
Copy link
Contributor

No more review seems to needed since you have already done the review on your proposal😆.

the above proposal didn't identify the right problem and RCA

@cristipaval cristipaval self-assigned this Dec 4, 2024
@shubham1206agra
Copy link
Contributor

Commenting here as per slack convo

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 4, 2024
@cristipaval
Copy link
Contributor

Assigning @shubham1206agra as the C+ since @rayane-djouah hasn't reviewed the proposals yet. We chatted here about the issue.

@Expensify Expensify deleted a comment from melvin-bot bot Dec 4, 2024
@cristipaval cristipaval added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 4, 2024
@shubham1206agra
Copy link
Contributor

I have discussed this with @cristipaval about this. And we have decided to assign @etCoderDysto's proposal.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Dec 4, 2024

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

@shubham1206agra
Copy link
Contributor

@FitseTLT and @etCoderDysto Fair warning to both of you. Be civil in the discussion. Otherwise, it will result in permanent ban from here.

@cristipaval
Copy link
Contributor

ah, no, sorry, my bad in that Slack channel. I wanted to actually point to @FitseTLT proposal.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 23, 2024
@melvin-bot melvin-bot bot changed the title [$250] Room - Deleted workspace appears in workspace list during room creation flow [HOLD for payment 2024-12-30] [$250] Room - Deleted workspace appears in workspace list during room creation flow Dec 23, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 23, 2024
Copy link

melvin-bot bot commented Dec 23, 2024

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

Copy link

melvin-bot bot commented Dec 23, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.77-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 2024-12-30. 🎊

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

Copy link

melvin-bot bot commented Dec 23, 2024

@shubham1206agra @zanyrenney @shubham1206agra 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 Dec 30, 2024
Copy link

melvin-bot bot commented Jan 2, 2025

@cristipaval, @FitseTLT, @zanyrenney, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

@zanyrenney
Copy link
Contributor

@shubham1206agra please complete the checklist in the comment here so I can issue payout -- #53315 (comment)

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

Thank you!

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

bump @shubham1206agra

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

I hve DM'd @shubham1206agra

@shubham1206agra
Copy link
Contributor

BugZero Checklist:

  • [Contributor] 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 (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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: https://github.com/Expensify/App/pull/36178/files#r1904109986

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: NA

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

  • Account only has one workspace.
  • Invoices feature is enabled.

Test:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Send invoice.
  3. Send an invoice to another user.
  4. As invoice receiver, open the invoice room and pay the invoice via Pay with business.
  5. Go back to the invoice sender.
  6. Go to FAB > Start chat > Room.
  7. Click Workspace.
  8. Observe that only one workspace is present in the workspace list.

Do we agree 👍 or 👎

@shubham1206agra
Copy link
Contributor

@zanyrenney Can you send the offer again?

Copy link

melvin-bot bot commented Jan 13, 2025

@cristipaval, @FitseTLT, @zanyrenney, @shubham1206agra Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Jan 13, 2025
@zanyrenney
Copy link
Contributor

@zanyrenney
Copy link
Contributor

offer sent @shubham1206agra

@shubham1206agra
Copy link
Contributor

@zanyrenney Offer accepted

@melvin-bot melvin-bot bot removed the Overdue label Jan 13, 2025
@zanyrenney
Copy link
Contributor

payment summary

$250 paid to @FitseTLT for their role via upwork.
$250 paid to @shubham1206agra via upwork.

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
None yet
Development

No branches or pull requests

7 participants