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.

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

Fix initializing paged search under some circumstances #6453

Merged
merged 5 commits into from
Sep 15, 2017
Merged

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Sep 11, 2017

Fixes #5273 and #6388

Basically the issue was, that a paged search might not be properly initialized, when no limit was given, but a previous search had a limit set. This resulted in strange behaviour as described in #5273. While debugging this, I figured out, @eigood recently also saw the wrong spot in the code as described in #6388.

Added Paged Search Integration tests cover the case.

Please test and review @nextcloud/ldap @zfzfzf33 @eigood @bline

Backport is necessary.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Existing and new LDAP config works 👍

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Seems to still work on my ldap test instance :)

@MorrisJobke
Copy link
Member

Let me rebase to get the latest fixes on master.

@MorrisJobke
Copy link
Member

Rebased

@codecov
Copy link

codecov bot commented Sep 14, 2017

Codecov Report

Merging #6453 into master will decrease coverage by 2.13%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master   #6453      +/-   ##
===========================================
- Coverage     55.53%   53.4%   -2.14%     
- Complexity    21243   22521    +1278     
===========================================
  Files          1253    1407     +154     
  Lines         73203   87100   +13897     
  Branches          0    1329    +1329     
===========================================
+ Hits          40653   46513    +5860     
- Misses        32550   40587    +8037
Impacted Files Coverage Δ Complexity Δ
apps/user_ldap/lib/Group_LDAP.php 60.11% <0%> (ø) 160 <0> (ø) ⬇️
apps/user_ldap/lib/Access.php 18.2% <0%> (-0.1%) 304 <0> (+3)
apps/sharebymail/tests/SettingsTest.php 52.17% <0%> (-47.83%) 3% <0%> (ø)
lib/private/Security/RateLimiting/Limiter.php 55.55% <0%> (-44.45%) 5% <0%> (ø)
settings/Controller/EncryptionController.php 56% <0%> (-37.34%) 8% <0%> (ø)
settings/Controller/GroupsController.php 64.61% <0%> (-35.39%) 9% <0%> (ø)
...ps/comments/tests/Unit/AppInfo/ApplicationTest.php 69.56% <0%> (-30.44%) 4% <0%> (ø)
apps/user_ldap/lib/Configuration.php 41.5% <0%> (-29.46%) 86% <0%> (ø)
apps/encryption/lib/Command/EnableMasterKey.php 75% <0%> (-25%) 5% <0%> (ø)
lib/private/AvatarManager.php 61.9% <0%> (-24.77%) 4% <0%> (ø)
... and 344 more

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 14, 2017
@blizzz
Copy link
Member Author

blizzz commented Sep 14, 2017

backports for 12: #6502 and 11: #6504

@MorrisJobke
Copy link
Member

@blizzz
Copy link
Member Author

blizzz commented Sep 15, 2017

Waiting for CI: https://drone.nextcloud.com/nextcloud/server/1537

error during connect: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.26/networks/create: EOF

😭

@MorrisJobke
Copy link
Member

@blizzz Ignore that single occurrence for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: ldap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants