Exclude empty chats (with no ADDCOMMENT actions or message drafts) from LNH by PauloGasparSv · Pull Request #19321 · Expensify/App

PauloGasparSv · 2023-05-19T21:00:57Z

Details

Updating the LNH logic so it doesn't show empty chats! We consider a chat empty if it's not active and has no drafts or messages (ADDCOMMENT actions).
This change must not influence how Threads, IOUS, Tasks and other features of the App work.

Fixed Issues

$ #14523
PROPOSAL: N/A

Tests

Empty chat shouldn't stay in LNH

Sign into newDot
Start a new chat with an account.
Make sure the new chat opens and is shown in the LNH
[WEB ONLY]: Copy the chat's reportID (last number in the chat's URL)
Open another existing chat (like concierge), make sure the chat disappears from the LNH
Start a chat again with that same account, make sure the account shows up in the Options list since you already had a chat with it before
Make sure the chat opens and has the same reportID (has the same URL)

Chat should stay in LNH if it has a Draft

In the same chat of used in the previous test, type some text into the composer but do not send the message
Open another existing chat (like concierge), make sure the chat stays in the LNH and shows the draft icon

Not-Empty chat should stay in LNH

Send a message in the same chat from the previous test.
Open another existing chat (like concierge), make sure the chat stays in the LNH.

Deleting messages and leaving chat empty should remove it from LNH

Open a chat that has a message sent inside it
Delete that message so the chat becomes empty
Open another existing chat (like concierge), make sure the chat disappears from the LNH.

Same tests w/ Group Chat

Start a new group chat with two accounts "B" and "C"
Make sure the new chat opens and is shown in the LNH
[WEB ONLY]: Copy the chat's reportID (last number in the chat's URL)
Open another existing chat (like concierge), make sure the chat disappears from the LNH
Start a group chat again with account B and C
Make sure the chat opens and has the same reportID (has the same URL)

Money request should stay in LNH

Start a new empty chat as userA with existing account userB
Sign in as userB and send a message back to userA so you become known
As userA, send new Money Request to userB inside the chat
As userB, delete the message sent in step 2 so the chat becomes empty
As userA, Open another existing chat (like concierge), make sure the reports don't disappear from the LNH
Make sure both the Money Request and the original report stay in the LNH

Workspace Chats should stay in LNH

Create a new Workspace in an account
Make sure the #admins, #announce and a workspace chats are created
Open another existing chat (like concierge), make sure none of the workspace related chats disappear from the LNH
Invite another user to the workspace, make sure another workspace chat is created for the invited user
Open another existing chat (like concierge), make sure none of the workspace related chats disappear from the LNH

TODO: add steps for these

Verify that no errors appear in the JS console

Offline tests

Sign into newDot
Turn connectivity off
Start a new chat with an account.
Make sure the new chat opens and is shown in the LNH
[WEB ONLY]: Copy that chat's reportID (last number in the chat's URL)
Open another existing chat (like concierge), make sure the chat disappears from the LNH
Start a chat again with that same account, make sure the account shows up in the Options list since you already had a chat with it before
[WEB ONLY]: That chat's reportID will not be the same as the first attempt because we are offline, but make note of that reportID as well
Open another existing chat (like concierge), make sure the chat disappears from the LNH
[WEB ONLY]: Toggle connectivity back on
[WEB ONLY]: Start a chat again with that same account
[WEB ONLY]: Make sure that chat's reportID is the same as the from step 5 (and not step 8)

Make sure the chat opens and has the same reportID (has the same URL)

QA Steps

Same as Tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web-up-to-group-chat.mov

web-2.mov

Mobile Web - Chrome

android-web-1.mov

Screen.Recording.2023-08-09.at.20.35.46.mov

Mobile Web - Safari

ios.Web.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

…clude-empty-chats-from-lnh

PauloGasparSv · 2023-05-30T21:59:04Z

Having some trouble fixing the Travis errors in SideBarOrderTest and SideBarFilterTest, getting back to this tomorrow. There are some tests that create empty chats expecting them to be at LNH but I don't understand where these chat reports are being created or converted. (e.g. the includes or excludes policy expense chats depending on the beta test seems to create 1 policy expense chat but after the beta is added it becomes a chat report)

I also think I'll have to change the condition to include a chatType check but I'm not 100% sure if I should check for an empty one or maybe add !isUserCreatedPolicyRoom(report) && ! isDefaultRoom(report) there.

…clude-empty-chats-from-lnh

… in shouldReportBeInOptionList

…ould render in LNH

…clude-empty-chats-from-lnh

PauloGasparSv · 2023-06-05T20:03:51Z

I think I fixed all tests!

