Skip to content

Commit

Permalink
fix(funnel): TypeError for funnel with display=ActionsLineGraph (#6538
Browse files Browse the repository at this point in the history
)

* Revert "perf(funnels): add filter on pdi.team_id to speed up query
(#5997)"

This merged caused an issue with the funnels endpoint when display was
set to `ActionsLineGraph`

I'm reverting so we can add in a snapshot test that will fail when this
revert is reverted.

This reverts commit 2fb7cf8.

* test(funnel): add snapshot test for funnel trend query for postgres

This test just checks the Funnel with `display="ActionsLineGraph"`
against what it previously did before
#5997 was merged. This code path
was previously untested, so the issue wasn't picked up and resulted in
#6530

* Revert "Revert "perf(funnels): add filter on pdi.team_id to speed up
query"

Now that we have a test in place for
#6530 I'm reverting this revert
so we can implement a fix.

This reverts commit 49761a3.

* only test on postgres

* fix(funnel): `TypeError` for funnel with display=ActionsLineGraph

This resolves an issue what was introduced by
#5997 where we would fail to
calculate a funnel result if display="ActionsLineGraph" was specified.
The fix was simply to wrap a string in `sql.SQL`.

Resolves #6530

* fix import order

* move test outside of test factory

* remove imports

* move test
  • Loading branch information
Harry Waye authored Oct 19, 2021
1 parent 500cf5b commit d10605d
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 2 deletions.
2 changes: 1 addition & 1 deletion posthog/queries/funnel.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ def _build_trends_query(self, filter: Filter) -> sql.SQL:
).format(
interval=sql.Literal(filter.interval),
particular_steps=sql.SQL(",\n").join(particular_steps),
steps_query=self._build_query(within_time="'1 day'"),
steps_query=sql.SQL(self._build_query(within_time="'1 day'")),
interval_field=sql.SQL("step_0")
if filter.interval != "week"
else sql.SQL("(\"step_0\" + interval '1 day') AT TIME ZONE 'UTC'"),
Expand Down
43 changes: 42 additions & 1 deletion posthog/queries/test/test_funnel.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from datetime import datetime
from unittest.mock import patch

import pytz
from freezegun import freeze_time

from posthog.constants import FILTER_TEST_ACCOUNTS, INSIGHT_FUNNELS
Expand Down Expand Up @@ -414,4 +416,43 @@ def test_funnel_filter_by_action_with_person_properties(self):


class TestFunnel(funnel_test_factory(Funnel, Event.objects.create, Person.objects.create)): # type: ignore
pass
def test_funnel_with_display_set_to_trends_linear(self):
"""
This is a limited regression test to ensure that the issue
highlighted by https://github.com/PostHog/posthog/issues/6530 is
resolved.
I am not attempting to evaluate the correctness of
`ActionsLineGraph` with this test, just that it is the same as
before https://github.com/PostHog/posthog/pull/5997#event-5326521193
was introduceed.
"""
with freeze_time("2020-01-01"):
Person.objects.create(
distinct_ids=["person1"], team_id=self.team.pk, properties={"email": "[email protected]"}
)
Event.objects.create(distinct_id="person1", event="event1", team=self.team)
Event.objects.create(distinct_id="person1", event="event2", team=self.team)

result = Funnel(
filter=Filter(
data={
"events": [{"id": "event1"}, {"id": "event2"}],
"insight": INSIGHT_FUNNELS,
"display": "ActionsLineGraph",
"interval": "day",
# Just get today, so the response isn't massive
"date_from": "-0days",
}
),
team=self.team,
).run()

assert result == [
{
"count": 0,
"data": [100],
"days": [datetime(2020, 1, 1).replace(tzinfo=pytz.UTC)],
"labels": ["1-Jan-2020"],
}
]

0 comments on commit d10605d

Please sign in to comment.