[$250] Submit Expense - Search list when submitting expense, doesn´t display the contacts section

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.25-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4890824&group_by=cases:section_id&group_order=asc&group_id=229066
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the staging.new.expensify.com website.
2. Click on the Global Action Button.
3. Click on "Submit Expense" or "Split Expense" and enter any amount.
4. Click on "Next"
5. Verify the search list is divided by "Recents" and "Contacts"

Expected Result:

During the expense submitting or splitting process, the search list, should be divided in "Recents" and "Contacts"

Actual Result:

Search list when submitting or splitting an expense, only displays the "Recents" section and not the "Contacts" one

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6584155_1724766273965.Contacts-Recents.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016d2f5d8be09bffb6
• Upwork Job ID: 1829406098502441268
• Last Price Increase: 2024-09-27

Issue Owner

Current Issue Owner: @eh2077

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[tsa321]

Edited by proposal-police: This proposal was edited at 2024-08-28 06:51:06 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search list when submitting expense, doesn't display contacts section

What is the root cause of that problem?

When submitting an expense, if there is no chat history with a contact in the contact list, it will be included in the Contacts section. However, if there is chat history, it will be included in recentReports or just Chats. The issue is that recentReports is limited to a maximum of 5 recent reports (because of the slice and splice in below code), causing the remaining reports to be discarded:

App/src/libs/OptionsListUtils.ts

Lines 2391 to 2393 in 071f11c

	if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
	return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
	}

and here:

App/src/libs/OptionsListUtils.ts

Lines 2465 to 2467 in 071f11c

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

As a result, it seems that some contacts are missing, and many reports are not displayed due to this filtering (slice / splice).

What changes do you think we should make in order to solve the problem?

We could store the remaining reports from recentReports into a separate olderReports (or another name) data and return it to the caller in MoneyRequestParticipantsSelector (must check other caller if necessary).

When generating section data:

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Lines 192 to 204 in 071f11c

	newSections.push(formatResults.section);
	newSections.push({
	title: translate('common.recents'),
	data: chatOptions.recentReports,
	shouldShow: chatOptions.recentReports.length > 0,
	});
	newSections.push({
	title: translate('common.contacts'),
	data: chatOptions.personalDetails,
	shouldShow: chatOptions.personalDetails.length > 0,
	});

include the olderReports data. The sketch code changes could be something like this. We must also check other usage of filterOptions and adjust the code accordingly. and If we want to re-order the olderReports by other criteria (other than date), we can adjust it there.

What alternative solutions did you explore? (Optional)

The alternative solution is to retrieve DM reports from older or discarded reports and include them in the contact list.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016d2f5d8be09bffb6

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

[c3024]

Edited by proposal-police: This proposal was edited at 2024-09-19 10:01:46 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Contacts section is missing in the participant selector though they are not included in Recents.

What is the root cause of that problem?

We get defaultOptions here

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Line 85 in fc849dd

