-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Replace all explicit hidden
comparisons with isHiddenParticipant()
#54146
Changes from 1 commit
0a0bf76
4898e6d
b416e25
c0a983b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1294,12 +1294,9 @@ function getDefaultNotificationPreferenceForReport(report: OnyxEntry<Report>): V | |
} | ||
|
||
/** | ||
* Get the notification preference given a report | ||
* 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; | ||
} | ||
|
||
|
@@ -1415,6 +1412,23 @@ function canCreateTaskInReport(report: OnyxEntry<Report>): boolean { | |
return true; | ||
} | ||
|
||
/** | ||
* For all intents and purposes a report that has no notificationPreference at all should be considered "hidden". | ||
* We will remove the 'hidden' field entirely once the backend changes for https://github.com/Expensify/Expensify/issues/450891 are done. | ||
*/ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see that. I think the confusion is that having this notification preference actually means both:
🤷♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gonna go with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
if (typeof reportOrPreference === 'object' && reportOrPreference !== null) { | ||
const notificationPreference = getReportNotificationPreference(reportOrPreference); | ||
return isHiddenParticipant(notificationPreference); | ||
} | ||
if (reportOrPreference === undefined || reportOrPreference == null) { | ||
mountiny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return true; | ||
} | ||
return reportOrPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN; | ||
} | ||
|
||
/** | ||
* Returns true if there are any guides accounts (team.expensify.com) in a list of accountIDs | ||
* by cross-referencing the accountIDs with personalDetails since guides that are participants | ||
|
@@ -1426,7 +1440,7 @@ function hasExpensifyGuidesEmails(accountIDs: number[]): boolean { | |
|
||
function getMostRecentlyVisitedReport(reports: Array<OnyxEntry<Report>>, reportMetadata: OnyxCollection<ReportMetadata>): OnyxEntry<Report> { | ||
const filteredReports = reports.filter((report) => { | ||
const shouldKeep = !isChatThread(report) || getReportNotificationPreference(report) !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN; | ||
const shouldKeep = !isChatThread(report) || !isHiddenParticipant(report); | ||
return shouldKeep && !!report?.reportID && !!(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]?.lastVisitTime ?? report?.lastReadTime); | ||
}); | ||
return lodashMaxBy(filteredReports, (a) => new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${a?.reportID}`]?.lastVisitTime ?? a?.lastReadTime ?? '').valueOf()); | ||
|
@@ -2233,7 +2247,7 @@ function getParticipantsAccountIDsForDisplay(report: OnyxEntry<Report>, shouldEx | |
return false; | ||
} | ||
|
||
if (shouldExcludeHidden && reportParticipants[accountID]?.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | ||
if (shouldExcludeHidden && isHiddenParticipant(reportParticipants[accountID]?.notificationPreference)) { | ||
return false; | ||
} | ||
|
||
|
@@ -8119,7 +8133,7 @@ function canJoinChat(report: OnyxEntry<Report>, parentReportAction: OnyxInputOrE | |
} | ||
|
||
// If the notification preference of the chat is not hidden that means we have already joined the chat | ||
if (getReportNotificationPreference(report) !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | ||
if (!isHiddenParticipant(report)) { | ||
return false; | ||
} | ||
|
||
|
@@ -8153,7 +8167,7 @@ function canLeaveChat(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boo | |
return false; | ||
} | ||
|
||
if (getReportNotificationPreference(report) === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | ||
if (isHiddenParticipant(report)) { | ||
return false; | ||
} | ||
|
||
|
@@ -8835,6 +8849,7 @@ export { | |
getAllReportActionsErrorsAndReportActionThatRequiresAttention, | ||
hasInvoiceReports, | ||
getReportMetadata, | ||
isHiddenParticipant, | ||
}; | ||
|
||
export type { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny / @youssef-lr / @c3024 Do you know why this is always defaulting to
CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN
instead ofgetDefaultNotificationPreferenceForReport(report)
?Sorry for asking you directly, but @marcaaron is ooo and you reviewed this.
I'm asking because:
#announce
room, we push the notificationPreference of all participants to everyone in the report. This means potentially pushing thousands of participants withnotificationPreference = always
notificationPreference = always
is the default for#announce
rooms (and maybe other rooms)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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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