From fbb0419c5f17673a360c9770095fe4db9fe6042c Mon Sep 17 00:00:00 2001
From: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Date: Tue, 14 May 2024 10:26:41 +0200
Subject: [PATCH] Make events with properties logic DRY and fix missed cloaked
 link

---
 lib/plausible/exports.ex                       | 17 ++++++++++++-----
 lib/plausible/imported.ex                      | 15 +++++++++++++++
 lib/plausible/stats/imported.ex                | 18 +++++++++++-------
 .../breakdown_test.exs                         |  2 +-
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/lib/plausible/exports.ex b/lib/plausible/exports.ex
index 8cfd183d5f44..9d4d2f9b22ca 100644
--- a/lib/plausible/exports.ex
+++ b/lib/plausible/exports.ex
@@ -468,9 +468,6 @@ defmodule Plausible.Exports do
       ]
   end
 
-  @events_with_url ["Outbound Link: Click", "Cloaked Link: Click", "File Download"]
-  @events_with_path ["404"]
-
   defp export_custom_events_q(site_id, timezone, date_range) do
     from e in sampled("events_v2"),
       where: ^export_filter(site_id, date_range),
@@ -486,11 +483,21 @@ defmodule Plausible.Exports do
         date(e.timestamp, ^timezone),
         e.name,
         selected_as(
-          fragment("if(? in ?, ?, '')", e.name, @events_with_url, get_by_key(e, :meta, "url")),
+          fragment(
+            "if(? in ?, ?, '')",
+            e.name,
+            ^Plausible.Imported.events_with_url(),
+            get_by_key(e, :meta, "url")
+          ),
           :link_url
         ),
         selected_as(
-          fragment("if(? in ?, ?, '')", e.name, @events_with_path, get_by_key(e, :meta, "path")),
+          fragment(
+            "if(? in ?, ?, '')",
+            e.name,
+            ^Plausible.Imported.events_with_path(),
+            get_by_key(e, :meta, "path")
+          ),
           :path
         ),
         visitors(e),
diff --git a/lib/plausible/imported.ex b/lib/plausible/imported.ex
index c914314a3e89..2dec0a443473 100644
--- a/lib/plausible/imported.ex
+++ b/lib/plausible/imported.ex
@@ -38,6 +38,11 @@ defmodule Plausible.Imported do
   # Maximum number of complete imports to account for when querying stats
   @max_complete_imports 5
 
+  # Events which can be filtered by url property
+  @events_with_url ["Outbound Link: Click", "Cloaked Link: Click", "File Download"]
+  # Events which can be filtered by path property
+  @events_with_path ["404"]
+
   @spec schemas() :: [module()]
   def schemas, do: @tables
 
@@ -49,6 +54,16 @@ defmodule Plausible.Imported do
     @max_complete_imports
   end
 
+  @spec events_with_url() :: [String.t()]
+  def events_with_url() do
+    @events_with_url
+  end
+
+  @spec events_with_path() :: [String.t()]
+  def events_with_path() do
+    @events_with_path
+  end
+
   @spec load_import_data(Site.t()) :: Site.t()
   def load_import_data(%{import_data_loaded: true} = site), do: site
 
diff --git a/lib/plausible/stats/imported.ex b/lib/plausible/stats/imported.ex
index 9fca14c0ae49..c6ec61c23a4e 100644
--- a/lib/plausible/stats/imported.ex
+++ b/lib/plausible/stats/imported.ex
@@ -8,6 +8,8 @@ defmodule Plausible.Stats.Imported do
   @no_ref "Direct / None"
   @not_set "(not set)"
   @none "(none)"
+  @events_with_url Plausible.Imported.events_with_url()
+  @events_with_path Plausible.Imported.events_with_path()
 
   @property_to_table_mappings %{
     "visit:source" => "imported_sources",
@@ -50,14 +52,15 @@ defmodule Plausible.Stats.Imported do
          filters: %{"event:goal" => {:is, {:event, event}}},
          property: "event:props:url"
        })
-       when event in ["Outbound Link: Click", "Cloaked Link: Click", "File Download"] do
+       when event in @events_with_url do
     true
   end
 
   defp supports_single_filter?(%Query{
-         filters: %{"event:goal" => {:is, {:event, "404"}}},
+         filters: %{"event:goal" => {:is, {:event, event}}},
          property: "event:props:path"
-       }) do
+       })
+       when event in @events_with_path do
     true
   end
 
@@ -229,17 +232,18 @@ defmodule Plausible.Stats.Imported do
          "event:props:url",
          _dim
        )
-       when event_name in ["Outbound Link: Click", "File Download"] do
+       when event_name in @events_with_url do
     where(q, [i], i.name == ^event_name)
   end
 
   defp maybe_apply_filter(
          q,
-         %{"event:goal" => {:is, {:event, "404"}}},
+         %{"event:goal" => {:is, {:event, event_name}}},
          "event:props:path",
          _dim
-       ) do
-    where(q, [i], i.name == "404")
+       )
+       when event_name in @events_with_path do
+    where(q, [i], i.name == ^event_name)
   end
 
   defp maybe_apply_filter(q, filters, property, dim) do
diff --git a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs
index 0b1ae850400f..dafdf1965b66 100644
--- a/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs
+++ b/test/plausible_web/controllers/api/external_stats_controller/breakdown_test.exs
@@ -3164,7 +3164,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do
       assert %{"source" => "Google", "events" => 1} = breakdown_and_first.("visit:source")
     end
 
-    for goal_name <- ["Outbound Link: Click", "File Download"] do
+    for goal_name <- ["Outbound Link: Click", "Cloaked Link: Click", "File Download"] do
       test "returns url breakdown for #{goal_name} goal", %{conn: conn, site: site} do
         insert(:goal, event_name: unquote(goal_name), site: site)
         site_import = insert(:site_import, site: site)