refactor(server): delete album (#2570)

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

@@ -6,18 +6,15 @@ import { Repository } from 'typeorm';
 import { AddAssetsDto } from './dto/add-assets.dto';
 import { AddUsersDto } from './dto/add-users.dto';
 import { RemoveAssetsDto } from './dto/remove-assets.dto';
-import { UpdateAlbumDto } from '@app/domain';
 import { AlbumCountResponseDto } from './response-dto/album-count-response.dto';
 import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto';
 
 export interface IAlbumRepository {
   get(albumId: string): Promise<AlbumEntity | null>;
-  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<number>;
   addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise<AddAssetsResponseDto>;
-  updateAlbum(album: AlbumEntity, updateAlbumDto: UpdateAlbumDto): Promise<AlbumEntity>;
   updateThumbnails(): Promise<number | undefined>;
   getCountByUserId(userId: string): Promise<AlbumCountResponseDto>;
   getSharedWithUserAlbumCount(userId: string, assetId: string): Promise<number>;
@@ -62,10 +59,6 @@ export class AlbumRepository implements IAlbumRepository {
     });
   }
 
-  async delete(album: AlbumEntity): Promise<void> {
-    await this.albumRepository.delete({ id: album.id, ownerId: album.ownerId });
-  }
-
   async addSharedUsers(album: AlbumEntity, addUsersDto: AddUsersDto): Promise<AlbumEntity> {
     album.sharedUsers.push(...addUsersDto.sharedUserIds.map((id) => ({ id } as UserEntity)));
     album.updatedAt = new Date().toISOString();
@@ -128,13 +121,6 @@ export class AlbumRepository implements IAlbumRepository {
     };
   }
 
-  updateAlbum(album: AlbumEntity, updateAlbumDto: UpdateAlbumDto): Promise<AlbumEntity> {
-    album.albumName = updateAlbumDto.albumName || album.albumName;
-    album.albumThumbnailAssetId = updateAlbumDto.albumThumbnailAssetId || album.albumThumbnailAssetId;
-
-    return this.albumRepository.save(album);
-  }
-
   /**
    * Makes sure all thumbnails for albums are updated by:
    * - Removing thumbnails from albums without assets

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

@@ -77,12 +77,6 @@ export class AlbumController {
     return this.service.removeAssets(authUser, id, dto);
   }
 
-  @Authenticated()
-  @Delete(':id')
-  deleteAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) {
-    return this.service.delete(authUser, id);
-  }
-
   @Authenticated()
   @Delete(':id/user/:userId')
   removeUserFromAlbum(

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

@@ -121,11 +121,9 @@ describe('Album service', () => {
     albumRepositoryMock = {
       addAssets: jest.fn(),
       addSharedUsers: jest.fn(),
-      delete: jest.fn(),
       get: jest.fn(),
       removeAssets: jest.fn(),
       removeUser: jest.fn(),
-      updateAlbum: jest.fn(),
       updateThumbnails: jest.fn(),
       getCountByUserId: jest.fn(),
       getSharedWithUserAlbumCount: jest.fn(),
@@ -197,21 +195,6 @@ describe('Album service', () => {
     await expect(sut.get(authUser, '0002')).rejects.toBeInstanceOf(NotFoundException);
   });
 
-  it('deletes an owned album', async () => {
-    const albumEntity = _getOwnedAlbum();
-    albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
-    albumRepositoryMock.delete.mockImplementation(() => Promise.resolve());
-    await sut.delete(authUser, albumId);
-    expect(albumRepositoryMock.delete).toHaveBeenCalledTimes(1);
-    expect(albumRepositoryMock.delete).toHaveBeenCalledWith(albumEntity);
-  });
-
-  it('prevents deleting a shared album (shared with auth user)', async () => {
-    const albumEntity = _getSharedWithAuthUserAlbum();
-    albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));
-    await expect(sut.delete(authUser, albumId)).rejects.toBeInstanceOf(ForbiddenException);
-  });
-
   it('removes a shared user from an owned album', async () => {
     const albumEntity = _getOwnedSharedAlbum();
     albumRepositoryMock.get.mockImplementation(() => Promise.resolve<AlbumEntity>(albumEntity));

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

@@ -3,7 +3,7 @@ import { AuthUserDto } from '../../decorators/auth-user.decorator';
 import { AlbumEntity, SharedLinkType } from '@app/infra/entities';
 import { AddUsersDto } from './dto/add-users.dto';
 import { RemoveAssetsDto } from './dto/remove-assets.dto';
-import { AlbumResponseDto, IJobRepository, JobName, mapAlbum } from '@app/domain';
+import { AlbumResponseDto, IJobRepository, mapAlbum } from '@app/domain';
 import { IAlbumRepository } from './album-repository';
 import { AlbumCountResponseDto } from './response-dto/album-count-response.dto';
 import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto';
@@ -64,17 +64,6 @@ export class AlbumService {
     return mapAlbum(updatedAlbum);
   }
 
-  async delete(authUser: AuthUserDto, albumId: string): Promise<void> {
-    const album = await this._getAlbum({ authUser, albumId });
-
-    for (const sharedLink of album.sharedLinks) {
-      await this.shareCore.remove(authUser.id, sharedLink.id);
-    }
-
-    await this.albumRepository.delete(album);
-    await this.jobRepository.queue({ name: JobName.SEARCH_REMOVE_ALBUM, data: { ids: [albumId] } });
-  }
-
   async removeUser(authUser: AuthUserDto, albumId: string, userId: string | 'me'): Promise<void> {
     const sharedUserId = userId == 'me' ? authUser.id : userId;
     const album = await this._getAlbum({ authUser, albumId, validateIsOwner: false });

server/apps/immich/src/controllers/album.controller.ts

@@ -1,6 +1,6 @@
 /*  */ import { AlbumService, AuthUserDto, CreateAlbumDto, UpdateAlbumDto } from '@app/domain';
 import { GetAlbumsDto } from '@app/domain/album/dto/get-albums.dto';
