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/06/20 00:48:41 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #10114: fix: dashboard filter scope bug
graceguo-supercat opened a new pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114
### SUMMARY
this PR is to fix a logic issue in dashboard filter scope.
### TEST PLAN
added unit test to cover this complicated use case.
@ktmud @etr2460
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443721824
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
You have two options:
1. Never store tab ids in scopes, but rather all charts in scope for that tab. All new charts has all filters turned on by default.
2. Change "immune" to `tabScopes`. If tab id is new `scopes`, then new charts added to tab will apply the filter by default, otherwise not.
It's probably better to just have a dict of flat arrays `{ [FilterId]: ChartId[] }`, in which empty means applicable to all charts, and non-empty means only apply to selected charts. Tab tree status can be computed on the fly.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443269971
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
typo: "an other" -> "another"
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
Would be nice if there's a short note that visualizes the dashboard layout:
```js
// - filter0
// - tab1
// - filter1
// - chart1
// - tab2
// - filter2
// - chart2
// - tab2-1
// - chart3
// - chart4
// - tab2-2
// - chart5
// - chart6
//
// The test case expects it's possible to apply filter2 to filter0
```
On a separate note, `chartsImmune` should probably be refactored to `chartsInScope` so the data structure can better align with the UI . Negative condition sometimes is difficult for a human brain to comprehend. ESLint has a rule for it: https://eslint.org/docs/rules/no-negated-condition . (Not that we need to do it soon, of course, but something to keep in mind when it's time to revamp the filters)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443703305
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
Dashboard filter's `scope` data is defined as
- `scopes`: empty or array of tab ids (or root_id when dashboard has no tab).
- `immune`: empty or array of chart ids under above tabs but immune from filter.
If we only hold a list of `chartsInScope` will lost tab's scope information.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443703305
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
Dashboard filter's `scope` data is defined as
- `scopes`: contain one or more tab ids (or root_id when dashboard has no tab).
- `immune` are chart ids under above tabs but immune from filter.
If we only hold a list of `chartsInScope` will lost tab's scope information.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat merged pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443721824
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
Maybe I missed some requirements for the filter scope thing, but it seems you have two options:
1. Never store tab ids in scopes, but rather all charts in scope for that tab. All new charts has all filters turned on by default.
2. Change "immune" to `tabScopes`. If tab id is in `scopes`, then new charts added to tab will apply the filter by default, otherwise not.
I'm wondering what's preventing us from having just a dict of flat arrays `{ [FilterId]: ChartId[] }`, in which empty means applicable to all charts, and non-empty means only apply to selected charts. Tab tree status can be computed on the fly.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#issuecomment-648951984
for `scopes` array, empty means not applicable to any tab. by default, `scopes` value is [`ROOT_ID`], which means apply to all.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443703305
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
Dashboard filter's `scope` data is defined as
- `scopes`: contain one or more tabs(or root_id when dashboard has no tab).
- `immune` are charts under tab but immune from filter.
If we only hold a list of `chartsInScope` will lost tab's scope information.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10114: fix: dashboard filter scope bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10114:
URL: https://github.com/apache/incubator-superset/pull/10114#discussion_r443721824
##########
File path: superset-frontend/spec/javascripts/dashboard/util/getFilterScopeFromNodesTree_spec.js
##########
@@ -157,6 +159,107 @@ describe('getFilterScopeFromNodesTree', () => {
},
];
+ // this is an other commonly used layout for dashboard:
Review comment:
You have two options:
1. Never store tab ids in scopes, but rather all charts in scope for that tab. All new charts has all filters turned on by default.
2. Change "immune" to `tabScopes`. If tab id is in `scopes`, then new charts added to tab will apply the filter by default, otherwise not.
It's probably better to just have a dict of flat arrays `{ [FilterId]: ChartId[] }`, in which empty means applicable to all charts, and non-empty means only apply to selected charts. Tab tree status can be computed on the fly.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org