Skip to content
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

feat(portal): Filtering, Fulltext Search, Pagination, Preloads #3751

Merged
merged 33 commits into from
Mar 16, 2024

Conversation

AndrewDryga
Copy link
Collaborator

@AndrewDryga AndrewDryga commented Feb 23, 2024

On the domain side this PR extends Domain.Repo with filtering, pagination, and ordering, along with some convention changes are removing the code that is not needed since we have the filtering now. This required to touch pretty much all contexts and code, but I went through all public functions and added missing tests to make sure nothing will be broken.

On the web side I've introduced a <.live_table /> which is as close as possible to being a drop-in replacement for the regular <.table /> (but requires to structure the LiveView module differently due to assigns anyways). I've updated all the listing tables to use it.

Copy link

vercel bot commented Feb 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Mar 16, 2024 7:12pm

Copy link

github-actions bot commented Feb 23, 2024

Terraform Cloud Plan Output

Plan: 9 to add, 8 to change, 9 to destroy.

Terraform Cloud Plan

@AndrewDryga AndrewDryga changed the title Filtering, Pagination, Preloads Filtering, Fulltext Search, Pagination, Preloads Feb 23, 2024
@AndrewDryga AndrewDryga force-pushed the andrew/filtering-and-paging branch 2 times, most recently from 4915e56 to 3af1033 Compare February 28, 2024 16:51
gateway_groups = Enum.map(gateways, & &1.group),
{relay_hosting_type, relay_connection_type} = Gateways.relay_strategy(gateway_groups),
{:ok, [_ | _] = relays} <-
Relays.list_connected_relays_for_resource(resource, relay_hosting_type) do
Relays.all_connected_relays_for_resource(resource, relay_hosting_type) do
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have various kinds of returns I've decided to come up with a convention just for us:

  • list_* returns {:ok, results, metadata} - basically a paginated query used everywhere in the UI
  • all_* returns {:ok, results} - used in places where we need to really fetch all the results without pagination (eg. list of enabled providers when we build the sign in form)
  • all_*! returns results or raises an error. This is when we just do the query and there is no need to wrap it in the tuple, mostly used for internal stuff like background jobs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)
end

def by_slug(queryable \\ not_deleted(), slug) do
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed \\ queryable() everywhere so that our domain modules are more explicit, before you had to go to query and see if it will return all, or deleted, or active, or whatever records. Now you have to explicitly set that as a first stage when a query is built.

