Scalable UX for user management #13158
Conversation
…lects, creating dropdown directives for dropdown w/ submenu using materializecss, change bulk actions to be dropdown button.
…onents/directives.
…nation in case the numbers are large.
…access change logic.
|
|
||
| $idSites = null; | ||
| if (!Piwik::hasUserSuperUserAccess()) { | ||
| $idSites = $this->access->getSitesIdWithAdminAccess(); |
There was a problem hiding this comment.
same here be good to also make sure idSites is not empty to prevent giving access to sites the user isn't allowed.
| { | ||
| $formatter = new Formatter(); | ||
|
|
||
| $lastSeenTimes = LastSeenTimeLogger::getLastSeenTimesForAllUsers(); |
There was a problem hiding this comment.
Off topic: at some point be good to move this into the user table instead of option...
plugins/UsersManager/API.php
Outdated
| * | ||
| * @param string $userLogin the user login. | ||
| * @param string|string[] $userLogin the user login(s). |
There was a problem hiding this comment.
Could we keep the API simple by using a bulk API request and request deleteUser for each userLogin?
| @@ -722,10 +889,12 @@ public function deleteUser($userLogin) | |||
| Piwik::checkUserHasSuperUserAccess(); | |||
There was a problem hiding this comment.
@mattab after letting admin users add user to a site, I would say we also need to let them delete users if they have access to only the same sites (or less) as the admin user? Otherwise the admin only has the option to remove access to any site, but never be able to delete the user...
There was a problem hiding this comment.
Sounds good indeed, could you make this improvement @diosmosis ?
There was a problem hiding this comment.
If we show a delete icon next to some users and not next to others I think it would be confusing. Ok to add a modal like the "add existing user" modal that uses a username/email?
plugins/UsersManager/API.php
Outdated
| @@ -802,22 +992,31 @@ public function getUserLoginFromUserEmail($userEmail) | |||
| * If access = 'noaccess' the current access (if any) will be deleted. | |||
| * If access = 'view' or 'admin' the current access level is deleted and updated with the new value. | |||
| * | |||
| * @param string $userLogin The user login | |||
| * @param string|string[] $userLogin The user login(s) | |||
There was a problem hiding this comment.
Same here can we use Bulk API requests instead of allowing multiple logins? Complexity is getting quite high and chances of (security) bugs high.
plugins/UsersManager/API.php
Outdated
| private function isUserHasAdminAccessTo($idSite) | ||
| { | ||
| try { | ||
| $this->access->checkUserHasAdminAccess([$idSite]); |
There was a problem hiding this comment.
isn't there a method for this in Piwik class or is this for the possibility to inject access? Cause when injecting FakeAccess it would also work through the Piwik class.
There was a problem hiding this comment.
There was already a $access member variable, so just using that. I can change it to the static method.
While reviewing #13158 I noticed the third parameter for `in_array` to force string checking should not be set here I suppose as idSites might be either strings or integers.
mattab
added
Major
|
@diosmosis Nicely executed! looking forward to hearing people's feedback. so many improvements packed in this PR 🚀 🚀 fyi: noticed some "fake" test failures in https://travis-ci.org/matomo-org/matomo/jobs/408768773 maybe you want to fix the build but I'll likely merge it for inclusion in the next beta 2 today |
diosmosis
force-pushed
the
6187-user-management
branch
from
1392f51
to
4083fb5
Compare
|
@diosmosis could you please make build green & merge this PR? |
While reviewing matomo-org#13158 I noticed the third parameter for `in_array` to force string checking should not be set here I suppose as idSites might be either strings or integers.
* Create empty components. * Mock up users list pagination. * Finish initial version of mockup. * Tweak to UI * More UI changes to new users manager screen. * More UI changes * Mock up user permission edits. * More tweaks to user permission editing (on both edit form & in users table). * add options * Another iteration on the UsersManager UI. * Update UsersManager UI again. * Implementing parts of the UI, fixing issue w/ overlapping material selects, creating dropdown directives for dropdown w/ submenu using materializecss, change bulk actions to be dropdown button. * Merge menu/submenu directives. * More superuser UI only functionality. * Fill out more logic of users manager UI + merging extra unneeded components/directives. * More users manager UI only changes. * Incomplete API method for new users list page. * Fill in server side pagination logic w/ tests & generally get to work in UI. * Make sure selects w/ placeholders can be unset. * Add loading state to users list + fix pagination issues + resize pagination in case the numbers are large. * Add last seen time to getUsersPlusAccessLevel() so it displays in UI. * Add permission edit pagination AJAX query + server side code. * Add "add access" button to user permission component. * Change permissions column to role + remove superuser checkbox & merge w/ Role column. * Delete user + bulk delete functionality. * Get delete users to work when entire search is selected. * Ask for confirmation before setting access in users list & implement access change logic. * Get bulk access functionality on users list to work (w/ tests). * Fix a bug in user table filtering + get permissions edit search to work. * Complete logic for permissions edit. * Change add user workflow so we do not have to save each permission edit in memory before saving whole user. * Add/edit user functionality. * Toggle superuser access functionality + some modal fixes. * in users list display ajax loading notification so counter is not changed visibly before rows are loaded. * initial review changes, disable functionality when viewing user is not superuser and some UI tweaks. * Redo top controls for user permission edit and add slide up toast notification for when a site is added. * Display warning in user permission edit if user has no access at all. * Do not reload users after going back from user edit form. * Force giving a new user access to a site when creating a user and make sure user list reloads if a user is modified, but does not realod if no user is modified. * Add form help to the non-straightforward fields. * Remove old usersmanager code & fix pagination bug. * Add help icon explaining roles to users list + permission edit. * Allow admin users to create other users + fix some regressions when making page-users-list not reload every time. * Apply self review changes. * Do not allow editing user details when an admin user edits a user. * Starting on UI tests. * Limit users displayed in page list to those that already have access to sites the current user is an admin of. * Refactor bulk/single AJAX calls & redraw component boundaries (users manager component owns user search state, paged users list owns table/control state). * Get add existing user modal to work. * write most UI tests + modify fixture * Fill out rest of UI test suite & get the rest to pass. * fix couple regressions * Get UI tests to pass and start on translation. * adding translations * try to fix some tests * Fixing API tests. * Fixing UsersManager tests. * Fix UI tests. * Add capabilities to new API output. * remove non-existant file references. * Add Write role to dropdowns. * Select from proper join. * tweak test * Updating UI tests. * Change styling of user permissions edit. * Update screenshots * Apply some PR feedback. * apply some review feedback * more review changes * update file headers * remove some TODOs * fix some tests * some more review fixes * update test files * Fix failing tests.
Fixes #6187
Initial PR for revamping the users management page to better handle thousands of users & sites.
Code Changes
New API:
ignoreExistingparam to UsersManager. setUserAccess which will add access for a site, but ignore it if it's already been added (for adding site access in user permission edit).AngularJS changes:
piwik-dropdown-menu<select>form fields to have placeholders & disabled options (disabled option hasdisabled: trueproperty)Outstanding Todo