From 21d2ca358b0783ef98457165f937ef8cbddb0259 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Wed, 27 Mar 2024 13:03:19 -0600 Subject: [PATCH] fix(portal): Fix pagination issues with flows and activities, improve error handling around live tables (#4330) Fixes issues from logs. Closes #4274 and similar issues for activities. Simplifies error handling for live tables (we just reset filters with a message when they are invalid because just showing an error 422 is not actionable). --- .../domain/lib/domain/actors/actor/query.ex | 18 + elixir/apps/domain/lib/domain/flows.ex | 28 +- .../domain/lib/domain/flows/activity/query.ex | 27 ++ elixir/apps/domain/lib/domain/repo/query.ex | 6 - elixir/apps/domain/test/domain/flows_test.exs | 108 +++--- .../domain/jobs/executors/global_test.exs | 2 +- .../lib/web/controllers/error_controller.ex | 1 - .../web/controllers/error_html/422.html.heex | 44 --- elixir/apps/web/lib/web/live/actors/index.ex | 2 + .../lib/web/live/flows/download_activities.ex | 2 - elixir/apps/web/lib/web/live/flows/show.ex | 73 +++- .../web/lib/web/live/groups/edit_actors.ex | 2 +- elixir/apps/web/lib/web/live/groups/show.ex | 2 +- elixir/apps/web/lib/web/live_errors.ex | 9 - elixir/apps/web/lib/web/live_table.ex | 321 ++++++++++++++---- .../test/web/controllers/error_html_test.exs | 10 - .../web/test/web/live/flows/show_test.exs | 35 ++ elixir/apps/web/test/web/live_table_test.exs | 41 ++- 18 files changed, 510 insertions(+), 221 deletions(-) delete mode 100644 elixir/apps/web/lib/web/controllers/error_html/422.html.heex diff --git a/elixir/apps/domain/lib/domain/actors/actor/query.ex b/elixir/apps/domain/lib/domain/actors/actor/query.ex index c0dba0b7c..d334c6ec2 100644 --- a/elixir/apps/domain/lib/domain/actors/actor/query.ex +++ b/elixir/apps/domain/lib/domain/actors/actor/query.ex @@ -159,6 +159,16 @@ defmodule Domain.Actors.Actor.Query do values: &Domain.Auth.all_providers!/1, fun: &filter_by_identity_provider_id/2 }, + %Domain.Repo.Filter{ + name: :status, + title: "Status", + type: :string, + values: [ + {"Enabled", "enabled"}, + {"Disabled", "disabled"} + ], + fun: &filter_by_status/2 + }, %Domain.Repo.Filter{ name: :type, title: "Type", @@ -189,6 +199,14 @@ defmodule Domain.Actors.Actor.Query do } ] + def filter_by_status(queryable, "enabled") do + {queryable, dynamic([actors: actors], is_nil(actors.disabled_at))} + end + + def filter_by_status(queryable, "disabled") do + {queryable, dynamic([actors: actors], not is_nil(actors.disabled_at))} + end + def filter_by_type(queryable, type) do {queryable, dynamic([actors: actors], actors.type == ^type)} end diff --git a/elixir/apps/domain/lib/domain/flows.ex b/elixir/apps/domain/lib/domain/flows.ex index 77ffdd415..aafee5f12 100644 --- a/elixir/apps/domain/lib/domain/flows.ex +++ b/elixir/apps/domain/lib/domain/flows.ex @@ -125,7 +125,6 @@ defmodule Domain.Flows do with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_flows_permission()) do queryable |> Authorizer.for_subject(Flow, subject) - |> Ecto.Query.order_by([flows: flows], desc: flows.inserted_at, desc: flows.id) |> Repo.list(Flow.Query, opts) end end @@ -145,39 +144,24 @@ defmodule Domain.Flows do end end - def list_flow_activities_for(assoc, ended_after, started_before, subject, opts \\ []) + def list_flow_activities_for(assoc, subject, opts \\ []) - def list_flow_activities_for( - %Flow{} = flow, - ended_after, - started_before, - %Auth.Subject{} = subject, - opts - ) do + def list_flow_activities_for(%Flow{} = flow, %Auth.Subject{} = subject, opts) do Activity.Query.all() |> Activity.Query.by_flow_id(flow.id) - |> list_activities(ended_after, started_before, subject, opts) + |> list_activities(subject, opts) end - def list_flow_activities_for( - %Accounts.Account{} = account, - ended_after, - started_before, - %Auth.Subject{} = subject, - opts - ) do + def list_flow_activities_for(%Accounts.Account{} = account, %Auth.Subject{} = subject, opts) do Activity.Query.all() |> Activity.Query.by_account_id(account.id) - |> list_activities(ended_after, started_before, subject, opts) + |> list_activities(subject, opts) end - defp list_activities(queryable, ended_after, started_before, subject, opts) do + defp list_activities(queryable, subject, opts) do with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_flows_permission()) do queryable - |> Activity.Query.by_window_ended_at({:greater_than, ended_after}) - |> Activity.Query.by_window_started_at({:less_than, started_before}) |> Authorizer.for_subject(Activity, subject) - |> Ecto.Query.order_by([activities: activities], asc: activities.window_started_at) |> Repo.list(Activity.Query, opts) end end diff --git a/elixir/apps/domain/lib/domain/flows/activity/query.ex b/elixir/apps/domain/lib/domain/flows/activity/query.ex index ba6b59cc4..a181a45fb 100644 --- a/elixir/apps/domain/lib/domain/flows/activity/query.ex +++ b/elixir/apps/domain/lib/domain/flows/activity/query.ex @@ -33,4 +33,31 @@ defmodule Domain.Flows.Activity.Query do {:activities, :asc, :window_started_at}, {:activities, :asc, :id} ] + + @impl Domain.Repo.Query + def filters, + do: [ + %Domain.Repo.Filter{ + name: :window_within, + title: "Window", + type: {:range, :datetime}, + fun: &filter_by_window/2 + } + ] + + def filter_by_window(queryable, %Domain.Repo.Filter.Range{from: from, to: nil}) do + {queryable, dynamic([activities: activities], ^from <= activities.window_started_at)} + end + + def filter_by_window(queryable, %Domain.Repo.Filter.Range{from: nil, to: to}) do + {queryable, dynamic([activities: activities], activities.window_ended_at <= ^to)} + end + + def filter_by_window(queryable, %Domain.Repo.Filter.Range{from: from, to: to}) do + {queryable, + dynamic( + [activities: activities], + ^from <= activities.window_started_at and activities.window_ended_at <= ^to + )} + end end diff --git a/elixir/apps/domain/lib/domain/repo/query.ex b/elixir/apps/domain/lib/domain/repo/query.ex index 4442e55b1..08d78cc7d 100644 --- a/elixir/apps/domain/lib/domain/repo/query.ex +++ b/elixir/apps/domain/lib/domain/repo/query.ex @@ -87,12 +87,6 @@ defmodule Domain.Repo.Query do def by_range(%Filter.Range{from: from, to: to}, fragment), do: dynamic(^from <= ^fragment and ^fragment <= ^to) - def by_range(%Filter.Range{to: to}, fragment), - do: dynamic(^fragment <= ^to) - - def by_range(%Filter.Range{from: from}, fragment), - do: dynamic(^fragment >= ^from) - @doc """ This function is to allow reuse of the filter function in the regular query helpers, it takes a return of a filter function (`{queryable, dynamic}`) and applies it to the queryable. diff --git a/elixir/apps/domain/test/domain/flows_test.exs b/elixir/apps/domain/test/domain/flows_test.exs index 58851b1ca..2884a182b 100644 --- a/elixir/apps/domain/test/domain/flows_test.exs +++ b/elixir/apps/domain/test/domain/flows_test.exs @@ -571,15 +571,11 @@ defmodule Domain.FlowsTest do flow: flow, subject: subject } do - now = DateTime.utc_now() - ended_after = DateTime.add(now, -30, :minute) - started_before = DateTime.add(now, 30, :minute) + assert {:ok, [], _metadata} = + list_flow_activities_for(account, subject) assert {:ok, [], _metadata} = - list_flow_activities_for(account, ended_after, started_before, subject) - - assert {:ok, [], _metadata} = - list_flow_activities_for(flow, ended_after, started_before, subject) + list_flow_activities_for(flow, subject) end test "does not list flow activities from other accounts", %{ @@ -589,15 +585,11 @@ defmodule Domain.FlowsTest do flow = Fixtures.Flows.create_flow() Fixtures.Flows.create_activity(flow: flow) - now = DateTime.utc_now() - ended_after = DateTime.add(now, -30, :minute) - started_before = DateTime.add(now, 30, :minute) + assert {:ok, [], _metadata} = + list_flow_activities_for(account, subject) assert {:ok, [], _metadata} = - list_flow_activities_for(account, ended_after, started_before, subject) - - assert {:ok, [], _metadata} = - list_flow_activities_for(flow, ended_after, started_before, subject) + list_flow_activities_for(flow, subject) end test "returns ordered by window start time flow activities within a time window", %{ @@ -623,49 +615,73 @@ defmodule Domain.FlowsTest do assert {:ok, [], _metadata} = list_flow_activities_for( account, - thirty_minutes_in_future, - sixty_minutes_in_future, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_in_future, + to: sixty_minutes_in_future + } + ] ) assert {:ok, [], _metadata} = list_flow_activities_for( flow, - thirty_minutes_in_future, - sixty_minutes_in_future, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_in_future, + to: sixty_minutes_in_future + } + ] ) assert {:ok, [], _metadata} = list_flow_activities_for( account, - thirty_minutes_ago, - five_minutes_ago, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_ago, + to: five_minutes_ago + } + ] ) assert {:ok, [], _metadata} = list_flow_activities_for( flow, - thirty_minutes_ago, - five_minutes_ago, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_ago, + to: five_minutes_ago + } + ] ) assert {:ok, [^activity1], _metadata} = list_flow_activities_for( account, - five_minutes_ago, - now, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: five_minutes_ago, + to: now + } + ] ) assert {:ok, [^activity1], _metadata} = list_flow_activities_for( flow, - five_minutes_ago, - now, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: five_minutes_ago, + to: now + } + ] ) activity2 = @@ -678,17 +694,25 @@ defmodule Domain.FlowsTest do assert {:ok, [^activity2, ^activity1], _metadata} = list_flow_activities_for( account, - thirty_minutes_ago, - now, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_ago, + to: now + } + ] ) assert {:ok, [^activity2, ^activity1], _metadata} = list_flow_activities_for( flow, - thirty_minutes_ago, - now, - subject + subject, + filter: [ + window_within: %Domain.Repo.Filter.Range{ + from: thirty_minutes_ago, + to: now + } + ] ) end @@ -697,19 +721,15 @@ defmodule Domain.FlowsTest do flow: flow, subject: subject } do - now = DateTime.utc_now() - ended_after = DateTime.add(now, -30, :minute) - started_before = DateTime.add(now, 30, :minute) - subject = Fixtures.Auth.remove_permissions(subject) - assert list_flow_activities_for(account, ended_after, started_before, subject) == + assert list_flow_activities_for(account, subject) == {:error, {:unauthorized, reason: :missing_permissions, missing_permissions: [Flows.Authorizer.manage_flows_permission()]}} - assert list_flow_activities_for(flow, ended_after, started_before, subject) == + assert list_flow_activities_for(flow, subject) == {:error, {:unauthorized, reason: :missing_permissions, diff --git a/elixir/apps/domain/test/domain/jobs/executors/global_test.exs b/elixir/apps/domain/test/domain/jobs/executors/global_test.exs index 9b3d34ccc..4d5e12f73 100644 --- a/elixir/apps/domain/test/domain/jobs/executors/global_test.exs +++ b/elixir/apps/domain/test/domain/jobs/executors/global_test.exs @@ -30,7 +30,7 @@ defmodule Domain.Jobs.Executors.GlobalTest do test "registers itself as a leader if there is no global name registered" do assert {:ok, pid} = start_link({{__MODULE__, :send_test_message}, 25, test_pid: self()}) - assert_receive {:executed, ^pid, _time}, 500 + assert_receive {:executed, ^pid, _time}, 1000 name = {Domain.Jobs.Executors.Global, __MODULE__, :send_test_message} assert :global.whereis_name(name) == pid diff --git a/elixir/apps/web/lib/web/controllers/error_controller.ex b/elixir/apps/web/lib/web/controllers/error_controller.ex index 5fe119d1d..d29bb2778 100644 --- a/elixir/apps/web/lib/web/controllers/error_controller.ex +++ b/elixir/apps/web/lib/web/controllers/error_controller.ex @@ -4,7 +4,6 @@ defmodule Web.ErrorController do def show(_conn, params) do case params["code"] do "404" -> raise Web.LiveErrors.NotFoundError - "422" -> raise Web.LiveErrors.InvalidRequestError "500" -> raise "internal server error" end diff --git a/elixir/apps/web/lib/web/controllers/error_html/422.html.heex b/elixir/apps/web/lib/web/controllers/error_html/422.html.heex deleted file mode 100644 index 86ca63921..000000000 --- a/elixir/apps/web/lib/web/controllers/error_html/422.html.heex +++ /dev/null @@ -1,44 +0,0 @@ - - - - - - - - - - - - - 422 Error - - - - -
-
-
- Firezone Logo -

- Error422 -

-

- Something went wrong. We've already been notified and will get it fixed as soon as possible. -

-
-
-
- - diff --git a/elixir/apps/web/lib/web/live/actors/index.ex b/elixir/apps/web/lib/web/live/actors/index.ex index 345ced74b..17b0580cf 100644 --- a/elixir/apps/web/lib/web/live/actors/index.ex +++ b/elixir/apps/web/lib/web/live/actors/index.ex @@ -9,6 +9,8 @@ defmodule Web.Actors.Index do |> assign(page_title: "Actors") |> assign_live_table("actors", query_module: Actors.Actor.Query, + # TODO[bmanifold]: Enable this filter once we start collapsing them + hide_filters: [:type], sortable_fields: [ {:actors, :name} ], diff --git a/elixir/apps/web/lib/web/live/flows/download_activities.ex b/elixir/apps/web/lib/web/live/flows/download_activities.ex index 153799a34..f2eb4c069 100644 --- a/elixir/apps/web/lib/web/live/flows/download_activities.ex +++ b/elixir/apps/web/lib/web/live/flows/download_activities.ex @@ -36,8 +36,6 @@ defmodule Web.Flows.DownloadActivities do with {:ok, activities, activities_metadata} <- Flows.list_flow_activities_for( flow, - flow.inserted_at, - flow.expires_at, conn.assigns.subject, page: [cursor: cursor, limit: 100] ), diff --git a/elixir/apps/web/lib/web/live/flows/show.ex b/elixir/apps/web/lib/web/live/flows/show.ex index 53990f711..58426760e 100644 --- a/elixir/apps/web/lib/web/live/flows/show.ex +++ b/elixir/apps/web/lib/web/live/flows/show.ex @@ -1,10 +1,11 @@ defmodule Web.Flows.Show do use Web, :live_view import Web.Policies.Components - alias Domain.{Flows, Flows} + alias Domain.{Accounts, Flows} def mount(%{"id" => id}, _session, socket) do - with {:ok, flow} <- + with true <- Accounts.flow_activities_enabled?(socket.assigns.account), + {:ok, flow} <- Flows.fetch_flow_by_id(id, socket.assigns.subject, preload: [ policy: [:resource, :actor_group], @@ -16,11 +17,18 @@ defmodule Web.Flows.Show do last_used_connectivity_type = get_last_used_connectivity_type(flow, socket.assigns.subject) socket = - assign(socket, + socket + |> assign( page_title: "Flow #{flow.id}", flow: flow, last_used_connectivity_type: last_used_connectivity_type ) + |> assign_live_table("activities", + query_module: Flows.Activity.Query, + sortable_fields: [], + limit: 10, + callback: &handle_activities_update!/2 + ) {:ok, socket} else @@ -35,6 +43,22 @@ defmodule Web.Flows.Show do end end + def handle_params(params, uri, socket) do + socket = handle_live_tables_params(socket, params, uri) + {:noreply, socket} + end + + def handle_activities_update!(socket, list_opts) do + with {:ok, activities, metadata} <- + Flows.list_flow_activities_for(socket.assigns.flow, socket.assigns.subject, list_opts) do + {:ok, + assign(socket, + activities: activities, + activities_metadata: metadata + )} + end + end + def render(assigns) do ~H""" <.breadcrumbs account={@account}> @@ -116,6 +140,49 @@ defmodule Web.Flows.Show do + + <.section> + <:title>Metrics + <:help> + Pre-aggregated metrics for this flow. + + <:content> + <.live_table + id="activities" + rows={@activities} + row_id={&"activities-#{&1.id}"} + filters={@filters_by_table_id["activities"]} + filter={@filter_form_by_table_id["activities"]} + ordered_by={@order_by_table_id["activities"]} + metadata={@activities_metadata} + > + <:col :let={activity} label="STARTED AT"> + <.relative_datetime datetime={activity.window_started_at} /> + + <:col :let={activity} label="ENDED AT"> + <.relative_datetime datetime={activity.window_ended_at} /> + + <:col :let={activity} label="DESTINATION"> + <%= activity.destination %> + + <:col :let={activity} label="CONNECTIVITY TYPE"> + <%= activity.connectivity_type %> + + <:col :let={activity} label="RX"> + <%= Sizeable.filesize(activity.rx_bytes) %> + + <:col :let={activity} label="TX"> + <%= Sizeable.filesize(activity.tx_bytes) %> + + <:empty> +
No metrics to display.
+ + + + """ end + + def handle_event(event, params, socket) when event in ["paginate", "order_by", "filter"], + do: handle_live_table_event(event, params, socket) end diff --git a/elixir/apps/web/lib/web/live/groups/edit_actors.ex b/elixir/apps/web/lib/web/live/groups/edit_actors.ex index 6bfb1670d..b910c02cb 100644 --- a/elixir/apps/web/lib/web/live/groups/edit_actors.ex +++ b/elixir/apps/web/lib/web/live/groups/edit_actors.ex @@ -29,7 +29,7 @@ defmodule Web.Groups.EditActors do sortable_fields: [ {:actors, :name} ], - hide_filters: [:type, :provider_id], + hide_filters: [:type, :provider_id, :status], callback: &handle_actors_update!/2 ) diff --git a/elixir/apps/web/lib/web/live/groups/show.ex b/elixir/apps/web/lib/web/live/groups/show.ex index 33b7b5c29..f98ce206f 100644 --- a/elixir/apps/web/lib/web/live/groups/show.ex +++ b/elixir/apps/web/lib/web/live/groups/show.ex @@ -22,7 +22,7 @@ defmodule Web.Groups.Show do sortable_fields: [ {:actors, :name} ], - hide_filters: [:type, :provider_id], + hide_filters: [:type, :status, :provider_id], callback: &handle_actors_update!/2 ) |> assign_live_table("policies", diff --git a/elixir/apps/web/lib/web/live_errors.ex b/elixir/apps/web/lib/web/live_errors.ex index bfc348dba..501d82b67 100644 --- a/elixir/apps/web/lib/web/live_errors.ex +++ b/elixir/apps/web/lib/web/live_errors.ex @@ -7,13 +7,4 @@ defmodule Web.LiveErrors do def actions(_exception), do: [] end end - - defmodule InvalidRequestError do - defexception message: "Unprocessable Entity" - - defimpl Plug.Exception do - def status(_exception), do: 422 - def actions(_exception), do: [] - end - end end diff --git a/elixir/apps/web/lib/web/live_table.ex b/elixir/apps/web/lib/web/live_table.ex index 5ad961d00..445e23868 100644 --- a/elixir/apps/web/lib/web/live_table.ex +++ b/elixir/apps/web/lib/web/live_table.ex @@ -103,6 +103,81 @@ defmodule Web.LiveTable do """ end + def datetime_input(assigns) do + ~H""" +
+ + "[#{@from_or_to}][time]"} + id={@field.id <> "[#{@from_or_to}][time]"} + value={normalize_value("time", Map.get(@field.value || %{}, @from_or_to)) || "00:00:00"} + class={[ + "bg-neutral-50 border border-neutral-300 text-neutral-900 text-sm rounded", + "block w-1/2", + "phx-no-feedback:border-neutral-300", + "disabled:bg-neutral-50 disabled:text-neutral-500 disabled:border-neutral-200 disabled:shadow-none", + "focus:outline-none focus:border-1 focus:ring-0", + "border-neutral-300", + @field.errors != [] && "border-rose-400" + ]} + /> + <.error :for={msg <- @field.errors} data-validation-error-for={@field.name}> + <%= msg %> + +
+ """ + end + + defp normalize_value("date", %DateTime{} = datetime), + do: DateTime.to_date(datetime) |> Date.to_iso8601() + + defp normalize_value("time", %DateTime{} = datetime), + do: DateTime.to_time(datetime) |> Time.to_iso8601() + + defp normalize_value(_, nil), + do: nil + + defp filter(%{filter: %{type: {:range, :datetime}}} = assigns) do + ~H""" +
+ <.datetime_input + field={@form[@filter.name]} + filter={@filter} + from_or_to={:from} + max={Date.utc_today()} + /> +
to
+ <.datetime_input + field={@form[@filter.name]} + filter={@filter} + from_or_to={:to} + max={Date.utc_today()} + /> +
+ """ + end + defp filter(%{filter: %{type: {:string, :websearch}}} = assigns) do ~H"""
@@ -432,7 +507,8 @@ defmodule Web.LiveTable do case callback.(socket, list_opts) do {:error, _reason} -> - push_navigate(socket, to: socket.assigns.uri) + uri = URI.parse(socket.assigns.uri) + push_navigate(socket, to: uri.path) {:ok, socket} -> :ok = maybe_notify_test_pid(id) @@ -458,75 +534,92 @@ defmodule Web.LiveTable do with the new state. """ def handle_live_tables_params(socket, params, uri) do - socket = - Enum.reduce(socket.assigns.live_table_ids, socket, fn id, socket -> - handle_live_table_params(socket, params, id) - end) + socket = assign(socket, uri: uri) - assign(socket, uri: uri) + Enum.reduce(socket.assigns.live_table_ids, socket, fn id, socket -> + handle_live_table_params(socket, params, id) + end) end defp handle_live_table_params(socket, params, id) do enforced_filters = Map.fetch!(socket.assigns.enforced_filters_by_table_id, id) - filter = enforced_filters ++ params_to_filter(id, params) + sortable_fields = Map.fetch!(socket.assigns.sortable_fields_by_table_id, id) limit = Map.fetch!(socket.assigns.limit_by_table_id, id) - page = params_to_page(id, limit, params) - order_by = - socket.assigns.sortable_fields_by_table_id - |> Map.fetch!(id) - |> params_to_order_by(id, params) + with {:ok, filter} <- params_to_filter(id, params), + filter = enforced_filters ++ filter, + {:ok, page} <- params_to_page(id, limit, params), + {:ok, order_by} <- params_to_order_by(sortable_fields, id, params) do + list_opts = [ + page: page, + filter: filter, + order_by: List.wrap(order_by) + ] - list_opts = [ - page: page, - filter: filter, - order_by: List.wrap(order_by) - ] + case maybe_apply_callback(socket, id, list_opts) do + {:ok, socket} -> + socket + |> assign( + filter_form_by_table_id: + put_table_state( + socket, + id, + :filter_form_by_table_id, + filter_to_form(filter, id) + ), + order_by_table_id: + put_table_state( + socket, + id, + :order_by_table_id, + order_by + ), + list_opts_by_table_id: + put_table_state( + socket, + id, + :list_opts_by_table_id, + list_opts + ) + ) - case maybe_apply_callback(socket, id, list_opts) do - {:ok, socket} -> - socket - |> assign( - filter_form_by_table_id: - put_table_state( - socket, - id, - :filter_form_by_table_id, - filter_to_form(filter, id) - ), - order_by_table_id: - put_table_state( - socket, - id, - :order_by_table_id, - order_by - ), - list_opts_by_table_id: - put_table_state( - socket, - id, - :list_opts_by_table_id, - list_opts - ) - ) + {:error, :invalid_cursor} -> + message = "The page was reset due to invalid pagination cursor." + reset_live_table_params(socket, id, message) - {:error, :invalid_cursor} -> - raise Web.LiveErrors.InvalidRequestError + {:error, {:unknown_filter, _metadata}} -> + message = "The page was reset due to use of undefined pagination filter." + reset_live_table_params(socket, id, message) - {:error, {:unknown_filter, _metadata}} -> - raise Web.LiveErrors.InvalidRequestError + {:error, {:invalid_type, _metadata}} -> + message = "The page was reset due to invalid value of a pagination filter." + reset_live_table_params(socket, id, message) - {:error, {:invalid_type, _metadata}} -> - raise Web.LiveErrors.InvalidRequestError + {:error, {:invalid_value, _metadata}} -> + message = "The page was reset due to invalid value of a pagination filter." + reset_live_table_params(socket, id, message) - {:error, {:invalid_value, _metadata}} -> - raise Web.LiveErrors.InvalidRequestError - - {:error, _reason} -> - raise Web.LiveErrors.NotFoundError + {:error, _reason} -> + raise Web.LiveErrors.NotFoundError + end + else + {:error, :invalid_filter} -> + message = "The page was reset due to invalid pagination filter." + reset_live_table_params(socket, id, message) end end + defp reset_live_table_params(socket, id, message) do + {:noreply, socket} = + socket + |> put_flash(:error, message) + |> update_query_params(fn query_params -> + Map.reject(query_params, fn {key, _} -> String.starts_with?(key, "#{id}_") end) + end) + + socket + end + defp maybe_apply_callback(socket, id, list_opts) do previous_list_opts = Map.get(socket.assigns[:list_opts_by_table_id] || %{}, id, []) @@ -557,19 +650,59 @@ defmodule Web.LiveTable do defp params_to_page(id, limit, params) do if cursor = Map.get(params, "#{id}_cursor") do - [cursor: cursor, limit: limit] + {:ok, [cursor: cursor, limit: limit]} else - [limit: limit] + {:ok, [limit: limit]} end end defp params_to_filter(id, params) do - for {key, value} <- Map.get(params, "#{id}_filter", []), - value != "" do - {String.to_existing_atom(key), value} + params + |> Map.get("#{id}_filter", []) + |> Enum.reduce_while({:ok, []}, fn {key, value}, {:ok, acc} -> + case cast_filter(value) do + {:ok, nil} -> {:cont, acc} + {:ok, value} -> {:cont, {:ok, [{String.to_existing_atom(key), value}] ++ acc}} + {:error, reason} -> {:halt, {:error, reason}} + end + end) + end + + defp cast_filter(%{"from" => from, "to" => to}) do + with {:ok, from, 0} <- DateTime.from_iso8601(from), + {:ok, to, 0} <- DateTime.from_iso8601(to) do + {:ok, %Domain.Repo.Filter.Range{from: from, to: to}} + else + _other -> {:error, :invalid_filter} end - rescue - ArgumentError -> [] + end + + defp cast_filter(%{"to" => to}) do + with {:ok, to, 0} <- DateTime.from_iso8601(to) do + {:ok, %Domain.Repo.Filter.Range{to: to}} + else + _other -> {:error, :invalid_filter} + end + end + + defp cast_filter(%{"from" => from}) do + with {:ok, from, 0} <- DateTime.from_iso8601(from) do + {:ok, %Domain.Repo.Filter.Range{from: from}} + else + _other -> {:error, :invalid_filter} + end + end + + defp cast_filter("") do + {:ok, nil} + end + + defp cast_filter(binary) when is_binary(binary) do + {:ok, binary} + end + + defp cast_filter(_other) do + {:error, :invalid_filter} end @doc false @@ -582,8 +715,11 @@ defmodule Web.LiveTable do end defp params_to_order_by(sortable_fields, id, params) do - Map.get(params, "#{id}_order_by", "") - |> parse_order_by(sortable_fields) + order_by = + Map.get(params, "#{id}_order_by", "") + |> parse_order_by(sortable_fields) + + {:ok, order_by} end defp parse_order_by(order_by, sortable_fields) do @@ -679,16 +815,61 @@ defmodule Web.LiveTable do end defp put_filter_to_params(params, id, filter) do - filter_params = - for {key, value} <- filter, - value != "", - value != "__all__", - into: %{} do - {"#{id}_filter[#{key}]", value} - end + filter_params = flatten_filter(filter, "#{id}_filter", %{}) params |> Map.reject(fn {key, _} -> String.starts_with?(key, "#{id}_filter") end) |> Map.merge(filter_params) end + + defp flatten_filter([], _key_prefix, acc) do + acc + end + + defp flatten_filter(map, key_prefix, acc) when is_map(map) do + flatten_filter(Map.to_list(map), key_prefix, acc) + end + + defp flatten_filter([{_key, ""} | rest], key_prefix, acc) do + flatten_filter(rest, key_prefix, acc) + end + + defp flatten_filter([{_key, "__all__"} | rest], key_prefix, acc) do + flatten_filter(rest, key_prefix, acc) + end + + defp flatten_filter([{key, %{"date" => _} = datetime_range_filter} | rest], key_prefix, acc) do + if value = normalize_datetime_filter(datetime_range_filter) do + flatten_filter(rest, key_prefix, Map.put(acc, "#{key_prefix}[#{key}]", value)) + else + flatten_filter(rest, key_prefix, acc) + end + end + + defp flatten_filter([{key, value} | rest], key_prefix, acc) + when is_list(value) or is_map(value) do + acc = Map.merge(acc, flatten_filter(value, "#{key_prefix}[#{key}]", %{})) + flatten_filter(rest, key_prefix, acc) + end + + defp flatten_filter([{key, value} | rest], key_prefix, acc) do + flatten_filter(rest, key_prefix, Map.put(acc, "#{key_prefix}[#{key}]", value)) + end + + defp normalize_datetime_filter(params) do + with {:ok, date} <- Date.from_iso8601(params["date"]), + {:ok, time} <- normalize_time_filter(params["time"] || "00:00:00") do + DateTime.new!(date, time) |> DateTime.to_iso8601() + else + _other -> nil + end + end + + defp normalize_time_filter(time) when byte_size(time) == 5 do + Time.from_iso8601(time <> ":00") + end + + defp normalize_time_filter(time) do + Time.from_iso8601(time) + end end diff --git a/elixir/apps/web/test/web/controllers/error_html_test.exs b/elixir/apps/web/test/web/controllers/error_html_test.exs index 2c7895d57..e58938fa9 100644 --- a/elixir/apps/web/test/web/controllers/error_html_test.exs +++ b/elixir/apps/web/test/web/controllers/error_html_test.exs @@ -10,16 +10,6 @@ defmodule Web.ErrorHTMLTest do assert body =~ "Sorry, we couldn't find this page" end - test "renders 422.html", %{conn: conn} do - {_code, _headers, body} = - assert_error_sent 422, fn -> - get(conn, ~p"/error/422") - end - - assert body =~ "Something went wrong" - assert body =~ "We've already been notified and will get it fixed as soon as possible" - end - test "renders 500.html", %{conn: conn} do {_code, _headers, body} = assert_error_sent 500, fn -> diff --git a/elixir/apps/web/test/web/live/flows/show_test.exs b/elixir/apps/web/test/web/live/flows/show_test.exs index defcb43d6..50eebf582 100644 --- a/elixir/apps/web/test/web/live/flows/show_test.exs +++ b/elixir/apps/web/test/web/live/flows/show_test.exs @@ -164,4 +164,39 @@ defmodule Web.Live.Flows.ShowTest do ] ] end + + test "renders activities table", %{ + account: account, + flow: flow, + identity: identity, + conn: conn + } do + activity = + Fixtures.Flows.create_activity( + account: account, + flow: flow, + window_started_at: DateTime.truncate(flow.inserted_at, :second), + window_ended_at: DateTime.truncate(flow.expires_at, :second), + tx_bytes: 1024 * 1024 * 1024 * 42 + ) + + {:ok, lv, _html} = + conn + |> authorize_conn(identity) + |> live(~p"/#{account}/flows/#{flow}") + + [row] = + lv + |> element("#activities") + |> render() + |> table_to_map() + + assert row["started at"] + assert row["ended at"] + + assert row["connectivity type"] == to_string(activity.connectivity_type) + assert row["destination"] == to_string(activity.destination) + assert row["rx"] == "#{activity.rx_bytes} B" + assert row["tx"] == "42 GB" + end end diff --git a/elixir/apps/web/test/web/live_table_test.exs b/elixir/apps/web/test/web/live_table_test.exs index 09f780f93..334a1762e 100644 --- a/elixir/apps/web/test/web/live_table_test.exs +++ b/elixir/apps/web/test/web/live_table_test.exs @@ -474,7 +474,7 @@ defmodule Web.LiveTableTest do socket = %Phoenix.LiveView.Socket{ - assigns: %{subject: subject, uri: "/current_uri", __changed__: %{}} + assigns: %{subject: subject, uri: "http://foo.bar/current_uri", __changed__: %{}} } |> assign_live_table("table-id", query_module: Actors.Actor.Query, @@ -559,7 +559,38 @@ defmodule Web.LiveTableTest do refute_receive {:callback, _socket, _list_opts} end - test "raises if the callback returns an error" do + test "raises if the table params are invalid" do + subject = Fixtures.Auth.create_subject() + + socket = + %Phoenix.LiveView.Socket{ + assigns: %{subject: subject, uri: "/current_uri", __changed__: %{}, flash: %{}} + } + + for {reason, message} <- [ + {:invalid_cursor, "The page was reset due to invalid pagination cursor."}, + {{:unknown_filter, []}, + "The page was reset due to use of undefined pagination filter."}, + {{:invalid_type, []}, + "The page was reset due to invalid value of a pagination filter."}, + {{:invalid_value, []}, + "The page was reset due to invalid value of a pagination filter."} + ] do + socket = + assign_live_table(socket, "table-id", + query_module: Actors.Actor.Query, + sortable_fields: [ + {:actors, :name} + ], + callback: fn _socket, _list_opts -> {:error, reason} end + ) + + socket = handle_live_tables_params(socket, %{}, "/foo") + assert socket.assigns.flash == %{"error" => message} + end + end + + test "raises if the callback returns a generic error" do subject = Fixtures.Auth.create_subject() socket = @@ -569,11 +600,7 @@ defmodule Web.LiveTableTest do for {reason, exception} <- [ {:not_found, Web.LiveErrors.NotFoundError}, - {:unauthorized, Web.LiveErrors.NotFoundError}, - {:invalid_cursor, Web.LiveErrors.InvalidRequestError}, - {{:unknown_filter, []}, Web.LiveErrors.InvalidRequestError}, - {{:invalid_type, []}, Web.LiveErrors.InvalidRequestError}, - {{:invalid_value, []}, Web.LiveErrors.InvalidRequestError} + {:unauthorized, Web.LiveErrors.NotFoundError} ] do socket = assign_live_table(socket, "table-id",