From 77a362f0c0fa6f7757519ab664c68c1ba0253cf2 Mon Sep 17 00:00:00 2001 From: SGT Date: Fri, 13 Jun 2025 16:18:44 -0300 Subject: [PATCH] chore(server): replace usage of deprecated orderBy and remove unnecessary instruction (#19072) * replace usage of deprecated orderBy instruction remove unnecesarry extra order instruction update e2e test * rename symbols --- e2e/src/api/specs/person.e2e-spec.ts | 75 ++++++++++++++++++-- server/src/repositories/person.repository.ts | 3 +- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/e2e/src/api/specs/person.e2e-spec.ts b/e2e/src/api/specs/person.e2e-spec.ts index 1826002af6..a56118a68b 100644 --- a/e2e/src/api/specs/person.e2e-spec.ts +++ b/e2e/src/api/specs/person.e2e-spec.ts @@ -11,11 +11,24 @@ describe('/people', () => { let hiddenPerson: PersonResponseDto; let multipleAssetsPerson: PersonResponseDto; + let nameAlicePerson: PersonResponseDto; + let nameBobPerson: PersonResponseDto; + let nameCharliePerson: PersonResponseDto; + let nameNullPerson: PersonResponseDto; + beforeAll(async () => { await utils.resetDatabase(); admin = await utils.adminSetup(); - [visiblePerson, hiddenPerson, multipleAssetsPerson] = await Promise.all([ + [ + visiblePerson, + hiddenPerson, + multipleAssetsPerson, + nameCharliePerson, + nameBobPerson, + nameAlicePerson, + nameNullPerson, + ] = await Promise.all([ utils.createPerson(admin.accessToken, { name: 'visible_person', }), @@ -26,10 +39,24 @@ describe('/people', () => { utils.createPerson(admin.accessToken, { name: 'multiple_assets_person', }), + // --- Setup for the specific sorting test --- + utils.createPerson(admin.accessToken, { + name: 'Charlie', + }), + utils.createPerson(admin.accessToken, { + name: 'Bob', + }), + utils.createPerson(admin.accessToken, { + name: 'Alice', + }), + utils.createPerson(admin.accessToken, { + name: '', + }), ]); const asset1 = await utils.createAsset(admin.accessToken); const asset2 = await utils.createAsset(admin.accessToken); + const asset3 = await utils.createAsset(admin.accessToken); await Promise.all([ utils.createFace({ assetId: asset1.id, personId: visiblePerson.id }), @@ -37,6 +64,15 @@ 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 }), + // 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 + utils.createFace({ assetId: asset1.id, personId: nameNullPerson.id }), + utils.createFace({ assetId: asset2.id, personId: nameNullPerson.id }), // 2 assets ]); }); @@ -51,26 +87,53 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ hasNextPage: false, - total: 3, + total: 7, hidden: 1, people: [ expect.objectContaining({ name: 'multiple_assets_person' }), + expect.objectContaining({ name: 'Bob' }), + expect.objectContaining({ name: 'Alice' }), + expect.objectContaining({ name: 'Charlie' }), expect.objectContaining({ name: 'visible_person' }), expect.objectContaining({ name: 'hidden_person' }), ], }); }); + it('should sort visible people by asset count (desc), then by name (asc, nulls last)', async () => { + const { status, body } = await request(app).get('/people').set('Authorization', `Bearer ${admin.accessToken}`); + + expect(status).toBe(200); + expect(body.hasNextPage).toBe(false); + expect(body.total).toBe(7); // 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 + nameBobPerson.id, // name: 'Bob', count: 2 + nameAlicePerson.id, // name: 'Alice', count: 1 + nameCharliePerson.id, // name: 'Charlie', count: 1 + visiblePerson.id, // name: 'visible_person', count: 1 + ]); + + expect(people.some((p) => p.id === hiddenPerson.id)).toBe(false); + }); + it('should return only visible people', async () => { const { status, body } = await request(app).get('/people').set('Authorization', `Bearer ${admin.accessToken}`); expect(status).toBe(200); expect(body).toEqual({ hasNextPage: false, - total: 3, + total: 7, hidden: 1, people: [ expect.objectContaining({ name: 'multiple_assets_person' }), + expect.objectContaining({ name: 'Bob' }), + expect.objectContaining({ name: 'Alice' }), + expect.objectContaining({ name: 'Charlie' }), expect.objectContaining({ name: 'visible_person' }), ], }); @@ -80,12 +143,12 @@ describe('/people', () => { const { status, body } = await request(app) .get('/people') .set('Authorization', `Bearer ${admin.accessToken}`) - .query({ withHidden: true, page: 2, size: 1 }); + .query({ withHidden: true, page: 5, size: 1 }); expect(status).toBe(200); expect(body).toEqual({ hasNextPage: true, - total: 3, + total: 7, hidden: 1, people: [expect.objectContaining({ name: 'visible_person' })], }); @@ -128,7 +191,7 @@ describe('/people', () => { .set('Authorization', `Bearer ${admin.accessToken}`); expect(status).toBe(200); - expect(body).toEqual(expect.objectContaining({ assets: 2 })); + expect(body).toEqual(expect.objectContaining({ assets: 3 })); }); }); diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index 229a523c17..6633fb68ac 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -179,9 +179,8 @@ export class PersonRepository { ) .$if(!options?.closestFaceAssetId, (qb) => qb - .orderBy(sql`NULLIF(person.name, '') is null`, 'asc') .orderBy((eb) => eb.fn.count('asset_faces.assetId'), 'desc') - .orderBy(sql`NULLIF(person.name, '')`, sql`asc nulls last`) + .orderBy(sql`NULLIF(person.name, '') asc nulls last`) .orderBy('person.createdAt'), ) .$if(!options?.withHidden, (qb) => qb.where('person.isHidden', '=', false))