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

Limit sync2 #36563

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Limit sync2 #36563

wants to merge 15 commits into from

Conversation

jvillafanez
Copy link
Member

Description

Include a sync limiter and disable LDAP users automatically if the number of users is above the limit
Cherry-picked and updated from #35105

Related Issue

https://github.com/owncloud/enterprise/issues/3245

Motivation and Context

How Has This Been Tested?

Requires testing with the latest ownCloud

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

TODO:

  • Update "since" tag
  • Check migrations (might need to change the filename)

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #36563 into master will decrease coverage by 0.11%.
The diff coverage is 47.49%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36563      +/-   ##
============================================
- Coverage     64.68%   64.57%   -0.12%     
+ Complexity    19118    19112       -6     
============================================
  Files          1269     1275       +6     
  Lines         74775    74858      +83     
  Branches       1320     1317       -3     
============================================
- Hits          48369    48340      -29     
- Misses        26018    26131     +113     
+ Partials        388      387       -1
Flag Coverage Δ Complexity Δ
#javascript 53.88% <13.04%> (-0.24%) 0 <0> (ø)
#phpunit 65.75% <49.71%> (-0.11%) 19112 <62> (-6)
Impacted Files Coverage Δ Complexity Δ
lib/private/User/Account.php 76% <ø> (ø) 10 <0> (ø) ⬇️
core/routes.php 48.71% <ø> (ø) 0 <0> (ø) ⬇️
core/templates/sync/htmlmail.hardlimit.php 0% <0%> (ø) 0 <0> (?)
core/templates/sync/htmlmail.softlimit.php 0% <0%> (ø) 0 <0> (?)
core/register_command.php 0% <0%> (ø) 0 <0> (ø) ⬇️
core/Migrations/Version20191211175735.php 0% <0%> (ø) 5 <5> (?)
core/Migrations/Version20170221114437.php 0% <0%> (ø) 2 <1> (+1) ⬆️
lib/private/Server.php 85.14% <100%> (+0.07%) 128 <0> (ø) ⬇️
lib/private/User/SyncLimiter.php 100% <100%> (ø) 8 <8> (?)
core/js/js.js 55.05% <100%> (ø) 0 <0> (ø) ⬇️
... and 49 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 d197d60...990190c. Read the comment docs.

@jvillafanez
Copy link
Member Author

I don't think I can rise coverage easily.
AccountMapper changes would require to modify the DB data, but restoring the previous state won't be clean taking into account how the tests are planned at the moment.
SyncBackend requires mocking 2-level dependencies (or more) due to the SyncService being created and not injected. In addition, the code is quite complex around the notifications, so we should consider to move that piece out to a different class.

@jvillafanez
Copy link
Member Author

/settings/users/<uid>/enabled endpoint allows enabling the users over the limit. This is a bug that should be fixed. The limit will be restored in the next login / sync, but it's very likely that a different user is disabled to force the limit.

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

Successfully merging this pull request may close these issues.

None yet

1 participant