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.

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

Exclude empty chats (with no ADDCOMMENT actions or message drafts) from LNH #19321

Merged
merged 49 commits into from
Aug 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
4c79b7e
Exclude empty chat reports from LNH
May 19, 2023
aa395b7
Fix typo in isChatReport call
May 19, 2023
ce42cfc
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
May 30, 2023
872a064
Add empty chat LNH test to SidebarFilterTest
May 30, 2023
b6aaafb
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 2, 2023
9896a54
Add excludeEmptyChats logic to shouldReportBeInOptionList
Jun 2, 2023
8c4344f
Exclude threads, default rooms and policy rooms from empty chat logic…
Jun 2, 2023
1345009
Add another test to SidebarFilterTest for reports with drafts that sh…
Jun 2, 2023
b20f5b9
Fix policy expense beta test in SidebarFilterTest
Jun 2, 2023
9142120
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 5, 2023
a1d349d
Linting
Jun 5, 2023
9e7e220
Fix empty chat with draft LNH test
Jun 5, 2023
3ba65be
Add lastMessageText to SideBarOrderTest reports so they show in LNH
Jun 5, 2023
a7777ed
Change LNH empty chat check order
Jun 5, 2023
45c7b60
Do not include groups in logic that excludes empty chats from LNH
Jun 5, 2023
1af5873
Fix group chat check for excluding empty chats from newDot
Jun 5, 2023
74b557a
Lint ReportUtils
Jun 5, 2023
7bd4aff
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 5, 2023
70c93d3
Exclude empty group chats from the LNH
Jun 6, 2023
2e8178c
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 6, 2023
b119af9
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 6, 2023
7283708
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 13, 2023
d529c54
Lint ReportUtils
Jun 13, 2023
1e97d17
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 14, 2023
7f9a53f
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 15, 2023
8a7971e
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 26, 2023
784532f
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 28, 2023
03fa3e9
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jun 29, 2023
066dceb
Fixing tests by using lastMessageText instead of lastMessageHtml
Jun 29, 2023
6161296
Fix new sidebarFilterTest tests
Jun 29, 2023
1e93d7b
Lint tests
Jun 29, 2023
3a3277d
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jul 5, 2023
31c76ab
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jul 6, 2023
8576cd0
Replace getChatByParticipantsByLoginList call with getChatByParticipa…
Jul 6, 2023
14d2c27
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Jul 6, 2023
c26cfdc
Undo previous change in navigateToAndOpenReport and call navigateToAn…
Jul 6, 2023
5bbaad3
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Aug 7, 2023
91135bf
Reintroduce excludeEmptyChats logic
Aug 7, 2023
311ef9a
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Aug 8, 2023
f05a02a
Reintroduce navigateToAndOpenReport login navigation
Aug 8, 2023
9c45580
Use getLastVisibleMessage to check for empty chats in shouldReportBeI…
Aug 8, 2023
aa01aff
Reuse isEmptyChat for empty chat threads check in shouldReportBeInOpt…
Aug 8, 2023
d9d1a7e
Fix typo and lint
Aug 8, 2023
b90eb07
Remove duplicated and faulty logic that hides empty threads
Aug 8, 2023
27cfda3
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Aug 9, 2023
32e84ec
Fix SidebarOrderTest tests
Aug 9, 2023
382691b
Keep empty policy expense chats in LNH
Aug 9, 2023
d059564
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Aug 9, 2023
ef0b220
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-ex…
Aug 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
12 changes: 10 additions & 2 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2341,9 +2341,10 @@ function canAccessReport(report, policies, betas, allReportActions) {
* @param {String[]} betas
* @param {Object} policies
* @param {Object} allReportActions
* @param {Boolean} excludeEmptyChats
* @returns {boolean}
*/
function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies, allReportActions) {
function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies, allReportActions, excludeEmptyChats = false) {
const isInDefaultMode = !isInGSDMode;

// Exclude reports that have no data because there wouldn't be anything to show in the option item.
Expand Down Expand Up @@ -2373,8 +2374,10 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep
return true;
}

const isEmptyChat = !ReportActionsUtils.getLastVisibleMessage(report.reportID).lastMessageText;

// Hide only chat threads that haven't been commented on (other threads are actionable)
if (isChatThread(report) && !report.lastMessageText) {
if (isChatThread(report) && isEmptyChat) {
return false;
}

Expand Down Expand Up @@ -2404,6 +2407,11 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep
return false;
}

// Hide chats between two users that haven't been commented on from the LNH
if (excludeEmptyChats && isEmptyChat && isChatReport(report) && !isChatRoom(report) && !isPolicyExpenseChat(report)) {
return false;
}

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p

// Filter out all the reports that shouldn't be displayed
const reportsToDisplay = _.filter(allReportsDict, (report) =>
ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReportsDict, betas, policies, allReportActions),
ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReportsDict, betas, policies, allReportActions, true),
);

if (_.isEmpty(reportsToDisplay)) {
Expand Down
58 changes: 57 additions & 1 deletion tests/unit/SidebarFilterTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import wrapOnyxWithWaitForPromisesToResolve from '../utils/wrapOnyxWithWaitForPr
import CONST from '../../src/CONST';
import DateUtils from '../../src/libs/DateUtils';
import * as Localize from '../../src/libs/Localize';
import * as Report from '../../src/libs/actions/Report';

// Be sure to include the mocked permissions library or else the beta tests won't work
jest.mock('../../src/libs/Permissions');
Expand Down Expand Up @@ -71,7 +72,59 @@ describe('Sidebar', () => {
);
});

