Replace all explicit hidden comparisons with isHiddenParticipant()
#54146
Conversation
marcaaron
changed the title
hidden to use isHiddenPartici…hidden comparisons with isHiddenParticipant()
marcaaron
changed the title
hidden comparisons with isHiddenParticipant()hidden comparisons with isHiddenParticipant()
|
@mountiny so far, I am testing various things in the App, but doesn't seem like there are any major inconsistencies caused by theses changes. I think most reports have a default |
melvin-bot
bot
requested review from
youssef-lr
and removed request for
a team
|
@youssef-lr 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] |
src/libs/ReportUtils.ts
Outdated
| */ | ||
| function isHiddenParticipant(notificationPreference: string | null | undefined): boolean; | ||
| function isHiddenParticipant(report: OnyxEntry<Report>): boolean; | ||
| function isHiddenParticipant(reportOrPreference: OnyxEntry<Report> | string | null | undefined): boolean { |
There was a problem hiding this comment.
I feel like this is a misnomer, to me it looks like this function would be about a participant being hidden, and not that they have a "hidden" notification preference. Wdyt?
There was a problem hiding this comment.
Good point, I think that isReportHidden or isHiddenForCurrentUser or something like that might be clearer
There was a problem hiding this comment.
I can see that. I think the confusion is that having this notification preference actually means both:
- hidden from other participants
- hidden from the LHN
🤷♂️
There was a problem hiding this comment.
isReportHidden seems too broad since it's not the report but the participant that is actually "hidden".
currentUserHasHiddenNotificationPreference() would be the most explicit maybe.
There was a problem hiding this comment.
Gonna go with isHiddenForCurrentUser() though I think it's fine.
There was a problem hiding this comment.
I think hasHiddenNotificationPreference would work as well? we can deduce who by finding out whose preferences we're passing to the function. If it's always applied to the current user, I think we can just say so in the function docs.
|
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
src/libs/ReportUtils.ts
Outdated
| */ | ||
| function isHiddenParticipant(notificationPreference: string | null | undefined): boolean; | ||
| function isHiddenParticipant(report: OnyxEntry<Report>): boolean; | ||
| function isHiddenParticipant(reportOrPreference: OnyxEntry<Report> | string | null | undefined): boolean { |
There was a problem hiding this comment.
Good point, I think that isReportHidden or isHiddenForCurrentUser or something like that might be clearer
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
Gonna motion to let eslint puke on this one since we're touching a bunch of different flows here. |
| */ | ||
| function isHiddenForCurrentUser(notificationPreference: string | null | undefined): boolean; | ||
| function isHiddenForCurrentUser(report: OnyxEntry<Report>): boolean; | ||
| function isHiddenForCurrentUser(reportOrPreference: OnyxEntry<Report> | string | null | undefined): boolean { |
There was a problem hiding this comment.
I think we can avoid overloading here because the number of params for the function and the return type are same. reportOrPreference also looks clear enough IMO.
There was a problem hiding this comment.
Very fair, that would be cleaner
| if (typeof reportOrPreference === 'object' && reportOrPreference !== null) { | ||
| const notificationPreference = getReportNotificationPreference(reportOrPreference); | ||
| return isHiddenForCurrentUser(notificationPreference); | ||
| } | ||
| if (reportOrPreference === undefined || reportOrPreference === null || reportOrPreference === '') { | ||
| return true; |
There was a problem hiding this comment.
| if (typeof reportOrPreference === 'object' && reportOrPreference !== null) { | |
| const notificationPreference = getReportNotificationPreference(reportOrPreference); | |
| return isHiddenForCurrentUser(notificationPreference); | |
| } | |
| if (reportOrPreference === undefined || reportOrPreference === null || reportOrPreference === '') { | |
| return true; | |
| if (reportOrPreference === undefined || reportOrPreference === null || reportOrPreference === '') { | |
| return true; | |
| } | |
| if (typeof reportOrPreference === 'object') { | |
| const notificationPreference = getReportNotificationPreference(reportOrPreference); | |
| return isHiddenForCurrentUser(notificationPreference); | |
NIT
|
If the default should be hidden, can we simply check if the notification preference is not one of mute, daily, or always, and return |
|
Tested generally. Looks good. hiddenChrome.mp4 |
I think that would also work. Seems like a different way of doing the same thing to me. However, if we add a new notification preference down the line it would be hidden by default until the App starts to support it. Trying to think of some advantage... good thought though. |
|
If we create a new preference, I guess we will use it on the frontend as well and update it in const { HIDDEN, ...filteredNotificationPreference } = originalNotificationPreference; |
|
@c3024 what is your ETA for the checklist? |
|
I have tested briefly on my account and could not see any issues. |
| */ | ||
| function isHiddenForCurrentUser(notificationPreference: string | null | undefined): boolean; | ||
| function isHiddenForCurrentUser(report: OnyxEntry<Report>): boolean; | ||
| function isHiddenForCurrentUser(reportOrPreference: OnyxEntry<Report> | string | null | undefined): boolean { |
There was a problem hiding this comment.
Very fair, that would be cleaner
Reviewer Checklist
Screenshots/VideosAndroid: NativehiddenAndroid.movAndroid: mWeb ChromehiddenAndroidmWeb.mp4iOS: Native
iOS: mWeb SafarihiddeniOSmWeb.MP4MacOS: Chrome / SafarihiddenChrome.mp4MacOS: DesktophiddenDesktop.mov |
|
@MonilBhavsar 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] |
|
Only NAB left so I think we can merge now, thanks! |
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
The ESLint failures is a rule for default IDs, since thi PR is touching many files we opted not to fix it in this PR |
|
✋ 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/mountiny in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
2 similar comments
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.78-6 🚀
|
| * Get the notification preference given a report. This should ALWAYS default to 'hidden'. Do not change this! | ||
| */ | ||
| function getReportNotificationPreference(report: OnyxEntry<Report>, shouldDefaltToHidden = true): ValueOf<typeof CONST.REPORT.NOTIFICATION_PREFERENCE> { | ||
| if (!shouldDefaltToHidden) { | ||
| return report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? getDefaultNotificationPreferenceForReport(report); | ||
| } | ||
| function getReportNotificationPreference(report: OnyxEntry<Report>): ValueOf<typeof CONST.REPORT.NOTIFICATION_PREFERENCE> { | ||
| return report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN; | ||
| } |
There was a problem hiding this comment.
@mountiny / @youssef-lr / @c3024 Do you know why this is always defaulting to CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN instead of getDefaultNotificationPreferenceForReport(report)?
Sorry for asking you directly, but @marcaaron is ooo and you reviewed this.
I'm asking because:
- For the moment, if you comment in an
#announceroom, we push the notificationPreference of all participants to everyone in the report. This means potentially pushing thousands of participants withnotificationPreference = always notificationPreference = alwaysis the default for#announcerooms (and maybe other rooms)- I would like to stop pushing those default preferences because they cause OOM in the php layer, and for me it would make sense the frontend (App) to assume that if the notificationPreference of a participant is missing, it means that the value for it should be the default (
getDefaultNotificationPreferenceForReport) and not'hidden'.
Does this make sense to you?
Context: https://github.com/Expensify/Expensify/issues/473841#issuecomment-2683501084
There was a problem hiding this comment.
This is an alternative that would not require to mess with the defaults in the front end: https://github.com/Expensify/Expensify/issues/473841#issuecomment-2695868187
Explanation of Change
cc @mountiny
This PR essentially replaces all of our current comparison checks that looks for the
'hidden'notificationPreferenceand moves them to use a util method instead. This is because when we stop setting certain reports with'hidden'by default they will instead not have anynotificationPreferenceat all and we will take that to mean that they are "hidden".This probably touches a lot of stuff so, I'm not entirely sure if some things will break badly or not or what we should test here. Open to ideas.
The idea behind the util is that we can come back later and remove
'hidden'from all the places where we are setting it optimistically and then kind of deprecated'hidden'completely. But not sure if it's something we need to do as the first step.Once this is merged we can do the same in the backend and also fix all places where we are setting reports to
'hidden'by default.If there are any edge cases - e.g. things that should be shown but for some reason do not have a notificationPreference at all when initialized this PR will reveal it and we can think more about what to do.
Fixed Issues (First part)
https://github.com/Expensify/Expensify/issues/450891
Tests
This touches a lot of the app so it's difficult to give exact QA steps.
Generally speaking, we need to test the app for any regressions around LHN reports that are expected to be visible, but might no longer be appearing unexpectedly.
QA should keep this in mind while testing.
Verify that no errors appear in the JS console
Offline tests
❌
QA Steps
Same as "Tests"
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop