From 8c3b541971d620866e69c3e270ad2fbf01389b79 Mon Sep 17 00:00:00 2001
From: Vini Brasil <vini@hey.com>
Date: Sat, 5 Aug 2023 11:13:16 -0300
Subject: [PATCH] Allow internal prop keys in queries (#3231)

* Remove `Plausible.Sites.set_allowed_event_props/2` function

This commit removes the `Plausible.Sites.set_allowed_event_props/2`
function in favor of `Plausible.Props.allow/2`.

* Allow internal prop keys in queries

This commit allows internal prop keys by default in queries. I decided
not to include internal keys in the database, because they might change
and we'd need to migrate it again.

* remove redundant 'props.' from behaviours/index.js

---------

Co-authored-by: Robert Joonas <robertjoonas16@gmail.com>
---
 assets/js/dashboard/stats/behaviours/index.js |  2 +-
 lib/plausible/props.ex                        | 16 +++++++---
 lib/plausible/site/admin.ex                   |  2 +-
 lib/plausible/sites.ex                        |  5 ---
 lib/plausible/stats/custom_props.ex           |  3 +-
 .../api/stats_controller/conversions_test.exs | 32 ++++++++++++-------
 .../api/stats_controller/suggestions_test.exs |  4 +--
 .../controllers/stats_controller_test.exs     |  6 ++--
 8 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/assets/js/dashboard/stats/behaviours/index.js b/assets/js/dashboard/stats/behaviours/index.js
index cccf0bc592..b014744e5d 100644
--- a/assets/js/dashboard/stats/behaviours/index.js
+++ b/assets/js/dashboard/stats/behaviours/index.js
@@ -167,7 +167,7 @@ export default function Behaviours(props) {
 
   function renderProps() {
     if (site.hasProps) {
-      return <Properties site={site} query={props.query} />
+      return <Properties site={site} query={query} />
     } else if (adminAccess) {
       return (
         <FeatureSetupNotice
diff --git a/lib/plausible/props.ex b/lib/plausible/props.ex
index 7410149420..ece53135b2 100644
--- a/lib/plausible/props.ex
+++ b/lib/plausible/props.ex
@@ -81,10 +81,16 @@ defmodule Plausible.Props do
     allow(site, props_to_allow)
   end
 
-  # List of props to be ignored from suggestions. For example, `url` is used both
-  # for file downloads and outbound links, and it doesn't make sense to suggest
-  # users to allow this prop key.
-  @internal_props ~w(url path)
+  @internal_keys ~w(url path)
+  @doc """
+  Lists prop keys used internally.
+
+  These props should be allowed by default, and should not be displayed in the
+  props settings page. For example, `url` is a special prop key used for file
+  downloads and outbound links. It doesn't make sense to remove this prop key
+  from the allow list, or to suggest users to add this prop key.
+  """
+  def internal_keys, do: @internal_keys
 
   @spec suggest_keys_to_allow(Plausible.Site.t(), non_neg_integer()) :: [String.t()]
   @doc """
@@ -104,7 +110,7 @@ defmodule Plausible.Props do
     Plausible.ClickhouseRepo.all(
       from uk in subquery(unnested_keys),
         where: uk.key not in ^allowed_event_props,
-        where: uk.key not in ^@internal_props,
+        where: uk.key not in ^@internal_keys,
         group_by: uk.key,
         select: uk.key,
         order_by: {:desc, count(uk.key)},
diff --git a/lib/plausible/site/admin.ex b/lib/plausible/site/admin.ex
index e666053f48..3083098ba0 100644
--- a/lib/plausible/site/admin.ex
+++ b/lib/plausible/site/admin.ex
@@ -85,7 +85,7 @@ defmodule Plausible.SiteAdmin do
         props -> String.split(props, ~r/\s*,\s*/)
       end
 
-    Plausible.Sites.set_allowed_event_props!(site, props_list)
+    {:ok, _site} = Plausible.Props.allow(site, props_list)
 
     :ok
   end
diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex
index 7d0c375ea1..3508e1aacd 100644
--- a/lib/plausible/sites.ex
+++ b/lib/plausible/sites.ex
@@ -177,9 +177,4 @@ defmodule Plausible.Sites do
         where: sm.role == :owner
     )
   end
-
-  def set_allowed_event_props!(site, props) do
-    Ecto.Changeset.change(site, allowed_event_props: props)
-    |> Repo.update!()
-  end
 end
diff --git a/lib/plausible/stats/custom_props.ex b/lib/plausible/stats/custom_props.ex
index 73e7a402f7..b33ec2c4e2 100644
--- a/lib/plausible/stats/custom_props.ex
+++ b/lib/plausible/stats/custom_props.ex
@@ -51,7 +51,8 @@ defmodule Plausible.Stats.CustomProps do
   end
 
   def maybe_allowed_props_only(q, allowed_props) when is_list(allowed_props) do
-    from [..., m] in q, where: m.key in ^allowed_props
+    props = allowed_props ++ Plausible.Props.internal_keys()
+    from [..., m] in q, where: m.key in ^props
   end
 
   def maybe_allowed_props_only(q, nil), do: q
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 200f222dc2..d14b9b24be 100644
--- a/test/plausible_web/controllers/api/stats_controller/conversions_test.exs
+++ b/test/plausible_web/controllers/api/stats_controller/conversions_test.exs
@@ -448,14 +448,8 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
       assert [%{"prop_names" => []}] = json_response(conn, 200)
     end
 
-    test "filters out garbage prop_names",
-         %{
-           conn: conn,
-           site: site
-         } do
-      site =
-        site
-        |> Plausible.Sites.set_allowed_event_props!(["author"])
+    test "filters out garbage prop_names", %{conn: conn, site: site} do
+      {:ok, site} = Plausible.Props.allow(site, ["author"])
 
       populate_stats(site, [
         build(:event,
@@ -488,9 +482,7 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
            conn: conn,
            site: site
          } do
-      site =
-        site
-        |> Plausible.Sites.set_allowed_event_props!(["author", "logged_in"])
+      {:ok, site} = Plausible.Props.allow(site, ["author", "logged_in"])
 
       populate_stats(site, [
         build(:event,
@@ -549,6 +541,24 @@ defmodule PlausibleWeb.Api.StatsController.ConversionsTest do
       assert "OnlyGarbage" in prop_names
     end
 
+    test "does not filter out special prop keys", %{conn: conn, site: site} do
+      {:ok, site} = Plausible.Props.allow(site, ["author"])
+
+      populate_stats(site, [
+        build(:event,
+          name: "Outbound Link: Click",
+          "meta.key": ["url", "path", "first_time_customer"],
+          "meta.value": ["http://link.test", "/abc", "true"]
+        )
+      ])
+
+      insert(:goal, %{site: site, event_name: "Outbound Link: Click"})
+
+      filters = Jason.encode!(%{goal: "Outbound Link: Click"})
+      conn = get(conn, "/api/stats/#{site.domain}/conversions?period=day&filters=#{filters}")
+      assert [%{"prop_names" => ["url", "path"]}] = json_response(conn, 200)
+    end
+
     test "can filter by multiple mixed goals", %{conn: conn, site: site} do
       populate_stats(site, [
         build(:pageview, pathname: "/"),
diff --git a/test/plausible_web/controllers/api/stats_controller/suggestions_test.exs b/test/plausible_web/controllers/api/stats_controller/suggestions_test.exs
index 7fdf2d05fe..c5c108cab8 100644
--- a/test/plausible_web/controllers/api/stats_controller/suggestions_test.exs
+++ b/test/plausible_web/controllers/api/stats_controller/suggestions_test.exs
@@ -282,9 +282,7 @@ defmodule PlausibleWeb.Api.StatsController.SuggestionsTest do
       conn: conn,
       site: site
     } do
-      site =
-        site
-        |> Plausible.Sites.set_allowed_event_props!(["author"])
+      {:ok, site} = Plausible.Props.allow(site, ["author"])
 
       populate_stats(site, [
         build(:pageview,
diff --git a/test/plausible_web/controllers/stats_controller_test.exs b/test/plausible_web/controllers/stats_controller_test.exs
index 82d47099fc..ef81027186 100644
--- a/test/plausible_web/controllers/stats_controller_test.exs
+++ b/test/plausible_web/controllers/stats_controller_test.exs
@@ -171,7 +171,7 @@ defmodule PlausibleWeb.StatsControllerTest do
     end
 
     test "exports allowed event props", %{conn: conn, site: site} do
-      site = Plausible.Sites.set_allowed_event_props!(site, ["author", "logged_in"])
+      {:ok, site} = Plausible.Props.allow(site, ["author", "logged_in"])
 
       populate_stats(site, [
         build(:pageview, "meta.key": ["author"], "meta.value": ["uku"]),
@@ -277,7 +277,7 @@ defmodule PlausibleWeb.StatsControllerTest do
       conn: conn,
       site: site
     } do
-      site = Plausible.Sites.set_allowed_event_props!(site, ["author", "logged_in"])
+      {:ok, site} = Plausible.Props.allow(site, ["author", "logged_in"])
 
       populate_stats(site, [
         build(:pageview, "meta.key": ["author"], "meta.value": ["uku"]),
@@ -395,7 +395,7 @@ defmodule PlausibleWeb.StatsControllerTest do
       conn: conn,
       site: site
     } do
-      site = Plausible.Sites.set_allowed_event_props!(site, ["author", "logged_in"])
+      {:ok, site} = Plausible.Props.allow(site, ["author", "logged_in"])
 
       populate_stats(site, [
         build(:event, name: "Newsletter Signup", "meta.key": ["author"], "meta.value": ["uku"]),