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

Fix inconsistent thread in group chat name on the details page and LHN #48046

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Fix inconsistent thread in group chat name on the details page and LHN #48046

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52f838d
fix inconsistent group name
bernhardoj Aug 27, 2024
acc115b
Merge branch 'main' into fix/47564-deleted-message-isnt-shown-for-del…
bernhardoj Sep 3, 2024
32f3257
fix inconsistent deprecated group DM title
bernhardoj Sep 3, 2024
379d8b1
Revert "fix inconsistent deprecated group DM title"
bernhardoj Sep 7, 2024
07987e7
Merge branch 'main' into fix/47564-deleted-message-isnt-shown-for-del…
bernhardoj Sep 7, 2024
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
5 changes: 1 addition & 4 deletions src/components/LHNOptionsList/OptionRowLHN.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,6 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti
const statusContent = formattedDate ? `${statusText ? `${statusText} ` : ''}(${formattedDate})` : statusText;
const isStatusVisible = !!emojiCode && ReportUtils.isOneOnOneChat(!isEmptyObject(report) ? report : undefined);

const isGroupChat = ReportUtils.isGroupChat(optionItem) || ReportUtils.isDeprecatedGroupDM(optionItem);

const fullTitle = isGroupChat ? ReportUtils.getGroupChatName(undefined, false, report) : optionItem.text;
const subscriptAvatarBorderColor = isFocused ? focusedBackgroundColor : theme.sidebar;
return (
<OfflineWithFeedback
Expand Down Expand Up @@ -213,7 +210,7 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}>
<DisplayNames
accessibilityLabel={translate('accessibilityHints.chatUserDisplayNames')}
fullTitle={fullTitle ?? ''}
fullTitle={optionItem.text ?? ''}
displayNamesWithTooltips={optionItem.displayNamesWithTooltips ?? []}
tooltipEnabled
numberOfLines={1}
Expand Down
4 changes: 2 additions & 2 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,9 @@ function getOptionData({
const status = personalDetail?.status ?? '';
const formattedLogin = Str.isSMSLogin(login) ? LocalePhoneNumber.formatPhoneNumber(login) : login;

// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
// We only create tooltips for the first 5 users or so since some reports have hundreds of users, causing performance to degrade.
const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(
(participantPersonalDetailList || []).slice(0, 10),
(participantPersonalDetailList || []).slice(0, 5),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how this can fix the inconsistent in name while the inconsistent is at the name beginning not at the end?

For example the current name is name3, name4, name5, name6, .... how .slice(0, 5) can fix it to name1, name2, name3, name4, name5, ...?

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, The issue is not fixed

Screenshot 2024-09-04 at 1 36 50 PM

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note getDisplayNamesWithTooltips use sort method but get getReportName is not

App/src/libs/ReportUtils.ts

Lines 2387 to 2396 in 0f26b8a

.sort((first, second) => {
// First sort by displayName/login
const displayNameLoginOrder = localeCompare(first.displayName, second.displayName);
if (displayNameLoginOrder !== 0) {
return displayNameLoginOrder;
}
// Then fallback on accountID as the final sorting criteria.
return first.accountID - second.accountID;
});

It looks LHN and header use getDisplayNamesWithTooltips and details page use getReportName, I just try edit getReportName and the only title was changed is details title. So we may need different fix for deprecated group chat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain how this can fix the inconsistent in name while the inconsistent is at the name beginning not at the end?

Since getDisplayNamesWithTooltips sorts the array by name, it's possible the account in index 5-9 has a "smaller" name.

Slicing it to 5 will make sure the account list will be the same between getDisplayNamesWithTooltips and getReportName.

Note getDisplayNamesWithTooltips use sort method but get getReportName is not
It looks LHN and header use getDisplayNamesWithTooltips and details page use getReportName

True, getDisplayNamesWithTooltips is used in LHN and header so there is a tooltip for each account.
Should we sort in getReportName too? Or should we remove the sort from getDisplayNamesWithTooltips?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should sort the both, but I prefer fixing it separately here #47803. I just wanted to confirm it is not a regression from our PR

hasMultipleParticipants,
undefined,
ReportUtils.isSelfDM(report),
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReportDetailsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ function ReportDetailsPage({policies, report, session, personalDetails}: ReportD

const shouldShowLeaveButton = ReportUtils.canLeaveChat(report, policy);

const reportName = ReportUtils.isDeprecatedGroupDM(report) || isGroupChat ? ReportUtils.getGroupChatName(undefined, false, report) : ReportUtils.getReportName(report);
const reportName = ReportUtils.getReportName(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to remove isDeprecatedGroupDM here? I see getReportName doesn't have a clear condition for isDeprecatedGroupDM

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 don't know why we don't check for isDeprecatedGroupDM in getReportName, but the other usages of getReportName doesn't check for isDeprecatedGroupDM too.

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think getReportName may cover it by the default return, but I am not sure about that. Do we have any test steps can help us to confirm whether the issue is reproduced on deprecated group and confirm this PR fix?

Copy link
Contributor Author

@bernhardoj bernhardoj Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I notice the default return too and it actually apply the limit by default.

App/src/libs/ReportUtils.ts

Lines 3774 to 3781 in 8c69c58

// Not a room or PolicyExpenseChat, generate title from first 5 other participants
const participantsWithoutCurrentUser: number[] = [];
Object.keys(report?.participants ?? {}).forEach((accountID) => {
const accID = Number(accountID);
if (accID !== currentUserAccountID && participantsWithoutCurrentUser.length < 5) {
participantsWithoutCurrentUser.push(accID);
}
});

Do we have any test steps can help us to test this PR on deprecated group?

I don't think there is a way to create a deprecated group anymore. The only way to test it by opening the old existing deprecated group DM.

I have one here and using the logic from main, the LHN title will show the participants with limit it to 5 participants, while the details page shows Chat Report as the name.

web.mp4

If we change it using this PR, the details title will show all the participants with a limit too.

web.mp4

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Aug 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am able to reproduce the issue and test the PR now. But why LHN title not the same as header and details titles on deprecated group?

Note, the below issue not have been happened on native android and iOS

Screenshot 2024-08-31 at 9 04 45 AM

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 haven't investigated it yet, but I notice it happens too without this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to investigate it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the investigations, but that fix still not works fine in all places, let's keep it to fix separately here #47803. I just wanted to confirm it is not a regression from our PR

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've reverted the commit. Lmk if I misunderstood it.


const additionalRoomDetails =
(isPolicyExpenseChat && !!report?.isOwnPolicyExpenseChat) || ReportUtils.isExpenseReport(report) || isPolicyExpenseChat || isInvoiceRoom
Expand Down
3 changes: 1 addition & 2 deletions src/pages/home/HeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ function HeaderView({report, parentReportAction, reportID, onNavigationMenuButto
const theme = useTheme();
const styles = useThemeStyles();
const isSelfDM = ReportUtils.isSelfDM(report);
const isGroupChat = ReportUtils.isGroupChat(report) || ReportUtils.isDeprecatedGroupDM(report);

const participants = ReportUtils.getParticipantsAccountIDsForDisplay(report).slice(0, 5);
const isMultipleParticipant = participants.length > 1;
Expand Down Expand Up @@ -129,7 +128,7 @@ function HeaderView({report, parentReportAction, reportID, onNavigationMenuButto
const brickRoadIndicator = ReportUtils.hasReportNameError(report) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';
const shouldShowBorderBottom = !isTaskReport || !shouldUseNarrowLayout;
const shouldDisableDetailPage = ReportUtils.shouldDisableDetailPage(report);
const shouldUseGroupTitle = isGroupChat && (!!report?.reportName || !isMultipleParticipant);
const shouldUseGroupTitle = ReportUtils.isGroupChat(report) && (!!report?.reportName || !isMultipleParticipant);
const isLoading = !report.reportID || !title;

return (
Expand Down
Loading