From f0b328fb6b52feb1a00e7d7fb59ebd67c28f00b3 Mon Sep 17 00:00:00 2001 From: martin <74269598+martabal@users.noreply.github.com> Date: Thu, 18 Jan 2024 02:52:11 +0100 Subject: [PATCH] feat(server, web): smart merge (#5796) * pr feedback * fix: tests * update assets statistics * pr feedback * pr feedback * fix: linter * pr feedback * fix: don't limit the smart merge * pr feedback * fix: server code --------- Co-authored-by: Jason Rasmussen --- .../src/domain/person/person.service.spec.ts | 37 ++++++- server/src/domain/person/person.service.ts | 16 ++- server/test/fixtures/person.stub.ts | 14 +++ .../admin-page/settings/setting-switch.svelte | 97 +++---------------- web/src/lib/components/elements/slider.svelte | 75 ++++++++++++++ .../faces-page/merge-face-selector.svelte | 7 +- web/src/routes/(user)/people/+page.svelte | 4 + .../(user)/people/[personId]/+page.svelte | 9 +- 8 files changed, 168 insertions(+), 91 deletions(-) create mode 100644 web/src/lib/components/elements/slider.svelte diff --git a/server/src/domain/person/person.service.spec.ts b/server/src/domain/person/person.service.spec.ts index ba576bc454..f4655c9a54 100644 --- a/server/src/domain/person/person.service.spec.ts +++ b/server/src/domain/person/person.service.spec.ts @@ -834,7 +834,7 @@ describe(PersonService.name, () => { expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); - it('should merge two people', async () => { + it('should merge two people without smart merge', async () => { personMock.getById.mockResolvedValueOnce(personStub.primaryPerson); personMock.getById.mockResolvedValueOnce(personStub.mergePerson); personMock.delete.mockResolvedValue(personStub.mergePerson); @@ -850,6 +850,8 @@ describe(PersonService.name, () => { oldPersonId: personStub.mergePerson.id, }); + expect(personMock.update).not.toHaveBeenCalled(); + expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.PERSON_DELETE, data: { id: personStub.mergePerson.id }, @@ -857,6 +859,35 @@ describe(PersonService.name, () => { expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); }); + it('should merge two people with smart merge', async () => { + personMock.getById.mockResolvedValueOnce(personStub.randomPerson); + personMock.getById.mockResolvedValueOnce(personStub.primaryPerson); + personMock.delete.mockResolvedValue(personStub.primaryPerson); + personMock.update.mockResolvedValue({ ...personStub.randomPerson, name: personStub.primaryPerson.name }); + accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-3'])); + accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-1'])); + + await expect(sut.mergePerson(authStub.admin, 'person-3', { ids: ['person-1'] })).resolves.toEqual([ + { id: 'person-1', success: true }, + ]); + + expect(personMock.reassignFaces).toHaveBeenCalledWith({ + newPersonId: personStub.randomPerson.id, + oldPersonId: personStub.primaryPerson.id, + }); + + expect(personMock.update).toHaveBeenCalledWith({ + id: personStub.randomPerson.id, + name: personStub.primaryPerson.name, + }); + + expect(jobMock.queue).toHaveBeenCalledWith({ + name: JobName.PERSON_DELETE, + data: { id: personStub.primaryPerson.id }, + }); + expect(accessMock.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); + }); + it('should throw an error when the primary person is not found', async () => { personMock.getById.mockResolvedValue(null); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); @@ -885,8 +916,8 @@ describe(PersonService.name, () => { }); it('should handle an error reassigning faces', async () => { - personMock.getById.mockResolvedValue(personStub.primaryPerson); - personMock.getById.mockResolvedValue(personStub.mergePerson); + personMock.getById.mockResolvedValueOnce(personStub.primaryPerson); + personMock.getById.mockResolvedValueOnce(personStub.mergePerson); personMock.reassignFaces.mockRejectedValue(new Error('update failed')); accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-1'])); accessMock.person.checkOwnerAccess.mockResolvedValueOnce(new Set(['person-2'])); diff --git a/server/src/domain/person/person.service.ts b/server/src/domain/person/person.service.ts index 928685da08..ad940c178b 100644 --- a/server/src/domain/person/person.service.ts +++ b/server/src/domain/person/person.service.ts @@ -460,7 +460,7 @@ export class PersonService { async mergePerson(auth: AuthDto, id: string, dto: MergePersonDto): Promise { const mergeIds = dto.ids; await this.access.requirePermission(auth, Permission.PERSON_WRITE, id); - const primaryPerson = await this.findOrFail(id); + let primaryPerson = await this.findOrFail(id); const primaryName = primaryPerson.name || primaryPerson.id; const results: BulkIdResponseDto[] = []; @@ -481,6 +481,19 @@ export class PersonService { continue; } + const update: Partial = {}; + if (!primaryPerson.name && mergePerson.name) { + update.name = mergePerson.name; + } + + if (!primaryPerson.birthDate && mergePerson.birthDate) { + update.birthDate = mergePerson.birthDate; + } + + if (Object.keys(update).length > 0) { + primaryPerson = await this.repository.update({ id: primaryPerson.id, ...update }); + } + const mergeName = mergePerson.name || mergePerson.id; const mergeData: UpdateFacesData = { oldPersonId: mergeId, newPersonId: id }; this.logger.log(`Merging ${mergeName} into ${primaryName}`); @@ -495,7 +508,6 @@ export class PersonService { results.push({ id: mergeId, success: false, error: BulkIdErrorReason.UNKNOWN }); } } - return results; } diff --git a/server/test/fixtures/person.stub.ts b/server/test/fixtures/person.stub.ts index 76e2d14f72..ad83d68001 100644 --- a/server/test/fixtures/person.stub.ts +++ b/server/test/fixtures/person.stub.ts @@ -128,4 +128,18 @@ export const personStub = { faceAsset: null, isHidden: false, }), + randomPerson: Object.freeze({ + id: 'person-3', + createdAt: new Date('2021-01-01'), + updatedAt: new Date('2021-01-01'), + ownerId: userStub.admin.id, + owner: userStub.admin, + name: '', + birthDate: null, + thumbnailPath: '/path/to/thumbnail', + faces: [], + faceAssetId: null, + faceAsset: null, + isHidden: false, + }), }; diff --git a/web/src/lib/components/admin-page/settings/setting-switch.svelte b/web/src/lib/components/admin-page/settings/setting-switch.svelte index 6797423a55..512de18669 100644 --- a/web/src/lib/components/admin-page/settings/setting-switch.svelte +++ b/web/src/lib/components/admin-page/settings/setting-switch.svelte @@ -2,6 +2,7 @@ import { quintOut } from 'svelte/easing'; import { fly } from 'svelte/transition'; import { createEventDispatcher } from 'svelte'; + import Slider from '$lib/components/elements/slider.svelte'; export let title: string; export let subtitle = ''; @@ -10,88 +11,22 @@ export let isEdited = false; const dispatch = createEventDispatcher<{ toggle: boolean }>(); - const onToggle = (event: Event) => dispatch('toggle', (event.target as HTMLInputElement).checked); -
-
-
- - {#if isEdited} -
- Unsaved change -
- {/if} -
- -

{subtitle}

+ dispatch('toggle', checked)}> +
+ + {#if isEdited} +
+ Unsaved change +
+ {/if}
- -
- - +

{subtitle}

+ diff --git a/web/src/lib/components/elements/slider.svelte b/web/src/lib/components/elements/slider.svelte new file mode 100644 index 0000000000..0b613ec010 --- /dev/null +++ b/web/src/lib/components/elements/slider.svelte @@ -0,0 +1,75 @@ + + +
+ + +
+ + diff --git a/web/src/lib/components/faces-page/merge-face-selector.svelte b/web/src/lib/components/faces-page/merge-face-selector.svelte index 56c1b86e7c..5dd48b04c6 100644 --- a/web/src/lib/components/faces-page/merge-face-selector.svelte +++ b/web/src/lib/components/faces-page/merge-face-selector.svelte @@ -26,7 +26,7 @@ let dispatch = createEventDispatcher<{ back: void; - merge: void; + merge: PersonResponseDto; }>(); $: hasSelection = selectedPeople.length > 0; @@ -68,16 +68,17 @@ const handleMerge = async () => { try { - const { data: results } = await api.personApi.mergePerson({ + let { data: results } = await api.personApi.mergePerson({ id: person.id, mergePersonDto: { ids: selectedPeople.map(({ id }) => id) }, }); + const { data: mergedPerson } = await api.personApi.getPerson({ id: person.id }); const count = results.filter(({ success }) => success).length; notificationController.show({ message: `Merged ${count} ${count === 1 ? 'person' : 'people'}`, type: NotificationType.Info, }); - dispatch('merge'); + dispatch('merge', mergedPerson); } catch (error) { handleError(error, 'Cannot merge people'); } finally { diff --git a/web/src/routes/(user)/people/+page.svelte b/web/src/routes/(user)/people/+page.svelte index eec418c6ad..323b160a82 100644 --- a/web/src/routes/(user)/people/+page.svelte +++ b/web/src/routes/(user)/people/+page.svelte @@ -165,8 +165,12 @@ id: personMerge2.id, mergePersonDto: { ids: [personToMerge.id] }, }); + + const { data: mergedPerson } = await api.personApi.getPerson({ id: personToMerge.id }); + countVisiblePeople--; people = people.filter((person: PersonResponseDto) => person.id !== personToMerge.id); + people = people.map((person: PersonResponseDto) => (person.id === personMerge2.id ? mergedPerson : person)); notificationController.show({ message: 'Merge people succesfully', diff --git a/web/src/routes/(user)/people/[personId]/+page.svelte b/web/src/routes/(user)/people/[personId]/+page.svelte index c97d3845cb..6d7bd77865 100644 --- a/web/src/routes/(user)/people/[personId]/+page.svelte +++ b/web/src/routes/(user)/people/[personId]/+page.svelte @@ -185,8 +185,13 @@ } }; - const handleMerge = () => { + const handleMerge = async (person: PersonResponseDto) => { + const { data: statistics } = await api.personApi.getPersonStatistics({ id: person.id }); + numberOfAssets = statistics.assets; handleGoBack(); + + data.person = person; + refreshAssetGrid = !refreshAssetGrid; }; @@ -374,7 +379,7 @@ {/if} {#if viewMode === ViewMode.MERGE_PEOPLE} - + handleMerge(detail)} /> {/if}