[HOLD for payment 2023-08-24] Show chats in the LHN that have at least 1 ADDComment action, or 1 draft message

[JmillsExpensify]

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 https://staging.new.expensify.com/
2. Login with any account
3. Search for a user that you don't have any messages with, open the chat but don't send any message

Expected Result:

Chat should not show in the LHN as there is not at least 1 ADDComment action, or 1 draft message.

Actual Result:

Empty chat shows in the LHN and remains in the LHN after simply navigating to it.

Workaround:

None

Platforms:

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

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.2.58-3 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fbd874b8451c4608
• Upwork Job ID: 1618016837618089984
2023-01-24
• Automatic offers:
• | | 0

[melvin-bot[bot]]

Current assignee @JmillsExpensify is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @thesahindia (Internal)

[JmillsExpensify]

Pretty sure this is internal based on the this discussion, so I'm starting with that route first for now.

[JmillsExpensify]

Huh, I'm just seeing that now one internal was added to this issue. I'm not sure why that's the case. Trying to more labels to hopefully get it right.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Current assignee @thesahindia is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[JmillsExpensify]

@PauloGasparSv Do you mind weighing in on this issue and whether we can open it up for external? Thank you!

[PauloGasparSv]

Hey @JmillsExpensify I'm pretty sure this can be an External change. I think we can add a filter in shouldReportBeInOptionList that filters out empty chats. That method is already called to filter out chats in LNH and the Search option (before you type anything)

But I'm not sure which fields to check so I created a thread on slack to discuss that

[JmillsExpensify]

Ok great! I'll add the external label in the meantime, looks like there is largely agreement in that thread.

[melvin-bot[bot]]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Current assignee @thesahindia is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Current assignee @PauloGasparSv is eligible for the External assigner, not assigning anyone new.

[Pujan92]

Proposal

We can add last condition to check the hasDraft or lastMessage for the report type Chat Report and based on that we can filter out those entries within shouldReportBeInOptionList.

diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 4bda454798..01a2dc2387 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -1194,6 +1194,10 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

+    if (_.isEqual(report.reportName, CONST.REPORT.DEFAULT_REPORT_NAME) && !report.hasDraft && (_.isEmpty(report.lastMessageTex
+ t) || (_.isEmpty(report.lastMessageHtml)))) {
+        return false;
+    }
+
     return true;
 }

[bernhardoj]

Proposal

The first thing we need to do is to check whether the given report is a direct message or not. Next, we can simply check whether the lastMessageText of the report is empty or not. We already have the filter for draft report.

https://github.com/Expensify/App/blob/7f78d52b032b9c8abab2b0b5f10081a00f34b28b/src/libs/ReportUtils.js#L1229-L1233

Based on the condition above, empty pinned chat will still be shown.

diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 4bda45479..d0ff1c66a 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -142,6 +142,10 @@ function canDeleteReportAction(reportAction) {
         && reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
 }

