Allow site transfer between different teams of the same user

lib/plausible/site/admin.ex

@@ -145,8 +145,8 @@ defmodule Plausible.SiteAdmin do
       {:error, :user_not_found} ->
         {:error, "User could not be found"}
 
-      {:error, :transfer_to_self} ->
-        {:error, "User is already an owner of one of the sites"}
+      {:error, %Ecto.Changeset{}} ->
+        {:error, "Site transfer request has failed for one of the sites"}
     end
   end
 
@@ -169,7 +169,7 @@ defmodule Plausible.SiteAdmin do
         {:error, "User could not be found"}
 
       {:error, :transfer_to_self} ->
-        {:error, "User is already an owner of one of the sites"}
+        {:error, "The site is already in the picked team"}
 
       {:error, :no_plan} ->
         {:error, "The new owner does not have a subscription"}

lib/plausible/site/memberships/accept_invitation.ex

@@ -33,6 +33,7 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do
           | Billing.Quota.Limits.over_limits_error()
           | Ecto.Changeset.t()
           | :no_plan
+          | :transfer_to_self
           | :multiple_teams
           | :permission_denied
 
@@ -75,8 +76,8 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do
   defp transfer_ownership(site, new_owner, team) do
     site = Repo.preload(site, :team)
 
-    with :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_owner, :owner),
-         {:ok, new_team} <- maybe_get_team(new_owner, team),
+    with {:ok, new_team} <- maybe_get_team(new_owner, team),
+         :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_team, :owner),
          :ok <- check_can_transfer_site(new_team, new_owner),
          :ok <- Teams.Invitations.ensure_can_take_ownership(site, new_team),
          :ok <- Teams.Invitations.transfer_site(site, new_team) do
@@ -89,8 +90,8 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do
   defp do_accept_ownership_transfer(site_transfer, new_owner, team) do
     site = Repo.preload(site_transfer.site, :team)
 
-    with :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_owner, :owner),
-         {:ok, new_team} <- maybe_get_team(new_owner, team),
+    with {:ok, new_team} <- maybe_get_team(new_owner, team),
+         :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_team, :owner),
          :ok <- check_can_transfer_site(new_team, new_owner),
          :ok <- Teams.Invitations.ensure_can_take_ownership(site, new_team),
          :ok <- Teams.Invitations.accept_site_transfer(site_transfer, new_team) do

lib/plausible/site/memberships/create_invitation.ex

@@ -13,8 +13,6 @@ defmodule Plausible.Site.Memberships.CreateInvitation do
   @type invite_error() ::
           Ecto.Changeset.t()
           | :already_a_member
-          | :transfer_to_self
-          | :no_plan
           | {:over_limit, non_neg_integer()}
           | :forbidden
 
@@ -65,12 +63,6 @@ defmodule Plausible.Site.Memberships.CreateInvitation do
              invitee_email
            ),
          invitee = Plausible.Auth.find_user_by(email: invitee_email),
-         :ok <-
-           Teams.Invitations.ensure_transfer_valid(
-             site.team,
-             invitee,
-             role
-           ),
          :ok <-
            Teams.Invitations.ensure_new_membership(
              site,

lib/plausible/teams/invitations.ex

@@ -233,10 +233,11 @@ defmodule Plausible.Teams.Invitations do
   @doc false
   def ensure_transfer_valid(_team, nil, :owner), do: :ok
 
-  def ensure_transfer_valid(team, new_owner, :owner) do
-    case Teams.Memberships.team_role(team, new_owner) do
-      {:ok, :owner} -> {:error, :transfer_to_self}
-      _ -> :ok
+  def ensure_transfer_valid(team, new_team, :owner) do
+    if team.id == new_team.id do
+      {:error, :transfer_to_self}
+    else
+      :ok
     end
   end
 

lib/plausible_web/controllers/invitation_controller.ex

@@ -41,6 +41,11 @@ defmodule PlausibleWeb.InvitationController do
         |> put_flash(:error, "Invitation missing or already accepted")
         |> redirect(to: "/sites")
 
+      {:error, :transfer_to_self} ->
+        conn
+        |> put_flash(:error, "The site is already in the current team")
+        |> redirect(to: "/sites")
+
       {:error, :permission_denied} ->
         conn
         |> put_flash(:error, "You can't add sites in the current team")

lib/plausible_web/controllers/site/membership_controller.ex

@@ -118,13 +118,6 @@ defmodule PlausibleWeb.Site.MembershipController do
         |> put_flash(:success, "Site transfer request has been sent to #{email}")
         |> redirect(external: Routes.site_path(conn, :settings_people, site.domain))
 
-      {:error, :transfer_to_self} ->
-        conn
-        |> put_flash(:ttl, :timer.seconds(5))
-        |> put_flash(:error_title, "Transfer error")
-        |> put_flash(:error, "Can't transfer ownership to existing owner")
-        |> redirect(external: Routes.site_path(conn, :settings_people, site.domain))
-
       {:error, changeset} ->
         errors = Plausible.ChangesetHelpers.traverse_errors(changeset)
 

test/plausible/site/admin_test.exs

@@ -38,14 +38,6 @@ defmodule Plausible.Site.AdminTest do
                {:error, "User could not be found"}
     end
 
-    test "new owner can't be the same as old owner", %{conn: conn, transfer_action: action} do
-      current_owner = new_user()
-      site = new_site(owner: current_owner)
-
-      assert {:error, "User is already an owner of one of the sites"} =
-               action.(conn, [site], %{"email" => current_owner.email})
-    end
-
     test "initiates ownership transfer for multiple sites in one action", %{
       conn: conn,
       transfer_action: action
@@ -81,11 +73,14 @@ defmodule Plausible.Site.AdminTest do
                {:error, "User could not be found"}
     end
 
-    test "new owner can't be the same as old owner", %{conn: conn, transfer_direct_action: action} do
+    test "new team (via owner) can't be the same as old team", %{
+      conn: conn,
+      transfer_direct_action: action
+    } do
       current_owner = new_user()
       site = new_site(owner: current_owner)
 
-      assert {:error, "User is already an owner of one of the sites"} =
+      assert {:error, "The site is already in the picked team"} =
                action.(conn, [site], %{"email" => current_owner.email})
     end
 
@@ -147,6 +142,29 @@ defmodule Plausible.Site.AdminTest do
                })
     end
 
