From 28d8357cc50b01b01a1533baf903a8c5c7d2b383 Mon Sep 17 00:00:00 2001
From: Jason Rasmussen <jason@rasm.me>
Date: Fri, 16 May 2025 11:56:25 -0400
Subject: [PATCH] feat(web): clear person birthdate (#18330)
---
e2e/src/api/specs/person.e2e-spec.ts | 94 ----------
open-api/immich-openapi-specs.json | 2 +
.../src/controllers/person.controller.spec.ts | 172 ++++++++++++++++++
server/src/controllers/person.controller.ts | 4 +-
server/src/dtos/person.dto.ts | 5 +-
.../modals/PersonEditBirthDateModal.svelte | 13 +-
.../[[assetId=id]]/+page.svelte | 1 +
7 files changed, 190 insertions(+), 101 deletions(-)
create mode 100644 server/src/controllers/person.controller.spec.ts
diff --git a/e2e/src/api/specs/person.e2e-spec.ts b/e2e/src/api/specs/person.e2e-spec.ts
index 6e7eba74ba..1826002af6 100644
--- a/e2e/src/api/specs/person.e2e-spec.ts
+++ b/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`)
diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json
index 89bdfef45e..e7bf81ce3e 100644
--- a/open-api/immich-openapi-specs.json
+++ b/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": {
diff --git a/server/src/controllers/person.controller.spec.ts b/server/src/controllers/person.controller.spec.ts
new file mode 100644
index 0000000000..0366829336
--- /dev/null
+++ b/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();
+ });
+ });
+});
diff --git a/server/src/controllers/person.controller.ts b/server/src/controllers/person.controller.ts
index e98dd6a002..3440042eda 100644
--- a/server/src/controllers/person.controller.ts
+++ b/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 })
diff --git a/server/src/dtos/person.dto.ts b/server/src/dtos/person.dto.ts
index 90490715ef..c59ab905bd 100644
--- a/server/src/dtos/person.dto.ts
+++ b/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;
}
diff --git a/web/src/lib/modals/PersonEditBirthDateModal.svelte b/web/src/lib/modals/PersonEditBirthDateModal.svelte
index 52d23f4075..d79b716364 100644
--- a/web/src/lib/modals/PersonEditBirthDateModal.svelte
+++ b/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>
diff --git a/web/src/routes/(user)/people/[personId]/[[photos=photos]]/[[assetId=id]]/+page.svelte b/web/src/routes/(user)/people/[personId]/[[photos=photos]]/[[assetId=id]]/+page.svelte
index 50dc8f8166..1dc213729d 100644
--- a/web/src/routes/(user)/people/[personId]/[[photos=photos]]/[[assetId=id]]/+page.svelte
+++ b/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;