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/02/26 22:19:35 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request
#9213: [fix] remove chart id from filter_scopes metadata if chart is not in
dash anymore
graceguo-supercat opened a new pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
when user or db migration removed a chart from dashboard, we didn't update filter_scopes metadata. It didn't cause any issue when we load filters in dashboard. But when user copy dashboard with slices, it will show error because dashboard can't find the new slice id from the copied dashboard:
<img width="610" alt="Screen Shot 2020-02-26 at 12 18 08 PM" src="https://user-images.githubusercontent.com/27990562/75393360-bdc56d00-58a2-11ea-8662-270c11ff496b.png">
### TEST PLAN
CI and manual test
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [X] Has associated issue: #9188
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@serenajiang @michellethomas
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384831230
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
Apologies. I didn't comprehend that `slice_id` was a local variable in the list comprehension. It's still not clear if `old_to_new_slc_id_dict[slice_id]` can be `None` and hence whether the logic on line 87 is correct or can be simplified to my previous comment.
----------------------------------------------------------------
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] john-bodley commented on issue #9213: [fix]
remove chart id from filter_scopes metadata if chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#issuecomment-591701669
@graceguo-supercat part of my confusion was the reuse of the `slice_id` local variable. For readability there may be merit in using a different variable name. This comment is non-blocking.
----------------------------------------------------------------
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] graceguo-supercat edited a comment on issue
#9213: [fix] remove chart id from filter_scopes metadata if chart is not in
dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#issuecomment-591707563
thanks John. I feel it is very bad to reuse local variable. I didn't notice it. Definitely need fix it.
> @graceguo-supercat part of my confusion was the reuse of the `slice_id` local variable. For readability there may be merit in using a different variable name. This comment is non-blocking.
----------------------------------------------------------------
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] graceguo-supercat edited a comment on issue
#9213: [fix] remove chart id from filter_scopes metadata if chart is not in
dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#issuecomment-591707563
thanks John. I feel it is very bad to reuse local variable. I didn't notice it. Definitely I need fix it.
> @graceguo-supercat part of my confusion was the reuse of the `slice_id` local variable. For readability there may be merit in using a different variable name. This comment is non-blocking.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384814308
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
Review comment:
I realize this logic was in the old code but is `slice_id` a `str` as opposed to an `int`?
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384821988
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
Actually I don't think line 87 is required since this is `new_filter_key` which on line 81 you already tested its truthiness.
----------------------------------------------------------------
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] graceguo-supercat commented on issue #9213:
[fix] remove chart id from filter_scopes metadata if chart is not in dash
anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#issuecomment-591707563
thanks John. I feel it is very bad to reuse local variable. I didn't notice it. Definitely need fix. it.
> @graceguo-supercat part of my confusion was the reuse of the `slice_id` local variable. For readability there may be merit in using a different variable name. This comment is non-blocking.
----------------------------------------------------------------
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] graceguo-supercat merged pull request #9213:
[fix] remove chart id from filter_scopes metadata if chart is not in dash
anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384807104
##########
File path: superset/views/core.py
##########
@@ -1207,6 +1207,14 @@ def copy_dash(self, dashboard_id):
data["filter_scopes"] = json.dumps(new_filter_scopes)
else:
dash.slices = original_dash.slices
+ # remove slice id from filter_scopes metadata if slice is removed from dashboard
+ if "filter_scopes" in data:
+ new_filter_scopes = copy_filter_scopes(
Review comment:
Not blocking, but is this still necessary even though the old slice and new slice ids are the same?
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384818983
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
Review comment:
Yes `filter_scopes` metadata, its key `slice_id` is `str`. `old_to_new_slc_id_dict` is Dict[int, int].
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384813744
##########
File path: superset/views/core.py
##########
@@ -1207,6 +1207,14 @@ def copy_dash(self, dashboard_id):
data["filter_scopes"] = json.dumps(new_filter_scopes)
else:
dash.slices = original_dash.slices
+ # remove slice id from filter_scopes metadata if slice is removed from dashboard
+ if "filter_scopes" in data:
+ new_filter_scopes = copy_filter_scopes(
Review comment:
the idea here is to reuse `copy_filter_scopes` function.
in the case where user wants to copy all charts, `old_to_new_slc_id_dict` will look like: `{1: 101, 2: 102, 3: 103}`.
and when user don't want to copy all charts, `old_to_new_slc_id_dict` will look like: `{1: 1, 2: 2, 3: 3}`. and all entries in the dict should be slices included in the dash.
`copy_filter_scopes` will scan slice id. it will replace filter_scopes metadata with new slice_ids from dict or just copy with old slice_id. But I don't have to write another function just to remove old slice ids.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384815514
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
Can the value of `old_to_new_slc_id_dict` be `None` for an existing key? If not I think having,
```python
if slice_id in old_to_new_slc_id_dict
```
would be clearer. Also on line 80 the logic is `get(int(slice_id))` whereas here it's `get(slice_id)`.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384818983
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
Review comment:
Yes `filter_scopes` metadata, its key `slice_id` is `str`. `old_to_new_slc_id_dict` is Dict[int, int]. I agree it is very confusing. But in JS, object (or dictionary)'s key is always string, or casted to be string.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384831899
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
Review comment:
You probably want `scope.get("immune, [])` so that it returns an iterable as opposed to `None`.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384813744
##########
File path: superset/views/core.py
##########
@@ -1207,6 +1207,14 @@ def copy_dash(self, dashboard_id):
data["filter_scopes"] = json.dumps(new_filter_scopes)
else:
dash.slices = original_dash.slices
+ # remove slice id from filter_scopes metadata if slice is removed from dashboard
+ if "filter_scopes" in data:
+ new_filter_scopes = copy_filter_scopes(
Review comment:
the idea here is to reuse `copy_filter_scopes` function.
in the case where user wants to copy all charts, `old_to_new_slc_id_dict` will look like: `{1: 101, 2: 102, 3: 103}`.
when user don't want to copy all charts, `old_to_new_slc_id_dict` will look like: `{1: 1, 2: 2, 3: 3}`. and all entries in the dict should be slices included in the dash.
`copy_filter_scopes` will scan slice id and create a new copy. it will replace filter_scopes metadata with new slice_ids from dict or just copy with old slice_id. But I don't have to write another function just to remove old slice ids.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384821647
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
Review comment:
I believe this could be `new_filter_key`.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384832504
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
`old_to_new_slc_id_dict[int(slice_id)]` should not be None as long as slice_id in the `old_to_new_slc_id_dict`
----------------------------------------------------------------
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 #9213: [fix]
remove chart id from filter_scopes metadata if chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#issuecomment-591728578
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9213?src=pr&el=h1) Report
> Merging [#9213](https://codecov.io/gh/apache/incubator-superset/pull/9213?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4f73f8a1f9fec7f15ec760d6d98617bbe04f4023?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9213?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9213 +/- ##
=======================================
Coverage 58.91% 58.91%
=======================================
Files 372 372
Lines 11996 11996
Branches 2937 2937
=======================================
Hits 7068 7068
Misses 4750 4750
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9213?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/9213?src=pr&el=footer). Last update [4f73f8a...9cb7db7](https://codecov.io/gh/apache/incubator-superset/pull/9213?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] john-bodley commented on a change in pull
request #9213: [fix] remove chart id from filter_scopes metadata if chart
is not in dash anymore
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384821344
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
Review comment:
@graceguo-supercat if that's the case lines 85 and 87 would need to be updated.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384818983
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
Review comment:
Yes `filter_scopes` metadata, its key `slice_id` is `str`. `old_to_new_slc_id_dict` is Dict[int, int]. I agree it is very confusing. But in JS, object (or dictionary)'s key is always string, or be casted to string.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384825903
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
it is necessary. line 81 is check filter slice is still in the dashboard, line 87 is to check if immune charts still in current dashboard.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9213: [fix] remove chart id from filter_scopes metadata if
chart is not in dash anymore
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9213: [fix] remove chart id from filter_scopes metadata if chart is not in dash anymore
URL: https://github.com/apache/incubator-superset/pull/9213#discussion_r384832504
##########
File path: superset/utils/dashboard_filter_scopes_converter.py
##########
@@ -77,10 +77,13 @@ def copy_filter_scopes(
) -> Dict:
new_filter_scopes = {}
for (slice_id, scopes) in old_filter_scopes.items():
- new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
- new_filter_scopes[str(new_filter_key)] = scopes
- for scope in scopes.values():
- scope["immune"] = [
- old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
- ]
+ new_filter_key = old_to_new_slc_id_dict.get(int(slice_id))
+ if new_filter_key:
+ new_filter_scopes[str(new_filter_key)] = scopes
+ for scope in scopes.values():
+ scope["immune"] = [
+ old_to_new_slc_id_dict[slice_id]
+ for slice_id in scope.get("immune")
+ if old_to_new_slc_id_dict.get(slice_id) is not None
Review comment:
`old_to_new_slc_id_dict[int(slice_id)]` should not be None as long as slice_id in the `old_to_new_slc_id_dict`. So now i use:
> if int(slice_id) in old_to_new_slc_id_dict
----------------------------------------------------------------
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