-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[$250] Submit Expense - Search list when submitting expense, doesn´t display the contacts section #48114
Comments
Triggered auto assignment to @kadiealexander ( |
@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 |
Edited by proposal-police: This proposal was edited at 2024-08-28 06:51:06 UTC. ProposalPlease 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 App/src/libs/OptionsListUtils.ts Lines 2391 to 2393 in 071f11c
and here: App/src/libs/OptionsListUtils.ts Lines 2465 to 2467 in 071f11c
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 When generating section data: App/src/pages/iou/request/MoneyRequestParticipantsSelector.tsx Lines 192 to 204 in 071f11c
include the olderReports data. The sketch code changes could be something like this. We must also check other usage of 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. |
Job added to Upwork: https://www.upwork.com/jobs/~016d2f5d8be09bffb6 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 ( |
Edited by proposal-police: This proposal was edited at 2024-09-19 10:01:46 UTC. ProposalPlease 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
with
and we pass maxRecentReportsToShow as 0 herewhich means we do not limit the max recent reports for the result from this function.
App/src/libs/OptionsListUtils.ts Line 2138 in fc849dd
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
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
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
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?
That is to remove this logic App/src/libs/OptionsListUtils.ts Lines 1981 to 1984 in 0e9f918
of excluding personal details for included reports from 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
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
This movement does not break anything because wherever we use Here is the branch with these changes. What alternative solutions did you explore? (Optional)
|
@kadiealexander, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Reviewing proposals |
Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Do we know which PR broke this? |
@eh2077, after testing @c3024's proposal by changing: to With @c3024's proposal, some reports are missing (notice the missing workspace reports): some.reports.missing.mp4With my proposal, all reports are shown (including workspace reports): my-proposal.mp4@eh2077 could you re-review my proposal again? |
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
of excluding personal details for included reports from 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
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
This does not require any changes in the individual pages and corrects the common logic. |
I’m not sure; changing the logic would require extensive testing on other pages as well. |
@tsa321 I just tested using a high traffic account and it works well, see Are you still able to reproduce it? |
@eh2077 I am still able to reproduce the issue. The root cause is that some reports are missing from the list. Test Steps:
bug-d.mp4Another Way to Test:
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@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. |
Updated the proposal. This App/src/libs/OptionsListUtils.ts Lines 1981 to 1984 in 0e9f918
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
before creating a user to invite. App/src/libs/OptionsListUtils.ts Lines 2002 to 2004 in 37bf6b2
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
So, when we remove that code block duplicates are passed to this Please check the branch specified in the proposal and let me know if something appears to be broken. |
@c3024 Thanks for the update!
But we do pass App/src/libs/OptionsListUtils.ts Lines 1981 to 1984 in 0e9f918
will impact the result returned by the method, see also App/src/libs/OptionsListUtils.ts Lines 2002 to 2019 in 0e9f918
|
Yes, Even now without making any changes, the The existing implementation itself can pass different |
We can pass the |
I did a bit investigating on the change histories. I found that
I think we have to understand the rationale before moving forward with the solution. |
Thanks for digging that out. Then I think moving the personal details exclusion logic to |
@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 🎀👀🎀 C+ reviewed |
Current assignee @MonilBhavsar is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@MonilBhavsar, @kadiealexander, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
We're waiting for @MonilBhavsar to review #48114 (comment) |
@MonilBhavsar @kadiealexander @eh2077 this issue is now 4 weeks old, please consider:
Thanks! |
We're waiting for @MonilBhavsar to review #48114 (comment) |
@eh2077 Does it also address this issue?
|
I think, as per the existing design, only the |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Reviewing now... Apologies for delay |
Okay the rca and proposal makes sense. Thanks all for digging into it. Great team work 🙌 |
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
|
@MonilBhavsar, @kadiealexander, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Makes sense! Thanks for clarifying! 🙌
So, this was not functioning correctly before too?
Do we plans to ensure it get's used correctly? Or we'll create a similar issue if we use getOptions on a new page, right? |
No, it was functioning correctly before.
We can add a lint rule for |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Not overdue, the discussion is ongoing |
@MonilBhavsar, @kadiealexander, @eh2077 Eep! 4 days overdue now. Issues have feelings too... |
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:
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?
Screenshots/Videos
Add any screenshot/video evidence
Bug6584155_1724766273965.Contacts-Recents.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @MonilBhavsarThe text was updated successfully, but these errors were encountered: