Users page lazy multiselect group dropdowns #25922

Merged
merged 3 commits into from Aug 24, 2016

Projects

None yet

7 participants

@PVince81
Collaborator
PVince81 commented Aug 23, 2016 edited

Instead of pre-rendering all multiselects with lots of group entries,
the current groups are now displayed as simple labels.
Behind the labels there is a pencil icon like for other fields.
When clicking the pencil icon, the dropdown will be spawned and will
open itself.
Upon closing of the dropdown, the label comes back with the updated
selection and the dropdown is destroyed.

The whole purpose of this is to solve the performance issue with hundreds of groups which would slow down the page to a crawl and even cause script timeout issues on Firefox.

  • BUG: the user creation dropdown is broken now

@mrow4a @cdamken please try it out

Would backport to 9.1 if it's a viable alternative to solve performance issues.

CC @butonic @DeepDiver1975

@PVince81 PVince81 added this to the 9.2 milestone Aug 23, 2016
@mention-bot

@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ringmaster, @DeepDiver1975 and @edozzo to be potential reviewers

@PVince81
Collaborator
PVince81 commented Aug 23, 2016 edited
  • looks like there's more code that reads the groups selection with $tr.find('.groupsselect').val() (undo behavior)
@PVince81
Collaborator
PVince81 commented Aug 23, 2016 edited
  • BUG: leaves stray empty spans
@mrow4a
Contributor
mrow4a commented Aug 23, 2016

@PVince81 Could you add checkbox, stating that we could check if the feature works at the current state?

@mrow4a
Contributor
mrow4a commented Aug 23, 2016

Ok, seems to work for me, I will check if that changes things in performance switching between branches

@PVince81
Collaborator

@mrow4a feel free to add your own test plan checkboxes

@PVince81
Collaborator

@mrow4a I just pushed a commit to fix the remaining issues.
I'll leave the performance testing up to you and will do my own quick regression testing.

Remaining issues:

  • "no group" looks ugly
  • user creation row "no group" looks ugly and drop down shifts the whole row