const defaultOptions = useMemo(() => {

with

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Line 99 in fc849dd

const optionList = OptionsListUtils.getFilteredOptions(

and we pass maxRecentReportsToShow as 0 here

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Line 123 in fc849dd

0,

which means we do not limit the max recent reports for the result from this function.

getFilteredOptions calls getOptions

App/src/libs/OptionsListUtils.ts

Line 2138 in fc849dd

return getOptions(

which in turn keeps adding the recent reports and continues adding all because maxRecentReportsToShow is zero.

App/src/libs/OptionsListUtils.ts

Lines 1930 to 1931 in fc849dd

	if (recentReportOptions.length > 0 && recentReportOptions.length === maxRecentReportsToShow) {
	break;

Along with this it also excludes the personal detail of these report logins here

App/src/libs/OptionsListUtils.ts

Lines 1980 to 1982 in fc849dd

	if (reportOption.login) {
	optionsToExclude.push({login: reportOption.login});
	}

Since, we are not limiting the recent reports to show so all these reportOption logins are excluded when processing personal details into options here. So, the default options returned from here includes all recent reports but do not include these personal details.

Back here in the MoneyRequestParticipantsSelector

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Line 167 in fc849dd

maxRecentReportsToShow: CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW,

we restrict the reports only to MAX_RECENT_REPORTS_TO_SHOW so only those reports show up and many personal details were already excluded earlier in the default options because of addition of all recent reports as explained earlier.

What changes do you think we should make in order to solve the problem?

1. We can consider moving max recent report limit to getFilteredOptions. This works well when there is no search term but when there is a search term the results it return may not include the correct results because the search term is not passed to this function.
2. Since we are filtering the options based on search term in filterOptions and this is where we are restricting the max reports also to MAX_RECENT_REPORTS_TO_SHOW, we can move the personal details excluding logic to filterOptions.

That is to remove this logic

App/src/libs/OptionsListUtils.ts

Lines 1981 to 1984 in 0e9f918

	// 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});
	}

of excluding personal details for included reports from getOptions and move that to filterOptions with something like this

if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
        // return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
        const recentReports = options.recentReports.slice(0, maxRecentReportsToShow);
        const excludedLogins = new Set(recentReports.map(report => report.login));
        const filteredPersonalDetails = options.personalDetails.filter(personalDetail => !excludedLogins.has(personalDetail.login));
        return {
            ...options,
            recentReports,
            personalDetails: filteredPersonalDetails,
        };
            
    }

at

App/src/libs/OptionsListUtils.ts

Lines 2393 to 2395 in 0e9f918

	if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
	return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
	}

and

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

    const excludedLogins = new Set(recentReports.map(report => report.login));
    const filteredPersonalDetails = personalDetails.filter(personalDetail => !excludedLogins.has(personalDetail.login));
    personalDetails = filteredPersonalDetails;

at

App/src/libs/OptionsListUtils.ts

Lines 2467 to 2469 in 0e9f918

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

This movement does not break anything because wherever we use getFilteredOptions or getOptions with reports in the repo we also filter those again with filteredOptions.

Here is the branch with these changes.

What alternative solutions did you explore? (Optional)

1. We can pass the maxReportsToShow to the getFilteredOptions function.
2. Then we can also pass the debouncedTerm to getFilteredOptions and this is not an expensive operation so doing the filtering with search term in both getFilteredOptions and filterOptions is also a viable solution and it retains the report-contact sections separation within getOptions function itself.

[melvin-bot]

@kadiealexander, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[eh2077]

Reviewing proposals

[eh2077]

Thanks for your proposals!

I think @c3024 pointed out the correct root cause of this issue. Their solution also looks good to me. So, let's go with @c3024 's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[trjExpensify]

Do we know which PR broke this?

[tsa321]

@eh2077, after testing @c3024's proposal by changing:

App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx

Line 123 in fc849dd

0,

to CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW, some reports are still missing. I tested it using my account, which has many reports and contacts. Here is in staging with the Workspace report included:

With @c3024's proposal, some reports are missing (notice the missing workspace reports):

some.reports.missing.mp4

With my proposal, all reports are shown (including workspace reports):

my-proposal.mp4

@eh2077 could you re-review my proposal again?
Thank you...

[c3024]

With @c3024's proposal, some reports are missing (notice the missing workspace reports):

Yes, I missed that.

I think the better solution is the alternative solution proposed. That is to remove this logic

App/src/libs/OptionsListUtils.ts

Lines 1981 to 1984 in 0e9f918

	// 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});
	}

of excluding personal details for included reports from getOptions and move that to filterOptions with something like this

if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
        // return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
        const recentReports = options.recentReports.slice(0, maxRecentReportsToShow);
        const excludedLogins = new Set(recentReports.map(report => report.login));
        const filteredPersonalDetails = options.personalDetails.filter(personalDetail => !excludedLogins.has(personalDetail.login));
        return {
            ...options,
            recentReports,
            personalDetails: filteredPersonalDetails,
        };
            
    }

at

App/src/libs/OptionsListUtils.ts

Lines 2393 to 2395 in 0e9f918

	if (searchInputValue.trim() === '' && maxRecentReportsToShow > 0) {
	return {...options, recentReports: options.recentReports.slice(0, maxRecentReportsToShow)};
	}

and

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

    const excludedLogins = new Set(recentReports.map(report => report.login));
    const filteredPersonalDetails = personalDetails.filter(personalDetail => !excludedLogins.has(personalDetail.login));
    personalDetails = filteredPersonalDetails;

at

App/src/libs/OptionsListUtils.ts

Lines 2467 to 2469 in 0e9f918

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

This does not require any changes in the individual pages and corrects the common logic.

[tsa321]

I’m not sure; changing the logic would require extensive testing on other pages as well.
I believe my proposal is simpler and safer, as it doesn’t alter the logic and only addresses(catch/store) the discarded reports.

[eh2077]

@tsa321 I just tested using a high traffic account and it works well, see

Are you still able to reproduce it?

[tsa321]

@eh2077 I am still able to reproduce the issue. The root cause is that some reports are missing from the list.

Test Steps:

1. Create a new test account.
2. Send a dm message to 8 people (using start new chat), make sure to send a message to each one of them.
3. Begin the submit expense step.
4. Notice that without entering a search term, only 5 people/items appear on the submit expense list. The remaining 3 are missing, and there is no contact section.

