Skip to content

Commit

Permalink
fix: prevent trashing of trashed assets (#10028)
Browse files Browse the repository at this point in the history
* 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:
#10028 (review)

* refactor:  follow @michelheusschen's 2nd review

See: #10028 (comment)
  • Loading branch information
roschaefer committed Jun 8, 2024
1 parent e1e7de4 commit d8d64ec
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 8 deletions.
41 changes: 41 additions & 0 deletions web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
12 changes: 6 additions & 6 deletions web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -16,7 +17,6 @@
mdiCogRefreshOutline,
mdiContentCopy,
mdiDatabaseRefreshOutline,
mdiDeleteOutline,
mdiDotsVertical,
mdiFolderDownloadOutline,
mdiHeart,
Expand Down Expand Up @@ -64,6 +64,7 @@
showDetail: void;
favorite: void;
delete: void;
permanentlyDelete: void;
toggleArchive: void;
addToAlbum: void;
restoreAsset: void;
Expand Down Expand Up @@ -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={{
Expand Down
3 changes: 2 additions & 1 deletion web/src/lib/components/asset-viewer/asset-viewer.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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()}
Expand Down
34 changes: 34 additions & 0 deletions web/src/lib/components/asset-viewer/delete-button.spec.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
27 changes: 27 additions & 0 deletions web/src/lib/components/asset-viewer/delete-button.svelte
Original file line number Diff line number Diff line change
@@ -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}
20 changes: 19 additions & 1 deletion web/src/test-data/factories/user-factory.ts
Original file line number Diff line number Diff line change
@@ -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>({
Expand All @@ -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,
});

0 comments on commit d8d64ec

Please sign in to comment.