-import { Body, Controller, Get, Param, Patch, Post, Query } from '@nestjs/common';
+import { Body, Controller, Delete, Get, Param, Patch, Post, Query } from '@nestjs/common';
 import { ApiTags } from '@nestjs/swagger';
 import { GetAuthUser } from '../decorators/auth-user.decorator';
 import { Authenticated } from '../decorators/authenticated.decorator';
@@ -15,7 +15,7 @@ export class AlbumController {
   constructor(private service: AlbumService) {}
 
   @Get()
-  async getAllAlbums(@GetAuthUser() authUser: AuthUserDto, @Query() query: GetAlbumsDto) {
+  getAllAlbums(@GetAuthUser() authUser: AuthUserDto, @Query() query: GetAlbumsDto) {
     return this.service.getAll(authUser, query);
   }
 
@@ -28,4 +28,9 @@ export class AlbumController {
   updateAlbumInfo(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body() dto: UpdateAlbumDto) {
     return this.service.update(authUser, id, dto);
   }
+
+  @Delete(':id')
+  deleteAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) {
+    return this.service.delete(authUser, id);
+  }
 }

server/immich-openapi-specs.json

@@ -143,7 +143,31 @@
           },
           {
             "api_key": []
+          }
+        ]
       },
+      "delete": {
+        "operationId": "deleteAlbum",
+        "parameters": [
+          {
+            "name": "id",
+            "required": true,
+            "in": "path",
+            "schema": {
+              "format": "uuid",
+              "type": "string"
+            }
+          }
+        ],
+        "responses": {
+          "200": {
+            "description": ""
+          }
+        },
+        "tags": [
+          "Album"
+        ],
+        "security": [
           {
             "bearer": []
           },
@@ -202,39 +226,6 @@
             "api_key": []
           }
         ]
-      },
-      "delete": {
-        "operationId": "deleteAlbum",
-        "parameters": [
-          {
-            "name": "id",
-            "required": true,
-            "in": "path",
-            "schema": {
-              "format": "uuid",
-              "type": "string"
-            }
-          }
-        ],
-        "responses": {
-          "200": {
-            "description": ""
-          }
-        },
-        "tags": [
-          "Album"
-        ],
-        "security": [
-          {
-            "bearer": []
-          },
-          {
-            "cookie": []
-          },
-          {
-            "api_key": []
-          }
-        ]
       }
     },
     "/api-key": {

server/libs/domain/src/album/album.repository.ts

@@ -20,4 +20,5 @@ export interface IAlbumRepository {
   getAll(): Promise<AlbumEntity[]>;
   create(album: Partial<AlbumEntity>): Promise<AlbumEntity>;
   update(album: Partial<AlbumEntity>): Promise<AlbumEntity>;
+  delete(album: AlbumEntity): Promise<void>;
 }

server/libs/domain/src/album/album.service.spec.ts

@@ -176,7 +176,22 @@ describe(AlbumService.name, () => {
       ).rejects.toBeInstanceOf(ForbiddenException);
     });
 
-    it('should all the owner to update the album', async () => {
+    it('should require a valid thumbnail asset id', async () => {
+      albumMock.getByIds.mockResolvedValue([albumStub.oneAsset]);
+      albumMock.update.mockResolvedValue(albumStub.oneAsset);
+      albumMock.hasAsset.mockResolvedValue(false);
+
+      await expect(
+        sut.update(authStub.admin, albumStub.oneAsset.id, {
+          albumThumbnailAssetId: 'not-in-album',
+        }),
+      ).rejects.toBeInstanceOf(BadRequestException);
+
+      expect(albumMock.hasAsset).toHaveBeenCalledWith(albumStub.oneAsset.id, 'not-in-album');
+      expect(albumMock.update).not.toHaveBeenCalled();
+    });
+
+    it('should allow the owner to update the album', async () => {
       albumMock.getByIds.mockResolvedValue([albumStub.oneAsset]);
       albumMock.update.mockResolvedValue(albumStub.oneAsset);
 
@@ -195,4 +210,33 @@ describe(AlbumService.name, () => {
       });
     });
   });
