From c89ed4efdfb3fdca5ec51308dc244b0575e6a411 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 27 Aug 2024 16:42:16 +0200 Subject: [PATCH 1/4] - FIx time comparison migration so charts that are not using comparison can be upgraded properly - Add extra tests to cover or new cases --- ..._update_charts_with_old_time_comparison.py | 26 ++-- ...e_charts_with_old_time_comparison__test.py | 138 ++++++++++++++++++ 2 files changed, 151 insertions(+), 13 deletions(-) diff --git a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py index 431d46799fb4a..0d592118de5b6 100644 --- a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py +++ b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py @@ -65,16 +65,17 @@ class Slice(Base): def upgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: params = deepcopy(slice_params) - if "enable_time_comparison" in params: - # Remove enable_time_comparison - del params["enable_time_comparison"] - # Update time_comparison to time_compare if "time_comparison" in params: time_comp = params.pop("time_comparison") - params["time_compare"] = time_map.get( - time_comp, "inherit" - ) # Default to 'inherit' if not found + params["time_compare"] = ( + time_map.get(time_comp, "inherit") + if "enable_time_comparison" in params and params["enable_time_comparison"] + else "" + ) + + if "enable_time_comparison" in params: + del params["enable_time_comparison"] # Add comparison_type params["comparison_type"] = "values" @@ -119,20 +120,19 @@ def upgrade(): def downgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: params = deepcopy(slice_params) + params["enable_time_comparison"] = False reverse_time_map = { v: k for k, v in time_map.items() } # Reverse the map from the upgrade function - # Add enable_time_comparison - params["enable_time_comparison"] = True - # Revert time_compare to time_comparison if "time_compare" in params: time_comp = params.pop("time_compare") - params["time_comparison"] = reverse_time_map.get( - time_comp, "r" - ) # Default to 'r' if not found + params["time_comparison"] = reverse_time_map.get(time_comp, "r") + # If the chart was using any time compare, enable time comparison + if time_comp: + params["enable_time_comparison"] = True # Remove comparison_type if "comparison_type" in params: diff --git a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py index 5f7fe505c03b1..224297ccee8f6 100644 --- a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py +++ b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py @@ -161,6 +161,70 @@ } ], } +params_v1_other_than_custom_false: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 2000", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_comparison": "r", + "enable_time_comparison": False, + "adhoc_custom": [ + { + "clause": "WHERE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "No filter", + "expressionType": "SIMPLE", + } + ], +} params_v2_with_custom: dict[str, Any] = { "datasource": "2__table", "viz_type": "pop_kpi", @@ -272,6 +336,61 @@ "time_compare": "inherit", "comparison_type": "values", } +params_v2_other_than_custom_false: dict[str, Any] = { + "datasource": "2__table", + "viz_type": "pop_kpi", + "metric": { + "expressionType": "SIMPLE", + "column": { + "advanced_data_type": None, + "certification_details": None, + "certified_by": None, + "column_name": "num_boys", + "description": None, + "expression": None, + "filterable": True, + "groupby": True, + "id": 334, + "is_certified": False, + "is_dttm": False, + "python_date_format": None, + "type": "BIGINT", + "type_generic": 0, + "verbose_name": None, + "warning_markdown": None, + }, + "aggregate": "SUM", + "sqlExpression": None, + "datasourceWarning": False, + "hasCustomLabel": False, + "label": "SUM(num_boys)", + "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", + }, + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "ds", + "operator": "TEMPORAL_RANGE", + "comparator": "1984 : 2000", + "clause": "WHERE", + "sqlExpression": None, + "isExtra": False, + "isNew": False, + "datasourceWarning": False, + "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", + } + ], + "row_limit": 10000, + "y_axis_format": "SMART_NUMBER", + "percentDifferenceFormat": "SMART_NUMBER", + "header_font_size": 0.2, + "subheader_font_size": 0.125, + "comparison_color_scheme": "Green", + "extra_form_data": {}, + "dashboards": [], + "time_compare": "", + "comparison_type": "values", +} def test_upgrade_chart_params_with_custom(): @@ -313,3 +432,22 @@ def test_downgrade_chart_params_other_than_custom(): original_params = deepcopy(params_v2_other_than_custom) downgraded_params = downgrade_comparison_params(original_params) assert downgraded_params == params_v1_other_than_custom + + +def test_upgrade_chart_params_other_than_custom_false(): + """ + ensure that the new time comparison params are added + """ + original_params = deepcopy(params_v1_other_than_custom_false) + upgraded_params = upgrade_comparison_params(original_params) + assert upgraded_params == params_v2_other_than_custom_false + + +def test_downgrade_chart_params_other_than_custom_false(): + """ + ensure that the params downgrade operation produces an almost identical dict + as the original value + """ + original_params = deepcopy(params_v2_other_than_custom_false) + downgraded_params = downgrade_comparison_params(original_params) + assert downgraded_params == params_v1_other_than_custom_false From 46c2af444d370545ba056f6eb87c9fc991772813 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 27 Aug 2024 18:56:50 +0200 Subject: [PATCH 2/4] - time_compare is an array not a string in the new version of the controls --- ...2_f84fde59123a_update_charts_with_old_time_comparison.py | 6 ++++-- ...de59123a_update_charts_with_old_time_comparison__test.py | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py index 0d592118de5b6..2116ac4ca1b8d 100644 --- a/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py +++ b/superset/migrations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py @@ -69,9 +69,9 @@ def upgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: if "time_comparison" in params: time_comp = params.pop("time_comparison") params["time_compare"] = ( - time_map.get(time_comp, "inherit") + [time_map.get(time_comp, "inherit")] if "enable_time_comparison" in params and params["enable_time_comparison"] - else "" + else [] ) if "enable_time_comparison" in params: @@ -129,6 +129,8 @@ def downgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]: # Revert time_compare to time_comparison if "time_compare" in params: time_comp = params.pop("time_compare") + # Max one element in the time_compare list + time_comp = time_comp[0] if time_comp else "" params["time_comparison"] = reverse_time_map.get(time_comp, "r") # If the chart was using any time compare, enable time comparison if time_comp: diff --git a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py index 224297ccee8f6..7a7a6284f3633 100644 --- a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py +++ b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py @@ -277,7 +277,7 @@ "comparison_color_scheme": "Green", "extra_form_data": {}, "dashboards": [], - "time_compare": "custom", + "time_compare": ["custom"], "comparison_type": "values", "start_date_offset": "1981-01-01", } @@ -333,7 +333,7 @@ "comparison_color_scheme": "Green", "extra_form_data": {}, "dashboards": [], - "time_compare": "inherit", + "time_compare": ["inherit"], "comparison_type": "values", } params_v2_other_than_custom_false: dict[str, Any] = { @@ -388,7 +388,7 @@ "comparison_color_scheme": "Green", "extra_form_data": {}, "dashboards": [], - "time_compare": "", + "time_compare": [], "comparison_type": "values", } From 6af8275d5c66611ad99431936cd3696899f4843d Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 27 Aug 2024 20:21:56 +0200 Subject: [PATCH 3/4] - Address comments on test readability by extracting common params --- ...e_charts_with_old_time_comparison__test.py | 298 +++--------------- 1 file changed, 38 insertions(+), 260 deletions(-) diff --git a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py index 7a7a6284f3633..f79f00f27657b 100644 --- a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py +++ b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py @@ -29,7 +29,8 @@ migrate_time_comparison_to_new_format.upgrade_comparison_params ) -params_v1_with_custom: dict[str, Any] = { +# Base object containing common properties +base_params: dict[str, Any] = { "datasource": "2__table", "viz_type": "pop_kpi", "metric": { @@ -57,20 +58,18 @@ "datasourceWarning": False, "hasCustomLabel": False, "label": "SUM(num_boys)", - "optionName": "metric_o6rj1h6jty_3t6mrruogfv", }, "adhoc_filters": [ { "expressionType": "SIMPLE", "subject": "ds", "operator": "TEMPORAL_RANGE", - "comparator": "1984 : 1986", + "comparator": "1984 : 2000", "clause": "WHERE", "sqlExpression": None, "isExtra": False, "isNew": False, "datasourceWarning": False, - "filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8", } ], "row_limit": 10000, @@ -81,6 +80,22 @@ "comparison_color_scheme": "Green", "extra_form_data": {}, "dashboards": [], +} + +# Specific parameter objects overriding only the differing properties +params_v1_with_custom = { + **base_params, + "metric": { + **base_params["metric"], + "optionName": "metric_o6rj1h6jty_3t6mrruogfv", + }, + "adhoc_filters": [ + { + **base_params["adhoc_filters"][0], + "comparator": "1984 : 1986", + "filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8", + } + ], "time_comparison": "c", "enable_time_comparison": True, "adhoc_custom": [ @@ -97,58 +112,13 @@ } ], } -params_v1_other_than_custom: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", + +params_v1_other_than_custom = { + **base_params, "metric": { - "expressionType": "SIMPLE", - "column": { - "advanced_data_type": None, - "certification_details": None, - "certified_by": None, - "column_name": "num_boys", - "description": None, - "expression": None, - "filterable": True, - "groupby": True, - "id": 334, - "is_certified": False, - "is_dttm": False, - "python_date_format": None, - "type": "BIGINT", - "type_generic": 0, - "verbose_name": None, - "warning_markdown": None, - }, - "aggregate": "SUM", - "sqlExpression": None, - "datasourceWarning": False, - "hasCustomLabel": False, - "label": "SUM(num_boys)", + **base_params["metric"], "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", }, - "adhoc_filters": [ - { - "expressionType": "SIMPLE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", - "comparator": "1984 : 2000", - "clause": "WHERE", - "sqlExpression": None, - "isExtra": False, - "isNew": False, - "datasourceWarning": False, - "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", - } - ], - "row_limit": 10000, - "y_axis_format": "SMART_NUMBER", - "percentDifferenceFormat": "SMART_NUMBER", - "header_font_size": 0.2, - "subheader_font_size": 0.125, - "comparison_color_scheme": "Green", - "extra_form_data": {}, - "dashboards": [], "time_comparison": "r", "enable_time_comparison": True, "adhoc_custom": [ @@ -161,235 +131,43 @@ } ], } -params_v1_other_than_custom_false: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", - "metric": { - "expressionType": "SIMPLE", - "column": { - "advanced_data_type": None, - "certification_details": None, - "certified_by": None, - "column_name": "num_boys", - "description": None, - "expression": None, - "filterable": True, - "groupby": True, - "id": 334, - "is_certified": False, - "is_dttm": False, - "python_date_format": None, - "type": "BIGINT", - "type_generic": 0, - "verbose_name": None, - "warning_markdown": None, - }, - "aggregate": "SUM", - "sqlExpression": None, - "datasourceWarning": False, - "hasCustomLabel": False, - "label": "SUM(num_boys)", - "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", - }, - "adhoc_filters": [ - { - "expressionType": "SIMPLE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", - "comparator": "1984 : 2000", - "clause": "WHERE", - "sqlExpression": None, - "isExtra": False, - "isNew": False, - "datasourceWarning": False, - "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", - } - ], - "row_limit": 10000, - "y_axis_format": "SMART_NUMBER", - "percentDifferenceFormat": "SMART_NUMBER", - "header_font_size": 0.2, - "subheader_font_size": 0.125, - "comparison_color_scheme": "Green", - "extra_form_data": {}, - "dashboards": [], - "time_comparison": "r", + +params_v1_other_than_custom_false = { + **params_v1_other_than_custom, "enable_time_comparison": False, - "adhoc_custom": [ - { - "clause": "WHERE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", - "comparator": "No filter", - "expressionType": "SIMPLE", - } - ], } -params_v2_with_custom: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", + +params_v2_with_custom = { + **base_params, "metric": { - "expressionType": "SIMPLE", - "column": { - "advanced_data_type": None, - "certification_details": None, - "certified_by": None, - "column_name": "num_boys", - "description": None, - "expression": None, - "filterable": True, - "groupby": True, - "id": 334, - "is_certified": False, - "is_dttm": False, - "python_date_format": None, - "type": "BIGINT", - "type_generic": 0, - "verbose_name": None, - "warning_markdown": None, - }, - "aggregate": "SUM", - "sqlExpression": None, - "datasourceWarning": False, - "hasCustomLabel": False, - "label": "SUM(num_boys)", + **base_params["metric"], "optionName": "metric_o6rj1h6jty_3t6mrruogfv", }, "adhoc_filters": [ { - "expressionType": "SIMPLE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", + **base_params["adhoc_filters"][0], "comparator": "1984 : 1986", - "clause": "WHERE", - "sqlExpression": None, - "isExtra": False, - "isNew": False, - "datasourceWarning": False, "filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8", } ], - "row_limit": 10000, - "y_axis_format": "SMART_NUMBER", - "percentDifferenceFormat": "SMART_NUMBER", - "header_font_size": 0.2, - "subheader_font_size": 0.125, - "comparison_color_scheme": "Green", - "extra_form_data": {}, - "dashboards": [], "time_compare": ["custom"], "comparison_type": "values", "start_date_offset": "1981-01-01", } -params_v2_other_than_custom: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", + +params_v2_other_than_custom = { + **base_params, "metric": { - "expressionType": "SIMPLE", - "column": { - "advanced_data_type": None, - "certification_details": None, - "certified_by": None, - "column_name": "num_boys", - "description": None, - "expression": None, - "filterable": True, - "groupby": True, - "id": 334, - "is_certified": False, - "is_dttm": False, - "python_date_format": None, - "type": "BIGINT", - "type_generic": 0, - "verbose_name": None, - "warning_markdown": None, - }, - "aggregate": "SUM", - "sqlExpression": None, - "datasourceWarning": False, - "hasCustomLabel": False, - "label": "SUM(num_boys)", + **base_params["metric"], "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", }, - "adhoc_filters": [ - { - "expressionType": "SIMPLE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", - "comparator": "1984 : 2000", - "clause": "WHERE", - "sqlExpression": None, - "isExtra": False, - "isNew": False, - "datasourceWarning": False, - "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", - } - ], - "row_limit": 10000, - "y_axis_format": "SMART_NUMBER", - "percentDifferenceFormat": "SMART_NUMBER", - "header_font_size": 0.2, - "subheader_font_size": 0.125, - "comparison_color_scheme": "Green", - "extra_form_data": {}, - "dashboards": [], "time_compare": ["inherit"], "comparison_type": "values", } -params_v2_other_than_custom_false: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", - "metric": { - "expressionType": "SIMPLE", - "column": { - "advanced_data_type": None, - "certification_details": None, - "certified_by": None, - "column_name": "num_boys", - "description": None, - "expression": None, - "filterable": True, - "groupby": True, - "id": 334, - "is_certified": False, - "is_dttm": False, - "python_date_format": None, - "type": "BIGINT", - "type_generic": 0, - "verbose_name": None, - "warning_markdown": None, - }, - "aggregate": "SUM", - "sqlExpression": None, - "datasourceWarning": False, - "hasCustomLabel": False, - "label": "SUM(num_boys)", - "optionName": "metric_96s7b8iypsr_4wrlgm0i7il", - }, - "adhoc_filters": [ - { - "expressionType": "SIMPLE", - "subject": "ds", - "operator": "TEMPORAL_RANGE", - "comparator": "1984 : 2000", - "clause": "WHERE", - "sqlExpression": None, - "isExtra": False, - "isNew": False, - "datasourceWarning": False, - "filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6", - } - ], - "row_limit": 10000, - "y_axis_format": "SMART_NUMBER", - "percentDifferenceFormat": "SMART_NUMBER", - "header_font_size": 0.2, - "subheader_font_size": 0.125, - "comparison_color_scheme": "Green", - "extra_form_data": {}, - "dashboards": [], + +params_v2_other_than_custom_false = { + **params_v2_other_than_custom, "time_compare": [], - "comparison_type": "values", } From e349e2863bce4050c1d269755f23ccd20dd7fe05 Mon Sep 17 00:00:00 2001 From: Antonio Rivero Date: Tue, 27 Aug 2024 20:33:13 +0200 Subject: [PATCH 4/4] - Add typing to fix mypy errors --- ...a_update_charts_with_old_time_comparison__test.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py index f79f00f27657b..cec6a636c56b5 100644 --- a/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py +++ b/tests/integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py @@ -83,7 +83,7 @@ } # Specific parameter objects overriding only the differing properties -params_v1_with_custom = { +params_v1_with_custom: dict[str, Any] = { **base_params, "metric": { **base_params["metric"], @@ -113,7 +113,7 @@ ], } -params_v1_other_than_custom = { +params_v1_other_than_custom: dict[str, Any] = { **base_params, "metric": { **base_params["metric"], @@ -132,12 +132,12 @@ ], } -params_v1_other_than_custom_false = { +params_v1_other_than_custom_false: dict[str, Any] = { **params_v1_other_than_custom, "enable_time_comparison": False, } -params_v2_with_custom = { +params_v2_with_custom: dict[str, Any] = { **base_params, "metric": { **base_params["metric"], @@ -155,7 +155,7 @@ "start_date_offset": "1981-01-01", } -params_v2_other_than_custom = { +params_v2_other_than_custom: dict[str, Any] = { **base_params, "metric": { **base_params["metric"], @@ -165,7 +165,7 @@ "comparison_type": "values", } -params_v2_other_than_custom_false = { +params_v2_other_than_custom_false: dict[str, Any] = { **params_v2_other_than_custom, "time_compare": [], }