From 405c33f7cd96310e450d08fa38c6ff051fb7116f Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 25 Nov 2024 09:49:48 +0100 Subject: [PATCH] Fix a regression in accepting site transfers with pending invitations (#4852) --- lib/plausible/teams/invitations.ex | 85 ++----------------- .../memberships/accept_invitation_test.exs | 58 +++---------- 2 files changed, 17 insertions(+), 126 deletions(-) diff --git a/lib/plausible/teams/invitations.ex b/lib/plausible/teams/invitations.ex index aad1426be456..2030c99f6924 100644 --- a/lib/plausible/teams/invitations.ex +++ b/lib/plausible/teams/invitations.ex @@ -75,7 +75,7 @@ defmodule Plausible.Teams.Invitations do :team, :owner, guest_memberships: [team_membership: :user], - guest_invitations: :team_invitation + guest_invitations: [team_invitation: :inviter] ]) {:ok, _} = @@ -84,19 +84,6 @@ defmodule Plausible.Teams.Invitations do end) end - def accept(invitation_id, user, now \\ NaiveDateTime.utc_now(:second)) do - case find_for_user(invitation_id, user) do - {:ok, %Teams.Invitation{} = team_invitation} -> - do_accept(team_invitation, user, now) - - {:ok, %Teams.SiteTransfer{} = site_transfer} -> - do_transfer(site_transfer, user, now) - - {:error, _} = error -> - error - end - end - def accept_invitation_sync(site_invitation, user) do site_invitation = Repo.preload( @@ -150,7 +137,7 @@ defmodule Plausible.Teams.Invitations do :team, :owner, guest_memberships: [team_membership: :user], - guest_invitations: :team_invitation + guest_invitations: [team_invitation: :inviter] ]) {:ok, site_transfer} = @@ -209,7 +196,7 @@ defmodule Plausible.Teams.Invitations do Plausible.Mailer.send(email) end - defp do_accept(team_invitation, user, now, opts \\ []) do + defp do_accept(team_invitation, user, now, opts) do send_email? = Keyword.get(opts, :send_email?, true) guest_invitations = Keyword.get(opts, :guest_invitations, team_invitation.guest_invitations) @@ -234,34 +221,6 @@ defmodule Plausible.Teams.Invitations do end) end - defp do_transfer(site_transfer, new_owner, now) do - # That's probably the most involved flow of all so far - # - if new owner does not have a team yet, create one - # - ensure the new team can take ownership of the site - # - move site to and create guest memberships in the new team - # - create editor guest membership for the old owner - # - remove old guest memberships - site_transfer = Repo.preload(site_transfer, [:initiator, site: :team]) - - with :ok <- ensure_transfer_valid(site_transfer.site.team, new_owner, :owner), - {:ok, team} <- Teams.get_or_create(new_owner), - :ok <- ensure_can_take_ownership(site_transfer.site, team) do - site = Repo.preload(site_transfer.site, guest_memberships: [team_membership: :user]) - - {:ok, _} = - Repo.transaction(fn -> - :ok = transfer_site_ownership(site, team, now) - Repo.delete!(site_transfer) - end) - - send_transfer_accepted_email(site_transfer) - - {:ok, team_membership} = Teams.Memberships.get(team, new_owner) - - {:ok, team_membership} - end - end - defp transfer_site_ownership(site, team, now) do prior_team = site.team @@ -275,7 +234,7 @@ defmodule Plausible.Teams.Invitations do old_team_invitation = old_guest_invitation.team_invitation {:ok, new_team_invitation} = - create_team_invitation(team, old_team_invitation.email, now) + create_team_invitation(team, old_team_invitation.email, old_team_invitation.inviter) {:ok, _new_guest_invitation} = create_guest_invitation(new_team_invitation, site, old_guest_invitation.role) @@ -330,8 +289,6 @@ defmodule Plausible.Teams.Invitations do ) end - # TODO: Update site lock status with SiteLocker - :ok end @@ -371,7 +328,7 @@ defmodule Plausible.Teams.Invitations do end end - defp send_transfer_accepted_email(site_transfer) do + def send_transfer_accepted_email(site_transfer) do PlausibleWeb.Email.ownership_transfer_accepted( site_transfer.email, site_transfer.initiator.email, @@ -380,38 +337,6 @@ defmodule Plausible.Teams.Invitations do |> Plausible.Mailer.send() end - defp find_for_user(invitation_id, user) do - with {:error, :invitation_not_found} <- find_invitation(invitation_id, user) do - find_site_transfer(invitation_id, user) - end - end - - defp find_invitation(invitation_id, user) do - invitation = - Teams.Invitation - |> Repo.get_by(invitation_id: invitation_id, email: user.email) - |> Repo.preload([:team, :inviter, guest_invitations: :site]) - - if invitation do - {:ok, invitation} - else - {:error, :invitation_not_found} - end - end - - defp find_site_transfer(transfer_id, user) do - site_transfer = - Teams.SiteTransfer - |> Repo.get_by(transfer_id: transfer_id, email: user.email) - |> Repo.preload([:initiator, site: :team]) - - if site_transfer do - {:ok, site_transfer} - else - {:error, :invitation_not_found} - end - end - @doc false def check_invitation_permissions(site, inviter, invitation_role, opts) do check_permissions? = Keyword.get(opts, :check_permissions, true) diff --git a/test/plausible/site/memberships/accept_invitation_test.exs b/test/plausible/site/memberships/accept_invitation_test.exs index 29a7d7693db7..88be6a24de77 100644 --- a/test/plausible/site/memberships/accept_invitation_test.exs +++ b/test/plausible/site/memberships/accept_invitation_test.exs @@ -36,6 +36,18 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do assert_no_emails_delivered() end + test "transfers ownership with pending invites" do + existing_owner = new_user() + site = new_site(owner: existing_owner) + + invite_guest(site, "some@example.com", role: :viewer, inviter: existing_owner) + + new_owner = new_user() |> subscribe_to_growth_plan() + + assert {:ok, _new_membership} = + AcceptInvitation.transfer_ownership(site, new_owner) + end + @tag :teams test "syncs ownership transfers provided memberships exist already" do site = insert(:site, memberships: []) @@ -310,52 +322,6 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do assert_guest_membership(team, site, invitee, :editor) end - @tag :teams - test "converts an invitation into a membership (TEAMS)" do - inviter = insert(:user) - invitee = insert(:user) - team = insert(:team) - site = insert(:site, team: team, members: [inviter]) - insert(:team_membership, team: team, user: inviter, role: :owner) - - _invitation = - insert(:invitation, - site_id: site.id, - inviter: inviter, - email: invitee.email, - role: :admin - ) - - team_invitation = - insert(:team_invitation, - team: team, - inviter: inviter, - email: invitee.email, - role: :guest - ) - - insert(:guest_invitation, team_invitation: team_invitation, site: site, role: :editor) - - assert {:ok, team_membership} = - Plausible.Teams.Invitations.accept(team_invitation.invitation_id, invitee) - - assert [guest_membership] = - Repo.preload(team_membership, :guest_memberships).guest_memberships - - assert guest_membership.site_id == site.id - assert team_membership.user_id == invitee.id - assert guest_membership.role == :editor - assert team_membership.role == :guest - assert team_membership.team_id == team.id - - refute Repo.reload(team_invitation) - - assert_email_delivered_with( - to: [nil: inviter.email], - subject: @subject_prefix <> "#{invitee.email} accepted your invitation to #{site.domain}" - ) - end - test "does not degrade role when trying to invite self as an owner" do user = insert(:user)