-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: hidden chat appears in finder page #50127
Conversation
@ZhenjaHorbach 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] |
@nkdengineer |
@nkdengineer |
@ZhenjaHorbach I need to re-record videos on all platforms, will update soon |
@ZhenjaHorbach this PR is ready for review |
Cool ! |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromeandroid-web.moviOS: Nativeios.moviOS: mWeb Safariios-web.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Plus I'm a little confused |
@ZhenjaHorbach I think if we decided to hide this chat on the finder page, we would not be able to find this chat using search. |
We should be able to find the hidden chat |
I approve these changes But only this question remains @tgolen |
The decision that we came to on Slack is that if a chat has "hidden" for the notification preference, it should appear in the LHN (and global search), but it should NOT be shown in bold which would indicate it is unread. So, I think this PR is incorrect if I read the tests properly. |
@tgolen So here is what we should do in this PR, right?
|
Correct, yeah. |
@nkdengineer |
@tgolen A bit confused here. Currently, we're not displaying the hidden chat in LHN except for some special cases in
Line 141 in 7947b8e
|
Sorry, I don't think I can answer that because I don't know what the impact of either decision would be. Can you help me understand the difference and what the impact might be of either choice? To restate the way I understand the bug today (going back to the OG description):
All this PR should do is stop making the chat shown as bold in the chat finder. |
@tgolen Alright, thanks for your detail. I got what I need to do now. Will update the PR soon. |
4756cdf
to
7ca5820
Compare
7ca5820
to
e31e6f6
Compare
@ZhenjaHorbach i updated code change and all screenshots, please check again |
Thanks ! |
@ZhenjaHorbach Based on the comment above, I removed |
Cool ! |
@nkdengineer After your changes we have no chats that are displayed in bold Line 1225 in a5a1003
As result shouldUseBoldText returns false But here we first need to find out why we are missing notificationPreference info in the LHN report We use Before 2024-10-17.22.10.38.movAfter 2024-10-17.22.10.00.mov |
I'm checking this. |
@ZhenjaHorbach The problem is |
I think everything will be fine now |
LGTM |
@tgolen looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.53-0 🚀
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.53-0 🚀
|
This wasn't an emergency. I just merged too quickly and didn't realize that there was a running test. |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.53-1 🚀
|
Details
Fixed Issues
$ #50100
PROPOSAL: #50100 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov