john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1128519681
##########
superset/cli/main.py:
##########
@@ -50,9 +50,12 @@ def make_shell_context() -> Dict[str, Any]:
):
module = importlib.import_module(module_name)
for attribute in module.__dict__.values():
- if isinstance(attribute, click.core.Command):
+ if isinstance(attribute, (click.core.Command, click.core.Group)):
superset.add_command(attribute)
+ if isinstance(attribute, click.core.Group):
Review Comment:
We need to ensure that group commands aren't added as a top-level command.
Regrettably there's no way of inspecting a (sub-)command to see if it's part of
a group, but given how groups are defined, i.e., prior to the sub-commands it
seems valid to simply short circuit.
I sense the only other option is to remove the magic auto-detection logic
and explicitly register all the commands.
##########
superset/dashboards/dao.py:
##########
@@ -200,7 +200,7 @@ def set_dash_metadata( # pylint: disable=too-many-locals
slice_ids = [
value.get("meta", {}).get("chartId")
for value in positions.values()
- if isinstance(value, dict)
+ if isinstance(value, dict) and value.get("type") == "CHART"
Review Comment:
See above. Without this logic, when editing a dashboard all the old
filter-box charts would be re-added given that the markdown elements still
contain the `chartId` field.
##########
superset/views/core.py:
##########
@@ -1271,7 +1271,11 @@ def copy_dash( # pylint: disable=no-self-use
# update chartId of layout entities
for value in data["positions"].values():
- if isinstance(value, dict) and value.get("meta",
{}).get("chartId"):
+ if (
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
Review Comment:
See above.
##########
superset/dashboards/commands/importers/v0.py:
##########
@@ -131,7 +131,7 @@ def alter_positions(
for value in position_json:
if (
isinstance(value, dict)
- and value.get("meta")
Review Comment:
This is not required per like #135.
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -88,3 +90,160 @@ def copy_filter_scopes(
if int(slice_id) in old_to_new_slc_id_dict
]
return new_filter_scopes
+
+
+def convert_filter_scopes_to_native_filters( # pylint:
disable=invalid-name,too-many-locals,too-many-nested-blocks
Review Comment:
This logic is taken from https://github.com/apache/superset/pull/16992
(specifically the
[ilterboxMigrationHelper.ts](https://github.com/graceguo-supercat/superset/blob/3a27565fe65f4f166ea7ade3d1ee8580ef8f9c17/superset-frontend/src/dashboard/util/filterboxMigrationHelper.ts)
file). Note however the logic here is considerably simpler, i.e., there's no:
1. Druid logic as the Druid NoSQL connector has been deprecated.
2. Complex dependency logic†.
† The reason being is previously the legacy filter-box charts remained in
the dashboards and thus there needed to be additional logic to wire up the
parent/child relationships between the native filters and legacy filter-box
charts. In the current formulation the filter-box charts have been removed
(having been replaced with markdown elements) and thus said logic isn't
required.
##########
superset/dashboards/commands/importers/v0.py:
##########
@@ -131,7 +131,7 @@ def alter_positions(
for value in position_json:
if (
isinstance(value, dict)
- and value.get("meta")
+ and value.get("type") == "CHART"
Review Comment:
This is needed given how as part of the migration we're temporarily storing
the old `chartId` in the metadata block for the new markdown elements (which
serve as a replacement for the filter-box charts) as a way to revert.
Granted this is bastardizing the metadata somewhat, but it does seem prudent
to also ensure that the type is `CHART` prior to fetching the `chartId`.
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -19,13 +19,15 @@
from collections import defaultdict
from typing import Any, Dict, List
+from shortid import ShortId
+
from superset.models.slice import Slice
logger = logging.getLogger(__name__)
def convert_filter_scopes(
- json_metadata: Dict[Any, Any], filters: List[Slice]
+ json_metadata: Dict[Any, Any], filter_boxes: List[Slice]
Review Comment:
Renaming adds clarity as to which type of charts/slices these are.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
