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

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

* Fix faulty member label in people list

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

---------

Co-authored-by: hq1 <[email protected]>

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

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

@@ -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}%"))

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

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

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

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 %>
-          <li class="py-4">
+        <%= for membership <- @memberships do %>
+          <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>