Skip to content

Commit fe91b44

Browse files
zackpollardzackpollard
authored
fix: people ordering incorrect (#19298)
1 parent 747a721 commit fe91b44

File tree

3 files changed

+98
-19
lines changed

3 files changed

+98
-19
lines changed

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

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ describe('/people', () => {
1414
let nameAlicePerson: PersonResponseDto;
1515
let nameBobPerson: PersonResponseDto;
1616
let nameCharliePerson: PersonResponseDto;
17-
let nameNullPerson: PersonResponseDto;
17+
let nameNullPerson4Assets: PersonResponseDto;
18+
let nameNullPerson3Assets: PersonResponseDto;
19+
let nameNullPerson1Asset: PersonResponseDto;
20+
let nameBillPersonFavourite: PersonResponseDto;
21+
let nameFreddyPersonFavourite: PersonResponseDto;
1822

1923
beforeAll(async () => {
2024
await utils.resetDatabase();
@@ -27,7 +31,11 @@ describe('/people', () => {
2731
nameCharliePerson,
2832
nameBobPerson,
2933
nameAlicePerson,
30-
nameNullPerson,
34+
nameNullPerson4Assets,
35+
nameNullPerson3Assets,
36+
nameNullPerson1Asset,
37+
nameBillPersonFavourite,
38+
nameFreddyPersonFavourite,
3139
] = await Promise.all([
3240
utils.createPerson(admin.accessToken, {
3341
name: 'visible_person',
@@ -52,27 +60,54 @@ describe('/people', () => {
5260
utils.createPerson(admin.accessToken, {
5361
name: '',
5462
}),
63+
utils.createPerson(admin.accessToken, {
64+
name: '',
65+
}),
66+
utils.createPerson(admin.accessToken, {
67+
name: '',
68+
}),
69+
utils.createPerson(admin.accessToken, {
70+
name: 'Bill',
71+
isFavorite: true,
72+
}),
73+
utils.createPerson(admin.accessToken, {
74+
name: 'Freddy',
75+
isFavorite: true,
76+
}),
5577
]);
5678

5779
const asset1 = await utils.createAsset(admin.accessToken);
5880
const asset2 = await utils.createAsset(admin.accessToken);
5981
const asset3 = await utils.createAsset(admin.accessToken);
82+
const asset4 = await utils.createAsset(admin.accessToken);
6083

6184
await Promise.all([
6285
utils.createFace({ assetId: asset1.id, personId: visiblePerson.id }),
6386
utils.createFace({ assetId: asset1.id, personId: hiddenPerson.id }),
6487
utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
6588
utils.createFace({ assetId: asset1.id, personId: multipleAssetsPerson.id }),
6689
utils.createFace({ assetId: asset2.id, personId: multipleAssetsPerson.id }),
67-
utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }),
90+
utils.createFace({ assetId: asset3.id, personId: multipleAssetsPerson.id }), // 4 assets
6891
// Named persons
6992
utils.createFace({ assetId: asset1.id, personId: nameCharliePerson.id }), // 1 asset
7093
utils.createFace({ assetId: asset1.id, personId: nameBobPerson.id }),
7194
utils.createFace({ assetId: asset2.id, personId: nameBobPerson.id }), // 2 assets
7295
utils.createFace({ assetId: asset1.id, personId: nameAlicePerson.id }), // 1 asset
73-
// Null-named person
74-
utils.createFace({ assetId: asset1.id, personId: nameNullPerson.id }),
75-
utils.createFace({ assetId: asset2.id, personId: nameNullPerson.id }), // 2 assets
96+
// Null-named person 4 assets
97+
utils.createFace({ assetId: asset1.id, personId: nameNullPerson4Assets.id }),
98+
utils.createFace({ assetId: asset2.id, personId: nameNullPerson4Assets.id }),
99+
utils.createFace({ assetId: asset3.id, personId: nameNullPerson4Assets.id }),
100+
utils.createFace({ assetId: asset4.id, personId: nameNullPerson4Assets.id }), // 4 assets
101+
// Null-named person 3 assets
102+
utils.createFace({ assetId: asset1.id, personId: nameNullPerson3Assets.id }),
103+
utils.createFace({ assetId: asset2.id, personId: nameNullPerson3Assets.id }),
104+
utils.createFace({ assetId: asset3.id, personId: nameNullPerson3Assets.id }), // 3 assets
105+
// Null-named person 1 asset
106+
utils.createFace({ assetId: asset3.id, personId: nameNullPerson1Asset.id }),
107+
// Favourite People
108+
utils.createFace({ assetId: asset1.id, personId: nameFreddyPersonFavourite.id }),
109+
utils.createFace({ assetId: asset2.id, personId: nameFreddyPersonFavourite.id }),
110+
utils.createFace({ assetId: asset1.id, personId: nameBillPersonFavourite.id }),
76111
]);
77112
});
78113

@@ -87,15 +122,19 @@ describe('/people', () => {
87122
expect(status).toBe(200);
88123
expect(body).toEqual({
89124
hasNextPage: false,
90-
total: 7,
125+
total: 11,
91126
hidden: 1,
92127
people: [
93-
expect.objectContaining({ name: 'multiple_assets_person' }),
94-
expect.objectContaining({ name: 'Bob' }),
128+
expect.objectContaining({ name: 'Bill' }),
129+
expect.objectContaining({ name: 'Freddy' }),
95130
expect.objectContaining({ name: 'Alice' }),
131+
expect.objectContaining({ name: 'Bob' }),
96132
expect.objectContaining({ name: 'Charlie' }),
133+
expect.objectContaining({ name: 'multiple_assets_person' }),
97134
expect.objectContaining({ name: 'visible_person' }),
98-
expect.objectContaining({ name: 'hidden_person' }),
135+
expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }),
136+
expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }),
137+
expect.objectContaining({ name: 'hidden_person' }), // Should really be before the null names
99138
],
100139
});
101140
});
@@ -105,17 +144,21 @@ describe('/people', () => {
105144

106145
expect(status).toBe(200);
107146
expect(body.hasNextPage).toBe(false);
108-
expect(body.total).toBe(7); // All persons
147+
expect(body.total).toBe(11); // All persons
109148
expect(body.hidden).toBe(1); // 'hidden_person'
110149

111150
const people = body.people as PersonResponseDto[];
112151

113152
expect(people.map((p) => p.id)).toEqual([
114-
multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3
115-
nameBobPerson.id, // name: 'Bob', count: 2
153+
nameBillPersonFavourite.id, // name: 'Bill', count: 2
154+
nameFreddyPersonFavourite.id, // name: 'Freddy', count: 2
116155
nameAlicePerson.id, // name: 'Alice', count: 1
156+
nameBobPerson.id, // name: 'Bob', count: 2
117157
nameCharliePerson.id, // name: 'Charlie', count: 1
158+
multipleAssetsPerson.id, // name: 'multiple_assets_person', count: 3
118159
visiblePerson.id, // name: 'visible_person', count: 1
160+
nameNullPerson4Assets.id, // name: '', count: 4
161+
nameNullPerson3Assets.id, // name: '', count: 3
119162
]);
120163

121164
expect(people.some((p) => p.id === hiddenPerson.id)).toBe(false);
@@ -127,14 +170,18 @@ describe('/people', () => {
127170
expect(status).toBe(200);
128171
expect(body).toEqual({
129172
hasNextPage: false,
130-
total: 7,
173+
total: 11,
131174
hidden: 1,
132175
people: [
133-
expect.objectContaining({ name: 'multiple_assets_person' }),
134-
expect.objectContaining({ name: 'Bob' }),
176+
expect.objectContaining({ name: 'Bill' }),
177+
expect.objectContaining({ name: 'Freddy' }),
135178
expect.objectContaining({ name: 'Alice' }),
179+
expect.objectContaining({ name: 'Bob' }),
136180
expect.objectContaining({ name: 'Charlie' }),
181+
expect.objectContaining({ name: 'multiple_assets_person' }),
137182
expect.objectContaining({ name: 'visible_person' }),
183+
expect.objectContaining({ id: nameNullPerson4Assets.id, name: '' }),
184+
expect.objectContaining({ id: nameNullPerson3Assets.id, name: '' }),
138185
],
139186
});
140187
});
@@ -148,9 +195,9 @@ describe('/people', () => {
148195
expect(status).toBe(200);
149196
expect(body).toEqual({
150197
hasNextPage: true,
151-
total: 7,
198+
total: 11,
152199
hidden: 1,
153-
people: [expect.objectContaining({ name: 'visible_person' })],
200+
people: [expect.objectContaining({ name: 'Charlie' })],
154201
});
155202
});
156203
});

‎server/src/queries/person.repository.sql

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,37 @@ delete from "person"
1212
where
1313
"person"."id" in ($1)
1414

15+
-- PersonRepository.getAllForUser
16+
select
17+
"person".*
18+
from
19+
"person"
20+
inner join "asset_faces" on "asset_faces"."personId" = "person"."id"
21+
inner join "assets" on "asset_faces"."assetId" = "assets"."id"
22+
and "assets"."visibility" = 'timeline'
23+
and "assets"."deletedAt" is null
24+
where
25+
"person"."ownerId" = $1
26+
and "asset_faces"."deletedAt" is null
27+
and "person"."isHidden" = $2
28+
group by
29+
"person"."id"
30+
having
31+
(
32+
"person"."name" != $3
33+
or count("asset_faces"."assetId") >= $4
34+
)
35+
order by
36+
"person"."isHidden" asc,
37+
"person"."isFavorite" desc,
38+
NULLIF(person.name, '') asc nulls last,
39+
count("asset_faces"."assetId") desc,
40+
"person"."createdAt"
41+
limit
42+
$5
43+
offset
44+
$6
45+
1546
-- PersonRepository.getAllWithoutFaces
1647
select
1748
"person".*

‎server/src/repositories/person.repository.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ export class PersonRepository {
138138
.stream();
139139
}
140140

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

0 commit comments

Comments
 (0)