From a066b5f301bd17972db47b478f4bf2fc8c7a4992 Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Thu, 20 Aug 2026 13:52:33 +0600 Subject: [PATCH] fix(mobile): order album/place/person timelines by local date (#29338) * fix(mobile): order album/place/person timelines by local date * fold the timeline date ordering into one helper and tighten the tests * drop the redundant none guard from the place asset query * format --- .../repositories/timeline.repository.dart | 43 +++++++--- .../timeline_repository_test.dart | 79 +++++++++++++++++++ mobile/test/medium/repository_context.dart | 3 +- 3 files changed, 112 insertions(+), 13 deletions(-) diff --git a/mobile/lib/infrastructure/repositories/timeline.repository.dart b/mobile/lib/infrastructure/repositories/timeline.repository.dart index 083c88955f5c74..824449e93978a2 100644 --- a/mobile/lib/infrastructure/repositories/timeline.repository.dart +++ b/mobile/lib/infrastructure/repositories/timeline.repository.dart @@ -185,7 +185,8 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor TimelineQuery remoteAlbum(String albumId, GroupAssetsBy groupBy) => ( bucketSource: () => _watchRemoteAlbumBucket(albumId, groupBy: groupBy), - assetSource: (offset, count) => _getRemoteAlbumBucketAssets(albumId, offset: offset, count: count), + assetSource: (offset, count) => + _getRemoteAlbumBucketAssets(albumId, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.remoteAlbum, ); @@ -239,7 +240,12 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor .handleError((error) => const []); } - Future> _getRemoteAlbumBucketAssets(String albumId, {required int offset, required int count}) async { + Future> _getRemoteAlbumBucketAssets( + String albumId, { + required int offset, + required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, + }) async { final albumData = await (_db.remoteAlbumEntity.select()..where((row) => row.id.equals(albumId))).getSingleOrNull(); // If album doesn't exist (was deleted), return empty list @@ -266,11 +272,9 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor ), ])..where(_db.remoteAssetEntity.deletedAt.isNull() & _db.remoteAlbumAssetEntity.albumId.equals(albumId)); - if (isAscending) { - query.orderBy([OrderingTerm.asc(_db.remoteAssetEntity.createdAt)]); - } else { - query.orderBy([OrderingTerm.desc(_db.remoteAssetEntity.createdAt)]); - } + query.orderBy( + _assetDateOrder(groupBy, ascending: isAscending).map((order) => order(_db.remoteAssetEntity)).toList(), + ); query.limit(count, offset: offset); @@ -377,13 +381,14 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor TimelineQuery place(String place, GroupAssetsBy groupBy) => ( bucketSource: () => _watchPlaceBucket(place, groupBy: groupBy), - assetSource: (offset, count) => _getPlaceBucketAssets(place, offset: offset, count: count), + assetSource: (offset, count) => _getPlaceBucketAssets(place, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.place, ); TimelineQuery person(String userId, String personId, GroupAssetsBy groupBy) => ( bucketSource: () => _watchPersonBucket(userId, personId, groupBy: groupBy), - assetSource: (offset, count) => _getPersonBucketAssets(userId, personId, offset: offset, count: count), + assetSource: (offset, count) => + _getPersonBucketAssets(userId, personId, groupBy: groupBy, offset: offset, count: count), origin: TimelineOrigin.person, ); @@ -420,7 +425,12 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor }).watch(); } - Future> _getPlaceBucketAssets(String place, {required int offset, required int count}) { + Future> _getPlaceBucketAssets( + String place, { + required int offset, + required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, + }) { final query = _db.remoteAssetEntity.select().join([ innerJoin( @@ -434,7 +444,7 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor _db.remoteAssetEntity.visibility.equalsValue(AssetVisibility.timeline) & _db.remoteExifEntity.city.equals(place), ) - ..orderBy([OrderingTerm.desc(_db.remoteAssetEntity.createdAt)]) + ..orderBy(_assetDateOrder(groupBy).map((order) => order(_db.remoteAssetEntity)).toList()) ..limit(count, offset: offset); return query.map((row) => row.readTable(_db.remoteAssetEntity).toDto()).get(); } @@ -490,6 +500,7 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor String personId, { required int offset, required int count, + GroupAssetsBy groupBy = GroupAssetsBy.day, }) { final idQuery = _db.assetFaceEntity.selectOnly() ..addColumns([_db.assetFaceEntity.assetId]) @@ -507,7 +518,7 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor row.ownerId.equals(userId) & row.visibility.equalsValue(AssetVisibility.timeline), ) - ..orderBy([(row) => OrderingTerm.desc(row.createdAt)]) + ..orderBy(_assetDateOrder(groupBy)) ..limit(count, offset: offset); return query.map((row) => row.toDto()).get(); @@ -717,6 +728,14 @@ class TimelineRepository extends DatabaseAccessor with $TimelineRepositor List _generateBuckets(int count) => count == 0 ? const [] : [Bucket(assetCount: count)]; +List _assetDateOrder(GroupAssetsBy groupBy, {bool ascending = false}) { + OrderingTerm order(Expression exp) => ascending ? OrderingTerm.asc(exp) : OrderingTerm.desc(exp); + return [ + if (groupBy != GroupAssetsBy.none) (row) => order(row.effectiveCreatedAt(groupBy)), + (row) => order(row.createdAt), + ]; +} + extension on Expression { Expression dateFmt(GroupAssetsBy groupBy, {bool toLocal = false}) { // DateTimes are stored in UTC, so we need to convert them to local time inside the query before formatting diff --git a/mobile/test/medium/repositories/timeline_repository_test.dart b/mobile/test/medium/repositories/timeline_repository_test.dart index bc572b0e2f0729..3de31e96658d90 100644 --- a/mobile/test/medium/repositories/timeline_repository_test.dart +++ b/mobile/test/medium/repositories/timeline_repository_test.dart @@ -46,6 +46,60 @@ void main() { expect((assets.first as RemoteAsset).id, remoteAsset.id); expect([localAsset1.id, localAsset2.id], contains((assets.first as RemoteAsset).localId)); }); + + test('orders shifted album assets in both directions and keeps normal asset order (#28852)', () async { + final user = await ctx.newUser(); + final descendingAlbum = await ctx.newRemoteAlbum(ownerId: user.id, order: .desc); + final ascendingAlbum = await ctx.newRemoteAlbum(ownerId: user.id, order: .asc); + final shiftedLater = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 2, 12), + localDateTime: DateTime.utc(2024, 9, 3, 12), + ); + final shiftedEarlier = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 3, 12), + localDateTime: DateTime.utc(2024, 9, 2, 12), + ); + final normalLater = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 4, 14), + localDateTime: DateTime.utc(2024, 9, 4, 14), + ); + final normalEarlier = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 4, 12), + localDateTime: DateTime.utc(2024, 9, 4, 12), + ); + final seeded = [shiftedLater, shiftedEarlier, normalLater, normalEarlier]; + for (final asset in seeded) { + await ctx.newRemoteAlbumAsset(albumId: descendingAlbum.id, assetId: asset.id); + await ctx.newRemoteAlbumAsset(albumId: ascendingAlbum.id, assetId: asset.id); + } + + final descending = sut.remoteAlbum(descendingAlbum.id, .day); + final ascending = sut.remoteAlbum(ascendingAlbum.id, .day); + + final buckets = await descending.bucketSource().first; + expect(buckets, hasLength(3)); + expect(buckets.map((bucket) => bucket.assetCount), [2, 1, 1]); + + final descendingAssets = await descending.assetSource(0, 10); + expect(descendingAssets.map((asset) => (asset as RemoteAsset).id), [ + normalLater.id, + normalEarlier.id, + shiftedLater.id, + shiftedEarlier.id, + ]); + + final ascendingAssets = await ascending.assetSource(0, 10); + expect(ascendingAssets.map((asset) => (asset as RemoteAsset).id), [ + shiftedEarlier.id, + shiftedLater.id, + normalEarlier.id, + normalLater.id, + ]); + }); }); group('person assets', () { @@ -69,6 +123,31 @@ void main() { expect(assets, hasLength(1)); expect((assets.first as RemoteAsset).id, asset.id); }); + + test('orders shifted person assets by effective date (#28852)', () async { + final user = await ctx.newUser(); + final person = await ctx.newPerson(ownerId: user.id); + final shiftedLater = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 2, 12), + localDateTime: DateTime.utc(2024, 9, 3, 12), + ); + final shiftedEarlier = await ctx.newRemoteAsset( + ownerId: user.id, + createdAt: DateTime.utc(2024, 9, 3, 12), + localDateTime: DateTime.utc(2024, 9, 2, 12), + ); + await ctx.newFace(assetId: shiftedLater.id, personId: person.id); + await ctx.newFace(assetId: shiftedEarlier.id, personId: person.id); + + final query = sut.person(user.id, person.id, .day); + + final buckets = await query.bucketSource().first; + expect(buckets, hasLength(2)); + + final assets = await query.assetSource(0, 10); + expect(assets.map((asset) => (asset as RemoteAsset).id), [shiftedLater.id, shiftedEarlier.id]); + }); }); group('live photos', () { diff --git a/mobile/test/medium/repository_context.dart b/mobile/test/medium/repository_context.dart index 09b8e2c7eb9c0f..c2d7a1f38043f5 100644 --- a/mobile/test/medium/repository_context.dart +++ b/mobile/test/medium/repository_context.dart @@ -121,6 +121,7 @@ class MediumRepositoryContext { String? stackId, String? thumbHash, String? libraryId, + DateTime? localDateTime, }) async { id ??= TestUtils.uuid(); createdAt ??= TestUtils.date(); @@ -144,7 +145,7 @@ class MediumRepositoryContext { isEdited: .new(isEdited ?? false), livePhotoVideoId: .new(livePhotoVideoId), stackId: .new(stackId), - localDateTime: .new(createdAt.toLocal()), + localDateTime: .new(localDateTime ?? createdAt.toLocal()), thumbHash: .new(TestUtils.uuid(thumbHash)), libraryId: .new(TestUtils.uuid(libraryId)), ),