Since the SidebarOrderTest tests deal with ordering and checking the created/updated time of actions, instead of creating a comment inside the test so the reports stick to the LNH I mocked the lastMessageHTML field on the reports. This way I don't influence the last action time or anything related to those tests

…clude-empty-chats-from-lnh

PauloGasparSv · 2023-06-06T00:06:21Z

I think I still have a problem to fix:

Sending a message then going to another chat really quickly makes the Chat disappear then pop back in! I think there might be some optimistic data in the chat creation that I should be setting to avoid that.

Will fix this tomorrow

…clude-empty-chats-from-lnh

PauloGasparSv · 2023-06-06T19:16:20Z

I think I still have a problem to fix:

Sending a message then going to another chat really quickly makes the Chat disappear then pop back in! I think there might be some optimistic data in the chat creation that I should be setting to avoid that.

Will fix this tomorrow

Trying to reproduce this reliably to fix it, this is not always happening!

I managed to reproduce this a couple of times by deleting multiple comments inside the report and then going to the concierge chat really quickly.

I think we are not clearing lastMessageText somewhere correctly and that is generating the behaviour.

Still trying to understand how force that behavior to replicate this

Details

Screen.Recording.2023-06-06.at.16.10.29.mov

PauloGasparSv · 2023-08-10T00:15:50Z

I'm having trouble with the Android emulator so I'll add it's evidence tomorrow morning

…clude-empty-chats-from-lnh

luacmartins

LGTM

luacmartins · 2023-08-11T16:18:54Z

@thesahindia bump for review!

thesahindia · 2023-08-12T10:29:38Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-08-12.at.1.49.00.PM.mov

Screen.Recording.2023-08-12.at.1.44.33.PM.mov

Mobile Web - Chrome

Screen.Recording.2023-08-12.at.2.33.23.PM.mov

Mobile Web - Safari

Screen.Recording.2023-08-12.at.2.36.08.PM.mov

Desktop

Screen.Recording.2023-08-12.at.3.26.07.PM.mov

iOS

Screen.Recording.2023-08-12.at.3.07.23.PM.mov

Android

Screen.Recording.2023-08-12.at.2.18.48.PM.mov

thesahindia

Tested well!

P.S. Sorry for the delay. I am having internet issues.

melvin-bot · 2023-08-12T10:31:36Z

@joelbettner Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

luacmartins · 2023-08-13T02:40:37Z

Not sure why Joel got assigned since we already had an internal engineer assigned. Merging.

joh42 · 2023-08-13T13:20:56Z

Running into a failing unit test on main, seems to be from this PR:

@luacmartins @PauloGasparSv

Beamanator · 2023-08-14T12:10:38Z

+                        }),
+                    )
+
+                    // Then no reports are rendered in the LHN


Probably copy / pasta error? Should be "The report is rendered in the LHN"

Oh yes, thks for catching that!

I'll create a P.R. to fix that unit test that is failing + fix that comment.

PauloGasparSv · 2023-08-14T12:13:46Z

Running into a failing unit test on main, seems to be from this PR:

Thks for tagging me and reporting here, taking a look at that test right now : )

I see the other issues that tagged this are also mentioning that failed Task.

OSBotify · 2023-08-14T20:03:55Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.54-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-08-14T21:04:11Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.54-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-08-17T02:54:47Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.54-13 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

namhihi237 · 2023-09-12T18:27:14Z

@PauloGasparSv @Beamanator In that case :
Not-Empty chat should stay in LNH:

Open new report
Chat any message
Go to the thread and send a message
Delete the parent message
=> In a parent report we have a [Deleted message] with a threaded chat. So in that case do we show on LHN?

github-actions · 2023-09-12T18:27:28Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

Paulo Vale seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

Paulo Vale added 2 commits May 19, 2023 17:55

Exclude empty chat reports from LNH

4c79b7e

Fix typo in isChatReport call

aa395b7

PauloGasparSv self-assigned this May 19, 2023

PauloGasparSv changed the title ~~Paulogasparsv exclude empty chats from lnh~~ Exclude empty chats from LNH May 19, 2023

PauloGasparSv changed the title ~~Exclude empty chats from LNH~~ Exclude empty chats (that have no AddComemnt action and no drafts) from LNH May 19, 2023

PauloGasparSv changed the title ~~Exclude empty chats (that have no AddComemnt action and no drafts) from LNH~~ Exclude empty chats (that have no AddComemnt actions or message drafts) from LNH May 19, 2023

PauloGasparSv mentioned this pull request May 22, 2023

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

Closed

6 tasks

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

ce42cfc

…clude-empty-chats-from-lnh

PauloGasparSv changed the title ~~Exclude empty chats (that have no AddComemnt actions or message drafts) from LNH~~ Exclude empty chats (with no ADDCOMMENT actions or message drafts) from LNH May 30, 2023

