Repository: PostHog/posthog
Pull request: Funnel persons per step and dropoff
## Changes

*Please describe.*  
- pointed at #4851 
- add ability to get persons by specific dropoff or step 
- the api will accept a `funnel_step` parameter that specifies the step at which you want the list of persons. Indexing starts at 1 so 1 = first step, 2 = second step, etc. The dropoffs are denoted by negatives so the people that dropped off from the first to second step would be -2, dropoffs from second to third step would be -3, etc.
*If this affects the frontend, include screenshots.*  

## Checklist

- [ ] All querysets/queries filter by Organization, by Team, and by User
- [ ] Django backend tests
- [ ] Jest frontend tests
- [ ] Cypress end-to-end tests
- [ ] Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
- [ ] Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)
- [ ] Breaking changes are backwards-compatible. Ensure old/new frontend requests work with new/old backends, and vice versa.


Problem statement:
Funnels - API for getting persons on a step or drop-off
Issue to document the task of creating the API that will retrieve the person list at each step or drop-off in a funnel.

@buwilliams feel free to take over this issue.

Hints from issue discussion:
Full product specs can be found [here](https://docs.google.com/document/d/1S7fwha7cph-79YaPADfFKHILCnPmBgk4rqaMu-gPZ3g/edit#heading=h.w1yaw9nwtpu), but an important consideration is that we clearly show in the interface whether a session recording is available for each specific person. I'm thinking this information might need to come from the API but I'll let you suggest the best approach here. CC @mariusandra

Commit message: fix tests

Patch to review:
diff --git a/ee/clickhouse/queries/funnels/base.py b/ee/clickhouse/queries/funnels/base.py
index 3202fba9c8d03..56cebd697c1e3 100644
--- a/ee/clickhouse/queries/funnels/base.py
+++ b/ee/clickhouse/queries/funnels/base.py
@@ -12,7 +12,7 @@
 from ee.clickhouse.sql.person import GET_LATEST_PERSON_DISTINCT_ID_SQL
 from posthog.constants import TREND_FILTER_TYPE_ACTIONS
 from posthog.models import Action, Entity, Filter, Team
-from posthog.models.filters.mixins.funnel_window_days import FunnelWindowDaysMixin
+from posthog.models.filters.mixins.funnel import FunnelWindowDaysMixin
 from posthog.queries.funnel import Funnel
 from posthog.utils import relative_date_parse
 
@@ -90,7 +90,6 @@ def _exec_query(self) -> List[Tuple]:
         }
 
         query = self.get_query(format_properties)
-
         return sync_execute(query, self.params)
 
     def _get_steps_per_person_query(self):
diff --git a/ee/clickhouse/queries/funnels/funnel_persons.py b/ee/clickhouse/queries/funnels/funnel_persons.py
index 02cee79410d69..dccf984a50737 100644
--- a/ee/clickhouse/queries/funnels/funnel_persons.py
+++ b/ee/clickhouse/queries/funnels/funnel_persons.py
@@ -1,15 +1,31 @@
 from ee.clickhouse.queries.funnels.base import ClickhouseFunnelBase
-from ee.clickhouse.sql.funnels.funnel import FUNNEL_PERSONS_SQL
+from ee.clickhouse.sql.funnels.funnel import FUNNEL_PERSONS_BY_STEP_SQL
 from posthog.models import Person
 
 
 class ClickhouseFunnelPersons(ClickhouseFunnelBase):
     def get_query(self, format_properties):
-        return FUNNEL_PERSONS_SQL.format(**format_properties)
+        steps_per_person_query = self._get_steps_per_person_query()
+        return FUNNEL_PERSONS_BY_STEP_SQL.format(
+            **format_properties,
+            steps_per_person_query=steps_per_person_query,
+            persons_steps=self._get_funnel_person_step_condition()
+        )
+
+    def _get_funnel_person_step_condition(self):
+        step_num = self._filter.funnel_step
+        max_steps = len(self._filter.entities)
+
+        if step_num >= 0:
+            self.params.update({"step_num": [i for i in range(step_num, max_steps + 1)]})
+            return "steps IN %(step_num)s"
+        else:
+            self.params.update({"step_num": abs(step_num) - 1})
+            return "steps = %(step_num)s"
 
     def _format_results(self, results):
         formatted_results = []
         for row in results:
