feat(web): clear person birthdate (#18330)

e2e/src/api/specs/person.e2e-spec.ts

@@ -5,22 +5,6 @@ import { app, asBearerAuth, utils } from 'src/utils';
 import request from 'supertest';
 import { beforeAll, beforeEach, describe, expect, it } from 'vitest';
 
-const invalidBirthday = [
-  {
-    birthDate: 'false',
-    response: ['birthDate must be a string in the format yyyy-MM-dd', 'Birth date cannot be in the future'],
-  },
-  {
-    birthDate: '123567',
-    response: ['birthDate must be a string in the format yyyy-MM-dd', 'Birth date cannot be in the future'],
-  },
-  {
-    birthDate: 123_567,
-    response: ['birthDate must be a string in the format yyyy-MM-dd', 'Birth date cannot be in the future'],
-  },
-  { birthDate: '9999-01-01', response: ['Birth date cannot be in the future'] },
-];
-
 describe('/people', () => {
   let admin: LoginResponseDto;
   let visiblePerson: PersonResponseDto;
@@ -58,14 +42,6 @@ describe('/people', () => {
 
   describe('GET /people', () => {
     beforeEach(async () => {});
-
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).get('/people');
-
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
     it('should return all people (including hidden)', async () => {
       const { status, body } = await request(app)
         .get('/people')
@@ -117,13 +93,6 @@ describe('/people', () => {
   });
 
   describe('GET /people/:id', () => {
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).get(`/people/${uuidDto.notFound}`);
-
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
     it('should throw error if person with id does not exist', async () => {
       const { status, body } = await request(app)
         .get(`/people/${uuidDto.notFound}`)
@@ -144,13 +113,6 @@ describe('/people', () => {
   });
 
   describe('GET /people/:id/statistics', () => {
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).get(`/people/${multipleAssetsPerson.id}/statistics`);
-
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
     it('should throw error if person with id does not exist', async () => {
       const { status, body } = await request(app)
         .get(`/people/${uuidDto.notFound}/statistics`)
@@ -171,23 +133,6 @@ describe('/people', () => {
   });
 
   describe('POST /people', () => {
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).post(`/people`);
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
-    for (const { birthDate, response } of invalidBirthday) {
-      it(`should not accept an invalid birth date [${birthDate}]`, async () => {
-        const { status, body } = await request(app)
-          .post(`/people`)
-          .set('Authorization', `Bearer ${admin.accessToken}`)
-          .send({ birthDate });
-        expect(status).toBe(400);
-        expect(body).toEqual(errorDto.badRequest(response));
-      });
-    }
-
     it('should create a person', async () => {
       const { status, body } = await request(app)
         .post(`/people`)
@@ -223,39 +168,6 @@ describe('/people', () => {
   });
 
   describe('PUT /people/:id', () => {
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).put(`/people/${uuidDto.notFound}`);
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
-    for (const { key, type } of [
-      { key: 'name', type: 'string' },
-      { key: 'featureFaceAssetId', type: 'string' },
-      { key: 'isHidden', type: 'boolean value' },
-      { key: 'isFavorite', type: 'boolean value' },
-    ]) {
-      it(`should not allow null ${key}`, async () => {
-        const { status, body } = await request(app)
-          .put(`/people/${visiblePerson.id}`)
-          .set('Authorization', `Bearer ${admin.accessToken}`)
-          .send({ [key]: null });
-        expect(status).toBe(400);
-        expect(body).toEqual(errorDto.badRequest([`${key} must be a ${type}`]));
-      });
-    }
-
-    for (const { birthDate, response } of invalidBirthday) {
-      it(`should not accept an invalid birth date [${birthDate}]`, async () => {
-        const { status, body } = await request(app)
-          .put(`/people/${visiblePerson.id}`)
-          .set('Authorization', `Bearer ${admin.accessToken}`)
-          .send({ birthDate });
-        expect(status).toBe(400);
-        expect(body).toEqual(errorDto.badRequest(response));
-      });
-    }
-
     it('should update a date of birth', async () => {
       const { status, body } = await request(app)
         .put(`/people/${visiblePerson.id}`)
@@ -312,12 +224,6 @@ describe('/people', () => {
   });
 
   describe('POST /people/:id/merge', () => {
-    it('should require authentication', async () => {
-      const { status, body } = await request(app).post(`/people/${uuidDto.notFound}/merge`);
-      expect(status).toBe(401);
-      expect(body).toEqual(errorDto.unauthorized);
-    });
-
     it('should not supporting merging a person into themselves', async () => {
       const { status, body } = await request(app)
         .post(`/people/${visiblePerson.id}/merge`)

open-api/immich-openapi-specs.json

@@ -11075,6 +11075,7 @@
           },
           "featureFaceAssetId": {
             "description": "Asset is used to get the feature face thumbnail.",
+            "format": "uuid",
             "type": "string"
           },
           "id": {
@@ -11280,6 +11281,7 @@
           },
           "featureFaceAssetId": {
             "description": "Asset is used to get the feature face thumbnail.",
+            "format": "uuid",
             "type": "string"
           },
           "isFavorite": {

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

@@ -0,0 +1,172 @@
+import { PersonController } from 'src/controllers/person.controller';
+import { LoggingRepository } from 'src/repositories/logging.repository';
+import { PersonService } from 'src/services/person.service';
+import request from 'supertest';
+import { errorDto } from 'test/medium/responses';
+import { factory } from 'test/small.factory';
+import { automock, ControllerContext, controllerSetup, mockBaseService } from 'test/utils';
+
+describe(PersonController.name, () => {
+  let ctx: ControllerContext;
+  const service = mockBaseService(PersonService);
+
+  beforeAll(async () => {
+    ctx = await controllerSetup(PersonController, [
+      { provide: PersonService, useValue: service },
+      { provide: LoggingRepository, useValue: automock(LoggingRepository, { strict: false }) },
+    ]);
+    return () => ctx.close();
+  });
+
+  beforeEach(() => {
+    service.resetAllMocks();
+    ctx.reset();
+  });
+
+  describe('GET /people', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).get('/people');
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+
+    it(`should require closestPersonId to be a uuid`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .get(`/people`)
+        .query({ closestPersonId: 'invalid' })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest([expect.stringContaining('must be a UUID')]));
+    });
+
+    it(`should require closestAssetId to be a uuid`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .get(`/people`)
+        .query({ closestAssetId: 'invalid' })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest([expect.stringContaining('must be a UUID')]));
+    });
+  });
+
+  describe('POST /people', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).post('/people');
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+
+    it('should map an empty birthDate to null', async () => {
+      await request(ctx.getHttpServer()).post('/people').send({ birthDate: '' });
+      expect(service.create).toHaveBeenCalledWith(undefined, { birthDate: null });
+    });
+  });
+
+  describe('GET /people/:id', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).get(`/people/${factory.uuid()}`);
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+  });
+
+  describe('PUT /people/:id', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).get(`/people/${factory.uuid()}`);
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+
+    it('should require a valid uuid', async () => {
+      const { status, body } = await request(ctx.getHttpServer()).put(`/people/123`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest([expect.stringContaining('id must be a UUID')]));
+    });
+
+    it(`should not allow a null name`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .post(`/people`)
+        .send({ name: null })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest(['name must be a string']));
+    });
+
+    it(`should require featureFaceAssetId to be a uuid`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ featureFaceAssetId: 'invalid' })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest(['featureFaceAssetId must be a UUID']));
+    });
+
+    it(`should require isFavorite to be a boolean`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ isFavorite: 'invalid' })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest(['isFavorite must be a boolean value']));
+    });
+
+    it(`should require isHidden to be a boolean`, async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ isHidden: 'invalid' })
+        .set('Authorization', `Bearer token`);
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest(['isHidden must be a boolean value']));
+    });
+
+    it('should map an empty birthDate to null', async () => {
+      const id = factory.uuid();
+      await request(ctx.getHttpServer()).put(`/people/${id}`).send({ birthDate: '' });
+      expect(service.update).toHaveBeenCalledWith(undefined, id, { birthDate: null });
+    });
+
+    it('should not accept an invalid birth date (false)', async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ birthDate: false });
+      expect(status).toBe(400);
+      expect(body).toEqual(
+        errorDto.badRequest([
+          'birthDate must be a string in the format yyyy-MM-dd',
+          'Birth date cannot be in the future',
+        ]),
+      );
+    });
+
+    it('should not accept an invalid birth date (number)', async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ birthDate: 123_456 });
+      expect(status).toBe(400);
+      expect(body).toEqual(
+        errorDto.badRequest([
+          'birthDate must be a string in the format yyyy-MM-dd',
+          'Birth date cannot be in the future',
+        ]),
+      );
+    });
+
+    it('should not accept a birth date in the future)', async () => {
+      const { status, body } = await request(ctx.getHttpServer())
+        .put(`/people/${factory.uuid()}`)
+        .send({ birthDate: '9999-01-01' });
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest(['Birth date cannot be in the future']));
+    });
+  });
+
+  describe('POST /people/:id/merge', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).post(`/people/${factory.uuid()}/merge`);
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+  });
+
+  describe('GET /people/:id/statistics', () => {
+    it('should be an authenticated route', async () => {
+      await request(ctx.getHttpServer()).get(`/people/${factory.uuid()}/statistics`);
+      expect(ctx.authenticate).toHaveBeenCalled();
+    });
+  });
+});

