Polishing of backend code for albums

cypress/e2e/shared_albums.cy.js

@@ -47,6 +47,9 @@ describe('Manage shared albums', () => {
     cy.visit(`${Cypress.env('baseUrl')}/index.php/apps/photos/albums`)
     cy.createAnAlbumFromAlbums('shared_album_test3')
     cy.addCollaborators([randUser2])
+    cy.visit(`${Cypress.env('baseUrl')}/index.php/apps/photos/albums`)
+    cy.createAnAlbumFromAlbums('shared_album_test4')
+    cy.addCollaborators([randUser2])
     cy.logout()
 
     cy.login(randUser2, 'password')
@@ -120,8 +123,23 @@ describe('Manage shared albums', () => {
     cy.get('[data-test="media"]').should('have.length', 0)
   })
 
-  xit('Remove collaborator from an album', () => {
+  xit('Remove shared album', () => {
     cy.goToSharedAlbum('shared_album_test3')
     cy.removeSharedAlbums()
   })
+
+  xit('Remove collaborator from an album', () => {
+    cy.get('[data-test="media"]').should('have.length', 4)
+
+    cy.logout()
+    cy.login(randUser, 'password')
+    cy.goToAlbum('shared_album_test4')
+    cy.removeCollaborators([randUser2])
+    cy.logout()
+
+    cy.login(randUser2, 'password')
+    cy.visit(`${Cypress.env('baseUrl')}/index.php/apps/photos/sharedalbums`)
+
+    cy.get('ul.collections__list li').should('have.length', 3)
+  })
 })

lib/Album/AlbumMapper.php

@@ -249,11 +249,11 @@ private function getLastAdded(int $albumId): int {
 
 	/**
 	 * @param int $albumId
-	 * @return array<array{'id': string, 'label': string, 'source': int}>
+	 * @return array<array{'id': string, 'label': string, 'type': int}>
 	 */
 	public function getCollaborators(int $albumId): array {
 		$query = $this->connection->getQueryBuilder();
-		$query->select("collaborator_id", "collaborator_source")
+		$query->select("collaborator_id", "collaborator_type")
 			->from("photos_collaborators")
 			->where($query->expr()->eq('album_id', $query->createNamedParameter($albumId, IQueryBuilder::PARAM_INT)));
 
@@ -263,15 +263,15 @@ public function getCollaborators(int $albumId): array {
 			/** @var IUser|IGroup|null */
 			$collaborator = null;
 
-			switch ($row['collaborator_source']) {
+			switch ($row['collaborator_type']) {
 				case self::TYPE_USER:
 					$collaborator = $this->userManager->get($row['collaborator_id']);
 					break;
 				case self::TYPE_GROUP:
 					$collaborator = $this->groupManager->get($row['collaborator_id']);
 					break;
 				default:
-					throw new \Exception('Invalid collaborator source: ' . $row['collaborator_source']);
+					throw new \Exception('Invalid collaborator type: ' . $row['collaborator_type']);
 			}
 
 			if (is_null($collaborator)) {
@@ -281,7 +281,7 @@ public function getCollaborators(int $albumId): array {
 			return [
 				'id' => $row['collaborator_id'],
 				'label' => $collaborator->getDisplayName(),
-				'source' => (int)$row['collaborator_source'],
+				'type' => $row['collaborator_type'],
 			];
 		}, $rows);
 
@@ -290,16 +290,18 @@ public function getCollaborators(int $albumId): array {
 
 	/**
 	 * @param int $albumId
-	 * @param array{'id': string, 'label': string, 'source': int} $newCollaborators
+	 * @param array{'id': string, 'label': string, 'type': int} $collaborators
 	 */
-	public function setCollaborators(int $albumId, array $newCollaborators): void {
+	public function setCollaborators(int $albumId, array $collaborators): void {
 		$existingCollaborators = $this->getCollaborators($albumId);
 
-		$collaboratorsToAdd = array_udiff($newCollaborators, $existingCollaborators, fn ($a, $b) => strcmp($a['id'].$a['source'], $b['id'].$b['source']));
-		$collaboratorsToRemove = array_udiff($existingCollaborators, $newCollaborators, fn ($a, $b) => strcmp($a['id'].$a['source'], $b['id'].$b['source']));
+		$collaboratorsToAdd = array_udiff($collaborators, $existingCollaborators, fn ($a, $b) => strcmp($a['id'].$a['type'], $b['id'].$b['type']));
+		$collaboratorsToRemove = array_udiff($existingCollaborators, $collaborators, fn ($a, $b) => strcmp($a['id'].$a['type'], $b['id'].$b['type']));
+
+		$this->connection->beginTransaction();
 
 		foreach ($collaboratorsToAdd as $collaborator) {
-			switch ($collaborator['source']) {
+			switch ($collaborator['type']) {
 				case self::TYPE_USER:
 					if (is_null($this->userManager->get($collaborator['id']))) {
 						throw new \Exception('Unknown collaborator: ' . $collaborator['id']);
@@ -311,14 +313,14 @@ public function setCollaborators(int $albumId, array $newCollaborators): void {
 					}
 					break;
 				default:
-					throw new \Exception('Invalid collaborator source: ' . $collaborator['source']);
+					throw new \Exception('Invalid collaborator type: ' . $collaborator['type']);
 			}
 
 			$query = $this->connection->getQueryBuilder();
 			$query->insert('photos_collaborators')
 				->setValue('album_id', $query->createNamedParameter($albumId, IQueryBuilder::PARAM_INT))
 				->setValue('collaborator_id', $query->createNamedParameter($collaborator['id']))
-				->setValue('collaborator_source', $query->createNamedParameter($collaborator['source'], IQueryBuilder::PARAM_INT))
+				->setValue('collaborator_type', $query->createNamedParameter($collaborator['type'], IQueryBuilder::PARAM_INT))
 				->executeStatement();
 		}
 
@@ -327,16 +329,19 @@ public function setCollaborators(int $albumId, array $newCollaborators): void {
 			$query->delete('photos_collaborators')
 				->where($query->expr()->eq('album_id', $query->createNamedParameter($albumId, IQueryBuilder::PARAM_INT)))
 				->andWhere($query->expr()->eq('collaborator_id', $query->createNamedParameter($collaborator['id'])))
-				->andWhere($query->expr()->eq('collaborator_source', $query->createNamedParameter($collaborator['source'], IQueryBuilder::PARAM_INT)))
+				->andWhere($query->expr()->eq('collaborator_type', $query->createNamedParameter($collaborator['type'], IQueryBuilder::PARAM_INT)))
 				->executeStatement();
 		}
+
+		$this->connection->commit();
 	}
 
 	/**
-	 * @param string $userId
+	 * @param string $collaboratorId
+	 * @param string $collaboratorsType - The type of the collaborator, either a user or a group.
 	 * @return AlbumWithFiles[]
 	 */
-	public function getSharedAlbumsForCollaboratorWithFiles(string $collaboratorId, int $collaboratorSource): array {
+	public function getSharedAlbumsForCollaboratorWithFiles(string $collaboratorId, int $collaboratorType): array {
 		$query = $this->connection->getQueryBuilder();
 		$rows = $query
 			->select("fileid", "mimetype", "a.album_id", "size", "mtime", "etag", "location", "created", "last_added_photo", "added", 'owner')
@@ -348,7 +353,7 @@ public function getSharedAlbumsForCollaboratorWithFiles(string $collaboratorId,
 			->leftJoin("a", "photos_albums_files", "p", $query->expr()->eq("a.album_id", "p.album_id"))
 			->leftJoin("p", "filecache", "f", $query->expr()->eq("p.file_id", "f.fileid"))
 			->where($query->expr()->eq('collaborator_id', $query->createNamedParameter($collaboratorId)))
-			->andWhere($query->expr()->eq('collaborator_source', $query->createNamedParameter($collaboratorSource, IQueryBuilder::PARAM_INT)))
+			->andWhere($query->expr()->eq('collaborator_type', $query->createNamedParameter($collaboratorType, IQueryBuilder::PARAM_INT)))
 			->executeQuery()
 			->fetchAll();
 
@@ -379,31 +384,31 @@ public function getSharedAlbumsForCollaboratorWithFiles(string $collaboratorId,
 	 * @param int $albumId
 	 * @return void
 	 */
-	public function deleteCollaboratorFromAlbum(string $userId, int $albumId): void {
+	public function deleteUserFromAlbumCollaboratorsList(string $userId, int $albumId): void {
 		// TODO: only delete if this was not a group share
 		$query = $this->connection->getQueryBuilder();
 		$query->delete('photos_collaborators')
 			->where($query->expr()->eq('album_id', $query->createNamedParameter($albumId, IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->eq('collaborator_id', $query->createNamedParameter($userId)))
-			->andWhere($query->expr()->eq('collaborator_source', $query->createNamedParameter(self::TYPE_USER, IQueryBuilder::PARAM_INT)))
+			->andWhere($query->expr()->eq('collaborator_type', $query->createNamedParameter(self::TYPE_USER, IQueryBuilder::PARAM_INT)))
 			->executeStatement();
 	}
 
 	/**
 	 * @param string $collaboratorId
-	 * @param int $collaboratorSource
+	 * @param int $collaboratorType
 	 * @param int $fileId
 	 * @return AlbumInfo[]
 	 */
-	public function getAlbumForCollaboratorIdAndFileId(string $collaboratorId, int $collaboratorSource, int $fileId): array {
+	public function getAlbumForCollaboratorIdAndFileId(string $collaboratorId, int $collaboratorType, int $fileId): array {
 		$query = $this->connection->getQueryBuilder();
 		$rows = $query
 			->select("a.album_id", "name", "user", "location", "created", "last_added_photo")
 			->from("photos_collaborators", "c")
 			->leftJoin("c", "photos_albums", "a", $query->expr()->eq("a.album_id", "c.album_id"))
 			->leftJoin("a", "photos_albums_files", "p", $query->expr()->eq("a.album_id", "p.album_id"))
 			->where($query->expr()->eq('collaborator_id', $query->createNamedParameter($collaboratorId)))
-			->andWhere($query->expr()->eq('collaborator_source', $query->createNamedParameter($collaboratorSource, IQueryBuilder::PARAM_INT)))
+			->andWhere($query->expr()->eq('collaborator_type', $query->createNamedParameter($collaboratorType, IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->eq('file_id', $query->createNamedParameter($fileId)))
 			->groupBy('album_id')
 			->executeQuery()

lib/Migration/Version20001Date20220830131446.php

@@ -45,8 +45,10 @@ class Version20001Date20220830131446 extends SimpleMigrationStep {
 	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
 		/** @var ISchemaWrapper $schema */
 		$schema = $schemaClosure();
+		$modified = false;
 
 		if (!$schema->hasTable("photos_collaborators")) {
+			$modified = true;
 			$table = $schema->createTable("photos_collaborators");
 			$table->addColumn('album_id', Types::BIGINT, [
 				'notnull' => true,
@@ -56,21 +58,26 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt
 				'notnull' => true,
 				'length' => 64,
 			]);
-			$table->addColumn('collaborator_source', Types::INTEGER, [
+			$table->addColumn('collaborator_type', Types::INTEGER, [
 				'notnull' => true,
 			]);
 
-			$table->addUniqueConstraint(['album_id', 'collaborator_id', 'collaborator_source'], 'collaborators_unique_idx');
+			$table->addUniqueConstraint(['album_id', 'collaborator_id', 'collaborator_type'], 'collaborators_unique_idx');
 		}
 
 		if (!$schema->getTable("photos_albums_files")->hasColumn("owner")) {
+			$modified = true;
 			$table = $schema->getTable("photos_albums_files");
 			$table->addColumn('owner', Types::STRING, [
 				'notnull' => true,
 				'length' => 64,
 			]);
 		}
 
-		return $schema;
+		if ($modified) {
+			return $schema;
+		} else {
+			return null;
+		}
 	}
 }

lib/Sabre/Album/AlbumPhoto.php

@@ -36,27 +36,27 @@
 class AlbumPhoto implements IFile {
 	private AlbumMapper $albumMapper;
 	private AlbumInfo $album;
-	private AlbumFile $file;
+	private AlbumFile $albumFile;
 	private IRootFolder $rootFolder;
 
 	public const TAG_FAVORITE = '_$!<Favorite>!$_';
 
-	public function __construct(AlbumMapper $albumMapper, AlbumInfo $album, AlbumFile $file, IRootFolder $rootFolder) {
+	public function __construct(AlbumMapper $albumMapper, AlbumInfo $album, AlbumFile $albumFile, IRootFolder $rootFolder) {
 		$this->albumMapper = $albumMapper;
 		$this->album = $album;
-		$this->file = $file;
+		$this->albumFile = $albumFile;
 		$this->rootFolder = $rootFolder;
 	}
 
 	/**
 	 * @return void
 	 */
 	public function delete() {
-		$this->albumMapper->removeFile($this->album->getId(), $this->file->getFileId());
+		$this->albumMapper->removeFile($this->album->getId(), $this->albumFile->getFileId());
 	}
 
 	public function getName() {
-		return $this->file->getFileId() . "-" . $this->file->getName();
+		return $this->albumFile->getFileId() . "-" . $this->albumFile->getName();
 	}
 
 	/**
@@ -67,7 +67,7 @@ public function setName($name) {
 	}
 
 	public function getLastModified() {
-		return $this->file->getMTime();
+		return $this->albumFile->getMTime();
 	}
 
 	public function put($data) {
@@ -76,8 +76,8 @@ public function put($data) {
 
 	public function get() {
 		$nodes = $this->rootFolder
-			->getUserFolder($this->file->getOwner())
-			->getById($this->file->getFileId());
+			->getUserFolder($this->albumFile->getOwner() || $this->album->getUserId())
+			->getById($this->albumFile->getFileId());
 		$node = current($nodes);
 		if ($node) {
 			/** @var Node $node */
@@ -92,13 +92,13 @@ public function get() {
 	}
 
 	public function getFileId(): int {
-		return $this->file->getFileId();
+		return $this->albumFile->getFileId();
 	}
 
 	public function getFileInfo(): Node {
 		$nodes = $this->rootFolder
-			->getUserFolder($this->file->getOwner())
-			->getById($this->file->getFileId());
+			->getUserFolder($this->albumFile->getOwner() ?? $this->album->getUserId())
+			->getById($this->albumFile->getFileId());
 		$node = current($nodes);
 		if ($node) {
 			return $node;
@@ -108,19 +108,19 @@ public function getFileInfo(): Node {
 	}
 
 	public function getContentType() {
-		return $this->file->getMimeType();
+		return $this->albumFile->getMimeType();
 	}
 
 	public function getETag() {
-		return $this->file->getEtag();
+		return $this->albumFile->getEtag();
 	}
 
 	public function getSize() {
-		return $this->file->getSize();
+		return $this->albumFile->getSize();
 	}
 
 	public function getFile(): AlbumFile {
-		return $this->file;
+		return $this->albumFile;
 	}
 
 	public function isFavorite(): bool {
@@ -141,9 +141,9 @@ public function setFavoriteState($favoriteState): bool {
 
 		switch ($favoriteState) {
 			case "0":
-				return $tagger->removeFromFavorites($this->file->getFileId());
+				return $tagger->removeFromFavorites($this->albumFile->getFileId());
 			case "1":
-				return $tagger->addToFavorites($this->file->getFileId());
+				return $tagger->addToFavorites($this->albumFile->getFileId());
 			default:
 				new \Exception('Favorite state is invalide, should be 0 or 1.');
 		}

lib/Sabre/Album/SharedAlbumRoot.php

@@ -32,7 +32,7 @@ class SharedAlbumRoot extends AlbumRoot {
 	 * @return void
 	 */
 	public function delete() {
-		$this->albumMapper->deleteCollaboratorFromAlbum($this->user->getUID(), $this->album->getAlbum()->getId());
+		$this->albumMapper->deleteUserFromAlbumCollaboratorsList($this->user->getUID(), $this->album->getAlbum()->getId());
 	}
 
 	/**
@@ -48,7 +48,7 @@ protected function addFile(int $sourceId, string $ownerUID): bool {
 		}
 
 		$collaboratorIds = array_map(
-			fn ($collaborator) => $collaborator['source'].':'.$collaborator['id'],
+			fn ($collaborator) => $collaborator['type'].':'.$collaborator['id'],
 			$this->albumMapper->getCollaborators($this->album->getAlbum()->getId()),
 		);
 		$uid = $this->user->getUID();