@PVince81
Collaborator
PVince81 commented Aug 23, 2016 edited
  • TEST: create user with no groups selected
  • TEST: create user with some groups selected => groups properly applied
  • TEST: change groups of user, refresh page => groups properly applied
  • TEST: change subadmin groups of user, refresh page => subadmin groups properly applied
  • TEST: "admin" group not present in subadmin dropdown as before
  • TEST: delete user, check groups sidebar, then undo: group count decreased then increased again (because the logic that reads the deleted user's group has changed)
  • TEST: creating new group from dropdown still works
  • TEST: in all browsers (due to simulated DOM events that might not always work properly)
    • Chromium
    • Firefox
    • Safari
    • IE11
    • IE10 (in case of backport)
    • IE9 (in case of backport)
@SergioBertolinSG
Member

This is too ugly, needs to be centered, have margins, start with upper case and perhaps a bigger font.

screen shot 2016-08-23 at 18 33 12

@SergioBertolinSG
Member

When clicking on the pencil some space appears over the boxes making them move down. (both safari & chrome)

@SergioBertolinSG
Member
SergioBertolinSG commented Aug 23, 2016 edited
  • BUG: When adding new groups, if you click out (closing the dropdown) and later try to remove groups of the list, you can't do that. Added groups doesn't appear.
@mrow4a
Contributor
mrow4a commented Aug 23, 2016 edited
  • BUG: when you select normal groups to edit, and then immedietely subadmin groups, the dropdown icon (instead of label) persists.
@mrow4a
Contributor
mrow4a commented Aug 23, 2016 edited

Great @PVince81 !! Awesome job, and just first iteration!

TEST:
500 users most in all groups
~20 groups
Firefox

Old version:

performance

Your branch:

performance-improved

Summary:

result

@PVince81
Collaborator

be centered

the hardest thing ever in CSS (vertical centering) ๐Ÿ˜‰

The reason I picked "no group" for the text is because it is already translated, having backports in mind.
But if we don't care about backports we could change the text to something better.

Regarding to shifting of dropdowns, I guess it might be possible to compensate using a negative margin.

@mrow4a
Contributor
mrow4a commented Aug 23, 2016 edited

@PVince81

Second test with LDAP

Users 50000
Groups 2000

OLD - performance test cannot be performed - buffer gets full after 1 minute. Scripts executes after 3 minutes (180s) of clicking "continue".

NEW - average for 5 measurements: 6.17s

@PVince81
Collaborator

Pfff... now struggling to try and align multiselect's dropdown button exactly with the same styles as the label text. I wonder if I shouldn't simply have hacked multiselect to accept a callback to lazy-load the dropdown contents...

@mrow4a
Contributor
mrow4a commented Aug 23, 2016 edited

Ok, I reloaded and it got reduced to 60s for LDAP with old solution. But anyways, it is huge difference and no script stop. Great job here! lets optimize it even more, but this needs more discussion.

@cdamken
Contributor
cdamken commented Aug 23, 2016

Really AWESOME!!

The reason I picked "no group" for the text is because it is already translated, having backports in mind.
But if we don't care about backports we could change the text to something better.

IMHO I think the backports only from 9.0 and up is needed

@mrow4a
Contributor
mrow4a commented Aug 23, 2016

Vince, when you are finished with polishing javascript and CSS, lets make more detailed performance tests in both cases with small number of groups and Enterprise Scale

@mrow4a
Contributor
mrow4a commented Aug 23, 2016

@PVince81 Do you do topX (e.g. top3) query to the database for the users in groups? I cannot spot it in the PR, and that might squeeze the performance even more.

@PVince81
Collaborator

@mrow4a that's something for another time... First things first...

@felixboehm
Contributor

you guys rock!

@butonic
Member
butonic commented Aug 23, 2016

omg this is so cool!

@PVince81
Collaborator

I'm deeply unhappy with the current solution because it requires a lot of CSS hacking.
The main problem is that we are trying to workaround jquery.multiselect's flaws by hacking around it.
The hack itself involves not initializing jquery.multiselect until the user clicks the edit button. And then when we do, the dropdown button does appear. Now to avoid jumpy UI behavior, we have to make the original text aligned/styled like the dropdown button so that the user only really sees a dropdown appear.
Furthermore the "no group" looks super-ugly at the moment and styling it to look properly could take quite some work or inventing new styles.

Instead of hacking around jquery.multiselect, I want to give a try at an alternative solution: beating the shit out of jquery.multiselect itself to make it eat an array of groups instead of DOM elements. This is technically possible. Then instead of hacking around with pencil icons we could just keep the current styles exactly as they are with the dropdown button, with the difference that the expensive stuff is only done when the dropdown opens. I'll submit a separate PR for this.

@PVince81
Collaborator

(also, with backporting in mind, ideally the PR should be as small as possible)

@PVince81
Collaborator

Ok... I dived into jquery.multiselect and it turns out it would require too many changes. That plugin just isn't designed for such a job.

Coming back to this PR: I'm now trying to make the "label" + "pencil" look like the dropdown button. This way from the UX perspective it will look exactly like before, and the unsuspecting user won't be aware of the horrors happening under the hood.

@PVince81 PVince81 Users page lazy multiselect group dropdowns
Instead of pre-rendering all multiselects with lots of group entries,
the current groups are now displayed as simple labels.
Behind the labels there is a pencil icon like for other fields.
When clicking the pencil icon, the dropdown will be spawned and will
open itself.
Upon closing of the dropdown, the label comes back with the updated
selection and the dropdown is destroyed.
69bb45e
@PVince81
Collaborator

Rebased and squashed.

The UX now looks exactly like before this PR, but under the hood the multiselect creation and destruction is happening.

Please review and test @mrow4a @SergioBertolinSG @DeepDiver1975 @butonic @felixboehm

@SergioBertolinSG
Member

Ok with the back of the dropdown, #25922 (comment) and #25922 (comment) are fixed.

But #25922 (comment) persist.

PVince81 added some commits Aug 24, 2016
@PVince81 PVince81 Extra non-available groups also in list dcddfc3
@PVince81 PVince81 Fix group sorting in user list group selection
7c619e1
@PVince81
Collaborator

@SergioBertolinSG fixed #25922 (comment) now.

The problem was that the dropdown was based only on existing groups. Needed to add logic to add pending groups too if one was selected.

@SergioBertolinSG
Member

Deselecting a new added group, closing the dropdown and opening it again makes new group to disappear from list. (not happening in master)

@PVince81
Collaborator

Deselecting a new added group, closing the dropdown and opening it again makes new group to disappear from list. (not happening in master)

I think this is acceptable because the group doesn't really exist at this point. If the admin deselected it here it is likely that it was a mistake or the admin didn't want to create this new group anyway.

@SergioBertolinSG
Member

Yes, but it is a change of behaviour not needed, just adds some annoyance by removing them from the list. If it is too complex to change, don't bother, keep it this way.

@PVince81
Collaborator

it is too complex to change, don't bother, keep it this way.

Yes, that's my worry...

The reason it worked before was because every list inside the page has a full list of all groups as select/option elements. In the case of the top element, when you created a new group it would simply append a new "option" element in the existing list.

This PR gets rid of the select/option DOM elements and recreates the selection list from scratch every time the user opens the dropdown. This is why the new group gets lost on deselection. To avoid losing it, we'd need to store it in a new location and also merge it into the list.

@mrow4a
Contributor
mrow4a commented Aug 24, 2016 edited

@PVince81 after 5 times clicking button "continue" with script being freezed I stoped. With this modification, you are again building some nasty DOM element like discussed previously:

523afd68-657e-11e6-8e9a-8fd62b01d080

You just reversed to the problem again.

@PVince81
Collaborator

@mrow4a this can't be, I didn't change the internal logic.

@PVince81
Collaborator

@mrow4a please delete the branch, git fetch, then check it out again. Since I force pushed / rebased, maybe you got mixed changes ?

I just tried on Firefox locally and the spinner only hangs one second like before.

@mrow4a
Contributor
mrow4a commented Aug 24, 2016

You reversed dropdowns instead of labels, and the problem with DOM building is back again.

perf1

@mrow4a mrow4a closed this Aug 24, 2016
@mrow4a mrow4a reopened this Aug 24, 2016
@mrow4a
Contributor
mrow4a commented Aug 24, 2016

Sorry, again problem with emptying the comment. OK. I think I just wrongly pulled your branch, I will verify it now

@mrow4a
Contributor
mrow4a commented Aug 24, 2016

OK, I verified it again! IT WORKS as previously.

@SergioBertolinSG
Member
SergioBertolinSG commented Aug 24, 2016 edited

@PVince81 OK. Works fine then ๐Ÿ‘
tested on
Safari: โœ…
Edge: โœ…

@mrow4a
Contributor
mrow4a commented Aug 24, 2016 edited

It seems you have even improved the performance :D

TEST:
50000 users
2000 groups

With test on MASTER, Firefox freezees and after 10 times clicking continue, and 60s it loads the page correctly

That is the result for now with your current solutions:

screenshot from 2016-08-24 11 11 00

@PVince81
Collaborator

Thanks. Now if someone could review the code and give a second thumbs up we can merge and backport this.

@mrow4a
Contributor
mrow4a commented Aug 24, 2016

When you create user and add him to the group, the groups you selected, after creation are still there being checked. Should it not refresh the label and stuff there?

@mrow4a
Contributor
mrow4a commented Aug 24, 2016

But apart from that issue in settings/templates/users/part.createuser.php,
image

@PVince81
Collaborator

@mrow4a no, it's by design, see master. Makes it possible to create several users in a row with the same groups.

@PVince81 PVince81 merged commit 259687e into master Aug 24, 2016

4 checks passed

Scrutinizer 9 new issues
Details
continuous-integration/jenkins/pr This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the users-lazydropdownwithpencil branch Aug 24, 2016
@PVince81
Collaborator

stable9.1: #25931
stable9: #25932

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment