Skip to content
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

fix: Allow only dttm columns in comparison filter in Period over Period chart #27209

Merged
merged 2 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
* under the License.
*/
import {
AdhocFilter,
ComparisonTimeRangeType,
SimpleAdhocFilter,
t,
validateTimeComparisonRangeValues,
} from '@superset-ui/core';
import {
ColumnMeta,
ControlPanelConfig,
ControlPanelState,
ControlState,
Expand Down Expand Up @@ -76,16 +79,29 @@
mapStateToProps: (
state: ControlPanelState,
controlState: ControlState,
) => ({
...(sharedControls.adhoc_filters.mapStateToProps?.(
state,
controlState,
) || {}),
externalValidationErrors: validateTimeComparisonRangeValues(
state.controls?.time_comparison?.value,
controlState.value,
),
}),
) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

const originalMapStateToPropsRes =
sharedControls.adhoc_filters.mapStateToProps?.(
state,
controlState,
) || {};
const columns = originalMapStateToPropsRes.columns.filter(

Check warning on line 88 in superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts#L88

Added line #L88 was not covered by tests
(col: ColumnMeta) =>
col.is_dttm &&
(state.controls.adhoc_filters.value as AdhocFilter[]).some(
(val: SimpleAdhocFilter) =>
val.subject === col.column_name,

Check warning on line 93 in superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts#L93

Added line #L93 was not covered by tests
),
);
return {

Check warning on line 96 in superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts

View check run for this annotation

Codecov / codecov/patch

superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts#L96

Added line #L96 was not covered by tests
...originalMapStateToPropsRes,
columns,
externalValidationErrors: validateTimeComparisonRangeValues(
state.controls?.time_comparison?.value,
controlState.value,
),
};
},
},
},
],
Expand Down Expand Up @@ -138,6 +154,9 @@
y_axis_format: {
label: t('Number format'),
},
adhoc_filters: {
rerender: ['adhoc_custom'],
},
},
formDataOverrides: formData => ({
...formData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,18 @@
return new AdhocFilter(config);
}, [droppedItem]);

const canDrop = useCallback(() => true, []);
const canDrop = useCallback(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non blocking: does any testable behavior changed with this block? If so, could add include it in the DndMetricSelect.test.tsx file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The drag and drop behavior tested in DndMetricSelect.test.tsx is related to reordering the metric values. Testing drag and drop from the datasource panel to the control field is more complicated - I suggest we add those tests in a separate PR, both for filters and metrics

(item: DatasourcePanelDndItem) => {
if (item.type === DndItemType.Column) {
return props.columns.some(
col => col.column_name === (item.value as ColumnMeta).column_name,

Check warning on line 389 in superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx#L388-L389

Added lines #L388 - L389 were not covered by tests
);
}
return true;

Check warning on line 392 in superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx

View check run for this annotation

Codecov / codecov/patch

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx#L392

Added line #L392 was not covered by tests
},
[props.columns],
);

const handleDrop = useCallback(
(item: DatasourcePanelDndItem) => {
setDroppedItem(item.value);
Expand Down
Loading