From 2237b28813d4244dc7aaa15d5d93f1d74da44aa1 Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Sat, 22 Aug 2026 08:12:11 +0600 Subject: [PATCH] fix(mobile): permanently delete local copies when moving to the locked folder (#29730) * permanently delete local copies when moving to the locked folder * simplify the delete path and move the stubs into the mock defaults * drop the kept count from the partial message and log the mismatch instead --- i18n/en.json | 2 + mobile/lib/domain/services/asset.service.dart | 6 +-- .../lib/presentation/actions/lock.action.dart | 38 ++++++++++++++-- .../repositories/asset_media.repository.dart | 14 +++--- mobile/test/unit/mocks.dart | 15 ++++++- .../actions/lock_action_test.dart | 31 ++++++++++++- .../unit/services/asset_service_test.dart | 43 ++++++++++++++++++- 7 files changed, 130 insertions(+), 19 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index f0761e209a..1a9d2fa3c7 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -849,6 +849,7 @@ "delete_api_key_prompt": "Are you sure you want to delete this API key?", "delete_dialog_alert": "These items will be permanently deleted from Immich and from your device", "delete_dialog_alert_local": "These items will be permanently removed from your device but still be available on the Immich server", + "delete_dialog_alert_local_ios": "These items will be deleted from Photos, but will still be available on the Immich server. They will be in Recently Deleted for 30 days.", "delete_dialog_alert_local_non_backed_up": "Some of the items aren't backed up to Immich and will be permanently removed from your device", "delete_dialog_title": "Delete Permanently", "delete_duplicates_confirmation": "Are you sure you want to permanently delete these duplicates?", @@ -1482,6 +1483,7 @@ "move_to": "Move to", "move_to_device_trash": "Move to device trash", "move_to_lock_folder_action_prompt": "{count} added to the locked folder", + "move_to_lock_folder_partial_prompt": "{count} added to the locked folder, some local copies were kept", "move_to_locked_folder": "Move to locked folder", "move_to_locked_folder_confirmation": "These photos and video will be removed from all albums, and only viewable from the locked folder", "moved_to_trash": "Moved to trash", diff --git a/mobile/lib/domain/services/asset.service.dart b/mobile/lib/domain/services/asset.service.dart index 0032403348..1693d74990 100644 --- a/mobile/lib/domain/services/asset.service.dart +++ b/mobile/lib/domain/services/asset.service.dart @@ -176,17 +176,17 @@ class AssetService { } } - Future deleteLocal(List localIds) async { + Future deleteLocal(List localIds, {bool trash = true}) async { if (localIds.isEmpty) { return 0; } - final deletedIds = await _mediaRepository.deleteAll(localIds); + final deletedIds = await _mediaRepository.deleteAll(localIds, trash: trash); if (deletedIds.isEmpty) { return 0; } - if (CurrentPlatform.isAndroid && Store.get(StoreKey.manageLocalMediaAndroid, false)) { + if (trash && CurrentPlatform.isAndroid && Store.get(StoreKey.manageLocalMediaAndroid, false)) { await _trashedLocalRepository.applyTrashedAssets(deletedIds); } else { await _localRepository.deleteAssets(deletedIds); diff --git a/mobile/lib/presentation/actions/lock.action.dart b/mobile/lib/presentation/actions/lock.action.dart index dad0d020a9..f9030917ab 100644 --- a/mobile/lib/presentation/actions/lock.action.dart +++ b/mobile/lib/presentation/actions/lock.action.dart @@ -1,12 +1,15 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/enums.dart'; +import 'package:immich_mobile/extensions/platform_extensions.dart'; import 'package:immich_mobile/generated/translations.g.dart'; import 'package:immich_mobile/presentation/actions/action.dart'; import 'package:immich_mobile/providers/infrastructure/asset.provider.dart'; import 'package:immich_mobile/providers/infrastructure/toast.provider.dart'; import 'package:immich_mobile/services/toast.service.dart'; import 'package:immich_mobile/utils/error_handler.dart'; +import 'package:immich_mobile/widgets/common/confirm_dialog.dart'; +import 'package:logging/logging.dart'; typedef _State = ({bool shouldLock, List assetIds, List localIds}); @@ -29,6 +32,8 @@ final _stateProvider = Provider.family.autoDispose<_State?, ActionSource>((ref, class LockAction extends AssetActionBuilder { const LockAction({required super.source}); + static final Logger _log = Logger('LockAction'); + @override ActionItem? create(BuildContext context, WidgetRef ref) { final shouldLock = ref.watch(_stateProvider(source).select((state) => state?.shouldLock)); @@ -50,19 +55,44 @@ class LockAction extends AssetActionBuilder { } final (:shouldLock, :assetIds, :localIds) = state; - final message = shouldLock - ? context.t.move_to_lock_folder_action_prompt(count: assetIds.length) - : context.t.remove_from_lock_folder_action_prompt(count: assetIds.length); + if (shouldLock && localIds.isNotEmpty) { + final confirmed = await showDialog( + context: context, + builder: (_) => ConfirmDialog( + title: context.t.move_to_locked_folder, + content: CurrentPlatform.isAndroid + ? context.t.delete_dialog_alert_local + : context.t.delete_dialog_alert_local_ios, + ok: context.t.confirm, + ), + ); + if (confirmed != true || !context.mounted) { + return; + } + } + final assetService = ref.read(assetServiceProvider); final toastService = ref.read(toastServiceProvider); final clearSelection = ref.read(clearSelectionProvider(source)); try { await assetService.update(assetIds, visibility: .some(shouldLock ? .locked : .timeline)); + var keptCount = 0; if (localIds.isNotEmpty) { // A locked asset still sits in the device gallery, so offer to remove the local copy. - await assetService.deleteLocal(localIds); + keptCount = localIds.length - await assetService.deleteLocal(localIds, trash: false); + if (keptCount != 0) { + _log.warning('Only deleted ${localIds.length - keptCount} of ${localIds.length} local copies'); + } } + if (!context.mounted) { + return; + } + final message = shouldLock + ? keptCount > 0 + ? context.t.move_to_lock_folder_partial_prompt(count: assetIds.length) + : context.t.move_to_lock_folder_action_prompt(count: assetIds.length) + : context.t.remove_from_lock_folder_action_prompt(count: assetIds.length); // Unlocking is a sensitive action and requires an elevated session final toast = shouldLock ? null diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index 6058883544..320a123ddc 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -45,15 +45,11 @@ class AssetMediaRepository { return false; } - Future> deleteAll(List ids) async { - if (CurrentPlatform.isAndroid) { - if (await _androidSupportsTrash()) { - return PhotoManager.editor.android.moveToTrash( - ids.map((e) => AssetEntity(id: e, width: 1, height: 1, typeInt: 0)).toList(), - ); - } else { - return PhotoManager.editor.deleteWithIds(ids); - } + Future> deleteAll(List ids, {bool trash = true}) async { + if (trash && CurrentPlatform.isAndroid && await _androidSupportsTrash()) { + return PhotoManager.editor.android.moveToTrash( + ids.map((e) => AssetEntity(id: e, width: 1, height: 1, typeInt: 0)).toList(), + ); } return PhotoManager.editor.deleteWithIds(ids); } diff --git a/mobile/test/unit/mocks.dart b/mobile/test/unit/mocks.dart index 754c20aa48..e8ef01d3d2 100644 --- a/mobile/test/unit/mocks.dart +++ b/mobile/test/unit/mocks.dart @@ -65,6 +65,7 @@ class RepositoryMocks { _stubAssetApiRepository(); _stubAssetMediaRepository(); _stubDownloadRepository(); + _stubTrashedAssetRepository(); _stubPermissionRepository(); } @@ -86,6 +87,7 @@ class RepositoryMocks { void _stubLocalAssetRepository() { when(localAsset.reconcileHashesFromCloudId).thenAnswer((_) async => {}); when(localAsset.updateHashes).thenAnswer((_) async => {}); + when(localAsset.deleteAssets).thenAnswer((_) async => {}); } void _stubNativeSyncApi() { @@ -97,6 +99,7 @@ class RepositoryMocks { } void _stubAssetMediaRepository() { + when(assetMedia.deleteAll).thenAnswer((inv) async => inv.positionalArguments.first as List); when(assetMedia.shareAssets).thenAnswer((_) async => 1); when(assetMedia.getOriginalFilename).thenAnswer((_) async => null); } @@ -105,6 +108,10 @@ class RepositoryMocks { when(download.downloadAllAssets).thenAnswer((_) async => const []); } + void _stubTrashedAssetRepository() { + when(() => trashedAsset.applyTrashedAssets(any())).thenAnswer((_) async => {}); + } + void _stubPermissionRepository() { when(permission.getStatus).thenAnswer((_) async => DevicePermissionStatus.denied); when(permission.request).thenAnswer((_) async => DevicePermissionStatus.denied); @@ -254,6 +261,9 @@ extension type const LocalAssetRepositoryStub(MockLocalAssetRepository repo) Future Function() get updateHashes => () => repo.updateHashes(any()); + + Future Function() get deleteAssets => + () => repo.deleteAssets(any()); } extension type const RemoteAssetRepositoryStub(MockRemoteAssetRepository repo) @@ -357,7 +367,7 @@ extension type const AssetServiceStub(MockAssetService service) implements Stub< () => service.applyEdits(any(), any()); Future Function() get deleteLocal => - () => service.deleteLocal(any()); + () => service.deleteLocal(any(), trash: any(named: 'trash')); } extension type const RemoteAlbumServiceStub(MockRemoteAlbumService service) implements Stub { @@ -393,6 +403,9 @@ extension type const AssetApiRepositoryStub(MockAssetApiRepository api) implemen } extension type const AssetMediaRepositoryStub(MockAssetMediaRepository api) implements Stub { + Future> Function() get deleteAll => + () => api.deleteAll(any(), trash: any(named: 'trash')); + Future Function() get shareAssets => () => api.shareAssets( any(), diff --git a/mobile/test/unit/presentation/actions/lock_action_test.dart b/mobile/test/unit/presentation/actions/lock_action_test.dart index 9bbbcaed7a..eeb7a752ed 100644 --- a/mobile/test/unit/presentation/actions/lock_action_test.dart +++ b/mobile/test/unit/presentation/actions/lock_action_test.dart @@ -1,8 +1,10 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; +import 'package:immich_mobile/generated/translations.g.dart'; import 'package:immich_mobile/presentation/actions/action.widget.dart'; import 'package:immich_mobile/presentation/actions/lock.action.dart'; +import 'package:immich_mobile/widgets/common/confirm_dialog.dart'; import 'package:immich_ui/immich_ui.dart'; import 'package:mocktail/mocktail.dart'; @@ -29,6 +31,13 @@ void main() { Future pumpLock(WidgetTester tester, Set selection) => tester.pumpTestAction(context, const LockAction(source: .timeline), overrides: context.selected(selection)); + Future respondToDialog(WidgetTester tester, {required bool confirm}) async { + await tester.pumpUntilFound(find.byType(ConfirmDialog)); + expect(find.text(StaticTranslations.instance.move_to_locked_folder), findsOneWidget); + await tester.tap(find.text(confirm ? StaticTranslations.instance.confirm : StaticTranslations.instance.cancel)); + await tester.pumpAndSettle(); + } + group('LockAction', () { testWidgets('locks the eligible owned assets', (tester) async { final asset = owned(); @@ -79,8 +88,28 @@ void main() { final remoteOnly = owned(); await pumpLock(tester, {merged, remoteOnly}); + await respondToDialog(tester, confirm: true); - verify(() => assetService.deleteLocal(['local-1'])).called(1); + verify(() => assetService.deleteLocal(['local-1'], trash: false)).called(1); + }); + + testWidgets('does nothing when the warning is cancelled', (tester) async { + final merged = RemoteAssetFactory.create(ownerId: context.currentUser.id, localId: 'local-1'); + + await pumpLock(tester, {merged}); + await respondToDialog(tester, confirm: false); + + verifyNever(() => assetService.update(any(), visibility: any(named: 'visibility'))); + verifyNever(() => assetService.deleteLocal(any(), trash: any(named: 'trash'))); + }); + + testWidgets('locks remote-only assets without a warning', (tester) async { + final remoteOnly = owned(); + + await pumpLock(tester, {remoteOnly}); + + expect(find.byType(ConfirmDialog), findsNothing); + verify(() => assetService.update([remoteOnly.id], visibility: const .some(.locked))).called(1); }); testWidgets('leaves the local copies alone when unlocking', (tester) async { diff --git a/mobile/test/unit/services/asset_service_test.dart b/mobile/test/unit/services/asset_service_test.dart index b857be1fcb..dc1146da08 100644 --- a/mobile/test/unit/services/asset_service_test.dart +++ b/mobile/test/unit/services/asset_service_test.dart @@ -1,5 +1,13 @@ +import 'package:drift/drift.dart' as drift; +import 'package:drift/native.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/services/asset.service.dart'; +import 'package:immich_mobile/domain/services/store.service.dart'; +import 'package:immich_mobile/entities/store.entity.dart'; +import 'package:immich_mobile/infrastructure/repositories/db.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/store.repository.dart'; import 'package:mocktail/mocktail.dart'; import '../../infrastructure/repository.mock.dart'; @@ -12,6 +20,20 @@ void main() { late MockAssetApiRepository apiRepository; late MockRemoteAssetRepository remoteRepository; late MockRemoteExifRepository exifRepository; + late Drift db; + + setUpAll(() async { + TestWidgetsFlutterBinding.ensureInitialized(); + debugDefaultTargetPlatformOverride = TargetPlatform.android; + db = Drift(drift.DatabaseConnection(NativeDatabase.memory(), closeStreamsSynchronously: true)); + await StoreService.init(storeRepository: StoreRepository(db)); + }); + + tearDownAll(() async { + debugDefaultTargetPlatformOverride = null; + await Store.clear(); + await db.close(); + }); setUp(() { mocks = RepositoryMocks(); @@ -22,13 +44,17 @@ void main() { sut = AssetService( remoteRepository: remoteRepository, exifRepository: exifRepository, - localRepository: MockLocalAssetRepository(), + localRepository: mocks.localAsset.repo, apiRepository: apiRepository, mediaRepository: mocks.assetMedia.api, trashedLocalRepository: mocks.trashedAsset, ); }); + tearDown(() async { + await Store.delete(StoreKey.manageLocalMediaAndroid); + }); + group('AssetService.updateDateTime', () { const ids = ['asset_id_1']; @@ -78,4 +104,19 @@ void main() { verifyZeroInteractions(remoteRepository); }); }); + + group('AssetService.deleteLocal', () { + const ids = ['l1', 'l2']; + + test('permanently deletes local copies without trashing, even when Android trash handling is on', () async { + await Store.put(StoreKey.manageLocalMediaAndroid, true); + + final result = await sut.deleteLocal(ids, trash: false); + + expect(result, ids.length); + verify(() => mocks.assetMedia.api.deleteAll(ids, trash: false)).called(1); + verify(() => mocks.localAsset.repo.deleteAssets(ids)).called(1); + verifyNever(() => mocks.trashedAsset.applyTrashedAssets(any())); + }); + }); }