bug-d.mp4

Another Way to Test:

1. Ensure you have more than 5 expense reports.
2. Begin the submit expense step.
3. Notice that without entering a search term, some expense reports are missing.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[eh2077]

@tsa321 Thanks for pointing out that!

That's interesting, I think it must be a recent PR broke this - Are you able to figure out the PR? So we can understood the root cause better.

[eh2077]

@c3024 Thanks for your comment. Let me take sometime to look into it and will update soon.

cc @MonilBhavsar

[eh2077]

@c3024 I think your RCA is right but I have concerns about your solution here #48114 (comment) - Will it break the logic to calculate userToInvite.

4. I do not think it makes much sense to pass this debouncedSearchTem to getFilteredOptions also because we will do the same filtering in both getFilteredOptions and filterOptions.

I agreed with you it's not ideal to do the filtering in both methods. But passing debouncedSearchTem to getFilteredOptions does fix the effect right?

[eh2077]

@tsa321 I doubted your RCA according to the subsequent discussions above. And your solution only covers the MoneyRequestParticipantsSelector.tsx page while the issue occurs in other pages, like new chat search page.

[c3024]

But passing debouncedSearchTem to getFilteredOptions does fix the effect right?

Yes. I tried some searching on a high traffic account. I think it is fine to pass the debouncedSearchTerm to getFilteredOptions since the search for matching reports happen on backend and only the ordering is done on the front-end which does not appear to be intensive. We can make this change as this is simpler and less likely to lead to any regressions.

[c3024]

Will it break the logic to calculate userToInvite.

I tried again with the changes here also.

It works fine for me for inviting a user.

submitExpenseUserToInvite.mp4

What am I missing?

[eh2077]

@c3024 I'm not sure what changes exactly you made on your client but according the change you suggested to remove

App/src/libs/OptionsListUtils.ts

Lines 1981 to 1984 in 0e9f918

	// 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});
	}

It's possibly to break the logic to calculate userToInvite, like including already invited users etc

Would you like to update your proposal? So that it'll be easier to follow both the RCA and solution, Thanks

[c3024]

@eh2077

Updated the proposal.

This

App/src/libs/OptionsListUtils.ts

Lines 1981 to 1984 in 0e9f918

	// 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});
	}

should not be a problem because when we concat all recent reports and personal details

App/src/libs/OptionsListUtils.ts

Lines 1619 to 1621 in 37bf6b2

	const noOptions = recentReportOptions.length + personalDetailsOptions.length === 0 && !currentUserOption;
	const noOptionsMatchExactly = !personalDetailsOptions
	.concat(recentReportOptions)

before creating a user to invite.

App/src/libs/OptionsListUtils.ts

