[Payment due @daledah] Add receipt-type filter on the Search (Spend) page#94129
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
melvin-bot
Bot
requested review from
a team,
abdulrahuman5196 and
flaviadefaria
and removed request for
a team
|
@abdulrahuman5196 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] |
melvin-bot
Bot
requested review from
joekaufmanexpensify
and removed request for
a team
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.
|
allgandalf
removed request for
abdulrahuman5196,
flaviadefaria and
joekaufmanexpensify
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }); | ||
| }, [allReceiptTypes, translate]); | ||
|
|
||
| const updateReceiptTypeFilter = useCallback((values: ReceiptTypeValues) => updateAdvancedFilters({receiptType: values}), []); |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and automatically memoizes closures based on their captured variables, so wrapping updateReceiptTypeFilter in useCallback is redundant.
Replace the useCallback with a plain function:
const updateReceiptTypeFilter = (values: ReceiptTypeValues) => updateAdvancedFilters({receiptType: values});Reviewed at: 46f3fb7 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 8dbce6a3d4
ℹ️ 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".
| if (isNegated) { | ||
| filtersForm[FILTER_KEYS.RECEIPT_TYPE_NOT] = receiptTypeValues; | ||
| } else { | ||
| filtersForm[FILTER_KEYS.RECEIPT_TYPE] = receiptTypeValues; |
There was a problem hiding this comment.
When the URL contains a negated receipt filter such as -receipt-type:hotel, this stores receiptTypeNot in the advanced-filter form, but the new Receipt type picker only reads/writes receiptType. If the user then opens Filters, selects Hotel, and applies, the stale receiptTypeNot is preserved and buildQueryStringFromFilterFormValues emits both -receiptType:hotel and receiptType:hotel, making the search unsatisfiable. Clear the opposite receipt-type field when converting or applying a positive selection, or surface the negated state in the picker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de91640. A positive receipt-type selection now drops those values from receiptTypeNot, so the same value can't end up in both receiptType and -receiptType.
|
|
||
| const [searchAdvancedFiltersForm, searchAdvancedFiltersFormResult] = useOnyx(ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM); | ||
| const initiallySelectedItems = searchAdvancedFiltersForm?.receiptType | ||
| ?.filter((receiptType) => Object.values(CONST.SEARCH.RECEIPT_TYPE).includes(receiptType as ValueOf<typeof CONST.SEARCH.RECEIPT_TYPE>)) |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Object.values(CONST.SEARCH.RECEIPT_TYPE) is called inside the .filter() callback but does not depend on the receiptType iterator variable, so it rebuilds the same array on every iteration. Iterator-independent calls inside array methods produce redundant O(n) work that should be hoisted out of the loop.
Hoist the lookup once (ideally as a Set for O(1) membership) before iterating:
const validReceiptTypes = new Set<string>(Object.values(CONST.SEARCH.RECEIPT_TYPE));
const initiallySelectedItems = searchAdvancedFiltersForm?.receiptType
?.filter((receiptType) => validReceiptTypes.has(receiptType))
.map((receiptType) => {
const receiptTypeName = translate(getReceiptTypeTranslationKey(receiptType as ValueOf<typeof CONST.SEARCH.RECEIPT_TYPE>));
return {name: receiptTypeName, value: receiptType};
});Reviewed at: 57c0ec6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
done in b29c110, hoisted it to a Set before the filter.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ad7564afd
ℹ️ 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".
| case CONST.SEARCH.SYNTAX_FILTER_KEYS.HAS: | ||
| case CONST.SEARCH.SYNTAX_FILTER_KEYS.IS: | ||
| case CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE: | ||
| case CONST.SEARCH.SYNTAX_FILTER_KEYS.RECEIPT_TYPE: |
There was a problem hiding this comment.
Fresh evidence after the follow-up fix: SearchAdvancedFiltersBase still routes every filter row through ROUTES.SEARCH_ADVANCED_FILTERS_CONTENT, so this generic multiselect is the path used from the Filters list, not the dedicated receipt-type page. When the current query contains -receipt-type:hotel, this path only merges {receiptType: ['hotel']} and leaves the existing receiptTypeNot in the draft filters; applying then serializes both -receiptType:hotel and receiptType:hotel, making the search unsatisfiable for that selected value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch, the dedicated page isn't the path the Filters list uses. fixed in 1c31e9a, the generic content page (SearchAdvancedFiltersContentBase) now drops the conflicting values from receiptTypeNot when a positive receipt-type selection is applied.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-resize.mp4Android: mWeb Chromeandroid-mweb-resize.mp4iOS: HybridAppios-resize.mp4iOS: mWeb Safariios-mweb-resize.mp4MacOS: Chrome / Safariweb-resize.mp4 |
daledah
left a comment
daledah
left a comment
There was a problem hiding this comment.
The failing checks are not related to this PR. The code changes LGTM and are well tested.
melvin-bot
Bot
changed the title
|
🎯 @daledah, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
@allgandalf can you try pulling main please? |
|
@JS00001 done! |
|
🚧 JS00001 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.4.21-2 🚀
|
4 participants
Explanation of Change
Adds a
receipt-typefilter to the Search (Spend) advanced filters, modeled on the existing "Expense type" filter. It lets users filter expenses by the kind of receipt they have, with three values: eReceipt, Itemized, and Hotel.The query sends the
receiptTypekey with valuesereceipt/itemized/hotelto the backend, which does the matching. This PR covers the frontend only: the search grammar (receipt-type:parses to thereceiptTypefilter key), the advanced filter row + dedicated multi-select page, autocomplete, query parse/build (including negation), display labels, and translations in every supported language.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/647827
PROPOSAL:
Tests
receipt-type:with the selected valuesreceipt-type:hoteldirectly in the search bar and verify it is parsed and applied as a filter-receipt-type:hoteland verify it is applied as a negated filter (excludes hotel receipts)Offline tests
QA Steps
receipt-type:itemizedin the search bar filters to itemized receipts-receipt-type:hotelexcludes hotel receiptsPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.