+
+  describe('delete', () => {
+    it('should throw an error for an album not found', async () => {
+      albumMock.getByIds.mockResolvedValue([]);
+
+      await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf(
+        BadRequestException,
+      );
+
+      expect(albumMock.delete).not.toHaveBeenCalled();
+    });
+
+    it('should not let a shared user delete the album', async () => {
+      albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
+
+      await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf(ForbiddenException);
+
+      expect(albumMock.delete).not.toHaveBeenCalled();
+    });
+
+    it('should let the owner delete an album', async () => {
+      albumMock.getByIds.mockResolvedValue([albumStub.empty]);
+
+      await sut.delete(authStub.admin, albumStub.empty.id);
+
+      expect(albumMock.delete).toHaveBeenCalledTimes(1);
+      expect(albumMock.delete).toHaveBeenCalledWith(albumStub.empty);
+    });
+  });
 });

server/libs/domain/src/album/album.service.ts

@@ -98,4 +98,18 @@ export class AlbumService {
 
     return mapAlbum(updatedAlbum);
   }
+
+  async delete(authUser: AuthUserDto, id: string): Promise<void> {
+    const [album] = await this.albumRepository.getByIds([id]);
+    if (!album) {
+      throw new BadRequestException('Album not found');
+    }
+
+    if (album.ownerId !== authUser.id) {
+      throw new ForbiddenException('Album not owned by user');
+    }
+
+    await this.albumRepository.delete(album);
+    await this.jobRepository.queue({ name: JobName.SEARCH_REMOVE_ALBUM, data: { ids: [id] } });
+  }
 }

server/libs/domain/test/album.repository.mock.ts

@@ -14,5 +14,6 @@ export const newAlbumRepositoryMock = (): jest.Mocked<IAlbumRepository> => {
     hasAsset: jest.fn(),
     create: jest.fn(),
     update: jest.fn(),
+    delete: jest.fn(),
   };
 };

server/libs/infra/src/entities/shared-link.entity.ts

@@ -53,7 +53,7 @@ export class SharedLinkEntity {
   assets!: AssetEntity[];
 
   @Index('IDX_sharedlink_albumId')
-  @ManyToOne(() => AlbumEntity, (album) => album.sharedLinks)
+  @ManyToOne(() => AlbumEntity, (album) => album.sharedLinks, { onDelete: 'CASCADE', onUpdate: 'CASCADE' })
   album?: AlbumEntity;
 }
 

server/libs/infra/src/migrations/1685044328272-AddSharedLinkCascade.ts

@@ -0,0 +1,16 @@
+import { MigrationInterface, QueryRunner } from "typeorm";
+
+export class AddSharedLinkCascade1685044328272 implements MigrationInterface {
+    name = 'AddSharedLinkCascade1685044328272'
+
+    public async up(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`ALTER TABLE "shared_links" DROP CONSTRAINT "FK_0c6ce9058c29f07cdf7014eac66"`);
+        await queryRunner.query(`ALTER TABLE "shared_links" ADD CONSTRAINT "FK_0c6ce9058c29f07cdf7014eac66" FOREIGN KEY ("albumId") REFERENCES "albums"("id") ON DELETE CASCADE ON UPDATE CASCADE`);
+    }
+
+    public async down(queryRunner: QueryRunner): Promise<void> {
+        await queryRunner.query(`ALTER TABLE "shared_links" DROP CONSTRAINT "FK_0c6ce9058c29f07cdf7014eac66"`);
+        await queryRunner.query(`ALTER TABLE "shared_links" ADD CONSTRAINT "FK_0c6ce9058c29f07cdf7014eac66" FOREIGN KEY ("albumId") REFERENCES "albums"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`);
+    }
+
+}

server/libs/infra/src/repositories/album.repository.ts

@@ -143,10 +143,14 @@ export class AlbumRepository implements IAlbumRepository {
     return this.save(album);
   }
 
-  async update(album: Partial<AlbumEntity>) {
+  async update(album: Partial<AlbumEntity>): Promise<AlbumEntity> {
     return this.save(album);
   }
 
+  async delete(album: AlbumEntity): Promise<void> {
+    await this.repository.remove(album);
+  }
+
   private async save(album: Partial<AlbumEntity>) {
     const { id } = await this.repository.save(album);
     return this.repository.findOneOrFail({ where: { id }, relations: { owner: true } });