Skip to content

Commit

Permalink
refactor(insights): Filter actions by project rather than team (#25471)
Browse files Browse the repository at this point in the history
  • Loading branch information
Twixes authored Oct 15, 2024
1 parent 35f3fb3 commit 68d4e00
Show file tree
Hide file tree
Showing 11 changed files with 42 additions and 14 deletions.
6 changes: 4 additions & 2 deletions posthog/hogql_queries/insights/funnels/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
5 changes: 3 additions & 2 deletions posthog/hogql_queries/insights/funnels/funnel_event_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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!")
Expand Down
6 changes: 4 additions & 2 deletions posthog/hogql_queries/insights/lifecycle_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion posthog/hogql_queries/insights/retention_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
6 changes: 4 additions & 2 deletions posthog/hogql_queries/insights/stickiness_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
15 changes: 15 additions & 0 deletions posthog/hogql_queries/insights/trends/test/test_trends.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion posthog/hogql_queries/insights/trends/trends_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
3 changes: 2 additions & 1 deletion posthog/hogql_queries/web_analytics/web_overview.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 68d4e00

Please sign in to comment.