fix(server, web): harden auto pick album thumbnails (#918)

server/apps/immich/src/api-v1/album/album-repository.ts

@@ -1,9 +1,9 @@
 import { AlbumEntity } from '@app/database/entities/album.entity';
 import { AssetAlbumEntity } from '@app/database/entities/asset-album.entity';
 import { UserAlbumEntity } from '@app/database/entities/user-album.entity';
-import { BadRequestException, Injectable } from '@nestjs/common';
+import { Injectable } from '@nestjs/common';
 import { InjectRepository } from '@nestjs/typeorm';
-import { Repository, SelectQueryBuilder, DataSource } from 'typeorm';
+import { In, Repository, SelectQueryBuilder, DataSource } from 'typeorm';
 import { AddAssetsDto } from './dto/add-assets.dto';
 import { AddUsersDto } from './dto/add-users.dto';
 import { CreateAlbumDto } from './dto/create-album.dto';
@@ -11,7 +11,7 @@ import { GetAlbumsDto } from './dto/get-albums.dto';
 import { RemoveAssetsDto } from './dto/remove-assets.dto';
 import { UpdateAlbumDto } from './dto/update-album.dto';
 import { AlbumCountResponseDto } from './response-dto/album-count-response.dto';
-import {AddAssetsResponseDto} from "./response-dto/add-assets-response.dto";
+import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto';
 
 export interface IAlbumRepository {
   create(ownerId: string, createAlbumDto: CreateAlbumDto): Promise<AlbumEntity>;
@@ -20,7 +20,7 @@ export interface IAlbumRepository {
   delete(album: AlbumEntity): Promise<void>;
   addSharedUsers(album: AlbumEntity, addUsersDto: AddUsersDto): Promise<AlbumEntity>;
   removeUser(album: AlbumEntity, userId: string): Promise<void>;
-  removeAssets(album: AlbumEntity, removeAssets: RemoveAssetsDto): Promise<AlbumEntity>;
+  removeAssets(album: AlbumEntity, removeAssets: RemoveAssetsDto): Promise<number>;
   addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise<AddAssetsResponseDto>;
   updateAlbum(album: AlbumEntity, updateAlbumDto: UpdateAlbumDto): Promise<AlbumEntity>;
   getListByAssetId(userId: string, assetId: string): Promise<AlbumEntity[]>;
@@ -237,28 +237,13 @@ export class AlbumRepository implements IAlbumRepository {
     await this.userAlbumRepository.delete({ albumId: album.id, sharedUserId: userId });
   }
 
-  async removeAssets(album: AlbumEntity, removeAssetsDto: RemoveAssetsDto): Promise<AlbumEntity> {
-    let deleteAssetCount = 0;
-    // TODO: should probably do a single delete query?
-    for (const assetId of removeAssetsDto.assetIds) {
-      const res = await this.assetAlbumRepository.delete({ albumId: album.id, assetId: assetId });
-      if (res.affected == 1) deleteAssetCount++;
-    }
-
-    // TODO: No need to return boolean if using a singe delete query
-    if (deleteAssetCount == removeAssetsDto.assetIds.length) {
-      const retAlbum = (await this.get(album.id)) as AlbumEntity;
-
-      if (retAlbum?.assets?.length === 0) {
-        // is empty album
-        await this.albumRepository.update(album.id, { albumThumbnailAssetId: null });
-        retAlbum.albumThumbnailAssetId = null;
-      }
+  async removeAssets(album: AlbumEntity, removeAssetsDto: RemoveAssetsDto): Promise<number> {
+    const res = await this.assetAlbumRepository.delete({
+      albumId: album.id,
+      assetId: In(removeAssetsDto.assetIds),
+    });
 
-      return retAlbum;
-    } else {
-      throw new BadRequestException('Some assets were not found in the album');
-    }
+    return res.affected || 0;
   }
 
   async addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise<AddAssetsResponseDto> {
@@ -267,7 +252,7 @@ export class AlbumRepository implements IAlbumRepository {
 
     for (const assetId of addAssetsDto.assetIds) {
       // Album already contains that asset
-      if (album.assets?.some(a => a.assetId === assetId)) {
+      if (album.assets?.some((a) => a.assetId === assetId)) {
         alreadyExisting.push(assetId);
         continue;
       }
@@ -288,7 +273,7 @@ export class AlbumRepository implements IAlbumRepository {
 
     return {
       successfullyAdded: newRecords.length,
-      alreadyInAlbum: alreadyExisting
+      alreadyInAlbum: alreadyExisting,
     };
   }
 

server/apps/immich/src/api-v1/album/album.service.ts

@@ -65,11 +65,13 @@ export class AlbumService {
    * @returns All Shared Album And Its Members
    */
   async getAllAlbums(authUser: AuthUserDto, getAlbumsDto: GetAlbumsDto): Promise<AlbumResponseDto[]> {
+    let albums: AlbumEntity[];
+
     if (typeof getAlbumsDto.assetId === 'string') {
-      const albums = await this._albumRepository.getListByAssetId(authUser.id, getAlbumsDto.assetId);
-      return albums.map(mapAlbumExcludeAssetInfo);
+      albums = await this._albumRepository.getListByAssetId(authUser.id, getAlbumsDto.assetId);
+    } else {
+      albums = await this._albumRepository.getList(authUser.id, getAlbumsDto);
     }
-    const albums = await this._albumRepository.getList(authUser.id, getAlbumsDto);
 
     for (const album of albums) {
       await this._checkValidThumbnail(album);
@@ -112,8 +114,18 @@ export class AlbumService {
     albumId: string,
   ): Promise<AlbumResponseDto> {
     const album = await this._getAlbum({ authUser, albumId });
-    const updateAlbum = await this._albumRepository.removeAssets(album, removeAssetsDto);
-    return mapAlbum(updateAlbum);
+    const deletedCount = await this._albumRepository.removeAssets(album, removeAssetsDto);
+    const newAlbum = await this._getAlbum({ authUser, albumId });
+
+    if (newAlbum) {
+      await this._checkValidThumbnail(newAlbum);
+    }
+
+    if (deletedCount !== removeAssetsDto.assetIds.length) {
+      throw new BadRequestException('Some assets were not found in the album');
+    }
+
+    return mapAlbum(newAlbum);
   }
 
   async addAssetsToAlbum(
@@ -178,17 +190,17 @@ export class AlbumService {
     }
   }
 
-  async _checkValidThumbnail(album: AlbumEntity): Promise<AlbumEntity> {
-    const assetId = album.albumThumbnailAssetId;
-    if (assetId) {
-      try {
-        await this._assetRepository.getById(assetId);
-      } catch (e) {
-        album.albumThumbnailAssetId = null;
-        return await this._albumRepository.updateAlbum(album, {});
+  async _checkValidThumbnail(album: AlbumEntity) {
+    const assets = album.assets || [];
+    const valid = assets.some((asset) => asset.assetId === album.albumThumbnailAssetId);
+    if (!valid) {
+      let dto: UpdateAlbumDto = {};
+      if (assets.length > 0) {
+        const albumThumbnailAssetId = assets[0].assetId;
+        dto = { albumThumbnailAssetId };
       }
+      await this._albumRepository.updateAlbum(album, dto);
+      album.albumThumbnailAssetId = dto.albumThumbnailAssetId || null;
     }
-
-    return album;
   }
 }

web/src/lib/components/album-page/__tests__/album-card.spec.ts

@@ -43,8 +43,8 @@ describe('AlbumCard component', () => {
 			const albumNameElement = sut.getByTestId('album-name');
 			const albumDetailsElement = sut.getByTestId('album-details');
 			const detailsText = `${count} items` + (shared ? ' . Shared' : '');
-			// TODO: is this a bug?
-			expect(albumImgElement).toHaveAttribute('src', '/api/asset/thumbnail/null?format=WEBP');
+
+			expect(albumImgElement).toHaveAttribute('src', 'no-thumbnail.png');
 			expect(albumImgElement).toHaveAttribute('alt', album.id);
 
 			await waitFor(() => expect(albumImgElement).toHaveAttribute('src', 'no-thumbnail.png'));

web/src/lib/components/album-page/album-card.svelte

@@ -19,7 +19,13 @@
 
 	export let album: AlbumResponseDto;
 
+	const NO_THUMBNAIL = 'no-thumbnail.png';
+
 	let imageData = `/api/asset/thumbnail/${album.albumThumbnailAssetId}?format=${ThumbnailFormat.Webp}`;
+	if (!album.albumThumbnailAssetId) {
+		imageData = NO_THUMBNAIL;
+	}
+
 	const dispatchClick = createEventDispatcher<OnClick>();
 	const dispatchShowContextMenu = createEventDispatcher<OnShowContextMenu>();
 
@@ -45,7 +51,7 @@
 	};
 
 	onMount(async () => {
-		imageData = (await loadHighQualityThumbnail(album.albumThumbnailAssetId)) || 'no-thumbnail.png';
+		imageData = (await loadHighQualityThumbnail(album.albumThumbnailAssetId)) || NO_THUMBNAIL;
 	});
 </script>