-
Notifications
You must be signed in to change notification settings - Fork 66
Paginate superuser users/projects lists with server-side search #4474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
17d082f
0679a9d
4c85c38
fb9006d
4681703
7990c77
1ee5649
fcf4b25
67ca8db
5ab22f0
a560814
d84db8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ defmodule Lightning.Accounts do | |||||
|
|
||||||
| alias Ecto.Changeset | ||||||
| alias Ecto.Multi | ||||||
| alias Lightning.Accounts.AdminSearchParams | ||||||
| alias Lightning.Accounts.Events | ||||||
| alias Lightning.Accounts.User | ||||||
| alias Lightning.Accounts.UserBackupCode | ||||||
|
|
@@ -114,6 +115,20 @@ defmodule Lightning.Accounts do | |||||
| Repo.all(User) | ||||||
| end | ||||||
|
|
||||||
| @doc """ | ||||||
| Returns a paginated list of users for the superuser admin table with | ||||||
| server-side filtering and sorting. | ||||||
| """ | ||||||
| @spec list_users_for_admin(map()) :: Scrivener.Page.t() | ||||||
| def list_users_for_admin(params \\ %{}) do | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The context function should take the typed params instead of building it inside the body. This way we establish a "contract" with the callers. We should also rename the function, to |
||||||
| params = AdminSearchParams.new(params) | ||||||
|
|
||||||
| User | ||||||
| |> filter_admin_users(params.filter) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| |> order_admin_users(params.sort, params.dir) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| |> Repo.paginate(AdminSearchParams.pagination_opts(params)) | ||||||
| end | ||||||
|
|
||||||
| @doc """ | ||||||
| Returns the list of users with the given emails | ||||||
| """ | ||||||
|
|
@@ -160,6 +175,48 @@ defmodule Lightning.Accounts do | |||||
| if User.valid_password?(user, password), do: user | ||||||
| end | ||||||
|
|
||||||
| defp filter_admin_users(query, ""), do: query | ||||||
|
|
||||||
| defp filter_admin_users(query, filter) do | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| search = "%#{filter}%" | ||||||
|
|
||||||
| where( | ||||||
| query, | ||||||
| [u], | ||||||
| ilike(u.first_name, ^search) or | ||||||
| ilike(u.last_name, ^search) or | ||||||
| ilike(u.email, ^search) or | ||||||
| ilike(fragment("?::text", u.role), ^search) | ||||||
| ) | ||||||
| end | ||||||
|
|
||||||
| defp order_admin_users(query, "enabled", "asc"), | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to sort by a boolean. This should be toggle somewhere on the form. Let's skip it for now |
||||||
| do: order_by(query, [u], desc: u.disabled) | ||||||
|
|
||||||
| defp order_admin_users(query, "enabled", "desc"), | ||||||
| do: order_by(query, [u], asc: u.disabled) | ||||||
|
|
||||||
| defp order_admin_users(query, "scheduled_deletion", "asc"), | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| do: order_by(query, [u], asc_nulls_last: u.scheduled_deletion) | ||||||
|
|
||||||
| defp order_admin_users(query, "scheduled_deletion", "desc"), | ||||||
| do: order_by(query, [u], desc_nulls_first: u.scheduled_deletion) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Given |
||||||
|
|
||||||
| defp order_admin_users(query, "role", dir) do | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't see the need to sort by |
||||||
| direction = dir_to_atom(dir) | ||||||
| order_by(query, [u], [{^direction, fragment("?::text", u.role)}]) | ||||||
| end | ||||||
|
|
||||||
| defp order_admin_users(query, sort, dir) do | ||||||
| direction = dir_to_atom(dir) | ||||||
| sort_field = String.to_existing_atom(sort) | ||||||
|
|
||||||
| order_by(query, [u], [{^direction, field(u, ^sort_field)}]) | ||||||
| end | ||||||
|
|
||||||
| defp dir_to_atom("asc"), do: :asc | ||||||
| defp dir_to_atom("desc"), do: :desc | ||||||
|
|
||||||
| @doc """ | ||||||
| Gets a single user. | ||||||
|
|
||||||
|
|
||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to stringify and normalize while using ecto changesets. The changesets will do this for you, look at https://github.com/OpenFn/lightning/blob/main/lib/lightning/workorders/search_params.ex. You can read more about Ecto changesets in https://hexdocs.pm/ecto/Ecto.Changeset.html
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, could you use the same field names as the workorders search params? The |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,118 @@ | ||||||
| defmodule Lightning.Accounts.AdminSearchParams do | ||||||
| @moduledoc """ | ||||||
| Normalized query params for the superuser users table. | ||||||
| """ | ||||||
|
|
||||||
| use Lightning.Schema | ||||||
|
|
||||||
| @primary_key false | ||||||
|
|
||||||
| @default_sort "email" | ||||||
| @allowed_sorts ~w(first_name last_name email role enabled support_user scheduled_deletion) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| @default_page 1 | ||||||
| @default_page_size 10 | ||||||
| @max_page_size 100 | ||||||
|
|
||||||
| @type t :: %__MODULE__{ | ||||||
| filter: String.t(), | ||||||
| sort: String.t(), | ||||||
| dir: String.t(), | ||||||
| page: pos_integer(), | ||||||
| page_size: pos_integer() | ||||||
| } | ||||||
|
|
||||||
| embedded_schema do | ||||||
| field :filter, :string, default: "" | ||||||
| field :sort, :string, default: @default_sort | ||||||
| field :dir, :string, default: "asc" | ||||||
| field :page, :integer, default: @default_page | ||||||
| field :page_size, :integer, default: @default_page_size | ||||||
| end | ||||||
|
|
||||||
| def new(params \\ %{}) | ||||||
| def new(%__MODULE__{} = params), do: params | ||||||
| def new(nil), do: new(%{}) | ||||||
|
|
||||||
| def new(params) when is_map(params) do | ||||||
| params = stringify_param_keys(params) | ||||||
|
|
||||||
| %__MODULE__{} | ||||||
| |> cast( | ||||||
| %{ | ||||||
| "filter" => normalize_filter(Map.get(params, "filter")), | ||||||
| "sort" => normalize_sort(Map.get(params, "sort")), | ||||||
| "dir" => normalize_dir(Map.get(params, "dir")), | ||||||
| "page" => parse_positive_int(Map.get(params, "page"), @default_page), | ||||||
| "page_size" => | ||||||
| Map.get(params, "page_size") | ||||||
| |> parse_positive_int(@default_page_size) | ||||||
| |> min(@max_page_size) | ||||||
| }, | ||||||
| [:filter, :sort, :dir, :page, :page_size] | ||||||
| ) | ||||||
| |> apply_action!(:validate) | ||||||
| end | ||||||
|
|
||||||
| def default_uri_params do | ||||||
| new() | ||||||
| |> to_uri_params() | ||||||
| end | ||||||
|
|
||||||
| def pagination_opts(%__MODULE__{} = params) do | ||||||
| [page: params.page, page_size: params.page_size] | ||||||
| end | ||||||
|
|
||||||
| def to_uri_params(%__MODULE__{} = params) do | ||||||
| %{ | ||||||
| "filter" => params.filter, | ||||||
| "sort" => params.sort, | ||||||
| "dir" => params.dir, | ||||||
| "page" => Integer.to_string(params.page), | ||||||
| "page_size" => Integer.to_string(params.page_size) | ||||||
| } | ||||||
| end | ||||||
|
|
||||||
| def to_uri_params(params) when is_map(params) do | ||||||
| params | ||||||
| |> new() | ||||||
| |> to_uri_params() | ||||||
| end | ||||||
|
|
||||||
| defp normalize_sort(sort) when is_binary(sort) do | ||||||
| if sort in @allowed_sorts, do: sort, else: @default_sort | ||||||
| end | ||||||
|
|
||||||
| defp normalize_sort(sort) when is_atom(sort) do | ||||||
| sort | ||||||
| |> Atom.to_string() | ||||||
| |> normalize_sort() | ||||||
| end | ||||||
|
|
||||||
| defp normalize_sort(_), do: @default_sort | ||||||
|
|
||||||
| defp normalize_dir(dir) when dir in ["asc", :asc], do: "asc" | ||||||
| defp normalize_dir(dir) when dir in ["desc", :desc], do: "desc" | ||||||
| defp normalize_dir(_), do: "asc" | ||||||
|
|
||||||
| defp normalize_filter(nil), do: "" | ||||||
|
|
||||||
| defp normalize_filter(filter) do | ||||||
| filter | ||||||
| |> to_string() | ||||||
| |> String.trim() | ||||||
| end | ||||||
|
|
||||||
| defp stringify_param_keys(params) do | ||||||
| Map.new(params, fn {key, value} -> {to_string(key), value} end) | ||||||
| end | ||||||
|
|
||||||
| defp parse_positive_int(value, _default) when is_integer(value) and value > 0, | ||||||
| do: value | ||||||
|
|
||||||
| defp parse_positive_int(value, default) do | ||||||
| case Integer.parse(to_string(value || "")) do | ||||||
| {int, ""} when int > 0 -> int | ||||||
| _ -> default | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
Uh oh!
There was an error while loading. Please reload this page.