App icon indicating copy to clipboard operation
App copied to clipboard

[$250] mWeb - Self DM - Self DM with track expense on mark as unread is not shown in bold in LHN

Open IuliiaHerets opened this issue 1 year ago • 3 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: V9. 0.73-6 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both If this was caught during regression testing, add the test name, ID and link from TestRail: N Email or phone of affected tester (no customers): N Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Open self dm with no conversation
  3. Create a track expense
  4. Navigate to LHN
  5. Long press self dm and select mark as unread
  6. Note self dm not shown in bold
  7. Tap fab --new workspace
  8. Open workspace chat
  9. Create a track expense
  10. Navigate to LHN and mark as unread the chat
  11. Note workspace chat shown in bold.

Expected Result:

Self DM with track expense on mark as unread must be shown in bold in LHN.

Actual Result:

Self DM with track expense on mark as unread is not shown in bold in LHN.

Workaround:

Unknown

Platforms:

  • [x] Android: Standalone
  • [x] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/5819115b-ba49-4dc9-a36c-1a2ec9d09bd9

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021866482645091110325
  • Upwork Job ID: 1866482645091110325
  • Last Price Increase: 2024-12-10
Issue OwnerCurrent Issue Owner: @paultsimura

IuliiaHerets avatar Dec 10 '24 11:12 IuliiaHerets

Triggered auto assignment to @bfitzexpensify (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] avatar Dec 10 '24 11:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 10 '24 13:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 10 '24 13:12 melvin-bot[bot]

Proposal

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

Self DM - Self DM with track expense on mark as unread is not shown in bold in LHN

What is the root cause of that problem?

When executing the markCommentAsUnread function, the created time of the action track expense is used as the new lastReadTime instead of the report action which is last visible.

https://github.com/Expensify/App/blob/3982ed03ff644e7a723e8d4131535a2cceb9488d/src/libs/actions/Report.ts#L1359-L1368

https://github.com/Expensify/App/blob/3982ed03ff644e7a723e8d4131535a2cceb9488d/src/libs/actions/Report.ts#L1372-L1379

However, the isUnread function uses the report's lastVisibleActionCreated value which is unchanged, and compares itto lastReadTime which is greater than the lastVisibleActionCreated.

https://github.com/Expensify/App/blob/3982ed03ff644e7a723e8d4131535a2cceb9488d/src/libs/ReportUtils.ts#L6275-L6280

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

Filter out actionable track expenses just like whisper actions.

https://github.com/Expensify/App/blob/3982ed03ff644e7a723e8d4131535a2cceb9488d/src/libs/actions/Report.ts#L1364

(!(ReportActionsUtils.isWhisperAction(current) || ReportActionsUtils.isActionableTrackExpense(current)) || current.actionName === CONST.REPORT.ACTIONS.TYPE.ACTIONABLE_MENTION_WHISPER)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We should test the markCommentAsUnread function by creating a variety of test reports that have actionable whisper actions.

What alternative solutions did you explore? (Optional)

Set the report's lastVisibleActionCreated optimistically to the newly created report action.

Tony-MK avatar Dec 10 '24 14:12 Tony-MK

The proposal by @Tony-MK does the job. However, I wonder if it should be fixed on the BE.

While tracking an expense in self-DM, we correctly set the optimistic report.lastVisibleActionCreated to the actionableTrackExpenseWhisper.created:

https://github.com/Expensify/App/blob/cb88e73de5eb3ae467e3a48de1bd3c4d46a0023a/src/libs/actions/IOU.ts#L1474-L1481

However, when the TrackExpense API response arrives, it changes the report.lastVisibleActionCreated to the tracked expense report action's created time.

So our options are:

  1. Go with the suggested FE fix
  2. Fix the BE to set the actionableTrackExpenseWhisper.created as the report.lastVisibleActionCreated

Side note: If we go with the FE fix, we'll have a side-effect that when marking Self-DM as unread, we'll not see the green line above the actionable tracked expense. I'm not sure it's bad though, because currently, we add the green line above the most recent such whisper. So even when there are other messages after it, we'll still add the green line above that whisper.


Current behavior with the green line:

https://github.com/user-attachments/assets/8667cd64-97d4-4b44-8c7b-0900ac0e441c


Behavior after the FE fix:

https://github.com/user-attachments/assets/edafa08a-487b-42e9-aa81-752c1623f699

🎀👀🎀 C+ reviewed

paultsimura avatar Dec 11 '24 11:12 paultsimura

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

melvin-bot[bot] avatar Dec 11 '24 11:12 melvin-bot[bot]

Hrm, I feel like yeah we should match the behavior of other chats - so green line above most recent message, and bold in LHN. So maybe the backend fix is the right one. I can look into this tomorrow; I actually just made a similar change for a different kind of actionable whisper, so I can see if I can do it for this one too.

dangrous avatar Dec 11 '24 21:12 dangrous

Okay I have a potential BE fix for this up, but I realized I need one clarification @paultsimura - with the existing code, why does marking the chat as unread, even with additional messages after the whisper, add the green line above the whisper? Is that a BE thing that I can fix here too?

Updating the lastVisibleActionCreated makes the LHN bold on mark as unread, but the green line still appears above the whisper, even when there are more messages below.

Thanks!

dangrous avatar Dec 12 '24 20:12 dangrous

Is that a BE thing that I can fix here too?

No, it's an FE thing – the logic for marking the chat unread is to find the last message sent by somebody else and mark it unread.

https://github.com/Expensify/App/blob/91699208ca24ffe378051507e7430f83c4203ebc/src/libs/actions/Report.ts#L1367-L1382

It so happens that, in the self-DM case, nobody else sends messages. Therefore, the last message sent by "not you" is that actionable whisper.

paultsimura avatar Dec 12 '24 21:12 paultsimura

Okay cool, that feels like sort of weird behavior to me but if that's how we have it then we can stick to that for now.

I'll push the backend change up for review today and we can go from there.

dangrous avatar Dec 13 '24 16:12 dangrous

Backend is merged, not yet deployed.

dangrous avatar Dec 16 '24 17:12 dangrous

Okay, now it's deployed. Do we need any FE changes? (And can we retest please?) thanks!

dangrous avatar Dec 17 '24 18:12 dangrous

Okay I just tested on staging and it seems to work.

I did notice another issue that is related but not super important probably? As @paultsimura called out here, mark as unread marks the last message that hasn't been sent by you. Which means you can't mark a self-DM as unread UNLESS it has a track expense whisper in it. I still find this weird, but if we decide to fix it it'll be out of scope of this issue.

I'm going to close this, if anyone disagrees - or if we need payment or anything - let me know!

dangrous avatar Dec 18 '24 19:12 dangrous