Trim goals when creating and retrieving from the DB (#2382)

People are likely to enter (copy/paste) goals from external sources
which can lead to whitespace characters appended by accident.
That renders the goal unusable and hard to distinct visually.

Normally to fix up existing goals we would use a data migration,
but this should be good enough to check if the problem
with never appearing goals resurfaces.

lib/plausible/goal/schema.ex

@@ -30,6 +30,8 @@ defmodule Plausible.Goal do
     |> cast(attrs, [:domain, :event_name, :page_path])
     |> validate_required([:domain])
     |> validate_event_name_and_page_path()
+    |> update_change(:event_name, &String.trim/1)
+    |> update_change(:page_path, &String.trim/1)
   end
 
   defp validate_event_name_and_page_path(changeset) do

lib/plausible/goals.ex

@@ -30,14 +30,33 @@ defmodule Plausible.Goals do
 
   def find_or_create(_, %{"goal_type" => "page"}), do: {:missing, "page_path"}
 
-  def for_site(domain) do
-    Repo.all(
+  def for_domain(domain) do
+    query =
       from g in Goal,
         where: g.domain == ^domain
-    )
+
+    query
+    |> Repo.all()
+    |> Enum.map(&maybe_trim/1)
   end
 
   def delete(id) do
     Repo.one(from g in Goal, where: g.id == ^id) |> Repo.delete!()
   end
+
+  defp maybe_trim(%Goal{} = goal) do
+    # we make sure that even if we saved goals erroneously with trailing
+    # space, it's removed during fetch
+    goal
+    |> Map.update!(:event_name, &maybe_trim/1)
+    |> Map.update!(:page_path, &maybe_trim/1)
+  end
+
+  defp maybe_trim(s) when is_binary(s) do
+    String.trim(s)
+  end
+
+  defp maybe_trim(other) do
+    other
+  end
 end

lib/plausible/stats/breakdown.ex

@@ -2,6 +2,7 @@ defmodule Plausible.Stats.Breakdown do
   use Plausible.ClickhouseRepo
   import Plausible.Stats.{Base, Imported}
   alias Plausible.Stats.Query
+  alias Plausible.Goals
   @no_ref "Direct / None"
 
   @event_metrics [:visitors, :pageviews, :events]
@@ -10,7 +11,8 @@ defmodule Plausible.Stats.Breakdown do
 
   def breakdown(site, query, "event:goal", metrics, pagination) do
     {event_goals, pageview_goals} =
-      Plausible.Repo.all(from g in Plausible.Goal, where: g.domain == ^site.domain)
+      site.domain
+      |> Goals.for_domain()
       |> Enum.split_with(fn goal -> goal.event_name end)
 
     events = Enum.map(event_goals, & &1.event_name)

lib/plausible/stats/filter_suggestions.ex

@@ -118,7 +118,8 @@ defmodule Plausible.Stats.FilterSuggestions do
   end
 
   def filter_suggestions(site, _query, "goal", filter_search) do
-    Repo.all(from g in Plausible.Goal, where: g.domain == ^site.domain)
+    site.domain
+    |> Plausible.Goals.for_domain()
     |> Enum.map(fn x -> if x.event_name, do: x.event_name, else: "Visit #{x.page_path}" end)
     |> Enum.filter(fn goal ->
       String.contains?(

lib/plausible_web/controllers/site_controller.ex

@@ -213,7 +213,7 @@ defmodule PlausibleWeb.SiteController do
 
   def settings_goals(conn, _params) do
     site = conn.assigns[:site] |> Repo.preload(:custom_domain)
-    goals = Goals.for_site(site.domain)
+    goals = Goals.for_domain(site.domain)
 
     conn
     |> assign(:skip_plausible_tracking, true)

test/plausible/goals_test.exs

@@ -0,0 +1,24 @@
+defmodule Plausible.GoalsTest do
+  use Plausible.DataCase
+
+  alias Plausible.Goals
+
+  test "create/2 trims input" do
+    site = insert(:site)
+    {:ok, goal} = Goals.create(site, %{"page_path" => "/foo bar "})
+    assert goal.page_path == "/foo bar"
+
+    {:ok, goal} = Goals.create(site, %{"event_name" => "  some event name   "})
+    assert goal.event_name == "some event name"
+  end
+
+  test "for_domain/2 returns trimmed input even if it was saved with trailing whitespace" do
+    site = insert(:site)
+    insert(:goal, %{domain: site.domain, event_name: " Signup "})
+    insert(:goal, %{domain: site.domain, page_path: " /Signup "})
+
+    goals = Goals.for_domain(site.domain)
+
+    assert [%{event_name: "Signup"}, %{page_path: "/Signup"}] = goals
+  end
+end