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

Move the logic of excluding contacts of DMs already included in reports to filterOptions #50426

Merged
merged 13 commits into from
Oct 18, 2024
Merged

Move the logic of excluding contacts of DMs already included in reports to filterOptions #50426

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a50a4df
move report deduplicating logic to filterOptions
c3024 Oct 8, 2024
deb1ba1
prettier and lint
c3024 Oct 8, 2024
ffd08a4
add missed filter case and fix tests
c3024 Oct 8, 2024
68709d9
extract filtering to function, fix lint in tests
c3024 Oct 9, 2024
b317a18
fix lint
c3024 Oct 9, 2024
28b226d
correct filtering logic for personal details
c3024 Oct 9, 2024
b69ea35
enforce empty search term with non-empty options
c3024 Oct 14, 2024
dfab13e
fix params for all getFilteredOptions calls
c3024 Oct 16, 2024
dd001f4
fix lint
c3024 Oct 16, 2024
f9a76cf
migrate withOnyx to useOnyx
c3024 Oct 16, 2024
63d5b19
pass correct maxRecentReports to getFilteredOptions
c3024 Oct 17, 2024
3e72ecc
review comments
c3024 Oct 18, 2024
0519359
comments explaining need for types and function
c3024 Oct 18, 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: 14 additions & 8 deletions src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2073,11 +2073,6 @@ function getOptions(
}
}
}

// Add this login to the exclude list so it won't appear when we process the personal details
if (reportOption.login) {
optionsToExclude.push({login: reportOption.login});
}
}
}

Expand Down Expand Up @@ -2492,8 +2487,18 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt
preferPolicyExpenseChat = false,
preferRecentExpenseReports = false,
} = config ?? {};
function filteredPersonalDetailsOfRecentReports(recentReports: ReportUtils.OptionData[], personalDetails: ReportUtils.OptionData[]) {
const excludedLogins = new Set(recentReports.map((report) => report.login));
return personalDetails.filter((personalDetail) => !excludedLogins.has(personalDetail.login));
}
if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
const recentReports = options.recentReports.slice(0, maxRecentReportsToShow);
const personalDetails = filteredPersonalDetailsOfRecentReports(recentReports, options.personalDetails);
return {
...options,
recentReports,
personalDetails,
};
}

const parsedPhoneNumber = PhoneNumber.parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));
Expand Down Expand Up @@ -2535,7 +2540,6 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt
const currentUserOptionSearchText = items.currentUserOption ? uniqFast(getCurrentUserSearchTerms(items.currentUserOption)).join(' ') : '';

const currentUserOption = isSearchStringMatch(term, currentUserOptionSearchText) ? items.currentUserOption : null;

