Enforce goals unique (#3343)

* Enforce goals unique

* Remove unnecessary alias

* Skip tests that can no longer run anymore

To run, make sure the migration from
priv/repo/migrations/20230914071245_goals_unique.exs
is rolled back.

* Use separate transactions for the migration

* Update priv/repo/migrations/20230914071245_goals_unique.exs

Co-authored-by: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>

---------

Co-authored-by: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>

lib/plausible/goal/schema.ex

@@ -42,6 +42,8 @@ defmodule Plausible.Goal do
     |> validate_event_name_and_page_path()
     |> update_change(:event_name, &String.trim/1)
     |> update_change(:page_path, &String.trim/1)
+    |> unique_constraint(:event_name, name: :goals_event_name_unique)
+    |> unique_constraint(:page_path, name: :goals_page_path_unique)
     |> validate_length(:event_name, max: 120)
     |> check_constraint(:event_name,
       name: :check_event_name_or_page_path,

priv/repo/migrations/20230914071245_goals_unique.exs

@@ -0,0 +1,50 @@
+defmodule Plausible.Repo.Migrations.GoalsUnique do
+  use Ecto.Migration
+
+  @disable_ddl_transaction true
+  @disable_migration_lock true
+
+  def up do
+    execute """
+    DELETE
+    FROM
+    goals g
+    WHERE
+    g.id NOT IN (
+      SELECT
+      min(g2.id)
+      FROM
+      goals g2
+      GROUP BY
+      (g2.site_id,
+        CASE
+        WHEN g2.page_path IS NOT NULL THEN g2.page_path
+        WHEN g2.event_name IS NOT NULL THEN g2.event_name
+        END )
+    )
+    AND g.id NOT IN (
+      SELECT fs.goal_id
+      FROM funnel_steps fs
+    );
+    """
+
+    create(
+      unique_index(:goals, [:site_id, :event_name],
+        where: "event_name IS NOT NULL",
+        name: :goals_event_name_unique
+      )
+    )
+
+    create(
+      unique_index(:goals, [:site_id, :page_path],
+        where: "page_path IS NOT NULL",
+        name: :goals_page_path_unique
+      )
+    )
+  end
+
+  def down do
+    drop(unique_index(:goals, [:user_id, :event_name], name: :goals_event_name_unique))
+    drop(unique_index(:goals, [:user_id, :page_path], name: :goals_page_path_unique))
+  end
+end

test/plausible/data_migration/rewrite_funnel_dupes_test.exs

@@ -6,6 +6,7 @@ defmodule Plausible.DataMigration.RewriteFunnelDupesTest do
   import ExUnit.CaptureIO
 
   for goal_type <- ["event_name", "page_path"] do
+    @tag :skip
     test "deletes a funnel that cannot be cleaned up from dupe goals (#{goal_type})" do
       site = insert(:site)
 
@@ -25,6 +26,7 @@ defmodule Plausible.DataMigration.RewriteFunnelDupesTest do
       refute Repo.reload(funnel)
     end
 
+    @tag :skip
     test "reduces a funnel if possible (#{goal_type})" do
       site = insert(:site)
 
@@ -50,6 +52,7 @@ defmodule Plausible.DataMigration.RewriteFunnelDupesTest do
     end
   end
 
+  @tag :skip
   test "dupe names in mixed goal types don't matter" do
     site = insert(:site)
 
@@ -69,6 +72,7 @@ defmodule Plausible.DataMigration.RewriteFunnelDupesTest do
     assert_funnel(site.id, funnel.id, gs)
   end
 
+  @tag :skip
   test "goals across multiple funnels" do
     site = insert(:site)
 

test/plausible/goals_test.exs

@@ -24,6 +24,20 @@ defmodule Plausible.GoalsTest do
     assert {"should be at most %{count} character(s)", _} = changeset.errors[:event_name]
   end
 
+  test "create/2 fails to create the same pageview goal twice" do
+    site = insert(:site)
+    {:ok, _} = Goals.create(site, %{"page_path" => "foo bar"})
+    assert {:error, changeset} = Goals.create(site, %{"page_path" => "foo bar"})
+    assert {"has already been taken", _} = changeset.errors[:page_path]
+  end
+
+  test "create/2 fails to create the same custom event goal twice" do
+    site = insert(:site)
+    {:ok, _} = Goals.create(site, %{"event_name" => "foo bar"})
+    assert {:error, changeset} = Goals.create(site, %{"event_name" => "foo bar"})
+    assert {"has already been taken", _} = changeset.errors[:event_name]
+  end
+
   test "create/2 sets site.updated_at for revenue goal" do
     site_1 = insert(:site, updated_at: DateTime.add(DateTime.utc_now(), -3600))
     {:ok, _goal_1} = Goals.create(site_1, %{"event_name" => "Checkout", "currency" => "BRL"})
@@ -170,7 +184,7 @@ defmodule Plausible.GoalsTest do
     assert Enum.count(f1.steps) == 2
 
     refute Plausible.Funnels.get(site.id, f2.id)
-    assert Repo.all(from fs in Plausible.Funnel.Step, where: fs.funnel_id == ^f2.id) == []
+    assert Repo.all(from(fs in Plausible.Funnel.Step, where: fs.funnel_id == ^f2.id)) == []
 
     assert [^g3, ^g2] = Goals.for_site(site)
   end