fix: people ordering incorrect (#19298)

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

@@ -14,7 +14,11 @@ describe('/people', () => {
   let nameAlicePerson: PersonResponseDto;
   let nameBobPerson: PersonResponseDto;
   let nameCharliePerson: PersonResponseDto;
-  let nameNullPerson: PersonResponseDto;
+  let nameNullPerson4Assets: PersonResponseDto;
+  let nameNullPerson3Assets: PersonResponseDto;
+  let nameNullPerson1Asset: PersonResponseDto;
+  let nameBillPersonFavourite: PersonResponseDto;
+  let nameFreddyPersonFavourite: PersonResponseDto;
 
   beforeAll(async () => {
     await utils.resetDatabase();
@@ -27,7 +31,11 @@ describe('/people', () => {
       nameCharliePerson,
       nameBobPerson,
       nameAlicePerson,
-      nameNullPerson,
+      nameNullPerson4Assets,
+      nameNullPerson3Assets,
+      nameNullPerson1Asset,
+      nameBillPersonFavourite,
+      nameFreddyPersonFavourite,
     ] = await Promise.all([
       utils.createPerson(admin.accessToken, {
         name: 'visible_person',
@@ -52,11 +60,26 @@ describe('/people', () => {
       utils.createPerson(admin.accessToken, {
         name: '',
       }),
+      utils.createPerson(admin.accessToken, {
+        name: '',
+      }),
+      utils.createPerson(admin.accessToken, {
+        name: '',
+      }),
+      utils.createPerson(admin.accessToken, {
+        name: 'Bill',
+        isFavorite: true,
+      }),
+      utils.createPerson(admin.accessToken, {
+        name: 'Freddy',
+        isFavorite: true,
+      }),
     ]);
 
     const asset1 = await utils.createAsset(admin.accessToken);
     const asset2 = await utils.createAsset(admin.accessToken);
     const asset3 = await utils.createAsset(admin.accessToken);
+    const asset4 = await utils.createAsset(admin.accessToken);
 
     await Promise.all([
       utils.createFace({ assetId: asset1.id, personId: visiblePerson.id }),
@@ -64,15 +87,27 @@ describe('/people', () => {
       utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
       utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
       utils.createFace({ assetId: asset2.id, personId: multipleAssetsPerson.id }),
-      utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }),
+      utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }), // 4 assets
       // Named persons
       utils.createFace({ assetId: asset1.id, personId: nameCharliePerson.id }), // 1 asset
       utils.createFace({ assetId: asset1.id, personId: nameBobPerson.id }),
       utils.createFace({ assetId: asset2.id, personId: nameBobPerson.id }), // 2 assets
       utils.createFace({ assetId: asset1.id, personId: nameAlicePerson.id }), // 1 asset
-      // Null-named person
+      // Null-named person 4 assets
-      utils.createFace({ assetId: asset1.id, personId: nameNullPerson.id }),
+      utils.createFace({ assetId: asset1.id, personId: nameNullPerson4Assets.id }),
-      utils.createFace({ assetId: asset2.id, personId: nameNullPerson.id }), // 2 assets
+      utils.createFace({ assetId: asset2.id, personId: nameNullPerson4Assets.id }),
+      utils.createFace({ assetId: asset3.id, personId: nameNullPerson4Assets.id }),
+      utils.createFace({ assetId: asset4.id, personId: nameNullPerson4Assets.id }), // 4 assets
+      // Null-named person 3 assets
+      utils.createFace({ assetId: asset1.id, personId: nameNullPerson3Assets.id }),
+      utils.createFace({ assetId: asset2.id, personId: nameNullPerson3Assets.id }),
+      utils.createFace({ assetId: asset3.id, personId: nameNullPerson3Assets.id }), // 3 assets
+      // Null-named person 1 asset
+      utils.createFace({ assetId: asset3.id, personId: nameNullPerson1Asset.id }),
+      // Favourite People
+      utils.createFace({ assetId: asset1.id, personId: nameFreddyPersonFavourite.id }),
+      utils.createFace({ assetId: asset2.id, personId: nameFreddyPersonFavourite.id }),
+      utils.createFace({ assetId: asset1.id, personId: nameBillPersonFavourite.id }),
     ]);
   });
 
@@ -87,15 +122,19 @@ describe('/people', () => {
       expect(status).toBe(200);
       expect(body).toEqual({
         hasNextPage: false,
-        total: 7,
+        total: 11,
         hidden: 1,
         people: [
-          expect.objectContaining({ name: 'multiple_assets_person' }),
+          expect.objectContaining({ name: 'Bill' }),
-          expect.objectContaining({ name: 'Bob' }),
+          expect.objectContaining({ name: 'Freddy' }),
           expect.objectContaining({ name: 'Alice' }),
+          expect.objectContaining({ name: 'Bob' }),
           expect.objectContaining({ name: 'Charlie' }),
+          expect.objectContaining({ name: 'multiple_assets_person' }),
           expect.objectContaining({ name: 'visible_person' }),
-          expect.objectContaining({ name: 'hidden_person' }),
+          expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }),
+          expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }),
+          expect.objectContaining({ name: 'hidden_person' }), // Should really be before the null names
         ],
       });
     });
@@ -105,17 +144,21 @@ describe('/people', () => {
 
       expect(status).toBe(200);
       expect(body.hasNextPage).toBe(false);
-      expect(body.total).toBe(7); // All persons
+      expect(body.total).toBe(11); // All persons
       expect(body.hidden).toBe(1); // 'hidden_person'
 
       const people = body.people as PersonResponseDto[];
 
       expect(people.map((p) => p.id)).toEqual([
-        multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3
+        nameBillPersonFavourite.id, // name: 'Bill', count: 2
-        nameBobPerson.id, // name: 'Bob', count: 2
+        nameFreddyPersonFavourite.id, // name: 'Freddy', count: 2
         nameAlicePerson.id, // name: 'Alice', count: 1
+        nameBobPerson.id, // name: 'Bob', count: 2
         nameCharliePerson.id, // name: 'Charlie', count: 1
+        multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3
         visiblePerson.id, // name: 'visible_person', count: 1
+        nameNullPerson4Assets.id, // name: '', count: 4
+        nameNullPerson3Assets.id, // name: '', count: 3
       ]);
 
       expect(people.some((p) => p.id === hiddenPerson.id)).toBe(false);
@@ -127,14 +170,18 @@ describe('/people', () => {
       expect(status).toBe(200);
       expect(body).toEqual({
         hasNextPage: false,
-        total: 7,
+        total: 11,
         hidden: 1,
         people: [
-          expect.objectContaining({ name: 'multiple_assets_person' }),
+          expect.objectContaining({ name: 'Bill' }),
-          expect.objectContaining({ name: 'Bob' }),
+          expect.objectContaining({ name: 'Freddy' }),
           expect.objectContaining({ name: 'Alice' }),
+          expect.objectContaining({ name: 'Bob' }),
           expect.objectContaining({ name: 'Charlie' }),
+          expect.objectContaining({ name: 'multiple_assets_person' }),
           expect.objectContaining({ name: 'visible_person' }),
+          expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }),
+          expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }),
         ],
       });
     });
