From d8d64ecc459437040637497a1482cf80dff68e5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= <git@roschaefer.de> Date: Sun, 9 Jun 2024 01:33:39 +0530 Subject: [PATCH] fix: prevent trashing of trashed assets (#10028) * fix: prevent trashing of trashed assets Motivation ---------- This will improve user experience by hiding a pointless action. You can not trash a trashed asset again. It won't get any trashier than it already is. How to test ----------- 1. Visit route `/trash` 2. Click on an asset 3. Press "Delete" on your keyboard 4. Nothing happens 5. Try to find the trash button in the top right 6. You can't find it * refactor: follow @michelheusschen's review See: https://github.com/immich-app/immich/pull/10028#pullrequestreview-2105296755 * refactor: follow @michelheusschen's 2nd review See: https://github.com/immich-app/immich/pull/10028#discussion_r1632057833 --- .../asset-viewer/asset-viewer-nav-bar.spec.ts | 41 +++++++++++++++++++ .../asset-viewer/asset-viewer-nav-bar.svelte | 12 +++--- .../asset-viewer/asset-viewer.svelte | 3 +- .../asset-viewer/delete-button.spec.ts | 34 +++++++++++++++ .../asset-viewer/delete-button.svelte | 27 ++++++++++++ web/src/test-data/factories/user-factory.ts | 20 ++++++++- 6 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts create mode 100644 web/src/lib/components/asset-viewer/delete-button.spec.ts create mode 100644 web/src/lib/components/asset-viewer/delete-button.svelte diff --git a/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts b/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts new file mode 100644 index 0000000000..c82d9f9659 --- /dev/null +++ b/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts @@ -0,0 +1,41 @@ +import { resetSavedUser, user as userStore } from '$lib/stores/user.store'; +import { assetFactory } from '@test-data/factories/asset-factory'; +import { userAdminFactory } from '@test-data/factories/user-factory'; +import '@testing-library/jest-dom'; +import { render } from '@testing-library/svelte'; +import AssetViewerNavBar from './asset-viewer-nav-bar.svelte'; + +describe('AssetViewerNavBar component', () => { + const additionalProps = { + showCopyButton: false, + showZoomButton: false, + showDetailButton: false, + showDownloadButton: false, + showMotionPlayButton: false, + showShareButton: false, + onZoomImage: () => {}, + onCopyImage: () => {}, + }; + + afterEach(() => { + vi.resetAllMocks(); + resetSavedUser(); + }); + + it('shows back button', () => { + const asset = assetFactory.build({ isTrashed: false }); + const { getByTitle } = render(AssetViewerNavBar, { asset, ...additionalProps }); + expect(getByTitle('go_back')).toBeInTheDocument(); + }); + + describe('if the current user owns the asset', () => { + it('shows delete button', () => { + const ownerId = 'id-of-the-user'; + const user = userAdminFactory.build({ id: ownerId }); + const asset = assetFactory.build({ ownerId, isTrashed: false }); + userStore.set(user); + const { getByTitle } = render(AssetViewerNavBar, { asset, ...additionalProps }); + expect(getByTitle('delete')).toBeInTheDocument(); + }); + }); +}); diff --git a/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte b/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte index 2dfb841e93..746871516f 100644 --- a/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte +++ b/web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte @@ -1,5 +1,6 @@ <script lang="ts"> import CircleIconButton from '$lib/components/elements/buttons/circle-icon-button.svelte'; + import DeleteButton from './delete-button.svelte'; import { user } from '$lib/stores/user.store'; import { photoZoomState } from '$lib/stores/zoom-image.store'; import { getAssetJobName } from '$lib/utils'; @@ -16,7 +17,6 @@ mdiCogRefreshOutline, mdiContentCopy, mdiDatabaseRefreshOutline, - mdiDeleteOutline, mdiDotsVertical, mdiFolderDownloadOutline, mdiHeart, @@ -64,6 +64,7 @@ showDetail: void; favorite: void; delete: void; + permanentlyDelete: void; toggleArchive: void; addToAlbum: void; restoreAsset: void; @@ -181,11 +182,10 @@ {/if} {#if isOwner} - <CircleIconButton - color="opaque" - icon={mdiDeleteOutline} - on:click={() => dispatch('delete')} - title={$t('delete')} + <DeleteButton + {asset} + on:delete={() => dispatch('delete')} + on:permanentlyDelete={() => dispatch('permanentlyDelete')} /> <div use:clickOutside={{ diff --git a/web/src/lib/components/asset-viewer/asset-viewer.svelte b/web/src/lib/components/asset-viewer/asset-viewer.svelte index 30e6222f28..3518528986 100644 --- a/web/src/lib/components/asset-viewer/asset-viewer.svelte +++ b/web/src/lib/components/asset-viewer/asset-viewer.svelte @@ -543,7 +543,7 @@ { shortcut: { key: 'ArrowLeft' }, onShortcut: () => navigateAsset('previous') }, { shortcut: { key: 'ArrowRight' }, onShortcut: () => navigateAsset('next') }, { shortcut: { key: 'd', shift: true }, onShortcut: () => downloadFile(asset) }, - { shortcut: { key: 'Delete' }, onShortcut: () => trashOrDelete(false) }, + { shortcut: { key: 'Delete' }, onShortcut: () => trashOrDelete(asset.isTrashed) }, { shortcut: { key: 'Delete', shift: true }, onShortcut: () => trashOrDelete(true) }, { shortcut: { key: 'Escape' }, onShortcut: closeViewer }, { shortcut: { key: 'f' }, onShortcut: toggleFavorite }, @@ -579,6 +579,7 @@ on:showDetail={showDetailInfoHandler} on:download={() => downloadFile(asset)} on:delete={() => trashOrDelete()} + on:permanentlyDelete={() => trashOrDelete(true)} on:favorite={toggleFavorite} on:addToAlbum={() => openAlbumPicker(false)} on:restoreAsset={() => handleRestoreAsset()} diff --git a/web/src/lib/components/asset-viewer/delete-button.spec.ts b/web/src/lib/components/asset-viewer/delete-button.spec.ts new file mode 100644 index 0000000000..7d14a86ab2 --- /dev/null +++ b/web/src/lib/components/asset-viewer/delete-button.spec.ts @@ -0,0 +1,34 @@ +import { type AssetResponseDto } from '@immich/sdk'; + +import { assetFactory } from '@test-data/factories/asset-factory'; +import '@testing-library/jest-dom'; +import { render } from '@testing-library/svelte'; +import DeleteButton from './delete-button.svelte'; + +let asset: AssetResponseDto; + +describe('DeleteButton component', () => { + describe('given an asset which is not trashed yet', () => { + beforeEach(() => { + asset = assetFactory.build({ isTrashed: false }); + }); + + it('displays a button to move the asset to the trash bin', () => { + const { getByTitle, queryByTitle } = render(DeleteButton, { asset }); + expect(getByTitle('delete')).toBeInTheDocument(); + expect(queryByTitle('deletePermanently')).toBeNull(); + }); + }); + + describe('but if the asset is already trashed', () => { + beforeEach(() => { + asset = assetFactory.build({ isTrashed: true }); + }); + + it('displays a button to permanently delete the asset', () => { + const { getByTitle, queryByTitle } = render(DeleteButton, { asset }); + expect(getByTitle('permanently_delete')).toBeInTheDocument(); + expect(queryByTitle('delete')).toBeNull(); + }); + }); +}); diff --git a/web/src/lib/components/asset-viewer/delete-button.svelte b/web/src/lib/components/asset-viewer/delete-button.svelte new file mode 100644 index 0000000000..597092fbc8 --- /dev/null +++ b/web/src/lib/components/asset-viewer/delete-button.svelte @@ -0,0 +1,27 @@ +<script lang="ts"> + import CircleIconButton from '$lib/components/elements/buttons/circle-icon-button.svelte'; + import { createEventDispatcher } from 'svelte'; + import { t } from 'svelte-i18n'; + import { mdiDeleteOutline } from '@mdi/js'; + import { type AssetResponseDto } from '@immich/sdk'; + + export let asset: AssetResponseDto; + + type EventTypes = { + delete: void; + permanentlyDelete: void; + }; + + const dispatch = createEventDispatcher<EventTypes>(); +</script> + +{#if asset.isTrashed} + <CircleIconButton + color="opaque" + icon={mdiDeleteOutline} + on:click={() => dispatch('permanentlyDelete')} + title={$t('permanently_delete')} + /> +{:else} + <CircleIconButton color="opaque" icon={mdiDeleteOutline} on:click={() => dispatch('delete')} title={$t('delete')} /> +{/if} diff --git a/web/src/test-data/factories/user-factory.ts b/web/src/test-data/factories/user-factory.ts index 24d75254d2..67ac638ea0 100644 --- a/web/src/test-data/factories/user-factory.ts +++ b/web/src/test-data/factories/user-factory.ts @@ -1,5 +1,5 @@ import { faker } from '@faker-js/faker'; -import { UserAvatarColor, type UserResponseDto } from '@immich/sdk'; +import { UserAvatarColor, UserStatus, type UserAdminResponseDto, type UserResponseDto } from '@immich/sdk'; import { Sync } from 'factory.ts'; export const userFactory = Sync.makeFactory<UserResponseDto>({ @@ -9,3 +9,21 @@ export const userFactory = Sync.makeFactory<UserResponseDto>({ profileImagePath: '', avatarColor: UserAvatarColor.Primary, }); + +export const userAdminFactory = Sync.makeFactory<UserAdminResponseDto>({ + id: Sync.each(() => faker.string.uuid()), + email: Sync.each(() => faker.internet.email()), + name: Sync.each(() => faker.person.fullName()), + profileImagePath: '', + avatarColor: UserAvatarColor.Primary, + isAdmin: true, + createdAt: Sync.each(() => faker.date.recent().toISOString()), + updatedAt: Sync.each(() => faker.date.recent().toISOString()), + deletedAt: null, + oauthId: '', + quotaUsageInBytes: 0, + quotaSizeInBytes: 1000, + shouldChangePassword: false, + status: UserStatus.Active, + storageLabel: null, +});