Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.02k stars 3.01k forks source link

[$250] Deleting a workspace in offline mode the archived chat isn’t unpinned #55687

Open m-natarajan opened 2 weeks ago

m-natarajan commented 2 weeks ago

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: 9.0.89-2 Reproducible in staging?: yes Reproducible in production?: yes 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: @srikarparsi Slack conversation (hyperlinked to channel name): expensify-bug

Action Performed:

  1. Create a workspace
  2. Go to offline mode
  3. Delete the workspace

Expected Result:

The workspace chat is unpinned

Actual Result:

The workspace chat isn’t unpinned

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/369288a4-4e61-45a3-ab98-ac87fb85cb20

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021883982996350227662
  • Upwork Job ID: 1883982996350227662
  • Last Price Increase: 2025-01-27
  • Automatic offers:
    • DylanDylann | Reviewer | 106009739
    • allgandalf | Contributor | 106009740
Issue OwnerCurrent Issue Owner: @jasperhuangg
melvin-bot[bot] commented 2 weeks ago

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.

FitseTLT commented 2 weeks ago

Proposal

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

Deleting a workspace in offline mode the archived chat isn’t unpinned

What is the root cause of that problem?

We are not resetting isPinned here for the linked reports of the workspace https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/libs/actions/Policy/Policy.ts#L343-L349

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

We should optimistically set isPinned to false https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/libs/actions/Policy/Policy.ts#L343-L349 We will then revert the value to previous value on failureData

Note for the C+ about the below proposal: It claims that expense don't get unpinned on workspace deletion but that's wrong BE now unpins expenses too and additionally the proposal misses an important point of reverting the value on failureData 👍

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

We will add a test for deleteWorkspace to assert it resets isPinned of the related reports of the policy

What alternative solutions did you explore? (Optional)

allgandalf commented 2 weeks ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 04:46:24 UTC.

[!NOTE] To any C+ getting assigned please review my proposal here as well, as it is technically first in queue 😅

Also c.c. @mallenexpensify as the process is yet to be documented from here

Proposal

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

Workspace chat is not archived when we delete the policy in offline mode

What is the root cause of that problem?

We are not settingsisPinned to false optimistically when we delete the policy, this causes the report to stay pinned when deleted in offline state

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

We need to add isPinned: false below:

https://github.com/Expensify/App/blob/b16abc8b3059d365d755de8729a4794d1d30df02/src/libs/actions/Policy/Policy.ts#L349-L350

Alternatively we can safe check to see if report?.isPinned is true and only then set it to false

I also observed that if we pin a expense that also doesn't get unpinned, so we need to update that too below:

https://github.com/Expensify/App/blob/b16abc8b3059d365d755de8729a4794d1d30df02/src/libs/actions/Policy/Policy.ts#L415-L416

Alternatively we can safe check to see if report?.isPinned is true and only then set it to false

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

We can write a unit test which simulates deleting of workspace, then set optimistic data and see if after we call the deleteWorkspace then it also sets isPinned to false for reports present in that policy

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 1 week ago

@greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 week ago

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

greg-schroeder commented 1 week ago

Next up is proposal review

melvin-bot[bot] commented 1 week ago

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

DylanDylann commented 1 week ago

Both proposal has the same idea. Let's go with @allgandalf's proposal because they posted proposals first on Slack channel. The lack of failureData is so minor and we can address it in PR phase

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 1 week ago

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

mallenexpensify commented 1 week ago

@FitseTLT , apologies for the mix up here. We've asked the QA team to include proposals from bug links in the GH when it's created and they forgot to (I reminded them, after I found this, to do so)

FitseTLT commented 1 week ago

@FitseTLT , apologies for the mix up here. We've asked the QA team to include proposals from bug links in the GH when it's created and they forgot to (I reminded them, after I found this, to do so)

Yeah It was a bit painful for me 😄 No problem ❤

melvin-bot[bot] commented 5 days ago

@greg-schroeder, @jasperhuangg, @DylanDylann Huh... This is 4 days overdue. Who can take care of this?

greg-schroeder commented 3 days ago

Bump @jasperhuangg to confirm contributor assignment per this comment

melvin-bot[bot] commented 3 days ago

@greg-schroeder, @jasperhuangg, @DylanDylann Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 days ago

📣 @DylanDylann 🎉 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[bot] commented 2 days ago

📣 @allgandalf 🎉 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[bot] commented 2 days ago

@greg-schroeder @jasperhuangg @allgandalf @DylanDylann 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!