return {
recentReports: recentReports ?? [],
personalDetails: personalDetails ?? [],
Expand All @@ -2550,6 +2554,7 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt
let {recentReports, personalDetails} = matchResults;

if (sortByReportTypeInSearch) {
personalDetails = filteredPersonalDetailsOfRecentReports(recentReports, personalDetails);
recentReports = recentReports.concat(personalDetails);
personalDetails = [];
recentReports = orderOptions(recentReports, searchValue);
Expand All @@ -2569,9 +2574,10 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt
if (maxRecentReportsToShow > 0 && recentReports.length > maxRecentReportsToShow) {
recentReports.splice(maxRecentReportsToShow);
}
const filteredPersonalDetails = filteredPersonalDetailsOfRecentReports(recentReports, personalDetails);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case when maxRecentReportsToShow = 0 and recentReports is a long list? If so, then it seems there will be a performance downgrade here. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will but that might not be significant.

We iterate over all recentReports here for each search word.

App/src/libs/OptionsListUtils.ts

Lines 2520 to 2542 in 9d86f30

const recentReports = filterArrayByMatch(items.recentReports, term, (item) => {
const values: string[] = [];
if (item.text) {
values.push(item.text);
}
if (item.login) {
values.push(item.login);
values.push(item.login.replace(CONST.EMAIL_SEARCH_REGEX, ''));
}
if (item.isThread) {
if (item.alternateText) {
values.push(item.alternateText);
}
} else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) {
if (item.subtitle) {
values.push(item.subtitle);
}
}
return uniqFast(values);
});

We iterate over the recentReports once more for this function filteredPersonalDetailsOfRecentReports.

Since, we pass searchTerm only to this function filterOptions and not getOptions, we need to do this exclusion of contacts only here.


return {
personalDetails,
personalDetails: filteredPersonalDetails,
recentReports: orderOptions(recentReports, searchValue, {preferChatroomsOverThreads, preferPolicyExpenseChat, preferRecentExpenseReports}),
userToInvite,
currentUserOption: matchResults.currentUserOption,
Expand Down
82 changes: 52 additions & 30 deletions tests/unit/OptionsListUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,10 @@ describe('OptionsListUtils', () => {
it('getSearchOptions()', () => {
// When we filter in the Search view without providing a searchValue
const results = OptionsListUtils.getSearchOptions(OPTIONS, '', [CONST.BETAS.ALL]);
// Then the 2 personalDetails that don't have reports should be returned
expect(results.personalDetails.length).toBe(2);

// All 2 personalDetails (including those that have reports) should be returned
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(9);

// Then all of the reports should be shown including the archived rooms.
expect(results.recentReports.length).toBe(Object.values(OPTIONS.reports).length);
Expand All @@ -422,15 +424,21 @@ describe('OptionsListUtils', () => {
// We should expect maximimum of 5 recent reports to be returned
expect(results.recentReports.length).toBe(MAX_RECENT_REPORTS);

// We should expect all personalDetails to be returned,
// minus the currently logged in user and recent reports count
expect(results.personalDetails.length).toBe(Object.values(OPTIONS.personalDetails).length - 1 - MAX_RECENT_REPORTS);
// We should expect all personalDetails except the currently logged in user to be returned
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS.personalDetails).length - 1);

// All personal details including those that have reports should be returned
// We should expect personal details sorted alphabetically
expect(results.personalDetails.at(0)?.text).toBe('Black Widow');
expect(results.personalDetails.at(1)?.text).toBe('Invisible Woman');
expect(results.personalDetails.at(2)?.text).toBe('Spider-Man');
expect(results.personalDetails.at(3)?.text).toBe('The Incredible Hulk');
expect(results.personalDetails.at(0)?.text).toBe('Black Panther');
expect(results.personalDetails.at(1)?.text).toBe('Black Widow');
expect(results.personalDetails.at(2)?.text).toBe('Captain America');
expect(results.personalDetails.at(3)?.text).toBe('Invisible Woman');
expect(results.personalDetails.at(4)?.text).toBe('Mister Fantastic');
expect(results.personalDetails.at(5)?.text).toBe('Mr Sinister');
expect(results.personalDetails.at(6)?.text).toBe('Spider-Man');
expect(results.personalDetails.at(7)?.text).toBe('The Incredible Hulk');
expect(results.personalDetails.at(8)?.text).toBe('Thor');

// Then the result which has an existing report should also have the reportID attached
const personalDetailWithExistingReport = results.personalDetails.find((personalDetail) => personalDetail.login === '[email protected]');
Expand Down Expand Up @@ -481,29 +489,33 @@ describe('OptionsListUtils', () => {
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CONCIERGE.reports, OPTIONS_WITH_CONCIERGE.personalDetails);

// Concierge is included in the results by default. We should expect all the personalDetails to show
// (minus the 5 that are already showing and the currently logged in user)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 1 - MAX_RECENT_REPORTS);
// (minus the currently logged in user)
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 1);
expect(results.recentReports).toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Concierge from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CONCIERGE.reports, OPTIONS_WITH_CONCIERGE.personalDetails, [], '', [], [CONST.EMAIL.CONCIERGE]);

// All the personalDetails should be returned minus the currently logged in user and Concierge
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 2 - MAX_RECENT_REPORTS);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Chronos from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CHRONOS.reports, OPTIONS_WITH_CHRONOS.personalDetails, [], '', [], [CONST.EMAIL.CHRONOS]);

// All the personalDetails should be returned minus the currently logged in user and Concierge
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CHRONOS.personalDetails).length - 2 - MAX_RECENT_REPORTS);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CHRONOS.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Receipts from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_RECEIPTS.reports, OPTIONS_WITH_RECEIPTS.personalDetails, [], '', [], [CONST.EMAIL.RECEIPTS]);

// All the personalDetails should be returned minus the currently logged in user and Concierge
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_RECEIPTS.personalDetails).length - 2 - MAX_RECENT_REPORTS);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_RECEIPTS.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));
});

