Add users list to admin dashboard

[davwheat]

Fixes #283

Changes proposed in this pull request:
Adds a list of all forum users to the Admin dashboard

Screenshot GIF!

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related documentation PR: (Remove if irrelevant)

To do

• Add username heading
• Add join date heading
• Add email heading
• Add Edit User button
• Add link to user profile
• Add group badges
• Add "other data" modal and button (will show all attributes of the User model, including those of extensions)

[askvortsov1]

Thank you for putting this together! It's certainly a much needed feature. A few initial comments (I know this is WIP) to guide development:

• EditUserModal doesn't rely on any forum components. Perhaps that could be added here as a button?
• A list of group badges might be appropriate here
• Would we want to include any kind of search? I'd lean towards doing that in a separate PR since all the search stuff is currently in the forum component
• Could a table component be a candidate for its own component? It would also be useful for Flag dismiss logging flags#21

[davwheat]

@askvortsov1 Thanks for the comments! I'll address those a bit later on.

EditUserModal is under forum at the moment. Should I move it to common or just import it from forum?

[askvortsov1]

EditUserModal is under forum at the moment. Should I move it to common or just import it from forum?

Stuff used by both common and forum should be under common.

I'm not 100% sure about some of the stuff I listed (especially search, as all components for that are on the forum side). I think that it might be preferable to keep this minimal at first. We'll certainly want to eventually add more powerful management (for example, searching users, selecting checkboxes on the side, batch group assignment, etc), but that might be better as an extension.

[davwheat]

• EditUserModal doesn't rely on any forum components. Perhaps that could be added here as a button?

Added a button to open it up.

• A list of group badges might be appropriate here

Also implemented!

• Would we want to include any kind of search? I'd lean towards doing that in a separate PR since all the search stuff is currently in the forum component

I think it'd be a great addition but, as you said, probably best in a separate PR.

• Could a table component be a candidate for its own component? It would also be useful for flarum/flags#21

If you'd like me to make it a separate component, we should probably discuss a better API for it. I personally like the dynamic columns idea I "created" where each cell is generated by a function with the data passed to it, but we might want to separate these into data and actions, for example, as actions would almost always go at the end of a table row.

---

On another note, I have absolutely no clue how to retrieve the total number of users on the forum to correctly display this at the bottom of the table. If someone could point me in the way to go, that'd be a big help. (But if it needs back-end changes, I will run away faster than you can say "PHP"...)

[askvortsov1]

I personally like the dynamic columns idea I "created" where each cell is generated by a function with the data passed to it, but we might want to separate these into data and actions, for example, as actions would almost always go at the end of a table row.

Yeah I think that's quite sensible. PermissionGrid uses something similar I believe, but I don't know if we'd want to refactor the two under a common use case, that might be a bit much (definitely not in this PR).

But if it needs back-end changes, I will run away faster than you can say "PHP"

It wouldn't not require PHP 😛

[davwheat]

That sounds like a good idea to me!

It wouldn't not require PHP 😛

🏃

[davwheat]

Ok, so I've implemented User::count() as part of the ListUsersController. This works perfectly and allows me to show the correct number of "total pages".

I've no clue about what sort of impact this has on the API response times, especially for very big forums. It's probably something that should be looked into, just in case.

I'm just adding emails to the grid -- I'm thinking they should be blurred by default, just an extra thing to prevent "over shoulder data leakage".

---

PS: Moving the EditUserModal to common is breaking -- should it be exported from forum as well to allow backwards-compatibility?

---

I've updated the starting PR comment (?) with an updated GIF of the user list in action:

[askvortsov1]

Overall this is looking very, very nice! Left a few more comments

[KyrneDev]

PS: Moving the EditUserModal to common is breaking -- should it be exported from forum as well to allow backwards-compatibility?

It actually shouldn't be breaking. Right now we don't use the forum/common/admin namespaces for imports on the extension side - so as long as it is re-exported in the common compat.js file compatibility should be maintained.

When the js files get compiled, the import looks like this flarum.core.compat["components/EditUserModal"]. The classes inside flarum.core.compat are a combination of the common compat.js and either the forum/admin compat.js.

Also, is it only 3 users per page? That could be quite cumbersome for a large forum, maybe expand to 10 users per page? I would also like to see a search/filter function on the grid. We already have a user search API so it's only a matter of connecting the two.

[davwheat]

@KyrneDev It actually shouldn't be breaking. Right now we don't use the forum/common/admin namespaces for imports on the extension side - so as long as it is re-exported in the common compat.js file compatibility should be maintained.

Oh, I learn something new every day! That's great news then.

Also, is it only 3 users per page? That could be quite cumbersome for a large forum, maybe expand to 10 users per page?

It is 10 and can be changed easily. I had it set to 3 for demonstration purposes because I didn't feel like creating 100s of fake users :P

I would also like to see a search/filter function on the grid. We already have a user search API so it's only a matter of connecting the two.

I would too. @askvortsov1 suggested that this should be done in a separate PR because stuffing all of the changes needed for it would bulk up the PR a bunch.

To be honest, a live search feature wouldn't be too tricky and could just use the endpoint without any of the forum search code/components. What do you think of that?

[KyrneDev]

To be honest, a live search feature wouldn't be too tricky and could just use the endpoint without any of the forum search code/components. What do you think of that?

That's what I was thinking. We don't need any special search components, it should be a filter (by username) rather than a search. That's the key distinction. Any thoughts, @askvortsov1/others?

[askvortsov1]

Right now we don't use the forum/common/admin namespaces for imports on the extension side - so as long as it is re-exported in the common compat.js file compatibility should be maintained.

Starting in beta 15, the forum/common/admin are the officially recommended ways to import, although I don't think we announced that. See #2085.

