Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(migrations): Fix the time comparison migration #30029

Merged
merged 4 commits into from
Aug 27, 2024
Merged

fix(migrations): Fix the time comparison migration #30029

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c89ed4e
- FIx time comparison migration so charts that are not using comparis…
Antonio-RiveroMartnez Aug 27, 2024
46c2af4
- time_compare is an array not a string in the new version of the con…
Antonio-RiveroMartnez Aug 27, 2024
6af8275
- Address comments on test readability by extracting common params
Antonio-RiveroMartnez Aug 27, 2024
e349e28
- Add typing to fix mypy errors
Antonio-RiveroMartnez Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions ...grations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down
142 changes: 140 additions & 2 deletions ...integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,70 @@
}
],
}
params_v1_other_than_custom_false: dict[str, Any] = {
Copy link
Member

@michael-s-molina michael-s-molina Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind extracting the common parts between params_v1_with_custom, params_v1_other_than_custom_false, params_v2_with_custom, params_v2_other_than_custom, params_v2_other_than_custom_false and just override the necessary properties for each variable to avoid code duplication and increase readability?

"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",
Expand Down Expand Up @@ -213,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",
}
Expand Down Expand Up @@ -269,7 +333,62 @@
"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] = {
"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",
}

Expand Down Expand Up @@ -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
Loading