Skip to content

Commit

Permalink
feat(insights): Hide action name on bar chart if only one series (#18718
Browse files Browse the repository at this point in the history
)

* 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
  • Loading branch information
robbie-c authored Nov 20, 2023
1 parent 1c6ec08 commit 58ffae9
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 85 deletions.
48 changes: 21 additions & 27 deletions ee/clickhouse/views/test/test_clickhouse_trends.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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)]
Expand All @@ -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)]
Expand Down Expand Up @@ -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)])

Expand All @@ -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):
Expand Down
18 changes: 13 additions & 5 deletions frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(' - ')
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 9 additions & 3 deletions posthog/hogql_queries/insights/trends/trends_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 []

Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 58ffae9

Please sign in to comment.