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.

Sign up for GitHub

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

Jump to bottom

fix: inability to remove chart filter when dashboard time filter is applied #25217

Merged
merged 2 commits into from
Sep 12, 2023
Merged

fix: inability to remove chart filter when dashboard time filter is applied #25217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2178a67
fix: Inability to Remove Chart Filter When Dashboard Time Filter is A…
Sep 7, 2023
5609462
fix lint and test
Sep 7, 2023
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
25 changes: 18 additions & 7 deletions superset-frontend/src/explore/actions/saveModalActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,19 @@ const extractAddHocFiltersFromFormData = formDataToHandle =>
{},
);

const hasTemporalRangeFilter = formData =>
(formData?.adhoc_filters || []).some(
filter => filter.operator === Operators.TEMPORAL_RANGE,
);

export const getSlicePayload = (
sliceName,
formDataWithNativeFilters,
dashboards,
owners,
formDataFromSlice = {},
) => {
let adhocFilters = extractAddHocFiltersFromFormData(
const adhocFilters = extractAddHocFiltersFromFormData(
formDataWithNativeFilters,
);

Expand All @@ -76,18 +81,24 @@ export const getSlicePayload = (
// would end up as an extra filter and when overwriting the chart the original
// time range adhoc_filter was lost
if (isEmpty(adhocFilters?.adhoc_filters) && !isEmpty(formDataFromSlice)) {
adhocFilters = extractAddHocFiltersFromFormData(formDataFromSlice);
formDataFromSlice?.adhoc_filters?.forEach(filter => {
Copy link
Member

Choose a reason for hiding this comment

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

@lilykuang the comment states:

Retain adhoc_filters from the slice if no adhoc_filters are present

but now we're only retaining temporal filters where !isExtra. Is this correct or do we also need to retain adhoc filters that are not temporal? If this is correct, can you adjust the comment to reflect the change?

if (filter.operator === Operators.TEMPORAL_RANGE && !filter.isExtra) {
adhocFilters.adhoc_filters.push({ ...filter, comparator: 'No filter' });
}
});
}

// This loop iterates through the adhoc_filters array in formDataWithNativeFilters.
// If a filter is of type TEMPORAL_RANGE and isExtra, it sets its comparator to
// 'No filter' and adds the modified filter to the adhocFilters array. This ensures that all
// TEMPORAL_RANGE filters are converted to 'No filter' when saving a chart.
formDataWithNativeFilters?.adhoc_filters?.forEach(filter => {
if (filter.operator === Operators.TEMPORAL_RANGE && filter.isExtra) {
adhocFilters.adhoc_filters.push({ ...filter, comparator: 'No filter' });
}
});
if (!hasTemporalRangeFilter(adhocFilters)) {
formDataWithNativeFilters?.adhoc_filters?.forEach(filter => {
if (filter.operator === Operators.TEMPORAL_RANGE && filter.isExtra) {
adhocFilters.adhoc_filters.push({ ...filter, comparator: 'No filter' });
}
});
}

const formData = {
...formDataWithNativeFilters,
Expand Down
14 changes: 1 addition & 13 deletions superset-frontend/src/explore/actions/saveModalActions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,19 +302,7 @@ describe('getSlicePayload', () => {
formDataWithNativeFilters,
dashboards,
owners,
{
datasource: '22__table',
viz_type: 'pie',
adhoc_filters: [
{
clause: 'WHERE',
subject: 'year',
operator: 'TEMPORAL_RANGE',
comparator: 'No filter',
expressionType: 'SIMPLE',
},
],
},
formDataFromSlice,
);
expect(result).toHaveProperty('params');
expect(result).toHaveProperty('slice_name', sliceName);
Expand Down
Loading