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.51k stars 2.87k forks source link

[$250] Task – GBR present in task details LHN when assign task to yourself as another group user #46883

Closed lanitochka17 closed 1 month ago

lanitochka17 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: .0.17-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4825045 Email or phone of affected tester (no customers): applausetester+jp_e_category_2@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in as user A
  3. Open group chat with user B
  4. Create a task without assignee
  5. Login as user B
  6. Open group chat with user A
  7. Open task from step 4
  8. Assign task for me
  9. Observe LHN

Expected Result:

GBR is not present in task details LHN, but only in group chat

Actual Result:

GBR present in task details LHN. GBR disappears when open task details again

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/5d160e46-e2ce-40be-be81-4059020a927f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0192b586199303ff17
  • Upwork Job ID: 1821860715015328595
  • Last Price Increase: 2024-09-06
Issue OwnerCurrent Issue Owner: @brunovjk
melvin-bot[bot] commented 2 months ago

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.

lanitochka17 commented 2 months ago

We think that this bug might be related to #vip-vsp

lanitochka17 commented 2 months ago

@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

zanyrenney commented 2 months ago

Sure, I think this is Low for VIP VSP.

zanyrenney commented 2 months ago

Added to the project board and added external.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

brunovjk commented 2 months ago

Still looking for proposals.

zanyrenney commented 2 months ago

can you still repro this @brunovjk I can't ?

ardent-dev commented 2 months ago

Proposal

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

The problem is that the GBR is incorrectly displayed when a task from a group chat is updated with assignee details. After the assignee is updated, the GBR for the task should disappear, leaving only the GBR for the group chat. However, the GBR continues to display until the task is reopened.

What is the root cause of that problem?

The root cause of this issue lies in how editing a task assignment updates the lastReadTime of the relevant task report. Report entries in the LHN use the ReportUtils.requiresAttentionFromCurrentUser function:

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/libs/ReportUtils.ts#L2559

to determine whether a GBR should be shown. Notably for this issue, the task report is being flagged as being in an 'unread with mention' state by ReportUtils.isUnreadWithMention:

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/libs/ReportUtils.ts#L2539-L2546

After a user assigns a task to themselves by editing the task, lastMentionedTime is set as expected to the time the task assignment modal closes (as is expected as the assignment resulted in a mention), however lastReadTime is not updated correctly (it remains at an old value from older activity when the task page was first opened). This causes isUnreadWithMention to flag the report entry as unread (as lastReadTime < lastMentionedTime).

lastReadTime is not correctly set due to two independent issues when handling a user selecting themselves as an assignee in the TaskAssigneeSelectorModal:

  1. On assignee selection, the TaskUtils.setAssigneeValue routine is first run when the task report already exists (i.e. in the case the task is being edited):

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/pages/tasks/TaskAssigneeSelectorModal.tsx#L190-L196

The two relevant parameters are on Line 194 and 195 in the previous code snippet (undefined is passed as the chatReport value and isCurrentUser resolves to true). This will result in the returned assigneeChatReport value being undefined.

  1. When subsequently moving on to TaskUtils.editTaskAssignee, session?.accountID is incorrectly passed as the ownerAccountID (this should be set to the owner of the task report as opposed to the currently logged in session):

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/pages/tasks/TaskAssigneeSelectorModal.tsx#L199

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/libs/actions/Task.ts#L571

Both of these issues have the effect of skipping this logic (in the case where the assignee is being set to the current user):

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/libs/actions/Task.ts#L664

which implicitly updates the lastReadTime to the current time:

https://github.com/Expensify/App/blob/6e61b1acc7e5e3ab6214cd16925ea84b3b918fb8/src/libs/ReportUtils.ts#L6797-L6801

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

2 minor changes are needed to

https://github.com/Expensify/App/blob/cfa84e21c1a5540f798f3fdfd76ecbd64a546704/src/pages/tasks/TaskAssigneeSelectorModal.tsx#L199

  1. Instead of session?.accountID, pass report.ownerID to correctly set the ownerAccountID value.
  2. Default the assigneeChatReport value (post invocation of setAssigneeValue) to report.

