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

Correctly calculate used space for quota with external storage #4357

Closed
wants to merge 3 commits into from
Closed

Correctly calculate used space for quota with external storage #4357

wants to merge 3 commits into from

Conversation

Ardinis
Copy link
Contributor

@Ardinis Ardinis commented Apr 16, 2017

issue #4348

@mention-bot
Copy link

@Ardinis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @MorrisJobke and @blizzz to be potential reviewers.

@MorrisJobke MorrisJobke requested a review from icewind1991 April 16, 2017 02:22
icewind1991
icewind1991 requested changes Apr 18, 2017
View reviewed changes
lib/private/legacy/helper.php Outdated
@@ -537,7 +537,7 @@ public static function getStorageInfo($path, $rootInfo = null) {
$includeExtStorage = \OC::$server->getSystemConfig()->getValue('quota_include_external_storage', false);

if (!$rootInfo) {
$rootInfo = \OC\Files\Filesystem::getFileInfo($path, false);
$rootInfo = \OC\Files\Filesystem::getFileInfo($path, $includeExtStorage);
Copy link
Member

Choose a reason for hiding this comment

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

This should be 'ext', when using true it also includes incoming shares

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure of what you are meaning here.
By using $includeExtStorage external storage are counted in quota calculation for the used part.

Copy link
Member

Choose a reason for hiding this comment

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

You can pass 'ext' as the seccond argument of getFileInfo to have it include external storages but not shares.

When passing true it includes both shares and external storages

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 19, 2017
@codecov
Copy link

codecov bot commented Apr 20, 2017 •
edited
Loading

Codecov Report

Merging #4357 into master will decrease coverage by 22.93%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #4357       +/-   ##
=============================================
- Coverage     54.08%   31.15%   -22.94%     
- Complexity    21589    21629       +40     
=============================================
  Files          1327     1328        +1     
  Lines         82303    82188      -115     
  Branches       1305     1311        +6     
=============================================
- Hits          44511    25602    -18909     
- Misses        37792    56586    +18794
Impacted Files Coverage Δ Complexity Δ
lib/private/legacy/helper.php 57.03% <0%> (-21.12%) 115 <0> (ø)
lib/private/DB/OCSqlitePlatform.php 0% <0%> (-100%) 5% <0%> (ø)
apps/files_sharing/lib/External/MountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
lib/public/Files/ForbiddenException.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/SystemTag/ManagerFactory.php 0% <0%> (-100%) 3% <0%> (ø)
apps/user_ldap/lib/Mapping/UserMapping.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Repair/NC11/FixMountStorages.php 0% <0%> (-100%) 5% <0%> (ø)
apps/provisioning_api/lib/AppInfo/Application.php 0% <0%> (-100%) 6% <0%> (ø)
apps/user_ldap/lib/Mapping/GroupMapping.php 0% <0%> (-100%) 1% <0%> (ø)
apps/files_trashbin/lib/AppInfo/Application.php 0% <0%> (-100%) 2% <0%> (ø)
... and 375 more

icewind1991
icewind1991 requested changes Apr 20, 2017
View reviewed changes
lib/private/legacy/helper.php Outdated
@@ -537,7 +537,7 @@ public static function getStorageInfo($path, $rootInfo = null) {
$includeExtStorage = \OC::$server->getSystemConfig()->getValue('quota_include_external_storage', false);

if (!$rootInfo) {
$rootInfo = \OC\Files\Filesystem::getFileInfo($path, false);
$rootInfo = \OC\Files\Filesystem::getFileInfo($path, 'ext');
Copy link
Member

Choose a reason for hiding this comment

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

It should still only apply external storages when the config is set.

$rootInfo = \OC\Files\Filesystem::getFileInfo($path, $includeExtStorage ? 'ext' : false);

@rullzer
Copy link
Member

rullzer commented Apr 28, 2017

@Ardinis could you address the comments of @icewind1991 ?

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 29, 2017
@MorrisJobke
Copy link
Member

Could you also have a look at the failing unit test:

1) Test\HelperStorageTest::testGetStorageInfoIncludingExtStorageWithNoUserQuota
Failed asserting that 22 matches expected 5.
/drone/src/github.com/nextcloud/server/tests/lib/HelperStorageTest.php:162

And could you squash all your commits into one, because 3 commits is a bit much for a 1 line change 😉

And if this commit then also would have a sign-off I would be more than happy :) https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 29, 2017
This was referenced May 11, 2017
Closed
Merged
@MorrisJobke
Copy link
Member

I fixed the unit tests: #4816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icewind1991 icewind1991 icewind1991 requested changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees
No one assigned
Labels
2. developing Work in progress feature: external storage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ardinis @mention-bot @rullzer @MorrisJobke @icewind1991