Improve code occ files_external:list --short #34033

mmattel · 2019-01-04T19:03:37Z

Description

This PR improves the code for the --short option in occ command files_external:list

removing unnecessary double code paths - make one code stream for:
standard output and json output
greatly improving the array handling by using if statemens instead looping thru arrays
better output of --all and if a user is given

Related Issue

Fixes show a short list of mount infos: files_external:list --short #32178 (comment)

Motivation and Context

Better code, easier to handle and enhance. Better output

How Has This Been Tested?

Screenshots (if appropriate):

Commands

sudo -uwww-data ./occ files_external:list -s 
+----------+-------------+------------------+-------------------+------+-------+
| Mount ID | Mount Point | Applicable Users | Applicable Groups | Auth | Type  |
+----------+-------------+------------------+-------------------+------+-------+
| 2        | /test       | user_x           | admin, testg      | User | Admin |
+----------+-------------+------------------+-------------------+------+-------+

sudo -uwww-data ./occ files_external:list -s user_y
+----------+---------------+------+----------+
| Mount ID | Mount Point   | Auth | Type     |
+----------+---------------+------+----------+
| 1        | /martin@filer | User | Personal |
+----------+---------------+------+----------+

sudo -uwww-data ./occ files_external:list -s -a
+----------+---------------+------------------+-------------------+------+----------+
| Mount ID | Mount Point   | Applicable Users | Applicable Groups | Auth | Type     |
+----------+---------------+------------------+-------------------+------+----------+
| 1        | /martin@filer | user_y           |                   | User | Personal |
| 2        | /test         | user_x           | admin, testg      | User | Admin    |
+----------+---------------+------------------+-------------------+------+----------+

Auth
This new field shows if a mount is based on User/Pwd (User) or Session (Session).
This info is important when using files:scan as session based mounts cant be scanned by occ.

The output of json is tested and correct to the plain one

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-01-05T08:18:54Z

Codecov Report

Merging #34033 into master will increase coverage by 1.28%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #34033      +/-   ##
============================================
+ Coverage     64.77%   66.05%   +1.28%     
- Complexity    18340    18503     +163     
============================================
  Files          1198     1152      -46     
  Lines         69418    66556    -2862     
  Branches       1276     1276              