-            distinct_ids, email = Person.get_distinct_ids_and_email_by_id(row[1], self._team.id)
+            distinct_ids, email = Person.get_distinct_ids_and_email_by_id(row[0], self._team.id)
             formatted_results.append({"max_step": row[0], "distinct_ids": distinct_ids, "email": email})
         return formatted_results
diff --git a/ee/clickhouse/queries/funnels/test/test_funnel.py b/ee/clickhouse/queries/funnels/test/test_funnel.py
index c703ed80817ff..f7ceec17d736c 100644
--- a/ee/clickhouse/queries/funnels/test/test_funnel.py
+++ b/ee/clickhouse/queries/funnels/test/test_funnel.py
@@ -47,6 +47,7 @@ def test_basic_funnel_with_repeat_steps(self):
                 {"id": "user signed up", "type": "events", "order": 1},
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
@@ -84,6 +85,7 @@ def test_advanced_funnel_with_repeat_steps(self):
                 {"id": "$pageview", "type": "events", "order": 4},
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
@@ -181,6 +183,7 @@ def test_advanced_funnel_with_repeat_steps_out_of_order_events(self):
                 {"id": "$pageview", "type": "events", "order": 4},
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
@@ -291,6 +294,7 @@ def test_funnel_with_actions(self):
                 {"id": sign_up_action.id, "math": "wau", "order": 1},
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
@@ -336,6 +340,7 @@ def test_funnel_with_actions_and_events(self):
                 {"id": sign_up_action.id, "math": "wau", "order": 3},
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
@@ -406,6 +411,7 @@ def test_funnel_with_matching_properties(self):
                 },  # TODO(nk): does this supercede the above event? i.e. order 3 is subset of order 4? doesn't make sense to allow this in a funnel
             ],
             "insight": INSIGHT_FUNNELS,
+            "funnel_window_days": 14,
         }
 
         filter = Filter(data=filters)
diff --git a/ee/clickhouse/queries/funnels/test/test_funnel_persons.py b/ee/clickhouse/queries/funnels/test/test_funnel_persons.py
index 5ac16e47dad16..d39eaa4eb3618 100644
--- a/ee/clickhouse/queries/funnels/test/test_funnel_persons.py
+++ b/ee/clickhouse/queries/funnels/test/test_funnel_persons.py
@@ -6,7 +6,7 @@
 from ee.clickhouse.util import ClickhouseTestMixin
 from posthog.constants import INSIGHT_FUNNELS, TRENDS_FUNNEL
 from posthog.models import Filter
-from posthog.models.filters.mixins.funnel_window_days import FunnelWindowDaysMixin
+from posthog.models.filters.mixins.funnel import FunnelWindowDaysMixin
 from posthog.models.person import Person
 from posthog.test.base import APIBaseTest
 
@@ -27,9 +27,97 @@ def _create_event(**kwargs):
 
 
 class TestFunnel(ClickhouseTestMixin, APIBaseTest):
