From 8e6bc135408814f19802f7f74f07c35ae0b5a0e7 Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Thu, 25 Jul 2024 21:59:28 +0200 Subject: [PATCH] feat: people infinite scroll (#11326) * feat: people infinite scroll * add infinite scroll to show & hide modal * update unit tests * show total people count instead of currently loaded * update personsearchdto --- e2e/src/api/specs/person.e2e-spec.ts | 17 +++++ mobile/openapi/lib/api/people_api.dart | 24 ++++++- .../lib/model/people_response_dto.dart | 20 +++++- open-api/immich-openapi-specs.json | 27 ++++++++ open-api/typescript-sdk/src/fetch-client.ts | 8 ++- server/src/dtos/person.dto.ts | 23 ++++++- server/src/interfaces/person.interface.ts | 2 +- server/src/queries/person.repository.sql | 7 +- server/src/repositories/person.repository.ts | 15 +++-- server/src/services/person.service.spec.ts | 10 ++- server/src/services/person.service.ts | 13 +++- .../__mocks__/intersection-observer.mock.ts | 9 +++ .../manage-people-visibility.spec.ts | 10 +++ .../manage-people-visibility.svelte | 59 ++++++++--------- .../faces-page/people-infinite-scroll.svelte | 33 ++++++++++ web/src/lib/i18n/en.json | 1 + web/src/routes/(user)/people/+page.svelte | 65 ++++++++++++++----- 17 files changed, 276 insertions(+), 67 deletions(-) create mode 100644 web/src/lib/__mocks__/intersection-observer.mock.ts create mode 100644 web/src/lib/components/faces-page/people-infinite-scroll.svelte diff --git a/e2e/src/api/specs/person.e2e-spec.ts b/e2e/src/api/specs/person.e2e-spec.ts index 39f531d513..a675f54437 100644 --- a/e2e/src/api/specs/person.e2e-spec.ts +++ b/e2e/src/api/specs/person.e2e-spec.ts @@ -65,6 +65,7 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ + hasNextPage: false, total: 3, hidden: 1, people: [ @@ -80,6 +81,7 @@ describe('/people', () => { expect(status).toBe(200); expect(body).toEqual({ + hasNextPage: false, total: 3, hidden: 1, people: [ @@ -88,6 +90,21 @@ describe('/people', () => { ], }); }); + + it('should support pagination', async () => { + const { status, body } = await request(app) + .get('/people') + .set('Authorization', `Bearer ${admin.accessToken}`) + .query({ withHidden: true, page: 2, size: 1 }); + + expect(status).toBe(200); + expect(body).toEqual({ + hasNextPage: true, + total: 3, + hidden: 1, + people: [expect.objectContaining({ name: 'visible_person' })], + }); + }); }); describe('GET /people/:id', () => { diff --git a/mobile/openapi/lib/api/people_api.dart b/mobile/openapi/lib/api/people_api.dart index 0086f26fa4..9fe62f0841 100644 --- a/mobile/openapi/lib/api/people_api.dart +++ b/mobile/openapi/lib/api/people_api.dart @@ -66,8 +66,14 @@ class PeopleApi { /// Performs an HTTP 'GET /people' operation and returns the [Response]. /// Parameters: /// + /// * [num] page: + /// Page number for pagination + /// + /// * [num] size: + /// Number of items per page + /// /// * [bool] withHidden: - Future getAllPeopleWithHttpInfo({ bool? withHidden, }) async { + Future getAllPeopleWithHttpInfo({ num? page, num? size, bool? withHidden, }) async { // ignore: prefer_const_declarations final path = r'/people'; @@ -78,6 +84,12 @@ class PeopleApi { final headerParams = {}; final formParams = {}; + if (page != null) { + queryParams.addAll(_queryParams('', 'page', page)); + } + if (size != null) { + queryParams.addAll(_queryParams('', 'size', size)); + } if (withHidden != null) { queryParams.addAll(_queryParams('', 'withHidden', withHidden)); } @@ -98,9 +110,15 @@ class PeopleApi { /// Parameters: /// + /// * [num] page: + /// Page number for pagination + /// + /// * [num] size: + /// Number of items per page + /// /// * [bool] withHidden: - Future getAllPeople({ bool? withHidden, }) async { - final response = await getAllPeopleWithHttpInfo( withHidden: withHidden, ); + Future getAllPeople({ num? page, num? size, bool? withHidden, }) async { + final response = await getAllPeopleWithHttpInfo( page: page, size: size, withHidden: withHidden, ); if (response.statusCode >= HttpStatus.badRequest) { throw ApiException(response.statusCode, await _decodeBodyBytes(response)); } diff --git a/mobile/openapi/lib/model/people_response_dto.dart b/mobile/openapi/lib/model/people_response_dto.dart index 423ea989d4..87e8c34fb0 100644 --- a/mobile/openapi/lib/model/people_response_dto.dart +++ b/mobile/openapi/lib/model/people_response_dto.dart @@ -13,11 +13,21 @@ part of openapi.api; class PeopleResponseDto { /// Returns a new [PeopleResponseDto] instance. PeopleResponseDto({ + this.hasNextPage, required this.hidden, this.people = const [], required this.total, }); + /// This property was added in v1.110.0 + /// + /// Please note: This property should have been non-nullable! Since the specification file + /// does not include a default value (using the "default:" property), however, the generated + /// source code must fall back to having a nullable type. + /// Consider adding a "default:" property in the specification file to hide this note. + /// + bool? hasNextPage; + int hidden; List people; @@ -26,6 +36,7 @@ class PeopleResponseDto { @override bool operator ==(Object other) => identical(this, other) || other is PeopleResponseDto && + other.hasNextPage == hasNextPage && other.hidden == hidden && _deepEquality.equals(other.people, people) && other.total == total; @@ -33,15 +44,21 @@ class PeopleResponseDto { @override int get hashCode => // ignore: unnecessary_parenthesis + (hasNextPage == null ? 0 : hasNextPage!.hashCode) + (hidden.hashCode) + (people.hashCode) + (total.hashCode); @override - String toString() => 'PeopleResponseDto[hidden=$hidden, people=$people, total=$total]'; + String toString() => 'PeopleResponseDto[hasNextPage=$hasNextPage, hidden=$hidden, people=$people, total=$total]'; Map toJson() { final json = {}; + if (this.hasNextPage != null) { + json[r'hasNextPage'] = this.hasNextPage; + } else { + // json[r'hasNextPage'] = null; + } json[r'hidden'] = this.hidden; json[r'people'] = this.people; json[r'total'] = this.total; @@ -56,6 +73,7 @@ class PeopleResponseDto { final json = value.cast(); return PeopleResponseDto( + hasNextPage: mapValueOfType(json, r'hasNextPage'), hidden: mapValueOfType(json, r'hidden')!, people: PersonResponseDto.listFromJson(json[r'people']), total: mapValueOfType(json, r'total')!, diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index de19f99bcf..da5b1e2ff3 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -3824,6 +3824,29 @@ "get": { "operationId": "getAllPeople", "parameters": [ + { + "name": "page", + "required": false, + "in": "query", + "description": "Page number for pagination", + "schema": { + "minimum": 1, + "default": 1, + "type": "number" + } + }, + { + "name": "size", + "required": false, + "in": "query", + "description": "Number of items per page", + "schema": { + "minimum": 1, + "maximum": 1000, + "default": 500, + "type": "number" + } + }, { "name": "withHidden", "required": false, @@ -9531,6 +9554,10 @@ }, "PeopleResponseDto": { "properties": { + "hasNextPage": { + "description": "This property was added in v1.110.0", + "type": "boolean" + }, "hidden": { "type": "integer" }, diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 9b6c3f2574..f2b03dcac1 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -607,6 +607,8 @@ export type UpdatePartnerDto = { inTimeline: boolean; }; export type PeopleResponseDto = { + /** This property was added in v1.110.0 */ + hasNextPage?: boolean; hidden: number; people: PersonResponseDto[]; total: number; @@ -2173,13 +2175,17 @@ export function updatePartner({ id, updatePartnerDto }: { body: updatePartnerDto }))); } -export function getAllPeople({ withHidden }: { +export function getAllPeople({ page, size, withHidden }: { + page?: number; + size?: number; withHidden?: boolean; }, opts?: Oazapfts.RequestOpts) { return oazapfts.ok(oazapfts.fetchJson<{ status: 200; data: PeopleResponseDto; }>(`/people${QS.query(QS.explode({ + page, + size, withHidden }))}`, { ...opts diff --git a/server/src/dtos/person.dto.ts b/server/src/dtos/person.dto.ts index 3ad41ecff2..8d60f383a3 100644 --- a/server/src/dtos/person.dto.ts +++ b/server/src/dtos/person.dto.ts @@ -1,6 +1,6 @@ -import { ApiProperty } from '@nestjs/swagger'; +import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { Type } from 'class-transformer'; -import { IsArray, IsNotEmpty, IsString, MaxDate, ValidateNested } from 'class-validator'; +import { IsArray, IsInt, IsNotEmpty, IsString, Max, MaxDate, Min, ValidateNested } from 'class-validator'; import { PropertyLifecycle } from 'src/decorators'; import { AuthDto } from 'src/dtos/auth.dto'; import { AssetFaceEntity } from 'src/entities/asset-face.entity'; @@ -63,6 +63,21 @@ export class MergePersonDto { export class PersonSearchDto { @ValidateBoolean({ optional: true }) withHidden?: boolean; + + /** Page number for pagination */ + @ApiPropertyOptional() + @IsInt() + @Min(1) + @Type(() => Number) + page: number = 1; + + /** Number of items per page */ + @ApiPropertyOptional() + @IsInt() + @Min(1) + @Max(1000) + @Type(() => Number) + size: number = 500; } export class PersonResponseDto { @@ -132,6 +147,10 @@ export class PeopleResponseDto { @ApiProperty({ type: 'integer' }) hidden!: number; people!: PersonResponseDto[]; + + // TODO: make required after a few versions + @PropertyLifecycle({ addedAt: 'v1.110.0' }) + hasNextPage?: boolean; } export function mapPerson(person: PersonEntity): PersonResponseDto { diff --git a/server/src/interfaces/person.interface.ts b/server/src/interfaces/person.interface.ts index b68bd534b8..358310a5cb 100644 --- a/server/src/interfaces/person.interface.ts +++ b/server/src/interfaces/person.interface.ts @@ -37,7 +37,7 @@ export interface PeopleStatistics { export interface IPersonRepository { getAll(pagination: PaginationOptions, options?: FindManyOptions): Paginated; - getAllForUser(userId: string, options: PersonSearchOptions): Promise; + getAllForUser(pagination: PaginationOptions, userId: string, options: PersonSearchOptions): Paginated; getAllWithoutFaces(): Promise; getById(personId: string): Promise; getByName(userId: string, personName: string, options: PersonNameSearchOptions): Promise; diff --git a/server/src/queries/person.repository.sql b/server/src/queries/person.repository.sql index 588606628c..4e4d36da8b 100644 --- a/server/src/queries/person.repository.sql +++ b/server/src/queries/person.repository.sql @@ -37,9 +37,12 @@ ORDER BY "person"."isHidden" ASC, NULLIF("person"."name", '') IS NULL ASC, COUNT("face"."assetId") DESC, - NULLIF("person"."name", '') ASC NULLS LAST + NULLIF("person"."name", '') ASC NULLS LAST, + "person"."createdAt" ASC LIMIT - 500 + 11 +OFFSET + 10 -- PersonRepository.getAllWithoutFaces SELECT diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index f0344e8ab8..876ed369f6 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -16,7 +16,7 @@ import { UpdateFacesData, } from 'src/interfaces/person.interface'; import { Instrumentation } from 'src/utils/instrumentation'; -import { Paginated, PaginationOptions, paginate } from 'src/utils/pagination'; +import { Paginated, PaginationMode, PaginationOptions, paginate, paginatedBuilder } from 'src/utils/pagination'; import { FindManyOptions, FindOptionsRelations, FindOptionsSelect, In, Repository } from 'typeorm'; @Instrumentation() @@ -64,8 +64,8 @@ export class PersonRepository implements IPersonRepository { return paginate(this.personRepository, pagination, options); } - @GenerateSql({ params: [DummyValue.UUID] }) - getAllForUser(userId: string, options?: PersonSearchOptions): Promise { + @GenerateSql({ params: [{ take: 10, skip: 10 }, DummyValue.UUID] }) + getAllForUser(pagination: PaginationOptions, userId: string, options?: PersonSearchOptions): Paginated { const queryBuilder = this.personRepository .createQueryBuilder('person') .leftJoin('person.faces', 'face') @@ -76,15 +76,18 @@ export class PersonRepository implements IPersonRepository { .addOrderBy("NULLIF(person.name, '') IS NULL", 'ASC') .addOrderBy('COUNT(face.assetId)', 'DESC') .addOrderBy("NULLIF(person.name, '')", 'ASC', 'NULLS LAST') + .addOrderBy('person.createdAt') .andWhere("person.thumbnailPath != ''") .having("person.name != '' OR COUNT(face.assetId) >= :faces", { faces: options?.minimumFaceCount || 1 }) - .groupBy('person.id') - .limit(500); + .groupBy('person.id'); if (!options?.withHidden) { queryBuilder.andWhere('person.isHidden = false'); } - return queryBuilder.getMany(); + return paginatedBuilder(queryBuilder, { + mode: PaginationMode.LIMIT_OFFSET, + ...pagination, + }); } @GenerateSql() diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 70f20d6250..3d195765b6 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -115,9 +115,13 @@ describe(PersonService.name, () => { describe('getAll', () => { it('should get all hidden and visible people with thumbnails', async () => { - personMock.getAllForUser.mockResolvedValue([personStub.withName, personStub.hidden]); + personMock.getAllForUser.mockResolvedValue({ + items: [personStub.withName, personStub.hidden], + hasNextPage: false, + }); personMock.getNumberOfPeople.mockResolvedValue({ total: 2, hidden: 1 }); - await expect(sut.getAll(authStub.admin, { withHidden: true })).resolves.toEqual({ + await expect(sut.getAll(authStub.admin, { withHidden: true, page: 1, size: 10 })).resolves.toEqual({ + hasNextPage: false, total: 2, hidden: 1, people: [ @@ -132,7 +136,7 @@ describe(PersonService.name, () => { }, ], }); - expect(personMock.getAllForUser).toHaveBeenCalledWith(authStub.admin.user.id, { + expect(personMock.getAllForUser).toHaveBeenCalledWith({ skip: 0, take: 10 }, authStub.admin.user.id, { minimumFaceCount: 3, withHidden: true, }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 338628e469..95c79573bd 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -91,15 +91,22 @@ export class PersonService { } async getAll(auth: AuthDto, dto: PersonSearchDto): Promise { + const { withHidden = false, page, size } = dto; + const pagination = { + take: size, + skip: (page - 1) * size, + }; + const { machineLearning } = await this.configCore.getConfig({ withCache: false }); - const people = await this.repository.getAllForUser(auth.user.id, { + const { items, hasNextPage } = await this.repository.getAllForUser(pagination, auth.user.id, { minimumFaceCount: machineLearning.facialRecognition.minFaces, - withHidden: dto.withHidden || false, + withHidden, }); const { total, hidden } = await this.repository.getNumberOfPeople(auth.user.id); return { - people: people.map((person) => mapPerson(person)), + people: items.map((person) => mapPerson(person)), + hasNextPage, total, hidden, }; diff --git a/web/src/lib/__mocks__/intersection-observer.mock.ts b/web/src/lib/__mocks__/intersection-observer.mock.ts new file mode 100644 index 0000000000..5565e9a139 --- /dev/null +++ b/web/src/lib/__mocks__/intersection-observer.mock.ts @@ -0,0 +1,9 @@ +import { vi } from 'vitest'; + +export const getIntersectionObserverMock = () => + vi.fn(() => ({ + disconnect: vi.fn(), + observe: vi.fn(), + takeRecords: vi.fn(), + unobserve: vi.fn(), + })); diff --git a/web/src/lib/components/faces-page/manage-people-visibility.spec.ts b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts index a6ad24ac2c..66d0e256ce 100644 --- a/web/src/lib/components/faces-page/manage-people-visibility.spec.ts +++ b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts @@ -1,3 +1,4 @@ +import { getIntersectionObserverMock } from '$lib/__mocks__/intersection-observer.mock'; import { sdkMock } from '$lib/__mocks__/sdk.mock'; import ManagePeopleVisibility from '$lib/components/faces-page/manage-people-visibility.svelte'; import type { PersonResponseDto } from '@immich/sdk'; @@ -18,6 +19,7 @@ describe('ManagePeopleVisibility Component', () => { }); beforeEach(() => { + vi.stubGlobal('IntersectionObserver', getIntersectionObserverMock()); personVisible = personFactory.build({ isHidden: false }); personHidden = personFactory.build({ isHidden: true }); personWithoutName = personFactory.build({ isHidden: false, name: undefined }); @@ -32,7 +34,9 @@ describe('ManagePeopleVisibility Component', () => { const { getByText } = render(ManagePeopleVisibility, { props: { people: [personVisible, personHidden, personWithoutName], + totalPeopleCount: 3, onClose: vi.fn(), + loadNextPage: vi.fn(), }, }); @@ -45,7 +49,9 @@ describe('ManagePeopleVisibility Component', () => { const { getByText, getByTitle } = render(ManagePeopleVisibility, { props: { people: [personVisible, personHidden, personWithoutName], + totalPeopleCount: 3, onClose: vi.fn(), + loadNextPage: vi.fn(), }, }); @@ -63,7 +69,9 @@ describe('ManagePeopleVisibility Component', () => { const { getByText, getByTitle } = render(ManagePeopleVisibility, { props: { people: [personVisible, personHidden, personWithoutName], + totalPeopleCount: 3, onClose: vi.fn(), + loadNextPage: vi.fn(), }, }); @@ -86,7 +94,9 @@ describe('ManagePeopleVisibility Component', () => { const { getByText, getByTitle } = render(ManagePeopleVisibility, { props: { people: [personVisible, personHidden, personWithoutName], + totalPeopleCount: 3, onClose: vi.fn(), + loadNextPage: vi.fn(), }, }); diff --git a/web/src/lib/components/faces-page/manage-people-visibility.svelte b/web/src/lib/components/faces-page/manage-people-visibility.svelte index 1fa9ea5b9b..23a69e7759 100644 --- a/web/src/lib/components/faces-page/manage-people-visibility.svelte +++ b/web/src/lib/components/faces-page/manage-people-visibility.svelte @@ -10,6 +10,7 @@ import { shortcut } from '$lib/actions/shortcut'; import ImageThumbnail from '$lib/components/assets/thumbnail/image-thumbnail.svelte'; import Button from '$lib/components/elements/buttons/button.svelte'; + import PeopleInfiniteScroll from '$lib/components/faces-page/people-infinite-scroll.svelte'; import LoadingSpinner from '$lib/components/shared-components/loading-spinner.svelte'; import { notificationController, @@ -24,8 +25,10 @@ import CircleIconButton from '../elements/buttons/circle-icon-button.svelte'; export let people: PersonResponseDto[]; - export let onClose: () => void; + export let totalPeopleCount: number; export let titleId: string | undefined = undefined; + export let onClose: () => void; + export let loadNextPage: () => void; let toggleVisibility = ToggleVisibility.SHOW_ALL; let showLoadingSpinner = false; @@ -121,7 +124,7 @@

{$t('show_and_hide_people')}

-

({people.length.toLocaleString($locale)})

+

({totalPeopleCount.toLocaleString($locale)})

@@ -138,31 +141,29 @@
-
- {#each people as person, index (person.id)} - {@const hidden = personIsHidden[person.id]} - - {/each} -
+ + {@const hidden = personIsHidden[person.id]} + +
diff --git a/web/src/lib/components/faces-page/people-infinite-scroll.svelte b/web/src/lib/components/faces-page/people-infinite-scroll.svelte new file mode 100644 index 0000000000..aefd6fe957 --- /dev/null +++ b/web/src/lib/components/faces-page/people-infinite-scroll.svelte @@ -0,0 +1,33 @@ + + +
+ {#each people as person, index (person.id)} + {#if hasNextPage && index === people.length - 1} +
+ +
+ {:else} + + {/if} + {/each} +
diff --git a/web/src/lib/i18n/en.json b/web/src/lib/i18n/en.json index 99cb356d2e..f97f6d35ed 100644 --- a/web/src/lib/i18n/en.json +++ b/web/src/lib/i18n/en.json @@ -567,6 +567,7 @@ "failed_to_get_people": "Failed to get people", "failed_to_load_asset": "Failed to load asset", "failed_to_load_assets": "Failed to load assets", + "failed_to_load_people": "Failed to load people", "failed_to_stack_assets": "Failed to stack assets", "failed_to_unstack_assets": "Failed to un-stack assets", "import_path_already_exists": "This import path already exists.", diff --git a/web/src/routes/(user)/people/+page.svelte b/web/src/routes/(user)/people/+page.svelte index d40ef625b6..f1a2674e24 100644 --- a/web/src/routes/(user)/people/+page.svelte +++ b/web/src/routes/(user)/people/+page.svelte @@ -1,12 +1,14 @@