Communicate site creation edge cases to the users (#2605)

* Fingerprint DBConnection.ConnectionError in Sentry

* Check events before creating a site

* Enable sites limit screen

* Remove debugging remnant

* Fix buggy assertions

This wasn't doing what expected:

  iex(1)> Repo.exists?(Plausible.Site, domain: "foo")
  [debug] QUERY OK source="sites" db=0.6ms idle=1906.2ms
  SELECT TRUE FROM "sites" AS s0 LIMIT 1 []

* Encapsulate check to satisfy credo

* Use less technically involved error message

* Bring back e-mail to the limit error message

lib/plausible/sites.ex

@@ -1,5 +1,6 @@
 defmodule Plausible.Sites do
   use Plausible.Repo
+  alias Plausible.ClickhouseRepo
   alias Plausible.Site
   alias Plausible.Site.SharedLink
   import Ecto.Query
@@ -9,27 +10,38 @@ defmodule Plausible.Sites do
   end
 
   def create(user, params) do
-    limit = Plausible.Billing.sites_limit(user)
+    site_changeset = Site.changeset(%Site{}, params)
 
-    if owned_sites_count(user) >= limit do
-      {:error, :limit, limit}
-    else
-      site_changeset = Site.changeset(%Site{}, params)
+    Ecto.Multi.new()
+    |> Ecto.Multi.run(:limit, fn _, _ ->
+      limit = Plausible.Billing.sites_limit(user)
+      count = owned_sites_count(user)
 
-      Ecto.Multi.new()
-      |> Ecto.Multi.insert(:site, site_changeset)
-      |> Ecto.Multi.run(:site_membership, fn repo, %{site: site} ->
-        membership_changeset =
-          Site.Membership.changeset(%Site.Membership{}, %{
-            site_id: site.id,
-            user_id: user.id
-          })
-
-        repo.insert(membership_changeset)
+      if count >= limit do
+        {:error, limit}
+      else
+        {:ok, count}
+      end
+    end)
+    |> Ecto.Multi.insert(:site, site_changeset)
+    |> Ecto.Multi.run(:existing_events, fn _, _ ->
+      site_changeset
+      |> Ecto.Changeset.validate_change(:domain, fn :domain, domain ->
+        check_for_existing_events(domain, params)
       end)
-      |> maybe_start_trial(user)
-      |> Repo.transaction()
-    end
+      |> Ecto.Changeset.apply_action(:insert)
+    end)
+    |> Ecto.Multi.run(:site_membership, fn repo, %{site: site} ->
+      membership_changeset =
+        Site.Membership.changeset(%Site.Membership{}, %{
+          site_id: site.id,
+          user_id: user.id
+        })
+
+      repo.insert(membership_changeset)
+    end)
+    |> maybe_start_trial(user)
+    |> Repo.transaction()
   end
 
   defp maybe_start_trial(multi, user) do
@@ -72,6 +84,11 @@ defmodule Plausible.Sites do
     !!stats_start_date(site)
   end
 
+  def has_events?(domain) do
+    q = from e in "events", where: e.domain == ^domain, select: true, limit: 1
+    ClickhouseRepo.one(q) == true
+  end
+
   def create_shared_link(site, name, password \\ nil) do
     changes =
       SharedLink.changeset(
@@ -166,4 +183,19 @@ defmodule Plausible.Sites do
         where: sm.role == :owner
     )
   end
+
+  defp check_for_existing_events(domain, params) do
+    if has_events?(domain) do
+      Sentry.capture_message("Refused to create a site with existing events",
+        extra: %{params: params}
+      )
+
+      [
+        domain:
+          "This domain has already been taken. Perhaps one of your team members registered it? If that's not the case, please contact support@plausible.io"
+      ]
+    else
+      []
+    end
+  end
 end

lib/plausible_web/controllers/api/external_sites_controller.ex

@@ -18,7 +18,7 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
         |> put_status(400)
         |> json(serialize_errors(changeset))
 
-      {:error, :limit, limit} ->
+      {:error, :limit, limit, _} ->
         conn
         |> put_status(403)
         |> json(%{

lib/plausible_web/controllers/site_controller.ex

@@ -84,16 +84,22 @@ defmodule PlausibleWeb.SiteController do
         |> put_session(site.domain <> "_offer_email_report", true)
         |> redirect(to: Routes.site_path(conn, :add_snippet, site.domain))
 
-      {:error, :site, changeset, _} ->
+      {:error, :limit, limit, _} ->
+        render(conn, "new.html",
+          changeset: Plausible.Site.changeset(%Plausible.Site{}),
+          is_first_site: is_first_site,
+          is_at_limit: true,
+          site_limit: limit,
+          layout: {PlausibleWeb.LayoutView, "focus.html"}
+        )
+
+      {:error, _, changeset, _} ->
         render(conn, "new.html",
           changeset: changeset,
           is_first_site: is_first_site,
           is_at_limit: false,
           layout: {PlausibleWeb.LayoutView, "focus.html"}
         )
-
-      {:error, :limit, _limit} ->
-        send_resp(conn, 400, "Site limit reached")
     end
   end
 

lib/plausible_web/templates/site/new.html.eex

@@ -16,7 +16,7 @@
             </h3>
             <div class="mt-2 text-sm text-yellow-700 dark:text-yellow-300">
               <p>
-              Your account is limited to <%= @site_limit %> sites. Please contact hello@plausible.io to add more sites.
+                Your account is limited to <%= @site_limit %> sites. Please contact support@plausible.io to add more sites.
               </p>
             </div>
           </div>

test/plausible_web/controllers/site_controller_test.exs

@@ -105,7 +105,30 @@ defmodule PlausibleWeb.SiteControllerTest do
         })
 
       assert redirected_to(conn) == "/example.com/snippet"
-      assert Repo.exists?(Plausible.Site, domain: "example.com")
+      assert Repo.get_by(Plausible.Site, domain: "example.com")
+    end
+
+    test "refuses to create the site when events exist (pending deletion)", %{conn: conn} do
+      domain = "events-exist.example.com"
+
+      populate_stats(%{domain: domain}, [
+        build(:pageview)
+      ])
+
+      :inserted = eventually(fn -> {Plausible.Sites.has_events?(domain), :inserted} end)
+
+      conn =
+        post(conn, "/sites", %{
+          "site" => %{
+            "domain" => domain,
+            "timezone" => "Europe/London"
+          }
+        })
+
+      assert html = html_response(conn, 200)
+      assert html =~ "This domain has already been taken."
+      assert html =~ "please contact support"
+      refute Repo.get_by(Plausible.Site, domain: domain)
     end
 
     test "starts trial if user does not have trial yet", %{conn: conn, user: user} do
@@ -160,12 +183,16 @@ defmodule PlausibleWeb.SiteControllerTest do
       conn =
         post(conn, "/sites", %{
           "site" => %{
-            "domain" => "example.com",
+            "domain" => "over-limit.example.com",
             "timezone" => "Europe/London"
           }
         })
 
-      assert conn.status == 400
+      assert html = html_response(conn, 200)
+      assert html =~ "Upgrade required"
+      assert html =~ "Your account is limited to 3 sites"
+      assert html =~ "Please contact support"
+      refute Repo.get_by(Plausible.Site, domain: "over-limit.example.com")
     end
 
     test "allows accounts registered before 2021-05-05 to go over the limit", %{
@@ -190,7 +217,7 @@ defmodule PlausibleWeb.SiteControllerTest do
         })
 
       assert redirected_to(conn) == "/example.com/snippet"
-      assert Repo.exists?(Plausible.Site, domain: "example.com")
+      assert Repo.get_by(Plausible.Site, domain: "example.com")
     end
 
     test "allows enterprise accounts to create unlimited sites", %{
@@ -213,7 +240,7 @@ defmodule PlausibleWeb.SiteControllerTest do
         })
 
       assert redirected_to(conn) == "/example.com/snippet"
-      assert Repo.exists?(Plausible.Site, domain: "example.com")
+      assert Repo.get_by(Plausible.Site, domain: "example.com")
     end
 
     test "cleans up the url", %{conn: conn} do
@@ -226,7 +253,7 @@ defmodule PlausibleWeb.SiteControllerTest do
         })
 
       assert redirected_to(conn) == "/example.com/snippet"
-      assert Repo.exists?(Plausible.Site, domain: "example.com")
+      assert Repo.get_by(Plausible.Site, domain: "example.com")
     end
 
     test "renders form again when domain is missing", %{conn: conn} do