Skip to content

read all relevant user attributes on login and user search, in one qu… - #18469

Merged
DeepDiver1975 merged 3 commits into
masterfrom
ldap-batch-read-attrs-ng
Sep 28, 2015
Merged

read all relevant user attributes on login and user search, in one qu…#18469
DeepDiver1975 merged 3 commits into
masterfrom
ldap-batch-read-attrs-ng

Conversation

@blizzz

@blizzz blizzz commented Aug 20, 2015

Copy link
Copy Markdown
Contributor

…ery. saves us some.

obsolotes #12943

fixes #17362

Basically it does what #12943 did, but also reads the user attributes when users are being fetched, for instance in share dialog (except: avatar image).

  • TODO: check performance impact (expectation: all in all better: saves LDAP requests, might cause more DB traffic)

Please review @MorrisJobke @Xenopathic @esakol @tkaufm or others

@blizzz blizzz added this to the 8.2-current milestone Aug 20, 2015
@blizzz blizzz mentioned this pull request Aug 20, 2015
Closed
1 task
This was referenced Aug 20, 2015
Closed
Closed
@blizzz

blizzz commented Aug 25, 2015

Copy link
Copy Markdown
Contributor Author

@rperezb could you get us some testing?

@tkaufm

tkaufm commented Aug 25, 2015

Copy link
Copy Markdown

@blizzz
on my test-system it seems to work in first glance (quick and dirty).
Please be aware that I didn't had time by now for intensive tests, also regarding the performance, I will try to get back as soon i got some time.

Happy to help
Tobias

@MorrisJobke

MorrisJobke commented Aug 26, 2015

Copy link
Copy Markdown
Contributor

@blizzz This contains a merge conflict due to #18042 Can you have a look?

@blizzz

blizzz commented Aug 26, 2015

Copy link
Copy Markdown
Contributor Author

@MorrisJobke yes, already on my list, gonna rebase it in a few.

@blizzz
blizzz force-pushed the ldap-batch-read-attrs-ng branch from db72a1e to 33b94cb Compare August 26, 2015 13:34
@scrutinizer-notifier

scrutinizer-notifier commented Aug 26, 2015

Copy link
Copy Markdown

A new inspection was created.

@blizzz

blizzz commented Aug 26, 2015

Copy link
Copy Markdown
Contributor Author

rebased, an adjustedment to changed codebase was necessary.

@tkaufm if you could confirm again with the latest changes, this would be awesome :)

MorrisJobke
MorrisJobke reviewed Aug 27, 2015
View reviewed changes
Comment thread apps/user_ldap/lib/user/user.php Outdated

@MorrisJobke MorrisJobke Aug 27, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is fired even if my home folder naming rule is an empty string.

@MorrisJobke MorrisJobke Aug 27, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Previously this was inside the if( strpos($this->access->connection->homeFolderNamingRule, 'attr:') === 0 && $this->access->connection->homeFolderNamingRule !== 'attr:')

@rperezb rperezb added the QA label Sep 9, 2015
@PVince81

PVince81 commented Sep 18, 2015

Copy link
Copy Markdown
Contributor

@blizzz any update ? (after your vacation)

@MorrisJobke

MorrisJobke commented Sep 23, 2015

Copy link
Copy Markdown
Contributor

@blizzz Ping :)

@blizzz

blizzz commented Sep 23, 2015

Copy link
Copy Markdown
Contributor Author

lol, added another commit (locally) this second to fix your finding

@blizzz
blizzz force-pushed the ldap-batch-read-attrs-ng branch from 33b94cb to 002b6bf Compare September 23, 2015 15:27
@blizzz

blizzz commented Sep 23, 2015

Copy link
Copy Markdown
Contributor Author

@MorrisJobke fixed, includes tests. Please try again, sänk ju.

@MorrisJobke

MorrisJobke commented Sep 24, 2015

Copy link
Copy Markdown
Contributor

Tested and works with existing users 👍

@MorrisJobke

MorrisJobke commented Sep 24, 2015

Copy link
Copy Markdown
Contributor

@rperezb @davitol Can we get this in a QA cycle? Thanks :)

@blizzz

blizzz commented Sep 25, 2015

Copy link
Copy Markdown
Contributor Author

Tested positively by @davitol #17362 (comment)

@blizzz

blizzz commented Sep 25, 2015

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 merge?

DeepDiver1975 added a commit that referenced this pull request Sep 28, 2015
@DeepDiver1975
Merge pull request #18469 from owncloud/ldap-batch-read-attrs-ng
0479cef 
read all relevant user attributes on login and user search, in one qu…
@DeepDiver1975
DeepDiver1975 merged commit 0479cef into master Sep 28, 2015
@DeepDiver1975
DeepDiver1975 deleted the ldap-batch-read-attrs-ng branch September 28, 2015 09:44
@MorrisJobke

MorrisJobke commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

@blizzz Can you create the backport request? This should go into 8.1.4 ;)

@blizzz

blizzz commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

Will do.

blizzz added a commit that referenced this pull request Sep 28, 2015
@blizzz
Backport #18469 (read all relevant user attributes on login and user …
368b97e 
…search, in one query)

read all relevant user attributes on login and user search, in one query. saves us some.

Conflicts:
	apps/user_ldap/user_ldap.php

adjust to nested group fix

do not throw exception when no attribute is specified
blizzz added a commit that referenced this pull request Sep 28, 2015
@blizzz
stable8.1 related adjustments to #18469 backport
3a12252
@blizzz blizzz mentioned this pull request Sep 28, 2015
Closed
@blizzz blizzz mentioned this pull request Oct 7, 2015
Merged
5 tasks
@PVince81 PVince81 mentioned this pull request Oct 15, 2015
Closed
@PVince81

PVince81 commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

Regression: #19816

blizzz added a commit that referenced this pull request Oct 22, 2015
@blizzz @MorrisJobke
Backport #18469 (read all relevant user attributes on login and user …
c1953f9 
…search, in one query)

read all relevant user attributes on login and user search, in one query. saves us some.

Conflicts:
	apps/user_ldap/user_ldap.php

adjust to nested group fix

do not throw exception when no attribute is specified
blizzz added a commit that referenced this pull request Oct 22, 2015
@blizzz @MorrisJobke
stable8.1 related adjustments to #18469 backport
0a99049
@blizzz blizzz mentioned this pull request Oct 27, 2015
Merged
@blizzz blizzz mentioned this pull request Nov 5, 2015
Closed
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

3 - To Review blue-ticket enhancement

Projects

None yet

Milestone

8.2

Development

Successfully merging this pull request may close these issues.

sharing notification by email not working for never logged on users

8 participants

@blizzz @tkaufm @MorrisJobke @scrutinizer-notifier @PVince81 @DeepDiver1975 @rperezb @davitol