feat: [Insights] [Release 2] Top Merchants - Add group-by:merchant and suggested search #80672
Conversation
…d suggested search
|
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 |
|
@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] |
TaduJR
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/libs/SearchUIUtils.ts
Outdated
| * | ||
| * Do not use directly, use only via `getSections()` facade. | ||
| */ | ||
| function getMerchantSections(data: OnyxTypes.SearchResults['data'], queryJSON: SearchQueryJSON | undefined): [TransactionMerchantGroupListItemType[], number] { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The getMerchantSections function is almost identical to getCategorySections function. The only differences are the entity type (merchant vs category) and the field names. This duplicated logic should be consolidated into a generic reusable function.
Suggested fix:
Create a generic helper function that can handle both merchants and categories:
function createGroupSections<T extends TransactionGroupListItemType>(
data: OnyxTypes.SearchResults'['data'],
queryJSON: SearchQueryJSON | undefined,
groupType: ValueOf<typeof CONST.SEARCH.GROUP_BY>,
filterKey: ValueOf<typeof CONST.SEARCH.SYNTAX_FILTER_KEYS>,
getGroupValue: (group: any) => string,
formatValue: (value: string) => string,
): [T[], number] {
const sections: Record<string, T> = {};
for (const key in data) {
if (isGroupEntry(key)) {
const group = data[key];
const groupValue = getGroupValue(group);
let transactionsQueryJSON: SearchQueryJSON | undefined;
if (queryJSON && groupValue \!== undefined) {
const newFlatFilters = queryJSON.flatFilters.filter((filter) => filter.key \!== filterKey);
newFlatFilters.push({
key: filterKey,
filters: [{operator: CONST.SEARCH.SYNTAX_OPERATORS.EQUAL_TO, value: groupValue}]
});
const newQueryJSON: SearchQueryJSON = {...queryJSON, groupBy: undefined, flatFilters: newFlatFilters};
const newQuery = buildSearchQueryString(newQueryJSON);
transactionsQueryJSON = buildSearchQueryJSON(newQuery);
}
sections[key] = {
groupedBy: groupType,
transactions: [],
transactionsQueryJSON,
...group,
formattedValue: formatValue(groupValue),
} as T;
}
}
const sectionsValues = Object.values(sections);
return [sectionsValues, sectionsValues.length];
}Then use it for both merchants and categories.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![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: 430a63ed06
ℹ️ 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".
neil-marcellini
requested review from
ShridharGoel
and removed request for
shubham1206agra
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari
|
|
Merchant shows as 
|
|
Bug: When sorting by merchant name, sorting uses 
|
Can you write this last step in a clearer way? It would just expand the group and not navigate, right? |
…ts-Release-2-Top-Merchants-Add-group-by-merchant-and-suggested-search
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Core PR for a WN project. 👍
- Display "No merchant" instead of "(none)" for empty merchants - Fix sorting to put empty merchants at bottom instead of top - Add noMerchant translations to all language files - Update comment to include TOP_MERCHANTS - Add test for empty merchant query normalization
Updated @ShridharGoel. Thanks! |
@neil-marcellini, I believe we should omit expenses with no merchant from BE, as it does not make sense in the Top Merchants tab. |
|
We are showing "Uncategorized", "No tag" etc., so I think it would make sense to have it here as well. |
|
@TaduJR Might not be the best idea to change "Expense" and "Unknown merchant" to "No merchant", but let's wait for @neil-marcellini |
Sure, but "Expense" is SelDM with No Merchant name which could make sense not certain about how "Unknown merchant" achieved. |
|
It feels like the comments put there can be handled as polish/follow-up items. I don't feel like this:
Should block us from merging the PR. If there aren't any bugs, I think we should proceed with the approval/merge. |
This can be handled as a follow-up too I'd say. I feel like we should update the list to filter out the original expense after splitting, as it will have the report ID -2 (soft-deleted). |
@lakchote Sure, but the fix has already been pushed but that will also change Expense and Unknown merchant. So, how do you suggest to proceed? I think it should be fine to merge it and then handle this in a follow-up. |
I'm in favour of handling this in a follow-up, yes cc @trjExpensify for visibility |
Screen.Recording.2026-01-28.at.4.51.58.PM.mov |
|
Summary of the follow-ups to be done
|
lakchote
dismissed
neil-marcellini’s stale review
Context explained here: https://expensify.slack.com/archives/C01GTK53T8Q/p1769602422027409?thread_ts=1769580954.489439&cid=C01GTK53T8Q
PR is now mergeable.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
10 participants
Explanation of Change
This PR adds the "Top Merchants" feature following the same pattern as "Top Spenders" and "Top Categories"
Fixed Issues
$ #80393
PROPOSAL:
Tests
Prerequisite: Log in with an account that has at least one paid workspace/policy and some expenses with merchant names.
Group By Merchant
Empty Merchant Handling
group-by:merchantTop Merchants Suggested Search
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR 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))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
Macbook-Chrome.mp4