fix(server): incorrect number of assets for a person (#7602)

* fix: incorrect number of assets * fix: tests * pr feedback * fix: e2e test * fix: e2e test * fix: e2e test * feat: more tests
immich-app · Mar 4, 2024 · 6ab4045 · 6ab4045
1 parent 5bc13c4
commit 6ab4045
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 20 deletions.
diff --git a/e2e/src/api/specs/person.e2e-spec.ts b/e2e/src/api/specs/person.e2e-spec.ts
@@ -9,6 +9,7 @@ describe('/activity', () => {
   let admin: LoginResponseDto;
   let visiblePerson: PersonResponseDto;
   let hiddenPerson: PersonResponseDto;
+  let multipleAssetsPerson: PersonResponseDto;
 
   beforeAll(async () => {
     apiUtils.setup();
@@ -19,21 +20,28 @@ describe('/activity', () => {
   beforeEach(async () => {
     await dbUtils.reset(['person']);
 
-    [visiblePerson, hiddenPerson] = await Promise.all([
+    [visiblePerson, hiddenPerson, multipleAssetsPerson] = await Promise.all([
       apiUtils.createPerson(admin.accessToken, {
         name: 'visible_person',
       }),
       apiUtils.createPerson(admin.accessToken, {
         name: 'hidden_person',
         isHidden: true,
       }),
+      apiUtils.createPerson(admin.accessToken, {
+        name: 'multiple_assets_person',
+      }),
     ]);
 
-    const asset = await apiUtils.createAsset(admin.accessToken);
+    const asset1 = await apiUtils.createAsset(admin.accessToken);
+    const asset2 = await apiUtils.createAsset(admin.accessToken);
 
     await Promise.all([
-      dbUtils.createFace({ assetId: asset.id, personId: visiblePerson.id }),
-      dbUtils.createFace({ assetId: asset.id, personId: hiddenPerson.id }),
+      dbUtils.createFace({ assetId: asset1.id, personId: visiblePerson.id }),
+      dbUtils.createFace({ assetId: asset1.id, personId: hiddenPerson.id }),
+      dbUtils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
+      dbUtils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
+      dbUtils.createFace({ assetId: asset2.id, personId: multipleAssetsPerson.id }),
     ]);
   });
 
@@ -55,9 +63,10 @@ describe('/activity', () => {
 
       expect(status).toBe(200);
       expect(body).toEqual({
-        total: 2,
+        total: 3,
         hidden: 1,
         people: [
+          expect.objectContaining({ name: 'multiple_assets_person' }),
           expect.objectContaining({ name: 'visible_person' }),
           expect.objectContaining({ name: 'hidden_person' }),
         ],
@@ -69,9 +78,12 @@ describe('/activity', () => {
 
       expect(status).toBe(200);
       expect(body).toEqual({
-        total: 2,
+        total: 3,
         hidden: 1,
-        people: [expect.objectContaining({ name: 'visible_person' })],
+        people: [
+          expect.objectContaining({ name: 'multiple_assets_person' }),
+          expect.objectContaining({ name: 'visible_person' }),
+        ],
       });
     });
   });
@@ -103,6 +115,33 @@ describe('/activity', () => {
     });
   });
 
+  describe('GET /person/:id/statistics', () => {
+    it('should require authentication', async () => {
+      const { status, body } = await request(app).get(`/person/${multipleAssetsPerson.id}/statistics`);
+
+      expect(status).toBe(401);
+      expect(body).toEqual(errorDto.unauthorized);
+    });
+
+    it('should throw error if person with id does not exist', async () => {
+      const { status, body } = await request(app)
+        .get(`/person/${uuidDto.notFound}/statistics`)
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+
+      expect(status).toBe(400);
+      expect(body).toEqual(errorDto.badRequest());
+    });
+
+    it('should return the correct number of assets', async () => {
+      const { status, body } = await request(app)
+        .get(`/person/${multipleAssetsPerson.id}/statistics`)
+        .set('Authorization', `Bearer ${admin.accessToken}`);
+
+      expect(status).toBe(200);
+      expect(body).toEqual(expect.objectContaining({ assets: 2 }));
+    });
+  });
+
   describe('PUT /person/:id', () => {
     it('should require authentication', async () => {
       const { status, body } = await request(app).put(`/person/${uuidDto.notFound}`);

diff --git a/server/src/infra/repositories/person.repository.ts b/server/src/infra/repositories/person.repository.ts
@@ -171,16 +171,17 @@ export class PersonRepository implements IPersonRepository {
 
   @GenerateSql({ params: [DummyValue.UUID] })
   async getStatistics(personId: string): Promise<PersonStatistics> {
+    const items = await this.assetFaceRepository
+      .createQueryBuilder('face')
+      .leftJoin('face.asset', 'asset')
+      .where('face.personId = :personId', { personId })
+      .andWhere('asset.isArchived = false')
+      .andWhere('asset.deletedAt IS NULL')
+      .andWhere('asset.livePhotoVideoId IS NULL')
+      .select('COUNT(DISTINCT(asset.id))', 'count')
+      .getRawOne();
     return {
-      assets: await this.assetFaceRepository
-        .createQueryBuilder('face')
-        .leftJoin('face.asset', 'asset')
-        .where('face.personId = :personId', { personId })
-        .andWhere('asset.isArchived = false')
-        .andWhere('asset.deletedAt IS NULL')
-        .andWhere('asset.livePhotoVideoId IS NULL')
-        .distinct(true)
-        .getCount(),
+      assets: items.count ?? 0,
     };
   }
 
@@ -223,8 +224,8 @@ export class PersonRepository implements IPersonRepository {
       .getRawOne();
 
     const result: PeopleStatistics = {
-      total: items ? Number.parseInt(items.total) : 0,
-      hidden: items ? Number.parseInt(items.hidden) : 0,
+      total: items.total ?? 0,
+      hidden: items.hidden ?? 0,
     };
 
     return result;

diff --git a/server/src/infra/sql/person.repository.sql b/server/src/infra/sql/person.repository.sql
@@ -224,8 +224,8 @@ LIMIT
   20
 
 -- PersonRepository.getStatistics
-SELECT DISTINCT
-  COUNT(DISTINCT ("face"."id")) AS "cnt"
+SELECT
+  COUNT(DISTINCT ("asset"."id")) AS "count"
 FROM
   "asset_faces" "face"
   LEFT JOIN "assets" "asset" ON "asset"."id" = "face"."assetId"