it('includes or excludes policy expense chats depending on the beta', () => {
it('excludes an empty chat report', () => {
LHNTestUtils.getDefaultRenderedSidebarLinks();

// Given a new report
const report = LHNTestUtils.getFakeReport(['[email protected]', '[email protected]'], 0);

return (
waitForPromisesToResolve()
// When Onyx is updated to contain that report
.then(() =>
Onyx.multiSet({
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report,
}),
)

// Then no reports are rendered in the LHN
.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(0);
})
);
});

it('includes an empty chat report if it has a draft', () => {
LHNTestUtils.getDefaultRenderedSidebarLinks();

// Given a new report
const report = {
...LHNTestUtils.getFakeReport([1, 2], 0),
hasDraft: true,
};

return (
waitForPromisesToResolve()
// When Onyx is updated to contain that report
.then(() =>
Onyx.multiSet({
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report,
[ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails,
}),
)

// Then no reports are rendered in the LHN
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably copy / pasta error? Should be "The report is rendered in the LHN"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, thks for catching that!

I'll create a P.R. to fix that unit test that is failing + fix that comment.

.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(1);
})
);
});

it('includes or excludes policy expensechats depending on the beta', () => {
LHNTestUtils.getDefaultRenderedSidebarLinks();

// Given a policy expense report
Expand All @@ -92,6 +145,9 @@ describe('Sidebar', () => {
}),
)

// When the report has at least one ADDCOMMENT action to be rendered in the LNH
.then(() => Report.addComment(report.reportID, 'Hi, this is a comment'))

// Then no reports are rendered in the LHN
.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down
47 changes: 38 additions & 9 deletions tests/unit/SidebarOrderTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as LHNTestUtils from '../utils/LHNTestUtils';
import CONST from '../../src/CONST';
import DateUtils from '../../src/libs/DateUtils';
import * as Localize from '../../src/libs/Localize';
import * as Report from '../../src/libs/actions/Report';

// Be sure to include the mocked Permissions and Expensicons libraries or else the beta tests won't work
jest.mock('../../src/libs/Permissions');
Expand Down Expand Up @@ -106,15 +107,14 @@ describe('Sidebar', () => {
LHNTestUtils.getDefaultRenderedSidebarLinks();

// Given three unread reports in the recently updated order of 3, 2, 1
const report1 = {
...LHNTestUtils.getFakeReport([1, 2], 3),
};
const report2 = {
...LHNTestUtils.getFakeReport([3, 4], 2),
};
const report3 = {
...LHNTestUtils.getFakeReport([5, 6], 1),
};
const report1 = LHNTestUtils.getFakeReport([1, 2], 3);
const report2 = LHNTestUtils.getFakeReport([3, 4], 2);
const report3 = LHNTestUtils.getFakeReport([5, 6], 1);

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

return (
waitForPromisesToResolve()
Expand All @@ -133,6 +133,7 @@ describe('Sidebar', () => {
.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);

expect(displayNames).toHaveLength(3);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Five, Six');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Three, Four');
Expand All @@ -151,8 +152,15 @@ describe('Sidebar', () => {
};
const report2 = LHNTestUtils.getFakeReport([3, 4], 2);
const report3 = LHNTestUtils.getFakeReport([5, 6], 1);

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

const currentReportId = report1.reportID;
LHNTestUtils.getDefaultRenderedSidebarLinks(currentReportId);

return (
waitForPromisesToResolve()
// When Onyx is updated with the data and the sidebar re-renders
Expand Down Expand Up @@ -190,6 +198,11 @@ describe('Sidebar', () => {
const report2 = LHNTestUtils.getFakeReport([3, 4], 2);
const report3 = LHNTestUtils.getFakeReport([5, 6], 1);

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

return (
waitForPromisesToResolve()
// When Onyx is updated with the data and the sidebar re-renders
Expand Down Expand Up @@ -233,6 +246,12 @@ describe('Sidebar', () => {
hasDraft: true,
};
const report3 = LHNTestUtils.getFakeReport([5, 6], 1);

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

const currentReportId = report2.reportID;
LHNTestUtils.getDefaultRenderedSidebarLinks(currentReportId);

Expand Down Expand Up @@ -538,6 +557,11 @@ describe('Sidebar', () => {
const report2 = LHNTestUtils.getFakeReport([3, 4]);
const report3 = LHNTestUtils.getFakeReport([5, 6]);

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

// Given the user is in all betas
const betas = [CONST.BETAS.DEFAULT_ROOMS, CONST.BETAS.POLICY_ROOMS, CONST.BETAS.POLICY_EXPENSE_CHAT];
LHNTestUtils.getDefaultRenderedSidebarLinks('0');
Expand Down Expand Up @@ -764,6 +788,11 @@ describe('Sidebar', () => {
lastVisibleActionCreated,
};

// Each report has at least one ADDCOMMENT action so should be rendered in the LNH
Report.addComment(report1.reportID, 'Hi, this is a comment');
Report.addComment(report2.reportID, 'Hi, this is a comment');
Report.addComment(report3.reportID, 'Hi, this is a comment');

LHNTestUtils.getDefaultRenderedSidebarLinks('0');
return (
waitForPromisesToResolve()
Expand Down