============================================
- Hits          44964    43966     -998     
+ Misses        24085    22221    -1864     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`53.09% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67.62% <ø> (+1.5%)`	`18503 <ø> (+163)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/lib/config.php	`3.75% <0%> (-6.97%)`	`21% <0%> (+2%)`
apps/files_external/lib/Lib/Api.php
apps/files_external/lib/Lib/Backend/SFTP_Key.php
apps/files_external/ajax/applicable.php
apps/files_external/lib/Command/Backends.php
apps/files_external/lib/Lib/Backend/DAV.php
apps/files_external/lib/Lib/Backend/OwnCloud.php
apps/files_external/lib/Lib/Storage/OwnCloud.php
apps/files_external/lib/Command/Verify.php
apps/files_external/lib/Lib/Backend/SMB.php
... and 37 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 361472c...5babef9. Read the comment docs.

codecov · 2019-01-05T08:18:54Z

Codecov Report

Merging #34033 into master will decrease coverage by 0.01%.
The diff coverage is 77.64%.

@@             Coverage Diff              @@
##             master   #34033      +/-   ##
============================================
- Coverage     65.15%   65.14%   -0.02%     
  Complexity    18352    18352              
============================================
  Files          1200     1200              
  Lines         69698    69672      -26     
  Branches       1283     1283              
============================================
- Hits          45413    45388      -25     
+ Misses        23911    23910       -1     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	`53.1% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.53% <77.64%> (-0.02%)`	`18352 <0> (ø)`

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/lib/Command/ListCommand.php	`67.66% <77.64%> (-3.84%)`	`0 <0> (ø)`

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 2dedd3d...bccc328. Read the comment docs.

mmattel · 2019-01-05T09:00:24Z

@sharidas can you help fixing the tests ?

sharidas · 2019-01-09T08:26:20Z

I have made a change to my oC instance for example:

And in the user2 personal storage I added sftp:

And after making this setting in the UI, I try to run the command as shown below:
For user2:

For user1:

From the user perspective who executes this command, I would expect the no mounts configured for user1. Am I missing something here? @mmattel

mmattel · 2019-01-09T09:29:22Z

I agree with you - it is not clean, but it is a matter of the definition of --all which I did not change from the logic side. show both system wide mounts and all personal mounts

	protected function execute(InputInterface $input, OutputInterface $output) {
		if ($input->getOption('all')) {
			/** @var  $mounts IStorageConfig[] */
			$mounts = $this->globalService->getStorageForAllUsers();
			$userId = self::ALL;
		} else {
			$userId = $input->getArgument('user_id');
			$storageService = $this->getStorageService($userId);

			/** @var  $mounts IStorageConfig[] */
			$mounts = $storageService->getAllStorages();
		}

		$this->listMounts($userId, $mounts, $input, $output);
	}

From that what I read above and from the other code elements where ALL is queried, --all has precedence over a user

So if we want to combine --all and user we would need an additional filter logic - or?
Or just be more precise in the helptext of --all...

mmattel · 2019-01-16T18:59:28Z

Improved helptext

Arguments:
  user_id                User id to list the personal mounts for, if no user is provided admin mounts will be listed

Options:
      --show-password    Show passwords and secrets
      --full             Don't truncate long values in table output
  -a, --all              Show mounts for all users. All has precedence over user_id
  -s, --short            Show only a reduced mount info
      --output[=OUTPUT]  The output format to use (plain, json or json_pretty). [default: "plain"]

davitol · 2019-01-17T08:56:27Z

Tested and the behavior is like commented in #34033 (comment)

IMHO, behavior should be like commented in #34033 (comment) . From the user perspective who executes this command, I would expect the no mounts configured for user1 when using -a option. Anyway, now it is documented in the help with Show mounts for all users. All has precedence over user_id

PVince81 · 2019-01-22T10:21:45Z

Seems this implementation aligns with how occ files:scan --all user1 works, so let's keep it for consistency.

PVince81 · 2019-01-22T10:21:54Z

@ownclouders rebase

ownclouders · 2019-01-22T10:22:01Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

ownclouders · 2019-01-22T10:24:47Z

Automated rebase with GitMate.io was successful! 🎉

PVince81 · 2019-02-08T11:44:35Z

@mmattel please run make test-php-style-fix

mmattel · 2019-02-08T14:15:09Z

@PVince81 Question:
updating master, running make clean and make rebasing this PR and running make test-php-style-fix I get following error message:

composer bin owncloud-codestyle install --no-progress
                                 
  [InvalidArgumentException]     
  Command "bin" is not defined.  
                                 

Makefile:364: recipe for target 'vendor-bin/owncloud-codestyle/vendor' failed
make: *** [vendor-bin/owncloud-codestyle/vendor] Error 1

Any ideas ?

PVince81 · 2019-02-08T14:23:54Z

@phil-davis ^

phil-davis · 2019-02-11T11:18:01Z

What to say? I just did

git fetch origin
git checkout master
git pull
make clean
make
make test-php-style

and it works hard for a while loading loads of dependencies for make and then for make test-php-style and finally it runs the style tests:

Changed current directory to vendor-bin/php_codesniffer
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.4.0): Loading from cache
Writing lock file
Generating autoload files
php -d zend.enable_gc=0 vendor-bin/owncloud-codestyle/vendor/bin/php-cs-fixer fix -v --diff --diff-format udiff --allow-risky yes --dry-run
Loaded config ownCloud coding standard from "/home/phil/git/owncloud/core/.php_cs.dist".
Using cache file ".php_cs.cache".
...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
Legend: ?-unknown, I-invalid file syntax, file ignored, S-Skipped, .-no changes, F-fixed, E-error

Checked all files in 29.321 seconds, 28.000 MB memory used
vendor-bin/php_codesniffer/vendor/bin/phpcs --runtime-set ignore_warnings_on_exit --standard=phpcs.xml tests/acceptance tests/TestHelpers
............................................................  60 / 107 (56%)
...............................................              107 / 107 (100%)


Time: 7.6 secs; Memory: 26MB

php build/OCPSinceChecker.php
Parsing all files in lib/public for the presence of @since or @deprecated on each method...

in the make it should be getting:

  - Installing bamarni/composer-bin-plugin (v1.2.0): Loading from cache

that is the magic thing that makes the composer bin command exist.

mmattel · 2019-02-18T20:30:37Z

I have updated the PR, run php-style-fix, added tests, squashed the commits

PVince81

👍

PVince81 · 2019-02-19T08:28:04Z

@mmattel please backport

Improve code occ files_external:list --short

mmattel added enhancement 3 - To Review backport-request labels Jan 4, 2019

mmattel requested review from PVince81, sharidas and davitol January 4, 2019 19:03

mmattel force-pushed the improve_short branch from 938a39e to 5babef9 Compare January 5, 2019 08:18

mmattel force-pushed the improve_short branch from 5babef9 to d4089d7 Compare January 16, 2019 18:49

ownclouders force-pushed the improve_short branch from d4089d7 to f21d25f Compare January 22, 2019 10:23

PVince81 added this to the development milestone Jan 22, 2019

ownclouders mentioned this pull request Jan 22, 2019

occ files_external:list .Mounts from all users are listed even if we specify a user when using --all option #34221

Closed

mmattel force-pushed the improve_short branch from f21d25f to 233a19d Compare February 15, 2019 15:33

improve code

bccc328

mmattel force-pushed the improve_short branch from aa02acb to bccc328 Compare February 18, 2019 20:29

PVince81 approved these changes Feb 19, 2019

View reviewed changes

PVince81 merged commit c62fd07 into master Feb 19, 2019

PVince81 deleted the improve_short branch February 19, 2019 08:27

mmattel pushed a commit that referenced this pull request Feb 19, 2019

[stable10] Merge pull request #34033 from owncloud/improve_short

d4aafea

Improve code occ files_external:list --short

mmattel mentioned this pull request Feb 19, 2019

[stable10] Improve code occ files_external:list --short #34549

Merged

11 tasks

mmattel removed the backport-request label Feb 19, 2019

lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code occ files_external:list --short #34033

Improve code occ files_external:list --short #34033

mmattel commented Jan 4, 2019 •

edited

Loading

codecov bot commented Jan 5, 2019

codecov bot commented Jan 5, 2019 •

edited

Loading

mmattel commented Jan 5, 2019

sharidas commented Jan 9, 2019

mmattel commented Jan 9, 2019 •

edited

Loading

mmattel commented Jan 16, 2019 •

edited

Loading

davitol commented Jan 17, 2019

PVince81 commented Jan 22, 2019

PVince81 commented Jan 22, 2019

ownclouders commented Jan 22, 2019

ownclouders commented Jan 22, 2019

PVince81 commented Feb 8, 2019

mmattel commented Feb 8, 2019

PVince81 commented Feb 8, 2019

phil-davis commented Feb 11, 2019 •

edited

Loading

mmattel commented Feb 18, 2019

PVince81 left a comment

PVince81 commented Feb 19, 2019

Improve code occ files_external:list --short #34033

Improve code occ files_external:list --short #34033

Conversation

mmattel commented Jan 4, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

codecov bot commented Jan 5, 2019

Codecov Report

codecov bot commented Jan 5, 2019 • edited Loading

Codecov Report

mmattel commented Jan 5, 2019

sharidas commented Jan 9, 2019

mmattel commented Jan 9, 2019 • edited Loading

mmattel commented Jan 16, 2019 • edited Loading

davitol commented Jan 17, 2019

PVince81 commented Jan 22, 2019

PVince81 commented Jan 22, 2019

ownclouders commented Jan 22, 2019

ownclouders commented Jan 22, 2019

PVince81 commented Feb 8, 2019

mmattel commented Feb 8, 2019

PVince81 commented Feb 8, 2019

phil-davis commented Feb 11, 2019 • edited Loading

mmattel commented Feb 18, 2019

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Feb 19, 2019

mmattel commented Jan 4, 2019 •

edited

Loading

codecov bot commented Jan 5, 2019 •

edited

Loading

mmattel commented Jan 9, 2019 •

edited

Loading

mmattel commented Jan 16, 2019 •

edited

Loading

phil-davis commented Feb 11, 2019 •

edited

Loading