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

[$250] Task – Task disappears from LHN when it is not open #46715

Open
3 of 6 tasks
lanitochka17 opened this issue Aug 2, 2024 · 55 comments
Open
3 of 6 tasks

[$250] Task – Task disappears from LHN when it is not open #46715

lanitochka17 opened this issue Aug 2, 2024 · 55 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 2, 2024

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.16-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/4804049
Email or phone of affected tester (no customers): ponikarchuks+32824@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in
  3. Open chat with user B
  4. Create a task without assignee
  5. Open task details and assign it to you
  6. Observe it in LHN
  7. Open another chat and open task details in LHN
  8. Open another chat and observe LHN

Expected Result:

Task present in LHN

Actual Result:

Task disappears from LHN

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6559911_1722572236482.Task_LHN.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015e6ab873971b6bf9
  • Upwork Job ID: 1820503061150367371
  • Last Price Increase: 2024-08-26
Issue OwnerCurrent Issue Owner: @hungvu193
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Triggered auto assignment to @twisterdotcom (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
Copy link
Author

@twisterdotcom 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

@lanitochka17
Copy link
Author

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

@nyomanjyotisa
Copy link
Contributor

Proposal

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

Task disappears from LHN when it is not open

What is the root cause of that problem?

We only show the task report on LHN if it is opened, pinned, or the notificationPreference is not hidden

const shouldOverrideHidden = hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned;
if (isHidden && !shouldOverrideHidden) {
return;
}

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

If we want to always display tasks that assigned to the current user we can include isTaskAssignedToCurrentUser in the shouldOverrideHidden condition

const isTaskAssignedToCurrentUser = ReportUtils.isTaskReport(report) && session?.accountID === report?.managerID;
const shouldOverrideHidden = ... || isTaskAssignedToCurrentUser;

What alternative solutions did you explore? (Optional)

Copy link
Contributor

github-actions bot commented Aug 2, 2024

true

@FitseTLT
Copy link
Contributor

FitseTLT commented Aug 2, 2024

BE.
The reason it appears in LHN in (6) is because the task report has become unread because when you assign yourself as an assigne the lastMentionedTime will be updated, but then it doesn't appear in step 8 because we have opened the task report and it is no more unread report.
The only thing that should be fixed here that lastMentionedTime should not be updated if a person assign himself for a task it only makes sense to assume it as a mention if a person is assigned by another user but that, I have checked, it is a BE issue can only be fixed in BE.

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
@twisterdotcom
Copy link
Contributor

Sounds good @FitseTLT, gonna get a C+ to confirm.

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@twisterdotcom twisterdotcom added the External Added to denote the issue can be worked on by a contributor label Aug 5, 2024
@melvin-bot melvin-bot bot changed the title Task – Task disappears from LHN when it is not open [$250] Task – Task disappears from LHN when it is not open Aug 5, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

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

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

melvin-bot bot commented Aug 5, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 6, 2024

Edited by proposal-police: This proposal was edited at 2024-08-08 05:02:02 UTC.

Proposal

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

Task disappears from LHN

What is the root cause of that problem?

After step 6, the task report's notificationPreference is always:

notificationPreference: [assigneeAccountID, ownerAccountID].includes(currentUserAccountID)
? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS
: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,

hence it appears in LHN.

After step 7, we call OpenReport API that reset notificationPreference to hidden.

Hence after step 8, the task report does not appear in LHN.

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

  1. We should fix that issue on BE side. A similar issue is fixed in this issue.

  2. After the above fix, we can notice another bug, related to the GBR.

In step 5, BE sets the task report 's lastMentionedTime to the new value but we do not do it in optimistic data, which does not match our Offline UX Patterns.
To fix that, we should set lastMentionedTime in this:

const optimisticReport: OptimisticReport = {

        lastMentionedTime: [assigneeAccountID, ownerAccountID].includes(currentUserAccountID) ? DateUtils.getDBTime() : undefined,

What alternative solutions did you explore? (Optional)

With the new expected behavior in comment, we can update:

notificationPreference: [assigneeAccountID, ownerAccountID].includes(currentUserAccountID)
? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS
: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,

        notificationPreference:
            [assigneeAccountID, taskOwnerAccountID].includes(currentUserAccountID) && !parentReport
                ? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS
                : CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,

with:

    const parentReport = getParentReport(report);
    const taskOwnerAccountID = getTaskOwnerAccountID(report);

and BE should apply the same changes.

@hungvu193
Copy link
Contributor

Daily update: I'm quite busy today but definitely can review this one tomorrow morning

@hungvu193
Copy link
Contributor

2. After the above fix, we can notice another bug, related to the GBR.

May I know what's the GBR bug that you mentioned?

@daledah
Copy link
Contributor

daledah commented Aug 7, 2024

May I know what's the GBR bug that you mentioned?

In step 5, In offline mode, we don't have the GBR when we assign the task ourselves. But in online mode, the GBR is displayed.

@hungvu193
Copy link
Contributor

May I know what's the GBR bug that you mentioned?

In step 5, In offline mode, we don't have the GBR when we assign the task ourselves. But in online mode, the GBR is displayed.

Thanks, I think that's good to fix but not related to this issue 🤔 .

My two cents here, I think a task shouldn't disappear from LHN if it's a self-assigned task (We fixed a similar bug here). We can fix this from BE or update FE's shouldOverrideHidden logic.

@hungvu193
Copy link
Contributor

I think a task shouldn't disappear from LHN if it's a self-assigned task (We fixed a similar bug here).

@twisterdotcom How do you think about this one?

@puneetlath
Copy link
Contributor

This isn't a bug. Tasks are like expense reports. They don't show in your LHN if you have access to the parent.

@daledah
Copy link
Contributor

daledah commented Aug 8, 2024

This isn't a bug. Tasks are like expense reports. They don't show in your LHN if you have access to the parent.

@hungvu193 if it is expected, so there is a bug when self-assigning as task when offline. In there, the task report is displayed in LHN.

@hungvu193
Copy link
Contributor

This isn't a bug. Tasks are like expense reports. They don't show in your LHN if you have access to the parent.

@hungvu193 if it is expected, so there is a bug when self-assigning as task when offline. In there, the task report is displayed in LHN.

@puneetlath What do you think about this case?

Copy link

melvin-bot bot commented Aug 12, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2024
@twisterdotcom
Copy link
Contributor

I agree here:

They don't show in your LHN if you have access to the parent.

But the bug shown in the video is that it is pinned with a GBR after creation and when navigating away, the task is removed from the LHN, even though it had a GBR. It's a confusing flow to show the GBR at both the highest and lowest context at once in the LHN because it feels like it shouldn't disappear.

@twisterdotcom
Copy link
Contributor

I do not truly understand the data hierarchy entirely, but I think if you have a GBR, and we always show the highest context for that GBR, we should show whatever report is the highest context for that GBR, even if you may not have access to the absolute highest - does that make sense?

ie, you can create a private room and then allocate tasks to individuals outside of that room. Just because that individual isn't a member of that room, if their is a task waiting on them, it should show in their LHN.

@hungvu193
Copy link
Contributor

I do not truly understand the data hierarchy entirely, but I think if you have a GBR, and we always show the highest context for that GBR, we should show whatever report is the highest context for that GBR, even if you may not have access to the absolute highest - does that make sense?

I also agree with this one.

@daledah
Copy link
Contributor

daledah commented Aug 26, 2024

@hungvu193

  • There are 2 cases:
  1. User is the task's assignee and has access to parent report.

  2. User is the task's assignee and has no access to parent report.

  • Your comment "however there's another issue, when you self-assign task, you see a GBR, a task with GBR shouldn't be hidden." related to the 1st case.

  • Currently, in LHN, we always display the parent report with GBR icon via the logic:

    const shouldShowGreenDotIndicator = !hasBrickError && ReportUtils.requiresAttentionFromCurrentUser(optionItem, optionItem.parentReportAction);

    App/src/libs/ReportUtils.ts

    Lines 2580 to 2582 in 0c84552

    if (isWaitingForAssigneeToCompleteTask(optionOrReport, parentReportAction)) {
    return true;
    }

    it already matched @twisterdotcom 's requirement above: "we should show whatever report is the highest context for that GBR, even if you may not have access to the absolute highest"

  • So I think we can hide the task report in LHN.

  • Also, we cannot display the report that has notificationPreference: hidden even if it has GBR because of our logic:

const shouldOverrideHidden = hasValidDraftComment(report.reportID) || hasErrorsOtherThanFailedReceipt || isFocused || isSystemChat || report.isPinned;
if (isHidden && !shouldOverrideHidden) {
return;
}

Copy link

melvin-bot bot commented Aug 26, 2024

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

@hungvu193
Copy link
Contributor

hungvu193 commented Aug 28, 2024

@daledah So okay, if you can prevent GBR showing on child task (user can access parent report) , then I think it's ok.

@daledah
Copy link
Contributor

daledah commented Aug 29, 2024

@hungvu193 Do I need to update the proposal anymore?

@hungvu193
Copy link
Contributor

hungvu193 commented Aug 29, 2024

@daledah No need to update.
Sorry for the delay, I've just tested again and @daledah 's alternative solution looks good to me!
We're currently waiting for the BE data to hide the child task, meanwhile if we can access the parent task, the notification should be hidden by default. Let's update optimistic data to handle that case!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 29, 2024

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

Copy link

melvin-bot bot commented Aug 30, 2024

@twisterdotcom @hungvu193 @aldo-expensify this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@hungvu193
Copy link
Contributor

Waiting for assigning

Copy link

melvin-bot bot commented Sep 3, 2024

@twisterdotcom, @hungvu193, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Sep 3, 2024
@twisterdotcom
Copy link
Contributor

Bump @aldo-expensify

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

melvin-bot bot commented Sep 3, 2024

📣 @daledah You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@daledah
Copy link
Contributor

daledah commented Sep 4, 2024

@aldo-expensify I've drafted a PR to fix the FE side. Waiting for BE fix.

@aldo-expensify
Copy link
Contributor

@aldo-expensify I've drafted a PR to fix the FE side. Waiting for BE fix.

Thanks @daledah , after reading the whole thread, I'm still a bit confused of what is the expected behaviour and what are the backend changes we want to do.

From testing, I see that when I open the unassigned task, it is added to the LHN. Then, I assign the task to myself and then I move away to another report. The newly assigned task remains in the LHN when it should have been removed... this is what the bug is about, right?

The proposed backend change is that lastMentionedTime shouldn't be updated when the task is self assigned, correct?

@daledah
Copy link
Contributor

daledah commented Sep 8, 2024

@aldo-expensify The expected behavior is described in #46715 (comment), The task report should disappear when we open another report. The backend change should be to set notification preference to always only when the user is the owner or assignee and user doesn't have access to the parent report.

@melvin-bot melvin-bot bot added the Overdue label Sep 8, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

@twisterdotcom, @hungvu193, @aldo-expensify, @daledah Huh... This is 4 days overdue. Who can take care of this?

@hungvu193
Copy link
Contributor

@daledah Let's complete and move forward with your PR

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Sep 10, 2024
@daledah
Copy link
Contributor

daledah commented Sep 10, 2024

@hungvu193 PR is up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

8 participants