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

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 all 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
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 @@ -252,7 +252,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
Loading