Expand All @@ -514,15 +526,21 @@ describe('OptionsListUtils', () => {
// Then we should expect only a maxmimum of 5 recent reports to be returned
expect(results.recentReports.length).toBe(5);

// And we should expect all the personalDetails to show (minus the 5 that are already
// showing and the currently logged in user)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS.personalDetails).length - 6);
// And we should expect all the personalDetails to show except the currently logged in user
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS.personalDetails).length - 1);

// All personal details including those that have reports should be returned
// We should expect personal details sorted alphabetically
expect(results.personalDetails.at(0)?.text).toBe('Black Widow');
expect(results.personalDetails.at(1)?.text).toBe('Invisible Woman');
expect(results.personalDetails.at(2)?.text).toBe('Spider-Man');
expect(results.personalDetails.at(3)?.text).toBe('The Incredible Hulk');
expect(results.personalDetails.at(0)?.text).toBe('Black Panther');
expect(results.personalDetails.at(1)?.text).toBe('Black Widow');
expect(results.personalDetails.at(2)?.text).toBe('Captain America');
expect(results.personalDetails.at(3)?.text).toBe('Invisible Woman');
expect(results.personalDetails.at(4)?.text).toBe('Mister Fantastic');
expect(results.personalDetails.at(5)?.text).toBe('Mr Sinister');
expect(results.personalDetails.at(6)?.text).toBe('Spider-Man');
expect(results.personalDetails.at(7)?.text).toBe('The Incredible Hulk');
expect(results.personalDetails.at(8)?.text).toBe('Thor');

// And none of our personalDetails should include any of the users with recent reports
const reportLogins = results.recentReports.map((reportOption) => reportOption.login);
Expand All @@ -548,34 +566,38 @@ describe('OptionsListUtils', () => {
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CONCIERGE.reports, OPTIONS_WITH_CONCIERGE.personalDetails);

// Concierge is included in the results by default. We should expect all the personalDetails to show
// (minus the 5 that are already showing and the currently logged in user)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 6);
// (minus the currently logged in user)
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 1);
expect(results.recentReports).toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Concierge from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CONCIERGE.reports, OPTIONS_WITH_CONCIERGE.personalDetails, [], '', [], [CONST.EMAIL.CONCIERGE]);

// We should expect all the personalDetails to show (minus the 5 that are already showing,
// We should expect all the personalDetails to show (minus
// the currently logged in user and Concierge)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 7);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CONCIERGE.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));
expect(results.recentReports).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Chronos from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_CHRONOS.reports, OPTIONS_WITH_CHRONOS.personalDetails, [], '', [], [CONST.EMAIL.CHRONOS]);

// We should expect all the personalDetails to show (minus the 5 that are already showing,
// We should expect all the personalDetails to show (minus
// the currently logged in user and Concierge)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CHRONOS.personalDetails).length - 7);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_CHRONOS.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));
expect(results.recentReports).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));

// Test by excluding Receipts from the results
results = OptionsListUtils.getFilteredOptions(OPTIONS_WITH_RECEIPTS.reports, OPTIONS_WITH_RECEIPTS.personalDetails, [], '', [], [CONST.EMAIL.RECEIPTS]);

// We should expect all the personalDetails to show (minus the 5 that are already showing,
// We should expect all the personalDetails to show (minus
// the currently logged in user and Concierge)
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_RECEIPTS.personalDetails).length - 7);
// Filtering of personalDetails that have reports is done in filterOptions
expect(results.personalDetails.length).toBe(Object.values(OPTIONS_WITH_RECEIPTS.personalDetails).length - 2);
expect(results.personalDetails).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));
expect(results.recentReports).not.toEqual(expect.arrayContaining([expect.objectContaining({login: '[email protected]'})]));
});
Expand Down Expand Up @@ -2610,7 +2632,7 @@ describe('OptionsListUtils', () => {
const options = OptionsListUtils.getSearchOptions(OPTIONS, '', [CONST.BETAS.ALL]);
const filteredOptions = OptionsListUtils.filterOptions(options, '');

expect(options.recentReports.length + options.personalDetails.length).toBe(filteredOptions.recentReports.length + filteredOptions.personalDetails.length);
expect(filteredOptions.recentReports.length + filteredOptions.personalDetails.length).toBe(12);
});

it('should return filtered options in correct order', () => {
Expand Down
Loading