[CP Staging] Restore You/Me search matching for current user in From filter#83291
[CP Staging] Restore You/Me search matching for current user in From filter#83291
Conversation
When PR #82490 migrated UserSelectPopup from OptionsListUtils to PersonalDetailOptionsListUtils, the "You"/"me" keyword matching for the current user was lost. The old getCurrentUserSearchTerms function explicitly added translated "You" and "me" strings as searchable terms for the current user, but the new filterOption/matchesSearchTerms functions only match against display name and email. This adds an optional extraSearchTerms parameter to filterOption and matchesSearchTerms, and passes the translated "You"/"me" strings from UserSelectPopup when filtering the current user option. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot Mark this PR ready for review |
melvin-bot
bot
requested review from
heyjennahay and
shubham1206agra
and removed request for
a team and
heyjennahay
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
PR has been marked as ready for review. ✅ |
|
@hungvu193 Please review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac918a0c84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Reviewing shortly... |
|
The only failing check is All code-related checks (ESLint, typecheck, Prettier, tests, builds, verifySignedCommits) are passing. |
|
The only failing check is All code checks are passing: ESLint, Prettier, typecheck, all test jobs, builds, spellcheck, and signed commits. |
Code Review: Comparison with Previous LogicI compared this PR's approach against the old Old Logic (OptionsListUtils)The old [item.text ?? item.displayName ?? '', item.login ?? '', item.login?.replace(CONST.EMAIL_SEARCH_REGEX, '') ?? '', translateLocal('common.you'), translateLocal('common.me')]This array was used by New Logic (This PR)This PR adds an optional Comparison Verdict: ✅ Correctly restores the old behavior
|
|
The only failing check is PR Reviewer Checklist ( |
Co-authored-by: Hans <hungvu193@users.noreply.github.com>
melvin-bot
bot
requested review from
mountiny
and removed request for
a team
|
Added unit tests for
All 53 tests pass. The test suite reports a failure from a pre-existing |
|
@MelvinBot Fix the eslint and spellcheck. |
- Replace type assertions (as/!) with proper null guards to satisfy both non-nullable-type-assertion-style and no-non-null-assertion rules - Add cspell:disable-next-line for peterparker test data references Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
Merge conflict resolved taking remote approach (eslint-disable with ! assertions), added cspell:disable-next-line for peterparker test data. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
Fixed the failing checks. The issues were all in
|
|
The failing Analysis: The job failed during the "Set up job" phase before any tests ran. The CI runner couldn't download the Evidence: The PR changes 4 files ( Please re-run the failed job to get a clean pass. |
|
@MelvinBot Please re-run the test. |
|
The failing check ( I don't have permission to re-run workflows, so someone with write access will need to re-run the failed job. You can do this by visiting the failed run and clicking "Re-run failed jobs". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb ChromemChrome.moviOS: HybridAppios.hyb.moviOS: mWeb SafariSAfari.movMacOS: Chrome / Safarichrome.mov |
|
@MelvinBot Please merge main. |
…InFromFilter Co-authored-by: Hans <hungvu193@users.noreply.github.com>
|
Merged main into this branch. No conflicts — merge was clean and has been pushed. |
|
@mountiny, it seems @hungvu193 reviewed and tested this PR. Can you assign him to the DB issue so he can get paid? |
mountiny
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 895a4cd350
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function matchesSearchTerms(option: OptionData, searchTerms: string[], extraSearchTerms?: string[]): boolean { | ||
| let searchText = deburr(`${option.text} ${option.login ?? ''}`.toLocaleLowerCase()); | ||
| if (extraSearchTerms?.length) { | ||
| searchText += ` ${extraSearchTerms.join(' ').toLocaleLowerCase()}`; |
There was a problem hiding this comment.
processSearchString() normalizes user input with deburr, but matchesSearchTerms() appends extraSearchTerms using only toLocaleLowerCase(), so accented translations like common.you ("Tú", "Você") are compared as accented text against deburred input ("tu", "voce") and fail to match. This means the restored current-user search still breaks in locales with diacritics when users type unaccented terms, which is a regression from the previous accent-insensitive matching behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — extraSearchTerms was only lowercased but not deburred, so accented translations like "Tú" would not match deburred user input "tu". Fixed in 11ca06e by wrapping extraSearchTerms with deburr() to match the same normalization applied to user input in processSearchString() and to option text on the line above.
…tive matching Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
…ilter [CP Staging] Restore You/Me search matching for current user in From filter (cherry picked from commit d00ded0) (cherry-picked to staging by mountiny)
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.25-9 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.25-10 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.25-13 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.3.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
6 participants
Explanation of Change
PR #82490 migrated
UserSelectPopupfrom the oldOptionsListUtilsapproach to the newPersonalDetailOptionsListUtils. The old code had agetCurrentUserSearchTermsfunction that explicitly added the translated "You" and "me" strings as searchable terms for the current user. The newmatchesSearchTerms/filterOptionfunctions only match against display name and email, so typing "You" or "Me" no longer matched the current user.This PR restores that behavior by:
extraSearchTermsparameter tomatchesSearchTermsandfilterOptioninPersonalDetailOptionsListUtilsUserSelectPopupwhen filtering the current user optionFixed Issues
$ #83252
Tests
Offline tests
Same as Tests — filtering is client-side so offline behavior is identical.
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari