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.53k stars 2.88k forks source link

[$500] Task - User C appears on User B's LHN when task assigned to User B is changed to User C by User A #28189

Closed lanitochka17 closed 1 year ago

lanitochka17 commented 1 year 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!


Action Performed:

  1. Go to staging.new.expensify.com
  2. As User A, create a task and assign it to User B
  3. As User A, go to the task report and change the assignee from User B to User C
  4. As User B, go to LHN

Expected Result:

User C will not appear in User B's LHN

Actual Result:

User C appears in User B's LHN and it disappears with not here page when User B opens User C's report

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.74-0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/a064a018-d660-4d0b-86e7-398caa98b2d5

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c82bc1ac5dd8430a
  • Upwork Job ID: 1709612411890421760
  • Last Price Increase: 2023-10-04
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

trjExpensify commented 1 year ago

@thienlnam can you clarify the behaviour here to understand how this should work?

Is that how it works?

thienlnam commented 1 year ago

Ah close but the taskReport is independent of these threads

A task was assigned to userB

The task gets re-assigned to userC

UserB should not lose access to the taskReport

I think the issue here is that UserB should not be losing access to that task report

trjExpensify commented 1 year ago

Oh, so taskReports don't have a parentChat like expense/iou/threadReports do?

What reason does userB have to still be shared the taskReport after being unassigned, and has this ever worked, so it's deemed a bug?

thienlnam commented 1 year ago

No they still have a parent like those reports - I might have been confused on the

the taskReport created is a thread on the userA <> userB DM

What reason does userB have to still be shared the taskReport after being unassigned

A task is basically a thread with a completion state, so if they participate in the report at all they would still have access to it.

and has this ever worked, so it's deemed a bug?

Yeah this should be working already so it would be a bug

trjExpensify commented 1 year ago

No they still have a parent like those reports - I might have been confused on the

Cool, that's an accurate statement though, right? The taskReport has a parentChat and that's the DM between userA <> userB, like an expense/iou/chat report are considered threads of the parent.

  1. A task is created by userA and assigned to userB
  2. A taskReport is created, with the userA <> userB DM as the parentChat
  3. userA re-assigns the task to userC and that shares the taskReport with userC
  4. The taskReport remains as a thread of the userA <> userB DM, shared with userB, and despite now being assigned to userC.

A task is basically a thread with a completion state, so if they participate in the report at all they would still have access to it.

Are you saying then if a task is assigned to userB, but they don't participate in the taskReport (i.e comment, or edit it) then the wouldn't have access after being unassigned?

thienlnam commented 1 year ago

Yes that's correct - the taskReport has a parent (chat or task) and it is where-ever the task was created

The taskReport remains as a thread of the userA <> userB DM, shared with userB, and despite now being assigned to userC.

Yup, but in this situation there is another parent of the user A <> userC DM that links to the taskReport, but the true 'parent' of the task still remains in the reportAction from the userA <> userB DM. Let me know if that part makes sense as it is kind of confusing. There are many links to the taskReport depending on how many times you re-assign it, but the parent will always be the original reportAction that created the task

Are you saying then if a task is assigned to userB, but they don't participate in the taskReport (i.e comment, or edit it) then the wouldn't have access after being unassigned?

Ah no, they would still have access from being the assignee. We don't ever unshare the task report with someone once they have access to it

trjExpensify commented 1 year ago

Cool, I think we need to consider updating the parentChat when the task is reassigned. Here's a good example of why when it comes to the logic being updated wrt to the GBR: https://expensify.slack.com/archives/C02MW39LT9N/p1695898060546159?thread_ts=1695897345.190449&cid=C02MW39LT9N

trjExpensify commented 1 year ago

Coming back to this. @jack, did you review the linked convo? For this issue I think this is where I'm at:

  1. The DM between userA <> userC shouldn't be popping up in the LHN of userB like in the OP. That's a bug in itself to resolve in the current implementation
  2. The parentChat not being the DM shared between the task creator and current assignee isn't a solution that will scale with the wider notion of the GBR being added to the "higher context" chat, so we should rethink how to solve that.
thienlnam commented 1 year ago

Took a brief look, but yeah this seems like it will take a larger conversation as updating the parentChat everytime the task is reassigned might have some other unintended consequences. I'm mostly OOO this week, but can start a convo next week about this

(Assigning to myself for tracking)

trjExpensify commented 1 year ago

Cool, so let's move this issue on for proposals to resolve this one in the current imp:

The DM between userA <> userC shouldn't be popping up in the LHN of userB like in the OP. That's a bug in itself to resolve in the current implementation

Then yeah, let's talk about this one separately somewhere. How are we going to align taskReports with this notion of the higher context chat getting the GBR when that chat could very well not be one the assignee has access to.

The parentChat not being the DM shared between the task creator and current assignee isn't a solution that will scale with the wider notion of the GBR being added to the "higher context" chat, so we should rethink how to solve that.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

tsa321 commented 1 year ago

Maybe back end issue? There is pusher event for user B with onyx method set of a report/chat between user A and user C that user B mustn't know about it.

trjExpensify commented 1 year ago

Perhaps! @thienlnam might be able to confirm.

melvin-bot[bot] commented 1 year ago

@trjExpensify @thienlnam @robertKozik 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[bot] commented 1 year ago

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

trjExpensify commented 1 year ago

waiting on @thienlnam to confirm this.

thienlnam commented 1 year ago

Yeah I think that might be the case - seems like we're sending the new chat report to all participants but should not be sending it to people that are not included in the chat

melvin-bot[bot] commented 1 year ago

Current assignee @robertKozik is eligible for the Internal assigner, not assigning anyone new.

thienlnam commented 1 year ago

I can get a PR up for it this week

trjExpensify commented 1 year ago

Sounds good!

trjExpensify commented 1 year ago

Oh, should the reviewing label be on this?

thienlnam commented 1 year ago

Yup, PR has been merged now just pending deploy to see if it fixes this issue

trjExpensify commented 1 year ago

Dope!

melvin-bot[bot] commented 1 year ago

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

thienlnam commented 1 year ago

Should be resolved now, can we see if it's still a problem and then close if not?

trjExpensify commented 1 year ago

@lanitochka17 can we have the team retest this one please? Thanks!

lanitochka17 commented 1 year ago

@tjferriss Issue is not reproducible on latest build 1.3.90-1

https://github.com/Expensify/App/assets/78819774/2f4a5203-6fcb-4dae-ae3d-465f1ad8b4f8

trjExpensify commented 1 year ago

Excellent - thanks for that! As this was reported by Applause and a backend PR, closing it out!