+function isDirectMessage(report) {
+    return _.isEmpty(getChatType(report));
+}
+
 /**
  * Whether the provided report is an Admin room
  * @param {Object} report
@@ -1194,6 +1198,10 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

+    if (isDirectMessage(report) && _.isEmpty(report.lastMessageText)) {
+        return false;
+    }
+
     return true;
 }

[sobitneupane]

Proposal: If we don't want the reports to be in LHN even when the report is opened, we need to remove if (report.reportID === reportIDFromRoute) {return true;} condition.

@@ -1145,13 +1145,6 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

-    // Include the currently viewed report. If we excluded the currently viewed report, then there
-    // would be no way to highlight it in the options list and it would be confusing to users because they lose
-    // a sense of context.
-    if (report.reportID === reportIDFromRoute) {
-        return true;
-    }
-
     // Include reports if they have a draft, are pinned, or have an outstanding IOU
     // These are always relevant to the user no matter what view mode the user prefers
     if (report.hasDraft || report.isPinned || hasOutstandingIOU(report, currentUserLogin, iouReports)) {
@@ -1194,6 +1187,10 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

+    if (_.isEmpty(getChatType(report)) && _.isEmpty(report.lastMessageText)) {
+        return false;
+    }
+
     return true;
 }

[thesahindia]

If we don't want the reports to be in LHN even when the report is opened, we need to remove if (report.reportID === reportIDFromRoute) {return true;} condition.

I think the chat should be present in LHN when it is open.

cc: @JmillsExpensify @PauloGasparSv (for confirmation^)

[PauloGasparSv]

Let's wait on @JmillsExpensify to confirm that IMO it will make the solution a bit more complicated and won't aggregate much value to the App!

Also, let me bring a bit of the slack discussion about which fields we should use to check if the chat is empty to this GH:

If all you have is the report object itself then I’d say the best way to confirm if a chat is “empty” would be to look at the reportActionCount. Check where we return the reports to the client in Auth here. One caveat is that IIRC we are going to backfill all reports so that they have at least 1 CREATED action so an empty chat would be reportActionCount === 1 .

I think we should be checking the reportActionCount field instead of lastMessageHtml or lastMessageText cc @Pujan92 @bernhardoj @sobitneupane

[PauloGasparSv]

Just confirmed with the team the reportActionCount should start at 1 now that we ensured every report created inserts a CREATED action!

So I think we should be skipping chats if reportActionCount === 1 as mentioned above!

[Pujan92]

@PauloGasparSv I am not seeing the field reportActionCount while logging the report details which we have while filtering.

[bernhardoj]

Yeah, looks like the reportActionCount is not present to the report data. Assuming it is a new property that is not deployed yet, we can simply change _.isEmpty(report.lastMessageText) to report.reportActionCount === 1.

[PauloGasparSv]

You are correct! Sorry for that @Pujan92 @bernhardoj

Let me check why that property is not returned and if I can make it available.

[PauloGasparSv]

All right, just checked and while formatting the field reportActionCount is returned as maxSequenceNumber.

Could everyone please update the proposals to use that field instead to check if the chat is empty making sure the proposal still works and that counter starts at 1?

[Pujan92]

Updated Proposal

Use maxSequenceNumber number to check for the empty chat report.

diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 4bda454798..01a2dc2387 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -1194,6 +1194,10 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

+    if (_.isEmpty(report. chatType) && report.maxSequenceNumber === 1) {
+        return false;
+    }
+
     return true;
 }

[sobitneupane]

@PauloGasparSv I don't think we can use maxSequenceNumber, if we add a comment and delete it, maxSequenceNumber number increases and is not equal to 1 though the report has no comments.

[PauloGasparSv]

In the meantime, created a fix so the maxSequenceNumber is not coalesced to 0 but instead to 1 in case it is missing.

Bump @JmillsExpensify on this.

[Pujan92]

@PauloGasparSv I don't think we can use maxSequenceNumber, if we add a comment and delete it, maxSequenceNumber number increases and is not equal to 1 though the report has no comments.

Yes, about to write on this point.

[PauloGasparSv]

@PauloGasparSv I don't think we can use maxSequenceNumber, if we add a comment and delete it, maxSequenceNumber number increases and is not equal to 1 though the report has no comments.

Nice catch @sobitneupane, I'll bring it up on the field discussion thread that suggested it! But I think it's ok to show the chat in that edge case, how about you @JmillsExpensify @thesahindia ?

[PauloGasparSv]

CC @marcaaron would you mind sharing your opinion on this too?

This issue is about not showing the chat in LNH if it has no reportActions or drafts in it. The field reportActionCount / maxSequenceNumber doesn’t decrease back to 1 after deleting everything in the chat, so it's not an absolute way to check if the chat is empty using just the report object.

I think this would be an edge case and would be ok to have, do you?

[marcaaron]

not showing the chat in LNH if it has no reportActions or drafts in it

yes, sounds like the expected behavior to me, but don't have any super strong feelings on it. I think I'd add that the "active" chat in view will still show whether there's a draft or not.

Also, all reports with have at least one 'CREATED' action soon so not sure how reliable it will be to check for reportActions.length === 0. In addition, the reportActions are not all loaded by default. They are paginated so you can't really depend on the stored reportActions at all.

The field reportActionCount / maxSequenceNumber doesn’t decrease back to 1 after deleting everything in the chat, so it's not an absolute way to check if the chat is empty using just the report object.

Cool. I forgot that this doesn't include deleted actions (I think that's the correct behavior).

I think this would be an edge case and would be ok to have, do you?

What's the worst case scenario? Every edge case is better evaluated as "what will the user experience if they do run into it". Sounds like the chat would just show in the LHN? Agree. That it doesn't sound so bad.

But if we do want to fix it then we can return from Auth a visibleActionCount or number of visible and non-deleted actions and update it as things are deleted client side.

[PauloGasparSv]

yes, sounds like the expected behavior to me, but don't have any super strong feelings on it. I think I'd add that the "active" chat in view will still show whether there's a draft or not.

I just re-read the issue and the "Actual results" section does mention that the new empty chat should remain in the LHN until we navigate out of it (also as @thesahindia commented here). I'll check if the proposals also meet that requirement later.

What's the worst case scenario? Every edge case is better evaluated as "what will the user experience if they do run into it". Sounds like the chat would just show in the LHN? Agree. That it doesn't sound so bad.

I agree and I think we should do nothing in that edge case. Will wait on @JmillsExpensify and @thesahindia opinions on this too before planning what to do next!

Thks @marcaaron!

[bernhardoj]

Updated Proposal

Updating proposal based on the discussion above.

diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 4bda45479..d0ff1c66a 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -142,6 +142,10 @@ function canDeleteReportAction(reportAction) {
         && reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
 }

+function isDirectMessage(report) {
+    return _.isEmpty(getChatType(report));
+}
+
 /**
  * Whether the provided report is an Admin room
  * @param {Object} report
@@ -1194,6 +1198,10 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
         return false;
     }

+    if (isDirectMessage(report) && report.maxSequenceNumber === 1) {
+        return false;
+    }
+
     return true;
 }

[thesahindia]

Will wait on @JmillsExpensify and @thesahindia opinions on this too before planning what to do next!

It's fine with me, let's wait for @JmillsExpensify's thoughts.

[PauloGasparSv]

Bump @JmillsExpensify @thesahindia

[JmillsExpensify]

I think the chat should be present in LHN when it is open.

I don't think I agree with this, though in any case it's also not how it works today, so I think that's out of scope for this issue.

[bernhardoj]

Actually, that's how it works currently. To test it, you can open any chat first, and then switched to #focus mode then open another chat. Notice the chat from the LHN is getting removed after you moved to another chat.

https://github.com/Expensify/App/blob/7f78d52b032b9c8abab2b0b5f10081a00f34b28b/src/libs/ReportUtils.js#L1222-L1227

[thesahindia]

As @bernhardoj mentioned it is the current behaviour. @JmillsExpensify, do you think we should change this?

[MelvinBot]

@JmillsExpensify @PauloGasparSv @thesahindia 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!

[thesahindia]

I think the chat should be present in LHN when it is open

We already have a working solution. We are just waiting for @JmillsExpensify approval on the above. It is the current behaviour to keep the chat in LHN when it's open so I think we should keep that as it is and just remove the chats that don't have any chat history.

[JmillsExpensify]

I replied to this question two days ago here. To repeat that comment, I don't agree that a chat should be present in the LHN when it is open.

[JmillsExpensify]

Actually this is interesting. I just realized what a couple of days ago that on desktop, we don't show the open chat in the LHN. If you're on web though, we do show the open chat in the LHN. So we're actually not consistent cross-platform at the moment.

[JmillsExpensify]

@thesahindia do you mind bringing this one to open-source tomorrow. Given that we don't have a standard at the moment, we should agree what the standard is.

[thesahindia]

I just realized what a couple of days ago that on desktop, we don't show the open chat in the LHN.

That's not the case, we also see chat in LHN if it's open. Here's a video -

https://user-images.githubusercontent.com/83179295/218129554-f847e5ee-fd99-4897-bafe-b2afc9d35d48.mov

[thesahindia]

@thesahindia do you mind bringing this one to open-source tomorrow. Given that we don't have a standard at the moment, we should agree what the standard is.

Posted here

[PauloGasparSv]

Not overdue, slack discussion still ongoing

[thesahindia]

I prefer @bernhardoj's proposal since their original proposal was first acceptable solution and only a suggested change was needed.

C+ reviewed 🎀👀🎀

cc: @PauloGasparSv

[JmillsExpensify]

Thanks for your patience everyone. We've aligned on this behavior, so we should be good to continue with proposals again.

[JmillsExpensify]

And...GH just updated after I commented. We'll wait for @PauloGasparSv to weigh in when he returns tomorrow. Thanks again for the patience everyone!