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

Introduce console command to transfer ownerships of files #20571

Merged
merged 3 commits into from
Feb 9, 2016

Conversation

DeepDiver1975
Copy link
Member

refs #19154

Requires Sharing 2.0 to implement getShares() and updateShare()

@schiesbn @rullzer

@DeepDiver1975 DeepDiver1975 added this to the 9.0-current milestone Nov 18, 2015
@karlitschek
Copy link
Contributor

nice! 👍

@DeepDiver1975 DeepDiver1975 force-pushed the transfer-ownership branch 2 times, most recently from 3e24ada to 5428e88 Compare November 26, 2015 16:12
@DeepDiver1975 DeepDiver1975 force-pushed the transfer-ownership branch 2 times, most recently from 7208a13 to 89546bb Compare December 18, 2015 07:01
@DeepDiver1975
Copy link
Member Author

@rullzer still waiting for getshares to be implemented 😉

@rullzer
Copy link
Contributor

rullzer commented Jan 7, 2016

@DeepDiver1975 it is on the todo... and a lot closer to happening now that createShares is in ;)

@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 it is on the todo... and a lot closer to happening now that createShares is in ;)

great!

@karlitschek
Copy link
Contributor

@msrex

@DeepDiver1975 DeepDiver1975 force-pushed the transfer-ownership branch 3 times, most recently from b536396 to cdb5e18 Compare January 22, 2016 10:12
@DeepDiver1975 DeepDiver1975 force-pushed the transfer-ownership branch 3 times, most recently from 72c699a to 477c7fd Compare January 29, 2016 16:00
@DeepDiver1975 DeepDiver1975 changed the title [WIP] Introduce console command to transfer ownerships of files Introduce console command to transfer ownerships of files Jan 29, 2016
@rullzer
Copy link
Contributor

rullzer commented Feb 1, 2016

Looks good. Did some testing and it seems to all work.

However I would very much like to have intergration tests for this... since I do assume there are corner cases where restoring shares will fail. Because it creates a duplicated share or whatever...

if ($share->getSharedWith() === $destinationUser) {
$this->shareManager->deleteShare($share);
} else {
$share->setShareOwner($destinationUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct.
The share owner might be a different user then we are migrating.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I only update the share owner in case the current owner is the user from whom we are moving away?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. Basically we get the file from the fileid in the owners folder... so if that is not set correctly.... 💥

@rullzer
Copy link
Contributor

rullzer commented Feb 1, 2016

So you copy the files from sourceUser to destUser. I get this. But this break the chain of shares. Since the fileId changes.

So assume.

  1. user1 shares foo with user2
  2. user2 shares foo with user3
  3. transfer ownership of user2 to user4.

Now all of a sudden the incomming share foo for user3 no longer is the same foo as user1 shared originally.

protected function transfer(OutputInterface $output) {
$view = new View();
$output->writeln("Transferring files to $this->finalTarget ...");
// $view->rename("$this->sourceUser/files", $this->finalTarget);
Copy link
Member Author

Choose a reason for hiding this comment

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

@rullzer rename is to be used - not copy. I use copy for testing - will fix asap

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good!

@DeepDiver1975
Copy link
Member Author

So you copy the files from sourceUser to destUser.

it's a move ....

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 Ready for review?

@DeepDiver1975 DeepDiver1975 force-pushed the transfer-ownership branch 3 times, most recently from 411a45e to 7e33e67 Compare February 8, 2016 14:54
@PVince81
Copy link
Contributor

PVince81 commented Feb 8, 2016

Tested, works now 👍

@PVince81
Copy link
Contributor

PVince81 commented Feb 8, 2016

Second reviewer ? @rullzer @icewind1991 @MorrisJobke @nickvergessen

@PVince81
Copy link
Contributor

PVince81 commented Feb 8, 2016

or @LukasReschke ?

private function restoreShares(OutputInterface $output) {
$output->writeln("Restoring shares ...");
$progress = new ProgressBar($output, count($this->shares));
/** @var Folder $sourceRoot */
Copy link
Contributor

Choose a reason for hiding this comment

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

This can go

@rullzer
Copy link
Contributor

rullzer commented Feb 8, 2016

Ran some tests and they passed.

I would recommend to add to the docs a big warning to backup their data. I do think there will still be corner cases where things might say boom.

Also it would be very nice if we could setup intergration tests for this.

My nitpickings aside.. 👍

public function __construct(IUserManager $userManager, IManager $shareManager, IRootFolder $rootFolder) {
$this->userManager = $userManager;
$this->shareManager = $shareManager;
$this->rootFolder = $rootFolder;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused

@DeepDiver1975
Copy link
Member Author

@icewind1991 description updated - please have a look

@icewind1991
Copy link
Contributor

Code looks good 👍

DeepDiver1975 added a commit that referenced this pull request Feb 9, 2016
Introduce console command to transfer ownerships of files
@DeepDiver1975 DeepDiver1975 merged commit ac08540 into master Feb 9, 2016
@DeepDiver1975 DeepDiver1975 deleted the transfer-ownership branch February 9, 2016 15:11
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants