-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from all commits
977e122
d66c2e8
ce44bde
047f320
ffa8eb5
ccf5350
f28e5e8
2b4c5c5
dd42f18
f558989
8822800
477260e
f8834dc
bef018a
4aaef6c
a8b581a
0bf553b
e873c96
17b178e
e862eb9
05f60cb
38147dc
767db86
c17880b
6a669c6
d3fb666
4e3ec39
79a7140
80e5b83
3c822e5
e3b1afa
49635ed
49a421f
712a381
146fbc9
4bb2ece
42e2aa9
9504d74
d894a22
07e7d50
ec4c747
d421d17
60c7149
d246f11
ad4a170
fb19c55
7716743
1d4cabc
c55c40f
cb671ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,7 +216,10 @@ type Options = { | |
|
||
type PreviewConfig = {showChatPreviewLine?: boolean; forcePolicyNamePreview?: boolean; showPersonalDetails?: boolean}; | ||
|
||
type FilterOptionsConfig = Pick<GetOptionsConfig, 'betas'> & {preferChatroomsOverThreads: boolean}; | ||
type FilterOptionsConfig = Pick< | ||
GetOptionsConfig, | ||
'sortByReportTypeInSearch' | 'canInviteUser' | 'betas' | 'selectedOptions' | 'excludeUnknownUsers' | 'excludeLogins' | 'maxRecentReportsToShow' | ||
> & {preferChatroomsOverThreads?: boolean}; | ||
|
||
/** | ||
* OptionsListUtils is used to build a list options passed to the OptionsList component. Several different UI views can | ||
|
@@ -1638,6 +1641,26 @@ function orderOptions(options: ReportUtils.OptionData[], searchValue: string | u | |
); | ||
} | ||
|
||
function canCreateOptimisticPersonalDetailOption({ | ||
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 | ||
|
@@ -2024,23 +2047,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 !== '') { | ||
|
@@ -2148,12 +2174,12 @@ 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[] = [], | ||
maxRecentReportsToShow = 5, | ||
includeInvoiceRooms = false, | ||
) { | ||
return getOptions( | ||
|
@@ -2390,14 +2416,25 @@ function getFirstKeyForList(data?: Option[] | null) { | |
* Filters options based on the search input value | ||
*/ | ||
function filterOptions(options: Options, searchInputValue: string, config?: FilterOptionsConfig): Options { | ||
const {betas = [], preferChatroomsOverThreads = false} = config ?? {}; | ||
const searchValue = getSearchValueForPhoneOrEmail(searchInputValue); | ||
const {sortByReportTypeInSearch = false, canInviteUser = true, betas = [], maxRecentReportsToShow = 0, excludeLogins = [], preferChatroomsOverThreads = false} = 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 ?? []; | ||
|
@@ -2430,16 +2467,23 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt | |
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Screen.Recording.2024-04-18.at.23.40.09.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I re-reviewed the |
||
} | ||
values = values.concat(getParticipantsLoginsArray(item)); | ||
|
||
return uniqFast(values); | ||
}); | ||
|
@@ -2458,20 +2502,35 @@ function filterOptions(options: Options, searchInputValue: string, config?: Filt | |
}; | ||
}, 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TMisiukiewicz @rinej This causes a regression #44200 where due to the default I think we also need to check the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opened a PR #44253 with a fix There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: [], | ||
personalDetails, | ||
recentReports: orderOptions(recentReports, searchValue, {preferChatroomsOverThreads}), | ||
userToInvite, | ||
currentUserOption: null, | ||
currentUserOption: matchResults.currentUserOption, | ||
categoryOptions: [], | ||
tagOptions: [], | ||
taxRatesOptions: [], | ||
|
@@ -2515,6 +2574,7 @@ export { | |
getReportOption, | ||
getTaxRatesSection, | ||
getFirstKeyForList, | ||
canCreateOptimisticPersonalDetailOption, | ||
getUserToInviteOption, | ||
}; | ||
|
||
|
There was a problem hiding this comment.
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?