fix(native-filters): add column required validation for filter_select #33377
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(cherry picked from commit 226c7f8)
(cherry picked from commit a4880ca)
apache#24593) (cherry picked from commit 04ae259)
Co-authored-by: Justin Park <[email protected]> (cherry picked from commit 0836000)
…ic (apache#24129) (cherry picked from commit 383dac6)
(cherry picked from commit 64d728f)
(cherry picked from commit d041648)
(cherry picked from commit 93ba4ad)
(cherry picked from commit a6e749d)
…pache#24609) (cherry picked from commit c573cfc)
(cherry picked from commit d74d7ec)
(cherry picked from commit c53b249)
(cherry picked from commit b809815)
(cherry picked from commit fe2c14f)
(cherry picked from commit e6e8276)
(cherry picked from commit 4881328)
(cherry picked from commit 65291a0)
apache#24610) (cherry picked from commit 0efb884)
(cherry picked from commit bbffc4c)
…pache#24630) (cherry picked from commit 4caf33b)
Co-authored-by: Justin Park <[email protected]> (cherry picked from commit ca8c8d2)
Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit 7409289)
Co-authored-by: Justin Park <[email protected]> (cherry picked from commit 1473d97)
(cherry picked from commit 462418b)
(cherry picked from commit 767afef)
Co-authored-by: Michael S. Molina <[email protected]> (cherry picked from commit a156816)
arthi-arumugam99
requested review from
rusackas,
geido,
eschutho,
betodealmeida,
nytai,
mistercrunch,
craig-rueda,
kgabryje,
dpgaspar,
sadpandajoe,
michael-s-molina and
Antonio-RiveroMartnez
as code owners
dosubot
bot
added
the
dashboard:native-filters
github-actions
bot
added
i18n
|
/korbit-review |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Misspelled 'length' Property ▹ view | 🧠 Not in scope |
|
Uninitialized Class Property ▹ view | 🧠 Not in scope |
 |
Unsafe Access of Column Types Array ▹ view | 🧠 Not in scope |
 |
Missing function documentation ▹ view | 🧠 Incorrect |
|
Insufficient timezone choice rationale ▹ view | 🧠 Not in scope |
 |
Silent Failure Due to Property Typo ▹ view | 🧠 Incorrect |
 |
