From f5bb02d36ed284bd15130d835fc886f47a2981d2 Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Fri, 29 Mar 2024 16:50:04 -0400 Subject: [PATCH] refactor(portal): Move actor groups to own table in actor show page (#4392) Why: * When viewing an actor in the portal, all of the groups were listed in the top info table. This works for a small number of groups, but becomes difficult to use when an actor is in a large number of groups. This commit moves that information to it's own `live_table` element so that it's easier to parse and can be paginated. --- elixir/apps/domain/lib/domain/actors.ex | 9 +++ .../domain/lib/domain/actors/group/query.ex | 7 ++ .../apps/domain/test/domain/actors_test.exs | 77 +++++++++++++++++++ elixir/apps/web/lib/web/live/actors/show.ex | 57 +++++++++++--- .../web/test/web/live/actors/show_test.exs | 31 ++++++-- 5 files changed, 161 insertions(+), 20 deletions(-) diff --git a/elixir/apps/domain/lib/domain/actors.ex b/elixir/apps/domain/lib/domain/actors.ex index d8cfe5273..cdaaf59fa 100644 --- a/elixir/apps/domain/lib/domain/actors.ex +++ b/elixir/apps/domain/lib/domain/actors.ex @@ -46,6 +46,15 @@ defmodule Domain.Actors do end end + def list_groups_for(%Actor{} = actor, %Auth.Subject{} = subject, opts \\ []) do + with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_actors_permission()) do + Group.Query.not_deleted() + |> Group.Query.by_actor_id(actor.id) + |> Authorizer.for_subject(subject) + |> Repo.list(Group.Query, opts) + end + end + def all_groups!(%Auth.Subject{} = subject, opts \\ []) do {preload, _opts} = Keyword.pop(opts, :preload, []) diff --git a/elixir/apps/domain/lib/domain/actors/group/query.ex b/elixir/apps/domain/lib/domain/actors/group/query.ex index 2d32cdf52..f5b1e002d 100644 --- a/elixir/apps/domain/lib/domain/actors/group/query.ex +++ b/elixir/apps/domain/lib/domain/actors/group/query.ex @@ -42,6 +42,13 @@ defmodule Domain.Actors.Group.Query do where(queryable, [groups: groups], groups.account_id == ^account_id) end + def by_actor_id(queryable, actor_id) do + join(queryable, :left, [groups: groups], memberships in assoc(groups, :memberships), + as: :memberships + ) + |> where([memberships: memberships], memberships.actor_id == ^actor_id) + end + def by_provider_id(queryable, provider_id) do where(queryable, [groups: groups], groups.provider_id == ^provider_id) end diff --git a/elixir/apps/domain/test/domain/actors_test.exs b/elixir/apps/domain/test/domain/actors_test.exs index ca27d4092..f48e3f8cf 100644 --- a/elixir/apps/domain/test/domain/actors_test.exs +++ b/elixir/apps/domain/test/domain/actors_test.exs @@ -189,6 +189,83 @@ defmodule Domain.ActorsTest do end end + describe "list_groups_for/2" do + setup do + account = Fixtures.Accounts.create_account() + actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account) + identity = Fixtures.Auth.create_identity(account: account, actor: actor) + subject = Fixtures.Auth.create_subject(identity: identity) + + %{ + account: account, + actor: actor, + identity: identity, + subject: subject + } + end + + test "returns empty list when there are no groups", %{actor: actor, subject: subject} do + assert {:ok, [], _metadata} = list_groups_for(actor, subject) + end + + test "does not list groups from other accounts", %{actor: actor, subject: subject} do + Fixtures.Actors.create_group() + assert {:ok, [], _metadata} = list_groups_for(actor, subject) + end + + test "does not list groups from other actors in account", %{ + account: account, + actor: actor, + subject: subject + } do + actor2 = Fixtures.Actors.create_actor(account: account) + group = Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_membership(account: account, actor: actor2, group: group) + + assert {:ok, [], _metadata} = list_groups_for(actor, subject) + end + + test "does not list deleted groups", %{account: account, actor: actor, subject: subject} do + group = Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_membership(account: account, actor: actor, group: group) + + Fixtures.Actors.delete_group(group) + + assert {:ok, [], _metadata} = list_groups_for(actor, subject) + end + + test "returns all groups for actor", %{ + account: account, + actor: actor, + subject: subject + } do + group1 = Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_membership(account: account, actor: actor, group: group1) + + group2 = Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_membership(account: account, actor: actor, group: group2) + + Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_group() + + assert {:ok, groups, _metadata} = list_groups_for(actor, subject) + assert length(groups) == 2 + end + + test "returns error when subject has no permission to manage groups", %{ + actor: actor, + subject: subject + } do + subject = Fixtures.Auth.remove_permissions(subject) + + assert list_groups_for(actor, subject) == + {:error, + {:unauthorized, + reason: :missing_permissions, + missing_permissions: [Actors.Authorizer.manage_actors_permission()]}} + end + end + describe "peek_group_actors/3" do setup do account = Fixtures.Accounts.create_account() diff --git a/elixir/apps/web/lib/web/live/actors/show.ex b/elixir/apps/web/lib/web/live/actors/show.ex index 946712222..d6f982b2c 100644 --- a/elixir/apps/web/lib/web/live/actors/show.ex +++ b/elixir/apps/web/lib/web/live/actors/show.ex @@ -42,6 +42,13 @@ defmodule Web.Actors.Show do limit: 10, callback: &handle_flows_update!/2 ) + |> assign_live_table("groups", + query_module: Actors.Group.Query, + sortable_fields: [], + hide_filters: [:provider_id], + limit: 15, + callback: &handle_groups_update!/2 + ) {:ok, socket} else @@ -67,6 +74,19 @@ defmodule Web.Actors.Show do end end + def handle_groups_update!(socket, list_opts) do + list_opts = Keyword.put(list_opts, :preload, [:provider]) + + with {:ok, groups, metadata} <- + Actors.list_groups_for(socket.assigns.actor, socket.assigns.subject, list_opts) do + {:ok, + assign(socket, + groups: groups, + groups_metadata: metadata + )} + end + end + def handle_tokens_update!(socket, list_opts) do list_opts = Keyword.put(list_opts, :preload, @@ -171,18 +191,6 @@ defmodule Web.Actors.Show do - <.vertical_table_row> - <:label>Groups - <:value> -
- none - - <.group account={@account} group={group} /> - -
- - - <.vertical_table_row> <:label>Last Signed In <:value><.relative_datetime datetime={@actor.last_seen_at} /> @@ -450,6 +458,31 @@ defmodule Web.Actors.Show do + <.section> + <:title>Groups + + <:content> + <.live_table + id="groups" + rows={@groups} + row_id={&"group-#{&1.id}"} + filters={@filters_by_table_id["groups"]} + filter={@filter_form_by_table_id["groups"]} + ordered_by={@order_by_table_id["groups"]} + metadata={@groups_metadata} + > + <:col :let={group} label="NAME"> + <.link navigate={~p"/#{@account}/groups/#{group.id}"} class={[link_style()]}> + <%= group.name %> + + + <:empty> +
No Groups to display.
+ + + + + <.danger_zone :if={is_nil(@actor.deleted_at)}> <:action :if={not Actors.actor_synced?(@actor) or @identities == []}> <.delete_button diff --git a/elixir/apps/web/test/web/live/actors/show_test.exs b/elixir/apps/web/test/web/live/actors/show_test.exs index cba86ed50..c5edb0205 100644 --- a/elixir/apps/web/test/web/live/actors/show_test.exs +++ b/elixir/apps/web/test/web/live/actors/show_test.exs @@ -222,6 +222,29 @@ defmodule Web.Live.Actors.ShowTest do "#{flow.gateway.group.name}-#{flow.gateway.name} (#{flow.gateway.last_seen_remote_ip})" end + test "renders groups table", %{ + conn: conn + } do + account = Fixtures.Accounts.create_account() + actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account) + identity = Fixtures.Auth.create_identity(account: account, actor: actor) + group = Fixtures.Actors.create_group(account: account) + Fixtures.Actors.create_membership(account: account, actor: actor, group: group) + + {:ok, lv, _html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/actors/#{actor}") + + [row] = + lv + |> element("#groups") + |> render() + |> table_to_map() + + assert row["name"] == group.name + end + describe "users" do setup do account = Fixtures.Accounts.create_account() @@ -268,9 +291,6 @@ defmodule Web.Live.Actors.ShowTest do identity: identity, conn: conn } do - group = Fixtures.Actors.create_group(account: account) - Fixtures.Actors.create_membership(account: account, actor: actor, group: group) - {:ok, lv, html} = conn |> authorize_conn(identity) @@ -285,7 +305,6 @@ defmodule Web.Live.Actors.ShowTest do |> render() |> vertical_table_to_map() - assert table["groups"] == group.name assert table["name"] == actor.name assert table["role"] == "admin" assert around_now?(table["last signed in"]) @@ -779,9 +798,6 @@ defmodule Web.Live.Actors.ShowTest do identity: identity, conn: conn } do - group = Fixtures.Actors.create_group(account: account) - Fixtures.Actors.create_membership(account: account, actor: actor, group: group) - {:ok, lv, html} = conn |> authorize_conn(identity) @@ -793,7 +809,6 @@ defmodule Web.Live.Actors.ShowTest do |> element("#actor") |> render() |> vertical_table_to_map() == %{ - "groups" => group.name, "last signed in" => "never", "name" => actor.name, "role" => "service account"