From 41b991188c538c9729c235afdeaafc33d5c8b6ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Oberm=C3=BCller?= Date: Fri, 13 Dec 2024 12:45:51 +0100 Subject: [PATCH] refactor(dashboards): convert dashboard templates from filters to queries (#26389) --- .../dashboard/newDashboardLogic.test.tsx | 135 +++++++++++++++--- .../src/scenes/dashboard/newDashboardLogic.ts | 43 +++++- mypy-baseline.txt | 3 - .../legacy_compatibility/filter_to_query.py | 135 ++++++++++++++---- .../test/test_filter_to_query.py | 35 +++++ ..._convert_dashboard_templates_to_queries.py | 31 ++++ posthog/migrations/max_migration.txt | 2 +- 7 files changed, 329 insertions(+), 55 deletions(-) create mode 100644 posthog/migrations/0530_convert_dashboard_templates_to_queries.py diff --git a/frontend/src/scenes/dashboard/newDashboardLogic.test.tsx b/frontend/src/scenes/dashboard/newDashboardLogic.test.tsx index 03757f853741f..e694c94ea1ad0 100644 --- a/frontend/src/scenes/dashboard/newDashboardLogic.test.tsx +++ b/frontend/src/scenes/dashboard/newDashboardLogic.test.tsx @@ -1,36 +1,46 @@ +import { NodeKind } from '~/queries/schema' + import { applyTemplate } from './newDashboardLogic' describe('template function in newDashboardLogic', () => { it('ignores unused variables', () => { expect( - applyTemplate({ a: 'hello', b: 'hi' }, [ - { - id: 'VARIABLE_1', - name: 'a', - default: { - event: '$pageview', + applyTemplate( + { a: 'hello', b: 'hi' }, + [ + { + id: 'VARIABLE_1', + name: 'a', + default: { + event: '$pageview', + }, + description: 'The description of the variable', + required: true, + type: 'event', }, - description: 'The description of the variable', - required: true, - type: 'event', - }, - ]) + ], + null + ) ).toEqual({ a: 'hello', b: 'hi' }) }) it('uses identified variables', () => { expect( - applyTemplate({ a: '{VARIABLE_1}', b: 'hi' }, [ - { - id: 'VARIABLE_1', - name: 'a', - default: { - event: '$pageview', + applyTemplate( + { a: '{VARIABLE_1}', b: 'hi' }, + [ + { + id: 'VARIABLE_1', + name: 'a', + default: { + event: '$pageview', + }, + description: 'The description of the variable', + required: true, + type: 'event', }, - description: 'The description of the variable', - required: true, - type: 'event', - }, - ]) + ], + null + ) ).toEqual({ a: { event: '$pageview', @@ -38,4 +48,85 @@ describe('template function in newDashboardLogic', () => { b: 'hi', }) }) + + it('replaces variables in query based tiles', () => { + expect( + applyTemplate( + { a: '{VARIABLE_1}' }, + [ + { + id: 'VARIABLE_1', + name: 'a', + default: { + id: '$pageview', + }, + description: 'The description of the variable', + required: true, + type: 'event', + }, + ], + NodeKind.TrendsQuery + ) + ).toEqual({ + a: { + event: '$pageview', + kind: 'EventsNode', + math: 'total', + }, + }) + }) + + it("removes the math property from query based tiles that don't support it", () => { + expect( + applyTemplate( + { a: '{VARIABLE_1}' }, + [ + { + id: 'VARIABLE_1', + name: 'a', + default: { + id: '$pageview', + }, + description: 'The description of the variable', + required: true, + type: 'event', + }, + ], + NodeKind.LifecycleQuery + ) + ).toEqual({ + a: { + event: '$pageview', + kind: 'EventsNode', + }, + }) + }) + + it('removes the math property from retention insight tiles', () => { + expect( + applyTemplate( + { a: '{VARIABLE_1}' }, + [ + { + id: 'VARIABLE_1', + name: 'a', + default: { + id: '$pageview', + math: 'dau' as any, + type: 'events' as any, + }, + description: 'The description of the variable', + required: true, + type: 'event', + }, + ], + NodeKind.RetentionQuery + ) + ).toEqual({ + a: { + id: '$pageview', + type: 'events', + }, + }) + }) }) diff --git a/frontend/src/scenes/dashboard/newDashboardLogic.ts b/frontend/src/scenes/dashboard/newDashboardLogic.ts index 6749067872258..564a24f736c1f 100644 --- a/frontend/src/scenes/dashboard/newDashboardLogic.ts +++ b/frontend/src/scenes/dashboard/newDashboardLogic.ts @@ -5,11 +5,15 @@ import api from 'lib/api' import { DashboardRestrictionLevel } from 'lib/constants' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' import { featureFlagLogic } from 'lib/logic/featureFlagLogic' +import { MathAvailability } from 'scenes/insights/filters/ActionFilter/ActionFilterRow/ActionFilterRow' import { teamLogic } from 'scenes/teamLogic' import { urls } from 'scenes/urls' import { dashboardsModel } from '~/models/dashboardsModel' +import { legacyEntityToNode, sanitizeRetentionEntity } from '~/queries/nodes/InsightQuery/utils/filtersToQueryNode' import { getQueryBasedDashboard } from '~/queries/nodes/InsightViz/utils' +import { NodeKind } from '~/queries/schema' +import { isInsightVizNode } from '~/queries/utils' import { DashboardTemplateType, DashboardTemplateVariableType, DashboardTile, DashboardType, JsonType } from '~/types' import type { newDashboardLogicType } from './newDashboardLogicType' @@ -35,24 +39,47 @@ export interface NewDashboardLogicProps { } // Currently this is a very generic recursive function incase we want to add template variables to aspects beyond events -export function applyTemplate(obj: DashboardTile | JsonType, variables: DashboardTemplateVariableType[]): JsonType { +export function applyTemplate( + obj: DashboardTile | JsonType, + variables: DashboardTemplateVariableType[], + queryKind: NodeKind | null +): JsonType { if (typeof obj === 'string') { if (obj.startsWith('{') && obj.endsWith('}')) { const variableId = obj.substring(1, obj.length - 1) const variable = variables.find((variable) => variable.id === variableId) if (variable && variable.default) { + // added for future compatibility - at the moment we only have event variables + const isEventVariable = variable.type === 'event' + + if (queryKind && isEventVariable) { + let mathAvailability = MathAvailability.None + if (queryKind === NodeKind.TrendsQuery) { + mathAvailability = MathAvailability.All + } else if (queryKind === NodeKind.StickinessQuery) { + mathAvailability = MathAvailability.ActorsOnly + } else if (queryKind === NodeKind.FunnelsQuery) { + mathAvailability = MathAvailability.FunnelsOnly + } + return ( + queryKind === NodeKind.RetentionQuery + ? sanitizeRetentionEntity(variable.default as any) + : legacyEntityToNode(variable.default as any, true, mathAvailability) + ) as JsonType + } + return variable.default as JsonType } return obj } } if (Array.isArray(obj)) { - return obj.map((item) => applyTemplate(item, variables)) + return obj.map((item) => applyTemplate(item, variables, queryKind)) } if (typeof obj === 'object' && obj !== null) { const newObject: JsonType = {} for (const [key, value] of Object.entries(obj)) { - newObject[key] = applyTemplate(value, variables) + newObject[key] = applyTemplate(value, variables, queryKind) } return newObject } @@ -60,7 +87,15 @@ export function applyTemplate(obj: DashboardTile | JsonType, variables: Dashboar } function makeTilesUsingVariables(tiles: DashboardTile[], variables: DashboardTemplateVariableType[]): JsonType[] { - return tiles.map((tile: DashboardTile) => applyTemplate(tile, variables)) + return tiles.map((tile: DashboardTile) => { + const isQueryBased = 'query' in tile && tile.query != null + const queryKind: NodeKind | null = isQueryBased + ? isInsightVizNode(tile.query as any) + ? (tile.query as any)?.source.kind + : (tile.query as any)?.kind + : null + return applyTemplate(tile, variables, queryKind) + }) } export const newDashboardLogic = kea([ diff --git a/mypy-baseline.txt b/mypy-baseline.txt index b23b377c86a6c..ca4b578d231b4 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -143,9 +143,6 @@ posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Item "No posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Argument 1 to "float" has incompatible type "Any | None"; expected "str | Buffer | SupportsFloat | SupportsIndex" [arg-type] posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Argument 1 to "clean_display" has incompatible type "Any | None"; expected "str" [arg-type] posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "FunnelsFilter"; expected "str": "TrendsFilter" [dict-item] -posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "RetentionFilter"; expected "str": "TrendsFilter" [dict-item] -posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Argument 1 to "to_base_entity_dict" has incompatible type "Any | None"; expected "dict[Any, Any]" [arg-type] -posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Argument 1 to "to_base_entity_dict" has incompatible type "Any | None"; expected "dict[Any, Any]" [arg-type] posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "PathsFilter"; expected "str": "TrendsFilter" [dict-item] posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "LifecycleFilter"; expected "str": "TrendsFilter" [dict-item] posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "StickinessFilter"; expected "str": "TrendsFilter" [dict-item] diff --git a/posthog/hogql_queries/legacy_compatibility/filter_to_query.py b/posthog/hogql_queries/legacy_compatibility/filter_to_query.py index a8cc01a0b89fb..3439983291d68 100644 --- a/posthog/hogql_queries/legacy_compatibility/filter_to_query.py +++ b/posthog/hogql_queries/legacy_compatibility/filter_to_query.py @@ -2,7 +2,9 @@ import json import re from enum import StrEnum -from typing import Any, Literal +from typing import Any, Literal, Optional, Union, cast + +from pydantic import Field from posthog.hogql_queries.legacy_compatibility.clean_properties import clean_entity_properties, clean_global_properties from posthog.models.entity.entity import Entity as LegacyEntity @@ -25,6 +27,7 @@ LifecycleQuery, PathsFilter, PathsQuery, + RetentionEntity, RetentionFilter, RetentionQuery, StickinessFilter, @@ -56,6 +59,10 @@ class MathAvailability(StrEnum): ] +def is_entity_variable(item: Any) -> bool: + return isinstance(item, str) and item.startswith("{") and item.endswith("}") + + def clean_display(display: str): if display not in [c.value for c in ChartDisplayType]: return None @@ -110,8 +117,16 @@ def transform_legacy_hidden_legend_keys(hidden_legend_keys): def legacy_entity_to_node( - entity: LegacyEntity, include_properties: bool, math_availability: MathAvailability -) -> EventsNode | ActionsNode | DataWarehouseNode: + entity: LegacyEntity | str, + include_properties: bool, + math_availability: MathAvailability, + allow_variables: bool = False, +) -> EventsNode | ActionsNode | DataWarehouseNode | str: + if allow_variables and is_entity_variable(entity): + return cast(str, entity) + + assert not isinstance(entity, str) + """ Takes a legacy entity and converts it into an EventsNode or ActionsNode. """ @@ -172,6 +187,7 @@ def exlusion_entity_to_node(entity) -> FunnelExclusionEventsNode | FunnelExclusi base_entity = legacy_entity_to_node( LegacyEntity(entity), include_properties=False, math_availability=MathAvailability.Unavailable ) + assert isinstance(base_entity, EventsNode | ActionsNode) if isinstance(base_entity, EventsNode): return FunnelExclusionEventsNode( **base_entity.model_dump(), @@ -187,7 +203,12 @@ def exlusion_entity_to_node(entity) -> FunnelExclusionEventsNode | FunnelExclusi # TODO: remove this method that returns legacy entities -def to_base_entity_dict(entity: dict): +def to_base_entity_dict(entity: dict | str): + if isinstance(entity, str): + if is_entity_variable(entity): + return entity + raise ValueError("Expecting valid entity or template variable") + return { "type": entity.get("type"), "id": entity.get("id"), @@ -206,6 +227,52 @@ def to_base_entity_dict(entity: dict): "STICKINESS": StickinessQuery, } + +class TrendsQueryWithTemplateVariables(TrendsQuery): + series: list[Union[EventsNode, ActionsNode, DataWarehouseNode, str]] = Field( # type: ignore + ..., description="Events and actions to include" + ) + + +class FunnelsQueryWithTemplateVariables(FunnelsQuery): + series: list[Union[EventsNode, ActionsNode, DataWarehouseNode, str]] = Field( # type: ignore + ..., description="Events and actions to include" + ) + + +class RetentionFilterWithTemplateVariables(RetentionFilter): + returningEntity: Optional[RetentionEntity | str] = None # type: ignore + targetEntity: Optional[RetentionEntity | str] = None # type: ignore + + +class RetentionQueryWithTemplateVariables(RetentionQuery): + retentionFilter: RetentionFilterWithTemplateVariables = Field( + ..., description="Properties specific to the retention insight" + ) + + +class LifecycleQueryWithTemplateVariables(LifecycleQuery): + series: list[Union[EventsNode, ActionsNode, DataWarehouseNode, str]] = Field( # type: ignore + ..., description="Events and actions to include" + ) + + +class StickinessQueryWithTemplateVariables(StickinessQuery): + series: list[Union[EventsNode, ActionsNode, DataWarehouseNode, str]] = Field( # type: ignore + ..., description="Events and actions to include" + ) + + +# +insight_to_query_type_with_variables = { + "TRENDS": TrendsQueryWithTemplateVariables, + "FUNNELS": FunnelsQueryWithTemplateVariables, + "RETENTION": RetentionQueryWithTemplateVariables, + "PATHS": PathsQuery, + "LIFECYCLE": LifecycleQueryWithTemplateVariables, + "STICKINESS": StickinessQueryWithTemplateVariables, +} + INSIGHT_TYPE = Literal["TRENDS", "FUNNELS", "RETENTION", "PATHS", "LIFECYCLE", "STICKINESS"] @@ -232,12 +299,12 @@ def _interval(filter: dict): return {"interval": filter.get("interval")} -def _series(filter: dict): +def _series(filter: dict, allow_variables: bool = False): if _insight_type(filter) == "RETENTION" or _insight_type(filter) == "PATHS": return {} # remove templates gone wrong - if filter.get("events") is not None: + if not allow_variables and filter.get("events") is not None: filter["events"] = [event for event in filter.get("events") if not (isinstance(event, str))] math_availability: MathAvailability = MathAvailability.Unavailable @@ -252,15 +319,16 @@ def _series(filter: dict): return { "series": [ - legacy_entity_to_node(entity, include_properties, math_availability) - for entity in _entities(filter) - if not (entity.type == "actions" and entity.id is None) + legacy_entity_to_node(entity, include_properties, math_availability, allow_variables) + for entity in _entities(filter, allow_variables) + if isinstance(entity, str) or not (entity.type == "actions" and entity.id is None) ] } -def _entities(filter: dict): - processed_entities: list[LegacyEntity] = [] +def _entities(filter: dict, allow_variables: bool = False): + processed_entities: list[LegacyEntity | str] = [] + has_variables = False # add actions actions = filter.get("actions", []) @@ -272,7 +340,18 @@ def _entities(filter: dict): events = filter.get("events", []) if isinstance(events, str): events = json.loads(events) - processed_entities.extend([LegacyEntity({**entity, "type": "events"}) for entity in events]) + + def process_event(entity) -> LegacyEntity | str: + nonlocal has_variables + + # strings represent template variables, return them as-is + if allow_variables and isinstance(entity, str): + has_variables = True + return entity + else: + return LegacyEntity({**entity, "type": "events"}) + + processed_entities.extend([process_event(entity) for entity in events]) # add data warehouse warehouse = filter.get("data_warehouse", []) @@ -280,12 +359,13 @@ def _entities(filter: dict): warehouse = json.loads(warehouse) processed_entities.extend([LegacyEntity({**entity, "type": "data_warehouse"}) for entity in warehouse]) - # order by order - processed_entities.sort(key=lambda entity: entity.order if entity.order else -1) + if not has_variables: + # order by order + processed_entities.sort(key=lambda entity: entity.order if entity.order else -1) # type: ignore - # set sequential index values on entities - for index, entity in enumerate(processed_entities): - entity.index = index + # set sequential index values on entities + for index, entity in enumerate(processed_entities): + entity.index = index # type: ignore return processed_entities @@ -394,7 +474,7 @@ def _group_aggregation_filter(filter: dict): return {"aggregation_group_type_index": filter.get("aggregation_group_type_index")} -def _insight_filter(filter: dict): +def _insight_filter(filter: dict, allow_variables: bool = False): if _insight_type(filter) == "TRENDS": insight_filter = { "trendsFilter": TrendsFilter( @@ -440,18 +520,19 @@ def _insight_filter(filter: dict): ), } elif _insight_type(filter) == "RETENTION": + RetentionFilterClass = RetentionFilterWithTemplateVariables if allow_variables else RetentionFilter insight_filter = { - "retentionFilter": RetentionFilter( + "retentionFilter": RetentionFilterClass( retentionType=filter.get("retention_type"), retentionReference=filter.get("retention_reference"), totalIntervals=filter.get("total_intervals"), returningEntity=( - to_base_entity_dict(filter.get("returning_entity")) + to_base_entity_dict(filter.get("returning_entity")) # type: ignore if filter.get("returning_entity") is not None else None ), targetEntity=( - to_base_entity_dict(filter.get("target_entity")) + to_base_entity_dict(filter.get("target_entity")) # type: ignore if filter.get("target_entity") is not None else None ), @@ -526,22 +607,26 @@ def _insight_type(filter: dict) -> INSIGHT_TYPE: return filter.get("insight", "TRENDS") -def filter_to_query(filter: dict) -> InsightQueryNode: +def filter_to_query(filter: dict, allow_variables: bool = False) -> InsightQueryNode: filter = copy.deepcopy(filter) # duplicate to prevent accidental filter alterations - Query = insight_to_query_type[_insight_type(filter)] + Query = ( + insight_to_query_type_with_variables[_insight_type(filter)] + if allow_variables + else insight_to_query_type[_insight_type(filter)] + ) data = { **_date_range(filter), **_interval(filter), - **_series(filter), + **_series(filter, allow_variables), **_sampling_factor(filter), **_filter_test_accounts(filter), **_properties(filter), **_breakdown_filter(filter), **_compare_filter(filter), **_group_aggregation_filter(filter), - **_insight_filter(filter), + **_insight_filter(filter, allow_variables), } # :KLUDGE: We do this dance to have default values instead of None, when setting diff --git a/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py b/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py index b0e9e6a6c4ec9..69407b235c05b 100644 --- a/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py +++ b/posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py @@ -1,3 +1,4 @@ +from typing import cast import pytest from posthog.hogql_queries.legacy_compatibility.filter_to_query import ( @@ -1820,3 +1821,37 @@ def test_ignores_digit_only_keys(self): indexes = hidden_legend_keys_to_breakdowns(hidden_legend_keys) self.assertEqual(indexes, ["Opera"]) + + +class TestDashboardTemplateConversion(BaseTest): + def test_trend_series_with_variables(self): + filter = { + "insight": "TRENDS", + "events": ["{VARIABLE}"], + } + + query = cast(TrendsQuery, filter_to_query(filter, allow_variables=True)) + + self.assertEqual(query.series, ["{VARIABLE}"]) + + def test_funnel_series_with_variables(self): + filter = { + "insight": "FUNNELS", + "events": ["{VARIABLE1}", "{VARIABLE2}"], + } + + query = cast(FunnelsQuery, filter_to_query(filter, allow_variables=True)) + + self.assertEqual(query.series, ["{VARIABLE1}", "{VARIABLE2}"]) + + def test_retention_entities_with_variables(self): + filter = { + "insight": "RETENTION", + "target_entity": "{VARIABLE1}", + "returning_entity": "{VARIABLE2}", + } + + query = cast(RetentionQuery, filter_to_query(filter, allow_variables=True)) + + self.assertEqual(query.retentionFilter.targetEntity, "{VARIABLE1}") + self.assertEqual(query.retentionFilter.returningEntity, "{VARIABLE2}") diff --git a/posthog/migrations/0530_convert_dashboard_templates_to_queries.py b/posthog/migrations/0530_convert_dashboard_templates_to_queries.py new file mode 100644 index 0000000000000..393032d32a1ca --- /dev/null +++ b/posthog/migrations/0530_convert_dashboard_templates_to_queries.py @@ -0,0 +1,31 @@ +# Generated by Django 4.2.15 on 2024-11-04 11:24 + +from django.db import migrations + +from posthog.hogql_queries.legacy_compatibility.filter_to_query import filter_to_query +from posthog.schema import InsightVizNode + + +def update_filters_to_queries(apps, schema_editor): + DashboardTemplate = apps.get_model("posthog", "DashboardTemplate") + + for template in DashboardTemplate.objects.all(): + for tile in template.tiles: + if "filters" in tile: + source = filter_to_query(tile["filters"], allow_variables=True) + query = InsightVizNode(source=source) + tile["query"] = query.model_dump(exclude_none=True) + del tile["filters"] + template.save() + + +def revert_queries_to_filters(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + dependencies = [("posthog", "0529_hog_function_mappings")] + + operations = [ + migrations.RunPython(update_filters_to_queries, revert_queries_to_filters), + ] diff --git a/posthog/migrations/max_migration.txt b/posthog/migrations/max_migration.txt index e0d6699d21ad6..3eb81911c811d 100644 --- a/posthog/migrations/max_migration.txt +++ b/posthog/migrations/max_migration.txt @@ -1 +1 @@ -0529_hog_function_mappings +0530_convert_dashboard_templates_to_queries