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 empty chat displayed in focus mode #40336

Merged
merged 9 commits into from
May 10, 2024
Merged

fix empty chat displayed in focus mode #40336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a743ffc
fix empty chat displayed in focus mode
tienifr Apr 17, 2024
787ad3b
fix jest test
tienifr Apr 17, 2024
f63b00a
fix merge main
tienifr Apr 19, 2024
5ba3936
fix merge main
tienifr Apr 22, 2024
8b1fce7
fix rename isInGSDMode
tienifr Apr 22, 2024
53fecfa
fix update comment
tienifr Apr 24, 2024
36b0a8d
fix merge main
tienifr Apr 24, 2024
580e4a0
fix perf test
tienifr May 7, 2024
9509578
fix merge main
tienifr May 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
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1731,7 +1731,7 @@ function getOptions(
betas,
policies,
doesReportHaveViolations,
isInGSDMode: false,
isInFocusMode: false,
excludeEmptyChats: false,
includeSelfDM,
});
Expand Down
24 changes: 18 additions & 6 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4872,11 +4872,23 @@ function buildOptimisticMoneyRequestEntities(
return [createdActionForChat, createdActionForIOUReport, iouAction, transactionThread, createdActionForTransactionThread];
}

// Check if the report is empty, meaning it has no visible messages (i.e. only a "created" report action).
function isEmptyReport(report: OnyxEntry<Report>): boolean {
if (!report) {
return true;
}
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from this bug #43994, we should have used ReportUtils.getLastVisibleMessage instead of ReportActionsUtils.getLastVisibleMessage to avoid such a bug, more context in #43994 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have two functions? We should merge into one. Looking at the function names alone you can't tell which one to use

Copy link
Contributor

Choose a reason for hiding this comment

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

The only difference is that when a message is deleted:

  • ReportUtils.getLastVisibleMessage will return [Deleted message];
  • ReportActionsUtils.getLastVisibleMessage will return an empty string '';

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I agree this is error prone and will tend to lead to more bugs.

return !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;
}

