feat(server): create a person with optional values (#7706)

* feat: create person dto

* chore: open api

* fix: e2e

* fix: web usage

server/src/domain/person/person.dto.ts

@@ -1,11 +1,11 @@
 import { AssetFaceEntity, PersonEntity } from '@app/infra/entities';
 import { ApiProperty } from '@nestjs/swagger';
 import { Transform, Type } from 'class-transformer';
-import { IsArray, IsBoolean, IsDate, IsNotEmpty, IsString, ValidateNested } from 'class-validator';
+import { IsArray, IsBoolean, IsDate, IsNotEmpty, IsString, MaxDate, ValidateNested } from 'class-validator';
 import { AuthDto } from '../auth';
 import { Optional, ValidateUUID, toBoolean } from '../domain.util';
 
-export class PersonUpdateDto {
+export class PersonCreateDto {
   /**
    * Person name.
    */
@@ -20,16 +20,10 @@ export class PersonUpdateDto {
   @Optional({ nullable: true })
   @IsDate()
   @Type(() => Date)
+  @MaxDate(() => new Date(), { message: 'Birth date cannot be in the future' })
   @ApiProperty({ format: 'date' })
   birthDate?: Date | null;
 
-  /**
-   * Asset is used to get the feature face thumbnail.
-   */
-  @Optional()
-  @IsString()
-  featureFaceAssetId?: string;
-
   /**
    * Person visibility
    */
@@ -38,6 +32,15 @@ export class PersonUpdateDto {
   isHidden?: boolean;
 }
 
+export class PersonUpdateDto extends PersonCreateDto {
+  /**
+   * Asset is used to get the feature face thumbnail.
+   */
+  @Optional()
+  @IsString()
+  featureFaceAssetId?: string;
+}
+
 export class PeopleUpdateDto {
   @IsArray()
   @ValidateNested({ each: true })

server/src/domain/person/person.service.spec.ts

@@ -227,8 +227,7 @@ describe(PersonService.name, () => {
     });
 
     it('should throw an error when personId is invalid', async () => {
-      personMock.getById.mockResolvedValue(null);
-      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
+      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set());
       await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).rejects.toBeInstanceOf(
         BadRequestException,
       );
@@ -237,20 +236,17 @@ describe(PersonService.name, () => {
     });
 
     it("should update a person's name", async () => {
-      personMock.getById.mockResolvedValue(personStub.noName);
       personMock.update.mockResolvedValue(personStub.withName);
       personMock.getAssets.mockResolvedValue([assetStub.image]);
       accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
 
       await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).resolves.toEqual(responseDto);
 
-      expect(personMock.getById).toHaveBeenCalledWith('person-1');
       expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', name: 'Person 1' });
       expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1']));
     });
 
     it("should update a person's date of birth", async () => {
-      personMock.getById.mockResolvedValue(personStub.noBirthDate);
       personMock.update.mockResolvedValue(personStub.withBirthDate);
       personMock.getAssets.mockResolvedValue([assetStub.image]);
       accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
@@ -262,71 +258,24 @@ describe(PersonService.name, () => {
         thumbnailPath: '/path/to/thumbnail.jpg',
         isHidden: false,
       });
-      expect(personMock.getById).toHaveBeenCalledWith('person-1');
       expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', birthDate: new Date('1976-06-30') });
       expect(jobMock.queue).not.toHaveBeenCalled();
       expect(jobMock.queueAll).not.toHaveBeenCalled();
       expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1']));
     });
 
-    it('should throw BadRequestException if birthDate is in the future', async () => {
-      personMock.getById.mockResolvedValue(personStub.noBirthDate);
-      personMock.update.mockResolvedValue(personStub.withBirthDate);
-      personMock.getAssets.mockResolvedValue([assetStub.image]);
-      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
-
-      const futureDate = new Date();
-      futureDate.setMinutes(futureDate.getMinutes() + 1); // Set birthDate to one minute in the future
-
-      await expect(sut.update(authStub.admin, 'person-1', { birthDate: futureDate })).rejects.toThrow(
-        new BadRequestException('Date of birth cannot be in the future'),
-      );
-    });
-
-    it('should not throw an error if birthdate is in the past', async () => {
-      const pastDate = new Date();
-      pastDate.setMinutes(pastDate.getMinutes() - 1); // Set birthDate to one minute in the past
-
-      personMock.getById.mockResolvedValue(personStub.noBirthDate);
-      personMock.update.mockResolvedValue({ ...personStub.withBirthDate, birthDate: pastDate });
-      personMock.getAssets.mockResolvedValue([assetStub.image]);
-      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
-
-      const result = await sut.update(authStub.admin, 'person-1', { birthDate: pastDate });
-
-      expect(result.birthDate).toEqual(pastDate);
-      expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', birthDate: pastDate });
-    });
-
-    it('should not throw an error if birthdate is today', async () => {
-      const today = new Date(); // Set birthDate to now()
-
-      personMock.getById.mockResolvedValue(personStub.noBirthDate);
-      personMock.update.mockResolvedValue({ ...personStub.withBirthDate, birthDate: today });
-      personMock.getAssets.mockResolvedValue([assetStub.image]);
-      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
-
-      const result = await sut.update(authStub.admin, 'person-1', { birthDate: today });
-
-      expect(result.birthDate).toEqual(today);
-      expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', birthDate: today });
-    });
-
     it('should update a person visibility', async () => {
-      personMock.getById.mockResolvedValue(personStub.hidden);
       personMock.update.mockResolvedValue(personStub.withName);
       personMock.getAssets.mockResolvedValue([assetStub.image]);
       accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
 
       await expect(sut.update(authStub.admin, 'person-1', { isHidden: false })).resolves.toEqual(responseDto);
 
-      expect(personMock.getById).toHaveBeenCalledWith('person-1');
       expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', isHidden: false });
       expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1']));
     });
 
     it("should update a person's thumbnailPath", async () => {
-      personMock.getById.mockResolvedValue(personStub.withName);
       personMock.update.mockResolvedValue(personStub.withName);
       personMock.getFacesByIds.mockResolvedValue([faceStub.face1]);
       accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id]));
@@ -336,7 +285,6 @@ describe(PersonService.name, () => {
         sut.update(authStub.admin, 'person-1', { featureFaceAssetId: faceStub.face1.assetId }),
       ).resolves.toEqual(responseDto);
 
-      expect(personMock.getById).toHaveBeenCalledWith('person-1');
       expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', faceAssetId: faceStub.face1.id });
       expect(personMock.getFacesByIds).toHaveBeenCalledWith([
         {
@@ -362,12 +310,11 @@ describe(PersonService.name, () => {
 
   describe('updateAll', () => {
     it('should throw an error when personId is invalid', async () => {
-      personMock.getById.mockResolvedValue(null);
-      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1']));
+      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set());
 
-      await expect(
-        sut.updatePeople(authStub.admin, { people: [{ id: 'person-1', name: 'Person 1' }] }),
-      ).resolves.toEqual([{ error: BulkIdErrorReason.UNKNOWN, id: 'person-1', success: false }]);
+      await expect(sut.updateAll(authStub.admin, { people: [{ id: 'person-1', name: 'Person 1' }] })).resolves.toEqual([
+        { error: BulkIdErrorReason.UNKNOWN, id: 'person-1', success: false },
+      ]);
       expect(personMock.update).not.toHaveBeenCalled();
       expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1']));
     });
@@ -488,10 +435,10 @@ describe(PersonService.name, () => {
   describe('createPerson', () => {
     it('should create a new person', async () => {
       personMock.create.mockResolvedValue(personStub.primaryPerson);
-      personMock.getFaceById.mockResolvedValue(faceStub.face1);
-      accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
 
-      await expect(sut.createPerson(authStub.admin)).resolves.toBe(personStub.primaryPerson);
+      await expect(sut.create(authStub.admin, {})).resolves.toBe(personStub.primaryPerson);
+
+      expect(personMock.create).toHaveBeenCalledWith({ ownerId: authStub.admin.user.id });
     });
   });
 

server/src/domain/person/person.service.ts

@@ -36,6 +36,7 @@ import {
   MergePersonDto,
   PeopleResponseDto,
   PeopleUpdateDto,
+  PersonCreateDto,
   PersonResponseDto,
   PersonSearchDto,
   PersonStatisticsResponseDto,
@@ -91,10 +92,6 @@ export class PersonService {
     };
   }
 
-  createPerson(auth: AuthDto): Promise<PersonResponseDto> {
-    return this.repository.create({ ownerId: auth.user.id });
-  }
-
   async reassignFaces(auth: AuthDto, personId: string, dto: AssetFaceUpdateDto): Promise<PersonResponseDto[]> {
     await this.access.requirePermission(auth, Permission.PERSON_WRITE, personId);
     const person = await this.findOrFail(personId);
@@ -199,21 +196,21 @@ export class PersonService {
     return assets.map((asset) => mapAsset(asset));
   }
 
+  create(auth: AuthDto, dto: PersonCreateDto): Promise<PersonResponseDto> {
+    return this.repository.create({
+      ownerId: auth.user.id,
+      name: dto.name,
+      birthDate: dto.birthDate,
+      isHidden: dto.isHidden,
+    });
+  }
+
   async update(auth: AuthDto, id: string, dto: PersonUpdateDto): Promise<PersonResponseDto> {
     await this.access.requirePermission(auth, Permission.PERSON_WRITE, id);
-    let person = await this.findOrFail(id);
 
     const { name, birthDate, isHidden, featureFaceAssetId: assetId } = dto;
-
-    // Check if the birthDate is in the future
-    if (birthDate && new Date(birthDate) > new Date()) {
-      throw new BadRequestException('Date of birth cannot be in the future');
-    }
-
-    if (name !== undefined || birthDate !== undefined || isHidden !== undefined) {
-      person = await this.repository.update({ id, name, birthDate, isHidden });
-    }
-
+    // TODO: set by faceId directly
+    let faceId: string | undefined = undefined;
     if (assetId) {
       await this.access.requirePermission(auth, Permission.ASSET_READ, assetId);
       const [face] = await this.repository.getFacesByIds([{ personId: id, assetId }]);
@@ -221,14 +218,19 @@ export class PersonService {
         throw new BadRequestException('Invalid assetId for feature face');
       }
 
-      person = await this.repository.update({ id, faceAssetId: face.id });
+      faceId = face.id;
+    }
+
+    const person = await this.repository.update({ id, faceAssetId: faceId, name, birthDate, isHidden });
+
+    if (assetId) {
       await this.jobRepository.queue({ name: JobName.GENERATE_PERSON_THUMBNAIL, data: { id } });
     }
 
     return mapPerson(person);
   }
 
-  async updatePeople(auth: AuthDto, dto: PeopleUpdateDto): Promise<BulkIdResponseDto[]> {
+  async updateAll(auth: AuthDto, dto: PeopleUpdateDto): Promise<BulkIdResponseDto[]> {
     const results: BulkIdResponseDto[] = [];
     for (const person of dto.people) {
       try {

server/src/immich/controllers/person.controller.ts

@@ -6,6 +6,7 @@ import {
   MergePersonDto,
   PeopleResponseDto,
   PeopleUpdateDto,
+  PersonCreateDto,
   PersonResponseDto,
   PersonSearchDto,
   PersonService,
@@ -32,22 +33,13 @@ export class PersonController {
   }
 
   @Post()
-  createPerson(@Auth() auth: AuthDto): Promise<PersonResponseDto> {
-    return this.service.createPerson(auth);
-  }
-
-  @Put(':id/reassign')
-  reassignFaces(
-    @Auth() auth: AuthDto,
-    @Param() { id }: UUIDParamDto,
-    @Body() dto: AssetFaceUpdateDto,
-  ): Promise<PersonResponseDto[]> {
-    return this.service.reassignFaces(auth, id, dto);
+  createPerson(@Auth() auth: AuthDto, @Body() dto: PersonCreateDto): Promise<PersonResponseDto> {
+    return this.service.create(auth, dto);
   }
 
   @Put()
   updatePeople(@Auth() auth: AuthDto, @Body() dto: PeopleUpdateDto): Promise<BulkIdResponseDto[]> {
-    return this.service.updatePeople(auth, dto);
+    return this.service.updateAll(auth, dto);
   }
 
   @Get(':id')
@@ -85,6 +77,15 @@ export class PersonController {
     return this.service.getAssets(auth, id);
   }
 
+  @Put(':id/reassign')
+  reassignFaces(
+    @Auth() auth: AuthDto,
+    @Param() { id }: UUIDParamDto,
+    @Body() dto: AssetFaceUpdateDto,
+  ): Promise<PersonResponseDto[]> {
+    return this.service.reassignFaces(auth, id, dto);
+  }
+
   @Post(':id/merge')
   mergePerson(
     @Auth() auth: AuthDto,