Lines 2002 to 2004 in 37bf6b2

	let userToInvite: ReportUtils.OptionData | null = null;
	if (
	canCreateOptimisticPersonalDetailOption({

Say there are 10 DMs with 10 users in an account. Then in the submit expense flow, the following are the recent reports and personal details passed to canCreateOptimisticPersonalDetailOption

	Max recent reports = 0	Max recent reports = 5
Without removing the block from getOptions	10 recent reports + 0 personal details	5 recent reports + 5 personal details
After removing the block from getOptions	10 recent reports + 10 personal details	5 recent reports + 10 personal details

So, when we remove that code block duplicates are passed to this canCreateOptimisticPersonalDetailOption function and this will not affect the userToInvite.

Please check the branch specified in the proposal and let me know if something appears to be broken.

[eh2077]

@c3024 Thanks for the update!

So, when we remove that code block duplicates are passed to this canCreateOptimisticPersonalDetailOption function and this will not affect the userToInvite.

But we do pass optionsToExclude to method getUserToInviteOption right? So, I think removing

App/src/libs/OptionsListUtils.ts

Lines 1981 to 1984 in 0e9f918

	// 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});
	}

will impact the result returned by the method, see also

App/src/libs/OptionsListUtils.ts

Lines 2002 to 2019 in 0e9f918

	if (
	canCreateOptimisticPersonalDetailOption({
	searchValue,
	recentReportOptions,
	personalDetailsOptions,
	currentUserOption,
	excludeUnknownUsers,
	})
	) {
	userToInvite = getUserToInviteOption({
	searchValue,
	excludeUnknownUsers,
	optionsToExclude,
	selectedOptions,
	reportActions,
	showChatPreviewLine,
	});
	}

[c3024]

@eh2077

Yes, getUserToInviteOption might return a different result with the code removed. But I don't think that getUserToInviteOption body is what is preventing from showing an optimistic user invite option for an existing user. The canCreateOptimisticPersonalDetailOption is what is blocking from getting to this function altogether.

Even now without making any changes, the maxRecentReportsToShow is what influences what needs to be included in the optionsToExclude part of the code block. Say if the maxRecentReportsToShow is 5 then only five personal details are included in the optionsToExclude. The other personal details are not included in this optionsToExclude. Then this function getUserToInviteOption can return an optimistic user to invite for a personal detail not included in the optionsToExclude. But the if block of canCreateOptimisticPersonalDetailOption is what is preventing it from getting to this getUserToInviteOption altogether.

The existing implementation itself can pass different optionsToExclude for different maxRecentReportsToShow values. Still, the optimistic user to invite option is shown correctly. So, even after the removal the user to invite will be shown correctly even though a different optionsToExclude might be passed thanks to the stopping by the canCreateOptimisticPersonalDetailOption if check. This is evident from testing the code changes also.

[c3024]

We can pass the debouncedSearchTerm to the getFilteredOptions if we want a simpler solution instead of moving the code across functions.

[eh2077]

@c3024

We can pass the debouncedSearchTerm to the getFilteredOptions if we want a simpler solution instead of moving the code across functions.

I did a bit investigating on the change histories. I found that debouncedSearchTerm was deliberately replaced with '' in the series of PRs to improve search performance, see #37619

• perf: Implement filtering in New Chat page #40350
• perf: Filter options in Request Money and Send Money #40235

I think we have to understand the rationale before moving forward with the solution.

[c3024]

Thanks for digging that out.

Then I think moving the personal details exclusion logic to filteredOptions is the way to go. Any comment on this explanation?

[eh2077]

@c3024 Thanks for your patience!

I think we can go with @c3024 's proposal. Their comment #48114 (comment) addresses my concerns about breaking how we calculate userToInvite. The change will be made in key search functions, so we'll need to do extensive tests in PR to avoid breaking things.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @MonilBhavsar is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@MonilBhavsar, @kadiealexander, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[eh2077]

We're waiting for @MonilBhavsar to review #48114 (comment)

[melvin-bot]

@MonilBhavsar @kadiealexander @eh2077 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[eh2077]

We're waiting for @MonilBhavsar to review #48114 (comment)

[tsa321]

@eh2077 Does it also address this issue?

1. Ensure you have more than 5 expense reports.
2. Begin the submit expense step.
3. Notice that without entering a search term, some expense reports are missing.

[c3024]

I think, as per the existing design, only the Recents section includes reports, which in this case are limited to 5. The other section is Contacts, which contains only contacts and no reports. Therefore, if there are more than 5 non-DM reports, some will not appear when there is no search term, according to the current sections' design. (Even fewer non-DM reports are enough to see this if some of the recent reports slots are taken by DM reports.)

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[MonilBhavsar]

Reviewing now... Apologies for delay

[MonilBhavsar]

Okay the rca and proposal makes sense. Thanks all for digging into it. Great team work 🙌
This issue seems to be reported from a regression test stating that the functionality already existed. I am bit skeptical about the major refactor, and if we did check any recent PR's that broke this

[c3024]

1. perf: Filter options in Request Money and Send Money #40235 caused this regression. In this PR, we changed the maxRecentReportsToShow passed to getFilteredOptions to 0 in MoneyRequestParticipantsSelector. As a result, all recent reports are returned from this function, and all personal details that have reports are excluded. Previously, we passed undefined, so the default value of 5 was used for maxRecentReportsToShow. This meant only 5 personal details were excluded, and the rest were returned.

I think this issue was not identified earlier by the QA team, even though it was deployed to production on June 26. This is because, for accounts with contacts that do not have DMs, not all personal details are excluded in getFilteredOptions, as these contacts have no reports. Therefore, if QA tested with an account containing such contacts, like a High Traffic Account, the test would not fail with the existing code because the contacts without reports are not excluded and still appear in the Contacts section, satisfying the TestRail expected result.

2. The potential regression from this change is that getOptions might return both a report and a contact for the same user. Since we are moving this exclusion code to filterOptions, this will not be an issue if we use filterOptions consistently wherever getOptions is used with reports. getOptions with reports is used in getFilteredOptions, getShareDestinationOptions, getMemberInviteOptions, getSearchOptions, and getShareLogOptions. In all instances where these functions (with reports passed to them) are used in the repo, they are followed by the filterOptions function to obtain the final options. Therefore, I have not found anything that could break due to this refactor.

[melvin-bot]

@MonilBhavsar, @kadiealexander, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!