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

perf: Filter options in Request Money and Send Money #40235

Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
977e122
add filtering to money requests
TMisiukiewicz Apr 12, 2024
d66c2e8
create optimistic user when filtering
TMisiukiewicz Apr 12, 2024
ce44bde
exclude already created users and restricted emails
TMisiukiewicz Apr 15, 2024
047f320
simplify generating header message
TMisiukiewicz Apr 15, 2024
ffa8eb5
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 15, 2024
ccf5350
fix error when loading options
TMisiukiewicz Apr 15, 2024
f28e5e8
fix typechecks and tests
TMisiukiewicz Apr 15, 2024
2b4c5c5
fix tests
TMisiukiewicz Apr 15, 2024
dd42f18
update filtering
TMisiukiewicz Apr 16, 2024
f558989
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 16, 2024
8822800
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 17, 2024
477260e
prettier
TMisiukiewicz Apr 17, 2024
f8834dc
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz Apr 18, 2024
bef018a
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 22, 2024
4aaef6c
get participants
TMisiukiewicz Apr 22, 2024
a8b581a
code review updates
TMisiukiewicz Apr 22, 2024
0bf553b
fix test
TMisiukiewicz Apr 22, 2024
e873c96
search recent by workspace name
TMisiukiewicz Apr 22, 2024
17b178e
update displaying recent reports
TMisiukiewicz Apr 22, 2024
e862eb9
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz Apr 22, 2024
05f60cb
update getFilteredOptions usage
TMisiukiewicz Apr 22, 2024
38147dc
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz Apr 23, 2024
767db86
resolve nab comments
TMisiukiewicz Apr 23, 2024
c17880b
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 24, 2024
6a669c6
code review updates
TMisiukiewicz Apr 24, 2024
d3fb666
use reduceRight when filtering
TMisiukiewicz Apr 24, 2024
4e3ec39
fix typecheck
TMisiukiewicz Apr 24, 2024
79a7140
add tests for canCreateOptimisticPersonalDetailOption
TMisiukiewicz Apr 24, 2024
80e5b83
add more tests for filterOptions
TMisiukiewicz Apr 24, 2024
3c822e5
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz Apr 25, 2024
e3b1afa
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz Apr 26, 2024
49635ed
fix types
TMisiukiewicz Apr 26, 2024
49a421f
fix tests
TMisiukiewicz Apr 26, 2024
712a381
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz Apr 29, 2024
146fbc9
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz May 6, 2024
4bb2ece
fix tests
TMisiukiewicz May 6, 2024
42e2aa9
update to max recent reports to show
TMisiukiewicz May 6, 2024
9504d74
fix not displaying workspaces in recents
TMisiukiewicz May 6, 2024
d894a22
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz May 10, 2024
07e7d50
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz May 13, 2024
ec4c747
Merge remote-tracking branch 'upstream/main' into perf/filter-money-r…
TMisiukiewicz May 13, 2024
d421d17
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz May 14, 2024
60c7149
Merge branch 'main' into perf/filter-money-request-participants
TMisiukiewicz May 17, 2024
d246f11
Merge branch 'main' into perf/filter-money-request-participants
rinej Jun 11, 2024
ad4a170
resolve conficts
rinej Jun 11, 2024
fb19c55
fix: fix typescript
rinej Jun 11, 2024
7716743
fix lint
rinej Jun 11, 2024
1d4cabc
adjust default return, fix tests
rinej Jun 11, 2024
c55c40f
fix: fix recent search logic
rinej Jun 17, 2024
cb671ac
Merge branch 'main' into perf/filter-money-request-participants
rinej Jun 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
1 change: 1 addition & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1514,6 +1514,7 @@ const CONST = {
},

