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

only collect shares inside the given path in transfer-ownership command #36222

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Sep 26, 2019

Description

files:transfer-ownership command is not respecting to the given path when collecting shares for transfer. With this PR it will only collect shares inside the given path.

Related Issue

Motivation and Context

Even path argument given, files:transfer-ownership command is trying to transfer all shares of sourceUser. This situation causes random errors like mentioned issue. files:transfer-ownership command should only transfer the shares inside of the given folder.

How Has This Been Tested?

Unit tests and ✋ Manually with the following steps:

  • Create at least two user (user1, user2)
  • user1 creates a folder and fills it with random files.
  • user1 creates a file outside of the created folder and shares this file with user2.
  • Transfer the created folder with this command;
    ./occ files:transfer-ownership --path="folder-path" user1 user2

Expected behavior

  • The folder should be transferred without any error messages.

Current behavior

  • There are some errors in console output like Share with id x points at deleted file, skipping, because the command is trying to transfer all the shares.

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:

@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #36222 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36222      +/-   ##
============================================
+ Coverage     64.84%      65%   +0.16%     
- Complexity    19768    19772       +4     
============================================
  Files          1271     1271              
  Lines         74697    74695       -2     
  Branches       1309     1309              
============================================
+ Hits          48434    48554     +120     
+ Misses        25877    25755     -122     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.21% <100%> (+0.18%) 19772 <3> (+4) ⬆️
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 81.08% <100%> (+81.08%) 50 <3> (+4) ⬆️

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 cc3c4a4...e063364. Read the comment docs.

@micbar
Copy link
Contributor

micbar commented Sep 27, 2019

@karakayasemi could you increase coverage?

* strpos result should be 1 for the shares inside source folder,
* because `/` character trimmed with ltrim in inputPath
*/
return \strpos($share->getNode()->getPath(), $this->inputPath) === 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Without testing myself, I have a suspicion that if:

  • the user has 2 folders sub and subfolder
  • both folders contain shares
  • someone tries to transfer for sub

This will also match subfolder, so shares from both will get transferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karakayasemi please investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phil-davis Most probably, you are right about your suspicions. I will investigate and fix it. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

We had to do similar strpos path-parsing logic recently in https://github.com/owncloud/core/pull/36170/files - it might give some clues, or you can think of a cleaner way.

@phil-davis
Copy link
Contributor

@karakayasemi could you increase coverage?

There is also tests/acceptance/features/cliMain/transfer-ownership.feature - hopefully it should be easy to make or adjust acceptance test scenarios to cover this.

@karakayasemi
Copy link
Contributor Author

@karakayasemi could you increase coverage?

The command has not any unit test class. But, I can add tests by using Symfony's CommandTester class.

@micbar
Copy link
Contributor

micbar commented Sep 27, 2019

@karakayasemi could you increase coverage?

The command has not any unit test class. But, I can add tests by using Symfony's CommandTester class.

Yes, there are good examples in other apps.

@karakayasemi karakayasemi force-pushed the transfer-ownership branch 2 times, most recently from 9336eb3 to 5b69941 Compare September 29, 2019 14:47
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Can you add a unit test where you transfer a subfolder and have another folder with shares which should not be transferred?
Like: testTransferSpecificFolderSkipOthers()

@micbar
Copy link
Contributor

micbar commented Oct 4, 2019

@karakayasemi Could you rebase?
After rebase you will get the new changelog generator templates.
You have the honour to place one of the the first changelog files into changelog. Just create a folder unreleased there if it does not exist and put a file there with your PR as a filename.
Write the changelog into that file according to https://github.com/owncloud/core/blob/master/changelog/TEMPLATE

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

3 participants