Conversation
| | ValueOf<typeof CONST.SEARCH.SYNTAX_FILTER_KEYS> | ||
| | typeof CONST.SEARCH.SYNTAX_ROOT_KEYS.TYPE | ||
| | typeof CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS | ||
| | typeof CONST.SEARCH.SYNTAX_FILTER_KEYS.POLICY_ID |
There was a problem hiding this comment.
This is already included in ValueOf<typeof CONST.SEARCH.SYNTAX_FILTER_KEYS>
| // We don't want to show the "Expensify Card" feed in the autocomplete suggestion list | ||
| // Thus passing an empty object to the `allCards` parameter. | ||
| return Object.values(getCardFeedsForDisplay(allFeeds, {})); |
There was a problem hiding this comment.
We can also just filter the results but passing an empty object creates less work
| // s77rt check if the value that we should send to the backend is "Expensify Card" (same as displayed text) | ||
| // And if so update buildSubstitutionsMap to handle highlighting |
There was a problem hiding this comment.
Do we need to address this comment? Seems like something we wanna fix before merging this PR
There was a problem hiding this comment.
This depends on the BE and I wanted to avoid raising concerns that are not worked on yet. I will circle back on each of the "s77rt" when removing the dev lock.
Currently we only add the highlights if the value is different than the displayed text. For Expensify Card I don't know what value we will be sending so the comment.
There was a problem hiding this comment.
@s77rt Here's a scenario where I have 3 virtual Expensify Cards added (2 on workspace owner and 1 on a member) and 1 Direct feed Company Card (AMEX) and here's how Feed dropdown looks:
I'd expect to see 3 Expensify Card entries correct ? If so, is this related to the same issue you mentioned above ?
Currently we only add the highlights if the value is different than the displayed text. For Expensify Card I don't know what value we will be sending so the comment.
| } | ||
|
|
||
| function getFeedOptions(allCardFeeds: OnyxCollection<OnyxTypes.CardFeeds>, allCards: OnyxTypes.CardList) { | ||
| // s77rt confirm if the feed value is just the feed key or if it should be prefixed with [fundID]_ |
There was a problem hiding this comment.
Same as the highlights concern. This is BE related. Currently for Visa feed I'm sending value vcf but I see some logic (in Advanced Filters -> Card -> All Visa) the value is [fundID]_vcf
| // s77rt remove DEV lock | ||
| const shouldDisplayGroupByFilter = isDevelopment; | ||
| // s77rt remove DEV lock | ||
| const shouldDisplayFeedFilter = isDevelopment && feedOptions.length > 1; |
There was a problem hiding this comment.
feedOptions.length > 1 here is preventing the display of the Feed filter dropdown even though I do have 1 valid feed in the feedOptions array (AMEX).
There was a problem hiding this comment.
If you have only one feed, the filter would be pointless as you will already get results from that feed
There was a problem hiding this comment.
But you also get all other expenses which don't have anything to do with that specific feed (since we're not force-filtering by that 1 Feed by default), which if you have a lot of them, might pose an issue if the user only wants to see expenses related to that specific feed (card), right ?
Edit: I mean if that's what the design doc says, all good - I was just wondering since to me it looks like an issue.
There was a problem hiding this comment.
Good point. I'm not really sure. Let's follow this on https://expensify.slack.com/archives/C0920G9GJ0N/p1751495500557969
There was a problem hiding this comment.
But let's not block on that. Changing this condition can be done in a follow (if a change is required)
There was a problem hiding this comment.
Cool, dropped a comment in that thread - starting work on the checklist.
There was a problem hiding this comment.
Another thing related to the same topic here, while doing manual testing on the PR I noticed that if I select American Express feed and filter by it, currently the exact same expenses are showing (like the filter doesn't do anything), same if I try with a 2nd feed: Expensify Card.
I'm assuming this is due to BE missing the feed filtering logic, therefore not applying any filter and simply returning the exact same expenses for the type:expense status:all feed:"Expensify Card" filter, omitting the feed part.
There was a problem hiding this comment.
I guess it's BE related. This will be tested again after removing dev lock
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
We change some logic with buildQueryString, and some other core logic, can we add some more generic QA steps too? I think regression testing will catch most issues, but maybe some key areas that were changed? |
|
Done! I have added some more tests |
|
Having some issues with running the HybridApps on both platforms, trying rebuild so I can finish-up testing on all devices. |
|
✋ 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.1.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.78-4 🚀
|
| FILTER_KEYS.EXPORTED_AFTER, | ||
| FILTER_KEYS.EXPORTED_BEFORE, | ||
| FILTER_KEYS.EXPORTED_ON, | ||
| FILTER_KEYS.GROUP_BY, |
Explanation of Change
Fixed Issues
$ #64487
PROPOSAL:
Tests
Preconditions:
Cardthen click ApplyFeedfilterFeedfilterExpensify Cardis listed (if you have any Expensify cards)Feed:Expensify CardOffline tests
Same as Tests (NOT for QA)
QA Steps
Test 1:
Typefilter and verify you can change the type.Filtersbutton to open advanced filters RHPTypeand verify you can change the typeTest 2:
type:type:chat,type:expense, etc)Test 3:
Filtersbutton to open advanced filters RHPExpenseDraftsYesSave searchbutton is displayedSavedsectionTest 4:
group-by:reportsTypefilter and change the type toChatgroup-by:reportsPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Screen.Recording.2025-07-02.at.11.27.44.PM.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-07-02.at.10.34.38.PM.mov
MacOS: Desktop