@@ -148,9 +195,9 @@ describe('/people', () => {
       expect(status).toBe(200);
       expect(body).toEqual({
         hasNextPage: true,
-        total: 7,
+        total: 11,
         hidden: 1,
-        people: [expect.objectContaining({ name: 'visible_person' })],
+        people: [expect.objectContaining({ name: 'Charlie' })],
       });
     });
   });

server/src/queries/person.repository.sql

@@ -12,6 +12,37 @@ delete from "person"
 where
   "person"."id" in ($1)
 
+-- PersonRepository.getAllForUser
+select
+  "person".*
+from
+  "person"
+  inner join "asset_faces" on "asset_faces"."personId" = "person"."id"
+  inner join "assets" on "asset_faces"."assetId" = "assets"."id"
+  and "assets"."visibility" = 'timeline'
+  and "assets"."deletedAt" is null
+where
+  "person"."ownerId" = $1
+  and "asset_faces"."deletedAt" is null
+  and "person"."isHidden" = $2
+group by
+  "person"."id"
+having
+  (
+    "person"."name" != $3
+    or count("asset_faces"."assetId") >= $4
+  )
+order by
+  "person"."isHidden" asc,
+  "person"."isFavorite" desc,
+  NULLIF(person.name, '') asc nulls last,
+  count("asset_faces"."assetId") desc,
+  "person"."createdAt"
+limit
+  $5
+offset
+  $6
+
 -- PersonRepository.getAllWithoutFaces
 select
   "person".*

server/src/repositories/person.repository.ts

@@ -138,6 +138,7 @@ export class PersonRepository {
       .stream();
   }
 
+  @GenerateSql({ params: [{ take: 1, skip: 0 }, DummyValue.UUID] })
   async getAllForUser(pagination: PaginationOptions, userId: string, options?: PersonSearchOptions) {
     const items = await this.db
       .selectFrom('person')
@@ -179,8 +180,8 @@ export class PersonRepository {
       )
       .$if(!options?.closestFaceAssetId, (qb) =>
         qb
+          .orderBy(sql`NULLIF(person.name, '')`, (om) => om.asc().nullsLast())
           .orderBy((eb) => eb.fn.count('asset_faces.assetId'), 'desc')
-          .orderBy(sql`NULLIF(person.name, '') asc nulls last`)
           .orderBy('person.createdAt'),
       )
       .$if(!options?.withHidden, (qb) => qb.where('person.isHidden', '=', false))