function isUnread(report: OnyxEntry<Report>): boolean {
if (!report) {
return false;
}

if (isEmptyReport(report)) {
return false;
}
// lastVisibleActionCreated and lastReadTime are both datetime strings and can be compared directly
const lastVisibleActionCreated = report.lastVisibleActionCreated ?? '';
const lastReadTime = report.lastReadTime ?? '';
Expand Down Expand Up @@ -5009,7 +5021,7 @@ function hasViolations(reportID: string, transactionViolations: OnyxCollection<T
function shouldReportBeInOptionList({
report,
currentReportId,
isInGSDMode,
isInFocusMode,
betas,
policies,
excludeEmptyChats,
Expand All @@ -5018,14 +5030,14 @@ function shouldReportBeInOptionList({
}: {
report: OnyxEntry<Report>;
currentReportId: string;
isInGSDMode: boolean;
isInFocusMode: boolean;
betas: OnyxEntry<Beta[]>;
policies: OnyxCollection<Policy>;
excludeEmptyChats: boolean;
doesReportHaveViolations: boolean;
includeSelfDM?: boolean;
}) {
const isInDefaultMode = !isInGSDMode;
const isInDefaultMode = !isInFocusMode;
// Exclude reports that have no data because there wouldn't be anything to show in the option item.
// This can happen if data is currently loading from the server or a report is in various stages of being created.
// This can also happen for anyone accessing a public room or archived room for which they don't have access to the underlying policy.
Expand Down Expand Up @@ -5076,8 +5088,8 @@ function shouldReportBeInOptionList({
if (hasDraftComment || requiresAttentionFromCurrentUser(report)) {
return true;
}
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
const isEmptyChat = !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;

const isEmptyChat = isEmptyReport(report);
const canHideReport = shouldHideReport(report, currentReportId);

// Include reports if they are pinned
Expand All @@ -5104,7 +5116,7 @@ function shouldReportBeInOptionList({
}

// All unread chats (even archived ones) in GSD mode will be shown. This is because GSD mode is specifically for focusing the user on the most relevant chats, primarily, the unread ones
if (isInGSDMode) {
if (isInFocusMode) {
return isUnread(report) && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE;
}

Expand Down
6 changes: 3 additions & 3 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ function getOrderedReportIDs(
currentPolicyID = '',
policyMemberAccountIDs: number[] = [],
): string[] {
const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInGSDMode;
const isInFocusMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInFocusMode;
const allReportsDictValues = Object.values(allReports ?? {});

// Filter out all the reports that shouldn't be displayed
Expand Down Expand Up @@ -97,7 +97,7 @@ function getOrderedReportIDs(
return ReportUtils.shouldReportBeInOptionList({
report,
currentReportId: currentReportId ?? '',
isInGSDMode,
isInFocusMode,
betas,
policies: policies as OnyxCollection<Policy>,
excludeEmptyChats: true,
Expand Down
2 changes: 1 addition & 1 deletion src/libs/UnreadIndicatorUpdater/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export default function getUnreadReportsForUnreadIndicator(reports: OnyxCollecti
betas: [],
policies: {},
doesReportHaveViolations: false,
isInGSDMode: false,
isInFocusMode: false,
excludeEmptyChats: false,
}) &&
/**
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2501,7 +2501,7 @@ function navigateToMostRecentReport(currentReport: OnyxEntry<Report>) {
ReportUtils.shouldReportBeInOptionList({
report: sortedReport,
currentReportId: '',
isInGSDMode: false,
isInFocusMode: false,
betas: [],
policies: {},
excludeEmptyChats: true,
Expand Down
4 changes: 2 additions & 2 deletions tests/perf-test/ReportUtils.perf-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,13 @@ describe('ReportUtils', () => {
test('[ReportUtils] shouldReportBeInOptionList on 1k participant', async () => {
const report = {...createRandomReport(1), participantAccountIDs, type: CONST.REPORT.TYPE.CHAT};
const currentReportId = '2';
const isInGSDMode = true;
const isInFocusMode = true;
const betas = [CONST.BETAS.DEFAULT_ROOMS];
const policies = getMockedPolicies();

await waitForBatchedUpdates();
await measureFunction(() =>
ReportUtils.shouldReportBeInOptionList({report, currentReportId, isInGSDMode, betas, policies, doesReportHaveViolations: false, excludeEmptyChats: false}),
ReportUtils.shouldReportBeInOptionList({report, currentReportId, isInFocusMode, betas, policies, doesReportHaveViolations: false, excludeEmptyChats: false}),
);
});

Expand Down
2 changes: 1 addition & 1 deletion tests/perf-test/SidebarLinks.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const getMockedReportsMap = (length = 100) => {
const reportID = index + 1;
const participants = [1, 2];
const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;
const report = LHNTestUtils.getFakeReport(participants, 1, true);
const report = {...LHNTestUtils.getFakeReport(participants, 1, true), lastMessageText: 'hey'};

return [reportKey, report];
}),
Expand Down
16 changes: 10 additions & 6 deletions tests/unit/SidebarOrderTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,10 +862,10 @@ describe('Sidebar', () => {
it('alphabetizes chats', () => {
LHNTestUtils.getDefaultRenderedSidebarLinks();

const report1 = LHNTestUtils.getFakeReport([1, 2], 3, true);
const report2 = LHNTestUtils.getFakeReport([3, 4], 2, true);
const report3 = LHNTestUtils.getFakeReport([5, 6], 1, true);
const report4 = LHNTestUtils.getFakeReport([7, 8], 0, true);
const report1 = {...LHNTestUtils.getFakeReport([1, 2], 3, true), lastMessageText: 'test'};
Copy link
Contributor

Choose a reason for hiding this comment

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

Add lastMessageText to getFakeReport

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually let's not block on this. I see getFakeReport used in many places where we just extend its response

const report2 = {...LHNTestUtils.getFakeReport([3, 4], 2, true), lastMessageText: 'test'};
const report3 = {...LHNTestUtils.getFakeReport([5, 6], 1, true), lastMessageText: 'test'};
const report4 = {...LHNTestUtils.getFakeReport([7, 8], 0, true), lastMessageText: 'test'};

const reportCollectionDataSet: ReportCollectionDataSet = {
[`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`]: report1,
Expand Down Expand Up @@ -919,9 +919,13 @@ describe('Sidebar', () => {
chatType: CONST.REPORT.CHAT_TYPE.POLICY_ROOM,
statusNum: CONST.REPORT.STATUS_NUM.CLOSED,
stateNum: CONST.REPORT.STATE_NUM.APPROVED,
lastMessageText: 'test',
};
const report2 = LHNTestUtils.getFakeReport([3, 4], 2, true);
const report3 = LHNTestUtils.getFakeReport([5, 6], 1, true);
const report2 = {
...LHNTestUtils.getFakeReport([3, 4], 2, true),
lastMessageText: 'test',
};
const report3 = {...LHNTestUtils.getFakeReport([5, 6], 1, true), lastMessageText: 'test'};

// Given the user is in all betas
const betas = [CONST.BETAS.DEFAULT_ROOMS];
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/SidebarTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ describe('Sidebar', () => {
chatType: CONST.REPORT.CHAT_TYPE.POLICY_ROOM,
statusNum: CONST.REPORT.STATUS_NUM.CLOSED,
stateNum: CONST.REPORT.STATE_NUM.APPROVED,
lastMessageText: 'test',
};

const action = {
Expand Down Expand Up @@ -93,6 +94,7 @@ describe('Sidebar', () => {
chatType: CONST.REPORT.CHAT_TYPE.POLICY_ROOM,
statusNum: CONST.REPORT.STATUS_NUM.CLOSED,
stateNum: CONST.REPORT.STATE_NUM.APPROVED,
lastMessageText: 'test',
};
const action = {
...LHNTestUtils.getFakeReportAction('email1@test.com', 3),
Expand Down
32 changes: 27 additions & 5 deletions tests/unit/UnreadIndicatorUpdaterTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,23 @@ describe('UnreadIndicatorUpdaterTest', () => {
describe('should return correct number of unread reports', () => {
it('given last read time < last visible action created', () => {
const reportsToBeUsed = {
1: {reportID: '1', reportName: 'test', type: CONST.REPORT.TYPE.EXPENSE, lastReadTime: '2023-07-08 07:15:44.030', lastVisibleActionCreated: '2023-08-08 07:15:44.030'},
2: {reportID: '2', reportName: 'test', type: CONST.REPORT.TYPE.TASK, lastReadTime: '2023-02-05 09:12:05.000', lastVisibleActionCreated: '2023-02-06 07:15:44.030'},
3: {reportID: '3', reportName: 'test', type: CONST.REPORT.TYPE.TASK},
1: {
reportID: '1',
reportName: 'test',
type: CONST.REPORT.TYPE.EXPENSE,
lastReadTime: '2023-07-08 07:15:44.030',
lastVisibleActionCreated: '2023-08-08 07:15:44.030',
lastMessageText: 'test',
},
2: {
reportID: '2',
reportName: 'test',
type: CONST.REPORT.TYPE.TASK,
lastReadTime: '2023-02-05 09:12:05.000',
lastVisibleActionCreated: '2023-02-06 07:15:44.030',
lastMessageText: 'test',
},
3: {reportID: '3', reportName: 'test', type: CONST.REPORT.TYPE.TASK, lastMessageText: 'test'},
};
expect(getUnreadReportsForUnreadIndicator(reportsToBeUsed, '3').length).toBe(2);
});
Expand All @@ -31,9 +45,17 @@ describe('UnreadIndicatorUpdaterTest', () => {
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
lastReadTime: '2023-07-08 07:15:44.030',
lastVisibleActionCreated: '2023-08-08 07:15:44.030',
lastMessageText: 'test',
},
2: {
reportID: '2',
reportName: 'test',
type: CONST.REPORT.TYPE.TASK,
lastReadTime: '2023-02-05 09:12:05.000',
lastVisibleActionCreated: '2023-02-06 07:15:44.030',
lastMessageText: 'test',
},
2: {reportID: '2', reportName: 'test', type: CONST.REPORT.TYPE.TASK, lastReadTime: '2023-02-05 09:12:05.000', lastVisibleActionCreated: '2023-02-06 07:15:44.030'},
3: {reportID: '3', reportName: 'test', type: CONST.REPORT.TYPE.TASK},
3: {reportID: '3', reportName: 'test', type: CONST.REPORT.TYPE.TASK, lastMessageText: 'test'},
};
expect(getUnreadReportsForUnreadIndicator(reportsToBeUsed, '3').length).toBe(1);
});
Expand Down
Loading