server/src/controllers/person.controller.ts

@@ -27,7 +27,9 @@ export class PersonController {
   constructor(
     private service: PersonService,
     private logger: LoggingRepository,
-  ) {}
+  ) {
+    this.logger.setContext(PersonController.name);
+  }
 
   @Get()
   @Authenticated({ permission: Permission.PERSON_READ })

server/src/dtos/person.dto.ts

@@ -33,7 +33,7 @@ export class PersonCreateDto {
   @ApiProperty({ format: 'date' })
   @MaxDateString(() => DateTime.now(), { message: 'Birth date cannot be in the future' })
   @IsDateStringFormat('yyyy-MM-dd')
-  @Optional({ nullable: true })
+  @Optional({ nullable: true, emptyToNull: true })
   birthDate?: Date | null;
 
   /**
@@ -54,8 +54,7 @@ export class PersonUpdateDto extends PersonCreateDto {
   /**
    * Asset is used to get the feature face thumbnail.
    */
-  @Optional()
-  @IsString()
+  @ValidateUUID({ optional: true })
   featureFaceAssetId?: string;
 }
 

web/src/lib/modals/PersonEditBirthDateModal.svelte

@@ -24,7 +24,7 @@
     try {
       const updatedPerson = await updatePerson({
         id: person.id,
-        personUpdateDto: { birthDate: birthDate.length > 0 ? birthDate : null },
+        personUpdateDto: { birthDate },
       });
 
       notificationController.show({ message: $t('date_of_birth_saved'), type: NotificationType.Info });
@@ -53,6 +53,13 @@
           bind:value={birthDate}
           max={todayFormatted}
         />
+        {#if person.birthDate}
+          <div class="flex justify-end">
+            <Button shape="round" color="secondary" size="small" onclick={() => (birthDate = '')}>
+              {$t('clear')}
+            </Button>
+          </div>
+        {/if}
       </div>
     </form>
   </ModalBody>
@@ -62,8 +69,8 @@
       <Button shape="round" color="secondary" fullWidth onclick={() => onClose()}>
         {$t('cancel')}
       </Button>
-      <Button type="submit" shape="round" color="primary" fullWidth>
-        {$t('set')}
+      <Button type="submit" shape="round" color="primary" fullWidth form="set-birth-date-form">
+        {$t('save')}
       </Button>
     </div>
   </ModalFooter>

web/src/routes/(user)/people/[personId]/[[photos=photos]]/[[assetId=id]]/+page.svelte

@@ -328,6 +328,7 @@
       return;
     }
 
+    person = updatedPerson;
     people = people.map((person: PersonResponseDto) => {
       if (person.id === updatedPerson.id) {
         return updatedPerson;