-    def setUp(self):
-        self._create_sample_data()
-        super().setUp()
+    def _create_sample_data_multiple_dropoffs(self):
+        for i in range(5):
+            _create_person(distinct_ids=[f"user_{i}"], team=self.team)
+            _create_event(event="step one", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-01 00:00:00")
+            _create_event(event="step two", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-03 00:00:00")
+            _create_event(event="step three", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-05 00:00:00")
+
+        for i in range(5, 15):
+            _create_person(distinct_ids=[f"user_{i}"], team=self.team)
+            _create_event(event="step one", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-01 00:00:00")
+            _create_event(event="step two", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-03 00:00:00")
+
+        for i in range(15, 30):
+            _create_person(distinct_ids=[f"user_{i}"], team=self.team)
+            _create_event(event="step one", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-01 00:00:00")
+
+    def test_first_step(self):
+        self._create_sample_data_multiple_dropoffs()
+        data = {
+            "insight": INSIGHT_FUNNELS,
+            "interval": "day",
+            "date_from": "2021-05-01 00:00:00",
+            "date_to": "2021-05-07 00:00:00",
+            "funnel_window_days": 7,
+            "funnel_step": 1,
+            "events": [
+                {"id": "step one", "order": 0},
+                {"id": "step two", "order": 1},
+                {"id": "step three", "order": 2},
+            ],
+        }
+        filter = Filter(data=data)
+        results = ClickhouseFunnelPersons(filter, self.team)._exec_query()
+        self.assertEqual(30, len(results))
+
+    def test_last_step(self):
+        self._create_sample_data_multiple_dropoffs()
+        data = {
+            "insight": INSIGHT_FUNNELS,
+            "interval": "day",
+            "date_from": "2021-05-01 00:00:00",
+            "date_to": "2021-05-07 00:00:00",
+            "funnel_window_days": 7,
+            "funnel_step": 3,
+            "events": [
+                {"id": "step one", "order": 0},
+                {"id": "step two", "order": 1},
+                {"id": "step three", "order": 2},
+            ],
+        }
+        filter = Filter(data=data)
+        results = ClickhouseFunnelPersons(filter, self.team)._exec_query()
+        self.assertEqual(5, len(results))
+
+    def test_second_step_dropoff(self):
+        self._create_sample_data_multiple_dropoffs()
+        data = {
+            "insight": INSIGHT_FUNNELS,
+            "interval": "day",
+            "date_from": "2021-05-01 00:00:00",
+            "date_to": "2021-05-07 00:00:00",
+            "funnel_window_days": 7,
+            "funnel_step": -2,
+            "events": [
+                {"id": "step one", "order": 0},
+                {"id": "step two", "order": 1},
+                {"id": "step three", "order": 2},
+            ],
+        }
+        filter = Filter(data=data)
+        results = ClickhouseFunnelPersons(filter, self.team)._exec_query()
+        self.assertEqual(15, len(results))
+
+    def test_last_step_dropoff(self):
+        self._create_sample_data_multiple_dropoffs()
+        data = {
+            "insight": INSIGHT_FUNNELS,
+            "interval": "day",
+            "date_from": "2021-05-01 00:00:00",
+            "date_to": "2021-05-07 00:00:00",
+            "funnel_window_days": 7,
+            "funnel_step": -3,
+            "events": [
+                {"id": "step one", "order": 0},
+                {"id": "step two", "order": 1},
+                {"id": "step three", "order": 2},
+            ],
+        }
+        filter = Filter(data=data)
+        results = ClickhouseFunnelPersons(filter, self.team)._exec_query()
+        self.assertEqual(10, len(results))
 
     def _create_sample_data(self):
         for i in range(250):
@@ -39,13 +127,14 @@ def _create_sample_data(self):
             _create_event(event="step three", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-05 00:00:00")
 
     def test_basic_offset(self):
+        self._create_sample_data()
         data = {
             "insight": INSIGHT_FUNNELS,
-            "display": TRENDS_FUNNEL,
             "interval": "day",
             "date_from": "2021-05-01 00:00:00",
             "date_to": "2021-05-07 00:00:00",
             "funnel_window_days": 7,
+            "funnel_step": 1,
             "events": [
                 {"id": "step one", "order": 0},
                 {"id": "step two", "order": 1},
@@ -79,13 +168,14 @@ def test_funnel_window_days_to_milliseconds(self):
         self.assertEqual(one_day, 86_400_000)
 
     def test_basic_conversion_window(self):
+        self._create_sample_data()
         data = {
             "insight": INSIGHT_FUNNELS,
-            "display": TRENDS_FUNNEL,
             "interval": "day",
             "date_from": "2021-05-01 00:00:00",
             "date_to": "2021-05-07 00:00:00",
             "funnel_window_days": 7,
+            "funnel_step": 1,
             "events": [
                 {"id": "step one", "order": 0},
                 {"id": "step two", "order": 1},
diff --git a/ee/clickhouse/sql/funnels/funnel.py b/ee/clickhouse/sql/funnels/funnel.py
index c78140af6ae2c..8ccd67ae017a3 100644
--- a/ee/clickhouse/sql/funnels/funnel.py
+++ b/ee/clickhouse/sql/funnels/funnel.py
@@ -55,6 +55,16 @@
 ;
 """
 
+FUNNEL_PERSONS_BY_STEP_SQL = """
+SELECT person_id
+FROM
+({steps_per_person_query})
+WHERE {persons_steps}
+LIMIT 100
+OFFSET {offset}
+SETTINGS allow_experimental_window_functions = 1
+"""
+
 FUNNEL_INNER_EVENT_STEPS_QUERY = """
 SELECT 
 person_id,
diff --git a/ee/clickhouse/views/test/test_clickhouse_funnel_person.py b/ee/clickhouse/views/test/test_clickhouse_funnel_person.py
index 5f87e2d01f2a1..b70e7143a59a8 100644
--- a/ee/clickhouse/views/test/test_clickhouse_funnel_person.py
+++ b/ee/clickhouse/views/test/test_clickhouse_funnel_person.py
@@ -1,22 +1,37 @@
 import json
+from uuid import uuid4
 
 from rest_framework import status
 
-from ee.clickhouse.generate_local import GenerateLocal
+from ee.clickhouse.models.event import create_event
 from ee.clickhouse.util import ClickhouseTestMixin
 from posthog.constants import INSIGHT_FUNNELS, TRENDS_FUNNEL
+from posthog.models.person import Person
 from posthog.test.base import APIBaseTest
 
 
+def _create_person(**kwargs):
+    person = Person.objects.create(**kwargs)
+    return Person(id=person.uuid, uuid=person.uuid)
+
+
+def _create_event(**kwargs):
+    kwargs.update({"event_uuid": uuid4()})
+    create_event(**kwargs)
+
+
 class TestFunnelPerson(ClickhouseTestMixin, APIBaseTest):
-    def setUp(self):
-        super().setUp()
-        GenerateLocal(self.team).generate()
+    def _create_sample_data(self):
+        for i in range(250):
+            _create_person(distinct_ids=[f"user_{i}"], team=self.team)
+            _create_event(event="step one", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-01 00:00:00")
+            _create_event(event="step two", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-03 00:00:00")
+            _create_event(event="step three", distinct_id=f"user_{i}", team=self.team, timestamp="2021-05-05 00:00:00")
 
     def test_basic_pagination(self):
+        self._create_sample_data()
         request_data = {
             "insight": INSIGHT_FUNNELS,
-            "display": TRENDS_FUNNEL,
             "interval": "day",
             "actions": json.dumps([]),
             "events": json.dumps(
@@ -24,10 +39,11 @@ def test_basic_pagination(self):
             ),
             "properties": json.dumps([]),
             "funnel_window_days": 14,
+            "funnel_step": 1,
             "filter_test_accounts": "false",
             "new_entity": json.dumps([]),
-            "date_from": "2010-01-01",
-            "date_to": "2010-01-10",
+            "date_from": "2021-05-01",
+            "date_to": "2021-05-10",
         }
 
         response = self.client.get("/api/person/funnel/", data=request_data)
diff --git a/ee/tasks/test/test_calculate_cohort.py b/ee/tasks/test/test_calculate_cohort.py
index c69036ff79efb..036e069a36f56 100644
--- a/ee/tasks/test/test_calculate_cohort.py
+++ b/ee/tasks/test/test_calculate_cohort.py
@@ -150,7 +150,6 @@ def test_create_trends_cohort(self, _insert_cohort_from_query):
                 ],
                 "entity_id": "$pageview",
                 "entity_type": "events",
-                "funnel_window_days": 14,
                 "insight": "TRENDS",
                 "interval": "day",
             },
@@ -184,7 +183,6 @@ def test_create_trends_cohort(self, _insert_cohort_from_query):
                 ],
                 "entity_id": "$pageview",
                 "entity_type": "events",
-                "funnel_window_days": 14,
                 "insight": "TRENDS",
                 "interval": "day",
             },
diff --git a/posthog/api/test/test_insight.py b/posthog/api/test/test_insight.py
index 93e79b5da3817..e6922102c5799 100644
--- a/posthog/api/test/test_insight.py
+++ b/posthog/api/test/test_insight.py
@@ -183,7 +183,8 @@ def test_insight_funnels_basic_post(self):
                     "events": [
                         {"id": "user signed up", "type": "events", "order": 0},
                         {"id": "user did things", "type": "events", "order": 1},
-                    ]
+                    ],
+                    "funnel_window_days": 14,
                 },
             ).json()
 
@@ -200,7 +201,7 @@ def test_insight_funnels_basic_get(self):
             event_factory(team=self.team, event="user signed up", distinct_id="1")
             event_factory(team=self.team, event="user did things", distinct_id="1")
             response = self.client.get(
-                "/api/insight/funnel/?events={}".format(
+                "/api/insight/funnel/?funnel_window_days=14&events={}".format(
                     json.dumps(
                         [
                             {"id": "user signed up", "type": "events", "order": 0},
diff --git a/posthog/constants.py b/posthog/constants.py
index c8dc0eb681086..eeff774ecc14f 100644
--- a/posthog/constants.py
+++ b/posthog/constants.py
@@ -86,6 +86,7 @@
 ENTITY_TYPE = "entity_type"
 ENTITY_MATH = "entity_math"
 FUNNEL_WINDOW_DAYS = "funnel_window_days"
+FUNNEL_STEP = "funnel_step"
 
 RETENTION_RECURRING = "retention_recurring"
 RETENTION_FIRST_TIME = "retention_first_time"
diff --git a/posthog/models/filters/filter.py b/posthog/models/filters/filter.py
index 5862b2e714ef2..1bf06285213f4 100644
--- a/posthog/models/filters/filter.py
+++ b/posthog/models/filters/filter.py
@@ -25,7 +25,7 @@
     SessionMixin,
     ShownAsMixin,
 )
-from posthog.models.filters.mixins.funnel_window_days import FunnelWindowDaysMixin
+from posthog.models.filters.mixins.funnel import FunnelStep, FunnelWindowDaysMixin
 from posthog.models.filters.mixins.property import PropertyMixin
 
 
@@ -50,6 +50,7 @@ class Filter(
     BaseFilter,
     FormulaMixin,
     FunnelWindowDaysMixin,
+    FunnelStep,
 ):
     """
     Filters allow us to describe what events to show/use in various places in the system, for example Trends or Funnels.
diff --git a/posthog/models/filters/mixins/funnel.py b/posthog/models/filters/mixins/funnel.py
new file mode 100644
index 0000000000000..6cebbae8d7fe6
--- /dev/null
+++ b/posthog/models/filters/mixins/funnel.py
@@ -0,0 +1,44 @@
+from typing import Optional
+
+from posthog.constants import FUNNEL_STEP, FUNNEL_WINDOW_DAYS
+from posthog.models.filters.mixins.base import BaseParamMixin
+from posthog.models.filters.mixins.utils import cached_property, include_dict
+
+
+class FunnelWindowDaysMixin(BaseParamMixin):
+    @cached_property
+    def funnel_window_days(self) -> Optional[int]:
+        _days = int(self._data.get(FUNNEL_WINDOW_DAYS, "0"))
+        if _days == 0:
+            return None
+        return _days
+
+    @include_dict
+    def funnel_window_days_to_dict(self):
+        return {FUNNEL_WINDOW_DAYS: self.funnel_window_days} if self.funnel_window_days else {}
+
+    @staticmethod
+    def milliseconds_from_days(days):
+        milliseconds, seconds, minutes, hours = [1000, 60, 60, 24]
+        return milliseconds * seconds * minutes * hours * days
+
+    @staticmethod
+    def microseconds_from_days(days):
+        microseconds = 1000
+        return microseconds * FunnelWindowDaysMixin.milliseconds_from_days(days)
+
+
+class FunnelStep(BaseParamMixin):
+
+    # first step is 0
+    # -1 means dropoff into step 1
+    @cached_property
+    def funnel_step(self) -> Optional[int]:
+        _step = int(self._data.get(FUNNEL_STEP, "0"))
+        if _step == 0:
+            return None
+        return _step
+
+    @include_dict
+    def funnel_step_to_dict(self):
+        return {FUNNEL_STEP: self.funnel_step} if self.funnel_step else {}
diff --git a/posthog/models/filters/test/test_filter.py b/posthog/models/filters/test/test_filter.py
index 1fdc87839e5fe..f1e75244a959d 100644
--- a/posthog/models/filters/test/test_filter.py
+++ b/posthog/models/filters/test/test_filter.py
@@ -35,8 +35,7 @@ def test_to_dict(self):
             }
         )
         self.assertCountEqual(
-            list(filter.to_dict().keys()),
-            ["events", "display", "compare", "insight", "date_from", "funnel_window_days",],
+            list(filter.to_dict().keys()), ["events", "display", "compare", "insight", "date_from"],
         )
 
 
diff --git a/posthog/queries/test/test_funnel.py b/posthog/queries/test/test_funnel.py
index b5ba451368fd2..2c02d8d715753 100644
--- a/posthog/queries/test/test_funnel.py
+++ b/posthog/queries/test/test_funnel.py
@@ -38,6 +38,7 @@ def _single_step_funnel(self, properties=None, filters=None):
                 filters = {
                     "events": [{"id": "user signed up", "type": "events", "order": 0},],
                     "insight": INSIGHT_FUNNELS,
+                    "funnel_window_days": 14,
                 }
 
             if properties is not None:
@@ -61,6 +62,7 @@ def _basic_funnel(self, properties=None, filters=None):
                         {"id": action_credit_card.pk, "type": "actions", "order": 1},
                         {"id": action_play_movie.pk, "type": "actions", "order": 2},
                     ],
+                    "funnel_window_days": 14,
                 }
 
             if properties is not None:
@@ -239,6 +241,7 @@ def test_funnel_prop_filters_per_entity(self):
                         "properties": [{"key": "$browser", "value": "Firefox"}],
                     },
                 ],
+                "funnel_window_days": 14,
             }
             funnel = self._basic_funnel(filters=filters)
 
@@ -289,6 +292,7 @@ def test_funnel_person_prop(self):
                     {"id": action_credit_card.pk, "type": "actions", "order": 1,},
                     {"id": action_play_movie.pk, "type": "actions", "order": 2,},
                 ],
+                "funnel_window_days": 14,
             }
             funnel = self._basic_funnel(filters=filters)
 
@@ -328,6 +332,7 @@ def test_funnel_multiple_actions(self):
                         "events": [{"id": "event1", "order": 0}],
                         "actions": [{"id": action1.pk, "order": 1,}, {"id": action2.pk, "order": 2,},],
                         "insight": INSIGHT_FUNNELS,
+                        "funnel_window_days": 14,
                     }
                 ),
                 team=self.team,
@@ -347,6 +352,7 @@ def test_funnel_filter_test_accounts(self):
                         "events": [{"id": "event1", "order": 0}],
                         "insight": INSIGHT_FUNNELS,
                         FILTER_TEST_ACCOUNTS: True,
+                        "funnel_window_days": 14,
                     }
                 ),
                 team=self.team,


Code review comments:
diff_hunk: @@ -55,6 +55,16 @@
 ;
 """
 
+FUNNEL_PERSONS_BY_STEP_SQL = """
+SELECT person_id
+FROM
+({steps_per_person_query})
+WHERE {persons_steps}
+LIMIT 100
path: ee/clickhouse/sql/funnels/funnel.py
text: @user1:
I think we need an order by here to ensure `offset` works properly / gives deterministic results?

@user1:
Otherwise, this is good, general enough to be used in every kind of funnel we have :D 

@author:
we should do this ordering for all the people pagination queries