IOU: {
MAX_RECENT_REPORTS_TO_SHOW: 5,
// This is the transactionID used when going through the create expense flow so that it mimics a real transaction (like the edit flow)
OPTIMISTIC_TRANSACTION_ID: '1',
// Note: These payment types are used when building IOU reportAction message values in the server and should
Expand Down
127 changes: 95 additions & 32 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ type Options = {

type PreviewConfig = {showChatPreviewLine?: boolean; forcePolicyNamePreview?: boolean; showPersonalDetails?: boolean};

type FilterOptionsConfig = Pick<
GetOptionsConfig,
'sortByReportTypeInSearch' | 'canInviteUser' | 'betas' | 'selectedOptions' | 'excludeUnknownUsers' | 'excludeLogins' | 'maxRecentReportsToShow'
>;

/**
* OptionsListUtils is used to build a list options passed to the OptionsList component. Several different UI views can
* be configured to display different results based on the options passed to the private getOptions() method. Public
Expand Down Expand Up @@ -1565,6 +1570,26 @@ function orderOptions(options: ReportUtils.OptionData[], searchValue: string | u
);
}

function canCreateOptimisticPersonalDetailOption({
Copy link
Contributor

Choose a reason for hiding this comment

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

what unit test coverage do we have for this function?

searchValue,
recentReportOptions,
personalDetailsOptions,
currentUserOption,
}: {
searchValue: string;
recentReportOptions: ReportUtils.OptionData[];
personalDetailsOptions: ReportUtils.OptionData[];
currentUserOption?: ReportUtils.OptionData | null;
excludeUnknownUsers: boolean;
}) {
const noOptions = recentReportOptions.length + personalDetailsOptions.length === 0 && !currentUserOption;
const noOptionsMatchExactly = !personalDetailsOptions
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
.concat(recentReportOptions)
.find((option) => option.login === PhoneNumber.addSMSDomainIfPhoneNumber(searchValue ?? '').toLowerCase() || option.login === searchValue?.toLowerCase());

return noOptions || noOptionsMatchExactly;
}

/**
* We create a new user option if the following conditions are satisfied:
* - There's no matching recent report and personal detail option
Expand Down Expand Up @@ -1941,23 +1966,26 @@ function getOptions(
currentUserOption = undefined;
}

const noOptions = recentReportOptions.length + personalDetailsOptions.length === 0 && !currentUserOption;
const noOptionsMatchExactly = !personalDetailsOptions
.concat(recentReportOptions)
.find((option) => option.login === PhoneNumber.addSMSDomainIfPhoneNumber(searchValue ?? '').toLowerCase() || option.login === searchValue?.toLowerCase());

const userToInvite =
noOptions || noOptionsMatchExactly
? getUserToInviteOption({
searchValue,
excludeUnknownUsers,
optionsToExclude,
selectedOptions,
betas,
reportActions,
showChatPreviewLine,
})
: null;
let userToInvite: ReportUtils.OptionData | null = null;
if (
canCreateOptimisticPersonalDetailOption({
searchValue,
recentReportOptions,
personalDetailsOptions,
currentUserOption,
excludeUnknownUsers,
})
) {
userToInvite = getUserToInviteOption({
searchValue,
excludeUnknownUsers,
optionsToExclude,
selectedOptions,
betas,
reportActions,
showChatPreviewLine,
});
}

// If we are prioritizing 1:1 chats in search, do it only once we started searching
if (sortByReportTypeInSearch && searchValue !== '') {
Expand Down Expand Up @@ -2075,13 +2103,13 @@ function getFilteredOptions(
canInviteUser = true,
includeSelectedOptions = false,
includeTaxRates = false,
maxRecentReportsToShow: number = CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW,
taxRates: TaxRatesWithDefault = {} as TaxRatesWithDefault,
includeSelfDM = false,
includePolicyReportFieldOptions = false,
policyReportFieldOptions: string[] = [],
recentlyUsedPolicyReportFieldOptions: string[] = [],
includePersonalDetails = true,
maxRecentReportsToShow = 5,
) {
return getOptions(
{reports, personalDetails},
Expand Down Expand Up @@ -2317,14 +2345,26 @@ function getFirstKeyForList(data?: Option[] | null) {
/**
* Filters options based on the search input value
*/
function filterOptions(options: Options, searchInputValue: string, betas: OnyxEntry<Beta[]> = []): Options {
const searchValue = getSearchValueForPhoneOrEmail(searchInputValue);
function filterOptions(options: Options, searchInputValue: string, config?: FilterOptionsConfig): Options {
const {sortByReportTypeInSearch = false, canInviteUser = true, betas = [], maxRecentReportsToShow = 0, excludeLogins = []} = config ?? {};
if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
}

const parsedPhoneNumber = PhoneNumber.parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));
const searchValue = parsedPhoneNumber.possible && parsedPhoneNumber.number?.e164 ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase();
const searchTerms = searchValue ? searchValue.split(' ') : [];

// The regex below is used to remove dots only from the local part of the user email (local-part@domain)
// so that we can match emails that have dots without explicitly writing the dots (e.g: fistlast@domain will match first.last@domain)
const emailRegex = /\.(?=[^\s@]*@)/g;

const optionsToExclude: Option[] = [{login: CONST.EMAIL.NOTIFICATIONS}];

excludeLogins.forEach((login) => {
optionsToExclude.push({login});
});

const getParticipantsLoginsArray = (item: ReportUtils.OptionData) => {
const keys: string[] = [];
const visibleChatMemberAccountIDs = item.participantsList ?? [];
Expand Down Expand Up @@ -2357,16 +2397,23 @@ function filterOptions(options: Options, searchInputValue: string, betas: OnyxEn
values.push(item.login.replace(emailRegex, ''));
}

if (!item.isChatRoom) {
const participantNames = getParticipantNames(item.participantsList ?? []);
values = values.concat(Array.from(participantNames));
}

if (item.isThread) {
if (item.alternateText) {
values.push(item.alternateText);
}
values = values.concat(getParticipantsLoginsArray(item));
} else if (!!item.isChatRoom || !!item.isPolicyExpenseChat) {
if (item.subtitle) {
values.push(item.subtitle);
}
} else {
values = values.concat(getParticipantsLoginsArray(item));
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 move this to the last else block, with that for the policy expense chat it does not consider the participants but only subtitle for the filter values. Due to that policy expense chats are missing in the Recent section while filtering.

Screen.Recording.2024-04-17.at.23.16.23.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recently this PR caused a big performance regression: #38887 and it was reverted in #40019. Since the filtering was merged somewhere close before reverting, I adjusted function to match the correct behavior. Seems like it works the same way on production

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, Ok. On prod I am not seeing those policy expense chat for the global search but it is shown for the request/send money participants filtration. I think that reverted PR tried manipulating the getSearchText but here for request money participants search I don't think we are making a call to getSearchText.

Screen.Recording.2024-04-18.at.23.40.09.mov

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 the reverted PR was adding all the participants of the chat to the searchText, so it became super inefficient, as some of the chats had thousands of participants.
getSearchText is called when options are created (first opening of any search page - then they are cached), then, when searching, we are not calling it anymore because we are not recreating the options. Once getSearchText is removed, the initial load of the list will speed up as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

That reverted PR is in production, doubt why it still shows the policy expense chats in the search filter for request/send money flow.

Screenshot 2024-04-22 at 01 00 19

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 re-reviewed the isSearchStringMatch function, responsible for searching a match in a string generated by getSearchText and found that for non-chatrooms, it also looks for matches in a participants list. Updated the filterOptions to match this behavior so now it should also display e.g. workspaces when you search by user name. Thanks for spotting this! 👍

}
values = values.concat(getParticipantsLoginsArray(item));

return uniqFast(values);
});
Expand All @@ -2385,20 +2432,35 @@ function filterOptions(options: Options, searchInputValue: string, betas: OnyxEn
};
}, options);

