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..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 @@ -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,21 @@ 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 + # 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: + 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..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 @@ -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: dict[str, Any] = { + **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", + **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,118 +131,45 @@ } ], } + +params_v1_other_than_custom_false: dict[str, Any] = { + **params_v1_other_than_custom, + "enable_time_comparison": False, +} + params_v2_with_custom: dict[str, Any] = { - "datasource": "2__table", - "viz_type": "pop_kpi", + **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", + "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", + **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", + "time_compare": ["inherit"], "comparison_type": "values", } +params_v2_other_than_custom_false: dict[str, Any] = { + **params_v2_other_than_custom, + "time_compare": [], +} + def test_upgrade_chart_params_with_custom(): """ @@ -313,3 +210,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