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

Handle exceptions for deleted share nodes while transfering ownership #37568

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Jun 22, 2020

When running files:transfer-ownership, we should not explode with exception when e.g. federated share reshare has been deleted by remote owner (thus resulting in NotFoundException or NoUserException)

This PR:

  • add exception handling for NotFoundException or NoUserException
  • prompt user if he wants to continue with transfer when
$ occ files:transfer-ownership -vvv --path='test' test1 test3
Analysing files of test1 ...
    1 [============================] < 1 sec 30.0 MiB
Collecting all share information for files and folder of test1 ...
Share with id 2 points at deleted file, skipping
    1 [============================]  1 sec 32.0 MiB
There were user shares that were skipped during the analysis of the shares. Do you want to continue with the transfer ?
  [0] yes
  [1] no
 > no
Execution terminated
  • add flag --accept-skipped-shares to always accept skipped shares

@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #37568 into master will decrease coverage by 0.00%.
The diff coverage is 58.06%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37568      +/-   ##
============================================
- Coverage     64.66%   64.65%   -0.01%     
- Complexity    19345    19351       +6     
============================================
  Files          1279     1279              
  Lines         75601    75624      +23     
  Branches       1333     1333              
============================================
+ Hits          48885    48896      +11     
- Misses        26324    26336      +12     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.83% <58.06%> (-0.01%) 19351.00 <9.00> (+6.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 77.14% <58.06%> (-4.44%) 49.00 <9.00> (+6.00) ⬇️

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 4116040...237097a. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #37568 into master will increase coverage by 0.01%.
The diff coverage is 96.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37568      +/-   ##
============================================
+ Coverage     64.70%   64.71%   +0.01%     
- Complexity    19353    19359       +6     
============================================
  Files          1281     1281              
  Lines         75614    75637      +23     
  Branches       1333     1333              
============================================
+ Hits          48926    48949      +23     
  Misses        26296    26296              
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.89% <96.77%> (+0.01%) 19359.00 <9.00> (+6.00)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 84.00% <96.77%> (+2.42%) 49.00 <9.00> (+6.00)

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 acd31e5...46a94fb. Read the comment docs.

@VicDeo
Copy link
Member

VicDeo commented Jun 25, 2020

@mrow4a looks good. Is it possible to increase coverage a bit?

@mrow4a
Copy link
Contributor Author

mrow4a commented Jun 25, 2020

Yes this is main problem, if you look into how tests are structured, there are no mocks, and we test here usecase with federation. I will see again if I can easily add test, probably by mocking everything and not using presettuped things.

@mmattel
Copy link
Contributor

mmattel commented Jun 26, 2020

Do you want to continue with the transfer ?

--allow-skipped-shares --> --continue-skipped-shares (proposal)

Doc relevant
This is a change in the occ command set --> pls file an issue in docs and reference this PR

@mrow4a mrow4a force-pushed the bugfix/handle-exceptions-transf-own branch from 237097a to c8a5da1 Compare June 28, 2020 17:06
@mrow4a
Copy link
Contributor Author

mrow4a commented Jun 28, 2020

@VicDeo @mmattel can you rereview, I added unit tests to cover changes and adjusted doc.

@mrow4a mrow4a requested review from VicDeo and mmattel June 28, 2020 17:07
@mmattel
Copy link
Contributor

mmattel commented Jun 28, 2020

You took my suggestion regarding naming the option (--continue-skipped-shares) in the changelog but in the code, you have to former version (--allow-skipped-shares)...

@mrow4a mrow4a force-pushed the bugfix/handle-exceptions-transf-own branch from c8a5da1 to 46a94fb Compare June 28, 2020 18:22
@mrow4a
Copy link
Contributor Author

mrow4a commented Jun 28, 2020

You took my suggestion regarding naming the option (--continue-skipped-shares) in the changelog but in the code, you have to former version (--allow-skipped-shares)...

Hi, sorry forgot to add changelong. I changed from --allow-skipped-shares to --accept-skipped-shares, it better reflects what it does - it just accepts by default to skip shares.

@mmattel
Copy link
Contributor

mmattel commented Jun 29, 2020

Pls file a doc issue, because of a change in the occ command set!

Copy link
Member

@VicDeo VicDeo left a comment

Choose a reason for hiding this comment

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

Great

@phil-davis
Copy link
Contributor

Doc issue owncloud/docs#2715

Possible port to 10.5.0 #37618 - TBD if it gets added there.

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.

4 participants