pr feedback

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

@@ -214,7 +214,7 @@ describe('/asset', () => {
         id: user1Assets[0].id,
         isFavorite: false,
         people: {
-          faces: [
+          visiblePeople: [
             {
               birthDate: null,
               id: expect.any(String),
@@ -484,7 +484,7 @@ describe('/asset', () => {
         id: user1Assets[0].id,
         isFavorite: true,
         people: {
-          faces: [
+          visiblePeople: [
             {
               birthDate: null,
               id: expect.any(String),

mobile/lib/services/asset.service.dart

@@ -72,7 +72,7 @@ class AssetService {
       final AssetResponseDto? dto =
           await _apiService.assetApi.getAssetInfo(remoteId);
 
-      return dto?.people?.faces;
+      return dto?.people?.visiblePeople;
     } catch (error, stack) {
       log.severe(
         'Error while getting remote asset info: ${error.toString()}',

mobile/openapi/doc/PeopleWithFacesResponseDto.md

@@ -8,8 +8,8 @@ import 'package:openapi/api.dart';
 ## Properties
 Name | Type | Description | Notes
 ------------ | ------------- | ------------- | -------------
-**faces** | [**List<PersonWithFacesResponseDto>**](PersonWithFacesResponseDto.md) |  | [default to const []]
 **numberOfFaces** | **int** |  | 
+**visiblePeople** | [**List<PersonWithFacesResponseDto>**](PersonWithFacesResponseDto.md) |  | [default to const []]
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)
 

mobile/openapi/lib/model/people_with_faces_response_dto.dart

@@ -13,32 +13,32 @@ part of openapi.api;
 class PeopleWithFacesResponseDto {
   /// Returns a new [PeopleWithFacesResponseDto] instance.
   PeopleWithFacesResponseDto({
-    this.faces = const [],
     required this.numberOfFaces,
+    this.visiblePeople = const [],
   });
 
-  List<PersonWithFacesResponseDto> faces;
-
   int numberOfFaces;
 
+  List<PersonWithFacesResponseDto> visiblePeople;
+
   @override
   bool operator ==(Object other) => identical(this, other) || other is PeopleWithFacesResponseDto &&
-    _deepEquality.equals(other.faces, faces) &&
-    other.numberOfFaces == numberOfFaces;
+    other.numberOfFaces == numberOfFaces &&
+    _deepEquality.equals(other.visiblePeople, visiblePeople);
 
   @override
   int get hashCode =>
     // ignore: unnecessary_parenthesis
-    (faces.hashCode) +
-    (numberOfFaces.hashCode);
+    (numberOfFaces.hashCode) +
+    (visiblePeople.hashCode);
 
   @override
-  String toString() => 'PeopleWithFacesResponseDto[faces=$faces, numberOfFaces=$numberOfFaces]';
+  String toString() => 'PeopleWithFacesResponseDto[numberOfFaces=$numberOfFaces, visiblePeople=$visiblePeople]';
 
   Map<String, dynamic> toJson() {
     final json = <String, dynamic>{};
-      json[r'faces'] = this.faces;
       json[r'numberOfFaces'] = this.numberOfFaces;
+      json[r'visiblePeople'] = this.visiblePeople;
     return json;
   }
 
@@ -50,8 +50,8 @@ class PeopleWithFacesResponseDto {
       final json = value.cast<String, dynamic>();
 
       return PeopleWithFacesResponseDto(
-        faces: PersonWithFacesResponseDto.listFromJson(json[r'faces']),
         numberOfFaces: mapValueOfType<int>(json, r'numberOfFaces')!,
+        visiblePeople: PersonWithFacesResponseDto.listFromJson(json[r'visiblePeople']),
       );
     }
     return null;
@@ -99,8 +99,8 @@ class PeopleWithFacesResponseDto {
 
   /// The list of required keys that must be present in a JSON.
   static const requiredKeys = <String>{
-    'faces',
     'numberOfFaces',
+    'visiblePeople',
   };
 }
 

mobile/openapi/test/people_with_faces_response_dto_test.dart

@@ -16,13 +16,13 @@ void main() {
   // final instance = PeopleWithFacesResponseDto();
 
   group('test PeopleWithFacesResponseDto', () {
-    // List<PersonWithFacesResponseDto> faces (default value: const [])
-    test('to test the property `faces`', () async {
+    // int numberOfFaces
+    test('to test the property `numberOfFaces`', () async {
       // TODO
     });
 
-    // int numberOfFaces
-    test('to test the property `numberOfFaces`', () async {
+    // List<PersonWithFacesResponseDto> visiblePeople (default value: const [])
+    test('to test the property `visiblePeople`', () async {
       // TODO
     });
 

open-api/immich-openapi-specs.json

@@ -9014,19 +9014,19 @@
       },
       "PeopleWithFacesResponseDto": {
         "properties": {
-          "faces": {
+          "numberOfFaces": {
+            "type": "integer"
+          },
+          "visiblePeople": {
             "items": {
               "$ref": "#/components/schemas/PersonWithFacesResponseDto"
             },
             "type": "array"
-          },
-          "numberOfFaces": {
-            "type": "integer"
           }
         },
         "required": [
-          "faces",
-          "numberOfFaces"
+          "numberOfFaces",
+          "visiblePeople"
         ],
         "type": "object"
       },

open-api/typescript-sdk/src/fetch-client.ts

@@ -101,8 +101,8 @@ export type PersonWithFacesResponseDto = {
     thumbnailPath: string;
 };
 export type PeopleWithFacesResponseDto = {
-    faces: PersonWithFacesResponseDto[];
     numberOfFaces: number;
+    visiblePeople: PersonWithFacesResponseDto[];
 };
 export type SmartInfoResponseDto = {
     objects?: string[] | null;

server/src/dtos/asset-response.dto.ts

@@ -78,7 +78,7 @@ const peopleWithFaces = (faces: AssetFaceEntity[]): PeopleWithFacesResponseDto =
     }
   }
 
-  return { faces: result, numberOfFaces: faces.length };
+  return { visiblePeople: result, numberOfFaces: faces.length };
 };
 
 export function mapAsset(entity: AssetEntity, options: AssetMapOptions = {}): AssetResponseDto {

server/src/dtos/person.dto.ts

@@ -78,7 +78,7 @@ export class PersonWithFacesResponseDto extends PersonResponseDto {
 }
 
 export class PeopleWithFacesResponseDto {
-  faces!: PersonWithFacesResponseDto[];
+  visiblePeople!: PersonWithFacesResponseDto[];
   @ApiProperty({ type: 'integer' })
   numberOfFaces!: number;
 }

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

@@ -449,6 +449,18 @@ describe(PersonService.name, () => {
       await expect(sut.unassignFace(authStub.admin, faceStub.face1.id)).resolves.toStrictEqual(
         mapFaces(faceStub.unassignedFace, authStub.admin),
       );
+
+      expect(mediaMock.generateThumbnail).not.toHaveBeenCalled();
+    });
+
+    it('should not unassign a face if user has no create access', async () => {
+      personMock.getFaceById.mockResolvedValueOnce(faceStub.face1);
+      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id]));
+      personMock.reassignFace.mockResolvedValue(1);
+      personMock.getRandomFace.mockResolvedValue(null);
+      personMock.getFaceById.mockResolvedValueOnce(faceStub.unassignedFace);
+
+      await expect(sut.unassignFace(authStub.admin, faceStub.face1.id)).rejects.toBeInstanceOf(BadRequestException);
     });
   });
 
@@ -465,6 +477,18 @@ describe(PersonService.name, () => {
         sut.unassignFaces(authStub.admin, { data: [{ assetId: faceStub.face1.id, personId: 'person-1' }] }),
       ).resolves.toStrictEqual([{ id: 'assetFaceId1', success: true }]);
     });
+
+    it('should not unassign a face if the user has no create access', async () => {
+      personMock.getFacesByIds.mockResolvedValueOnce([faceStub.face1]);
+      accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id]));
+      personMock.reassignFace.mockResolvedValue(1);
+      personMock.getRandomFace.mockResolvedValue(null);
+      personMock.getFaceById.mockResolvedValueOnce(faceStub.unassignedFace);
+
+      await expect(
+        sut.unassignFaces(authStub.admin, { data: [{ assetId: faceStub.face1.id, personId: 'person-1' }] }),
+      ).rejects.toBeInstanceOf(BadRequestException);
+    });
   });
 
   describe('handlePersonCleanup', () => {

web/src/lib/components/asset-viewer/detail-panel.svelte

@@ -218,7 +218,7 @@
       <div class="flex h-10 w-full items-center justify-between">
         <h2>PEOPLE</h2>
         <div class="flex gap-2 items-center">
-          {#if people.faces.some((person) => person.isHidden)}
+          {#if people.visiblePeople.some((person) => person.isHidden)}
             <CircleIconButton
               title="Show hidden people"
               icon={showingHiddenPeople ? mdiEyeOff : mdiEye}
@@ -239,7 +239,7 @@
       </div>
 
       <div class="mt-2 flex flex-wrap gap-2">
-        {#each people.faces as person (person.id)}
+        {#each people.visiblePeople as person (person.id)}
           {#if showingHiddenPeople || !person.isHidden}
             <a
               class="w-[90px]"

web/src/lib/components/faces-page/person-side-panel.svelte

@@ -64,6 +64,10 @@
   let loaderLoadingDoneTimeout: ReturnType<typeof setTimeout>;
   let automaticRefreshTimeout: ReturnType<typeof setTimeout>;
 
+  $: mapFacesToBeCreated = Object.entries(selectedPersonToAdd)
+    .filter(([_, value]) => value.person === null)
+    .map(([key, _]) => key);
+
   const thumbnailWidth = '90px';
 
   const generatePeopleWithoutFaces = async () => {
@@ -94,9 +98,23 @@
     isShowLoadingPeople = false;
   }
 
+  /*
+   * we wait for the server to create the feature photo for:
+   * - people which has been reassigned to a new person
+   * - faces removed assigned to a new person
+   *
+   * if after 15 seconds the server has not generated the feature photos,
+   * we go back to the detail-panel
+   */
   const onPersonThumbnail = (personId: string) => {
     assetFaceGenerated.push(personId);
-    if (isEqual(assetFaceGenerated, peopleToCreate) && loaderLoadingDoneTimeout && automaticRefreshTimeout) {
+
+    if (
+      isEqual(assetFaceGenerated, peopleToCreate) &&
+      isEqual(assetFaceGenerated, mapFacesToBeCreated) &&
+      loaderLoadingDoneTimeout &&
+      automaticRefreshTimeout
+    ) {
       clearTimeout(loaderLoadingDoneTimeout);
       clearTimeout(automaticRefreshTimeout);
       onRefresh();

web/src/lib/utils/thumbnail-util.ts

@@ -45,7 +45,7 @@ export function getAltText(asset: AssetResponseDto) {
     altText += ` in ${asset.exifInfo.city}, ${asset.exifInfo.country}`;
   }
 
-  const names = asset.people?.faces.filter((p) => p.name).map((p) => p.name) ?? [];
+  const names = asset.people?.visiblePeople.filter((p) => p.name).map((p) => p.name) ?? [];
   if (names.length == 1) {
     altText += ` with ${names[0]}`;
   }