Skip to content

[full-ci] Add per-user storage view#40657

Closed
pako81 wants to merge 8 commits intorelease-10.12.0from
per_user_storage
Closed

[full-ci] Add per-user storage view#40657
pako81 wants to merge 8 commits intorelease-10.12.0from
per_user_storage

Conversation

@pako81
Copy link
Copy Markdown

@pako81 pako81 commented Feb 27, 2023

Description

Add per-user storage view

Related Issue

Motivation and Context

Make possible to visualise used storage on per-user basis

How Has This Been Tested?

  • Access ~/settings/users URL. A new column Storage Used will display the amount of used storage for every user. This option can be also opted in/out under Settings in order to align with all other available users' options.

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

@pako81 pako81 added this to the development milestone Feb 27, 2023
@pako81 pako81 self-assigned this Feb 27, 2023
@jnweiger jnweiger changed the base branch from master to release-10.12.0 February 27, 2023 12:40
@jnweiger
Copy link
Copy Markdown
Contributor

Changed target to release-10.12

@pako81 pako81 mentioned this pull request Feb 27, 2023
11 tasks
Pasquale Tripodi added 2 commits February 27, 2023 22:40
@ownclouders
Copy link
Copy Markdown
Contributor

ownclouders commented Feb 27, 2023

💥 Acceptance tests pipeline webUISettingsMenu-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/38027/133

@jnweiger jnweiger changed the title Add per-user storage view [full ci] Add per-user storage view Feb 27, 2023
@jnweiger jnweiger changed the base branch from release-10.12.0 to master February 27, 2023 22:55
@jnweiger jnweiger changed the base branch from master to release-10.12.0 February 27, 2023 22:56
Comment thread changelog/unreleased/40657 Outdated
@owncloud owncloud deleted a comment from update-docs Bot Feb 28, 2023
@phil-davis
Copy link
Copy Markdown
Contributor

$ make test-php-unit NOCOVERAGE=true TEST_PHP_SUITE=tests/Settings/Controller/UsersControllerTest.php 
PHPUNIT="/home/phil/git/owncloud/core/lib/composer/phpunit/phpunit/phpunit" build/autotest.sh sqlite tests/Settings/Controller/UsersControllerTest.php
Using PHP executable /usr/bin/php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Updated 0 paths from the index
Installing ....
creating sqlite db
ownCloud was successfully installed
Testing with sqlite ...
No coverage
/home/phil/git/owncloud/core/lib/composer/phpunit/phpunit/phpunit --configuration phpunit-autotest.xml --log-junit autotest-results-sqlite.xml ../tests/Settings/Controller/UsersControllerTest.php 
PHPUnit 9.6.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.33
Configuration: phpunit-autotest.xml

EEEE.EEEE..........EEEEEE.............................E.......... 65 / 73 ( 89%)
.......E                                                          73 / 73 (100%)

Time: 00:00.403, Memory: 32.00 MB

There were 16 errors:

1) Tests\Settings\Controller\UsersControllerTest::testIndexAdmin
Error: Call to a member function getFileInfo() on null

/home/phil/git/owncloud/core/lib/private/Files/Filesystem.php:1030
/home/phil/git/owncloud/core/lib/private/legacy/helper.php:582
/home/phil/git/owncloud/core/settings/Controller/UsersController.php:233
/home/phil/git/owncloud/core/settings/Controller/UsersController.php:339
/home/phil/git/owncloud/core/tests/Settings/Controller/UsersControllerTest.php:354

Unit tests are failing in CI and locally for me. Is the code different from what was in the previous PR #40506 ?

@pako81
Copy link
Copy Markdown
Author

pako81 commented Feb 28, 2023

The code should be exactly the same and it proves to work locally for me.

@phil-davis
Copy link
Copy Markdown
Contributor

The code should be exactly the same and it proves to work locally for me.

"somebody" needs to investigate why those unit tests are failing, and fix them.

@jnweiger jnweiger changed the title [full ci] Add per-user storage view [full-ci] Add per-user storage view Mar 1, 2023
Comment thread changelog/unreleased/40657 Outdated
@prsnbrg
Copy link
Copy Markdown
Contributor

prsnbrg commented Mar 1, 2023

From my point of view, the used space is always given in MB. This is not user friendly in my view for over 100 GB.

A possible solution can be \OC_Helper::humanFileSize()

@pako81
Copy link
Copy Markdown
Author

pako81 commented Mar 1, 2023

From my point of view, the used space is always given in MB. This is not user friendly in my view for over 100 GB.

Thanks for the pointer - indeed it seems better to use humanFileSize(). Changed it accordingly.

@prsnbrg
Copy link
Copy Markdown
Contributor

prsnbrg commented Mar 1, 2023

In my opinion the tests fails because in the test an integer is declared as correct value for storageUsed, but the return value of the system is a string like "0 MB" or the null value of humanFileSize (e.g. "0 B").

Additional hint: the value for 0 Bytes of humanFileSize and the value for errors should be the same.

@pako81
Copy link
Copy Markdown
Author

pako81 commented Mar 2, 2023

I have adjusted the string for not-computed storage to 0 B. This is consistent with the case where user has no files at all --> in the personal settings page is currently reported You are using 0 B

Additionally, I have increased the log level to debug otherwise an entry at level 3 would be logged for every user who has not logged in yet.

@pako81
Copy link
Copy Markdown
Author

pako81 commented Mar 2, 2023

@jvillafanez do you see something in the settings/Controller/UsersController.php code which could explain the 16 failing tests?

1) Tests\Settings\Controller\UsersControllerTest::testIndexAdmin
Error: Call to a member function getFileInfo() on null

/home/phil/git/owncloud/core/lib/private/Files/Filesystem.php:1030
/home/phil/git/owncloud/core/lib/private/legacy/helper.php:582
/home/phil/git/owncloud/core/settings/Controller/UsersController.php:233
/home/phil/git/owncloud/core/settings/Controller/UsersController.php:339
/home/phil/git/owncloud/core/tests/Settings/Controller/UsersControllerTest.php:354

@jvillafanez
Copy link
Copy Markdown
Member

The filesystem is likely mocked or not initialized in the tests.
I guess the problem is that the \OC_Helper::getStorageInfo('/'); call can't be mocked. We might need a way to mock that call somehow so it doesn't go all the way through.

@mrow4a
Copy link
Copy Markdown
Contributor

mrow4a commented Mar 10, 2023

@pako81 isnt it better to use Metrics App for this?

cd /var/www/owncloud/apps/metrics
make build-js
make vendor
composer install

occ a:e metrics
occ config:system:set "metrics_shared_secret" --value 1234

Screenshot 2023-03-10 at 15 46 46

@mrow4a
Copy link
Copy Markdown
Contributor

mrow4a commented Mar 10, 2023

@pako81 take decision and close if so or let me know and I make tests work.

@jnweiger
Copy link
Copy Markdown
Contributor

jnweiger commented Mar 14, 2023

@mrow4a is right, this information is more logically found in the metrics app.
But as we have quota in the users listing, and each user sees his own storage usage + his quota, I would not mind to give the admin an overview already in the users listing. There better accessible (more likely in admin's focus), than with metrics. The column is optional, so it does not take away screen space for those who don't want to have it.

@pako81 pako81 closed this Jan 29, 2024
@pako81 pako81 deleted the per_user_storage branch January 29, 2024 09:46
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.

7 participants