I had it set to 3 for demonstration purposes because I didn't feel like creating 100s of fake users :P

https://discuss.flarum.org/d/21160-fake-data has been a godsend for testing

maybe expand to 10 users per page

I'd say significantly more, maybe 25-50? 10 is not that many with properly sized table rows.

To be honest, a live search feature wouldn't be too tricky and could just use the endpoint without any of the forum search code/components. What do you think of that?

If there's something we can remove from this PR and move into a different one, we should do so because it's already quite large.

[davwheat]

https://discuss.flarum.org/d/21160-fake-data has been a godsend for testing

I'll use that, thanks! Riiight after I update it to work with Beta 16+ :P

I'd say significantly more, maybe 25-50? 10 is not that many with properly sized table rows.

50 sounds good. Shouldn't be heavy on the API or anything, and shows enough data to be worthwhile.

If there's something we can remove from this PR and move into a different one, we should do so because it's already quite large.

Good idea -- let's not add more features here, just review-based changes and fixes :)

[davwheat]

I think this is ready for review, minus the issue I identified above.

About to rebase onto master, though!

[tankerkiller125]

I forced the checks to re-run as they failed during the PHP selection part originally. It seems that their passing now.

[askvortsov1]

I forced the checks to re-run as they failed during the PHP selection part originally. It seems that their passing now.

Unfortunately they aren't

[KyrneDev]

I don't think it's his fault. Tests on master are failing as well.

https://github.com/flarum/core/pull/2626/checks?check_run_id=2015700780

PHP issue?

[askvortsov1]

Unless a user is deleted (that would throw off pages), I think it might make sense to cache page datas so if someone is switching back and forth, we might not need to reload it.

Also, we might want to support sorting users by username.

I've also been thinking more about extraction, and I feel like having an "AbstractModelTable" would be very powerful: most of the code here (other than the columns, but those would be in each subclass) is applicable to any model.

[Ralkage]

Awesome work on this addition!

A little tidbit regarding the core.admin.nav.userlist_button translation; could we perhaps rename the translation value to just "Users" instead of "Users list"? Currently, the admin nav buttons have single-word names, and I believe "Users" would fit right in place with the other nav items in that aspect. The same should apply to the page header/title. It can also cut down on new translations by simply reusing core.ref.users.

We can probably keep the route name as /userlist or if /users is an option, that'd be awesome to have instead!

[davwheat]

@Ralkage

Implemented your suggestions. I think it fits in to the nav a bit better now! :)

Do you think we should keep internal references as user_list to match with the existing View user list permission, or just swap all of the internal references (from this PR) to users as well?

[Ralkage]

Do you think we should keep internal references as user_list to match with the existing View user list permission, or just swap all of the internal references (from this PR) to users as well?

That's a good question 😅 both are good options, but I would seek input from the other core devs on which would be more practical. I like users as it would also match the current theme of the Admin area like how it is for the rest of the Admin nav buttons and their associated routes.

Permissions button -> admin#/permissions
Appearance button -> admin#/appearance
Basics button -> admin#/basics
and Users button -> admin#/users

[askvortsov1]

with the existing View user list permission

This permission badly needs to be renamed to search users (or something like that). I would lean towards renaming things to users.

[Ralkage]

Could we have the profile link icon/button left-aligned like the rest of the rows in the table? I also believe we can simplify it further by allowing the usernames that are returned to be clickable and take said Admin to the user's profile that way.

Now for the "Edit User" column, I think renaming it to "Actions" would leave room for additional operations later down the road such as adding a "Delete" button. We could probably take it one step further to match the front end and simply name it "Controls".

I don't want us to add too much to this PR as @askvortsov1 already noted as it is quite huge areadly, but this is more food for thought if anything; let me know how y'all feel about these suggestions if anything ;)

[Ralkage]

There are so many ways to approach the way the table is presented, such as only adding icons for actions if we wanted to keep it minimal (bless thee mobile users who have to scroll 🙏)

For example [click me pls]

@KyrneDev did this with ReFlar's User Management extension if I'm not mistaken.

[askvortsov1]

Also,

• Let's drop the placeholder value for emails, it's already blurred. Placeholder makes things messy because if the actual value is longer, the whole table shifts.
• Can we just unblur when clicking on the email? Then we can drop the button entirely.

[davwheat]

Can we just unblur when clicking on the email?

We discussed this, and I think we should keep the button. I can still make it unblur on click, if you'd like though.

[SychO9]

So I've been thinking about this, and I think we should be using a table instead. While I do like grid it doesn't make much sense to use it here since we want a straightforward table, and it would be more semantically correct to use that, right now we're just trying to recreate a table with grid and the fact that the HTML code has little to no separation between the key elements of a tabular layout which is can also end up limiting customization ability.

We could leave it as is for now and change it when we tackle the Table component abstraction.

[davwheat]

So I've been thinking about this, and I think we should be using a table instead.

I completely agree.

I brought this up when discussing it with Sasha on Discord and I suggested to leave it until we refactor the table into a separate component. For now this is only on the Admin page, so it's not the biggest deal, but we should look into it.

When subgrid comes, I propose we switch to that with table roles instead, but we need to use a table for now.

[SychO9]

I brought this up when discussing it with Sasha on Discord and I suggested to leave it until we refactor the table into a separate component. For now this is only on the Admin page, so it's not the biggest deal, but we should look into it.

Oh I must've missed that, agreed, I'll create an issue for it then and we can look into it sometime after stable.

When subgrid comes, I propose we switch to that with table roles instead, but we need to use a table for now.

Yeah, that'll probably be a while, we can look into it when it's available for use.

[davwheat]

ITS MERGE TIME! 🔥🔥