From 070822519d8bf20e8d6cf0a4f911126057955fb3 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 10 Sep 2021 14:14:38 +0100 Subject: [PATCH 1/4] yeet --- .../queries/paths/path_event_query.py | 22 +---- ee/clickhouse/queries/paths/paths.py | 2 - ee/clickhouse/queries/test/test_paths.py | 25 ++---- ee/clickhouse/sql/paths/path.py | 12 +-- frontend/src/scenes/paths/Paths.js | 62 +++---------- frontend/src/scenes/paths/pathsLogic.ts | 8 +- posthog/models/filters/mixins/paths.py | 13 +-- posthog/queries/test/test_paths.py | 87 ------------------- 8 files changed, 24 insertions(+), 207 deletions(-) diff --git a/ee/clickhouse/queries/paths/path_event_query.py b/ee/clickhouse/queries/paths/path_event_query.py index 2eddc7494a1db..db31deee6234a 100644 --- a/ee/clickhouse/queries/paths/path_event_query.py +++ b/ee/clickhouse/queries/paths/path_event_query.py @@ -2,7 +2,7 @@ from ee.clickhouse.models.property import get_property_string_expr from ee.clickhouse.queries.event_query import ClickhouseEventQuery -from posthog.constants import AUTOCAPTURE_EVENT, PAGEVIEW_EVENT, SCREEN_EVENT +from posthog.constants import PAGEVIEW_EVENT, SCREEN_EVENT from posthog.models.filters.path_filter import PathFilter @@ -38,12 +38,7 @@ def get_query(self) -> Tuple[str, Dict[str, Any]]: if self._should_query_url() else "if(0, '', " ) - event_conditional += ( - f"if({self.EVENT_TABLE_ALIAS}.event = '{AUTOCAPTURE_EVENT}', concat('autocapture:', {self.EVENT_TABLE_ALIAS}.elements_chain), " - if self._should_query_elements_chain() - else "if(0, '', " - ) - event_conditional += f"{self.EVENT_TABLE_ALIAS}.event))) AS path_item" + event_conditional += f"{self.EVENT_TABLE_ALIAS}.event)) AS path_item" _fields.append(event_conditional) @@ -96,9 +91,6 @@ def _get_event_query(self) -> Tuple[str, Dict[str, Any]]: if self._filter.include_screenviews: or_conditions.append(f"event = '{SCREEN_EVENT}'") - if self._filter.include_autocaptures: - or_conditions.append(f"event = '{AUTOCAPTURE_EVENT}'") - if self._filter.include_all_custom_events: or_conditions.append(f"NOT event LIKE '$%%'") @@ -137,13 +129,3 @@ def _should_query_screen(self) -> bool: return True return False - - def _should_query_elements_chain(self) -> bool: - if ( - self._filter.target_events == [] and self._filter.custom_events == [] - ) and AUTOCAPTURE_EVENT not in self._filter.exclude_events: - return True - elif self._filter.include_autocaptures: - return True - - return False diff --git a/ee/clickhouse/queries/paths/paths.py b/ee/clickhouse/queries/paths/paths.py index 65e3f81aef0a3..4aca232159322 100644 --- a/ee/clickhouse/queries/paths/paths.py +++ b/ee/clickhouse/queries/paths/paths.py @@ -26,7 +26,6 @@ def __init__(self, filter: PathFilter, team: Team, funnel_filter: Optional[Filte "events": [], # purely a speed optimization, don't need this for filtering "event_in_session_limit": self._filter.step_limit or EVENT_IN_SESSION_LIMIT_DEFAULT, "session_time_threshold": SESSION_TIME_THRESHOLD_DEFAULT, - "autocapture_match": "%autocapture:%", } self._funnel_filter = funnel_filter @@ -34,7 +33,6 @@ def __init__(self, filter: PathFilter, team: Team, funnel_filter: Optional[Filte raise ValidationError("Cannot include all custom events and specific custom events in the same query") # TODO: don't allow including $pageview and excluding $pageview at the same time - # TODO: Filter on specific autocapture / page URLs def run(self, *args, **kwargs): diff --git a/ee/clickhouse/queries/test/test_paths.py b/ee/clickhouse/queries/test/test_paths.py index b5162d0efd904..b86ba53a76573 100644 --- a/ee/clickhouse/queries/test/test_paths.py +++ b/ee/clickhouse/queries/test/test_paths.py @@ -895,28 +895,24 @@ def test_paths_start_and_end(self): def test_properties_queried_using_path_filter(self): def should_query_list(filter) -> Tuple[bool, bool, bool]: path_query = PathEventQuery(filter, self.team.id) - return ( - path_query._should_query_url(), - path_query._should_query_screen(), - path_query._should_query_elements_chain(), - ) + return (path_query._should_query_url(), path_query._should_query_screen()) filter = PathFilter() - self.assertEqual(should_query_list(filter), (True, True, True)) + self.assertEqual(should_query_list(filter), (True, True)) filter = PathFilter({"include_event_types": ["$pageview"]}) - self.assertEqual(should_query_list(filter), (True, False, False)) + self.assertEqual(should_query_list(filter), (True, False)) filter = PathFilter({"include_event_types": ["$screen"]}) - self.assertEqual(should_query_list(filter), (False, True, False)) + self.assertEqual(should_query_list(filter), (False, True)) filter = filter.with_data({"include_event_types": [], "include_custom_events": ["/custom1", "/custom2"]}) - self.assertEqual(should_query_list(filter), (False, False, False)) + self.assertEqual(should_query_list(filter), (False, False)) filter = filter.with_data( {"include_event_types": ["$pageview", "$screen", "custom_event"], "include_custom_events": []} ) - self.assertEqual(should_query_list(filter), (True, True, False)) + self.assertEqual(should_query_list(filter), (True, True)) filter = filter.with_data( { @@ -925,14 +921,9 @@ def should_query_list(filter) -> Tuple[bool, bool, bool]: "exclude_events": ["/custom1"], } ) - self.assertEqual(should_query_list(filter), (True, True, False)) + self.assertEqual(should_query_list(filter), (True, True)) filter = filter.with_data( {"include_event_types": [], "include_custom_events": [], "exclude_events": ["$pageview"],} ) - self.assertEqual(should_query_list(filter), (False, True, True)) - - filter = filter.with_data( - {"include_event_types": [], "include_custom_events": [], "exclude_events": ["$autocapture"],} - ) - self.assertEqual(should_query_list(filter), (True, True, False)) + self.assertEqual(should_query_list(filter), (False, True)) diff --git a/ee/clickhouse/sql/paths/path.py b/ee/clickhouse/sql/paths/path.py index 246ac308c2869..57546b0f1a8a6 100644 --- a/ee/clickhouse/sql/paths/path.py +++ b/ee/clickhouse/sql/paths/path.py @@ -137,16 +137,7 @@ PATH_ARRAY_QUERY = """ -SELECT if(target_event LIKE %(autocapture_match)s, concat(arrayElement(splitByString('autocapture:', assumeNotNull(source_event)), 1), final_source_element), source_event) final_source_event, - if(target_event LIKE %(autocapture_match)s, concat(arrayElement(splitByString('autocapture:', assumeNotNull(target_event)), 1), final_target_element), target_event) final_target_event, - event_count, - average_conversion_time, - if(target_event LIKE %(autocapture_match)s, arrayElement(splitByString('autocapture:', assumeNotNull(source_event)), 2), NULL) source_event_elements_chain, - concat('<', extract(source_event_elements_chain, '^(.*?)[.|:]'), '> ', extract(source_event_elements_chain, 'text="(.*?)"')) final_source_element, - if(target_event LIKE %(autocapture_match)s, arrayElement(splitByString('autocapture:', assumeNotNull(target_event)), 2), NULL) target_event_elements_chain, - concat('<', extract(target_event_elements_chain, '^(.*?)[.|:]'), '> ', extract(target_event_elements_chain, 'text="(.*?)"')) final_target_element -FROM ( - SELECT last_path_key as source_event, +SELECT last_path_key as source_event, path_key as target_event, COUNT(*) AS event_count, avg(conversion_time) AS average_conversion_time @@ -201,6 +192,5 @@ source_event, target_event LIMIT 20 -) """ diff --git a/frontend/src/scenes/paths/Paths.js b/frontend/src/scenes/paths/Paths.js index aca6f702c773b..7acaeb97e8b8f 100644 --- a/frontend/src/scenes/paths/Paths.js +++ b/frontend/src/scenes/paths/Paths.js @@ -1,9 +1,6 @@ -import React, { useRef, useState, useEffect } from 'react' -import api from 'lib/api' +import React, { useRef, useEffect } from 'react' import { useValues } from 'kea' import { stripHTTP } from 'lib/utils' -import { Modal, Button, Spin } from 'antd' -import { EventElements } from 'scenes/events/EventElements' import * as d3 from 'd3' import * as Sankey from 'd3-sankey' import { pathsLogic } from 'scenes/paths/pathsLogic' @@ -11,7 +8,6 @@ import { useWindowSize } from 'lib/hooks/useWindowSize' // TODO: Replace with PathType enums when moving to TypeScript const PAGEVIEW = '$pageview' -const AUTOCAPTURE = '$autocapture' function rounded_rect(x, y, w, h, r, tl, tr, bl, br) { var retval @@ -95,9 +91,6 @@ export function Paths({ dashboardItemId = null, filters = null, color = 'white' const size = useWindowSize() const { paths, loadedFilter, resultsLoading: pathsLoading } = useValues(pathsLogic({ dashboardItemId, filters })) - const [modalVisible, setModalVisible] = useState(false) - const [event, setEvent] = useState(null) - useEffect(() => { renderPaths() }, [paths, !pathsLoading, size, color]) @@ -246,15 +239,7 @@ export function Paths({ dashboardItemId = null, filters = null, color = 'white' .attr('text-anchor', (d) => (d.x0 < width / 2 ? 'start' : 'end')) .attr('display', (d) => (d.value > 0 ? 'inherit' : 'none')) .text(loadedFilter?.path_type === PAGEVIEW ? pageUrl : pathText) - .on('click', async (node) => { - if (loadedFilter.path_type === AUTOCAPTURE) { - setModalVisible(true) - setEvent(null) - let result = await api.get('api/event/' + node.id) - setEvent(result) - } - }) - .style('cursor', loadedFilter.path_type === AUTOCAPTURE ? 'pointer' : 'auto') + .style('cursor', 'auto') .style('fill', color === 'white' ? '#000' : '#fff') textSelection @@ -268,42 +253,15 @@ export function Paths({ dashboardItemId = null, filters = null, color = 'white' } return ( -
- {loadedFilter.path_type === AUTOCAPTURE && ( -
Click on a tag to see related DOM tree
- )} -
-
- {!pathsLoading && paths && paths.nodes.length === 0 && !paths.error && } -
+
+
+ {!pathsLoading && paths && paths.nodes.length === 0 && !paths.error && }
- setModalVisible(false)} - onCancel={() => setModalVisible(false)} - closable={false} - style={{ minWidth: '50%' }} - footer={[ - , - ]} - bodyStyle={ - !event - ? { - alignItems: 'center', - justifyContent: 'center', - } - : {} - } - > - {event ? : } -
) } diff --git a/frontend/src/scenes/paths/pathsLogic.ts b/frontend/src/scenes/paths/pathsLogic.ts index 884adacddcddc..f83f67810434e 100644 --- a/frontend/src/scenes/paths/pathsLogic.ts +++ b/frontend/src/scenes/paths/pathsLogic.ts @@ -15,14 +15,12 @@ import { dashboardsModel } from '~/models/dashboardsModel' export const pathOptionsToLabels = { [PathType.PageView]: 'Page views (Web)', [PathType.Screen]: 'Screen views (Mobile)', - [PathType.AutoCapture]: 'Autocaptured events', [PathType.CustomEvent]: 'Custom events', } export const pathOptionsToProperty = { [PathType.PageView]: '$current_url', [PathType.Screen]: '$screen_name', - [PathType.AutoCapture]: 'autocaptured_event', [PathType.CustomEvent]: 'custom_event', } @@ -47,9 +45,7 @@ interface PathResult { interface PathNode { target: string - target_id: number source: string - source_id: number value: number } @@ -147,10 +143,10 @@ export const pathsLogic = kea>({ const nodes: Record = {} for (const path of paths) { if (!nodes[path.source]) { - nodes[path.source] = { name: path.source, id: path.source_id } + nodes[path.source] = { name: path.source } } if (!nodes[path.target]) { - nodes[path.target] = { name: path.target, id: path.target_id } + nodes[path.target] = { name: path.target } } } diff --git a/posthog/models/filters/mixins/paths.py b/posthog/models/filters/mixins/paths.py index 44b6baf37c797..b4f7ac037b46b 100644 --- a/posthog/models/filters/mixins/paths.py +++ b/posthog/models/filters/mixins/paths.py @@ -1,7 +1,6 @@ from typing import Dict, List, Literal, Optional, Tuple, cast from posthog.constants import ( - AUTOCAPTURE_EVENT, CUSTOM_EVENT, END_POINT, FUNNEL_PATHS, @@ -17,7 +16,7 @@ from posthog.models.filters.mixins.common import BaseParamMixin from posthog.models.filters.mixins.utils import cached_property, include_dict, process_bool -PathType = Literal["$pageview", "$autocapture", "$screen", "custom_event"] +PathType = Literal["$pageview", "$screen", "custom_event"] class PathTypeMixin(BaseParamMixin): @@ -55,8 +54,6 @@ class PropTypeDerivedMixin(PathTypeMixin): def prop_type(self) -> str: if self.path_type == SCREEN_EVENT: return "properties->> '$screen_name'" - elif self.path_type == AUTOCAPTURE_EVENT: - return "tag_name_source" elif self.path_type == CUSTOM_EVENT: return "event" else: @@ -68,8 +65,6 @@ class ComparatorDerivedMixin(PropTypeDerivedMixin): def comparator(self) -> str: if self.path_type == SCREEN_EVENT: return "{} =".format(self.prop_type) - elif self.path_type == AUTOCAPTURE_EVENT: - return "group_id =" elif self.path_type == CUSTOM_EVENT: return "event =" else: @@ -81,8 +76,6 @@ class TargetEventDerivedMixin(PropTypeDerivedMixin): def target_event(self) -> Tuple[Optional[PathType], Dict[str, str]]: if self.path_type == SCREEN_EVENT: return cast(PathType, SCREEN_EVENT), {"event": SCREEN_EVENT} - elif self.path_type == AUTOCAPTURE_EVENT: - return cast(PathType, AUTOCAPTURE_EVENT), {"event": AUTOCAPTURE_EVENT} elif self.path_type == CUSTOM_EVENT: return None, {} else: @@ -110,10 +103,6 @@ def include_pageviews(self) -> bool: def include_screenviews(self) -> bool: return SCREEN_EVENT in self.target_events - @property - def include_autocaptures(self) -> bool: - return AUTOCAPTURE_EVENT in self.target_events - @property def include_all_custom_events(self) -> bool: return CUSTOM_EVENT in self.target_events diff --git a/posthog/queries/test/test_paths.py b/posthog/queries/test/test_paths.py index 7992f828714c4..81da2c71bf55a 100644 --- a/posthog/queries/test/test_paths.py +++ b/posthog/queries/test/test_paths.py @@ -228,93 +228,6 @@ def test_screen_paths(self): self.assertEqual(response[3]["target"], "3_/about") self.assertEqual(response[3]["value"], 1) - def test_autocapture_paths(self): - person_factory(team_id=self.team.pk, distinct_ids=["person_1"]) - - event_factory( - event="$autocapture", - team=self.team, - distinct_id="person_1", - elements=[ - Element(tag_name="a", text="hello", href="/a-url", nth_child=1, nth_of_type=0), - Element(tag_name="button", nth_child=0, nth_of_type=0), - Element(tag_name="div", nth_child=0, nth_of_type=0), - Element(tag_name="div", nth_child=0, nth_of_type=0, attr_id="nested",), - ], - ) - - event_factory( - event="$autocapture", - team=self.team, - distinct_id="person_1", - elements=[ - Element(tag_name="a", text="goodbye", nth_child=2, nth_of_type=0, attr_id="someId",), - Element(tag_name="div", nth_child=0, nth_of_type=0), - # make sure elements don't get double counted if they're part of the same event - Element(href="/a-url-2", nth_child=0, nth_of_type=0), - ], - ) - - person_factory(team_id=self.team.pk, distinct_ids=["person_2"]) - event_factory( - event="$autocapture", - team=self.team, - distinct_id="person_2", - elements=[ - Element(tag_name="a", text="hello1", href="/a-url", nth_child=1, nth_of_type=0,), - Element(tag_name="button", nth_child=0, nth_of_type=0), - Element(tag_name="div", nth_child=0, nth_of_type=0), - Element(tag_name="div", nth_child=0, nth_of_type=0, attr_id="nested",), - ], - ) - - event_factory( - event="$autocapture", - team=self.team, - distinct_id="person_2", - elements=[ - Element(tag_name="a", text="goodbye1", nth_child=2, nth_of_type=0, attr_id="someId",), - Element(tag_name="div", nth_child=0, nth_of_type=0), - # make sure elements don't get double counted if they're part of the same event - Element(href="/a-url-2", nth_child=0, nth_of_type=0), - ], - ) - - event_factory( - event="$autocapture", - team=self.team, - distinct_id="person_2", - elements=[ - Element(tag_name="div", text="goodbye1", nth_child=2, nth_of_type=0, attr_id="someId",), - Element(tag_name="div", nth_child=0, nth_of_type=0), - # make sure elements don't get double counted if they're part of the same event - Element(href="/a-url-2", nth_child=0, nth_of_type=0), - ], - ) - filter = PathFilter(data={"path_type": "$autocapture"}) - - response = paths(team=self.team, filter=filter).run(team=self.team, filter=filter) - - self.assertEqual(response[0]["source"], "1_ hello") - self.assertEqual(response[0]["target"], "2_ goodbye") - self.assertEqual(response[0]["value"], 1) - - self.assertEqual(response[1]["source"], "1_ hello1") - self.assertEqual(response[1]["target"], "2_ goodbye1") - self.assertEqual(response[1]["value"], 1) - - self.assertEqual(response[2]["source"], "2_ goodbye1") - self.assertEqual(response[2]["target"], "3_
goodbye1") - self.assertEqual(response[2]["value"], 1) - - elements = self.client.get("/api/paths/elements/").json() - self.assertEqual(elements[0]["name"], " goodbye") # first since captured twice - self.assertEqual(elements[1]["name"], " goodbye1") - self.assertEqual(elements[2]["name"], " hello") - self.assertEqual(elements[3]["name"], " hello1") - self.assertEqual(elements[4]["name"], "
goodbye1") - self.assertEqual(len(elements), 5) - def test_paths_properties_filter(self): person_factory(team_id=self.team.pk, distinct_ids=["person_1"]) event_factory( From 57666f3f68112a1f23c36369a746ade997e67286 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 10 Sep 2021 14:45:12 +0100 Subject: [PATCH 2/4] lint --- ee/clickhouse/queries/test/test_paths.py | 2 +- frontend/src/scenes/insights/InsightTabs/PathTab.tsx | 2 -- frontend/src/types.ts | 1 - posthog/queries/paths.py | 3 --- 4 files changed, 1 insertion(+), 7 deletions(-) diff --git a/ee/clickhouse/queries/test/test_paths.py b/ee/clickhouse/queries/test/test_paths.py index b86ba53a76573..b31b994aef596 100644 --- a/ee/clickhouse/queries/test/test_paths.py +++ b/ee/clickhouse/queries/test/test_paths.py @@ -893,7 +893,7 @@ def test_paths_start_and_end(self): ) def test_properties_queried_using_path_filter(self): - def should_query_list(filter) -> Tuple[bool, bool, bool]: + def should_query_list(filter) -> Tuple[bool, bool]: path_query = PathEventQuery(filter, self.team.id) return (path_query._should_query_url(), path_query._should_query_screen()) diff --git a/frontend/src/scenes/insights/InsightTabs/PathTab.tsx b/frontend/src/scenes/insights/InsightTabs/PathTab.tsx index bebcb201565ea..c36ab51abff7e 100644 --- a/frontend/src/scenes/insights/InsightTabs/PathTab.tsx +++ b/frontend/src/scenes/insights/InsightTabs/PathTab.tsx @@ -57,7 +57,6 @@ export function PathTab({ annotationsToCreate }: BaseTabProps): JSX.Element { starting at ({ @@ -72,7 +71,6 @@ export function PathTab({ annotationsToCreate }: BaseTabProps): JSX.Element { value={filter.start_point} placeholder={'Select start element'} autoFocus={false} - allowCustom={filter.path_type !== PathType.AutoCapture} /> diff --git a/frontend/src/types.ts b/frontend/src/types.ts index b42695975f893..6ceca978278d6 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -638,7 +638,6 @@ export enum ViewType { export enum PathType { PageView = '$pageview', - AutoCapture = '$autocapture', Screen = '$screen', CustomEvent = 'custom_event', } diff --git a/posthog/queries/paths.py b/posthog/queries/paths.py index df3b2559a7eae..1660ad7557cf5 100644 --- a/posthog/queries/paths.py +++ b/posthog/queries/paths.py @@ -86,9 +86,6 @@ def calculate_paths(self, filter: PathFilter, team: Team): sessions_sql, sessions_sql_params = sessions.query.sql_with_params() - if event == "$autocapture": - sessions_sql = self._add_elements(query_string=sessions_sql) - events_notated = "\ SELECT *, CASE WHEN EXTRACT('EPOCH' FROM (timestamp - previous_timestamp)) >= (60 * 30) OR previous_timestamp IS NULL THEN 1 ELSE 0 END AS new_session\ FROM ({}) AS inner_sessions\ From 3252429d310ac68a0d438d1c1e8fad8da4d7d961 Mon Sep 17 00:00:00 2001 From: eric Date: Mon, 13 Sep 2021 14:05:18 -0400 Subject: [PATCH 3/4] remove autocapture from postgres paths --- posthog/queries/paths.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/posthog/queries/paths.py b/posthog/queries/paths.py index 1660ad7557cf5..92a62384253b3 100644 --- a/posthog/queries/paths.py +++ b/posthog/queries/paths.py @@ -65,11 +65,7 @@ def calculate_paths(self, filter: PathFilter, team: Team): sessions = ( Event.objects.add_person_id(team.pk) .filter(team=team, **(event_filter), **date_query) - .filter( - ~Q(event__in=["$autocapture", "$pageview", "$identify", "$pageleave", "$screen"]) - if event is None - else Q() - ) + .filter(~Q(event__in=["$pageview", "$identify", "$pageleave", "$screen"]) if event is None else Q()) .filter( properties_to_Q(filter.properties, team_id=team.pk, filter_test_accounts=filter.filter_test_accounts) if filter and (filter.properties or filter.filter_test_accounts) From a1d9fb58b342d449f12d08ba7de31e0344dba987 Mon Sep 17 00:00:00 2001 From: eric Date: Mon, 20 Sep 2021 10:34:23 -0400 Subject: [PATCH 4/4] restore autocapture negation filter in postgres paths --- posthog/queries/paths.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/posthog/queries/paths.py b/posthog/queries/paths.py index 92a62384253b3..1660ad7557cf5 100644 --- a/posthog/queries/paths.py +++ b/posthog/queries/paths.py @@ -65,7 +65,11 @@ def calculate_paths(self, filter: PathFilter, team: Team): sessions = ( Event.objects.add_person_id(team.pk) .filter(team=team, **(event_filter), **date_query) - .filter(~Q(event__in=["$pageview", "$identify", "$pageleave", "$screen"]) if event is None else Q()) + .filter( + ~Q(event__in=["$autocapture", "$pageview", "$identify", "$pageleave", "$screen"]) + if event is None + else Q() + ) .filter( properties_to_Q(filter.properties, team_id=team.pk, filter_test_accounts=filter.filter_test_accounts) if filter and (filter.properties or filter.filter_test_accounts)