From 1cc5ac0b11de11bb1e2931a39b93caab9f050526 Mon Sep 17 00:00:00 2001
From: Uku Taht <Uku.taht@gmail.com>
Date: Thu, 9 Feb 2023 15:14:09 +0200
Subject: [PATCH] Fix bug with multiple ARRAY JOIN query (#2653)

* Fix bug with multiple ARRAY JOIN query

* Add changelog entry

* Remove 'bug' label from test description

* Simplify pattern match

Co-authored-by: Adam Rutkowski <hq@mtod.org>

* Rename has_join_with_table -> joins_table?

Co-authored-by: Adam Rutkowski <hq@mtod.org>

---------

Co-authored-by: Adam Rutkowski <hq@mtod.org>
---
 CHANGELOG.md                                  |  1 +
 lib/plausible/stats/breakdown.ex              | 34 +++++++-------
 .../api/stats_controller/conversions_test.exs | 45 +++++++++++++++++++
 3 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9ae7b8478..575abdb38 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file.
 - Fix breakdown API pagination when using event metrics plausible/analytics#2562
 - Automatically update all visible dashboard reports in the realtime view
 - Connect via TLS when using HTTPS scheme in ClickHouse URL plausible/analytics#2570
+- Fix bug with [showing property breakdown with a prop filter](https://github.com/plausible/analytics/issues/1789)
 
 ### Changed
 - Reject events with long URIs and data URIs plausible/analytics#2536
diff --git a/lib/plausible/stats/breakdown.ex b/lib/plausible/stats/breakdown.ex
index 77842528e..395b2a70c 100644
--- a/lib/plausible/stats/breakdown.ex
+++ b/lib/plausible/stats/breakdown.ex
@@ -316,20 +316,13 @@ defmodule Plausible.Stats.Breakdown do
     end
   end
 
-  defp do_group_by(
-         %Ecto.Query{
-           from: %Ecto.Query.FromExpr{source: {"events", _}},
-           joins: [%Ecto.Query.JoinExpr{source: {"meta", _}}]
-         } = q,
-         "event:props:" <> prop
-       ) do
-    from(
-      [e, meta] in q,
-      group_by: e.name,
-      where: meta.key == ^prop,
-      group_by: meta.value,
-      select_merge: %{^prop => meta.value},
-      order_by: {:asc, meta.value}
+  defp joins_table?(ecto_q, table) do
+    Enum.any?(
+      ecto_q.joins,
+      fn
+        %Ecto.Query.JoinExpr{source: {^table, _}} -> true
+        _ -> false
+      end
     )
   end
 
@@ -337,9 +330,18 @@ defmodule Plausible.Stats.Breakdown do
          %Ecto.Query{from: %Ecto.Query.FromExpr{source: {"events", _}}} = q,
          "event:props:" <> prop
        ) do
+    q =
+      if joins_table?(q, "meta") do
+        q
+      else
+        from(
+          e in q,
+          inner_lateral_join: meta in fragment("meta")
+        )
+      end
+
     from(
-      e in q,
-      inner_lateral_join: meta in fragment("meta"),
+      [e, meta] in q,
       where: meta.key == ^prop,
       group_by: meta.value,
       select_merge: %{^prop => meta.value},
diff --git a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs
index 7dbe29b76..83930e53e 100644
--- a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs
+++ b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs
@@ -352,6 +352,51 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
                }
              ]
     end
+
+    test "Property breakdown with prop and goal filter", %{conn: conn, site: site} do
+      populate_stats(site, [
+        build(:pageview, user_id: 1, utm_campaign: "campaignA"),
+        build(:event,
+          user_id: 1,
+          name: "ButtonClick",
+          "meta.key": ["variant"],
+          "meta.value": ["A"]
+        ),
+        build(:pageview, user_id: 2, utm_campaign: "campaignA"),
+        build(:event,
+          user_id: 2,
+          name: "ButtonClick",
+          "meta.key": ["variant"],
+          "meta.value": ["B"]
+        )
+      ])
+
+      insert(:goal, %{domain: site.domain, event_name: "ButtonClick"})
+
+      filters =
+        Jason.encode!(%{
+          goal: "ButtonClick",
+          props: %{variant: "A"},
+          utm_campaign: "campaignA"
+        })
+
+      prop_key = "variant"
+
+      conn =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/property/#{prop_key}?period=day&filters=#{filters}"
+        )
+
+      assert json_response(conn, 200) == [
+               %{
+                 "name" => "A",
+                 "unique_conversions" => 1,
+                 "total_conversions" => 1,
+                 "conversion_rate" => 50.0
+               }
+             ]
+    end
   end
 
   describe "GET /api/stats/:domain/conversions - with glob goals" do