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/33760 doubled display name on search page #33966

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Fix/33760 doubled display name on search page #33966

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
16b3280
fix: remove doubled name for IOUs previews in SearchPage
koko57 Jan 3, 2024
2a924ae
fix: resolve conflicts
koko57 Jan 3, 2024
6eff84d
Merge branch 'main' into fix/33760-doubled-displayName-on-searchPage
koko57 Jan 4, 2024
11e24a3
fix: restore getDisplayNameOrDefault
koko57 Jan 4, 2024
a3f2abc
Merge branch 'main' into fix/33760-doubled-displayName-on-searchPage
koko57 Jan 8, 2024
37e83cd
fix: display actor name for threads
koko57 Jan 8, 2024
4b985db
fix: run prettier
koko57 Jan 8, 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
22 changes: 21 additions & 1 deletion src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ Onyx.connect({
const lastReportActions = {};
const allSortedReportActions = {};
const allReportActions = {};
const visibleReportActionItems = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
Expand All @@ -83,6 +84,18 @@ Onyx.connect({
const sortedReportActions = ReportActionUtils.getSortedReportActions(_.toArray(actions), true);
allSortedReportActions[reportID] = sortedReportActions;
lastReportActions[reportID] = _.first(sortedReportActions);

// The report is only visible if it is the last action not deleted that
// does not match a closed or created state.
const reportActionsForDisplay = _.filter(
sortedReportActions,
(reportAction, actionKey) =>
ReportActionUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.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 code is also duplicated with SidebarUtils.
Let's try to avoid duplication as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but I think that the whole code for generating could be extracted to a separate function - we would not need a separate function for getting lastActorDisplayName then. I even wanted to do this in this PR, but it's a lot of changes, so I wanted to fix the issue this way, then take care of this refactor in a follow-up. Let me know what you think about that cc @mountiny

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I agree to handle this in a follow up PR to get the fix in and limit chances of regressions. Will you handle the follow up @koko57 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, of course! 😊

},
});

Expand Down Expand Up @@ -519,7 +532,7 @@ function createOption(accountIDs, personalDetails, report, reportActions = {}, {
hasMultipleParticipants && lastActorDetails && lastActorDetails.accountID !== currentUserAccountID
? lastActorDetails.firstName || PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails)
: '';
let lastMessageText = lastActorDisplayName ? `${lastActorDisplayName}: ${lastMessageTextFromReport}` : lastMessageTextFromReport;
let lastMessageText = lastMessageTextFromReport;
mountiny marked this conversation as resolved.
Show resolved Hide resolved

if (result.isArchivedRoom) {
const archiveReason =
Expand All @@ -531,6 +544,13 @@ function createOption(accountIDs, personalDetails, report, reportActions = {}, {
});
}

const lastAction = visibleReportActionItems[report.reportID];
const shouldDisplayLastActorName = lastAction && lastAction.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastAction.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU;

if (shouldDisplayLastActorName && lastActorDisplayName && lastMessageTextFromReport) {
lastMessageText = `${lastActorDisplayName}: ${lastMessageTextFromReport}`;
Copy link
Contributor

@situchan situchan Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's define util function whether to add prefix or not and use it in both here and SidebarUtils

App/src/libs/SidebarUtils.ts

Lines 401 to 402 in 3ee24ec

} else if (lastAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;

We should make alternateText consistent between LHN and search page.

}

if (result.isThread || result.isMoneyRequestReport) {
result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');
} else if (result.isChatRoom || result.isPolicyExpenseChat) {
Expand Down
Loading