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
3.55k stars 2.9k forks source link

[HOLD https://github.com/Expensify/Auth/pull/12960] [$250] Workspace-As admin,opening expense details page in archived control WS directs to hmm not here #49088

Open izarutskaya opened 2 months ago

izarutskaya commented 2 months 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.33-1 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Launch app
  2. Create a workspace and invite a member as employee
  3. Tap on a category - gl code
  4. Upgrade to control workspace
  5. Enter gl and payroll code and save it
  6. From settings, select members must categorize all expenses
  7. In mweb, login as employee
  8. Open employee workspace chat
  9. Create a expense with big amount, future date
  10. As admin, delete this control workspace
  11. Note the expense details page in archived workspace of both employee and admin

Expected Result:

As admin, opening expense details page in archived control workspace must not direct to hmm not here .

Actual Result:

As admin, opening expense details page in archived control workspace directs to hmm not here but as employee can view expense.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/267f3c2a-765e-48df-9a4f-7404e69921a1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836148946035230114
  • Upwork Job ID: 1836148946035230114
  • Last Price Increase: 2024-09-24
  • Automatic offers:
    • shubham1206agra | Reviewer | 104114173
melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 1 month ago

@johncschuster Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

burczu commented 1 month ago

Hi, I’m Bartek from Callstack and I would like to investigate this issue.

shubham1206agra commented 1 month ago

@johncschuster Please assign @burczu here

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @shubham1206agra 🎉 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 1 month ago

@johncschuster @burczu @shubham1206agra 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!

burczu commented 1 month ago

Thanks for assigning me to the issue. I was able to reproduce the issue and from what I see for admin, when we try to access the expense details report, we get the Report not found error from the backend:

Screenshot 2024-09-27 at 11 23 01

So I suspect this issue may be related to the backend - especially we don't get this error for an employee, so it looks like the report is not accessible in circumstances described in this issue. I'll spend some more time digging deeper to check if there is something else on the frontend side that may cause this, but I didn't find anything promising yet.

johncschuster commented 1 month ago

Thanks, @burczu! I'll take it internal then!

johncschuster commented 1 month ago

@shubham1206agra are you able to work on this?

shubham1206agra commented 1 month ago

@johncschuster I am not an internal employee. Please apply hot pick to assign an engineer.

johncschuster commented 1 month ago

Thanks, @shubham1206agra! I've done that now.

johncschuster commented 1 month ago

Waiting on this to get picked up

melvin-bot[bot] commented 1 month ago

@johncschuster @burczu @shubham1206agra this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 1 month ago

@johncschuster, @burczu, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

johncschuster commented 1 month ago

Still waiting on this one to get picked up

trjExpensify commented 1 month ago

@srikarparsi curious for your input here, seems like another reason to execute this plan to not archive expense reports on workspace deletion: https://expensify.slack.com/archives/C036QM0SLJK/p1728047968818889?thread_ts=1728047962.737329&cid=C036QM0SLJK

Moving to the #quality GH project where the remaining "closed and archived" work has moved.

melvin-bot[bot] commented 1 month ago

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

johncschuster commented 1 month ago

Not overdue, Melvin. We're looking for some insight from @srikarparsi.

srikarparsi commented 1 month ago

@srikarparsi curious for your input here, seems like another reason to execute this plan to not archive expense reports on workspace deletion:

So it's kind of two different things in the code at least.

trjExpensify commented 4 weeks ago

I think this bug is happening because of the unsharing part.

Intereeesting. Are you saying these open expense reports remain on the workspace, but the admins/auditors of said workspace lose access to them? 🤔

johncschuster commented 3 weeks ago

Not overdue. Still discussing

johncschuster commented 3 weeks ago

@srikarparsi can you take a look at Tom's last message when you get a moment?

srikarparsi commented 3 weeks ago

Intereeesting. Are you saying these open expense reports remain on the workspace, but the admins/auditors of said workspace lose access to them? 🤔

Yup, exactly. This comment: Unshare all open reports from admins and auditors.

trjExpensify commented 3 weeks ago

Yeah, so I think the question comes down to:

Q: When a workspace is deleted, should the admins/auditors retain access to the open expense reports on the workspace to be able to move those somewhere else?

  1. If yes, then we shouldn't unshare them and allow the admins to Move report somewhere else (i.e another workspace that they're an admin of).
  2. If no, then we should continue to unshare them with anyone but the submitter (who can Move report elsewhere, including an option for the selfDM which would unreport the expenses).