@AndrewDryga AndrewDryga force-pushed the andrew/filtering-and-paging branch 3 times, most recently from 6df1ec9 to ebf1f1e Compare March 14, 2024 17:48
@AndrewDryga AndrewDryga changed the title Filtering, Fulltext Search, Pagination, Preloads feat(portal): Filtering, Fulltext Search, Pagination, Preloads Mar 14, 2024
@AndrewDryga AndrewDryga marked this pull request as ready for review March 14, 2024 17:58
Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to do an in-depth review of the code (will leave that to @bmanifold after he's back).

Some usability fixes / requests:

  • I don't think the Showing 25 of 304 is updated when flipping through pages (e.g. on the Groups page). Is that intended?
  • Clients page: Could we add sorting to the User column too? I think that will be used quite a bit. And maybe add the "All / Online / Offline" button group (or allow sorting by status).
  • Policies -- add sort for Group and Resource
  • Can you space the sort form elements horizontally to make better use of the space? e.g. this is fine on small screens (grid-cols-2 that collapses to grid-cols-1)
Screenshot 2024-03-15 at 11 10 44 AM

But on larger screens I would expect the filter elements to be side-by-side:

Screenshot 2024-03-15 at 11 11 59 AM

On the Actors page it's a little more problematic:
Screenshot 2024-03-15 at 11 15 38 AM

Maybe all of these can be contained inside a dropdown menu that's activated by a filter button?

gateway_groups = Enum.map(gateways, & &1.group),
{relay_hosting_type, relay_connection_type} = Gateways.relay_strategy(gateway_groups),
{:ok, [_ | _] = relays} <-
Relays.list_connected_relays_for_resource(resource, relay_hosting_type) do
Relays.all_connected_relays_for_resource(resource, relay_hosting_type) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@AndrewDryga
Copy link
Collaborator Author

AndrewDryga commented Mar 15, 2024

@jamilbk

I don't think the Showing 25 of 304 is updated when flipping through pages (e.g. on the Groups page). Is that intended?

Yes, 25 is the page size, and 304 is the total count. We can't and don't say on which page you are currently since it's cursor-based.

Clients page: Could we add sorting to the User column too? I think that will be used quite a bit.

We can but:

  • I think sorting by actor name doesn't make a ton of sense because you can go to the actor and see all its clients there;
  • Sorting on a result of a join expression will be expensive and we won't ever be able to build an index for it so better we don't add this feature without a customer request.

Policies -- add sort for Group and Resource

Same reason here, it's not the best idea to paginate using the result of a join, you can always use a filter to find the ones you need or use a different view to see group policies (group show view) or resource policies (resource show view).

And maybe add the "All / Online / Offline" button group (or allow sorting by status).

This is hard to do because presence is not stored in the database and we need to come up with a smart way for those filters to work without hitting our database with a gigantic WHERE IN ... for large customers, so I postponed this kind of filter for the future.

Maybe all of these can be contained inside a dropdown menu that's activated by a filter button?

I'd suggest improving UX in a separate PR, this will spawn all sorts of look&feel discussions that will take time, while I think it's better if we get this merged (it's already functional and not so horrible) to unblock the clients that have thousands of groups first. There are lots of ways how filters can be improved: we hide them below the collapse button, use grids, add/remove some of the filters, etc.

Can you space the sort form elements horizontally to make better use of the space? e.g. this is fine on small screens (grid-cols-2 that collapses to grid-cols-1)

I think we need a better breakpoint so it extends on smaller screens, I'll take care of that.

@jamilbk jamilbk self-requested a review March 15, 2024 19:38
Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to fix the timeouts currently impacting customers. Fixes can be addressed in followup PRs.

Copy link

github-actions bot commented Mar 16, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 231.3 MiB (+1%) 233.2 MiB (+2%) 284 (+21%)
direct-tcp-server2client 238.6 MiB (+0%) 240.4 MiB (+0%) 256 (+17%)
relayed-tcp-client2server 147.2 MiB (-1%) 147.7 MiB (-1%) 176 (-5%)
relayed-tcp-server2client 160.5 MiB (+8%) 161.0 MiB (+7%) 166 (+3%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 50.0 MiB (-0%) 0.04ms (+21%) 0.00% (NaN%)
direct-udp-server2client 50.0 MiB (-0%) 0.01ms (+12%) 0.00% (NaN%)
relayed-udp-client2server 50.0 MiB (-0%) 0.09ms (-4%) 0.00% (NaN%)
relayed-udp-server2client 50.0 MiB (-0%) 0.05ms (+11%) 0.00% (NaN%)

@AndrewDryga AndrewDryga merged commit f3c8c73 into main Mar 16, 2024
137 checks passed
@AndrewDryga AndrewDryga deleted the andrew/filtering-and-paging branch March 16, 2024 19:27
@jamilbk
Copy link
Member

jamilbk commented Mar 17, 2024

@bmanifold This was merged to fix production for a customer with lots of accounts and groups, but could still use your review (and a little bit of UI/UX CSS cleanup noted in my review).

@AndrewDryga I think in the future what might be good for large PRs like this is to maybe split the PR into a UI PR and backend PR? That way we can discuss UI/UX (maybe sooner) without it impeding the backend fixes needed. Or use something quicker like wireframes to figure out how it will work/look before implementing.

@AndrewDryga
Copy link
Collaborator Author

AndrewDryga commented Mar 17, 2024

I think in the future what might be good for large PRs like this is to maybe split the PR into a UI PR and backend PR?

I'm not a big fan of having dead code in the main that we can deploy at any time (like the back-end that can not be used anywhere). We can split work into smaller PRs and then merge them into a temporary branch, but this will just add more work and still require one final "big" review.

That way we can discuss UI/UX (maybe sooner) without it impeding the backend fixes needed. Or use something quicker like wireframes to figure out how it will work/look before implementing.

It's easier for anyone on our team to go and update the HTML than sit and discuss prototypes without being able to use the portal. I think with designs the best approach is to do "good enough and polish after" that rather than trying to wireframe everything and kill our productivity.

For example, I would be always faster writing HTML and LiveView code than using wireframing tools, and by doing so I can also understand the limitations better, what is easy to do and what is not, which components are available in FlowByte, while dragging square boxes around we will make decisions that then can be really hard to implement, which will require another set of discussions, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants