From 81c1a4f1da2e893eb617631fc59d756e4122c771 Mon Sep 17 00:00:00 2001 From: Tom graham Date: Tue, 7 Jan 2025 18:00:35 +1100 Subject: [PATCH 1/5] Initial look at fixing issue where images are uploaded to the wrong album if a shared album conflicts with a local users album. --- mobile/lib/providers/album/album.provider.dart | 6 +++--- mobile/lib/services/album.service.dart | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mobile/lib/providers/album/album.provider.dart b/mobile/lib/providers/album/album.provider.dart index b3d619a81579a..42a87c5176f6e 100644 --- a/mobile/lib/providers/album/album.provider.dart +++ b/mobile/lib/providers/album/album.provider.dart @@ -46,8 +46,8 @@ class AlbumNotifier extends StateNotifier> { ) => _albumService.createAlbum(albumTitle, assets, []); - Future getAlbumByName(String albumName, {bool remoteOnly = false}) => - _albumService.getAlbumByName(albumName, remoteOnly); + Future getAlbumByName(String albumName, {bool? remote, bool? shared}) => + _albumService.getAlbumByName(albumName, remote: remote, shared: shared); /// Create an album on the server with the same name as the selected album for backup /// First this will check if the album already exists on the server with name @@ -55,7 +55,7 @@ class AlbumNotifier extends StateNotifier> { Future createSyncAlbum( String albumName, ) async { - final album = await getAlbumByName(albumName, remoteOnly: true); + final album = await getAlbumByName(albumName, remote: true, shared: false); if (album != null) { return; } diff --git a/mobile/lib/services/album.service.dart b/mobile/lib/services/album.service.dart index 5f013c0e53e5e..ba0eb1d8985c5 100644 --- a/mobile/lib/services/album.service.dart +++ b/mobile/lib/services/album.service.dart @@ -408,8 +408,8 @@ class AlbumService { } } - Future getAlbumByName(String name, bool remoteOnly) => - _albumRepository.getByName(name, remote: remoteOnly ? true : null); + Future getAlbumByName(String name, {bool? remote, bool? shared}) => + _albumRepository.getByName(name, remote: remote, shared: shared); /// /// Add the uploaded asset to the selected albums @@ -419,7 +419,7 @@ class AlbumService { List assetIds, ) async { for (final albumName in albumNames) { - Album? album = await getAlbumByName(albumName, true); + Album? album = await getAlbumByName(albumName, remote: true, shared: false); album ??= await createAlbum(albumName, []); if (album != null && album.remoteId != null) { await _albumApiRepository.addAssets(album.remoteId!, assetIds); From a42911b2901ecc74aed1581a96c56c85af4313d6 Mon Sep 17 00:00:00 2001 From: Tom graham Date: Wed, 8 Jan 2025 15:42:37 +1100 Subject: [PATCH 2/5] Use owner instead of shared flag when fetching albums. --- mobile/lib/interfaces/album.interface.dart | 1 + mobile/lib/providers/album/album.provider.dart | 16 +++++++++++++--- mobile/lib/repositories/album.repository.dart | 7 ++++++- mobile/lib/services/album.service.dart | 18 ++++++++++++++---- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/mobile/lib/interfaces/album.interface.dart b/mobile/lib/interfaces/album.interface.dart index bdf11f18de8ac..cabf2dee53124 100644 --- a/mobile/lib/interfaces/album.interface.dart +++ b/mobile/lib/interfaces/album.interface.dart @@ -13,6 +13,7 @@ abstract interface class IAlbumRepository implements IDatabaseRepository { String name, { bool? shared, bool? remote, + bool? owner, }); Future> getAll({ diff --git a/mobile/lib/providers/album/album.provider.dart b/mobile/lib/providers/album/album.provider.dart index 42a87c5176f6e..8c06faaa6ad27 100644 --- a/mobile/lib/providers/album/album.provider.dart +++ b/mobile/lib/providers/album/album.provider.dart @@ -46,8 +46,18 @@ class AlbumNotifier extends StateNotifier> { ) => _albumService.createAlbum(albumTitle, assets, []); - Future getAlbumByName(String albumName, {bool? remote, bool? shared}) => - _albumService.getAlbumByName(albumName, remote: remote, shared: shared); + Future getAlbumByName( + String albumName, { + bool? remote, + bool? shared, + bool? owner, + }) => + _albumService.getAlbumByName( + albumName, + remote: remote, + shared: shared, + owner: owner, + ); /// Create an album on the server with the same name as the selected album for backup /// First this will check if the album already exists on the server with name @@ -55,7 +65,7 @@ class AlbumNotifier extends StateNotifier> { Future createSyncAlbum( String albumName, ) async { - final album = await getAlbumByName(albumName, remote: true, shared: false); + final album = await getAlbumByName(albumName, remote: true, owner: true); if (album != null) { return; } diff --git a/mobile/lib/repositories/album.repository.dart b/mobile/lib/repositories/album.repository.dart index 2c78e4c2389f1..5016e575b8490 100644 --- a/mobile/lib/repositories/album.repository.dart +++ b/mobile/lib/repositories/album.repository.dart @@ -34,11 +34,16 @@ class AlbumRepository extends DatabaseRepository implements IAlbumRepository { Future create(Album album) => txn(() => db.albums.store(album)); @override - Future getByName(String name, {bool? shared, bool? remote}) { + Future getByName(String name, {bool? shared, bool? remote, bool? owner}) { var query = db.albums.filter().nameEqualTo(name); if (shared != null) { query = query.sharedEqualTo(shared); } + if (owner == true) { + query = query.owner((q) => q.isarIdEqualTo(Store.get(StoreKey.currentUser).isarId)); + } else if (owner == false) { + query = query.owner((q) => q.not().isarIdEqualTo(Store.get(StoreKey.currentUser).isarId)); + } if (remote == true) { query = query.localIdIsNull(); } else if (remote == false) { diff --git a/mobile/lib/services/album.service.dart b/mobile/lib/services/album.service.dart index ba0eb1d8985c5..f36ff7b9f0cce 100644 --- a/mobile/lib/services/album.service.dart +++ b/mobile/lib/services/album.service.dart @@ -212,7 +212,7 @@ class AlbumService { for (int round = 0;; round++) { final proposedName = "$baseName${round == 0 ? "" : " ($round)"}"; - if (null == await _albumRepository.getByName(proposedName)) { + if (null == await _albumRepository.getByName(proposedName, owner: true)) { return proposedName; } } @@ -408,8 +408,18 @@ class AlbumService { } } - Future getAlbumByName(String name, {bool? remote, bool? shared}) => - _albumRepository.getByName(name, remote: remote, shared: shared); + Future getAlbumByName( + String name, { + bool? remote, + bool? shared, + bool? owner, + }) => + _albumRepository.getByName( + name, + remote: remote, + shared: shared, + owner: owner, + ); /// /// Add the uploaded asset to the selected albums @@ -419,7 +429,7 @@ class AlbumService { List assetIds, ) async { for (final albumName in albumNames) { - Album? album = await getAlbumByName(albumName, remote: true, shared: false); + Album? album = await getAlbumByName(albumName, remote: true, owner: true); album ??= await createAlbum(albumName, []); if (album != null && album.remoteId != null) { await _albumApiRepository.addAssets(album.remoteId!, assetIds); From 979ce90abf22c6cd80ff06f61a08d0c03092991b Mon Sep 17 00:00:00 2001 From: Tom graham Date: Wed, 8 Jan 2025 15:45:30 +1100 Subject: [PATCH 3/5] Fix issue with refreshRemoteAlbums getting shared items twice and removed incorrect isShared comment. Using `getAll(shared: true)` gets all shared albums the user can access (regardless of owner, despite the previous comment). Using `getAll(shared: null)` gets all albums (incuding shared = true and shared = false). I presume the intent here was to get albums that were shared (and not mine), and not shared (ie: mine), but the logic is way off. It also just then combines them - so makes more sense to just get them in a single call. --- mobile/lib/services/album.service.dart | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/mobile/lib/services/album.service.dart b/mobile/lib/services/album.service.dart index f36ff7b9f0cce..161ce0bd71322 100644 --- a/mobile/lib/services/album.service.dart +++ b/mobile/lib/services/album.service.dart @@ -157,7 +157,7 @@ class AlbumService { return result; } - /// Checks remote albums (owned if `isShared` is false) for changes, + /// Checks remote albums for changes, /// updates the local database and returns `true` if there were any changes Future refreshRemoteAlbums() async { if (!_remoteCompleter.isCompleted) { @@ -169,18 +169,14 @@ class AlbumService { bool changes = false; try { await _userService.refreshUsers(); - final (sharedAlbum, ownedAlbum) = await ( - _albumApiRepository.getAll(shared: true), - _albumApiRepository.getAll(shared: null) - ).wait; + final allAlbums = await _albumApiRepository.getAll(); final albums = HashSet( equals: (a, b) => a.remoteId == b.remoteId, hashCode: (a) => a.remoteId.hashCode, ); - albums.addAll(sharedAlbum); - albums.addAll(ownedAlbum); + albums.addAll(allAlbums); changes = await _syncService.syncRemoteAlbumsToDb(albums.toList()); } finally { From 3a92fa896e30233848e8518cee005c21a49967f8 Mon Sep 17 00:00:00 2001 From: Tom graham Date: Wed, 8 Jan 2025 15:53:57 +1100 Subject: [PATCH 4/5] Fix formatting. --- mobile/lib/repositories/album.repository.dart | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/mobile/lib/repositories/album.repository.dart b/mobile/lib/repositories/album.repository.dart index 5016e575b8490..adf83b33d432b 100644 --- a/mobile/lib/repositories/album.repository.dart +++ b/mobile/lib/repositories/album.repository.dart @@ -34,15 +34,24 @@ class AlbumRepository extends DatabaseRepository implements IAlbumRepository { Future create(Album album) => txn(() => db.albums.store(album)); @override - Future getByName(String name, {bool? shared, bool? remote, bool? owner}) { + Future getByName( + String name, { + bool? shared, + bool? remote, + bool? owner, + }) { var query = db.albums.filter().nameEqualTo(name); if (shared != null) { query = query.sharedEqualTo(shared); } if (owner == true) { - query = query.owner((q) => q.isarIdEqualTo(Store.get(StoreKey.currentUser).isarId)); + query = query.owner( + (q) => q.isarIdEqualTo(Store.get(StoreKey.currentUser).isarId), + ); } else if (owner == false) { - query = query.owner((q) => q.not().isarIdEqualTo(Store.get(StoreKey.currentUser).isarId)); + query = query.owner( + (q) => q.not().isarIdEqualTo(Store.get(StoreKey.currentUser).isarId), + ); } if (remote == true) { query = query.localIdIsNull(); From c38f5af5acfc38444b00ed716755c241d297216a Mon Sep 17 00:00:00 2001 From: Tom graham Date: Thu, 9 Jan 2025 14:32:28 +1100 Subject: [PATCH 5/5] Fixed tests. --- mobile/test/fixtures/album.stub.dart | 8 ++++++-- mobile/test/services/album.service_test.dart | 10 +++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/mobile/test/fixtures/album.stub.dart b/mobile/test/fixtures/album.stub.dart index e820f193d5450..d57b876bd7999 100644 --- a/mobile/test/fixtures/album.stub.dart +++ b/mobile/test/fixtures/album.stub.dart @@ -26,7 +26,9 @@ final class AlbumStub { shared: true, activityEnabled: false, endDate: DateTime(2020), - )..sharedUsers.addAll([UserStub.admin]); + ) + ..sharedUsers.addAll([UserStub.admin]) + ..owner.value = UserStub.user2; static final oneAsset = Album( name: "album-with-single-asset", @@ -38,7 +40,9 @@ final class AlbumStub { activityEnabled: false, startDate: DateTime(2020), endDate: DateTime(2023), - )..assets.addAll([AssetStub.image1]); + ) + ..assets.addAll([AssetStub.image1]) + ..owner.value = UserStub.user1; static final twoAsset = Album( name: "album-with-two-assets", diff --git a/mobile/test/services/album.service_test.dart b/mobile/test/services/album.service_test.dart index c0775a1c3e070..6decf72e1e82c 100644 --- a/mobile/test/services/album.service_test.dart +++ b/mobile/test/services/album.service_test.dart @@ -84,11 +84,8 @@ void main() { group('refreshRemoteAlbums', () { test('is working', () async { when(() => userService.refreshUsers()).thenAnswer((_) async => true); - when(() => albumApiRepository.getAll(shared: true)) - .thenAnswer((_) async => [AlbumStub.sharedWithUser]); - - when(() => albumApiRepository.getAll(shared: null)) - .thenAnswer((_) async => [AlbumStub.oneAsset, AlbumStub.twoAsset]); + when(() => albumApiRepository.getAll()) + .thenAnswer((_) async => [AlbumStub.oneAsset, AlbumStub.twoAsset, AlbumStub.sharedWithUser]); when( () => syncService.syncRemoteAlbumsToDb([ @@ -100,8 +97,7 @@ void main() { final result = await sut.refreshRemoteAlbums(); expect(result, true); verify(() => userService.refreshUsers()).called(1); - verify(() => albumApiRepository.getAll(shared: true)).called(1); - verify(() => albumApiRepository.getAll(shared: null)).called(1); + verify(() => albumApiRepository.getAll()).called(1); verify( () => syncService.syncRemoteAlbumsToDb( [