Skip to content
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

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ function getPolicyExpenseReportOption(participant: Participant | ReportUtils.Opt
const expenseReport = ReportUtils.isPolicyExpenseChat(participant) ? ReportUtils.getReportOrDraftReport(participant.reportID) : null;

const visibleParticipantAccountIDs = Object.entries(expenseReport?.participants ?? {})
.filter(([, reportParticipant]) => reportParticipant && reportParticipant.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN)
.filter(([, reportParticipant]) => reportParticipant && !ReportUtils.isHiddenParticipant(reportParticipant.notificationPreference))
.map(([accountID]) => Number(accountID));

const option = createOption(
Expand Down Expand Up @@ -1787,7 +1787,7 @@ function getEmptyOptions(): Options {

function shouldUseBoldText(report: ReportUtils.OptionData): boolean {
const notificationPreference = report.notificationPreference ?? ReportUtils.getReportNotificationPreference(report);
return report.isUnread === true && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
return report.isUnread === true && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE && !ReportUtils.isHiddenParticipant(notificationPreference);
}

export {
Expand Down
33 changes: 24 additions & 9 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +1297 to 1301
Copy link
Contributor

@aldo-expensify aldo-expensify Mar 4, 2025

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 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 #announce room, we push the notificationPreference of all participants to everyone in the report. This means potentially pushing thousands of participants with notificationPreference = always
  • notificationPreference = always is the default for #announce rooms (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

Copy link
Contributor

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


Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think that isReportHidden or isHiddenForCurrentUser or something like that might be clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

  • hidden from other participants
  • hidden from the LHN

🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isReportHidden seems too broad since it's not the report but the participant that is actually "hidden".

currentUserHasHiddenNotificationPreference() would be the most explicit maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna go with isHiddenForCurrentUser() though I think it's fine.

Copy link
Contributor

@youssef-lr youssef-lr Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (typeof reportOrPreference === 'object' && reportOrPreference !== null) {
const notificationPreference = getReportNotificationPreference(reportOrPreference);
return isHiddenParticipant(notificationPreference);
}
if (reportOrPreference === undefined || reportOrPreference == null) {
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
Expand All @@ -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());
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -8835,6 +8849,7 @@ export {
getAllReportActionsErrorsAndReportActionThatRequiresAttention,
hasInvoiceReports,
getReportMetadata,
isHiddenParticipant,
};

export type {
Expand Down
2 changes: 1 addition & 1 deletion src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ function getOrderedReportIDs(
}
const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '-1', report?.parentReportActionID ?? '-1');
const doesReportHaveViolations = ReportUtils.shouldDisplayViolationsRBRInLHN(report, transactionViolations);
const isHidden = ReportUtils.getReportNotificationPreference(report) === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const isHidden = ReportUtils.isHiddenParticipant(report);
const isFocused = report.reportID === currentReportId;
const hasErrorsOtherThanFailedReceipt = ReportUtils.hasReportErrorsOtherThanFailedReceipt(report, doesReportHaveViolations, transactionViolations);
const isReportInAccessible = report?.errorFields?.notFound;
Expand Down
2 changes: 1 addition & 1 deletion src/libs/UnreadIndicatorUpdater/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function getUnreadReportsForUnreadIndicator(reports: OnyxCollection<Report>, cur
* Furthermore, muted reports may or may not appear in the LHN depending on priority mode,
* but they should not be considered in the unread indicator count.
*/
notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN &&
!ReportUtils.isHiddenParticipant(notificationPreference) &&
notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE
);
});
Expand Down
19 changes: 12 additions & 7 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -545,9 +545,8 @@ function addActions(reportID: string, text = '', file?: FileObject) {
};

const report = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
const shouldUpdateNotificationPrefernece = !isEmptyObject(report) && ReportUtils.getReportNotificationPreference(report) === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;

if (shouldUpdateNotificationPrefernece) {
const shouldUpdateNotificationPreference = !isEmptyObject(report) && ReportUtils.isHiddenParticipant(report);
if (shouldUpdateNotificationPreference) {
optimisticReport.participants = {
[currentUserAccountID]: {notificationPreference: ReportUtils.getDefaultNotificationPreferenceForReport(report)},
};
Expand Down Expand Up @@ -1932,7 +1931,7 @@ function toggleSubscribeToChildReport(childReportID = '-1', parentReportAction:
if (childReportID !== '-1') {
openReport(childReportID);
const parentReportActionID = parentReportAction?.reportActionID ?? '-1';
if (!prevNotificationPreference || prevNotificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
if (!prevNotificationPreference || ReportUtils.isHiddenParticipant(prevNotificationPreference)) {
updateNotificationPreference(childReportID, prevNotificationPreference, CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS, parentReportID, parentReportActionID);
} else {
updateNotificationPreference(childReportID, prevNotificationPreference, CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, parentReportID, parentReportActionID);
Expand All @@ -1957,8 +1956,9 @@ function toggleSubscribeToChildReport(childReportID = '-1', parentReportAction:

const participantLogins = PersonalDetailsUtils.getLoginsByAccountIDs(participantAccountIDs);
openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID);
const notificationPreference =
prevNotificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN ? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS : CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const notificationPreference = ReportUtils.isHiddenParticipant(prevNotificationPreference)
? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS
: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
updateNotificationPreference(newChat.reportID, prevNotificationPreference, notificationPreference, parentReportID, parentReportAction?.reportActionID);
}
}
Expand Down Expand Up @@ -3062,7 +3062,12 @@ function leaveRoom(reportID: string, isWorkspaceMemberLeavingWorkspaceRoom = fal
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`,
value: {[report.parentReportActionID]: {childReportNotificationPreference: ReportUtils.getReportNotificationPreference(report, false)}},
value: {
[report.parentReportActionID]: {
childReportNotificationPreference:
report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? ReportUtils.getDefaultNotificationPreferenceForReport(report),
},
},
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ function createTaskAndNavigate(
},
);

const shouldUpdateNotificationPreference = !isEmptyObject(parentReport) && ReportUtils.getReportNotificationPreference(parentReport) === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const shouldUpdateNotificationPreference = !isEmptyObject(parentReport) && ReportUtils.isHiddenParticipant(parentReport);
if (shouldUpdateNotificationPreference) {
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
Expand Down
5 changes: 2 additions & 3 deletions src/libs/actions/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as Pusher from '@libs/Pusher/pusher';
import PusherUtils from '@libs/PusherUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import playSound, {SOUNDS} from '@libs/Sound';
import playSoundExcludingMobile from '@libs/Sound/playSoundExcludingMobile';
import Visibility from '@libs/Visibility';
Expand Down Expand Up @@ -795,9 +796,7 @@ const isChannelMuted = (reportId: string) =>
Onyx.disconnect(connection);
const notificationPreference = report?.participants?.[currentUserAccountID]?.notificationPreference;

resolve(
!notificationPreference || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
);
resolve(!notificationPreference || notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE || ReportUtils.isHiddenParticipant(notificationPreference));
},
});
});
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ProfilePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function ProfilePage({route}: ProfilePageProps) {

const notificationPreferenceValue = ReportUtils.getReportNotificationPreference(report);

const shouldShowNotificationPreference = !isEmptyObject(report) && !isCurrentUser && notificationPreferenceValue !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const shouldShowNotificationPreference = !isEmptyObject(report) && !isCurrentUser && !ReportUtils.isHiddenParticipant(notificationPreferenceValue);
const notificationPreference = shouldShowNotificationPreference
? translate(`notificationPreferencesPage.notificationPreferences.${notificationPreferenceValue}` as TranslationPaths)
: '';
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReportDetailsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ function ReportDetailsPage({policies, report, route, reportMetadata}: ReportDeta
roomDescription = translate('newRoomPage.roomName');
}

const shouldShowNotificationPref = !isMoneyRequestReport && ReportUtils.getReportNotificationPreference(report) !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const shouldShowNotificationPref = !isMoneyRequestReport && !ReportUtils.isHiddenParticipant(report);
const shouldShowWriteCapability = !isMoneyRequestReport;
const shouldShowMenuItem = shouldShowNotificationPref || shouldShowWriteCapability || (!!report?.visibility && report.chatType !== CONST.REPORT.CHAT_TYPE.INVOICE);

Expand Down
2 changes: 1 addition & 1 deletion src/pages/RoomInvitePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function RoomInvitePage({
// Any existing participants and Expensify emails should not be eligible for invitation
const excludedUsers = useMemo(() => {
const visibleParticipantAccountIDs = Object.entries(report.participants ?? {})
.filter(([, participant]) => participant && participant.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN)
.filter(([, participant]) => participant && !ReportUtils.isHiddenParticipant(participant.notificationPreference))
.map(([accountID]) => Number(accountID));
return [...PersonalDetailsUtils.getLoginsByAccountIDs(visibleParticipantAccountIDs), ...CONST.EXPENSIFY_EMAILS].map((participant) =>
PhoneNumber.addSMSDomainIfPhoneNumber(participant),
Expand Down
9 changes: 1 addition & 8 deletions src/pages/home/ReportScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -557,14 +557,7 @@ function ReportScreen({route, currentReportID = '', navigation}: ReportScreenPro

// If a user has chosen to leave a thread, and then returns to it (e.g. with the back button), we need to call `openReport` again in order to allow the user to rejoin and to receive real-time updates
useEffect(() => {
if (
!shouldUseNarrowLayout ||
!isFocused ||
prevIsFocused ||
!ReportUtils.isChatThread(report) ||
ReportUtils.getReportNotificationPreference(report) !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN ||
isSingleTransactionView
) {
if (!shouldUseNarrowLayout || !isFocused || prevIsFocused || !ReportUtils.isChatThread(report) || !ReportUtils.isHiddenParticipant(report) || isSingleTransactionView) {
return;
}
Report.openReport(reportID ?? '');
Expand Down
6 changes: 2 additions & 4 deletions src/pages/settings/Report/NotificationPreferencePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,9 @@ function NotificationPreferencePage({report}: NotificationPreferencePageProps) {
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentNotificationPreference = ReportUtils.getReportNotificationPreference(report);
const shouldDisableNotificationPreferences =
ReportUtils.isArchivedRoom(report, reportNameValuePairs) ||
ReportUtils.isSelfDM(report) ||
(!isMoneyRequestReport && currentNotificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN);
ReportUtils.isArchivedRoom(report, reportNameValuePairs) || ReportUtils.isSelfDM(report) || (!isMoneyRequestReport && ReportUtils.isHiddenParticipant(currentNotificationPreference));
const notificationPreferenceOptions = Object.values(CONST.REPORT.NOTIFICATION_PREFERENCE)
.filter((pref) => pref !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN)
.filter((pref) => !ReportUtils.isHiddenParticipant(pref))
.map((preference) => ({
value: preference,
text: translate(`notificationPreferencesPage.notificationPreferences.${preference}`),
Expand Down
4 changes: 2 additions & 2 deletions src/pages/settings/Report/ReportSettingsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function ReportSettingsPage({report, policies, route}: ReportSettingsPageProps)
const shouldDisableSettings = isEmptyObject(report) || ReportUtils.isArchivedRoom(report, reportNameValuePairs) || ReportUtils.isSelfDM(report);
const notificationPreferenceValue = ReportUtils.getReportNotificationPreference(report);
const notificationPreference =
notificationPreferenceValue && notificationPreferenceValue !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN
notificationPreferenceValue && !ReportUtils.isHiddenParticipant(notificationPreferenceValue)
? translate(`notificationPreferencesPage.notificationPreferences.${notificationPreferenceValue}`)
: '';
const writeCapability = ReportUtils.isAdminRoom(report) ? CONST.REPORT.WRITE_CAPABILITIES.ADMINS : report?.writeCapability ?? CONST.REPORT.WRITE_CAPABILITIES.ALL;
Expand All @@ -45,7 +45,7 @@ function ReportSettingsPage({report, policies, route}: ReportSettingsPageProps)
const shouldAllowWriteCapabilityEditing = useMemo(() => ReportUtils.canEditWriteCapability(report, linkedWorkspace), [report, linkedWorkspace]);
const shouldAllowChangeVisibility = useMemo(() => ReportUtils.canEditRoomVisibility(report, linkedWorkspace), [report, linkedWorkspace]);

const shouldShowNotificationPref = !isMoneyRequestReport && notificationPreferenceValue !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const shouldShowNotificationPref = !isMoneyRequestReport && !ReportUtils.isHiddenParticipant(notificationPreferenceValue);

const shouldShowWriteCapability = !isMoneyRequestReport;

Expand Down
Loading