-
Notifications
You must be signed in to change notification settings - Fork 204
fix: Initialize property filter string value to empty string #4039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
770a021 to
e0e818b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4039 +/- ##
=======================================
Coverage 96.99% 96.99%
=======================================
Files 861 861
Lines 25207 25209 +2
Branches 9098 9099 +1
=======================================
+ Hits 24450 24452 +2
- Misses 710 751 +41
+ Partials 47 6 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| detail: { | ||
| operation: 'and', | ||
| tokenGroups: [{ propertyKey: 'other-string', operator: '=', value: null }], | ||
| tokenGroups: [{ propertyKey: 'other-string', operator: '=', value: '' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue for string properties, but breaks enum and custom properties, see: #4041
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Changed the logic based on the expected behavior of those tests (empty array for enum and null for custom properties). Also refactored into one test for each.
I rely on the presence of a form property in order to infer a custom property (see token-editor.tsx below? Is there a better way? Nor the internal operator, which is a string, nor the matchedProperty, which is of type InternalFilteringProperty, have enough information about this, and the PropertyFilterTokenType type returned by property.getTokenType only knows about value or enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, relying on the form sounds good 👍
a9aa911 to
86be646
Compare
src/property-filter/token-editor.tsx
Outdated
| : allowedOperators[0]; | ||
| const matchedProperty = filteringProperties.find(property => property.propertyKey === newPropertyKey) ?? null; | ||
| setTemporaryToken({ ...temporaryToken, property: matchedProperty, operator, value: null }); | ||
| const isCustomType = !!matchedProperty?.externalProperty.operators?.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matchedProperty is of internal property type. It supports derived properties for better ergonomics, so there is no need to find the form like this. Instead, you can either check for getValueFormRenderer(operator) prop, or even extend the getTokenType() to return "value" | "enum" | "custom".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks. Updated to use getValueFormRenderer.
Description
Ticket:
AWSUI-61464Dry run:
7600116220How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.