Method has multiple responsibilities ▹ view | 🧠 Not in standard |
|
Incorrect Empty Value Validation Logic ▹ view | 🧠 Not in standard |
|
Checkbox Props Override Risk ▹ view | 🧠 Not in standard |
|
Incomplete Type Guard Validation ▹ view | 🧠 Incorrect |
Files scanned
Due to the exceptionally large size of this PR, I've limited my review to the following files:
| File Path | Reviewed |
|---|---|
| superset-frontend/src/explore/components/controls/CurrencyControl/index.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/validator/validateMaxValue.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/currency-format/index.ts | ✅ |
| docker/docker-entrypoint-initdb.d/examples-init.sh | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/index.tsx | ✅ |
| superset-frontend/packages/superset-ui-core/src/validator/index.ts | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/index.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/types.ts | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/components/ControlSubSectionHeader.tsx | ✅ |
| superset-frontend/src/explore/reducers/saveModalReducer.js | ✅ |
| superset-frontend/src/components/Tooltip/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/state.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/CancelConfirmationAlert.tsx | ✅ |
| superset-frontend/src/components/WarningIconWithTooltip/index.tsx | ✅ |
| superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/ValidatedInputField.tsx | ✅ |
| superset-frontend/src/dashboard/constants.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/time-format/index.ts | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/utils/isEqualColumns.ts | ✅ |
| superset-frontend/plugins/plugin-chart-handlebars/src/types.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FilterCard/Styles.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts | ✅ |
| superset-frontend/jest.config.js | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnTypeLabel/ColumnTypeLabel.tsx | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js | ✅ |
| superset-frontend/src/components/Tags/utils.tsx | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/controls.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/chart/types/Base.ts | ✅ |
| superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts | ✅ |
| superset-frontend/src/dataMask/actions.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts | ✅ |
| superset-frontend/src/dashboard/util/getDashboardUrl.ts | ✅ |
| superset-frontend/cypress-base/cypress.config.ts | ✅ |
| superset-frontend/src/dashboard/components/DashboardBuilder/state.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/getYAxisFormatter.ts | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/types.ts | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx | ✅ |
| superset-frontend/src/GlobalStyles.tsx | ✅ |
| superset-frontend/src/explore/components/controls/index.js | ✅ |
| superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/buildQuery.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/index.tsx | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-heatmap/src/transformProps.js | ✅ |
| superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx | ✅ |
| superset-frontend/src/dashboard/util/crossFilters.ts | ✅ |
| superset-frontend/src/dashboard/util/permissionUtils.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/state.ts | ✅ |
| superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/controlPanel.ts | ✅ |
| superset-frontend/src/components/ErrorMessage/types.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx | ✅ |
| superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts | ✅ |
| superset-frontend/plugins/legacy-preset-chart-nvd3/src/transformProps.js | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Treemap/controlPanel.tsx | ✅ |
| superset-frontend/src/dataMask/reducer.ts | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-heatmap/src/ReactHeatmap.jsx | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/sections/chartTitle.tsx | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-world-map/src/controlPanel.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/time-format/formatters/finestTemporalGrain.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/types.ts | ✅ |
| superset-frontend/packages/superset-ui-core/src/query/types/AnnotationLayer.ts | ✅ |
| superset-frontend/src/explore/components/ControlHeader.tsx | ✅ |
| superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx | ✅ |
| superset-frontend/src/features/dashboards/DashboardCard.tsx | ✅ |
| superset-frontend/src/features/databases/types.ts | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitleContainer.tsx | ✅ |
| superset-frontend/src/dashboard/actions/sliceEntities.ts | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigPopover.tsx | ✅ |
| superset-frontend/src/features/charts/ChartCard.tsx | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ControlForm/ControlFormItem.tsx | ✅ |
| superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts | ✅ |
| superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts | ✅ |
| superset-frontend/src/components/Select/types.ts | ✅ |
| superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx | ✅ |
| docker/pythonpath_dev/superset_config.py | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx | ✅ |
| superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigControl.tsx | ✅ |
| superset-frontend/src/components/DynamicPlugins/index.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/controlPanel.tsx | ✅ |
| superset-embedded-sdk/src/index.ts | ✅ |
| superset-frontend/src/SqlLab/App.jsx | ✅ |
| superset-frontend/src/dashboard/components/SaveModal.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx | ✅ |
| superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/index.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx | ✅ |
| superset-frontend/src/components/MetadataBar/MetadataBar.tsx | ✅ |
| superset-frontend/src/components/ErrorMessage/ErrorAlert.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx | ✅ |
| superset-frontend/src/SqlLab/components/SaveQuery/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/nativeFilters/utils.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts | ✅ |
| superset-frontend/src/SqlLab/components/App/index.jsx | ✅ |
| superset-frontend/src/explore/components/controls/CurrencyControl/CurrencyControl.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| @@ -121,14 +124,27 @@ class App extends React.PureComponent { | |||
| } | |||
|
|
|||
| componentDidUpdate() { | |||
| const { localStorageUsageInKilobytes, actions, queries } = this.props; | |||
| const queryCount = queries?.lenghth || 0; | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ); | ||
| this.hasLoggedLocalStorageUsage = true; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } = formData; | ||
|
|
||
| const { data = [], coltypes = [] } = queriesData[0]; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| getTimeFormatter, | ||
| getValueFormatter, | ||
| } from '@superset-ui/core'; | ||
|
|
||
| export default function transformProps(chartProps) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
+20
to
+21
| // timezone for unit tests | ||
| process.env.TZ = 'America/New_York'; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
126
to
+128
| componentDidUpdate() { | ||
| const { localStorageUsageInKilobytes, actions, queries } = this.props; | ||
| const queryCount = queries?.lenghth || 0; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
126
to
149
| componentDidUpdate() { | ||
| const { localStorageUsageInKilobytes, actions, queries } = this.props; | ||
| const queryCount = queries?.lenghth || 0; | ||
| if ( | ||
| this.props.localStorageUsageInKilobytes >= | ||
| localStorageUsageInKilobytes >= | ||
| LOCALSTORAGE_WARNING_THRESHOLD * LOCALSTORAGE_MAX_USAGE_KB | ||
| ) { | ||
| this.showLocalStorageUsageWarning( | ||
| this.props.localStorageUsageInKilobytes, | ||
| this.props.queries?.lenghth || 0, | ||
| localStorageUsageInKilobytes, | ||
| queryCount, | ||
| ); | ||
| } | ||
| if (localStorageUsageInKilobytes > 0 && !this.hasLoggedLocalStorageUsage) { | ||
| const eventData = { | ||
| current_usage: localStorageUsageInKilobytes, | ||
| query_count: queryCount, | ||
| }; | ||
| actions.logEvent( | ||
| LOG_ACTIONS_SQLLAB_MONITOR_LOCAL_STORAGE_USAGE, | ||
| eventData, | ||
| ); | ||
| this.hasLoggedLocalStorageUsage = true; | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
69
to
74
| const errors = | ||
| (validators | ||
| ?.map(validator => | ||
| !required && isEmptyValue(fieldValue) ? false : validator(fieldValue), | ||
| isEmptyValue(fieldValue) ? false : validator(fieldValue), | ||
| ) | ||
| .filter(x => !!x) as string[]) || []; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
96
to
+104
| <ControlFormItemComponents.Checkbox | ||
| checked={value as boolean} | ||
| value={value as boolean} | ||
| onChange={handleChange} | ||
| > | ||
| {label}{' '} | ||
| {hovered && description && ( | ||
| <InfoTooltipWithTrigger tooltip={description} /> | ||
| )} | ||
| </ControlFormItemComponents.Checkbox> | ||
| name={name} | ||
| label={label} | ||
| description={description} | ||
| validationErrors={validationErrors} | ||
| {...props} | ||
| /> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
+67
to
+71
| export function isTabLayoutItem( | ||
| layoutItem: ColumnConfigFormItem[] | TabLayoutItem, | ||
| ): layoutItem is TabLayoutItem { | ||
| return !!(layoutItem as TabLayoutItem)?.tab; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
PR isn't right, shows 215 commits. Please start from a recent commit on |
|
Looks like a rebase went wrong here. You can open a fresh PR or try the rebase again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a required field validation to ensure that users select a column when configuring a native filter of type
filter_select.Bug Fix Summary
Previously, users could create a native filter of type
Value(i.e.,filter_select) without choosing a column. This caused runtime issues and invalid filters.This PR fixes it by enforcing required validation on the column field in the filter configuration panel.
Screenshot
Test Plan
Bug reproduced and resolved successfully.