Allow admins to search for users by display name or email, per #1749 #2660

justin-sleep · 2016-12-13T22:21:08Z

Should resolve #1749. Tested with my 4 users. @hregnier, could you test this with your 30k users?

Signed-off-by: justin-sleep justin@quarterfull.com

mention-bot · 2016-12-13T22:21:10Z

@justin-sleep, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @schiessle and @MorrisJobke to be potential reviewers.

justin-sleep · 2016-12-14T07:12:05Z

Not sure why it's failing some tests on drone after I implemented the null array check. Seems to be git cloning errors unrelated to my changes.

hregnier · 2016-12-14T08:36:36Z

@justin-sleep thanks for your PR! It works on my 30k+ users, no lags or whatever.
By the way, as the comments says, partial strings for mail (and maybe display name?) would be great.
Thanks again!

justin-sleep · 2016-12-14T08:39:59Z

@hregnier no problem! Partial strings for display name should actually already work, I've just tested it on my own server. I'll see if I can figure out something for email, though.

hregnier · 2016-12-14T08:46:14Z

@justin-sleep About display name, I didn't express myself well : For exemple I have a user who's DisplayName is "Doe John". I can't find him by typing "John Doe". But maybe that is a bug for the searchDisplayName function and needed another issue report?

justin-sleep · 2016-12-14T09:01:23Z

Hmm, I see what you mean; that would certainly add more versatility to the search. Off the top of my head, a potential solution would be to split the display name into an array of strings ("sub-patterns") using space as a delimiter, than calling searchDisplayName on each sub-pattern. I'll take a look at it.

justin-sleep · 2016-12-14T16:51:13Z

@hregnier the latest commit I pushed should allow the more flexible display name matching you described.

codecov-io · 2016-12-14T17:14:10Z

Codecov Report

Merging #2660 into master will increase coverage by -24.05%.

@@             Coverage Diff             @@
##           master    #2660       +/-   ##
===========================================
- Coverage   57.23%   33.18%   -24.05%     
===========================================
  Files        1146     1197       +51     
  Lines       65733    69522     +3789     
  Branches        0     1253     +1253     
===========================================
- Hits        37620    23070    -14550     
- Misses      28113    46452    +18339

Impacted Files	Coverage Δ
settings/Controller/UsersController.php	`0% <ø> (-64.78%)`	❌
apps/files_trashbin/lib/AppInfo/Application.php	`0% <ø> (-100%)`	❌
lib/private/DB/SQLiteSessionInit.php	`0% <ø> (-100%)`	❌
apps/user_ldap/appinfo/routes.php	`0% <ø> (-100%)`	❌
...rivate/Authentication/Token/DefaultTokenMapper.php	`0% <ø> (-100%)`	❌
apps/files_sharing/lib/AppInfo/Application.php	`0% <ø> (-100%)`	❌
apps/dav/lib/Connector/Sabre/Server.php	`0% <ø> (-100%)`	❌
lib/private/Files/Cache/MoveFromCacheTrait.php	`0% <ø> (-100%)`	❌
lib/private/Files/Mount/LocalHomeMountProvider.php	`0% <ø> (-100%)`	❌
apps/dav/lib/CalDAV/PublicCalendarRoot.php	`0% <ø> (-100%)`	❌
... and 640 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1b8064...885cefa. Read the comment docs.

Signed-off-by: justin-sleep <justin@quarterfull.com>

MorrisJobke · 2016-12-16T22:16:55Z

settings/Controller/UsersController.php

