From 74dcd3d29b7033b94e681bca06f86d7587435801 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Wed, 13 Nov 2024 13:32:57 +0100 Subject: [PATCH] Switch listing team members view to read from Teams schemas (#4802) * Modify test utils to use teams test factories * Implement alternative routes for updating and removing membership * Implement teams read adapter for listing site members and invitees * Use new teams read adapter for Settings > People view * Add `invitation_id` column to `guest_invitations` schema * Add `invitation_id` to `GuestInvitation` schema and populate it * Sync guest invitation's invitation ID instead of team invitation * Expose guest invitation's invitation ID in sites list * Sync guest invitation invitation ID instead of team invitation in backfill * Update team consistency check to account for guest invitation IDs * Remove workaround for no invitation ID on guest invitation in `list_people` * Test listing pending invitations * Test listing memberships * Format * Test membership changes via new routes * Remove old membership altering routes * Clean up * Revert "Modify test utils to use teams test factories" This reverts commit 5eb8754782a118e66242f0198518c1ddc23d5595. * Ensure test setup provisions teams for people listing * See if we can avoid exposing user id * Revert "See if we can avoid exposing user id" This reverts commit 672429b9d1694d0b77623fac08292305e269f725. * Fix faulty member label in people list * Fix sites listing for a case of pending invite with existing pin --------- Co-authored-by: hq1 --- .../data_migration/backfill_teams.ex | 17 ++- .../data_migration/teams_consistency_check.ex | 1 + lib/plausible/teams/adapter/read/sites.ex | 76 +++++++++- lib/plausible/teams/guest_invitation.ex | 3 +- lib/plausible/teams/invitations.ex | 2 +- lib/plausible/teams/sites.ex | 20 ++- .../controllers/site/membership_controller.ex | 12 +- .../controllers/site_controller.ex | 10 +- lib/plausible_web/router.ex | 7 +- .../templates/site/settings_people.html.heex | 28 ++-- test/plausible/site/sites_test.exs | 34 ++++- .../site/membership_controller_test.exs | 139 +++++------------- .../controllers/site_controller_test.exs | 47 +++++- test/support/factory.ex | 1 + test/support/teams/test.ex | 53 +++++-- 15 files changed, 291 insertions(+), 159 deletions(-) diff --git a/lib/plausible/data_migration/backfill_teams.ex b/lib/plausible/data_migration/backfill_teams.ex index 5ffe865a743a..eb1aee2c697e 100644 --- a/lib/plausible/data_migration/backfill_teams.ex +++ b/lib/plausible/data_migration/backfill_teams.ex @@ -422,8 +422,9 @@ defmodule Plausible.DataMigration.BackfillTeams do where: ti.role == :guest, where: (gi.role == :viewer and si.role == :admin) or - (gi.role == :editor and si.role == :viewer), - select: {gi, si.role} + (gi.role == :editor and si.role == :viewer) or + is_distinct(gi.invitation_id, si.invitation_id), + select: {gi, si} ) |> @repo.all(timeout: :infinity) @@ -770,7 +771,6 @@ defmodule Plausible.DataMigration.BackfillTeams do role: :guest, inviter: first_site_invitation.inviter ) - |> Ecto.Changeset.put_change(:invitation_id, first_site_invitation.invitation_id) |> Ecto.Changeset.put_change(:inserted_at, first_site_invitation.inserted_at) |> Ecto.Changeset.put_change(:updated_at, first_site_invitation.updated_at) |> @repo.insert!( @@ -784,6 +784,7 @@ defmodule Plausible.DataMigration.BackfillTeams do site_invitation.site, translate_role(site_invitation.role) ) + |> Ecto.Changeset.put_change(:invitation_id, site_invitation.invitation_id) |> Ecto.Changeset.put_change(:inserted_at, site_invitation.inserted_at) |> Ecto.Changeset.put_change(:updated_at, site_invitation.updated_at) |> @repo.insert!() @@ -795,12 +796,14 @@ defmodule Plausible.DataMigration.BackfillTeams do end) end - defp sync_guest_invitations(guest_invitations_and_roles) do - guest_invitations_and_roles + defp sync_guest_invitations(guest_and_site_invitations) do + guest_and_site_invitations |> Enum.with_index() - |> Enum.each(fn {{guest_invitation, role}, idx} -> + |> Enum.each(fn {{guest_invitation, site_invitation}, idx} -> guest_invitation - |> Ecto.Changeset.change(role: translate_role(role)) + |> Ecto.Changeset.change() + |> Ecto.Changeset.put_change(:role, translate_role(site_invitation.role)) + |> Ecto.Changeset.put_change(:invitation_id, site_invitation.invitation_id) |> Ecto.Changeset.put_change(:updated_at, guest_invitation.updated_at) |> @repo.update!() diff --git a/lib/plausible/data_migration/teams_consistency_check.ex b/lib/plausible/data_migration/teams_consistency_check.ex index 63e59f74242d..e192b0405771 100644 --- a/lib/plausible/data_migration/teams_consistency_check.ex +++ b/lib/plausible/data_migration/teams_consistency_check.ex @@ -290,6 +290,7 @@ defmodule Plausible.DataMigration.TeamsConsitencyCheck do where: (i.role == :viewer and parent_as(:guest_invitation).role == :viewer) or (i.role == :admin and parent_as(:guest_invitation).role == :editor), + where: i.invitation_id == parent_as(:guest_invitation).invitation_id, select: 1 ) diff --git a/lib/plausible/teams/adapter/read/sites.ex b/lib/plausible/teams/adapter/read/sites.ex index 173394c202b0..76ec43022a30 100644 --- a/lib/plausible/teams/adapter/read/sites.ex +++ b/lib/plausible/teams/adapter/read/sites.ex @@ -1,11 +1,14 @@ defmodule Plausible.Teams.Adapter.Read.Sites do @moduledoc """ - Transition adapter for new schema reads + Transition adapter for new schema reads """ + import Ecto.Query + + alias Plausible.Auth alias Plausible.Repo alias Plausible.Site - alias Plausible.Auth + alias Plausible.Teams def list(user, pagination_params, opts \\ []) do if Plausible.Teams.read_team_schemas?(user) do @@ -112,6 +115,75 @@ defmodule Plausible.Teams.Adapter.Read.Sites do %{result | entries: entries} end + def list_people(site, user) do + if Plausible.Teams.read_team_schemas?(user) do + owner_membership = + from( + tm in Teams.Membership, + where: tm.team_id == ^site.team_id, + where: tm.role == :owner, + select: %Plausible.Site.Membership{ + user_id: tm.user_id, + role: tm.role + } + ) + |> Repo.one!() + + memberships = + from( + gm in Teams.GuestMembership, + inner_join: tm in assoc(gm, :team_membership), + where: gm.site_id == ^site.id, + select: %Plausible.Site.Membership{ + user_id: tm.user_id, + role: + fragment( + """ + CASE + WHEN ? = 'editor' THEN 'admin' + ELSE ? + END + """, + gm.role, + gm.role + ) + } + ) + |> Repo.all() + + memberships = Repo.preload([owner_membership | memberships], :user) + + invitations = + from( + gi in Teams.GuestInvitation, + inner_join: ti in assoc(gi, :team_invitation), + where: gi.site_id == ^site.id, + select: %Plausible.Auth.Invitation{ + invitation_id: gi.invitation_id, + email: ti.email, + role: + fragment( + """ + CASE + WHEN ? = 'editor' THEN 'admin' + ELSE ? + END + """, + gi.role, + gi.role + ) + } + ) + |> Repo.all() + + %{memberships: memberships, invitations: invitations} + else + site + |> Repo.preload([:invitations, memberships: :user]) + |> Map.take([:memberships, :invitations]) + end + end + defp maybe_filter_by_domain(query, domain) when byte_size(domain) >= 1 and byte_size(domain) <= 64 do where(query, [s], ilike(s.domain, ^"%#{domain}%")) diff --git a/lib/plausible/teams/guest_invitation.ex b/lib/plausible/teams/guest_invitation.ex index 9f0e710025cf..815a608fabac 100644 --- a/lib/plausible/teams/guest_invitation.ex +++ b/lib/plausible/teams/guest_invitation.ex @@ -8,6 +8,7 @@ defmodule Plausible.Teams.GuestInvitation do import Ecto.Changeset schema "guest_invitations" do + field :invitation_id, :string field :role, Ecto.Enum, values: [:viewer, :editor] belongs_to :site, Plausible.Site @@ -17,7 +18,7 @@ defmodule Plausible.Teams.GuestInvitation do end def changeset(team_invitation, site, role) do - %__MODULE__{} + %__MODULE__{invitation_id: Nanoid.generate()} |> cast(%{role: role}, [:role]) |> validate_required(:role) |> put_assoc(:team_invitation, team_invitation) diff --git a/lib/plausible/teams/invitations.ex b/lib/plausible/teams/invitations.ex index ec88523bea47..92ce7002b00a 100644 --- a/lib/plausible/teams/invitations.ex +++ b/lib/plausible/teams/invitations.ex @@ -63,7 +63,7 @@ defmodule Plausible.Teams.Invitations do site_invitation.inviter ) - guest_invitation.team_invitation + guest_invitation |> Ecto.Changeset.change(invitation_id: site_invitation.invitation_id) |> Repo.update!() end diff --git a/lib/plausible/teams/sites.ex b/lib/plausible/teams/sites.ex index 84c63757a41a..4d2e4995ca1f 100644 --- a/lib/plausible/teams/sites.ex +++ b/lib/plausible/teams/sites.ex @@ -134,7 +134,8 @@ defmodule Plausible.Teams.Sites do select: %{ site_id: s.id, entry_type: "site", - invitation_id: 0, + guest_invitation_id: 0, + team_invitation_id: 0, role: tm.role, transfer_id: 0 } @@ -147,7 +148,8 @@ defmodule Plausible.Teams.Sites do select: %{ site_id: s.id, entry_type: "site", - invitation_id: 0, + guest_invitation_id: 0, + team_invitation_id: 0, role: fragment( """ @@ -184,7 +186,8 @@ defmodule Plausible.Teams.Sites do select: %{ site_id: s.id, entry_type: "invitation", - invitation_id: ti.id, + guest_invitation_id: gi.id, + team_invitation_id: ti.id, role: fragment( """ @@ -216,7 +219,8 @@ defmodule Plausible.Teams.Sites do select: %{ site_id: s.id, entry_type: "invitation", - invitation_id: 0, + guest_invitation_id: 0, + team_invitation_id: 0, role: "owner", transfer_id: st.id } @@ -234,7 +238,9 @@ defmodule Plausible.Teams.Sites do left_join: up in Site.UserPreference, on: up.site_id == s.id and up.user_id == ^user.id, left_join: ti in Teams.Invitation, - on: ti.id == u.invitation_id, + on: ti.id == u.team_invitation_id, + left_join: gi in Teams.GuestInvitation, + on: gi.id == u.guest_invitation_id, left_join: st in Teams.SiteTransfer, on: st.id == u.transfer_id, select: %{ @@ -244,10 +250,12 @@ defmodule Plausible.Teams.Sites do fragment( """ CASE + WHEN ? IS NOT NULL THEN 'invitation' WHEN ? IS NOT NULL THEN 'pinned_site' ELSE ? END """, + gi.id, up.pinned_at, u.entry_type ), @@ -263,7 +271,7 @@ defmodule Plausible.Teams.Sites do ], invitations: [ %Plausible.Auth.Invitation{ - invitation_id: coalesce(ti.invitation_id, st.transfer_id), + invitation_id: coalesce(gi.invitation_id, st.transfer_id), email: coalesce(ti.email, st.email), role: type(u.role, ^@role_type), site_id: s.id, diff --git a/lib/plausible_web/controllers/site/membership_controller.ex b/lib/plausible_web/controllers/site/membership_controller.ex index 7c69ec3db5d0..d17849f77317 100644 --- a/lib/plausible_web/controllers/site/membership_controller.ex +++ b/lib/plausible_web/controllers/site/membership_controller.ex @@ -151,10 +151,12 @@ defmodule PlausibleWeb.Site.MembershipController do |> Enum.map(fn {k, v} -> {v, k} end) |> Enum.into(%{}) - def update_role(conn, %{"id" => id, "new_role" => new_role_str}) do + def update_role_by_user(conn, %{"id" => user_id, "new_role" => new_role_str}) do %{site: site, current_user: current_user, current_user_role: current_user_role} = conn.assigns - membership = Repo.get!(Membership, id) |> Repo.preload(:user) + membership = + Membership |> Repo.get_by!(user_id: user_id, site_id: site.id) |> Repo.preload(:user) + new_role = Map.fetch!(@role_mappings, new_role_str) can_grant_role? = @@ -202,13 +204,13 @@ defmodule PlausibleWeb.Site.MembershipController do defp can_grant_role_to_other?(:admin, :viewer), do: true defp can_grant_role_to_other?(_, _), do: false - def remove_member(conn, %{"id" => id}) do - site = conn.assigns[:site] + def remove_member_by_user(conn, %{"id" => user_id} = _params) do + site = conn.assigns.site site_id = site.id membership_q = from m in Membership, - where: m.id == ^id, + where: m.user_id == ^user_id, where: m.site_id == ^site_id, inner_join: user in assoc(m, :user), inner_join: site in assoc(m, :site), diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 970d8983a6dc..b2f8620f8884 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -122,13 +122,17 @@ defmodule PlausibleWeb.SiteController do end def settings_people(conn, _params) do - site = - conn.assigns[:site] - |> Repo.preload(memberships: :user, invitations: []) + current_user = conn.assigns.current_user + site = conn.assigns.site + + %{memberships: memberships, invitations: invitations} = + Plausible.Teams.Adapter.Read.Sites.list_people(site, current_user) conn |> render("settings_people.html", site: site, + memberships: memberships, + invitations: invitations, dogfood_page_path: "/:dashboard/settings/people", layout: {PlausibleWeb.LayoutView, "site_settings.html"} ) diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 6cedfd09336e..335fe37ca39b 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -451,8 +451,11 @@ defmodule PlausibleWeb.Router do get "/sites/:domain/transfer-ownership", Site.MembershipController, :transfer_ownership_form post "/sites/:domain/transfer-ownership", Site.MembershipController, :transfer_ownership - put "/sites/:domain/memberships/:id/role/:new_role", Site.MembershipController, :update_role - delete "/sites/:domain/memberships/:id", Site.MembershipController, :remove_member + put "/sites/:domain/memberships/u/:id/role/:new_role", + Site.MembershipController, + :update_role_by_user + + delete "/sites/:domain/memberships/u/:id", Site.MembershipController, :remove_member_by_user get "/sites/:domain/weekly-report/unsubscribe", UnsubscribeController, :weekly_report get "/sites/:domain/monthly-report/unsubscribe", UnsubscribeController, :monthly_report diff --git a/lib/plausible_web/templates/site/settings_people.html.heex b/lib/plausible_web/templates/site/settings_people.html.heex index 4fe73ef766f6..fdb43ddbb0da 100644 --- a/lib/plausible_web/templates/site/settings_people.html.heex +++ b/lib/plausible_web/templates/site/settings_people.html.heex @@ -14,8 +14,8 @@
    - <%= for membership <- @site.memberships do %> -
  • + <%= for membership <- @memberships do %> +
  • - <%= membership.role |> Atom.to_string() |> String.capitalize() %> + <%= membership.role |> to_string() |> String.capitalize() %> - <.tile :if={Enum.count(@site.invitations) > 0}> + <.tile :if={Enum.count(@invitations) > 0}> <:title>Pending invitations <:subtitle>Waiting for new members to accept their invitations - <.table rows={@site.invitations}> + <.table + rows={@invitations} + row_attrs={fn invitation -> %{id: "invitation-#{invitation.invitation_id}"} end} + > <:thead> <.th>Email <.th hide_on_mobile>Role diff --git a/test/plausible/site/sites_test.exs b/test/plausible/site/sites_test.exs index cb5b4e2e7fd0..e1699cbd1fb1 100644 --- a/test/plausible/site/sites_test.exs +++ b/test/plausible/site/sites_test.exs @@ -243,7 +243,12 @@ defmodule Plausible.SitesTest do assert %{entries: [%{domain: "one.example.com"}]} = Sites.list(user1, %{}) - assert %{entries: [%{domain: "two.example.com"}, %{domain: "one.example.com"}]} = + assert %{ + entries: [ + %{domain: "two.example.com", entry_type: "invitation"}, + %{domain: "one.example.com", entry_type: "pinned_site"} + ] + } = Sites.list_with_invitations(user1, %{}) assert %{entries: [%{domain: "one.example.com"}]} = Plausible.Teams.Sites.list(user1, %{}) @@ -252,6 +257,33 @@ defmodule Plausible.SitesTest do Plausible.Teams.Sites.list_with_invitations(user1, %{}) end + test "pinned site on active invitation" do + user1 = new_user(email: "user1@example.com") + user2 = new_user(email: "user2@example.com") + + site1 = new_site(domain: "one.example.com", owner: user2) + + add_guest(site1, user: user1, role: :editor) + {:ok, _} = Sites.toggle_pin(user1, site1) + revoke_membership(site1, user1) + + invite_guest(site1, user1, role: :editor, inviter: user2) + + assert %{entries: []} = Sites.list(user1, %{}) + + assert %{ + entries: [ + %{domain: "one.example.com", entry_type: "invitation"} + ] + } = + Sites.list_with_invitations(user1, %{}) + + assert %{entries: []} = Plausible.Teams.Sites.list(user1, %{}) + + assert %{entries: [%{domain: "one.example.com", entry_type: "invitation"}]} = + Plausible.Teams.Sites.list_with_invitations(user1, %{}) + end + test "puts invitations first, pinned sites second, sites last" do user1 = new_user() user2 = new_user() diff --git a/test/plausible_web/controllers/site/membership_controller_test.exs b/test/plausible_web/controllers/site/membership_controller_test.exs index bb7edd25071a..ba4f761fa36a 100644 --- a/test/plausible_web/controllers/site/membership_controller_test.exs +++ b/test/plausible_web/controllers/site/membership_controller_test.exs @@ -4,6 +4,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do use Plausible.Repo use Bamboo.Test + use Plausible.Teams.Test import Plausible.Test.Support.HTML @subject_prefix if ee?(), do: "[Plausible Analytics] ", else: "[Plausible CE] " @@ -288,24 +289,17 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do end describe "PUT /sites/memberships/:id/role/:new_role" do - test "updates a site member's role", %{conn: conn, user: user} do - admin = insert(:user) - - site = - insert(:site, - memberships: [ - build(:site_membership, user: user, role: :owner), - build(:site_membership, user: admin, role: :admin) - ] - ) + test "updates a site member's role by user id", %{conn: conn, user: user} do + site = new_site(owner: user) + collaborator = add_guest(site, role: :editor) + assert_team_membership(collaborator, site.team, :editor) - membership = Repo.get_by(Plausible.Site.Membership, user_id: admin.id) + put(conn, "/sites/#{site.domain}/memberships/u/#{collaborator.id}/role/viewer") - put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer") + assert_team_membership(collaborator, site.team, :viewer) - membership = Repo.reload!(membership) - - assert membership.role == :viewer + old_model_membership = Repo.get_by(Plausible.Site.Membership, user_id: collaborator.id) + assert old_model_membership.role == :viewer end @tag :teams @@ -327,9 +321,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do guest_membership = insert(:guest_membership, team_membership: team_membership, site: site, role: :editor) - membership = Repo.get_by(Plausible.Site.Membership, user_id: admin.id) - - put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer") + put(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}/role/viewer") assert Repo.reload!(guest_membership).role == :viewer end @@ -340,11 +332,9 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do } do site = insert(:site, memberships: [build(:site_membership, user: user, role: :admin)]) - membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id) - - conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer") + conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/viewer") - membership = Repo.reload!(membership) + membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id) assert membership.role == :viewer assert redirected_to(conn) == "/#{URI.encode_www_form(site.domain)}" @@ -354,23 +344,12 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do conn: conn, user: user } do - admin = insert(:user) - - site = - insert(:site, - memberships: [ - build(:site_membership, user: user, role: :owner), - build(:site_membership, user: admin, role: :admin) - ] - ) - - membership = Repo.get_by(Plausible.Site.Membership, user_id: admin.id) - - conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/owner") + site = new_site() + admin = add_guest(site, user: user, role: :editor) - membership = Repo.reload!(membership) + conn = put(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}/role/owner") - assert membership.role == :admin + assert_team_membership(user, site.team, :editor) assert Phoenix.Flash.get(conn.assigns.flash, :error) == "You are not allowed to grant the owner role" @@ -382,11 +361,9 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do } do site = insert(:site, memberships: [build(:site_membership, user: user, role: :owner)]) - membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id) - - conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/admin") + conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/admin") - membership = Repo.reload!(membership) + membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id) assert membership.role == :owner @@ -398,44 +375,24 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do conn: conn, user: user } do - viewer = insert(:user) + site = new_site() - site = - insert(:site, - memberships: [ - build(:site_membership, user: user, role: :admin), - build(:site_membership, user: viewer, role: :viewer) - ] - ) + add_guest(site, user: user, role: :editor) + viewer = add_guest(site, user: new_user(), role: :viewer) - viewer_membership = Repo.get_by(Plausible.Site.Membership, user_id: viewer.id) + conn = put(conn, "/sites/#{site.domain}/memberships/u/#{viewer.id}/role/admin") - conn = put(conn, "/sites/#{site.domain}/memberships/#{viewer_membership.id}/role/admin") - - viewer_membership = Repo.reload!(viewer_membership) - - assert viewer_membership.role == :admin + assert_team_membership(viewer, site.team, :editor) assert redirected_to(conn) == "/#{URI.encode_www_form(site.domain)}/settings/people" end test "admin can't make themselves an owner", %{conn: conn, user: user} do - owner = insert(:user) + site = new_site() + add_guest(site, user: user, role: :editor) - site = - insert(:site, - memberships: [ - build(:site_membership, user: owner, role: :owner), - build(:site_membership, user: user, role: :admin) - ] - ) - - membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id) + conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/owner") - conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/owner") - - membership = Repo.reload!(membership) - - assert membership.role == :admin + assert_team_membership(user, site.team, :editor) assert Phoenix.Flash.get(conn.assigns.flash, :error) == "You are not allowed to grant the owner role" @@ -443,20 +400,11 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do end describe "DELETE /sites/:domain/memberships/:id" do - test "removes a member from a site", %{conn: conn, user: user} do - admin = insert(:user) + test "removes a member from a site by user id", %{conn: conn, user: user} do + site = new_site(owner: user) + admin = add_guest(site, role: :editor) - site = - insert(:site, - memberships: [ - build(:site_membership, user: user, role: :owner), - build(:site_membership, user: admin, role: :admin) - ] - ) - - membership = Enum.find(site.memberships, &(&1.role == :admin)) - - conn = delete(conn, "/sites/#{site.domain}/memberships/#{membership.id}") + conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}") assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed" refute Repo.exists?(from sm in Plausible.Site.Membership, where: sm.user_id == ^admin.id) @@ -481,9 +429,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do guest_membership = insert(:guest_membership, team_membership: team_membership, site: site, role: :editor) - membership = Enum.find(site.memberships, &(&1.role == :admin)) - - conn = delete(conn, "/sites/#{site.domain}/memberships/#{membership.id}") + conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}") assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed" refute Repo.reload(guest_membership) @@ -529,9 +475,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do role: :editor ) - membership = Enum.find(site.memberships, &(&1.role == :admin)) - - conn = delete(conn, "/sites/#{site.domain}/memberships/#{membership.id}") + conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}") assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed" refute Repo.reload(guest_membership) @@ -556,7 +500,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do ] ) - conn = delete(conn, "/sites/#{site.domain}/memberships/#{foreign_membership.id}") + conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{foreign_membership.user_id}") assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Failed to find membership to remove" @@ -568,19 +512,10 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do end test "notifies the user who has been removed via email", %{conn: conn, user: user} do - admin = insert(:user) - - site = - insert(:site, - memberships: [ - build(:site_membership, user: user, role: :owner), - build(:site_membership, user: admin, role: :admin) - ] - ) - - membership = Enum.find(site.memberships, &(&1.role == :admin)) + site = new_site() + admin = add_guest(site, user: user, role: :editor) - delete(conn, "/sites/#{site.domain}/memberships/#{membership.id}") + delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}") assert_email_delivered_with( to: [nil: admin.email], diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index f1d96e9de9eb..0962a49ad4ea 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -500,9 +500,9 @@ defmodule PlausibleWeb.SiteControllerTest do @tag :ee_only test "shows members page with links to CRM for super admin", %{ conn: conn, - user: user, - site: site + user: user } do + site = new_site(owner: user) patch_env(:super_admin_user_ids, [user.id]) conn = get(conn, "/#{site.domain}/settings/people") @@ -511,12 +511,53 @@ defmodule PlausibleWeb.SiteControllerTest do assert resp =~ "/crm/auth/user/#{user.id}" end - test "does not show CRM links to non-super admin user", %{conn: conn, user: user, site: site} do + test "does not show CRM links to non-super admin user", %{conn: conn, user: user} do + site = new_site(owner: user) conn = get(conn, "/#{site.domain}/settings/people") resp = html_response(conn, 200) refute resp =~ "/crm/auth/user/#{user.id}" end + + test "lists current members", %{conn: conn, user: user} do + site = new_site(owner: user) + editor = add_guest(site, role: :editor) + viewer = add_guest(site, role: :viewer) + conn = get(conn, "/#{site.domain}/settings/people") + resp = html_response(conn, 200) + + owner_row = + text_of_element(resp, "#membership-#{user.id}") + + editor_row = text_of_element(resp, "#membership-#{editor.id}") + editor_row_button = text_of_element(resp, "#membership-#{editor.id} button") + viewer_row = text_of_element(resp, "#membership-#{viewer.id}") + viewer_row_button = text_of_element(resp, "#membership-#{viewer.id} button") + + assert owner_row =~ user.email + assert owner_row =~ "Owner" + + assert editor_row =~ editor.email + assert editor_row_button == "Admin" + refute editor_row =~ "Owner" + + assert viewer_row =~ viewer.email + assert viewer_row_button == "Viewer" + refute viewer_row =~ "Owner" + end + + test "lists pending invitations", %{conn: conn, user: user} do + site = new_site(owner: user) + i1 = invite_guest(site, "admin@example.com", role: :editor, inviter: user) + i2 = invite_guest(site, "viewer@example.com", role: :viewer, inviter: user) + conn = get(conn, "/#{site.domain}/settings/people") + resp = html_response(conn, 200) + + assert text_of_element(resp, "#invitation-#{i1.invitation_id}") == "admin@example.com Admin" + + assert text_of_element(resp, "#invitation-#{i2.invitation_id}") == + "viewer@example.com Viewer" + end end describe "GET /:domain/settings/goals" do diff --git a/test/support/factory.ex b/test/support/factory.ex index aeb164b4f6e2..3344046d8f0a 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -33,6 +33,7 @@ defmodule Plausible.Factory do def guest_invitation_factory do %Plausible.Teams.GuestInvitation{ + invitation_id: Nanoid.generate(), role: :editor, team_invitation: build(:team_invitation, role: :guest) } diff --git a/test/support/teams/test.ex b/test/support/teams/test.ex index 350437ff22ab..f1695ae1680f 100644 --- a/test/support/teams/test.ex +++ b/test/support/teams/test.ex @@ -11,6 +11,12 @@ defmodule Plausible.Teams.Test do import Plausible.Factory + defmacro __using__(_) do + quote do + import Plausible.Teams.Test + end + end + def new_site(args \\ []) do args = if user = args[:owner] do @@ -83,14 +89,18 @@ defmodule Plausible.Teams.Test do team_invitation = insert(:team_invitation, - invitation_id: old_model_invitation.invitation_id, team: team, email: email, inviter: inviter, role: :guest ) - insert(:guest_invitation, team_invitation: team_invitation, site: site, role: role) + insert(:guest_invitation, + invitation_id: old_model_invitation.invitation_id, + team_invitation: team_invitation, + site: site, + role: role + ) old_model_invitation end @@ -131,12 +141,6 @@ defmodule Plausible.Teams.Test do insert(:growth_subscription, user: user, team: team) end - defmacro __using__(_) do - quote do - import Plausible.Teams.Test - end - end - def assert_team_exists(user, team_id \\ nil) do assert %{team_memberships: memberships} = Repo.preload(user, team_memberships: :team) @@ -157,14 +161,31 @@ defmodule Plausible.Teams.Test do end def assert_team_membership(user, team, role \\ :owner) do - assert membership = - Repo.get_by(Plausible.Teams.Membership, - team_id: team.id, - user_id: user.id, - role: role - ) - - membership + if role == :owner do + assert membership = + Repo.get_by(Teams.Membership, + team_id: team.id, + user_id: user.id, + role: role + ) + + membership + else + assert team_membership = + Repo.get_by(Teams.Membership, + team_id: team.id, + user_id: user.id, + role: :guest + ) + + assert membership = + Repo.get_by(Teams.GuestMembership, + team_membership_id: team_membership.id, + role: role + ) + + membership + end end def assert_team_attached(site, team_id \\ nil) do