Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: people infinite scroll #11326

Merged
merged 5 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions e2e/src/api/specs/person.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ describe('/people', () => {

expect(status).toBe(200);
expect(body).toEqual({
hasNextPage: false,
total: 3,
hidden: 1,
people: [
Expand All @@ -80,6 +81,7 @@ describe('/people', () => {

expect(status).toBe(200);
expect(body).toEqual({
hasNextPage: false,
total: 3,
hidden: 1,
people: [
Expand All @@ -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', () => {
Expand Down
24 changes: 21 additions & 3 deletions mobile/openapi/lib/api/people_api.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 19 additions & 1 deletion mobile/openapi/lib/model/people_response_dto.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions open-api/immich-openapi-specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -9531,6 +9554,10 @@
},
"PeopleResponseDto": {
"properties": {
"hasNextPage": {
"description": "This property was added in v1.110.0",
"type": "boolean"
},
"hidden": {
"type": "integer"
},
Expand Down
8 changes: 7 additions & 1 deletion open-api/typescript-sdk/src/fetch-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
23 changes: 21 additions & 2 deletions server/src/dtos/person.dto.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the type annotation, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conversion is necessary because implicit conversions are disabled. Pagination is optional with the same defaults as before, so there shouldn't be any breaking changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok cool!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only do because it is a query param so it would be a string otherwise.

}

export class PersonResponseDto {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion server/src/interfaces/person.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export interface PeopleStatistics {

export interface IPersonRepository {
getAll(pagination: PaginationOptions, options?: FindManyOptions<PersonEntity>): Paginated<PersonEntity>;
getAllForUser(userId: string, options: PersonSearchOptions): Promise<PersonEntity[]>;
getAllForUser(pagination: PaginationOptions, userId: string, options: PersonSearchOptions): Paginated<PersonEntity>;
getAllWithoutFaces(): Promise<PersonEntity[]>;
getById(personId: string): Promise<PersonEntity | null>;
getByName(userId: string, personName: string, options: PersonNameSearchOptions): Promise<PersonEntity[]>;
Expand Down
7 changes: 5 additions & 2 deletions server/src/queries/person.repository.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 9 additions & 6 deletions server/src/repositories/person.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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<PersonEntity[]> {
@GenerateSql({ params: [{ take: 10, skip: 10 }, DummyValue.UUID] })
getAllForUser(pagination: PaginationOptions, userId: string, options?: PersonSearchOptions): Paginated<PersonEntity> {
const queryBuilder = this.personRepository
.createQueryBuilder('person')
.leftJoin('person.faces', 'face')
Expand All @@ -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()
Expand Down
10 changes: 7 additions & 3 deletions server/src/services/person.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand All @@ -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,
});
Expand Down
13 changes: 10 additions & 3 deletions server/src/services/person.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,22 @@ export class PersonService {
}

async getAll(auth: AuthDto, dto: PersonSearchDto): Promise<PeopleResponseDto> {
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,
};
Expand Down
9 changes: 9 additions & 0 deletions web/src/lib/__mocks__/intersection-observer.mock.ts
Original file line number Diff line number Diff line change
@@ -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(),
}));
Loading
Loading