From c5929d40635748a0c1452dcf07e1c8aebbf3c27c Mon Sep 17 00:00:00 2001 From: Jamil Date: Mon, 24 Feb 2025 07:39:16 -0800 Subject: [PATCH] fix(portal): Show reload button when table data is stale (#8143) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sentry uncovered a bug in the resources index liveview where it looks like some code copy-pasted from the policies index view wasn't updated properly to work in the resources live view, causing the view to crash if an admin was viewing the table while the resources are changed in another page. In debugging that, I realized the best UX when viewing these tables is usually just to show a `Reload` button and not update the data live while the admin is viewing it, as this can cause missed clicks and other annoyances. This PR adds an optional `stale` component attribute that, if true, will render a `Reload` button in the live table which upon clicking will reload the live table. Not all index views are updated with this - in some views there is already logic to handle making an intelligent update without breaking the view if the data is updated - for example for the clients table. Ideally, we live-update things that don't reflow layout inline (such as `online/offline` presence) and for things that do cause layout reflow (create/delete), we show the `Reload` button. However that work is saved for a future PR as this one fixes the immediate bug and this is not the highest priority. Screenshot 2025-02-16 at 8 44 43 AM Screenshot 2025-02-16 at 9 59 53 AM --- .../apps/web/lib/web/live/policies/index.ex | 27 +++------- .../apps/web/lib/web/live/resources/index.ex | 26 +++------- elixir/apps/web/lib/web/live_table.ex | 25 ++++++++- .../web/test/web/live/policies/index_test.exs | 51 +++++++++++++++++++ .../test/web/live/resources/index_test.exs | 51 +++++++++++++++++++ 5 files changed, 137 insertions(+), 43 deletions(-) diff --git a/elixir/apps/web/lib/web/live/policies/index.ex b/elixir/apps/web/lib/web/live/policies/index.ex index 08156bc34..fa323dd7c 100644 --- a/elixir/apps/web/lib/web/live/policies/index.ex +++ b/elixir/apps/web/lib/web/live/policies/index.ex @@ -9,6 +9,7 @@ defmodule Web.Policies.Index do socket = socket + |> assign(stale: false) |> assign(page_title: "Policies") |> assign_live_table("policies", query_module: Policies.Policy.Query, @@ -64,6 +65,7 @@ defmodule Web.Policies.Index do <:content> <.flash_group flash={@flash} /> <.live_table + stale={@stale} id="policies" rows={@policies} row_id={&"policies-#{&1.id}"} @@ -117,28 +119,11 @@ defmodule Web.Policies.Index do """ end - def handle_event(event, params, socket) when event in ["paginate", "order_by", "filter"], - do: handle_live_table_event(event, params, socket) - - def handle_info({:create_policy, _policy_id}, socket) do - {:noreply, socket} - end - - def handle_info({:delete_policy, policy_id}, socket) do - if Enum.find(socket.assigns.policies, fn policy -> policy.id == policy_id end) do - policies = - Enum.filter(socket.assigns.policies, fn - policy -> policy.id != policy_id - end) - - {:noreply, assign(socket, policies: policies)} - else - {:noreply, socket} - end - end + def handle_event(event, params, socket) + when event in ["paginate", "order_by", "filter", "reload"], + do: handle_live_table_event(event, params, socket) def handle_info({_action, _policy_id}, socket) do - socket = reload_live_table!(socket, "policies") - {:noreply, socket} + {:noreply, assign(socket, stale: true)} end end diff --git a/elixir/apps/web/lib/web/live/resources/index.ex b/elixir/apps/web/lib/web/live/resources/index.ex index d98f590b8..9bbfcc106 100644 --- a/elixir/apps/web/lib/web/live/resources/index.ex +++ b/elixir/apps/web/lib/web/live/resources/index.ex @@ -15,6 +15,7 @@ defmodule Web.Resources.Index do socket = socket + |> assign(stale: false) |> assign(page_title: "Resources") |> assign(internet_site: internet_site) |> assign_live_table("resources", @@ -87,6 +88,7 @@ defmodule Web.Resources.Index do <:content> <.flash_group flash={@flash} /> <.live_table + stale={@stale} id="resources" rows={@resources} row_id={&"resource-#{&1.id}"} @@ -161,27 +163,11 @@ defmodule Web.Resources.Index do """ end - def handle_event(event, params, socket) when event in ["paginate", "order_by", "filter"], - do: handle_live_table_event(event, params, socket) - - def handle_info({:create_resource, _resource_id}, socket) do - {:noreply, socket} - end - - def handle_info({:delete_resource, resource_id}, socket) do - if Enum.find(socket.assigns.policies, fn resource -> resource.id == resource_id end) do - policies = - Enum.filter(socket.assigns.policies, fn resource -> - resource.id != resource_id - end) - - {:noreply, assign(socket, policies: policies)} - else - {:noreply, socket} - end - end + def handle_event(event, params, socket) + when event in ["paginate", "order_by", "filter", "reload"], + do: handle_live_table_event(event, params, socket) def handle_info({_action, _resource_id}, socket) do - {:noreply, reload_live_table!(socket, "resources")} + {:noreply, assign(socket, stale: true)} end end diff --git a/elixir/apps/web/lib/web/live_table.ex b/elixir/apps/web/lib/web/live_table.ex index 450974ef2..f5f655551 100644 --- a/elixir/apps/web/lib/web/live_table.ex +++ b/elixir/apps/web/lib/web/live_table.ex @@ -15,6 +15,7 @@ defmodule Web.LiveTable do attr :ordered_by, :any, required: true, doc: "the current order for the table" attr :filters, :list, required: true, doc: "the query filters enabled for the table" attr :filter, :map, required: true, doc: "the filter form for the table" + attr :stale, :boolean, default: false, doc: "hint to the UI that the table data is stale" attr :metadata, :map, required: true, @@ -40,7 +41,7 @@ defmodule Web.LiveTable do def live_table(assigns) do ~H""" - <.resource_filter live_table_id={@id} form={@filter} filters={@filters} /> + <.resource_filter stale={@stale} live_table_id={@id} form={@filter} filters={@filters} />
<.table_header table_id={@id} columns={@col} actions={@action} ordered_by={@ordered_by} /> @@ -152,7 +153,21 @@ defmodule Web.LiveTable do > <.input type="hidden" name="table_id" value={@live_table_id} /> -
+
+
+ <.button + :if={@stale} + id={"#{@live_table_id}-reload-btn"} + type="button" + style="info" + title="The table data has changed." + phx-click="reload" + phx-value-table_id={@live_table_id} + > + <.icon name="hero-arrow-path" class="mr-1 w-3.5 h-3.5" /> Reload + +
+ <.filter :for={filter <- @filters} live_table_id={@live_table_id} @@ -536,6 +551,8 @@ defmodule Web.LiveTable do callback = Map.fetch!(socket.assigns.callback_by_table_id, id) list_opts = Map.get(socket.assigns[:list_opts_by_table_id] || %{}, id, []) + socket = assign(socket, stale: false) + case callback.(socket, list_opts) do {:error, _reason} -> uri = URI.parse(socket.assigns.uri) @@ -776,6 +793,10 @@ defmodule Web.LiveTable do end end + def handle_live_table_event("reload", %{"table_id" => id}, socket) do + {:noreply, reload_live_table!(socket, id)} + end + def handle_live_table_event("paginate", %{"table_id" => id, "cursor" => cursor}, socket) do update_query_params(socket, fn query_params -> put_cursor_to_params(query_params, id, cursor) diff --git a/elixir/apps/web/test/web/live/policies/index_test.exs b/elixir/apps/web/test/web/live/policies/index_test.exs index ec66ffdc8..78398eb69 100644 --- a/elixir/apps/web/test/web/live/policies/index_test.exs +++ b/elixir/apps/web/test/web/live/policies/index_test.exs @@ -77,4 +77,55 @@ defmodule Web.Live.Policies.IndexTest do assert rendered_policy["group"] =~ policy.actor_group.name assert rendered_policy["resource"] =~ policy.resource.name end + + describe "handle_info/2" do + test "Shows reload button when policy is created", %{ + account: account, + identity: identity, + conn: conn + } do + {:ok, lv, html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/policies") + + refute html =~ "The table data has changed." + refute html =~ "reload-btn" + + Fixtures.Policies.create_policy(account: account, description: "foo bar") + + reload_btn = + lv + |> element("#policies-reload-btn") + |> render() + + assert reload_btn + end + + test "Shows reload button when policy is deleted", %{ + account: account, + identity: identity, + conn: conn + } do + policy = Fixtures.Policies.create_policy(account: account, description: "foo bar") + subject = Fixtures.Auth.create_subject(identity: identity) + + {:ok, lv, html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/policies") + + refute html =~ "The table data has changed." + refute html =~ "reload-btn" + + Domain.Policies.delete_policy(policy, subject) + + reload_btn = + lv + |> element("#policies-reload-btn") + |> render() + + assert reload_btn + end + end end diff --git a/elixir/apps/web/test/web/live/resources/index_test.exs b/elixir/apps/web/test/web/live/resources/index_test.exs index 3efddacf3..a057918fb 100644 --- a/elixir/apps/web/test/web/live/resources/index_test.exs +++ b/elixir/apps/web/test/web/live/resources/index_test.exs @@ -205,4 +205,55 @@ defmodule Web.Live.Resources.IndexTest do assert row["authorized groups"] =~ "and 1 more" end) end + + describe "handle_info/2" do + test "Shows reload button when resource is created", %{ + account: account, + identity: identity, + conn: conn + } do + {:ok, lv, html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/resources") + + refute html =~ "The table data has changed." + refute html =~ "reload-btn" + + Fixtures.Resources.create_resource(account: account) + + reload_btn = + lv + |> element("#resources-reload-btn") + |> render() + + assert reload_btn + end + + test "Shows reload button when resource is deleted", %{ + account: account, + identity: identity, + conn: conn + } do + resource = Fixtures.Resources.create_resource(account: account) + subject = Fixtures.Auth.create_subject(identity: identity) + + {:ok, lv, html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/resources") + + refute html =~ "The table data has changed." + refute html =~ "reload-btn" + + Domain.Resources.delete_resource(resource, subject) + + reload_btn = + lv + |> element("#resources-reload-btn") + |> render() + + assert reload_btn + end + end end