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.

Sign up for GitHub

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

Jump to bottom

Add notification preference for all the reports and update the design for the group chats #28200

Merged
merged 22 commits into from
Oct 10, 2023
Merged

Add notification preference for all the reports and update the design for the group chats #28200

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
32df987
update the design for group chats
techievivek Sep 25, 2023
78bcb7e
Merge branch 'main' into techievivek_add_notif_pref_all
techievivek Sep 26, 2023
d122a0d
fix profile page
techievivek Sep 27, 2023
c5946e3
updated details page to also count for group chats
techievivek Sep 27, 2023
cf82041
fix the spacing
techievivek Sep 27, 2023
083c924
use prettier
techievivek Sep 27, 2023
994140d
get chat report for profile page
techievivek Sep 27, 2023
5910aa9
Merge branch 'main' into techievivek_add_notif_pref_all
techievivek Sep 29, 2023
d4f11fa
Merge branch 'main' into techievivek_add_notif_pref_all
techievivek Oct 2, 2023
3df0e95
resolve conflicts
techievivek Oct 4, 2023
89fc4ae
revert the double withonyx change
techievivek Oct 4, 2023
5b731c0
Merge branch 'main' into techievivek_add_notif_pref_all
techievivek Oct 4, 2023
491e044
fix the report
techievivek Oct 4, 2023
f050e20
use the display name property
techievivek Oct 4, 2023
d21f21c
get rid of default value
techievivek Oct 4, 2023
3c880c6
don't show notification option if not chatted before
techievivek Oct 4, 2023
79560ff
use getReportName method
techievivek Oct 4, 2023
626ec91
Revert "revert the double withonyx change"
techievivek Oct 5, 2023
4417b5c
fix showing notification preference
techievivek Oct 5, 2023
b8a66ed
use correct property name
techievivek Oct 5, 2023
e2ee8df
Merge branch 'main' into techievivek_add_notif_pref_all
techievivek Oct 6, 2023
ac77a36
resolve conflicts
techievivek Oct 10, 2023
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
2 changes: 1 addition & 1 deletion src/libs/Notification/LocalNotification/BrowserNotifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export default {
const plainTextMessage = (_.find(message, (f) => f.type === 'COMMENT') || {}).text;

if (isChatRoom) {
const roomName = _.get(report, 'displayName', '');
const roomName = ReportUtils.getReportName(report);
title = roomName;
body = `${plainTextPerson}: ${plainTextMessage}`;
} else {
Expand Down
17 changes: 2 additions & 15 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,15 +1900,11 @@ function getParentNavigationSubtitle(report) {
function navigateToDetailsPage(report) {
const participantAccountIDs = lodashGet(report, 'participantAccountIDs', []);

if (isChatRoom(report) || isPolicyExpenseChat(report) || isChatThread(report) || isTaskReport(report) || isMoneyRequestReport(report)) {
Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID));
return;
}
if (participantAccountIDs.length === 1) {
if (isDM(report) && participantAccountIDs.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This created a regression which is fixed in #30182

Navigation.navigate(ROUTES.PROFILE.getRoute(participantAccountIDs[0]));
return;
}
Navigation.navigate(ROUTES.REPORT_PARTICIPANTS.getRoute(report.reportID));
Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID));
}

/**
Expand Down Expand Up @@ -3539,14 +3535,6 @@ function getPolicyExpenseChatReportIDByOwner(policyOwner) {
return expenseChat.reportID;
}

/*
* @param {Object|null} report
* @returns {Boolean}
*/
function shouldDisableSettings(report) {
return !isMoneyRequestReport(report) && !isPolicyExpenseChat(report) && !isChatRoom(report) && !isChatThread(report);
}

