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

Providing --path option to transfer-ownership #27343

Merged
merged 2 commits into from Mar 17, 2017

Conversation

Projects
None yet
4 participants
@sharidas
Member

sharidas commented Mar 9, 2017

This will help user to selectively move the folders
specified using --path option, instead of moving
entire folder under files directory.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

The change has been made for occ command's transfer-ownership. An additional --path argument has been added to support. This will help user to pass particular folder/file to move to destination user/target. So I have introduced an optional argument --path. I have deliberately made it optional, as its user choice to move entire folder under files or selectively.

Related Issue

This issue which this patch addresses is owncloud/platform#91

Motivation and Context

This helps the user to selectively transfer the ownership of the files/folders instead of moving entire folder under files.

How Has This Been Tested?

Below is the way I have tested:

The folder files/1/1a/1aa/ has 1aaa, 1aab and 1aac folders
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# sudo -u www-data ./occ files:transfer-ownership --path "test1/files/1/1a/1aa" test1 test2
Analysing files of test1 ...
0 [>---------------------------]
Collecting all share information for files and folder of test1 ...
0 [>---------------------------]
Transferring files to test2/files/transferred from test1 on 20170309_101206 ...
Restoring shares ...
0 [>---------------------------]
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

after the execution I was able to see in test2 user:

root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# ls -lt data/test2/files/transferred\ from\ test1\ on\ 20170309_101206/
total 12
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aac
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aab
drwxr-xr-x 2 www-data www-data 4096 Mar 9 15:42 1aaa
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

and in test1 user:

root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud# ls data/test1/files/1/1a/
1ab
root@silburyadmin-Inspiron-5567:/home/sujith/test/owncloud#

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 9, 2017

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @chagrawil to be potential reviewers.

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @chagrawil to be potential reviewers.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
'path',
'',
InputArgument::OPTIONAL,
'selectively provide the path to copy. For example --path "alice/files/Music"'

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

