fix: [filter_box] fix 2 issues in single value filter_box #9829
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9829 +/- ##
==========================================
+ Coverage 66.16% 71.01% +4.84%
==========================================
Files 585 583 -2
Lines 30427 30634 +207
Branches 3152 3165 +13
==========================================
+ Hits 20133 21755 +1622
+ Misses 10113 8766 -1347
+ Partials 181 113 -68
Continue to review full report at Codecov.
|
| datasource.columns.filter( | ||
| col => col.column_name === selectedColumnName, | ||
| ) || [] | ||
| ).pop(); |
There was a problem hiding this comment.
selectedColumn = datasource.columns.find(x => x.column_name === selectedColumnName);
| @@ -27,6 +27,9 @@ import SelectControl from './SelectControl'; | |||
| import CheckboxControl from './CheckboxControl'; | |||
| import TextControl from './TextControl'; | |||
|
|
|||
| const INTEGRAL_TYPES = ['TINYINT', 'SMALLINT', 'INT', 'INTEGER', 'BIGINT']; | |||
| const DECIMAL_TYPES = ['FLOAT', 'DOUBLE']; | |||
There was a problem hiding this comment.
Can we use Set? It's faster than arrays.
const DECIMAL_TYPES = new Set(['FLOAT', 'DOUBLE'])
DECIMAL_TYPES.has(columnType)
Not sure what types are returned to the frontend, but are we sure these types are exhaustive: https://github.com/apache/incubator-superset/blob/a6cedaaa879348aca49a520793bb20e63d152a1f/superset/connectors/base/models.py#L480-L493
Might want to add LONG to INTEGRAL_TYPES; and NUMERIC, NUMBER, REAL and DECIMAL to DECIMAL_TYPES, too?
|
LGTM, just a couple of style nits. |
| this.setState({ [attr]: value }, this.onChange); | ||
| let typedValue = value; | ||
| const { column: selectedColumnName, multiple } = this.state; | ||
| if (value && !multiple && attr === 'defaultValue') { |
There was a problem hiding this comment.
should defaultValue be a string constant somewhere that's used here and in the filter config?
There was a problem hiding this comment.
added 2 constants, defaultValue and multiple (used by this feature). but there are quite a few other attributes used in many places...
| } else if (DECIMAL_TYPES.includes(type)) { | ||
| typedValue = parseFloat(value); | ||
| } | ||
| } catch (ex) { |
There was a problem hiding this comment.
when does this fail? I don't think parseInt or parseFloat throw errors with invalid input. instead they return NaN. I think we can remove the try/catch block
graceguo-supercat
force-pushed
the
gg-FilterBoxDefaultValueDataType
branch
2 times, most recently
from
fbc4fcb
to
c8afb83
Compare
graceguo-supercat
force-pushed
the
gg-FilterBoxDefaultValueDataType
branch
from
c8afb83
to
183f980
Compare
* fix: [filter_box] fix 2 issues in single value filter_box * add unit test * add fix per comments
mistercrunch
added
🏷️ bot
SUMMARY
This PR is to fix 2 issues in single value filter_box.
BEFORE
Issue 1: Clear filter_box won't clear filter's default value
Issue 2: Set default value for a single value filter_box, the column's data type is FLOAT:
In dashboard, this string value doesn't match with filter's data type, so that it looks the same option show twice:Proposed solution:
TEST PLAN
CI and manual test.
@mistercrunch @ktmud @etr2460