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 <jrasm91@gmail.com>
2024-11-24 08:52:28 +02:00 · 2024-01-18 02:52:11 +01:00 · 2024-01-18 02:52:11 +01:00 · f0b328fb6b
commit f0b328fb6b
parent c55503496f
8 changed files with 168 additions and 91 deletions
--- 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']));
--- 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<BulkIdResponseDto[]> {
    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<PersonEntity> = {};
+        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;
  }

--- 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<PersonEntity>({
+    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,
+  }),
 };
--- 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);
 </script>

-<div class="flex place-items-center justify-between">
-  <div>
-    <div class="flex h-[26px] place-items-center gap-1">
-      <label class="font-medium text-immich-primary dark:text-immich-dark-primary text-sm" for={title}>
-        {title}
-      </label>
-      {#if isEdited}
-        <div
-          transition:fly={{ x: 10, duration: 200, easing: quintOut }}
-          class="rounded-full bg-orange-100 px-2 text-[10px] text-orange-900"
-        >
-          Unsaved change
-        </div>
-      {/if}
-    </div>
-
-    <p class="text-sm dark:text-immich-dark-fg">{subtitle}</p>
+<Slider bind:checked {disabled} on:toggle={() => dispatch('toggle', checked)}>
+  <div class="flex h-[26px] place-items-center gap-1">
+    <label class="font-medium text-immich-primary dark:text-immich-dark-primary text-sm" for={title}>
+      {title}
+    </label>
+    {#if isEdited}
+      <div
+        transition:fly={{ x: 10, duration: 200, easing: quintOut }}
+        class="rounded-full bg-orange-100 px-2 text-[10px] text-orange-900"
+      >
+        Unsaved change
+      </div>
+    {/if}
  </div>

-  <label class="relative inline-block h-[10px] w-[36px] flex-none">
-    <input
-      class="disabled::cursor-not-allowed h-0 w-0 opacity-0"
-      type="checkbox"
-      bind:checked
-      on:click={onToggle}
-      {disabled}
-    />
-
-    {#if disabled}
-      <span class="slider slider-disabled cursor-not-allowed" />
-    {:else}
-      <span class="slider slider-enabled cursor-pointer" />
-    {/if}
-  </label>
-</div>
-
-<style>
-  .slider {
-    position: absolute;
-    top: 0;
-    left: 0;
-    right: 0;
-    bottom: 0;
-    background-color: #ccc;
-    -webkit-transition: 0.4s;
-    transition: 0.4s;
-    border-radius: 34px;
-  }
-
-  input:disabled {
-    cursor: not-allowed;
-  }
-
-  .slider:before {
-    position: absolute;
-    content: '';
-    height: 20px;
-    width: 20px;
-    left: 0px;
-    right: 0px;
-    bottom: -4px;
-    background-color: gray;
-    -webkit-transition: 0.4s;
-    transition: 0.4s;
-    border-radius: 50%;
-  }
-
-  input:checked + .slider:before {
-    -webkit-transform: translateX(18px);
-    -ms-transform: translateX(18px);
-    transform: translateX(18px);
-    background-color: #4250af;
-  }
-
-  input:checked + .slider-disabled {
-    background-color: gray;
-  }
-
-  input:checked + .slider-enabled {
-    background-color: #adcbfa;
-  }
-</style>
+  <p class="text-sm dark:text-immich-dark-fg">{subtitle}</p>
+</Slider>
--- a/web/src/lib/components/elements/slider.svelte
+++ b/web/src/lib/components/elements/slider.svelte
@ -0,0 +1,75 @@
+<script lang="ts">
+  import { createEventDispatcher } from 'svelte';
+
+  export let checked = false;
+  export let disabled = false;
+
+  const dispatch = createEventDispatcher<{ toggle: boolean }>();
+  const onToggle = (event: Event) => dispatch('toggle', (event.target as HTMLInputElement).checked);
+</script>
+
+<div class="flex place-items-center justify-between">
+  <slot name="leading" />
+  <label class="relative inline-block h-[10px] w-[36px] flex-none">
+    <input
+      class="disabled::cursor-not-allowed h-0 w-0 opacity-0"
+      type="checkbox"
+      bind:checked
+      on:click={onToggle}
+      {disabled}
+    />
+
+    {#if disabled}
+      <span class="slider slider-disabled cursor-not-allowed" />
+    {:else}
+      <span class="slider slider-enabled cursor-pointer" />
+    {/if}
+  </label>
+</div>
+
+<style>
+  .slider {
+    position: absolute;
+    top: 0;
+    left: 0;
+    right: 0;
+    bottom: 0;
+    background-color: #ccc;
+    -webkit-transition: 0.4s;
+    transition: 0.4s;
+    border-radius: 34px;
+  }
+
+  input:disabled {
+    cursor: not-allowed;
+  }
+
+  .slider:before {
+    position: absolute;
+    content: '';
+    height: 20px;
+    width: 20px;
+    left: 0px;
+    right: 0px;
+    bottom: -4px;
+    background-color: gray;
+    -webkit-transition: 0.4s;
+    transition: 0.4s;
+    border-radius: 50%;
+  }
+
+  input:checked + .slider:before {
+    -webkit-transform: translateX(18px);
+    -ms-transform: translateX(18px);
+    transform: translateX(18px);
+    background-color: #4250af;
+  }
+
+  input:checked + .slider-disabled {
+    background-color: gray;
+  }
+
+  input:checked + .slider-enabled {
+    background-color: #adcbfa;
+  }
+</style>
--- 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 {
--- 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',
--- 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}
-  <MergeFaceSelector person={data.person} on:back={handleGoBack} on:merge={handleMerge} />
+  <MergeFaceSelector person={data.person} on:back={handleGoBack} on:merge={({ detail }) => handleMerge(detail)} />
 {/if}

 <header>