search

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 – Task disappears from LHN when it is not open #46715

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 3 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.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?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/416b4d08-8df6-4e50-941f-73430e46d46b

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
melvin-bot[bot] commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

nyomanjyotisa commented 3 months ago

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 https://github.com/Expensify/App/blob/49435db77e31fb327003cdf744e0efd60f55be48/src/libs/SidebarUtils.ts#L138-L141

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)

github-actions[bot] commented 3 months ago

true

FitseTLT commented 3 months ago

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.

twisterdotcom commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

daledah commented 2 months ago

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: https://github.com/Expensify/App/blob/f7e265a65f695edd85870bd0c8048cf4578e14fb/src/libs/actions/Task.ts#L584-L586 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: https://github.com/Expensify/App/blob/f7e265a65f695edd85870bd0c8048cf4578e14fb/src/libs/actions/Task.ts#L578

        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: https://github.com/Expensify/App/blob/f7e265a65f695edd85870bd0c8048cf4578e14fb/src/libs/actions/Task.ts#L584-L586

        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 commented 2 months ago

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

hungvu193 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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?

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

twisterdotcom commented 2 months ago

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.

puneetlath commented 2 months ago

Ahh I see, you're right. Ignore me. That does seem to be a bug.

hungvu193 commented 2 months ago

Thanks for confirming. Waiting for proposals

daledah commented 2 months ago

@hungvu193 What do you think about my alternative solution?

hungvu193 commented 2 months ago

App/src/libs/actions/Task.ts

I don't think that's the correct solution, remove Hidden as notificationPreference will make the task visible even when it doesn't have GBR. We still want to keep the task hidden if user has access to its parent and only show it if it has GBR.

daledah commented 2 months ago

@hungvu193 Thanks. I updated my alternative solution. With it:

if notificationPreference is hidden, report is not displayed in LHN, regardless it has GBR or not.

melvin-bot[bot] commented 2 months ago

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

hungvu193 commented 2 months ago

Thanks for your update! I'll take a look today

hungvu193 commented 2 months ago
  • If user cannot access the parent report: The task's notificationPreference is always if user is the owner or assignee. Otherwise, notificationPreference is hidden.

Wait, is there any case that user cannot access the parent report of the task?

daledah commented 2 months ago

Wait, is there any case that user cannot access the parent report of the task?

For example, you can open the selfDM, then create a task then assign it to anyone (named user B). Then you sign in with user B. User B cannot access the selfDM (the parent report of the task).

hungvu193 commented 2 months ago

Wait, is there any case that user cannot access the parent report of the task?

For example, you can open the selfDM, then create a task then assign it to anyone (named user B). Then you sign in with user B. User B cannot access the selfDM (the parent report of the task).

That's the user that got assigned to the task, if you create the task you should always access the parent report. Then this should be fixed from our BE.

daledah commented 2 months ago

Then this should be fixed from our BE.

Sorry, what do you mean about "this" here?

hungvu193 commented 2 months ago

I mean this issue should be fixed from our BE.

daledah commented 2 months ago

Do you think we need to fix the issue in FE, in other words, the optimistic data. Let's take a look at the code below: https://github.com/Expensify/App/blob/f7e265a65f695edd85870bd0c8048cf4578e14fb/src/libs/actions/Task.ts#L584-L586

Now if you assign the task to yourself, we always set the notificationPreference to always (leads to the task report is displayed in LHN), regardless of whether the user can access the parent report or not, right?

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

hungvu193 commented 2 months ago

Do you think we need to fix the issue in FE, in other words, the optimistic data. Let's take a look at the code below:

https://github.com/Expensify/App/blob/f7e265a65f695edd85870bd0c8048cf4578e14fb/src/libs/actions/Task.ts#L584-L586

Now if you assign the task to yourself, we always set the notificationPreference to always (leads to the task report is displayed in LHN), regardless of whether the user can access the parent report or not, right?

That makes sense, however there's another issue, when you self-assign task, you see a GBR, a task with GBR shouldn't be hidden.

daledah commented 2 months ago

however there's another issue, when you self-assign task, you see a GBR, a task with GBR shouldn't be hidden

I think we need to confirm the expected behavior here. @puneetlath In above, you said

"Tasks are like expense reports. They don't show in your LHN if you have access to the parent.",

but @hungvu193 thinks:

"when you self-assign task, you see a GBR, a task with GBR shouldn't be hidden"

So when the task report has GBR, should we display it in LHN?

twisterdotcom commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@hungvu193

  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.

https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/libs/SidebarUtils.ts#L132-L135

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

hungvu193 commented 2 months ago

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

daledah commented 2 months ago

@hungvu193 Do I need to update the proposal anymore?

hungvu193 commented 2 months ago

@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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

Thanks!

hungvu193 commented 2 months ago

Waiting for assigning

melvin-bot[bot] commented 2 months ago

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

twisterdotcom commented 2 months ago

Bump @aldo-expensify

melvin-bot[bot] commented 2 months ago

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

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