fix(insights): Actors Query didn't output results for the "Other" bre…

…akdown value (#23789)
PostHog · Jul 18, 2024 · 69771bf · 69771bf
1 parent d2b9cd7
commit 69771bf
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 31 deletions.
diff --git a/posthog/hogql_queries/insights/trends/breakdown.py b/posthog/hogql_queries/insights/trends/breakdown.py
@@ -274,6 +274,9 @@ def _get_actors_query_where_expr(
         histogram_bin_count: int | None = None,
         group_type_index: int | None = None,
     ):
+        if lookup_value == BREAKDOWN_OTHER_STRING_LABEL:
+            return None
+
         is_numeric_breakdown = isinstance(histogram_bin_count, int)
 
         if breakdown_type == "hogql":

diff --git a/posthog/hogql_queries/insights/trends/test/test_trends_persons.py b/posthog/hogql_queries/insights/trends/test/test_trends_persons.py
@@ -8,7 +8,7 @@
 
 from posthog.api.test.test_team import create_team
 from posthog.hogql_queries.actors_query_runner import ActorsQueryRunner
-from posthog.hogql_queries.insights.trends.breakdown import BREAKDOWN_NULL_STRING_LABEL
+from posthog.hogql_queries.insights.trends.breakdown import BREAKDOWN_NULL_STRING_LABEL, BREAKDOWN_OTHER_STRING_LABEL
 from posthog.models import Team, Cohort, GroupTypeMapping
 from posthog.models.group.util import create_group
 from posthog.models.property_definition import PropertyDefinition, PropertyType
@@ -25,6 +25,7 @@
     EventsNode,
     InsightActorsQuery,
     MathGroupTypeIndex,
+    MultipleBreakdownType,
     PropertyMathType,
     TrendsFilter,
     TrendsQuery,
@@ -393,6 +394,70 @@ def test_trends_breakdown_others_persons(self):
         self.assertEqual(get_distinct_id(result[0]), "person2")
         self.assertEqual(get_event_count(result[0]), 2)
 
+    @skip("fails, as other returns all breakdowns, even those that should be display with the breakdown_limit")
+    def test_trends_multiple_breakdowns_others_persons(self):
+        self._create_events()
+        source_query = TrendsQuery(
+            series=[EventsNode(event="$pageview")],
+            dateRange=InsightDateRange(date_from="-7d"),
+            breakdownFilter=BreakdownFilter(
+                breakdowns=[
+                    Breakdown(
+                        value="$browser",
+                    )
+                ],
+                breakdown_limit=1,
+            ),
+        )
+
+        result = self._get_actors(trends_query=source_query, day="2023-04-29", breakdown=["Chrome"])
+
+        self.assertEqual(len(result), 1)
+        self.assertEqual(get_distinct_id(result[0]), "person1")
+        self.assertEqual(get_event_count(result[0]), 2)
+
+        result = self._get_actors(
+            trends_query=source_query, day="2023-04-29", breakdown=["$$_posthog_breakdown_other_$$"]
+        )
+
+        self.assertEqual(len(result), 1)
+        self.assertEqual(get_distinct_id(result[0]), "person2")
+        self.assertEqual(get_event_count(result[0]), 2)
+
+    # TODO: remove this test once "Other" actually filters out all other values
+    def test_trends_filter_by_other(self):
+        self._create_events()
+        source_query = TrendsQuery(
+            series=[EventsNode(event="$pageview")],
+            dateRange=InsightDateRange(date_from="-7d"),
+            breakdownFilter=BreakdownFilter(
+                breakdowns=[Breakdown(value="some_property", type=MultipleBreakdownType.EVENT)],
+                breakdown_limit=1,
+            ),
+        )
+
+        result = self._get_actors(trends_query=source_query, day="2023-05-01", breakdown=[BREAKDOWN_OTHER_STRING_LABEL])
+        self.assertEqual(len(result), 3)
+
+        source_query = TrendsQuery(
+            series=[EventsNode(event="$pageview")],
+            dateRange=InsightDateRange(date_from="-7d"),
+            breakdownFilter=BreakdownFilter(
+                breakdowns=[
+                    Breakdown(value="some_property", type=MultipleBreakdownType.EVENT),
+                    Breakdown(value="$browser", type=MultipleBreakdownType.EVENT),
+                ],
+                breakdown_limit=1,
+            ),
+        )
+
+        result = self._get_actors(
+            trends_query=source_query,
+            day="2023-05-01",
+            breakdown=[BREAKDOWN_OTHER_STRING_LABEL, BREAKDOWN_OTHER_STRING_LABEL],
+        )
+        self.assertEqual(len(result), 3)
+
     def test_trends_breakdown_null_persons(self):
         self._create_events()
         source_query = TrendsQuery(
@@ -741,36 +806,6 @@ def test_trends_person_multiple_breakdown_persons(self):
 
         self.assertEqual(len(result), 0)
 
-    @skip("fails, as other returns all breakdowns, even those that should be display with the breakdown_limit")
-    def test_trends_multiple_breakdowns_others_persons(self):
-        self._create_events()
-        source_query = TrendsQuery(
-            series=[EventsNode(event="$pageview")],
-            dateRange=InsightDateRange(date_from="-7d"),
-            breakdownFilter=BreakdownFilter(
-                breakdowns=[
-                    Breakdown(
-                        value="$browser",
-                    )
-                ],
-                breakdown_limit=1,
-            ),
-        )
-
-        result = self._get_actors(trends_query=source_query, day="2023-04-29", breakdown=["Chrome"])
-
-        self.assertEqual(len(result), 1)
-        self.assertEqual(get_distinct_id(result[0]), "person1")
-        self.assertEqual(get_event_count(result[0]), 2)
-
-        result = self._get_actors(
-            trends_query=source_query, day="2023-04-29", breakdown=["$$_posthog_breakdown_other_$$"]
-        )
-
-        self.assertEqual(len(result), 1)
-        self.assertEqual(get_distinct_id(result[0]), "person2")
-        self.assertEqual(get_event_count(result[0]), 2)
-
     def test_trends_multiple_breakdown_null_persons(self):
         self._create_events()
         source_query = TrendsQuery(