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 5eb8754782.

* 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 672429b9d1.

* Fix faulty member label in people list

* Fix sites listing for a case of pending invite with existing pin

---------

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

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),
+            (gi.role == :editor and si.role == :viewer) or
-        select: {gi, si.role}
+            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
+  defp sync_guest_invitations(guest_and_site_invitations) do
-    guest_invitations_and_roles
+    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!()
 

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
       )
 

lib/plausible/teams/adapter/read/sites.ex

@@ -2,10 +2,13 @@ defmodule Plausible.Teams.Adapter.Read.Sites do
   @moduledoc """
   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}%"))

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)

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

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,

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
+  def remove_member_by_user(conn, %{"id" => user_id} = _params) do
-    site = conn.assigns[:site]
+    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),

lib/plausible_web/controllers/site_controller.ex

@@ -122,13 +122,17 @@ defmodule PlausibleWeb.SiteController do
   end
 
   def settings_people(conn, _params) do
-    site =
+    current_user = conn.assigns.current_user
-      conn.assigns[:site]
+    site = conn.assigns.site
-      |> Repo.preload(memberships: :user, invitations: [])
+
+    %{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"}
     )

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
+    put "/sites/:domain/memberships/u/:id/role/:new_role",
-    delete "/sites/:domain/memberships/:id", Site.MembershipController, :remove_member
+        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

lib/plausible_web/templates/site/settings_people.html.heex

@@ -14,8 +14,8 @@
 
     <div class="flow-root">
       <ul class="divide-y divide-gray-200 dark:divide-gray-400">
-        <%= for membership <- @site.memberships do %>
+        <%= for membership <- @memberships do %>
-          <li class="py-4">
+          <li class="py-4" id={"membership-#{membership.user_id}"}>
             <div class="flex items-center space-x-4">
               <div class="flex-shrink-0">
                 <img
@@ -47,7 +47,7 @@
                   @click="open = !open"
                   class="inline-flex items-center shadow-sm px-2.5 py-0.5 border border-gray-300 dark:border-gray-500 text-sm leading-5 font-medium rounded-full bg-white dark:bg-gray-800 text-gray-700 dark:text-gray-300 hover:bg-gray-50 dark:hover:bg-gray-800"
                 >
-                  <%= membership.role |> Atom.to_string() |> String.capitalize() %>
+                  <%= membership.role |> to_string() |> String.capitalize() %>
                   <svg
                     class="w-4 h-4 pt-px ml-1"
                     fill="currentColor"
@@ -124,9 +124,9 @@
                       href={
                         Routes.membership_path(
                           @conn,
-                          :update_role,
+                          :update_role_by_user,
                           @site.domain,
-                          membership.id,
+                          membership.user.id,
                           "admin"
                         )
                       }
@@ -164,9 +164,9 @@
                       href={
                         Routes.membership_path(
                           @conn,
-                          :update_role,
+                          :update_role_by_user,
                           @site.domain,
-                          membership.id,
+                          membership.user.id,
                           "viewer"
                         )
                       }
@@ -203,7 +203,12 @@
 
                     <.unstyled_link
                       href={
-                        Routes.membership_path(@conn, :remove_member, @site.domain, membership.id)
+                        Routes.membership_path(
+                          @conn,
+                          :remove_member_by_user,
+                          @site.domain,
+                          membership.user.id
+                        )
                       }
                       method="delete"
                       class="p-4 flex hover:bg-gray-100 dark:hover:bg-gray-900 text-red-600"
@@ -220,11 +225,14 @@
     </div>
   </.tile>
 
-  <.tile :if={Enum.count(@site.invitations) > 0}>
+  <.tile :if={Enum.count(@invitations) > 0}>
     <:title>Pending invitations</:title>
     <:subtitle>Waiting for new members to accept their invitations</:subtitle>
 
-    <.table rows={@site.invitations}>
+    <.table
+      rows={@invitations}
+      row_attrs={fn invitation -> %{id: "invitation-#{invitation.invitation_id}"} end}
+    >
       <:thead>
         <.th>Email</.th>
         <.th hide_on_mobile>Role</.th>

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()

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
+    test "updates a site member's role by user id", %{conn: conn, user: user} do
-      admin = insert(:user)
+      site = new_site(owner: user)
+      collaborator = add_guest(site, role: :editor)
+      assert_team_membership(collaborator, site.team, :editor)
 
-      site =
+      put(conn, "/sites/#{site.domain}/memberships/u/#{collaborator.id}/role/viewer")
-        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)
+      assert_team_membership(collaborator, site.team, :viewer)
 
-      put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer")
+      old_model_membership = Repo.get_by(Plausible.Site.Membership, user_id: collaborator.id)
-
+      assert old_model_membership.role == :viewer
-      membership = Repo.reload!(membership)
-
-      assert 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/u/#{admin.id}/role/viewer")
-
-      put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer")
 
       assert Repo.reload!(guest_membership).role == :viewer
     end
@@ -340,12 +332,10 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
     } do
       site = insert(:site, memberships: [build(:site_membership, user: user, role: :admin)])
 
+      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/viewer")
+
       membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id)
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/viewer")
-
-      membership = Repo.reload!(membership)
-
       assert membership.role == :viewer
       assert redirected_to(conn) == "/#{URI.encode_www_form(site.domain)}"
     end
@@ -354,23 +344,12 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       conn: conn,
       user: user
     } do
-      admin = insert(:user)
+      site = new_site()
+      admin = add_guest(site, user: user, role: :editor)
 
-      site =
+      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}/role/owner")
-        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)
+      assert_team_membership(user, site.team, :editor)
-
-      conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/owner")
-
-      membership = Repo.reload!(membership)
-
-      assert membership.role == :admin
 
       assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
                "You are not allowed to grant the owner role"
@@ -382,12 +361,10 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
     } do
       site = insert(:site, memberships: [build(:site_membership, user: user, role: :owner)])
 
+      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/admin")
+
       membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id)
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/admin")
-
-      membership = Repo.reload!(membership)
-
       assert membership.role == :owner
 
       assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
@@ -398,44 +375,24 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       conn: conn,
       user: user
     } do
-      viewer = insert(:user)
+      site = new_site()
 
-      site =
+      add_guest(site, user: user, role: :editor)
-        insert(:site,
+      viewer = add_guest(site, user: new_user(), role: :viewer)
-          memberships: [
-            build(:site_membership, user: user, role: :admin),
-            build(:site_membership, user: viewer, 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")
+      assert_team_membership(viewer, site.team, :editor)
-
-      viewer_membership = Repo.reload!(viewer_membership)
-
-      assert viewer_membership.role == :admin
       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 =
+      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/owner")
-        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)
+      assert_team_membership(user, site.team, :editor)
-
-      conn = put(conn, "/sites/#{site.domain}/memberships/#{membership.id}/role/owner")
-
-      membership = Repo.reload!(membership)
-
-      assert membership.role == :admin
 
       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
+    test "removes a member from a site by user id", %{conn: conn, user: user} do
-      admin = insert(:user)
+      site = new_site(owner: user)
+      admin = add_guest(site, role: :editor)
 
-      site =
+      conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
-        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}")
       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/u/#{admin.id}")
-
-      conn = delete(conn, "/sites/#{site.domain}/memberships/#{membership.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/u/#{admin.id}")
-
-      conn = delete(conn, "/sites/#{site.domain}/memberships/#{membership.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 = new_site()
+      admin = add_guest(site, user: user, role: :editor)
 
-      site =
+      delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
-        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))
-
-      delete(conn, "/sites/#{site.domain}/memberships/#{membership.id}")
 
       assert_email_delivered_with(
         to: [nil: admin.email],

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,
+      user: user
-      site: site
     } 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

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)
     }

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
+    if role == :owner do
       assert membership =
-             Repo.get_by(Plausible.Teams.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