s/copy/transfer/ (it's a move, not a copy)

@PVince81

PVince81 Mar 9, 2017

Member

s/copy/transfer/ (it's a move, not a copy)

This comment has been minimized.

@sharidas

sharidas Mar 9, 2017

Member

Replaced copy with transfer in the new update.

@sharidas

sharidas Mar 9, 2017

Member

Replaced copy with transfer in the new update.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
$view->mkdir("$this->sourceUser/files");
$sourcePath = (strlen($this->inputPath) > 0) ? $this->inputPath : "$this->sourceUser/files";
$view->rename($sourcePath, $this->finalTarget);
if (!is_dir($this->sourceUser/files)) {

This comment has been minimized.

@PVince81

PVince81 Mar 9, 2017

Member

missing double quotes on string ?

@PVince81

PVince81 Mar 9, 2017

Member

missing double quotes on string ?

This comment has been minimized.

@sharidas

sharidas Mar 9, 2017

Member

Fixed the double quotes on string in the new update.

@sharidas

sharidas Mar 9, 2017

Member

Fixed the double quotes on string in the new update.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 10, 2017

Member

The updated version of the patch is ready for review. @PVince81 Can you please review this change.

Member

sharidas commented Mar 10, 2017

The updated version of the patch is ready for review. @PVince81 Can you please review this change.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 10, 2017

Member
  • BUG: specifying a non-existing path on the CLI must fail instead of attempting transfer

  • BUG: some problem with transferring shares, steps:

  1. Create three users "user1", "user2" and "user3"
  2. From "user3" share a folder "foruser1" with "user1"
  3. Login as "user1"
  4. Create a folder "transferthis"
  5. Move "foruser1" to "transferthis"
  6. Create a folder "transferthis/foruser2/x"
  7. Share folder "transferthis/foruser2" with "user2" AND "user3"
  8. Create a folder "transferthis/normal/y"
  9. Log out
± % occ files:transfer-ownership user1 user2 --path=transferthis
Analysing files of user1 ...
    0 [>---------------------------]
Collecting all share information for files and folder of user1 ...
    2 [============================]
Transferring files to user2/files/transferred from user1 on 20170310_021750 ...
Restoring shares ...
 1/2 [==============>-------------]  50%Share with id 3 points at deleted file, skipping
 2/2 [============================] 100%
  1. Login as "user1" and "user2" to check.

Expected result: folder "transferthis" is no more in "user1"'s account and is present in "user2's" account.
Actual result: nothing was transferred.

Please have a look.

Member

PVince81 commented Mar 10, 2017

  • BUG: specifying a non-existing path on the CLI must fail instead of attempting transfer

  • BUG: some problem with transferring shares, steps:

  1. Create three users "user1", "user2" and "user3"
  2. From "user3" share a folder "foruser1" with "user1"
  3. Login as "user1"
  4. Create a folder "transferthis"
  5. Move "foruser1" to "transferthis"
  6. Create a folder "transferthis/foruser2/x"
  7. Share folder "transferthis/foruser2" with "user2" AND "user3"
  8. Create a folder "transferthis/normal/y"
  9. Log out
± % occ files:transfer-ownership user1 user2 --path=transferthis
Analysing files of user1 ...
    0 [>---------------------------]
Collecting all share information for files and folder of user1 ...
    2 [============================]
Transferring files to user2/files/transferred from user1 on 20170310_021750 ...
Restoring shares ...
 1/2 [==============>-------------]  50%Share with id 3 points at deleted file, skipping
 2/2 [============================] 100%
  1. Login as "user1" and "user2" to check.

Expected result: folder "transferthis" is no more in "user1"'s account and is present in "user2's" account.
Actual result: nothing was transferred.

Please have a look.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 10, 2017

Member

you might need to debug into $view->rename to find out what happens and whether it fails somewhere there

Member

PVince81 commented Mar 10, 2017

you might need to debug into $view->rename to find out what happens and whether it fails somewhere there

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 10, 2017

Member

@PVince81 Thanks for the feedback. I will have a look at it.

Member

sharidas commented Mar 10, 2017

@PVince81 Thanks for the feedback. I will have a look at it.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 10, 2017

Member

Best would be to write an integration test right away because setting this up for testing is time consuming as you need to start from scratch after a transfer.

Running the test when debugging would help save this time.

Member

PVince81 commented Mar 10, 2017

Best would be to write an integration test right away because setting this up for testing is time consuming as you need to start from scratch after a transfer.

Running the test when debugging would help save this time.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 13, 2017

Member

@sharidas it seems you have found a bug deep in the FS code.

In the old code, the move operation would do: rename('test1/files', 'test2/files/transferred ...') which moves "files" to the target and renames it to "transferred ...". So far so good, this is legitimate.

But now with the new logic the rename is: rename('test1/files/a', 'test2/files/transferred .../a').
However the folder "transferred ..." doesn't exist yet.

Still, it seems the View happily moves the folder to this non-existing target folder instead of failing... resulting in the inconsistency you saw. The filecache will contain "a" but with a parent -1 because the parent did not exist / have an id.

@sharidas for you the fix is to call $view->mkdir() for the target folder first.

The View but will need to be fixed separately, I'll make a ticket for it.

Member

PVince81 commented Mar 13, 2017

@sharidas it seems you have found a bug deep in the FS code.

In the old code, the move operation would do: rename('test1/files', 'test2/files/transferred ...') which moves "files" to the target and renames it to "transferred ...". So far so good, this is legitimate.

But now with the new logic the rename is: rename('test1/files/a', 'test2/files/transferred .../a').
However the folder "transferred ..." doesn't exist yet.

Still, it seems the View happily moves the folder to this non-existing target folder instead of failing... resulting in the inconsistency you saw. The filecache will contain "a" but with a parent -1 because the parent did not exist / have an id.

@sharidas for you the fix is to call $view->mkdir() for the target folder first.

The View but will need to be fixed separately, I'll make a ticket for it.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 13, 2017

Member

Raised View issue here: #27376

Member

PVince81 commented Mar 13, 2017

Raised View issue here: #27376

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 13, 2017

Member

@PVince81 I have updated the PR by adding $view->mkdir at https://github.com/owncloud/core/pull/27343/files#diff-0674cc8bda871e32d63705af16fa8144R258. I have verified that share folders and other folders which are created by user1 do get transferred.

Member

sharidas commented Mar 13, 2017

@PVince81 I have updated the PR by adding $view->mkdir at https://github.com/owncloud/core/pull/27343/files#diff-0674cc8bda871e32d63705af16fa8144R258. I have verified that share folders and other folders which are created by user1 do get transferred.

Show outdated Hide outdated tests/integration/features/transfer-ownership.feature
Given user "user0" exists
And user "user1" exists
And User "user0" created a folder "/test"
When transfering ownership path from path "test" "user0" to "user1"

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

This sentence is not clear.
Something like this seems better:

When transfering ownership of path "test" from "user0" to "user1"

Also I recommend to put the starting slash before any paths as it is the decided choice to name paths in scenarios.

@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

This sentence is not clear.
Something like this seems better:

When transfering ownership of path "test" from "user0" to "user1"

Also I recommend to put the starting slash before any paths as it is the decided choice to name paths in scenarios.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 14, 2017

Member

@SergioBertolinSG I have updated the integration tests. Kindly review the same.

Member

sharidas commented Mar 14, 2017

@SergioBertolinSG I have updated the integration tests. Kindly review the same.

Show outdated Hide outdated tests/integration/features/bootstrap/CommandLineContext.php
* @When /^transfering ownership path from path "([^"]+)" "([^"]+)" to "([^"]+)"/
*/
public function transferingOwnershipPath($path, $user1, $user2) {
$path = '--path "' . $path . '"';

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

The occ command is expecting a --path=$PATH. So this is failing because the lack of '='.

Also the occ command example is wrong because it is not adding the '='.

Usage:
  files:transfer-ownership [options] [--] <source-user> <destination-user>

Arguments:
  source-user           owner of files which shall be moved
  destination-user      user who will be the new owner of the files

Options:
      --path=PATH       selectively provide the path to transfer. For example --path "alice/files/Music"
@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

The occ command is expecting a --path=$PATH. So this is failing because the lack of '='.

Also the occ command example is wrong because it is not adding the '='.

Usage:
  files:transfer-ownership [options] [--] <source-user> <destination-user>

Arguments:
  source-user           owner of files which shall be moved
  destination-user      user who will be the new owner of the files

Options:
      --path=PATH       selectively provide the path to transfer. For example --path "alice/files/Music"
Show outdated Hide outdated tests/integration/features/transfer-ownership.feature
And user "user1" exists
And User "user0" created a folder "/test"
When transfering ownership of path "test" from "user0" to "user1"
And the command was successful

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

Missing Then

@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

Missing Then

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Mar 14, 2017

Member

Tests passed locally.

But this explanation in the help of the command is wrong:

Options:
--path=PATH selectively provide the path to transfer. For example --path "alice/files/Music"

Which this changes It has to be:
--path PATH

Or change it to use '=' with the command.

Member

SergioBertolinSG commented Mar 14, 2017

Tests passed locally.

But this explanation in the help of the command is wrong:

Options:
--path=PATH selectively provide the path to transfer. For example --path "alice/files/Music"

Which this changes It has to be:
--path PATH

Or change it to use '=' with the command.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 14, 2017

Member

@PVince81 @SergioBertolinSG I have made the changes. Kindly review them.

Member

sharidas commented Mar 14, 2017

@PVince81 @SergioBertolinSG I have made the changes. Kindly review them.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 14, 2017

Member

Seems to have trouble with PHP 5.6 ?

± % sudo -u wwwrun ./occ
PHP Fatal error:  Class 'OCA\Files\Command\InputOption' not found in /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php on line 96
PHP Stack trace:
PHP   1. {main}() /srv/www/htdocs/owncloud/occ:0
PHP   2. require_once() /srv/www/htdocs/owncloud/occ:11
PHP   3. OC\Console\Application->loadCommands() /srv/www/htdocs/owncloud/console.php:98
PHP   4. OC\Console\Application->loadCommandsFromInfoXml() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:108
PHP   5. OC\ServerContainer->query() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:166
PHP   6. OC\AppFramework\Utility\SimpleContainer->query() /srv/www/htdocs/owncloud/lib/private/ServerContainer.php:80
PHP   7. OC\AppFramework\Utility\SimpleContainer->resolve() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:113
PHP   8. OC\AppFramework\Utility\SimpleContainer->buildClass() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:92
PHP   9. ReflectionClass->newInstanceArgs() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  10. OCA\Files\Command\TransferOwnership->__construct() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  11. Symfony\Component\Console\Command\Command->__construct() /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php:76
PHP  12. OCA\Files\Command\TransferOwnership->configure() /srv/www/htdocs/owncloud/lib/composer/symfony/console/Command/Command.php:63

I did run 'make cleanandmake` to make sure it wasn't a deps issue.

Member

PVince81 commented Mar 14, 2017

Seems to have trouble with PHP 5.6 ?

± % sudo -u wwwrun ./occ
PHP Fatal error:  Class 'OCA\Files\Command\InputOption' not found in /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php on line 96
PHP Stack trace:
PHP   1. {main}() /srv/www/htdocs/owncloud/occ:0
PHP   2. require_once() /srv/www/htdocs/owncloud/occ:11
PHP   3. OC\Console\Application->loadCommands() /srv/www/htdocs/owncloud/console.php:98
PHP   4. OC\Console\Application->loadCommandsFromInfoXml() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:108
PHP   5. OC\ServerContainer->query() /srv/www/htdocs/owncloud/lib/private/Console/Application.php:166
PHP   6. OC\AppFramework\Utility\SimpleContainer->query() /srv/www/htdocs/owncloud/lib/private/ServerContainer.php:80
PHP   7. OC\AppFramework\Utility\SimpleContainer->resolve() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:113
PHP   8. OC\AppFramework\Utility\SimpleContainer->buildClass() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:92
PHP   9. ReflectionClass->newInstanceArgs() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  10. OCA\Files\Command\TransferOwnership->__construct() /srv/www/htdocs/owncloud/lib/private/AppFramework/Utility/SimpleContainer.php:75
PHP  11. Symfony\Component\Console\Command\Command->__construct() /srv/www/htdocs/owncloud/apps/files/lib/Command/TransferOwnership.php:76
PHP  12. OCA\Files\Command\TransferOwnership->configure() /srv/www/htdocs/owncloud/lib/composer/symfony/console/Command/Command.php:63

I did run 'make cleanandmake` to make sure it wasn't a deps issue.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
'path',
null,
InputOption::VALUE_OPTIONAL,
'selectively provide the path to transfer. For example --path "alice/files/Music"'

This comment has been minimized.

@PVince81

PVince81 Mar 14, 2017

Member

the example is wrong, it should not contain the "$userId/files/" piece

@PVince81

PVince81 Mar 14, 2017

Member

the example is wrong, it should not contain the "$userId/files/" piece

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
$objectsDir = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path));
$dirFound = false;
foreach ($objectsDir as $name => $object) {
if (strpos($name, $this->inputPath)) {

This comment has been minimized.

@PVince81

PVince81 Mar 14, 2017

Member

is this safe ? what if there are two directories within the same path, like "somedir/test/somedir" ?

Can you add a PHP comment to explain why you are using strpos here ?

Would also be good to add an explicit condition like >= 0

@PVince81

PVince81 Mar 14, 2017

Member

is this safe ? what if there are two directories within the same path, like "somedir/test/somedir" ?

Can you add a PHP comment to explain why you are using strpos here ?

Would also be good to add an explicit condition like >= 0

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
if (strlen($this->inputPath) >= 1) {
$unknownDir = $this->inputPath;
$this->inputPath = $this->sourceUser . "/files/" . $this->inputPath;
$path = realpath(getcwd());

This comment has been minimized.

@PVince81

PVince81 Mar 14, 2017

Member

Argh, no. Don't do anything with the direct FS. There are some storages that do not store anything in the "data" folder but store in object store.

Please always use the $view methods for checking path existence. $view->is_dir($this->inputPath).

In general in OC never ever access the FS directly, always use a $view or the files public API.

@PVince81

PVince81 Mar 14, 2017

Member

Argh, no. Don't do anything with the direct FS. There are some storages that do not store anything in the "data" folder but store in object store.

Please always use the $view methods for checking path existence. $view->is_dir($this->inputPath).

In general in OC never ever access the FS directly, always use a $view or the files public API.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 15, 2017

Member

@PVince81 Kindly review my updated PR. I have removed ugly changes which directly access FS. Used $view->is_dir in the updated PR. I have also modified the example 'selectively provide the path to transfer. For example --path "folder_name" or --path="folder_name'.
The reason for failure in 5.6 was not having "use Symfony\Component\Console\Input\InputOption;". I hav e fixed that too.

Member

sharidas commented Mar 15, 2017

@PVince81 Kindly review my updated PR. I have removed ugly changes which directly access FS. Used $view->is_dir in the updated PR. I have also modified the example 'selectively provide the path to transfer. For example --path "folder_name" or --path="folder_name'.
The reason for failure in 5.6 was not having "use Symfony\Component\Console\Input\InputOption;". I hav e fixed that too.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
'path',
null,
InputOption::VALUE_OPTIONAL,
'selectively provide the path to transfer. For example --path "folder_name" or --path="folder_name"'

This comment has been minimized.

@PVince81

PVince81 Mar 15, 2017

Member

--path "folder_name" or --path="folder_name" ? it's twice the same

@PVince81

PVince81 Mar 15, 2017

Member

--path "folder_name" or --path="folder_name" ? it's twice the same

@PVince81

Almost there!

A few more required changes, especially the one about init mount points.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
$unknownDir = $this->inputPath;
$this->inputPath = $this->sourceUser . "/files/" . $this->inputPath;
if (!$view->is_dir($this->inputPath)) {
$output->writeln("<error>Unknown path provided:$unknownDir</error>");

This comment has been minimized.

@PVince81

PVince81 Mar 15, 2017

Member

add a space after ":" for better readability

@PVince81

PVince81 Mar 15, 2017

Member

add a space after ":" for better readability

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
if (strlen($this->inputPath) >= 1) {
$view = new View();
$unknownDir = $this->inputPath;
$this->inputPath = $this->sourceUser . "/files/" . $this->inputPath;

This comment has been minimized.

@PVince81

PVince81 Mar 15, 2017

Member

I suggest you use ltrim($this->inputPath, '/') to avoid doubling slashes in case the user input a leading slash in the path

@PVince81

PVince81 Mar 15, 2017

Member

I suggest you use ltrim($this->inputPath, '/') to avoid doubling slashes in case the user input a leading slash in the path

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
$this->inputPath = $input->getOption('path');
if (strlen($this->inputPath) >= 1) {
$view = new View();

This comment has been minimized.

@PVince81

PVince81 Mar 15, 2017

Member

Before you can use the view you need to first call initMountPoints which is done a bit later.
I suggest you move this validation after the initMountPoints call below, or at least the one from the source user.

The reason for this is that sometimes the storage is not directly on FS but in an object store. Only if the object store is mounted for that user will you be able to access it with the View.

@PVince81

PVince81 Mar 15, 2017

Member

Before you can use the view you need to first call initMountPoints which is done a bit later.
I suggest you move this validation after the initMountPoints call below, or at least the one from the source user.

The reason for this is that sometimes the storage is not directly on FS but in an object store. Only if the object store is mounted for that user will you be able to access it with the View.

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
// This change will help user to transfer the folder specified using --path option.
// Else only the content inside folder is transferred which is not correct.
if (strlen($this->inputPath) > 0) {
if($this->inputPath !== "$this->sourceUser/files") {

This comment has been minimized.

@PVince81

PVince81 Mar 15, 2017

Member

ah yes, when comparing paths you really need to make sure there are no double slashes.

this makes the ltrim($path, '/') call I suggested above really necessary.

@PVince81

PVince81 Mar 15, 2017

Member

ah yes, when comparing paths you really need to make sure there are no double slashes.

this makes the ltrim($path, '/') call I suggested above really necessary.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 15, 2017

Member

@PVince81 I have moved the validation part for the path under the initMount. Have provided ltrim for the path ( i.e, $this->inputPath ) and during the path comparison I have used ltrim both in the left hand side and right hand side of the comparison. I have added leading space for $unknownDir ( around the ). I have removed --path="folder_name" in the help. All these are made in the updated commit.

Member

sharidas commented Mar 15, 2017

@PVince81 I have moved the validation part for the path under the initMount. Have provided ltrim for the path ( i.e, $this->inputPath ) and during the path comparison I have used ltrim both in the left hand side and right hand side of the comparison. I have added leading space for $unknownDir ( around the ). I have removed --path="folder_name" in the help. All these are made in the updated commit.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Mar 15, 2017

Member

This behaviour should not be this way:

/occ files:transfer-ownership --help

--path[=PATH] selectively provide the path to transfer. For example --path "folder_name"

it looks like path value is optional, that's misleading.

It should look like --path=PATH

Also when running the command without --help it should show the same [--path=PATH] In this case:

./occ files:transfer-ownership       

                                                                    
  [Symfony\Component\Console\Exception\RuntimeException]            
  Not enough arguments (missing: "source-user, destination-user").  
                                                                    

files:transfer-ownership [--path [PATH]] [--] <source-user> <destination-user>
Member

SergioBertolinSG commented Mar 15, 2017

This behaviour should not be this way:

/occ files:transfer-ownership --help

--path[=PATH] selectively provide the path to transfer. For example --path "folder_name"

it looks like path value is optional, that's misleading.

It should look like --path=PATH

Also when running the command without --help it should show the same [--path=PATH] In this case:

./occ files:transfer-ownership       

                                                                    
  [Symfony\Component\Console\Exception\RuntimeException]            
  Not enough arguments (missing: "source-user, destination-user").  
                                                                    

files:transfer-ownership [--path [PATH]] [--] <source-user> <destination-user>
Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
@@ -103,6 +113,8 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$this->sourceUser = $sourceUserObject->getUID();
$this->destinationUser = $destinationUserObject->getUID();
$this->inputPath = $input->getOption('path');
$this->inputPath = ltrim($this->inputPath);

This comment has been minimized.

@PVince81

PVince81 Mar 16, 2017

Member

ltrim($this->inputPath,'/') to remove leading slashes

@PVince81

PVince81 Mar 16, 2017

Member

ltrim($this->inputPath,'/') to remove leading slashes

Show outdated Hide outdated apps/files/lib/Command/TransferOwnership.php
// This change will help user to transfer the folder specified using --path option.
// Else only the content inside folder is transferred which is not correct.
if (strlen($this->inputPath) > 0) {
if(ltrim($this->inputPath, '/') !== ltrim("$this->sourceUser/files", '/')) {

This comment has been minimized.

@PVince81

PVince81 Mar 16, 2017

Member

you don't need to ltrim the input path if you do it right at the start

@PVince81

PVince81 Mar 16, 2017

Member

you don't need to ltrim the input path if you do it right at the start

if (strlen($this->inputPath) >= 1) {
$view = new View();
$unknownDir = $this->inputPath;
$this->inputPath = $this->sourceUser . "/files/" . $this->inputPath;

This comment has been minimized.

@PVince81

PVince81 Mar 16, 2017

Member

the reason I'm asking you to ltrim the path is that if someone sets --path=/somedir, this concatenation here would result in "$userId/files//somedir" (double slash)

@PVince81

PVince81 Mar 16, 2017

Member

the reason I'm asking you to ltrim the path is that if someone sets --path=/somedir, this concatenation here would result in "$userId/files//somedir" (double slash)

sharidas added some commits Mar 9, 2017

Providing --path option to transfer-ownership
This will help user to selectively move the folders
specified using --path option, instead of moving
entire folder under files directory.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
Update the integration test for transfer-ownership
Update the integration test for transfer-ownership
as the new option --path is introduced in the command.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 16, 2017

Member

👍 if tests still pass

Member

PVince81 commented Mar 16, 2017

👍 if tests still pass

@PVince81 PVince81 added this to the 10.0 milestone Mar 16, 2017

@PVince81 PVince81 merged commit 90a094a into owncloud:master Mar 17, 2017

4 checks passed

Scrutinizer 3 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment