From 58ffae93856ea00f49078c0581604f7c98ab1f2a Mon Sep 17 00:00:00 2001 From: Robbie Date: Mon, 20 Nov 2023 11:05:53 +0000 Subject: [PATCH] feat(insights): Hide action name on bar chart if only one series (#18718) * Hide action name on bar chart if only one series * Fix hogql trends breakdown tests * Fix old trends query tests * Fix EE tests * Fix more EE tests * Fix clickhouse trends EE --- .../views/test/test_clickhouse_trends.py | 48 ++++++------ .../insights/views/LineGraph/LineGraph.tsx | 18 +++-- .../trends/test/test_trends_query_runner.py | 59 +++++++++++---- .../insights/trends/trends_query_runner.py | 12 ++- posthog/queries/test/test_trends.py | 73 ++++++++++--------- posthog/queries/trends/breakdown.py | 6 +- 6 files changed, 131 insertions(+), 85 deletions(-) diff --git a/ee/clickhouse/views/test/test_clickhouse_trends.py b/ee/clickhouse/views/test/test_clickhouse_trends.py index 75ab015e39a15..8bf86c1524006 100644 --- a/ee/clickhouse/views/test/test_clickhouse_trends.py +++ b/ee/clickhouse/views/test/test_clickhouse_trends.py @@ -118,7 +118,7 @@ def test_includes_only_intervals_within_range(client: Client): { "action": ANY, "breakdown_value": cohort["id"], - "label": "$pageview - test cohort", + "label": "test cohort", "count": 3.0, "data": [1.0, 1.0, 1.0], # Prior to the fix this would also include '29-Aug-2021' @@ -827,14 +827,12 @@ def test_insight_trends_cumulative(self): ], ) data_response = get_trends_time_series_ok(self.client, request, self.team) - person_response = get_people_from_url_ok( - self.client, data_response["$pageview - val"]["2012-01-14"].person_url - ) + person_response = get_people_from_url_ok(self.client, data_response["val"]["2012-01-14"].person_url) - assert data_response["$pageview - val"]["2012-01-13"].value == 1 - assert data_response["$pageview - val"]["2012-01-13"].breakdown_value == "val" - assert data_response["$pageview - val"]["2012-01-14"].value == 3 - assert data_response["$pageview - val"]["2012-01-14"].label == "14-Jan-2012" + assert data_response["val"]["2012-01-13"].value == 1 + assert data_response["val"]["2012-01-13"].breakdown_value == "val" + assert data_response["val"]["2012-01-14"].value == 3 + assert data_response["val"]["2012-01-14"].label == "14-Jan-2012" assert sorted([p["id"] for p in person_response]) == sorted( [str(created_people["p1"].uuid), str(created_people["p3"].uuid)] @@ -862,12 +860,12 @@ def test_insight_trends_cumulative(self): properties=[{"type": "person", "key": "key", "value": "some_val"}], ) data_response = get_trends_time_series_ok(self.client, request, self.team) - people = get_people_from_url_ok(self.client, data_response["$pageview - val"]["2012-01-14"].person_url) + people = get_people_from_url_ok(self.client, data_response["val"]["2012-01-14"].person_url) - assert data_response["$pageview - val"]["2012-01-13"].value == 1 - assert data_response["$pageview - val"]["2012-01-13"].breakdown_value == "val" - assert data_response["$pageview - val"]["2012-01-14"].value == 3 - assert data_response["$pageview - val"]["2012-01-14"].label == "14-Jan-2012" + assert data_response["val"]["2012-01-13"].value == 1 + assert data_response["val"]["2012-01-13"].breakdown_value == "val" + assert data_response["val"]["2012-01-14"].value == 3 + assert data_response["val"]["2012-01-14"].label == "14-Jan-2012" assert sorted([p["id"] for p in people]) == sorted( [str(created_people["p1"].uuid), str(created_people["p3"].uuid)] @@ -894,12 +892,12 @@ def test_insight_trends_cumulative(self): ], ) data_response = get_trends_time_series_ok(self.client, request, self.team) - people = get_people_from_url_ok(self.client, data_response["$pageview - val"]["2012-01-14"].person_url) + people = get_people_from_url_ok(self.client, data_response["val"]["2012-01-14"].person_url) - assert data_response["$pageview - val"]["2012-01-13"].value == 1 - assert data_response["$pageview - val"]["2012-01-13"].breakdown_value == "val" - assert data_response["$pageview - val"]["2012-01-14"].value == 2 - assert data_response["$pageview - val"]["2012-01-14"].label == "14-Jan-2012" + assert data_response["val"]["2012-01-13"].value == 1 + assert data_response["val"]["2012-01-13"].breakdown_value == "val" + assert data_response["val"]["2012-01-14"].value == 2 + assert data_response["val"]["2012-01-14"].label == "14-Jan-2012" assert sorted([p["id"] for p in people]) == sorted( [str(created_people["p1"].uuid), str(created_people["p3"].uuid)] @@ -933,12 +931,10 @@ def test_breakdown_with_filter(self): properties=[{"key": "key", "value": "oh", "operator": "not_icontains"}], ) data_response = get_trends_time_series_ok(self.client, params, self.team) - person_response = get_people_from_url_ok( - self.client, data_response["sign up - val"]["2012-01-13"].person_url - ) + person_response = get_people_from_url_ok(self.client, data_response["val"]["2012-01-13"].person_url) - assert data_response["sign up - val"]["2012-01-13"].value == 1 - assert data_response["sign up - val"]["2012-01-13"].breakdown_value == "val" + assert data_response["val"]["2012-01-13"].value == 1 + assert data_response["val"]["2012-01-13"].breakdown_value == "val" assert sorted([p["id"] for p in person_response]) == sorted([str(created_people["person1"].uuid)]) @@ -950,11 +946,9 @@ def test_breakdown_with_filter(self): events=[{"id": "sign up", "name": "sign up", "type": "events", "order": 0}], ) aggregate_response = get_trends_aggregate_ok(self.client, params, self.team) - aggregate_person_response = get_people_from_url_ok( - self.client, aggregate_response["sign up - val"].person_url - ) + aggregate_person_response = get_people_from_url_ok(self.client, aggregate_response["val"].person_url) - assert aggregate_response["sign up - val"].value == 1 + assert aggregate_response["val"].value == 1 assert sorted([p["id"] for p in aggregate_person_response]) == sorted([str(created_people["person1"].uuid)]) def test_insight_trends_compare(self): diff --git a/frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx b/frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx index 8c87f776db48b..568ecadebd755 100644 --- a/frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx +++ b/frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx @@ -698,11 +698,19 @@ export function LineGraph_({ precision, autoSkip: true, callback: function _renderYLabel(_, i) { - const labelDescriptors = [ - datasets?.[0]?.actions?.[i]?.custom_name ?? datasets?.[0]?.actions?.[i]?.name, // action name - datasets?.[0]?.breakdownValues?.[i], // breakdown value - datasets?.[0]?.compareLabels?.[i], // compare value - ].filter((l) => !!l) + const labelDescriptors = ( + datasets?.[0]?.labels?.[i] + ? [ + // prefer to use the label over the action name if it exists + datasets?.[0]?.labels?.[i], + datasets?.[0]?.compareLabels?.[i], + ] + : [ + datasets?.[0]?.actions?.[i]?.custom_name ?? datasets?.[0]?.actions?.[i]?.name, // action name + datasets?.[0]?.breakdownValues?.[i], // breakdown value + datasets?.[0]?.compareLabels?.[i], // compare value + ] + ).filter((l) => !!l) return labelDescriptors.join(' - ') }, }, diff --git a/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py b/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py index f6afdfd591e85..f7499741cd51e 100644 --- a/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py @@ -420,10 +420,10 @@ def test_trends_breakdowns(self): assert len(response.results) == 4 assert breakdown_labels == ["Chrome", "Edge", "Firefox", "Safari"] - assert response.results[0]["label"] == f"$pageview - Chrome" - assert response.results[1]["label"] == f"$pageview - Edge" - assert response.results[2]["label"] == f"$pageview - Firefox" - assert response.results[3]["label"] == f"$pageview - Safari" + assert response.results[0]["label"] == f"Chrome" + assert response.results[1]["label"] == f"Edge" + assert response.results[2]["label"] == f"Firefox" + assert response.results[3]["label"] == f"Safari" assert response.results[0]["count"] == 6 assert response.results[1]["count"] == 1 assert response.results[2]["count"] == 2 @@ -479,11 +479,11 @@ def test_trends_breakdowns_histogram(self): "[32.5,40.01]", ] - assert response.results[0]["label"] == '$pageview - ["",""]' - assert response.results[1]["label"] == "$pageview - [10.0,17.5]" - assert response.results[2]["label"] == "$pageview - [17.5,25.0]" - assert response.results[3]["label"] == "$pageview - [25.0,32.5]" - assert response.results[4]["label"] == "$pageview - [32.5,40.01]" + assert response.results[0]["label"] == '["",""]' + assert response.results[1]["label"] == "[10.0,17.5]" + assert response.results[2]["label"] == "[17.5,25.0]" + assert response.results[3]["label"] == "[25.0,32.5]" + assert response.results[4]["label"] == "[32.5,40.01]" assert response.results[0]["data"] == [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] assert response.results[1]["data"] == [0, 0, 1, 1, 1, 0, 1, 0, 1, 0, 1, 0] @@ -554,14 +554,47 @@ def test_trends_breakdowns_hogql(self): assert len(response.results) == 4 assert breakdown_labels == ["Chrome", "Edge", "Firefox", "Safari"] + assert response.results[0]["label"] == f"Chrome" + assert response.results[1]["label"] == f"Edge" + assert response.results[2]["label"] == f"Firefox" + assert response.results[3]["label"] == f"Safari" + assert response.results[0]["count"] == 6 + assert response.results[1]["count"] == 1 + assert response.results[2]["count"] == 2 + assert response.results[3]["count"] == 1 + + def test_trends_breakdowns_multiple_hogql(self): + self._create_test_events() + + response = self._run_trends_query( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview"), EventsNode(event="$pageleave")], + None, + BreakdownFilter(breakdown_type=BreakdownType.hogql, breakdown="properties.$browser"), + ) + + breakdown_labels = [result["breakdown_value"] for result in response.results] + + assert len(response.results) == 8 + assert breakdown_labels == ["Chrome", "Edge", "Firefox", "Safari", "Chrome", "Edge", "Firefox", "Safari"] assert response.results[0]["label"] == f"$pageview - Chrome" assert response.results[1]["label"] == f"$pageview - Edge" assert response.results[2]["label"] == f"$pageview - Firefox" assert response.results[3]["label"] == f"$pageview - Safari" + assert response.results[4]["label"] == f"$pageleave - Chrome" + assert response.results[5]["label"] == f"$pageleave - Edge" + assert response.results[6]["label"] == f"$pageleave - Firefox" + assert response.results[7]["label"] == f"$pageleave - Safari" assert response.results[0]["count"] == 6 assert response.results[1]["count"] == 1 assert response.results[2]["count"] == 2 assert response.results[3]["count"] == 1 + assert response.results[4]["count"] == 3 + assert response.results[5]["count"] == 1 + assert response.results[6]["count"] == 1 + assert response.results[7]["count"] == 1 def test_trends_breakdowns_and_compare(self): self._create_test_events() @@ -626,10 +659,10 @@ def test_trends_breakdown_and_aggregation_query_orchestration(self): assert len(response.results) == 4 assert breakdown_labels == ["Chrome", "Edge", "Firefox", "Safari"] - assert response.results[0]["label"] == f"$pageview - Chrome" - assert response.results[1]["label"] == f"$pageview - Edge" - assert response.results[2]["label"] == f"$pageview - Firefox" - assert response.results[3]["label"] == f"$pageview - Safari" + assert response.results[0]["label"] == f"Chrome" + assert response.results[1]["label"] == f"Edge" + assert response.results[2]["label"] == f"Firefox" + assert response.results[3]["label"] == f"Safari" assert response.results[0]["data"] == [ 0, diff --git a/posthog/hogql_queries/insights/trends/trends_query_runner.py b/posthog/hogql_queries/insights/trends/trends_query_runner.py index a7ceb785f18d8..3aac186437f1c 100644 --- a/posthog/hogql_queries/insights/trends/trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/trends_query_runner.py @@ -136,7 +136,7 @@ def calculate(self): timings.extend(response.timings) - res.extend(self.build_series_response(response, series_with_extra)) + res.extend(self.build_series_response(response, series_with_extra, len(queries))) if ( self.query.trendsFilter is not None @@ -147,7 +147,7 @@ def calculate(self): return TrendsQueryResponse(results=res, timings=timings) - def build_series_response(self, response: HogQLQueryResponse, series: SeriesWithExtras): + def build_series_response(self, response: HogQLQueryResponse, series: SeriesWithExtras, series_count: int): if response.results is None: return [] @@ -246,7 +246,13 @@ def get_value(name: str, val: Any): series_object["label"] = "{} - {}".format(series_object["label"], cohort_name) series_object["breakdown_value"] = get_value("breakdown_value", val) else: - series_object["label"] = "{} - {}".format(series_object["label"], get_value("breakdown_value", val)) + # If there's multiple series, include the object label in the series label + if series_count > 1: + series_object["label"] = "{} - {}".format( + series_object["label"], get_value("breakdown_value", val) + ) + else: + series_object["label"] = get_value("breakdown_value", val) series_object["breakdown_value"] = get_value("breakdown_value", val) res.append(series_object) diff --git a/posthog/queries/test/test_trends.py b/posthog/queries/test/test_trends.py index 63b7024d3d6bf..2dfe50e24b7d9 100644 --- a/posthog/queries/test/test_trends.py +++ b/posthog/queries/test/test_trends.py @@ -474,14 +474,14 @@ def test_trends_breakdown_cumulative(self): self.team, ) - self.assertEqual(response[0]["label"], "sign up - none") + self.assertEqual(response[0]["label"], "none") self.assertEqual(response[0]["labels"][4], "1-Jan-2020") self.assertEqual(response[0]["data"], [0.0, 0.0, 0.0, 0.0, 1.0, 1.0, 1.0, 1.0]) - self.assertEqual(response[1]["label"], "sign up - other_value") + self.assertEqual(response[1]["label"], "other_value") self.assertEqual(response[1]["data"], [0.0, 0.0, 0.0, 0.0, 0.0, 1.0, 1.0, 1.0]) - self.assertEqual(response[2]["label"], "sign up - value") + self.assertEqual(response[2]["label"], "value") self.assertEqual(response[2]["data"], [0.0, 0.0, 0.0, 0.0, 1.0, 1.0, 1.0, 1.0]) def test_trends_single_aggregate_dau(self): @@ -919,13 +919,14 @@ def test_trends_breakdown_single_aggregate_cohorts(self): ) for result in event_response: - if result["label"] == "sign up - cohort1": + if result["label"] == "cohort1": self.assertEqual(result["aggregated_value"], 2) - elif result["label"] == "sign up - cohort2": + elif result["label"] == "cohort2": self.assertEqual(result["aggregated_value"], 2) - elif result["label"] == "sign up - cohort3": + elif result["label"] == "cohort3": self.assertEqual(result["aggregated_value"], 3) else: + self.assertEqual(result["label"], "all users") self.assertEqual(result["aggregated_value"], 7) def test_trends_breakdown_single_aggregate(self): @@ -3869,7 +3870,7 @@ def test_breakdown_by_empty_cohort(self): self.team, ) - self.assertEqual(event_response[0]["label"], "$pageview - all users") + self.assertEqual(event_response[0]["label"], "all users") self.assertEqual(sum(event_response[0]["data"]), 1) @also_test_with_person_on_events_v2 @@ -3935,15 +3936,15 @@ def test_breakdown_by_cohort(self): counts[res["label"]] = sum(res["data"]) break_val[res["label"]] = res["breakdown_value"] - self.assertEqual(counts["watched movie - cohort1"], 1) - self.assertEqual(counts["watched movie - cohort2"], 3) - self.assertEqual(counts["watched movie - cohort3"], 4) - self.assertEqual(counts["watched movie - all users"], 7) + self.assertEqual(counts["cohort1"], 1) + self.assertEqual(counts["cohort2"], 3) + self.assertEqual(counts["cohort3"], 4) + self.assertEqual(counts["all users"], 7) - self.assertEqual(break_val["watched movie - cohort1"], cohort.pk) - self.assertEqual(break_val["watched movie - cohort2"], cohort2.pk) - self.assertEqual(break_val["watched movie - cohort3"], cohort3.pk) - self.assertEqual(break_val["watched movie - all users"], "all") + self.assertEqual(break_val["cohort1"], cohort.pk) + self.assertEqual(break_val["cohort2"], cohort2.pk) + self.assertEqual(break_val["cohort3"], cohort3.pk) + self.assertEqual(break_val["all users"], "all") self.assertEntityResponseEqual(event_response, action_response) @@ -4085,7 +4086,7 @@ def test_breakdown_by_person_property(self): for response in event_response: if response["breakdown_value"] == "person1": self.assertEqual(response["count"], 1) - self.assertEqual(response["label"], "watched movie - person1") + self.assertEqual(response["label"], "person1") if response["breakdown_value"] == "person2": self.assertEqual(response["count"], 3) if response["breakdown_value"] == "person3": @@ -4126,7 +4127,7 @@ def test_breakdown_by_person_property_for_person_on_events(self): for response in event_response: if response["breakdown_value"] == "person1": self.assertEqual(response["count"], 1) - self.assertEqual(response["label"], "watched movie - person1") + self.assertEqual(response["label"], "person1") if response["breakdown_value"] == "person2": self.assertEqual(response["count"], 3) if response["breakdown_value"] == "person3": @@ -4666,9 +4667,9 @@ def test_trends_aggregate_by_distinct_id(self): self.team, ) self.assertEqual(daily_response[0]["data"][0], 2) - self.assertEqual(daily_response[0]["label"], "sign up - some_val") + self.assertEqual(daily_response[0]["label"], "some_val") self.assertEqual(daily_response[1]["data"][0], 1) - self.assertEqual(daily_response[1]["label"], "sign up - none") + self.assertEqual(daily_response[1]["label"], "none") # MAU with freeze_time("2019-12-31T13:00:01Z"): @@ -4809,8 +4810,8 @@ def test_breakdown_filtering(self): ) self.assertEqual(response[0]["label"], "sign up - none") - self.assertEqual(response[2]["label"], "sign up - other_value") self.assertEqual(response[1]["label"], "sign up - value") + self.assertEqual(response[2]["label"], "sign up - other_value") self.assertEqual(response[3]["label"], "no events - none") self.assertEqual(sum(response[0]["data"]), 2) @@ -4869,9 +4870,9 @@ def test_breakdown_filtering_persons(self): ), self.team, ) - self.assertEqual(response[0]["label"], "sign up - none") - self.assertEqual(response[1]["label"], "sign up - test@gmail.com") - self.assertEqual(response[2]["label"], "sign up - test@posthog.com") + self.assertEqual(response[0]["label"], "none") + self.assertEqual(response[1]["label"], "test@gmail.com") + self.assertEqual(response[2]["label"], "test@posthog.com") self.assertEqual(response[0]["count"], 1) self.assertEqual(response[1]["count"], 1) @@ -4927,9 +4928,9 @@ def test_breakdown_filtering_persons_with_action_props(self): ), self.team, ) - self.assertEqual(response[0]["label"], "sign up - none") - self.assertEqual(response[1]["label"], "sign up - test@gmail.com") - self.assertEqual(response[2]["label"], "sign up - test@posthog.com") + self.assertEqual(response[0]["label"], "none") + self.assertEqual(response[1]["label"], "test@gmail.com") + self.assertEqual(response[2]["label"], "test@posthog.com") self.assertEqual(response[0]["count"], 1) self.assertEqual(response[1]["count"], 1) @@ -5003,8 +5004,8 @@ def test_breakdown_filtering_with_properties(self): ) response = sorted(response, key=lambda x: x["label"]) - self.assertEqual(response[0]["label"], "sign up - first url") - self.assertEqual(response[1]["label"], "sign up - second url") + self.assertEqual(response[0]["label"], "first url") + self.assertEqual(response[1]["label"], "second url") self.assertEqual(sum(response[0]["data"]), 1) self.assertEqual(response[0]["breakdown_value"], "first url") @@ -5086,7 +5087,7 @@ def test_breakdown_filtering_with_properties_in_new_format(self): ) response = sorted(response, key=lambda x: x["label"]) - self.assertEqual(response[0]["label"], "sign up - second url") + self.assertEqual(response[0]["label"], "second url") self.assertEqual(sum(response[0]["data"]), 1) self.assertEqual(response[0]["breakdown_value"], "second url") @@ -5170,8 +5171,8 @@ def test_mau_with_breakdown_filtering_and_prop_filter(self): self.team, ) - self.assertEqual(event_response[0]["label"], "sign up - some_val") - self.assertEqual(event_response[1]["label"], "sign up - some_val2") + self.assertEqual(event_response[0]["label"], "some_val") + self.assertEqual(event_response[1]["label"], "some_val2") self.assertEqual(sum(event_response[0]["data"]), 2) self.assertEqual(event_response[0]["data"][5], 1) @@ -5211,8 +5212,8 @@ def test_dau_with_breakdown_filtering(self): self.team, ) - self.assertEqual(event_response[1]["label"], "sign up - other_value") - self.assertEqual(event_response[2]["label"], "sign up - value") + self.assertEqual(event_response[1]["label"], "other_value") + self.assertEqual(event_response[2]["label"], "value") self.assertEqual(sum(event_response[1]["data"]), 1) self.assertEqual(event_response[1]["data"][5], 1) @@ -5256,8 +5257,8 @@ def test_dau_with_breakdown_filtering_with_sampling(self): self.team, ) - self.assertEqual(event_response[1]["label"], "sign up - other_value") - self.assertEqual(event_response[2]["label"], "sign up - value") + self.assertEqual(event_response[1]["label"], "other_value") + self.assertEqual(event_response[2]["label"], "value") self.assertEqual(sum(event_response[1]["data"]), 1) self.assertEqual(event_response[1]["data"][5], 1) @@ -5301,7 +5302,7 @@ def test_dau_with_breakdown_filtering_with_prop_filter(self): self.team, ) - self.assertEqual(event_response[0]["label"], "sign up - other_value") + self.assertEqual(event_response[0]["label"], "other_value") self.assertEqual(sum(event_response[0]["data"]), 1) self.assertEqual(event_response[0]["data"][5], 1) # property not defined diff --git a/posthog/queries/trends/breakdown.py b/posthog/queries/trends/breakdown.py index e891190f6e310..458aabdc14198 100644 --- a/posthog/queries/trends/breakdown.py +++ b/posthog/queries/trends/breakdown.py @@ -676,7 +676,11 @@ def _breakdown_result_descriptors(self, breakdown_value, filter: Filter, entity: extra_label = self._determine_breakdown_label( breakdown_value, filter.breakdown_type, filter.breakdown, breakdown_value ) - label = "{} - {}".format(entity.name, extra_label) + if len(filter.entities) > 1: + # if there are multiple entities in the query, include the entity name in the labels + label = "{} - {}".format(entity.name, extra_label) + else: + label = extra_label additional_values = {"label": label} if filter.breakdown_type == "cohort": additional_values["breakdown_value"] = "all" if breakdown_value == ALL_USERS_COHORT_ID else breakdown_value