From 68d4e00f8694bc95f9deaa1dbb62bddee00f9159 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Tue, 15 Oct 2024 13:48:42 +0200 Subject: [PATCH] refactor(insights): Filter actions by project rather than team (#25471) --- posthog/hogql_queries/insights/funnels/base.py | 6 ++++-- .../funnels/funnel_correlation_query_runner.py | 3 ++- .../insights/funnels/funnel_event_query.py | 5 +++-- .../insights/lifecycle_query_runner.py | 6 ++++-- .../insights/retention_query_runner.py | 3 ++- .../insights/stickiness_query_runner.py | 6 ++++-- .../insights/trends/test/test_trends.py | 15 +++++++++++++++ .../trends/trends_actors_query_builder.py | 3 ++- .../insights/trends/trends_query_builder.py | 3 ++- .../insights/trends/trends_query_runner.py | 3 ++- .../hogql_queries/web_analytics/web_overview.py | 3 ++- 11 files changed, 42 insertions(+), 14 deletions(-) diff --git a/posthog/hogql_queries/insights/funnels/base.py b/posthog/hogql_queries/insights/funnels/base.py index cf6836a4dd168..05d8fc4dca064 100644 --- a/posthog/hogql_queries/insights/funnels/base.py +++ b/posthog/hogql_queries/insights/funnels/base.py @@ -304,7 +304,8 @@ def _serialize_step( "Data warehouse tables are not supported in funnels just yet. For now, please try this funnel without the data warehouse-based step." ) else: - action = Action.objects.get(pk=step.id) + assert self.context.team.project_id is not None + action = Action.objects.get(pk=step.id, team__project_id=self.context.team.project_id) name = action.name action_id = step.id type = "actions" @@ -676,7 +677,8 @@ def _build_step_query( if isinstance(entity, ActionsNode) or isinstance(entity, FunnelExclusionActionsNode): # action - action = Action.objects.get(pk=int(entity.id), team=self.context.team) + assert self.context.team.project_id is not None + action = Action.objects.get(pk=int(entity.id), team__project_id=self.context.team.project_id) event_expr = action_to_expr(action) elif isinstance(entity, DataWarehouseNode): raise ValidationError( diff --git a/posthog/hogql_queries/insights/funnels/funnel_correlation_query_runner.py b/posthog/hogql_queries/insights/funnels/funnel_correlation_query_runner.py index e67e00d9d04a1..60265cb332b3a 100644 --- a/posthog/hogql_queries/insights/funnels/funnel_correlation_query_runner.py +++ b/posthog/hogql_queries/insights/funnels/funnel_correlation_query_runner.py @@ -830,7 +830,8 @@ def _get_funnel_step_names(self) -> list[str]: events: set[str] = set() for entity in self.funnels_query.series: if isinstance(entity, ActionsNode): - action = Action.objects.get(pk=int(entity.id), team=self.context.team) + assert self.context.team.project_id is not None + action = Action.objects.get(pk=int(entity.id), team__project_id=self.context.team.project_id) events.update([x for x in action.get_step_events() if x]) elif isinstance(entity, EventsNode): if entity.event is not None: diff --git a/posthog/hogql_queries/insights/funnels/funnel_event_query.py b/posthog/hogql_queries/insights/funnels/funnel_event_query.py index c4cb9507534ef..49e4b304c1270 100644 --- a/posthog/hogql_queries/insights/funnels/funnel_event_query.py +++ b/posthog/hogql_queries/insights/funnels/funnel_event_query.py @@ -132,7 +132,7 @@ def _date_range_expr(self) -> ast.Expr: ) def _entity_expr(self, skip_entity_filter: bool) -> ast.Expr | None: - team, query, funnelsFilter = self.context.team, self.context.query, self.context.funnelsFilter + query, funnelsFilter = self.context.query, self.context.funnelsFilter exclusions = funnelsFilter.exclusions or [] if skip_entity_filter is True: @@ -144,8 +144,9 @@ def _entity_expr(self, skip_entity_filter: bool) -> ast.Expr | None: if isinstance(node, EventsNode) or isinstance(node, FunnelExclusionEventsNode): events.add(node.event) elif isinstance(node, ActionsNode) or isinstance(node, FunnelExclusionActionsNode): + assert self.context.team.project_id is not None try: - action = Action.objects.get(pk=int(node.id), team=team) + action = Action.objects.get(pk=int(node.id), team__project_id=self.context.team.project_id) events.update(action.get_step_events()) except Action.DoesNotExist: raise ValidationError(f"Action ID {node.id} does not exist!") diff --git a/posthog/hogql_queries/insights/lifecycle_query_runner.py b/posthog/hogql_queries/insights/lifecycle_query_runner.py index b56632e9de4b5..0f97bc3c1f313 100644 --- a/posthog/hogql_queries/insights/lifecycle_query_runner.py +++ b/posthog/hogql_queries/insights/lifecycle_query_runner.py @@ -181,7 +181,8 @@ def calculate(self) -> LifecycleQueryResponse: action_object = {} label = "{} - {}".format("", val[2]) if isinstance(self.query.series[0], ActionsNode): - action = Action.objects.get(pk=int(self.query.series[0].id), team=self.team) + assert self.team.project_id is not None + action = Action.objects.get(pk=int(self.query.series[0].id), team__project_id=self.team.project_id) label = "{} - {}".format(action.name, val[2]) action_object = { "id": str(action.pk), @@ -248,7 +249,8 @@ def event_filter(self) -> ast.Expr: with self.timings.measure("series_filters"): for serie in self.query.series or []: if isinstance(serie, ActionsNode): - action = Action.objects.get(pk=int(serie.id), team=self.team) + assert self.team.project_id is not None + action = Action.objects.get(pk=int(serie.id), team__project_id=self.team.project_id) event_filters.append(action_to_expr(action)) elif isinstance(serie, EventsNode): if serie.event is not None: diff --git a/posthog/hogql_queries/insights/retention_query_runner.py b/posthog/hogql_queries/insights/retention_query_runner.py index 27db6819148e1..a1566f6f81085 100644 --- a/posthog/hogql_queries/insights/retention_query_runner.py +++ b/posthog/hogql_queries/insights/retention_query_runner.py @@ -87,7 +87,8 @@ def filter_timestamp(self) -> ast.Expr: def _get_events_for_entity(self, entity: RetentionEntity) -> list[str | None]: if entity.type == EntityType.ACTIONS and entity.id: - action = Action.objects.get(pk=int(entity.id)) + assert self.team.project_id is not None + action = Action.objects.get(pk=int(entity.id), team__project_id=self.team.project_id) return action.get_step_events() return [entity.id] if isinstance(entity.id, str) else [None] diff --git a/posthog/hogql_queries/insights/stickiness_query_runner.py b/posthog/hogql_queries/insights/stickiness_query_runner.py index 4fd7b096d006d..538423b1d3a80 100644 --- a/posthog/hogql_queries/insights/stickiness_query_runner.py +++ b/posthog/hogql_queries/insights/stickiness_query_runner.py @@ -275,8 +275,9 @@ def where_clause(self, series_with_extra: SeriesWithExtras) -> ast.Expr: ) ) elif isinstance(series, ActionsNode): + assert self.team.project_id is not None try: - action = Action.objects.get(pk=int(series.id), team=self.team) + action = Action.objects.get(pk=int(series.id), team__project_id=self.team.project_id) filters.append(action_to_expr(action)) except Action.DoesNotExist: # If an action doesn't exist, we want to return no events @@ -330,8 +331,9 @@ def series_event(self, series: EventsNode | ActionsNode | DataWarehouseNode) -> return series.table_name if isinstance(series, ActionsNode): + assert self.team.project_id is not None # TODO: Can we load the Action in more efficiently? - action = Action.objects.get(pk=int(series.id), team=self.team) + action = Action.objects.get(pk=int(series.id), team__project_id=self.team.project_id) return action.name def intervals_num(self): diff --git a/posthog/hogql_queries/insights/trends/test/test_trends.py b/posthog/hogql_queries/insights/trends/test/test_trends.py index 7f7977f406bfb..18e0e2b267fda 100644 --- a/posthog/hogql_queries/insights/trends/test/test_trends.py +++ b/posthog/hogql_queries/insights/trends/test/test_trends.py @@ -3850,6 +3850,21 @@ def test_action_filtering(self): self.assertEntityResponseEqual(action_response, event_response) + def test_action_filtering_for_action_in_different_env_of_project(self): + sign_up_action, person = self._create_events() + other_team_in_project = Team.objects.create(organization=self.organization, project=self.project) + sign_up_action.team = other_team_in_project + sign_up_action.save() + + action_response = self._run( + Filter(team=self.team, data={"actions": [{"id": sign_up_action.id}]}), + self.team, + ) + event_response = self._run(Filter(team=self.team, data={"events": [{"id": "sign up"}]}), self.team) + self.assertEqual(len(action_response), 1) + + self.assertEntityResponseEqual(action_response, event_response) + @also_test_with_person_on_events_v2 @snapshot_clickhouse_queries def test_action_filtering_with_cohort(self): diff --git a/posthog/hogql_queries/insights/trends/trends_actors_query_builder.py b/posthog/hogql_queries/insights/trends/trends_actors_query_builder.py index 1ce6a5b593f28..8e19c9bfbec8d 100644 --- a/posthog/hogql_queries/insights/trends/trends_actors_query_builder.py +++ b/posthog/hogql_queries/insights/trends/trends_actors_query_builder.py @@ -292,8 +292,9 @@ def _entity_where_expr(self) -> list[ast.Expr]: def _event_or_action_where_expr(self) -> ast.Expr | None: if isinstance(self.entity, ActionsNode): # Actions + assert self.team.project_id is not None try: - action = Action.objects.get(pk=int(self.entity.id), team=self.team) + action = Action.objects.get(pk=int(self.entity.id), team__project_id=self.team.project_id) return action_to_expr(action) except Action.DoesNotExist: # If an action doesn't exist, we want to return no events diff --git a/posthog/hogql_queries/insights/trends/trends_query_builder.py b/posthog/hogql_queries/insights/trends/trends_query_builder.py index 7a93be2d548b0..7f319dc0347d5 100644 --- a/posthog/hogql_queries/insights/trends/trends_query_builder.py +++ b/posthog/hogql_queries/insights/trends/trends_query_builder.py @@ -738,8 +738,9 @@ def _event_or_action_where_expr(self) -> ast.Expr | None: # Actions if isinstance(self.series, ActionsNode): + assert self.team.project_id is not None try: - action = Action.objects.get(pk=int(self.series.id), team=self.team) + action = Action.objects.get(pk=int(self.series.id), team__project_id=self.team.project_id) return action_to_expr(action) except Action.DoesNotExist: # If an action doesn't exist, we want to return no events diff --git a/posthog/hogql_queries/insights/trends/trends_query_runner.py b/posthog/hogql_queries/insights/trends/trends_query_runner.py index 4d7e6ec6198ff..06d47b2259e44 100644 --- a/posthog/hogql_queries/insights/trends/trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/trends_query_runner.py @@ -640,8 +640,9 @@ def series_event(self, series: Union[EventsNode, ActionsNode, DataWarehouseNode] if isinstance(series, EventsNode): return series.event if isinstance(series, ActionsNode): + assert self.team.project_id is not None # TODO: Can we load the Action in more efficiently? - action = Action.objects.get(pk=int(series.id), team=self.team) + action = Action.objects.get(pk=int(series.id), team__project_id=self.team.project_id) return action.name if isinstance(series, DataWarehouseNode): diff --git a/posthog/hogql_queries/web_analytics/web_overview.py b/posthog/hogql_queries/web_analytics/web_overview.py index 31d0bb43d337a..3680235d20d14 100644 --- a/posthog/hogql_queries/web_analytics/web_overview.py +++ b/posthog/hogql_queries/web_analytics/web_overview.py @@ -100,7 +100,8 @@ def session_properties(self) -> ast.Expr: @cached_property def conversion_goal_expr(self) -> ast.Expr: if isinstance(self.query.conversionGoal, ActionConversionGoal): - action = Action.objects.get(pk=self.query.conversionGoal.actionId) + assert self.team.project_id is not None + action = Action.objects.get(pk=self.query.conversionGoal.actionId, team__project_id=self.team.project_id) return action_to_expr(action) elif isinstance(self.query.conversionGoal, CustomEventConversionGoal): return ast.CompareOperation(