From 3cb089eab415099c8b8b54ce67eff4563dba686d Mon Sep 17 00:00:00 2001 From: hq1 Date: Mon, 10 Apr 2023 10:29:10 +0200 Subject: [PATCH] Migrate and freeze goals creation (#2833) * Full migration (to be submitted separately) * Do not remove `Goal.domain` just yet * Do not make Goal.site not nullable just yet * Temporarily disable goal creation * Rephrase error message * Add down migration --- lib/plausible/goals.ex | 11 +++++++- .../api/external_sites_controller.ex | 5 ++++ ...30406110926_associate-goals-with-sites.exs | 28 +++++++++++++++++++ test/plausible/goals_test.exs | 1 + .../api/external_sites_controller_test.exs | 6 ++++ .../controllers/site_controller_test.exs | 2 ++ 6 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 priv/repo/migrations/20230406110926_associate-goals-with-sites.exs diff --git a/lib/plausible/goals.ex b/lib/plausible/goals.ex index a0760e51d4..9c25c27109 100644 --- a/lib/plausible/goals.ex +++ b/lib/plausible/goals.ex @@ -5,7 +5,16 @@ defmodule Plausible.Goals do def create(site, params) do params = Map.merge(params, %{"domain" => site.domain}) - Goal.changeset(%Goal{}, params) |> Repo.insert() + Goal.changeset(%Goal{}, params) + |> Ecto.Changeset.add_error( + :event_name, + "Sorry! Due to the ongoing maintenance, adding new goals is currently paused. Please try again in one hour. Thanks for your patience!" + ) + |> Ecto.Changeset.add_error( + :page_path, + "Sorry! Due to the ongoing maintenance, adding new goals is currently paused. Please try again in one hour. Thanks for your patience!" + ) + |> Repo.insert() end def find_or_create(site, %{"goal_type" => "event", "event_name" => event_name}) do diff --git a/lib/plausible_web/controllers/api/external_sites_controller.ex b/lib/plausible_web/controllers/api/external_sites_controller.ex index bbf15c0d01..78106a849a 100644 --- a/lib/plausible_web/controllers/api/external_sites_controller.ex +++ b/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -124,6 +124,11 @@ defmodule PlausibleWeb.Api.ExternalSitesController do {:missing, param} -> H.bad_request(conn, "Parameter `#{param}` is required to create a goal") + {:error, changeset} -> + conn + |> put_status(400) + |> json(serialize_errors(changeset)) + e -> H.bad_request(conn, "Something went wrong: #{inspect(e)}") end diff --git a/priv/repo/migrations/20230406110926_associate-goals-with-sites.exs b/priv/repo/migrations/20230406110926_associate-goals-with-sites.exs new file mode 100644 index 0000000000..52ca6fd07b --- /dev/null +++ b/priv/repo/migrations/20230406110926_associate-goals-with-sites.exs @@ -0,0 +1,28 @@ +defmodule :"Elixir.Plausible.Repo.Migrations.Associate-goals-with-sites" do + use Ecto.Migration + + def up do + alter table(:goals) do + add :site_id, :integer, null: true + end + + execute """ + DELETE FROM goals g WHERE NOT EXISTS ( + SELECT 1 FROM sites s + WHERE s.domain = g.domain + ) + """ + + execute """ + UPDATE goals g SET site_id = ( + SELECT s.id FROM sites s WHERE s.domain = g.domain + ) + """ + end + + def down do + alter table(:goals) do + remove :site_id + end + end +end diff --git a/test/plausible/goals_test.exs b/test/plausible/goals_test.exs index 25f98d912f..18facbb2ba 100644 --- a/test/plausible/goals_test.exs +++ b/test/plausible/goals_test.exs @@ -3,6 +3,7 @@ defmodule Plausible.GoalsTest do alias Plausible.Goals + @tag :skip test "create/2 trims input" do site = insert(:site) {:ok, goal} = Goals.create(site, %{"page_path" => "/foo bar "}) diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 03d3f6230e..9a865a1aa0 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -245,6 +245,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "PUT /api/v1/sites/goals" do setup :create_site + @tag :skip test "can add a goal as event to a site", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/goals", %{ @@ -258,6 +259,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert res["event_name"] == "Signup" end + @tag :skip test "can add a goal as page to a site", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/goals", %{ @@ -272,6 +274,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end @tag :v2_only + @tag :skip test "can add a goal using old site_id after domain change", %{conn: conn, site: site} do old_domain = site.domain new_domain = "new.example.com" @@ -290,6 +293,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert res["event_name"] == "Signup" end + @tag :skip test "is idempotent find or create op", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/goals", %{ @@ -390,6 +394,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end end + @tag :skip describe "DELETE /api/v1/sites/goals/:goal_id" do setup :create_new_site @@ -412,6 +417,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do end @tag :v2_only + @tag :skip test "delete a goal using old site_id after domain change", %{conn: conn, site: site} do old_domain = site.domain new_domain = "new.example.com" diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index 19e28b7e95..460fa0ad35 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -620,6 +620,7 @@ defmodule PlausibleWeb.SiteControllerTest do end end + @tag :skip describe "POST /:website/goals" do setup [:create_user, :log_in, :create_site] @@ -639,6 +640,7 @@ defmodule PlausibleWeb.SiteControllerTest do assert redirected_to(conn, 302) == "/#{site.domain}/settings/goals" end + @tag :skip test "creates a custom event goal for the website", %{conn: conn, site: site} do conn = post(conn, "/#{site.domain}/goals", %{