+    test "the new owner can be the same as old owner given selected team is different", %{
+      conn: conn,
+      transfer_direct_action: action
+    } do
+      today = Date.utc_today()
+      current_owner = new_user()
+      site = new_site(owner: current_owner)
+
+      another_owner =
+        new_user()
+        |> subscribe_to_growth_plan(last_bill_date: Date.shift(today, day: -5))
+
+      another_site = new_site(owner: another_owner)
+      new_team = another_site.team
+      add_member(new_team, user: current_owner, role: :owner)
+
+      assert :ok =
+               action.(conn, [site], %{
+                 "email" => current_owner.email,
+                 "team_id" => new_team.identifier
+               })
+    end
+
     @tag :ee_only
     test "new owner's plan must accommodate the transferred site", %{
       conn: conn,

test/plausible/site/memberships/accept_invitation_test.exs

@@ -65,6 +65,24 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do
                )
     end
 
+    test "allows transferring between teams of the same owner" do
+      current_owner = new_user() |> subscribe_to_growth_plan()
+      another_owner = new_user() |> subscribe_to_growth_plan()
+
+      site1 = new_site(owner: current_owner)
+      site2 = new_site(owner: current_owner)
+
+      new_team = team_of(another_owner)
+      add_member(new_team, user: current_owner, role: :owner)
+
+      assert {:ok, _} =
+               AcceptInvitation.bulk_transfer_ownership_direct(
+                 [site1, site2],
+                 current_owner,
+                 new_team
+               )
+    end
+
     test "does not allow transferring ownership to a team where user has no permission" do
       other_owner = new_user() |> subscribe_to_growth_plan()
       other_team = team_of(other_owner)
@@ -664,6 +682,20 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do
                AcceptInvitation.accept_invitation(transfer.transfer_id, current_owner)
     end
 
+    test "allows transferring between different teams of the same owner" do
+      current_owner = new_user() |> subscribe_to_growth_plan()
+      site = new_site(owner: current_owner)
+
+      another_owner = new_user() |> subscribe_to_growth_plan()
+      new_team = team_of(another_owner)
+      add_member(new_team, user: current_owner, role: :owner)
+
+      transfer = invite_transfer(site, current_owner, inviter: current_owner)
+
+      assert {:ok, _} =
+               AcceptInvitation.accept_invitation(transfer.transfer_id, current_owner, new_team)
+    end
+
     @tag :ee_only
     test "does not allow transferring to and account without suitable plan" do
       current_owner = new_user()

test/plausible/site/memberships/create_invitation_test.exs

@@ -150,14 +150,6 @@ defmodule Plausible.Site.Memberships.CreateInvitationTest do
                CreateInvitation.create_invitation(site, inviter, invitee.email, :owner)
     end
 
-    test "does not allow transferring ownership to existing owner" do
-      inviter = new_user(email: "[email protected]")
-      site = new_site(owner: inviter)
-
-      assert {:error, :transfer_to_self} =
-               CreateInvitation.create_invitation(site, inviter, "[email protected]", :owner)
-    end
-
     test "allows creating an ownership transfer even when at team member limit" do
       inviter = new_user()
       site = new_site(owner: inviter)

test/plausible_web/controllers/invitation_controller_test.exs

@@ -110,6 +110,49 @@ defmodule PlausibleWeb.Site.InvitationControllerTest do
                "You can't add sites in the current team"
     end
 
+    test "fails when transferring to the same team", %{conn: conn, user: user} do
+      current_owner = user |> subscribe_to_growth_plan()
+      site = new_site(owner: current_owner)
+
+      transfer = invite_transfer(site, current_owner, inviter: current_owner)
+
+      conn = post(conn, "/sites/invitations/#{transfer.transfer_id}/accept")
+
+      assert redirected_to(conn, 302) == "/sites"
+
+      assert Phoenix.Flash.get(conn.assigns.flash, :error) =~
+               "The site is already in the current team"
+    end
+
+    test "allows transferring between different teams of the same owner", %{
+      conn: conn,
+      user: user
+    } do
+      current_owner = user |> subscribe_to_growth_plan()
+      site = new_site(owner: current_owner)
+
+      another_owner = new_user() |> subscribe_to_growth_plan()
+      new_team = team_of(another_owner)
+      add_member(new_team, user: current_owner, role: :owner)
+
+      transfer = invite_transfer(site, current_owner, inviter: current_owner)
+
+      conn = set_current_team(conn, new_team)
+
+      conn = post(conn, "/sites/invitations/#{transfer.transfer_id}/accept")
+
+      assert redirected_to(conn, 302) == "/#{URI.encode_www_form(site.domain)}"
+
+      assert Phoenix.Flash.get(conn.assigns.flash, :success) =~
+               "You now have access to"
+
+      refute Repo.reload(transfer)
+
+      assert_team_membership(current_owner, new_team, :owner)
+
+      assert_team_attached(site, new_team.id)
+    end
+
     @tag :ee_only
     test "fails when new owner has no plan", %{conn: conn, user: user} do
       old_owner = new_user()