What alternative solutions did you explore? (Optional)

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.

melvin-bot[bot] commented 2 months ago

📣 @ardent-dev! 📣 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>
ardent-dev commented 2 months ago

Contributor details Your Expensify account email: ardent.digital.au@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~012c16fe40faf63452

melvin-bot[bot] commented 2 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

brunovjk commented 2 months ago

can you still repro this @brunovjk I can't ?

Yes I do @zanyrenney. Reviewing proposal now.

repro_46883_v9.0.20-6 https://github.com/user-attachments/assets/233eff65-7a2f-4745-9a4c-6da72ce446dd

Edit: Although the proposal's RCA seems to make sense to me, I have not been able to confirm yet. I will get back to it tomorrow morning.

melvin-bot[bot] commented 2 months ago

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

brunovjk commented 2 months ago

Thank you for the proposal, @ardent-dev. Although the lastReadTime proposed update clears the GBR:

https://github.com/user-attachments/assets/2603bdf4-c792-404f-a11c-563625a27b0d

I'm wondering what is causing it to appear in the first place. If your tests yielded different results, could you please share a test branch or any additional insights? 😄

ardent-dev commented 2 months ago

@brunovjk Great question, it appears to be because lastMentionedTime is erroneously being set, but I can't see where this is happening. The best I can see is that network responses from the EditTaskAssignee BE API contain the lastMentionedTime, however I can't see whether this is originating from FE or being calculated on backend.

image
brunovjk commented 2 months ago

I've applied the proposed changes, and ReadNewestAction is properly called, which successfully removes the GBR, but we are wondering what causes the GBR to appear in the first place when the response for assigning a task to yourself is received. I'll trigger an internal check to confirm if the BE codebase might be causing this issue. Thank you.

🎀👀🎀

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@rlinoz @zanyrenney @brunovjk 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!

rlinoz commented 2 months ago

Hmmm apparently we are not updating the lastReadTime for that user, we should not have to call ReadNewestAction, it should just come in the response the same way it does for AddComment.

I will try to raise a PR for this soon.

brunovjk commented 2 months ago

Awesome! Thank you @rlinoz!

ardent-dev commented 2 months ago

Thanks @brunovjk, can I confirm then that the FE changes as proposed will not be required?

rlinoz commented 2 months ago

Just put a PR up, and from what I was testing I don't think we will need to change things in the FE.

melvin-bot[bot] commented 2 months ago

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

brunovjk commented 2 months ago

Update: Working in a BE solution for the issue: https://github.com/Expensify/App/issues/46883#issuecomment-2302034091

brunovjk commented 2 months ago

Hi @rlinoz, do we have any updates here? Thanks.

rlinoz commented 2 months ago

PR is pending deploy

rlinoz commented 2 months ago

PR is deployed and I think this is fixed.

brunovjk commented 2 months ago

Indeed, I just tested (v9.0.25-2), it looks the issue is fixed:

https://github.com/user-attachments/assets/f156af99-96c7-4234-a49b-d06d12da5a0e

brunovjk commented 2 months ago

@rlinoz Can we ask for a retest here? Thank you :D

melvin-bot[bot] commented 2 months ago

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

brunovjk commented 2 months ago

Update: Unable to reproduce v9.0.28-0:

https://github.com/user-attachments/assets/27e1a3e1-9c11-42fd-b6c1-a334444a0406

melvin-bot[bot] commented 2 months ago

@rlinoz @zanyrenney @brunovjk this issue is now 4 weeks old, please consider:

Thanks!

zanyrenney commented 1 month ago

bit confused it payment is due here for @brunovjk

@rlinoz can you confirm seeing as you seemed to raise the PR?

brunovjk commented 1 month ago

@zanyrenney I believe no payment is required here. @rlinoz raise a PR on the backend to resolve this. We need to retest and make sure this bug is squashed :D

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? 💸

mvtglobally commented 1 month ago

Issue not reproducible during KI retests. (First week)

zanyrenney commented 1 month ago

2 weeks since PR was merged and 2 weeks (1), (2) unable to reproduce, awesome work @rlinoz @brunovjk 🎉

Going to close this out. No payment required.