/**
* @param {Object|null} report
* @param {Object|null} policy - the workspace the report is on, null if the user isn't a member of the workspace
Expand Down Expand Up @@ -3895,7 +3883,6 @@ export {
isDM,
getPolicy,
getPolicyExpenseChatReportIDByOwner,
shouldDisableSettings,
shouldDisableRename,
hasSingleParticipant,
getReportRecipientAccountIDs,
Expand Down
33 changes: 29 additions & 4 deletions src/pages/ProfilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import variables from '../styles/variables';
import * as ValidationUtils from '../libs/ValidationUtils';
import Permissions from '../libs/Permissions';
import ROUTES from '../ROUTES';
import MenuItemWithTopDescription from '../components/MenuItemWithTopDescription';

const matchType = PropTypes.shape({
params: PropTypes.shape({
Expand Down Expand Up @@ -135,7 +136,7 @@ function ProfilePage(props) {

const navigateBackTo = lodashGet(props.route, 'params.backTo', ROUTES.HOME);

const chatReportWithCurrentUser = !isCurrentUser && !Session.isAnonymousUser() ? ReportUtils.getChatByParticipants([accountID]) : 0;
const notificationPreference = !_.isEmpty(props.report) ? props.translate(`notificationPreferencesPage.notificationPreferences.${props.report.notificationPreference}`) : '';

// eslint-disable-next-line rulesdir/prefer-early-return
useEffect(() => {
Expand Down Expand Up @@ -231,6 +232,15 @@ function ProfilePage(props) {
) : null}
{shouldShowLocalTime && <AutoUpdateTime timezone={timezone} />}
</View>
{!_.isEmpty(props.report) && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN && (
<MenuItemWithTopDescription
techievivek marked this conversation as resolved.
Show resolved Hide resolved
shouldShowRightIcon
title={notificationPreference}
techievivek marked this conversation as resolved.
Show resolved Hide resolved
description={props.translate('notificationPreferencesPage.label')}
onPress={() => Navigation.navigate(ROUTES.REPORT_SETTINGS_NOTIFICATION_PREFERENCES.getRoute(props.report.reportID))}
wrapperStyle={[styles.mtn6, styles.mb5]}
/>
)}
{!isCurrentUser && !Session.isAnonymousUser() && (
<MenuItem
title={`${props.translate('common.message')}${displayName}`}
Expand All @@ -241,15 +251,15 @@ function ProfilePage(props) {
shouldShowRightIcon
/>
)}
{!_.isEmpty(chatReportWithCurrentUser) && (
{!_.isEmpty(props.report) && (
<MenuItem
title={`${props.translate('privateNotes.title')}`}
titleStyle={styles.flex1}
icon={Expensicons.Pencil}
onPress={() => Navigation.navigate(ROUTES.PRIVATE_NOTES_LIST.getRoute(chatReportWithCurrentUser.reportID))}
onPress={() => Navigation.navigate(ROUTES.PRIVATE_NOTES_LIST.getRoute(props.report.reportID))}
wrapperStyle={styles.breakAll}
shouldShowRightIcon
brickRoadIndicator={Report.hasErrorInPrivateNotes(chatReportWithCurrentUser) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
brickRoadIndicator={Report.hasErrorInPrivateNotes(props.report) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
/>
)}
</ScrollView>
Expand Down Expand Up @@ -289,5 +299,20 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
session: {
key: ONYXKEYS.SESSION,
},
}),
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having second thoughts about this. What was the reason for adding the second withOnyx call? And could we fix this by just adding a privateNotesReportID to the user's session key maybe?

Copy link
Contributor Author

@techievivek techievivek Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm having second thoughts about this. What was the reason for adding the second withOnyx call?

Because we are dependent on the value of session props to see if the notes belong to the current user. We have a few use cases with similar patterns.

And could we fix this by just adding a privateNotesReportID to the user's session key maybe?

Users can have multiple private notes. On each chat, they have a different note. To say, private notes are report rNVPs characterized by the user's accountID(so each participant) can have their notes on chats they belong to.

For e.g. on chat reportID 1 a user can have notes `notes_1`
And for a different report `2` they can have notes as `notes_2`.

So do you think we can still add the key to the user's session?

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly didn't pay attention to the second call of withOnyx.

When withOnyx is used multiple times while composing components, it adds overhead to what Onyx has to keep track of and also prevents optimizations like batched render updates from working to their full potential.

From #27463 we created a rule to prevent multiple uses withOnyx, it's better we avoid it.

I know that having a report key listener here is to re-render the page and show the latest value for the preference menu item. So I think it's better we find another way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm okay it sounds like these private notes reportIDs should probably go in it's own key/collection. Something like privateNotesReportID_{accountID} though we should probably take that to #engi-chat to get second opinions.

In the meantime, did we really need to change the way we load this to begin with? What was wrong with the logic we had before here?

Copy link
Contributor Author

@techievivek techievivek Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm okay it sounds like these private notes reportIDs should probably go in it's own key/collection. Something like privateNotesReportID_{accountID} though we should probably take that to #engi-chat to get second opinions.

Yeah, I did bring that up in the design doc but we went with adding it as a key to report object. See here: https://docs.google.com/document/d/1StLBx3Y6yJsZp54rHMu-mGo20pUt1niV6Y0MPNDR6A4/edit?disco=AAAA0B4ClCw

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I did bring that up in the design doc but we went with adding it as a key to report object. See here: https://docs.google.com/document/d/1StLBx3Y6yJsZp54rHMu-mGo20pUt1niV6Y0MPNDR6A4/edit?disco=AAAA0B4ClCw

If I'm understanding correctly, It sounds like these private note reports are tied to accounts not reports right? If so I think the concerns in that thread were misguided 😄. Ideally we shouldn't have to load a report just to get the privateNotesReportID for an account. Either way, I guess there's no problem with the revert so we can probably just :donothing:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each user can have private notes on all the chat reports they are part of. If I have access to reports 1 and 2, then I can have private notes on both report 1 and report 2. We also discussed bringing public notes, so in that case, they won't be tied to accounts but only to the reports.

Ideally we shouldn't have to load a report just to get the privateNotesReportID for an account.

💯 agree with this. But it looks like we do that for a couple more things, no? like lastReadTime and stuff? Since there are no major concerns with how we are handling the data, I too will just stick with :donothing: for now.

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 seriously can't recall why I went with this change, I have reverted back everything to get rid of double withOnyx usage.

Now I know why I made that change
#28200 (review)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand that comment, but it sounds like there's urgency behind this so I won't be a blocker. However, can we create a follow up issue to clean this up and remove the need for a second Onyx subscription? I'm not sure exactly how it should be cleaned up but it definitely doesn't need to be like this. Thanks!

Copy link
Contributor Author

@techievivek techievivek Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Let me just reiterate the problem statement so it's clear for you.

We have now added the notification preference option on the profile page. The challenge is that the profile page doesn't subscribe to any report by default. Instead, it depends on the user profile you've navigated to. It's not just about fetching the chat report between you and another user; the ProfilePage needs to actively subscribe to it. This subscription is crucial because when you update your notification preference, the component should re-render and display the updated notification preference from the report.

This was first reported here: #28200 (comment) and we did figure out a better solution for this here #28200 (comment), which involved creating a separate component for the notification preference. This new component would subscribe to the DM chat report using Onyx, ensuring that when you update your notification preference, it automatically re-renders and displays the updated data.

We agreed that it might be overkill #28200 (comment) so we just so we opted to stick with a double Onyx call for now. A future solution is to create a dedicated notification preference page component, which can be reused throughout the entire application as needed.

withOnyx({
report: {
key: ({route, session}) => {
const accountID = Number(lodashGet(route.params, 'accountID', 0));
if (Number(session.accountID) === accountID || Session.isAnonymousUser()) {
return null;
}
return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`;
Comment on lines +311 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused regression when user opens new user profile link first time.
This happens because report_ returning all reports.
We should fix this by setting report_0 or report_null or null

Suggested change
if (Number(session.accountID) === accountID || Session.isAnonymousUser()) {
return null;
}
return `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '')}`;
const reportID = lodashGet(ReportUtils.getChatByParticipants([accountID]), 'reportID', '');
if (Number(session.accountID) === accountID || Session.isAnonymousUser() || !reportID) {
return null;
}
return `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;

Right now, app crashes and will be a deploy blocker soon

Screenshot 2023-10-11 at 10 51 16 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

raised PR for the quick fix

Copy link
Contributor

Choose a reason for hiding this comment

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

You haven't taken into account the dependency of getChatByParticipants on reports which caused #35654

},
},
}),
)(ProfilePage);
37 changes: 19 additions & 18 deletions src/pages/ReportDetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ const defaultProps = {
function ReportDetailsPage(props) {
const policy = useMemo(() => props.policies[`${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}`], [props.policies, props.report.policyID]);
const isPolicyAdmin = useMemo(() => PolicyUtils.isPolicyAdmin(policy), [policy]);
const shouldDisableSettings = useMemo(() => ReportUtils.shouldDisableSettings(props.report), [props.report]);
const shouldUseFullTitle = !shouldDisableSettings || ReportUtils.isTaskReport(props.report);
const shouldUseFullTitle = ReportUtils.isTaskReport(props.report);
Copy link
Contributor

@situchan situchan Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there reason for removing !shouldDisableSettings || condition here?
This caused regression - #29321

Edit: Ah I think I got answer here

const isChatRoom = useMemo(() => ReportUtils.isChatRoom(props.report), [props.report]);
const isThread = useMemo(() => ReportUtils.isChatThread(props.report), [props.report]);
const isUserCreatedPolicyRoom = useMemo(() => ReportUtils.isUserCreatedPolicyRoom(props.report), [props.report]);
Expand All @@ -75,16 +74,20 @@ function ReportDetailsPage(props) {
const canLeaveRoom = useMemo(() => ReportUtils.canLeaveRoom(props.report, !_.isEmpty(policy)), [policy, props.report]);
const participants = useMemo(() => ReportUtils.getParticipantsIDs(props.report), [props.report]);

const isGroupDMChat = useMemo(() => ReportUtils.isDM(props.report) && participants.length > 1, [props.report, participants.length]);
techievivek marked this conversation as resolved.
Show resolved Hide resolved

const menuItems = useMemo(() => {
const items = [
{
const items = [];

if (!isGroupDMChat) {
items.push({
key: CONST.REPORT_DETAILS_MENU_ITEM.SHARE_CODE,
translationKey: 'common.shareCode',
icon: Expensicons.QrCode,
isAnonymousAction: true,
action: () => Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS_SHARE_CODE.getRoute(props.report.reportID)),
},
];
});
}

if (isArchivedRoom) {
return items;
Expand All @@ -103,17 +106,15 @@ function ReportDetailsPage(props) {
});
}

if (!shouldDisableSettings) {
items.push({
key: CONST.REPORT_DETAILS_MENU_ITEM.SETTINGS,
translationKey: 'common.settings',
icon: Expensicons.Gear,
isAnonymousAction: false,
action: () => {
Navigation.navigate(ROUTES.REPORT_SETTINGS.getRoute(props.report.reportID));
},
});
}
items.push({
key: CONST.REPORT_DETAILS_MENU_ITEM.SETTINGS,
translationKey: 'common.settings',
icon: Expensicons.Gear,
isAnonymousAction: false,
action: () => {
Navigation.navigate(ROUTES.REPORT_SETTINGS.getRoute(props.report.reportID));
},
});

// Prevent displaying private notes option for threads and task reports
if (!isThread && !isMoneyRequestReport && !ReportUtils.isTaskReport(props.report)) {
Expand All @@ -138,7 +139,7 @@ function ReportDetailsPage(props) {
}

return items;
}, [isArchivedRoom, participants.length, shouldDisableSettings, isThread, isMoneyRequestReport, props.report, isUserCreatedPolicyRoom, canLeaveRoom]);
}, [isArchivedRoom, participants.length, isThread, isMoneyRequestReport, props.report, isUserCreatedPolicyRoom, canLeaveRoom, isGroupDMChat]);

const displayNamesWithTooltips = useMemo(() => {
const hasMultipleParticipants = participants.length > 1;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Report/NotificationPreferencePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const propTypes = {
const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};

function NotificationPreferencePage(props) {
const shouldDisableNotificationPreferences = ReportUtils.shouldDisableSettings(props.report) || ReportUtils.isArchivedRoom(props.report);
const shouldDisableNotificationPreferences = ReportUtils.isArchivedRoom(props.report);
const notificationPreferenceOptions = _.map(
_.filter(_.values(CONST.REPORT.NOTIFICATION_PREFERENCE), (pref) => pref !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN),
(preference) => ({
Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Report/ReportSettingsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function ReportSettingsPage(props) {
const shouldDisableWelcomeMessage =
isMoneyRequestReport || ReportUtils.isArchivedRoom(report) || !ReportUtils.isChatRoom(report) || _.isEmpty(linkedWorkspace) || linkedWorkspace.role !== CONST.POLICY.ROLE.ADMIN;

const shouldDisableSettings = _.isEmpty(report) || ReportUtils.shouldDisableSettings(report) || ReportUtils.isArchivedRoom(report);
const shouldDisableSettings = _.isEmpty(report) || ReportUtils.isArchivedRoom(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

After enabling settings for all the reports and revealing access to the notification preference page. We had to test the offline behavior for the newly enabled reports. Coming from #29334 this caused a regression where the creation of a new task missed the optimistic prop notificationPreference.

const shouldShowRoomName = !ReportUtils.isPolicyExpenseChat(report) && !ReportUtils.isChatThread(report);
const notificationPreference =
report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN
Expand Down
4 changes: 4 additions & 0 deletions src/styles/utilities/spacing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,10 @@ export default {
marginTop: 'auto',
},

mtn6: {
marginTop: -24,
},

mb0: {
marginBottom: 0,
},
Expand Down
Loading