Repository navigation
[Payment due @truph01] fix: Add Contains / Equal to match type for the Merchant filter [80823] - #83733
Conversation
Switch merchant filter from : (eq) to *: (contains) operator to preserve partial-matching UX when backend starts respecting eq vs contains distinction. Changes: - Add *: operator support to parser grammar (baseRules.peggy) - Regenerate searchParser.js and autocompleteParser.js - Update buildQueryStringFromFilterFormValues to use *: for merchant - Update unit test expectations Fixes Expensify#80823
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e98c1cb565
ℹ️ 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".
|
I have read the CLA Document and I hereby sign the CLA |
|
@HiepPP Please address all AI's suggestions above. Also, your PR author checklist doesn't include videos. Please update it. |
|
@HiepPP Since it is your first PR, please follow this rule:
|
Thank you for mentioning the rule. I will also update PR tonight. |
|
@truph01 I've addressed your feedback:
Please re-review when you have a chance. Thanks!! |
|
Hi @heyjennahay and @truph01, I wanted to follow up on PR #83733 which has been open for about a week. This PR fixes issue #80823 where expenses with different merchants (e.g., "I" and "Ig") were incorrectly grouped together in the "Top Merchants" view. Summary of changes:
CI status: All checks passing (CLA, license, security) The PR is ready for review when you have a moment. Let me know if you need any clarification on the implementation. Thanks! |
|
@HiepPP Please read this part https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request, it looks like your commits aren't signed. |
Switch merchant filter from : (eq) to *: (contains) operator to preserve partial-matching UX when backend starts respecting eq vs contains distinction. - Add *: operator support to parser grammar - Update buildQueryStringFromFilterFormValues to use *: for merchant - Update unit test expectations Fixes Expensify#80823
48d7f11 to
167132a
Compare
|
thank @truph01 I've signed the commit with GPG |
|
@HiepPP Could you help to merge main? |
|
@MelvinBot This PR focuses on updating the merchant filter to take two operators: contains and eq. For the normal merchant filter to pass, it must contain. However, when searching for individual groups, we would pass the merchant eq. Please help review it. |
Code ReviewI reviewed the hand-written changes in this PR (the grammar, query utils, and tests). Here are my findings: Core Logic (SearchQueryUtils.ts) - Looks correctThe change at SearchQueryUtils.ts:691-698 correctly branches on the merchant filter:
This handles the round-trip serialization properly - when parsed back, the The "Top Merchants" group drill-down at Grammar (baseRules.peggy) - Looks correctThe Concerns
SummaryThe hand-written logic is sound and correctly addresses the issue. The main concern is the unintentional Peggy version upgrade inflating the diff. I'd recommend regenerating with the pinned Peggy 4.0.3 and adding a negated-merchant test case. |
|
hi @neil-marcellini @luacmartins I merged the latest main and fixed the conflicts. Could you please review again? Thank you |
|
Shoot @HiepPP this is getting a lot of conflicts. I think we should coordinate on timing so we can get this merged without more back and forth. I'm usually working 6:30 am to about 4:00 pm PDT. How about you and when would be a good time for you to update and for me to approve? |
…ntains-operator # Conflicts: # src/hooks/useSearchBulkActions.ts
|
hi @neil-marcellini I just merged the latest main and fixed the conflicts in 37fd19d, there are no conflicts right now |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 neil-marcellini 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.5.10-0 🚀
|
|
A help site update is needed. I opened a draft PR: Docs updates for E/A#83733 Document merchant= operator for exact merchant matches. This PR adds a new feature with no beta gate: exact merchant matching with The docs PR has the @HiepPP, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #104133 was identified to be related to this PR. |
|
Deploy Blocker #104147 was identified to be related to this PR. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.5.12-0 🚀
|
|
A help site update is needed, and the draft docs PR already exists: [No QA] Docs updates for E/A#83733 Document merchant= operator for exact merchant matches. I didn't open a second one. It edits one row in the expense operators table of "Use Search Operators to Filter and Analyze". The row now covers The two deploy blockers filed against this PR are bugs: [$175] Android - Spend - Merchant filter modal only shows Merchant title and Apply button and [$175] Spend-Expense appears in Spend results despite not matching the applied merchant filter. Neither changes the documented behavior, and the feature is still on GitHub still won't let me assign the docs PR to @HiepPP, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.12-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #104212 |
Fixed Issues
$ #80823
PROPOSAL: #80823 (comment)
Explanation of Change
Expenses with different merchants (e.g. "I" and "Ig") were grouped together in the Top Merchants view, because the merchant filter only did partial matching. This PR adds a match type to the Merchant filter so users can choose between a partial and an exact match.
Changes:
merchant=→ exact match (Equal to)merchant*:→ partial match (Contains)merchant:→ still a partial match, so existing saved searches and typed queries keep their current behavior-merchant:→ unchanged (exclusion)*:(contains) operator to the search parser grammar (baseRules.peggy,searchParser.peggy,autocompleteParser.peggy) and regenerated the parsers.*:is only accepted formerchant; on other keys it falls back to a keyword search.merchant=, so a group only shows its own merchant.Contains "Ig"orEqual to "Ig".Tests
Match type on the advanced filters page
Ig, keep Contains, and save. Then click View results.merchant*:Ig(ormerchant:Ig), and "Ig" and "Igloo" are shown.merchant=Igand only "Ig" is shown.Match type in the filter chip
Ig, select Equal to, and apply.Equal to "Ig"and only "Ig" is shown.Contains "Ig", and "Ig" and "Igloo" are shown.Negated filter
Igand apply. Verify the query contains-merchant:Igand "Ig" is excluded.Merchant group drill-down
merchant="I".Small screen with the keyboard open
Offline tests
QA Steps
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-native.mp4
Android: mWeb Chrome
android-mweb-chrome-compressed.mp4
iOS: Native
ios-native-compressed.mp4
iOS: mWeb Safari
ios-mweb-safari-compressed.mp4
MacOS: Chrome / Safari
macos-chrome-compressed.mp4