Handle moving calendar to an user who already has the share

Extra:
    * Fix @ChristophWurst style remarks
    * Add a Note that share links have changed when calendars has user shares (see #13603)

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix forgotten test change

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

2 files changed, 42 insertions, 19 deletions

diff --git a/apps/dav/lib/Command/ListCalendars.php b/apps/dav/lib/Command/ListCalendars.php
index 9080e9f8e29..6c2f5bdb506 100644
--- a/apps/dav/lib/Command/ListCalendars.php
+++ b/apps/dav/lib/Command/ListCalendars.php
@@ -59,13 +59,13 @@ class ListCalendars extends Command {
 		$this
 			->setName('dav:list-calendars')
 			->setDescription('List all calendars of a user')
-			->addArgument('user',
+			->addArgument('uid',
 				InputArgument::REQUIRED,
 				'User for whom all calendars will be listed');
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output) {
-		$user = $input->getArgument('user');
+		$user = $input->getArgument('uid');
 		if (!$this->userManager->userExists($user)) {
 			throw new \InvalidArgumentException("User <$user> is unknown.");
 		}
diff --git a/apps/dav/lib/Command/MoveCalendar.php b/apps/dav/lib/Command/MoveCalendar.php
index 23aee0f2d86..a2c7ca8c4d8 100644
--- a/apps/dav/lib/Command/MoveCalendar.php
+++ b/apps/dav/lib/Command/MoveCalendar.php
@@ -93,18 +93,18 @@ class MoveCalendar extends Command {
 			->addArgument('name',
 				InputArgument::REQUIRED,
 				'Name of the calendar to move')
-			->addArgument('userorigin',
+			->addArgument('sourceuid',
 				InputArgument::REQUIRED,
 				'User who currently owns the calendar')
-			->addArgument('userdestination',
+			->addArgument('destinationuid',
 				InputArgument::REQUIRED,
 				'User who will receive the calendar')
-			->addOption('force', 'f', InputOption::VALUE_NONE, "Force the migration by removing shares with groups that the destination user is not in");
+			->addOption('force', 'f', InputOption::VALUE_NONE, "Force the migration by removing existing shares");
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output) {
-		$userOrigin = $input->getArgument('userorigin');
-		$userDestination = $input->getArgument('userdestination');
+		$userOrigin = $input->getArgument('sourceuid');
+		$userDestination = $input->getArgument('destinationuid');
 
 		$this->io = new SymfonyStyle($input, $output);
 
@@ -128,9 +128,7 @@ class MoveCalendar extends Command {
 			throw new \InvalidArgumentException("User <$userDestination> already has a calendar named <$name>.");
 		}
 
-		if ($this->shareManager->shareWithGroupMembersOnly() === true) {
-			$this->checkShares($calendar, $userDestination, $input->getOption('force'));
-		}
+		$this->checkShares($calendar, $userOrigin, $userDestination, $input->getOption('force'));
 
 		$this->calDav->moveCalendar($name, self::URI_USERS . $userOrigin, self::URI_USERS . $userDestination);
 
@@ -138,25 +136,50 @@ class MoveCalendar extends Command {
 	}
 
 	/**
-	 * Check that user destination is member of the groups which whom the calendar was shared
-	 * If we ask to force the migration, the share with the group is dropped
+	 * Check that moving the calendar won't break shares
 	 *
-	 * @param $calendar
-	 * @param $userDestination
+	 * @param array $calendar
+	 * @param string $userOrigin
+	 * @param string $userDestination
 	 * @param bool $force
 	 */
-	private function checkShares($calendar, $userDestination, $force = false)
+	private function checkShares(array $calendar, string $userOrigin, string $userDestination, bool $force = false)
 	{
 		$shares = $this->calDav->getShares($calendar['id']);
 		foreach ($shares as $share) {
-			list(, $prefix, $group) = explode('/', $share['href'], 3);
-			if ('groups' === $prefix && !$this->groupManager->isInGroup($userDestination, $group)) {
+			list(, $prefix, $userOrGroup) = explode('/', $share['href'], 3);
+
+			/**
+			 * Check that user destination is member of the groups which whom the calendar was shared
+			 * If we ask to force the migration, the share with the group is dropped
+			 */
+			if ($this->shareManager->shareWithGroupMembersOnly() === true && 'groups' === $prefix && !$this->groupManager->isInGroup($userDestination, $userOrGroup)) {
+				if ($force) {
+					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/groups/' . $userOrGroup]);
+				} else {
+					throw new \InvalidArgumentException("User <$userDestination> is not part of the group <$userOrGroup> with whom the calendar <" . $calendar['uri'] . "> was shared. You may use -f to move the calendar while deleting this share.");
+				}
+			}
+
+			/**
+			 * Check that calendar isn't already shared with user destination
+			 */
+			if ($userOrGroup === $userDestination) {
 				if ($force) {
-					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/groups/' . $group]);
+					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/users/' . $userOrGroup]);
 				} else {
-					throw new \InvalidArgumentException("User <$userDestination> is not part of the group <$group> with whom the calendar <" . $calendar['uri'] . "> was shared. You may use -f to move the calendar while deleting this share.");
+					throw new \InvalidArgumentException("The calendar <" . $calendar['uri'] . "> is already shared to user <$userDestination>.You may use -f to move the calendar while deleting this share.");
 				}
 			}
 		}
+		/**
+		 * Warn that share links have changed if there are shares
+		 */
+		if (count($shares) > 0) {
+			$this->io->note([
+				"Please note that moving calendar " . $calendar['uri'] . " from user <$userOrigin> to <$userDestination> has caused share links to change.", 
+				"Sharees will need to change \"example.com/remote.php/dav/calendars/uid/" . $calendar['uri'] . "_shared_by_$userOrigin\" to \"example.com/remote.php/dav/calendars/uid/" . $calendar['uri'] . "_shared_by_$userDestination\""
+			]);
+		}
 	}
 }