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

feat: [explore] don't save filters inherited from a dashboard #9340

Merged
merged 2 commits into from
Mar 24, 2020
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 @@ -40,6 +40,7 @@ describe('AdhocFilter', () => {
filterOptionName: adhocFilter.filterOptionName,
sqlExpression: null,
fromFormData: false,
isExtra: false,
});
});

Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/explore/AdhocFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export default class AdhocFilter {
this.operator = null;
this.comparator = null;
}
this.isExtra = !!adhocFilter.isExtra;
this.fromFormData = !!adhocFilter.filterOptionName;

this.filterOptionName =
Expand Down
40 changes: 27 additions & 13 deletions superset-frontend/src/explore/components/AdhocFilterOption.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Label, OverlayTrigger } from 'react-bootstrap';
import { t } from '@superset-ui/translation';

import AdhocFilterEditPopover from './AdhocFilterEditPopover';
import AdhocFilter from '../AdhocFilter';
import columnType from '../propTypes/columnType';
import adhocMetricType from '../propTypes/adhocMetricType';
import InfoTooltipWithTrigger from '../../components/InfoTooltipWithTrigger';

const propTypes = {
adhocFilter: PropTypes.instanceOf(AdhocFilter).isRequired,
Expand Down Expand Up @@ -80,7 +82,6 @@ export default class AdhocFilterOption extends React.PureComponent {
datasource={this.props.datasource}
/>
);

return (
<OverlayTrigger
ref="overlay"
Expand All @@ -93,18 +94,31 @@ export default class AdhocFilterOption extends React.PureComponent {
onEntered={this.onOverlayEntered}
onExited={this.onOverlayExited}
>
<Label className="adhoc-filter-option">
<div onMouseDownCapture={this.onMouseDown}>
<span className="m-r-5 option-label">
{adhocFilter.getDefaultLabel()}
<i
className={`glyphicon glyphicon-triangle-${
this.state.overlayShown ? 'left' : 'right'
} adhoc-label-arrow`}
/>
</span>
</div>
</Label>
<div>
{adhocFilter.isExtra && (
<InfoTooltipWithTrigger
icon="exclamation-triangle"
placement="top"
className="m-r-5 text-muted"
tooltip={t(`
This filter was inherited from the dashboard's context.
It won't be saved when saving the chart.
`)}
/>
)}
<Label className="adhoc-filter-option">
<div onMouseDownCapture={this.onMouseDown}>
<span className="m-r-5 option-label">
{adhocFilter.getDefaultLabel()}
<i
className={`glyphicon glyphicon-triangle-${
this.state.overlayShown ? 'left' : 'right'
} adhoc-label-arrow`}
/>
</span>
</div>
</Label>
</div>
</OverlayTrigger>
);
}
Expand Down
4 changes: 2 additions & 2 deletions superset/examples/random_time_series.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ def load_random_time_series_data(only_metadata=False, force=False):
slice_data = {
"granularity_sqla": "day",
"row_limit": config["ROW_LIMIT"],
"since": "1 year ago",
"until": "now",
"since": "2019-01-01",
"until": "2019-02-01",
"metric": "count",
"viz_type": "cal_heatmap",
"domain_granularity": "month",
Expand Down
2 changes: 2 additions & 0 deletions superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,7 @@ def to_adhoc(filt, expressionType="SIMPLE", clause="where"):
"clause": clause.upper(),
"expressionType": expressionType,
"filterOptionName": str(uuid.uuid4()),
"isExtra": True if filt.get("isExtra") is True else False,
}

if expressionType == "SIMPLE":
Expand Down Expand Up @@ -860,6 +861,7 @@ def get_filter_key(f):
existing_filters[get_filter_key(existing)] = existing["comparator"]

for filtr in form_data["extra_filters"]:
filtr["isExtra"] = True
# Pull out time filters/options and merge into form data
if date_options.get(filtr["col"]):
if filtr.get("val"):
Comment on lines 863 to 867
Copy link
Member

Choose a reason for hiding this comment

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

nit: extra_filter or filter_

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree filter_ is slightly better but being lazy and mergin' as is.
ship

Expand Down
10 changes: 10 additions & 0 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,12 @@ def filter(self, datasource_type, datasource_id, column):
)
return json_success(payload)

@staticmethod
def remove_extra_filters(filters):
"""Extra filters are ones inherited from the dashboard's temporary context
Those should not be saved when saving the chart"""
return [f for f in filters if not f.get("isExtra")]

def save_or_overwrite_slice(
self,
args,
Expand All @@ -967,6 +973,10 @@ def save_or_overwrite_slice(
form_data.pop("slice_id") # don't save old slice_id
slc = Slice(owners=[g.user] if g.user else [])

form_data["adhoc_filters"] = self.remove_extra_filters(
form_data.get("adhoc_filters", [])
)

slc.params = json.dumps(form_data, indent=2, sort_keys=True)
slc.datasource_name = datasource_name
slc.viz_type = form_data["viz_type"]
Expand Down
19 changes: 14 additions & 5 deletions tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,18 +288,20 @@ def test_save_slice(self):
self.login(username="admin")
slice_name = f"Energy Sankey"
slice_id = self.get_slice(slice_name, db.session).id
copy_name = f"Test Sankey Save_{random.random()}"
copy_name_prefix = "Test Sankey"
copy_name = f"{copy_name_prefix}[save]{random.random()}"
tbl_id = self.table_ids.get("energy_usage")
new_slice_name = f"Test Sankey Overwrite_{random.random()}"
new_slice_name = f"{copy_name_prefix}[overwrite]{random.random()}"

url = (
"/superset/explore/table/{}/?slice_name={}&"
"action={}&datasource_name=energy_usage"
)

form_data = {
"adhoc_filters": [],
"viz_type": "sankey",
"groupby": "target",
"groupby": ["target"],
"metric": "sum__value",
"row_limit": 5000,
"slice_id": slice_id,
Expand All @@ -319,8 +321,9 @@ def test_save_slice(self):
self.assertEqual(slc.viz.form_data, form_data)

form_data = {
"adhoc_filters": [],
"viz_type": "sankey",
"groupby": "source",
"groupby": ["source"],
"metric": "sum__value",
"row_limit": 5000,
"slice_id": new_slice_id,
Expand All @@ -338,7 +341,13 @@ def test_save_slice(self):
self.assertEqual(slc.viz.form_data, form_data)

# Cleanup
db.session.delete(slc)
slices = (
db.session.query(Slice)
.filter(Slice.slice_name.like(copy_name_prefix + "%"))
.all()
)
for slc in slices:
db.session.delete(slc)
db.session.commit()

def test_filter_endpoint(self):
Expand Down
4 changes: 4 additions & 0 deletions tests/viz_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,7 @@ def test_filter_nulls(self, mock_uuid4):
"comparator": "",
"operator": "IS NOT NULL",
"subject": "lat",
"isExtra": False,
},
{
"clause": "WHERE",
Expand All @@ -1089,6 +1090,7 @@ def test_filter_nulls(self, mock_uuid4):
"comparator": "",
"operator": "IS NOT NULL",
"subject": "lon",
"isExtra": False,
},
],
"delimited_key": [
Expand All @@ -1099,6 +1101,7 @@ def test_filter_nulls(self, mock_uuid4):
"comparator": "",
"operator": "IS NOT NULL",
"subject": "lonlat",
"isExtra": False,
}
],
"geohash_key": [
Expand All @@ -1109,6 +1112,7 @@ def test_filter_nulls(self, mock_uuid4):
"comparator": "",
"operator": "IS NOT NULL",
"subject": "geo",
"isExtra": False,
}
],
}
Expand Down