+                //if the pattern to search for isn't potentially complex, just call searchDisplayName with it
+                if (count($subPatterns) == 1)
+                    $arrays[1] = $this->userManager->searchDisplayName($pattern, $limit, $offset);
+                else {


Please always use brackets for if blocks.

Signed-off-by: justin-sleep <justin@quarterfull.com>

MorrisJobke · 2016-12-16T22:27:33Z

settings/Controller/UsersController.php

-				$batch = $this->userManager->search($pattern, $limit, $offset);
+
+
+                $batch = array();


The indentation for this whole block seems to be wrong. Could you please check that you use tabs to indent? Thanks

MorrisJobke · 2016-12-16T22:27:55Z

settings/Controller/UsersController.php

+                if (count($subPatterns) == 1) {
+                    $arrays[1] = $this->userManager->searchDisplayName($pattern, $limit, $offset);
+                }
+                else {


Please use } else { on a single line. Sorry for this nitpicking

MorrisJobke · 2016-12-16T22:28:51Z

settings/Controller/UsersController.php

+
+
+                $batch = array();
+                $arrays = array();


Please don't use such generic names.

Yeah, I had been meaning to change this one. Coding late at night means limited creativity 💤

MorrisJobke · 2016-12-16T22:29:27Z

settings/Controller/UsersController.php

+                $arrays = array();
+
+                //split display name into individual words for flexible matching
+                $arrays[0] = $this->userManager->search($pattern, $limit, $offset);


use searchResult or something more descriptive.

MorrisJobke · 2016-12-16T22:29:58Z

settings/Controller/UsersController.php

+
+                //if the pattern to search for isn't potentially complex, just call searchDisplayName with it
+                if (count($subPatterns) == 1) {
+                    $arrays[1] = $this->userManager->searchDisplayName($pattern, $limit, $offset);


Please use displaynameSearchResult or something more descriptive here instead of $array[1]

MorrisJobke · 2016-12-16T22:30:37Z

settings/Controller/UsersController.php

+                }
+
+                // TODO: allow partial strings for email matches
+                $arrays[2] = $this->userManager->getByEmail($pattern);


use emailSearchResult or something more descriptive here

MorrisJobke · 2016-12-16T22:31:17Z

settings/Controller/UsersController.php

+
+                // TODO: allow partial strings for email matches
+                $arrays[2] = $this->userManager->getByEmail($pattern);
+                foreach ($arrays as $arr) {


Either build here the array of arrays or simply call array_merge three times. It's more crucial to have readable code instead of clever or smart code.

Signed-off-by: justin-sleep <justin@quarterfull.com>

hregnier · 2016-12-20T10:12:55Z

thanks @justin-sleep it seems to works! Some bug appear (If i search "John " only one john on the dozens appear, but typing another letter or trim the space refresh the search) but it's not so disturbing compare to the improvement :)

justin-sleep · 2016-12-20T20:17:35Z

@hregnier I'm having trouble replicating what you're describing. This is what I'm doing to try to run into your bug:

Create multiple users named John (e.g. display name is John Doe, John Goodman, etc.) with random usernames
Type "John " into the search bar

But for me, this returns a list that contains every user whose display name contains John. Could you clarify the properties of the users that aren't getting returned?

hregnier · 2016-12-21T08:07:01Z

That's a problem. I can't get you clearer information because i can't find any pattern of the bug.

Another example : I have a user who username is "Dupont Jean". I can't find him when I search "dupont jean" but find him with "jean dupont" ! But I do not have the same problem with every user...

I understand you can't reproduce the bug. I can't show you a real set of users, although it should be usefull.

MorrisJobke

Works here 👍

MorrisJobke · 2017-01-26T05:14:48Z

@blizzz Could you give this a try?

cc @nickvergessen @rullzer @schiessle @icewind1991

nickvergessen · 2017-01-26T09:19:35Z

settings/Controller/UsersController.php

+
+				// TODO: allow partial strings for email matches
+				$emailSearchResult = $this->userManager->getByEmail($pattern);
+				$searchResults = array($uidSearchResult, $displayNameSearchResult, $emailSearchResult);


They all are arrays only, so you can simply do:

$batch = array_merge($uidSearchResult, $displayNameSearchResult, $emailSearchResult);

Signed-off-by: justin-sleep <justin@quarterfull.com>

blizzz

I am not sure I like this approach. Basically it forces this search behavior over all user backends. Especially in case of LDAP it would fire 2 searches (plus the local email lookup, unnecessarily) for "Joann Doe" while the LDAP backend supports this all already, in just one go. I'd prefer to have this implemented on the specific backend.

ChristophWurst · 2017-02-28T09:51:10Z

@justin-sleep @nickvergessen @blizzz what's the status here? How can we proceed with this?

justin-sleep · 2017-03-02T22:41:31Z

@ChristophWurst I haven't proceeded with the changes @blizzz requested. I haven't looked into the codebase enough to know where the individual backends are located to implement the changes there rather than in UsersController, so if someone could point me to the backends I could give it a look.

blizzz · 2017-03-08T09:08:19Z

@justin-sleep starting point for local backend: https://github.com/nextcloud/server/blob/master/lib/private/User/Database.php

LDAP backend allows this already, so it does not need to be touched.

I have no overview about other backends.

blizzz · 2017-11-13T15:44:04Z

moving to 14 due to freeze. @justin-sleep are you still interested in finishing it?

MorrisJobke · 2017-11-14T16:47:17Z

The search for displayname is fixed with #7169

MorrisJobke · 2017-11-14T22:42:03Z

There is only the search by email missing. And as it needs to be done in a similar place like #7169 I will close this and have opened a issue for this remaining thing in #7175.

justin-sleep added 3 commits December 14, 2016 11:21

Allow admins to search for users by display name or email, per #1749

06afa53

Signed-off-by: justin-sleep <justin@quarterfull.com>

Account for null arrays when merging user search results

a325162

Signed-off-by: justin-sleep <justin@quarterfull.com>

Allow more flexible display name matching in user search

71d191a

Signed-off-by: justin-sleep <justin@quarterfull.com>

rullzer added this to the Nextcloud 12.0 milestone Dec 16, 2016

rullzer added 3. to review Waiting for reviews enhancement labels Dec 16, 2016

MorrisJobke requested changes Dec 16, 2016

View reviewed changes

Add brackets to if block

19c8180

Signed-off-by: justin-sleep <justin@quarterfull.com>

MorrisJobke requested changes Dec 16, 2016

View reviewed changes

justin-sleep added 2 commits December 16, 2016 20:57

Fix formatting, use more descriptive names / improve readability

fd91c36

Signed-off-by: justin-sleep <justin@quarterfull.com>

Fix typo

fed7964

Signed-off-by: justin-sleep <justin@quarterfull.com>

MorrisJobke approved these changes Jan 26, 2017

View reviewed changes

nickvergessen requested changes Jan 26, 2017

View reviewed changes

Remove unnecessary array validity check

885cefa

Signed-off-by: justin-sleep <justin@quarterfull.com>

blizzz requested changes Jan 30, 2017

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 8, 2017

MorrisJobke removed this from the Nextcloud 12.0 milestone May 9, 2017

rullzer added this to the Nextcloud 13 milestone May 23, 2017

blizzz modified the milestones: Nextcloud 13, Nextcloud 14 Nov 13, 2017

MorrisJobke mentioned this pull request Nov 14, 2017

Fix search in user managent when no group is selected #7169

Merged

MorrisJobke self-assigned this Nov 14, 2017

MorrisJobke closed this Nov 14, 2017

MorrisJobke removed this from the Nextcloud 14 milestone Nov 14, 2017

MorrisJobke removed their assignment Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow admins to search for users by display name or email, per #1749 #2660

Allow admins to search for users by display name or email, per #1749 #2660

justin-sleep commented Dec 13, 2016 •

edited

Loading

mention-bot commented Dec 13, 2016

justin-sleep commented Dec 14, 2016

hregnier commented Dec 14, 2016

justin-sleep commented Dec 14, 2016

hregnier commented Dec 14, 2016

justin-sleep commented Dec 14, 2016 •

edited

Loading

justin-sleep commented Dec 14, 2016

codecov-io commented Dec 14, 2016 •

edited

Loading

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

justin-sleep Dec 17, 2016

MorrisJobke Dec 18, 2016

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

MorrisJobke Dec 16, 2016

hregnier commented Dec 20, 2016

justin-sleep commented Dec 20, 2016

hregnier commented Dec 21, 2016

MorrisJobke left a comment

MorrisJobke commented Jan 26, 2017

nickvergessen Jan 26, 2017

justin-sleep Jan 27, 2017

blizzz left a comment

ChristophWurst commented Feb 28, 2017

justin-sleep commented Mar 2, 2017

blizzz commented Mar 8, 2017

blizzz commented Nov 13, 2017

MorrisJobke commented Nov 14, 2017

MorrisJobke commented Nov 14, 2017

		$batch = $this->userManager->search($pattern, $limit, $offset);


		$batch = array();

Allow admins to search for users by display name or email, per #1749 #2660

Allow admins to search for users by display name or email, per #1749 #2660

Conversation

justin-sleep commented Dec 13, 2016 • edited Loading

mention-bot commented Dec 13, 2016

justin-sleep commented Dec 14, 2016

hregnier commented Dec 14, 2016

justin-sleep commented Dec 14, 2016

hregnier commented Dec 14, 2016

justin-sleep commented Dec 14, 2016 • edited Loading

justin-sleep commented Dec 14, 2016

codecov-io commented Dec 14, 2016 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hregnier commented Dec 20, 2016

justin-sleep commented Dec 20, 2016

hregnier commented Dec 21, 2016

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz left a comment

Choose a reason for hiding this comment

ChristophWurst commented Feb 28, 2017

justin-sleep commented Mar 2, 2017

blizzz commented Mar 8, 2017

blizzz commented Nov 13, 2017

MorrisJobke commented Nov 14, 2017

MorrisJobke commented Nov 14, 2017

justin-sleep commented Dec 13, 2016 •

edited

Loading

justin-sleep commented Dec 14, 2016 •

edited

Loading

codecov-io commented Dec 14, 2016 •

edited

Loading