X Tutup
Skip to content

Handle exceptions for deleted share nodes while transfering ownership#37568

Merged
phil-davis merged 1 commit intomasterfrom
bugfix/handle-exceptions-transf-own
Jul 1, 2020
Merged

Handle exceptions for deleted share nodes while transfering ownership#37568
phil-davis merged 1 commit intomasterfrom
bugfix/handle-exceptions-transf-own

Conversation

@mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Jun 22, 2020 •
edited
Loading

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

@mrow4a mrow4a self-assigned this Jun 22, 2020
@mrow4a mrow4a force-pushed the bugfix/handle-exceptions-transf-own branch from 82f41bd to 237097a Compare June 22, 2020 06:43
@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 •
edited
Loading

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 •
edited
Loading

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

mmattel
mmattel reviewed Jun 26, 2020
View reviewed changes
@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 •
edited
Loading

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!

VicDeo
VicDeo approved these changes Jun 30, 2020
View reviewed changes
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

@owncloud owncloud deleted a comment from update-docs bot Jul 1, 2020
@phil-davis phil-davis mentioned this pull request Jul 1, 2020
Closed
@phil-davis phil-davis merged commit ca9e2b0 into master Jul 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/handle-exceptions-transf-own branch July 1, 2020 03:13
@phil-davis phil-davis mentioned this pull request Jul 1, 2020
Merged
11 tasks
@phil-davis
Copy link
Contributor

phil-davis commented Jul 1, 2020

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

Reviewers

@VicDeo VicDeo VicDeo approved these changes

@mmattel mmattel mmattel approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@karakayasemi karakayasemi Awaiting requested review from karakayasemi

Assignees

@mrow4a mrow4a

Labels

3 - To Review app:federation app:files Type:Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrow4a @VicDeo @mmattel @phil-davis
X Tutup