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.

Sign up for GitHub

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

Jump to bottom

Ajaxify user list for files external #8507

Merged
merged 7 commits into from
Aug 28, 2014
Merged

Ajaxify user list for files external #8507

merged 7 commits into from
Aug 28, 2014

Conversation

butonic
Copy link
Member

@butonic butonic commented May 8, 2014

replaces chosen with select2, fixes #7499

@DeepDiver1975 @blizzz @PVince81 please review

@DeepDiver1975
Copy link
Member

tested 👍

@DeepDiver1975 DeepDiver1975 mentioned this pull request May 15, 2014
Merged
PVince81
PVince81 reviewed May 15, 2014
View reviewed changes
apps/files_external/settings.php
OCP\Util::addStyle('files_external', '../3rdparty/select2/select2');

//OCP\Util::addscript('3rdparty', 'chosen/chosen.jquery.min');
//OCP\Util::addStyle('3rdparty', 'chosen/chosen');
Copy link
Contributor

Choose a reason for hiding this comment

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

Remoeve the commented out code ?

@DeepDiver1975 DeepDiver1975 mentioned this pull request May 15, 2014
Closed
@PVince81
Copy link
Contributor

I just tested this with 200 users and 2 groups.
I randomly assigned the 2 groups to a few users.
The result set is messy, it's a mix between groups and users, the groups block reappears on later pages:
Page 1:
extstorage_page1
Page 2:
extstorage_page2

@PVince81
Copy link
Contributor

Page 3: (see the next block with Groups):
extstorage_page3

@PVince81
Copy link
Contributor

@owncloud/designers the list might need a few styling tweaks (could be done after the merge)

@PVince81
Copy link
Contributor

A few more issues:

  • The blur event from the select2 field should trigger the "addMountPoint" call to make it save, it doesn't seem so. I have to edit any other field then click outside to trigger saving.
  • When reading the config, it will call "userlist.php" for every previously selected user with a filter. It might work ok when there aren't that many selected users (which is probably the regular case), but not sure whether the filtered LDAP query would still run fast even when the DB has lots of users (@blizzz please check this)
  • Fix for the messy order mentioned in my previous comments (note: this was with local users, not LDAP)

blizzz
blizzz reviewed Jun 5, 2014
View reviewed changes
settings/ajax/userlist.php
@@ -42,6 +42,11 @@
} else {
$pattern = '';
}
if (isset($_GET['filter']) && !empty($_GET['filter'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use pattern instead of filter in the ajax call, avoids the duplication here

@blizzz
Copy link
Contributor

blizzz commented Jun 5, 2014

Yet not exactly where it is triggered, but when opening admin page still every user is pulled.

@blizzz
Copy link
Contributor

blizzz commented Jun 12, 2014

  • adding a new config always has a (the first?) user pre-selected

@blizzz
Copy link
Contributor

blizzz commented Jun 12, 2014

  • single groups sometimes appear in a block with other groups
  • with a second row, the user/group box is covered by something, and it does not work (as in does not start a search)

@butonic butonic added this to the ownCloud 7 CE milestone Jun 23, 2014
@blizzz
Copy link
Contributor

blizzz commented Jun 26, 2014

Atm, I don't know how to avoid the repeated and chaotic display of "Users" and "Groups" sections, select2 does not give any possibilities on this, at least it did not find it yet. Maybe when i spent more time on this … another issue would be that results could be inserted on the top while you scrolled down. So even if this screams to be ugly, it is for now probably the better method in fact. With a bigger set of users I expect the admin to do a search over users instead of scrolling down endlessly.

@jancborchardt what do you think?

@blizzz
Copy link
Contributor

blizzz commented Jun 26, 2014

Having in mind what was said above, it probably makes most sense to first read users, then groups… this is what i switched to now.

  • don't repeat sections/optgroups

@craigpg craigpg modified the milestones: ownCloud 7.0.1, ownCloud 7 CE Jun 26, 2014
@butonic butonic force-pushed the ajaxify_user_list_for_files_external branch from 7442110 to 6bbf8ad Compare August 20, 2014 13:19
@butonic
Copy link
Member Author

butonic commented Aug 20, 2014

@PVince81 @blizzz @DeepDiver1975 I rebased and simplified the select dropdown. Would be awesome if this would make it into 7.0.2, as it fixes #7499.

@jancborchardt Users are shown with their avatar, groups use the contacts icon.

@karlitschek @craigpg I tried to do this the old way so backporting should be easy. stable7? stable6?

@butonic
Copy link
Member Author

butonic commented Aug 22, 2014

Oh, and this needs #10591 to actually work ...

@LukasReschke
Copy link
Member

@butonic Does 63d7c00 looks better to you? :-)

@butonic
Copy link
Member Author

butonic commented Aug 24, 2014

@LukasReschke much better!

@butonic
Copy link
Member Author

butonic commented Aug 25, 2014

@blizzz displayname is no longer 'searched'. Can I have your 👍 ?

@butonic butonic force-pushed the ajaxify_user_list_for_files_external branch from 63d7c00 to f33312f Compare August 25, 2014 09:50
@scrutinizer-notifier
Copy link

A new inspection was created.

@butonic
Copy link
Member Author

butonic commented Aug 25, 2014

rebased because #10591 was merged

@ghost
Copy link

ghost commented Aug 25, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6850/

@PVince81 PVince81 mentioned this pull request Aug 25, 2014
Closed
@blizzz
Copy link
Contributor

blizzz commented Aug 28, 2014

now this one looks great! Thank you @butonic 👍

blizzz added a commit that referenced this pull request Aug 28, 2014
@blizzz
Merge pull request #8507 from owncloud/ajaxify_user_list_for_files_ex…
6fa6093 
…ternal

Ajaxify user list for files external
@blizzz blizzz merged commit 6fa6093 into master Aug 28, 2014
@blizzz blizzz deleted the ajaxify_user_list_for_files_external branch August 28, 2014 11:39
@blizzz
Copy link
Contributor

blizzz commented Aug 28, 2014

What about backports? I think at least stable7 should profit. @karlitschek

@karlitschek
Copy link
Contributor

yes. backport is definitely needed.

@blizzz
Copy link
Contributor

blizzz commented Sep 5, 2014

For reference, Backport to stable7 was done here #10558
For reference, Backport to stable6 is yet open here #10576

@blizzz blizzz mentioned this pull request Sep 5, 2014
Merged
@PVince81 PVince81 mentioned this pull request Sep 5, 2014
Closed
@butonic butonic mentioned this pull request Feb 2, 2015
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@butonic butonic

Labels
4 - To release app:files_external blue-ticket design enhancement settings:admin technical debt
Projects
None yet
Milestone
2014-sprint-02
Development

Successfully merging this pull request may close these issues.

files_external loads all users on admin page, which opens extremely slow with large user base