Add filter negation feature#93909
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 |
|
@DylanDylann 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] |
|
Can I run the translation script myself? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5746a1607
ℹ️ 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".
|
Btw, when we apply a negated HAS filter with a Tag value, it will be applied as "Tag: No tag". w.mp4It's from this code. It causes weird buggy behavior with the Has filter component, as shown in the video above. App/src/libs/SearchQueryUtils.ts Lines 1110 to 1112 in e5928ed Is that expected? |
|
🚧 @dubielzyk-expensify 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 is looking great already @bernhardoj. Let's make this text
|
|
I'm not convinced we should use the is/is not in the chip itself:
I have to go and will have a noodle, but I think we might want an icon or do cc @Expensify/design @trjExpensify @JmillsExpensify |
Done |
I'm fine with what I'm seeing in the adhoc. |
|
@bernhardoj Looks like we're still missing support for
|
|
Status and workspace are still having their BE PRs worked on: https://github.com/Expensify/Auth/pull/22241 |
|
Opened a seperate PR (because it's quite big) to allow workspace and status to be negatable. cc: @aimane-chnaif |
|
Works for me. |
|
Is this ready for review? Lots of tests failing |
|
Not yet. I'll let you know when it's ready |
|
@aimane-chnaif ready now |
|
lint is failing @bernhardoj |
| if (isFilterNegatable(key)) { | ||
| const prefix = isFilterNegated(key) ? '-' : ''; | ||
| return `${prefix}${translate(labelKey)}`; | ||
| } |
There was a problem hiding this comment.
chip label is now a raw - prefix, contradicting the PR's own QA steps
So an applied negated filter renders as -Merchant: Uber / -Has: Receipt.
This conflicts with three things:
- The PR's own QA steps (steps 8–9) still specify
Merchant is not: testandDoesn't have: Receipt. - The picker UI itself —
NegatableFilterstill labels the toggle buttons "is / is not" and "has / doesn't have", and those strings are still translated across all 9 languages. A user picks "is not" and gets back a chip reading "-Merchant", which doesn't echo the wording they chose. - The
-is hardcoded and unlocalized — it bypassestranslateentirely, so thesearch.filters.filterType.*.negativestrings are now only used by the toggle, never by the chip.
There was a problem hiding this comment.
chip label is now a raw - prefix, contradicting the PR's own QA steps
Looks like the AI still sees the old steps 😄
There was a problem hiding this comment.
yup.
Btw non-blocking nit: the - is hardcoded in getLabelValue rather than coming from a translation, so the chip prefix isn't localized the way the picker toggle ("is not" / "doesn't have") is.
There was a problem hiding this comment.
Can - syntax be localized? 🤔
There was a problem hiding this comment.
No need localization but define in CONST?
There was a problem hiding this comment.
Ah, that makes more sense. Updated.
| default: | ||
| return null; | ||
| break; |
There was a problem hiding this comment.
default: case can return undefined.
The switch ends with default: break;, then unconditionally returns <NegatableFilter>{content}</NegatableFilter>.
For an unhandled key content is undefined, and NegatableFilter early-returns children for non-negatable keys — so the component returns undefined.
The switch is currently exhaustive so it's latent, but the old code returned null here.
Suggestion: have NegatableFilter return children ?? null.
There was a problem hiding this comment.
Undefined also renders nothing, same as null
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e483905be6
ℹ️ 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".
| policyID={getFilterNegatableValue(CONST.SEARCH.SYNTAX_FILTER_KEYS.POLICY_ID, values)} | ||
| ready={ready} | ||
| isNegated={isNegated} | ||
| onChange={(newValue, negated) => onChange(getFilterFormValues(baseFilterKey, newValue, negated))} |
There was a problem hiding this comment.
Preserve negated Has Tag selections
Negating the Has filter with the Tag option does not round-trip through the existing query normalization. This path writes hasNot: ['tag'], which serializes to -has:tag, but buildFilterFormValuesFromQuery translates -has:tag into tag: [No tag] instead of hasNot; after applying/reopening, the chip becomes Tag: No tag and the Has filter no longer shows doesn't have Tag selected. Special-case Tag here or prevent it from being written as a negated Has value so the new negation UI preserves its own state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@JmillsExpensify @JS00001 currently, when we apply a negated Has with Tag value (-has:tag), it translates to {tag: ['none']}. Do we want to keep that behavior? (this is an existing behavior on prod)
no.tag.mp4
Or should we change it so it translates to {hasNot: tag}?
|
🤖 Code review Overall this is a clean, well-structured refactor — renaming 🔴 Blocker — ESLint is failingThe ESLint check is red. Two categories, both from this PR: 1. Parent-relative imports that must use the path alias (
2. New unsafe type assertions exceeding the 🟡 Minor —
|
|
All done, except #93909 (comment), waiting for confirmation. |




Explanation of Change
Fixed Issues
$ #93208
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
-For steps 3-7, if you want more test steps, follow this:
No filter is applied yet
For filter chip/bar:
PR 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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4