You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/21 06:41:42 UTC
[GitHub] [incubator-superset] mistercrunch opened a new pull request #9340:
feat: [explore] don't save filters inherited from a dashboard
mistercrunch opened a new pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340
<img width="604" alt="Screen Shot 2020-03-20 at 11 30 17 PM" src="https://user-images.githubusercontent.com/487433/77221143-534fc780-6b04-11ea-8257-34b6579b8cdb.png">
When navigating to explore from a dashboard context, the current
dashboard filter(s) are passed along to explore so that the context is
kept. So say you're filtering on "country=Romania", in your dashboard
and pivot to explore, that filter is still there and keep on exploring.
Now a common issue is that you'll want to make some tweak to your chart
that are unrelated to the filter, say toggling the legend off for
instance, and then save it. Now you back to your dashboard and even
though you started with an "all countries" dashboard, with a global
filter on country, now that one chart is stuck on "Romania". Typically
you notice this when filtering on something else, say "Italy" and then
that one chart now has two mutually exclusive filters, and show "No data".
Now, the fix is to flag the filter as "extra" (that's the not-so-good internal
name we use for these inherited filters) and make it clear that that
specific filter is special and won't be saved when saving the chart.
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9340: feat:
[explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340#issuecomment-603035613
FYI did a bit of design review with @rusackas today and we decided it was ok. Discussed alternative like introducing a "Inherited Filters" control similar to `Filters`, but would be confusing if empty. Decided to roll with this for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch opened a new pull request #9340:
feat: [explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
mistercrunch opened a new pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340
<img width="604" alt="Screen Shot 2020-03-20 at 11 30 17 PM" src="https://user-images.githubusercontent.com/487433/77221143-534fc780-6b04-11ea-8257-34b6579b8cdb.png">
When navigating to explore from a dashboard context, the current
dashboard filter(s) are passed along to explore so that the context is
kept. So say you're filtering on "country=Romania", in your dashboard
and pivot to explore, that filter is still there and keep on exploring.
Now a common issue is that you'll want to make some tweak to your chart
that are unrelated to the filter, say toggling the legend off for
instance, and then save it. Now you back to your dashboard and even
though you started with an "all countries" dashboard, with a global
filter on country, now that one chart is stuck on "Romania". Typically
you notice this when filtering on something else, say "Italy" and then
that one chart now has two mutually exclusive filters, and show "No data".
Now, the fix is to flag the filter as "extra" (that's the not-so-good internal
name we use for these inherited filters) and make it clear that that
specific filter is special and won't be saved when saving the chart.
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch closed pull request #9340: feat:
[explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
mistercrunch closed pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9340: feat:
[explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340#issuecomment-602390387
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=h1) Report
> Merging [#9340](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/5e6662ab12e4b5d3c392e8cc1bfd37afc5b20b63&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9340 +/- ##
=======================================
Coverage 58.96% 58.96%
=======================================
Files 374 374
Lines 12137 12140 +3
Branches 2992 2994 +2
=======================================
+ Hits 7156 7158 +2
- Misses 4802 4803 +1
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ntend/src/explore/components/AdhocFilterOption.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9340/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9BZGhvY0ZpbHRlck9wdGlvbi5qc3g=) | `57.69% <66.66%> (-0.65%)` | :arrow_down: |
| [superset-frontend/src/explore/AdhocFilter.js](https://codecov.io/gh/apache/incubator-superset/pull/9340/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQWRob2NGaWx0ZXIuanM=) | `90.62% <100.00%> (+0.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=footer). Last update [5e6662a...935a46b](https://codecov.io/gh/apache/incubator-superset/pull/9340?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #9340: feat: [explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340#discussion_r396918585
##########
File path: superset/utils/core.py
##########
@@ -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"):
Review comment:
I agree `filter_ ` is slightly better but being lazy and mergin' as is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9340: feat: [explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340#discussion_r396652702
##########
File path: superset/utils/core.py
##########
@@ -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"):
Review comment:
nit: `extra_filter` or `filter_`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch merged pull request #9340: feat:
[explore] don't save filters inherited from a dashboard
Posted by GitBox <gi...@apache.org>.
mistercrunch merged pull request #9340: feat: [explore] don't save filters inherited from a dashboard
URL: https://github.com/apache/incubator-superset/pull/9340
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org