Remove url and path prop keys from suggestions (#3228)

* Remove url prop key from suggestions

This commit prevents the `url` prop key from being suggested by the
props settings page. This prop is used internally for file downloads and
outbound link clicks, and doesn't need to be manually allowed.

* Document Props module

lib/plausible/props.ex

@@ -18,6 +18,11 @@ defmodule Plausible.Props do
 
   @spec allow(Plausible.Site.t(), [prop()] | prop()) ::
           {:ok, Plausible.Site.t()} | {:error, Ecto.Changeset.t()}
+  @doc """
+  Allows a prop key or a list of props keys to be included in ClickHouse
+  queries. Allowing prop keys does not affect ingestion, as we don't want any
+  data to be dropped or lost.
+  """
   def allow(site, prop_or_props) do
     site
     |> allow_changeset(prop_or_props)
@@ -33,6 +38,11 @@ defmodule Plausible.Props do
 
   @spec disallow(Plausible.Site.t(), prop()) ::
           {:ok, Plausible.Site.t()} | {:error, Ecto.Changeset.t()}
+  @doc """
+  Removes a previously allowed prop key from the allow list. This means this
+  prop key won't be included in ClickHouse queries. This doesn't drop any
+  ClickHouse data, nor affects ingestion.
+  """
   def disallow(site, prop) do
     allowed_event_props = site.allowed_event_props || []
 
@@ -71,6 +81,11 @@ 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)
+
   @spec suggest_keys_to_allow(Plausible.Site.t(), non_neg_integer()) :: [String.t()]
   @doc """
   Queries the events table to fetch the #{@max_props} most frequent prop keys
@@ -89,6 +104,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,
         group_by: uk.key,
         select: uk.key,
         order_by: {:desc, count(uk.key)},

test/plausible/props_test.exs

@@ -191,4 +191,54 @@ defmodule Plausible.PropsTest do
        ]
      }} = Plausible.Props.allow_existing_props(site)
   end
+
+  test "suggest_keys_to_allow/2 returns prop keys from events" do
+    site = insert(:site)
+
+    populate_stats(site, [
+      build(:event,
+        name: "Payment",
+        "meta.key": ["amount", "os"],
+        "meta.value": ["500", "Windows"]
+      ),
+      build(:event,
+        name: "Payment",
+        "meta.key": ["amount", "logged_in", "with_error"],
+        "meta.value": ["100", "false", "true"]
+      ),
+      build(:event,
+        name: "Payment",
+        "meta.key": ["amount", "is_customer", "first_time_customer"],
+        "meta.value": ["100", "false", "true"]
+      )
+    ])
+
+    assert ["amount", "first_time_customer", "is_customer", "logged_in", "os", "with_error"] ==
+             site |> Plausible.Props.suggest_keys_to_allow() |> Enum.sort()
+  end
+
+  test "suggest_keys_to_allow/2 does not return internal prop keys from special event types" do
+    site = insert(:site)
+
+    populate_stats(site, [
+      build(:event,
+        name: "File Download",
+        "meta.key": ["url", "logged_in"],
+        "meta.value": ["http://file.test", "false"]
+      ),
+      build(:event,
+        name: "Outbound Link: Click",
+        "meta.key": ["url", "first_time_customer"],
+        "meta.value": ["http://link.test", "true"]
+      ),
+      build(:event,
+        name: "404",
+        "meta.key": ["path", "with_error"],
+        "meta.value": ["/i-dont-exist", "true"]
+      )
+    ])
+
+    assert ["first_time_customer", "logged_in", "with_error"] ==
+             site |> Plausible.Props.suggest_keys_to_allow() |> Enum.sort()
+  end
 end