Add empty chat LNH test to SidebarFilterTest

872a064

Paulo Vale added 10 commits June 2, 2023 14:27

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

b6aaafb

…clude-empty-chats-from-lnh

Add excludeEmptyChats logic to shouldReportBeInOptionList

9896a54

Exclude threads, default rooms and policy rooms from empty chat logic…

8c4344f

… in shouldReportBeInOptionList

Add another test to SidebarFilterTest for reports with drafts that sh…

1345009

…ould render in LNH

Fix policy expense beta test in SidebarFilterTest

b20f5b9

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

9142120

…clude-empty-chats-from-lnh

Linting

a1d349d

Fix empty chat with draft LNH test

9e7e220

Add lastMessageText to SideBarOrderTest reports so they show in LNH

3ba65be

Change LNH empty chat check order

a7777ed

Paulo Vale added 5 commits June 5, 2023 20:26

Do not include groups in logic that excludes empty chats from LNH

45c7b60

Fix group chat check for excluding empty chats from newDot

1af5873

Lint ReportUtils

74b557a

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

7bd4aff

…clude-empty-chats-from-lnh

Exclude empty group chats from the LNH

70c93d3

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

2e8178c

…clude-empty-chats-from-lnh

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…

ef0b220

…clude-empty-chats-from-lnh

PauloGasparSv requested a review from luacmartins August 10, 2023 15:54

luacmartins approved these changes Aug 10, 2023

View reviewed changes

thesahindia approved these changes Aug 12, 2023

View reviewed changes

melvin-bot Bot requested a review from joelbettner August 12, 2023 10:31

luacmartins merged commit a03a21d into main Aug 13, 2023

luacmartins deleted the paulogasparsv-exclude-empty-chats-from-lnh branch August 13, 2023 02:40

joh42 mentioned this pull request Aug 13, 2023

[OLD] Fix scroll on native when going back to the login page #24499

Closed

59 tasks

aimane-chnaif mentioned this pull request Aug 13, 2023

Hide reports with error from task share somewhere list #24506

Merged

59 tasks

situchan mentioned this pull request Aug 13, 2023

formatting phone login #24441

Merged

59 tasks

Beamanator reviewed Aug 14, 2023

View reviewed changes

This was referenced Aug 14, 2023

Paulogasparsv fix exclude comment from lnh tests #24530

Closed

Fix Sidebar Tests #24531

Merged

OSBotify mentioned this pull request Aug 14, 2023

Deploy Checklist: New Expensify 2023-08-14 #24550

Closed

49 tasks

Nikhil-Vats mentioned this pull request Aug 15, 2023

[HOLD for payment 2023-08-28] [HOLD for payment 2023-08-24] LNH - The conversation will be removed from the LHN if the last message is an attachment #24593

Closed

6 tasks

shawnborton mentioned this pull request Aug 23, 2023

App shows an unread count of 1 while no unreads appear in LHN #25774

Closed

sobitneupane mentioned this pull request Sep 5, 2023

[HOLD for payment 2023-09-29] [$1000] Task - Chat report for task assignee disappears from LHN when logged in on a different session #25727

Closed

6 tasks

dummy-1111 mentioned this pull request Sep 15, 2023

fix: empty checking of report #27499

Merged

59 tasks

Uh oh!

Conversation

PauloGasparSv commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Empty chat shouldn't stay in LNH

Chat should stay in LNH if it has a Draft

Not-Empty chat should stay in LNH

Deleting messages and leaving chat empty should remove it from LNH

Same tests w/ Group Chat

Money request should stay in LNH

Workspace Chats should stay in LNH

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

PauloGasparSv commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PauloGasparSv commented Jun 5, 2023

Uh oh!

PauloGasparSv commented Jun 6, 2023

Uh oh!

PauloGasparSv commented Jun 6, 2023

Uh oh!

PauloGasparSv commented Aug 10, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Aug 11, 2023

Uh oh!

thesahindia commented Aug 12, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

thesahindia left a comment

Choose a reason for hiding this comment

Uh oh!

melvin-bot Bot commented Aug 12, 2023

Uh oh!

luacmartins commented Aug 13, 2023

Uh oh!

joh42 commented Aug 13, 2023

Uh oh!

Beamanator Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

PauloGasparSv Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

PauloGasparSv commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Aug 14, 2023

Uh oh!

OSBotify commented Aug 14, 2023

Uh oh!

OSBotify commented Aug 17, 2023

Uh oh!

namhihi237 commented Sep 12, 2023

Uh oh!

github-actions Bot commented Sep 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PauloGasparSv commented May 19, 2023 •

edited

Loading

PauloGasparSv commented May 30, 2023 •

edited

Loading

PauloGasparSv commented Aug 14, 2023 •

edited

Loading