I don't really see the downside of (1). Do you, @JmillsExpensify @twisterdotcom?

Another option might have been to automatically move the open expense reports off the workspace on deletion and to the member's individual workspace, but the notion of a "personal workspace" in NewDot doesn't exist. The closest equivalent would be the selfDM, but that would mean auto-unreporting all the expenses (as only unreported expenses exist in the selfDM), which I'm not sure a submitter would appreciate more than just keeping the open report intact and being able to move it elsewhere to another workspace or unreport the expenses on their own accord.

johncschuster commented 3 weeks ago

(Still discussing, Melvin)

twisterdotcom commented 2 weeks ago

I do prefer leaving the former admins with some access, and I don't see any problem with doing that.

melvin-bot[bot] commented 2 weeks ago

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

johncschuster commented 2 weeks ago

Discussing

trjExpensify commented 2 weeks ago

I do prefer leaving the former admins with some access, and I don't see any problem with doing that.

Cool, then I think we do (1) then. 👍

johncschuster commented 2 weeks ago

Sounds good! @srikarparsi, will you be taking this on?

melvin-bot[bot] commented 1 week ago

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

johncschuster commented 1 week ago

@trjExpensify I believe @srikarparsi's plate is pretty full at the moment. Should we get this issue redistributed, or can it wait until his bandwidth opens back up?

trjExpensify commented 1 week ago

Srikar isn't assigned in the first place, so it's available as a Hot pick in the meantime if someone can pick it up.

srikarparsi commented 1 week ago

Hey sorry! I started looking at this and there's a little more to unpack because we also unshare all archived reports from everyone except the owner here. I'm going to think about this tomorrow and might have a couple of questions about expected behavior but I think I can take this on.

srikarparsi commented 1 week ago

Ok, so we do two things in terms of un-sharing:

  1. When a policy is deleted, we un-share all open reports with admins and auditors.
  2. When a policy is deleted, we un-share all archived reports from everyone except the policy owner.

We're still archiving expense reports in the context of the code (they will still have the private_isArchived rNVP so that we can know to hide action buttons - submit, pay, etc. - in the frontend)

I know we had some other discussions about what reports should be accessible to who after a policy has been deleted. @trjExpensify or @twisterdotcom do you have any thoughts on this and I can move forward with implementing it?

trjExpensify commented 1 week ago

I'm not sure what exactly you're asking for thoughts on, but to summarise on this.

srikarparsi commented 1 week ago

I'm not sure what exactly you're asking for thoughts on

Sorry, the part I wanted thoughts on was who loses access to each type of report after a policy has been deleted. So for example right now I believe:

Owners: Lose access to all open expense reports Admins: Lose access to all archived reports Auditors: Lose access to all archived reports Members: Lose access to all archived reports

And based on what you said above, is expected that: Owners: Retain access to all reports Admins: Lose access to all archived chat and task reports (not expense reports) Auditors: Lose access to all archived chat and task reports (not expense reports) Members: Lose access to all archived chat and task reports (not expense reports)

Is this right? Sorry for not clarifying earlier.

trjExpensify commented 1 week ago

Right, we aren't looking to modify the existing behaviour of other report types on policy deletion. We're clarifying what to do about expense reports only with this.

srikarparsi commented 1 week ago

Cool started on a PR here

melvin-bot[bot] commented 4 days ago

@johncschuster, @burczu, @srikarparsi, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

johncschuster commented 3 days ago

Chill out, Melvin. A PR is in the works!

srikarparsi commented 8 hours ago

We are holding this for https://github.com/Expensify/Auth/pull/12960