improve transfer ownership error message (#2651)

* improve transfer ownership error message

* add changelog

* simplify

* revert changeset invitation error message

* more descriptive error message

---------

Co-authored-by: Adam Rutkowski <hq@mtod.org>

CHANGELOG.md

@@ -15,6 +15,7 @@ All notable changes to this project will be documented in this file.
 - Fix breakdown API pagination when using event metrics plausible/analytics#2562
 - Automatically update all visible dashboard reports in the realtime view
 - Connect via TLS when using HTTPS scheme in ClickHouse URL plausible/analytics#2570
+- Add error message in case a transfer to an invited (but not joined) user is requested plausible/analytics#2651
 - Fix bug with [showing property breakdown with a prop filter](https://github.com/plausible/analytics/issues/1789)
 - Fix bug when combining goal and prop filters plausible/analytics#2654
 

lib/plausible/helpers/changeset.ex

@@ -0,0 +1,13 @@
+defmodule Plausible.ChangesetHelpers do
+  @moduledoc "Helper function for working with Ecto changesets"
+
+  def traverse_errors(changeset) do
+    Ecto.Changeset.traverse_errors(changeset, fn {msg, opts} ->
+      Regex.replace(~r"%{(\w+)}", msg, fn _, key ->
+        opts
+        |> Keyword.get(String.to_existing_atom(key), key)
+        |> to_string()
+      end)
+    end)
+  end
+end

lib/plausible_web/controllers/api/external_controller.ex

@@ -26,7 +26,7 @@ defmodule PlausibleWeb.Api.ExternalController do
             conn
             |> put_resp_header("x-plausible-dropped", "#{Enum.count(dropped)}")
             |> put_status(400)
-            |> json(%{errors: traverse_errors(first_invalid_changeset)})
+            |> json(%{errors: Plausible.ChangesetHelpers.traverse_errors(first_invalid_changeset)})
           else
             conn
             |> put_resp_header("x-plausible-dropped", "#{Enum.count(dropped)}")
@@ -38,7 +38,7 @@ defmodule PlausibleWeb.Api.ExternalController do
       {:error, %Ecto.Changeset{} = changeset} ->
         conn
         |> put_status(400)
-        |> json(%{errors: traverse_errors(changeset)})
+        |> json(%{errors: Plausible.ChangesetHelpers.traverse_errors(changeset)})
     end
   end
 
@@ -102,14 +102,4 @@ defmodule PlausibleWeb.Api.ExternalController do
       end
     end)
   end
-
-  defp traverse_errors(changeset) do
-    Ecto.Changeset.traverse_errors(changeset, fn {msg, opts} ->
-      Regex.replace(~r"%{(\w+)}", msg, fn _, key ->
-        opts
-        |> Keyword.get(String.to_existing_atom(key), key)
-        |> to_string()
-      end)
-    end)
-  end
 end

lib/plausible_web/controllers/site/membership_controller.ex

@@ -116,22 +116,41 @@ defmodule PlausibleWeb.Site.MembershipController do
     site = Sites.get_for_user!(conn.assigns[:current_user].id, site_domain)
     user = Plausible.Auth.find_user_by(email: email)
 
-    invitation =
+    invite_result =
       Invitation.new(%{
         email: email,
         role: :owner,
         site_id: site.id,
         inviter_id: conn.assigns[:current_user].id
       })
-      |> Repo.insert!()
+      |> Repo.insert()
-      |> Repo.preload([:site, :inviter])
 
-    PlausibleWeb.Email.ownership_transfer_request(invitation, user)
+    conn =
-    |> Plausible.Mailer.send()
+      case invite_result do
+        {:ok, invitation} ->
+          invitation
+          |> Repo.preload([:site, :inviter])
+          |> PlausibleWeb.Email.ownership_transfer_request(user)
+          |> Plausible.Mailer.send()
 
-    conn
+          put_flash(conn, :success, "Site transfer request has been sent to #{email}")
-    |> put_flash(:success, "Site transfer request has been sent to #{email}")
+
-    |> redirect(to: Routes.site_path(conn, :settings_people, site.domain))
+        {:error, changeset} ->
+          errors = Plausible.ChangesetHelpers.traverse_errors(changeset)
+
+          message =
+            case errors do
+              %{invitation: ["already sent" | _]} -> "Invitation has already been sent"
+              _other -> "Site transfer request to #{email} has failed"
+            end
+
+          conn
+          |> put_flash(:ttl, :timer.seconds(5))
+          |> put_flash(:error_title, "Transfer error")
+          |> put_flash(:error, message)
+      end
+
+    redirect(conn, to: Routes.site_path(conn, :settings_people, site.domain))
   end
 
   @doc """

test/plausible_web/controllers/site/membership_controller_test.exs

@@ -207,6 +207,33 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
 
       refute Repo.get_by(Plausible.Auth.Invitation, email: "john.doe@example.com")
     end
+
+    test "fails to transfer ownership to invited user with proper error message", ctx do
+      %{conn: conn, user: user} = ctx
+      site = insert(:site, members: [user])
+      invited = "john.doe@example.com"
+
+      # invite a user but don't join
+
+      conn =
+        post(conn, "/sites/#{site.domain}/memberships/invite", %{
+          email: invited,
+          role: "admin"
+        })
+
+      conn = get(recycle(conn), redirected_to(conn, 302))
+
+      assert html_response(conn, 200) =~
+               "#{invited} has been invited to #{site.domain} as an admin"
+
+      # transferring ownership to that domain now fails
+
+      conn = post(conn, "/sites/#{site.domain}/transfer-ownership", %{email: invited})
+      conn = get(recycle(conn), redirected_to(conn, 302))
+      html = html_response(conn, 200)
+      assert html =~ "Transfer error"
+      assert html =~ "Invitation has already been sent"
+    end
   end
 
   describe "PUT /sites/memberships/:id/role/:new_role" do