const recentReports = matchResults.recentReports.concat(matchResults.personalDetails);
const userToInvite =
recentReports.length === 0
? getUserToInviteOption({
searchValue,
betas,
})
: null;
let {recentReports, personalDetails} = matchResults;

if (sortByReportTypeInSearch) {
recentReports = recentReports.concat(personalDetails);
personalDetails = [];
recentReports = orderOptions(recentReports, searchValue);
}

let userToInvite = null;
if (canInviteUser) {
if (recentReports.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@TMisiukiewicz @rinej This causes a regression #44200 where due to the default sortByReportTypeInSearch is false which won't concat recentReports & personalDetails. With that even though we have entries in personalDetails it will add that entry to userToInvite as well because of this truthy condition.

I think we also need to check the personalDetails.length here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened a PR #44253 with a fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I will check it.

userToInvite = getUserToInviteOption({
searchValue,
betas,
selectedOptions: config?.selectedOptions,
optionsToExclude,
});
}
}

if (maxRecentReportsToShow > 0 && recentReports.length > maxRecentReportsToShow) {
recentReports.splice(maxRecentReportsToShow);
}

return {
personalDetails: [],
recentReports: orderOptions(recentReports, searchValue),
personalDetails,
recentReports,
userToInvite,
currentUserOption: null,
currentUserOption: matchResults.currentUserOption,
categoryOptions: [],
tagOptions: [],
taxRatesOptions: [],
Expand Down Expand Up @@ -2443,6 +2505,7 @@ export {
getReportOption,
getTaxRatesSection,
getFirstKeyForList,
canCreateOptimisticPersonalDetailOption,
};

export type {MemberForList, CategorySection, CategoryTreeSection, Options, OptionList, SearchOption, PayeePersonalDetails, Category, Tax, TaxRatesOption, Option, OptionTree};
2 changes: 1 addition & 1 deletion src/pages/ChatFinderPage/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ function ChatFinderPage({betas, isSearchingForReports, navigation}: ChatFinderPa
};
}

const newOptions = OptionsListUtils.filterOptions(searchOptions, debouncedSearchValue, betas);
const newOptions = OptionsListUtils.filterOptions(searchOptions, debouncedSearchValue, {sortByReportTypeInSearch: true, betas});
const header = OptionsListUtils.getHeaderMessage(newOptions.recentReports.length + Number(!!newOptions.userToInvite) > 0, false, debouncedSearchValue);
return {
recentReports: newOptions.recentReports,
Expand Down
2 changes: 2 additions & 0 deletions src/pages/EditReportFieldDropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import useDebouncedState from '@hooks/useDebouncedState';
import useLocalize from '@hooks/useLocalize';
import useTheme from '@hooks/useTheme';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {RecentlyUsedReportFields} from '@src/types/onyx';

Expand Down Expand Up @@ -86,6 +87,7 @@ function EditReportFieldDropdownPage({onSubmit, fieldKey, fieldValue, fieldOptio
false,
false,
undefined,
CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW,
undefined,
undefined,
true,
Expand Down
1 change: 1 addition & 0 deletions src/pages/NewChatPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function useOptions({isGroupChat}: NewChatPageProps) {
true,
undefined,
undefined,
CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW,
undefined,
true,
);
Expand Down
Loading
Loading