Skip to content

Commit

Permalink
fix(web): sorting options for albums (immich-app#5233)
Browse files Browse the repository at this point in the history 
* fix: albums

* pr feedback

* fix: current behavior

* rename

* fix: album metadatas

* fix: tests

* fix: e2e test

* simplify

* fix: cover shared links

* rename function

* merge main

* merge main

---------

Co-authored-by: Alex Tran <[email protected]>
  • Loading branch information
@martabal @alextran1502
martabal and alextran1502 authored Nov 26, 2023
1 parent c04340c commit 3aa2927
Show file tree
Hide file tree
Showing 9 changed files with 221 additions and 79 deletions.
80 changes: 66 additions & 14 deletions server/src/domain/album/album.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ describe(AlbumService.name, () => {
describe('getAll', () => {
it('gets list of albums for auth user', async () => {
albumMock.getOwned.mockResolvedValue([albumStub.empty, albumStub.sharedWithUser]);
albumMock.getAssetCountForIds.mockResolvedValue([
{ albumId: albumStub.empty.id, assetCount: 0 },
{ albumId: albumStub.sharedWithUser.id, assetCount: 0 },
albumMock.getMetadataForIds.mockResolvedValue([
{ albumId: albumStub.empty.id, assetCount: 0, startDate: undefined, endDate: undefined },
{ albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: undefined, endDate: undefined },
]);
albumMock.getInvalidThumbnail.mockResolvedValue([]);

Expand All @@ -72,7 +72,14 @@ describe(AlbumService.name, () => {

it('gets list of albums that have a specific asset', async () => {
albumMock.getByAssetId.mockResolvedValue([albumStub.oneAsset]);
albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.oneAsset.id, assetCount: 1 }]);
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAsset.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);
albumMock.getInvalidThumbnail.mockResolvedValue([]);

const result = await sut.getAll(authStub.admin, { assetId: albumStub.oneAsset.id });
Expand All @@ -83,7 +90,9 @@ describe(AlbumService.name, () => {

it('gets list of albums that are shared', async () => {
albumMock.getShared.mockResolvedValue([albumStub.sharedWithUser]);
albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.sharedWithUser.id, assetCount: 0 }]);
albumMock.getMetadataForIds.mockResolvedValue([
{ albumId: albumStub.sharedWithUser.id, assetCount: 0, startDate: undefined, endDate: undefined },
]);
albumMock.getInvalidThumbnail.mockResolvedValue([]);

const result = await sut.getAll(authStub.admin, { shared: true });
Expand All @@ -94,7 +103,9 @@ describe(AlbumService.name, () => {

it('gets list of albums that are NOT shared', async () => {
albumMock.getNotShared.mockResolvedValue([albumStub.empty]);
albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.empty.id, assetCount: 0 }]);
albumMock.getMetadataForIds.mockResolvedValue([
{ albumId: albumStub.empty.id, assetCount: 0, startDate: undefined, endDate: undefined },
]);
albumMock.getInvalidThumbnail.mockResolvedValue([]);

const result = await sut.getAll(authStub.admin, { shared: false });
Expand All @@ -106,7 +117,14 @@ describe(AlbumService.name, () => {

it('counts assets correctly', async () => {
albumMock.getOwned.mockResolvedValue([albumStub.oneAsset]);
albumMock.getAssetCountForIds.mockResolvedValue([{ albumId: albumStub.oneAsset.id, assetCount: 1 }]);
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAsset.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);
albumMock.getInvalidThumbnail.mockResolvedValue([]);

const result = await sut.getAll(authStub.admin, {});
Expand All @@ -118,8 +136,13 @@ describe(AlbumService.name, () => {

it('updates the album thumbnail by listing all albums', async () => {
albumMock.getOwned.mockResolvedValue([albumStub.oneAssetInvalidThumbnail]);
albumMock.getAssetCountForIds.mockResolvedValue([
{ albumId: albumStub.oneAssetInvalidThumbnail.id, assetCount: 1 },
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAssetInvalidThumbnail.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);
albumMock.getInvalidThumbnail.mockResolvedValue([albumStub.oneAssetInvalidThumbnail.id]);
albumMock.update.mockResolvedValue(albumStub.oneAssetValidThumbnail);
Expand All @@ -134,8 +157,13 @@ describe(AlbumService.name, () => {

it('removes the thumbnail for an empty album', async () => {
albumMock.getOwned.mockResolvedValue([albumStub.emptyWithInvalidThumbnail]);
albumMock.getAssetCountForIds.mockResolvedValue([
{ albumId: albumStub.emptyWithInvalidThumbnail.id, assetCount: 1 },
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.emptyWithInvalidThumbnail.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);
albumMock.getInvalidThumbnail.mockResolvedValue([albumStub.emptyWithInvalidThumbnail.id]);
albumMock.update.mockResolvedValue(albumStub.emptyWithValidThumbnail);
Expand Down Expand Up @@ -413,10 +441,18 @@ describe(AlbumService.name, () => {
it('should get a shared album', async () => {
albumMock.getById.mockResolvedValue(albumStub.oneAsset);
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.oneAsset.id]));
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAsset.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);

await sut.get(authStub.admin, albumStub.oneAsset.id, {});

expect(albumMock.getById).toHaveBeenCalledWith(albumStub.oneAsset.id, { withAssets: true });
expect(albumMock.getById).toHaveBeenCalledWith(albumStub.oneAsset.id, { withAssets: false });
expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith(
authStub.admin.id,
new Set([albumStub.oneAsset.id]),
Expand All @@ -426,10 +462,18 @@ describe(AlbumService.name, () => {
it('should get a shared album via a shared link', async () => {
albumMock.getById.mockResolvedValue(albumStub.oneAsset);
accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123']));
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAsset.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);

await sut.get(authStub.adminSharedLink, 'album-123', {});

expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true });
expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: false });
expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalledWith(
authStub.adminSharedLink.sharedLinkId,
new Set(['album-123']),
Expand All @@ -439,10 +483,18 @@ describe(AlbumService.name, () => {
it('should get a shared album via shared with user', async () => {
albumMock.getById.mockResolvedValue(albumStub.oneAsset);
accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123']));
albumMock.getMetadataForIds.mockResolvedValue([
{
albumId: albumStub.oneAsset.id,
assetCount: 1,
startDate: new Date('1970-01-01'),
endDate: new Date('1970-01-01'),
},
]);

await sut.get(authStub.user1, 'album-123', {});

expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true });
expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: false });
expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalledWith(authStub.user1.id, new Set(['album-123']));
});

Expand Down
34 changes: 27 additions & 7 deletions server/src/domain/album/album.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { AuthUserDto } from '../auth';
import { setUnion } from '../domain.util';
import { JobName } from '../job';
import {
AlbumAssetCount,
AlbumInfoOptions,
IAccessRepository,
IAlbumRepository,
Expand Down Expand Up @@ -69,19 +70,29 @@ export class AlbumService {

// Get asset count for each album. Then map the result to an object:
// { [albumId]: assetCount }
const albumsAssetCount = await this.albumRepository.getAssetCountForIds(albums.map((album) => album.id));
const albumsAssetCountObj = albumsAssetCount.reduce((obj: Record<string, number>, { albumId, assetCount }) => {
obj[albumId] = assetCount;
return obj;
}, {});
const albumMetadataForIds = await this.albumRepository.getMetadataForIds(albums.map((album) => album.id));
const albumMetadataForIdsObj: Record<string, AlbumAssetCount> = albumMetadataForIds.reduce(
(obj: Record<string, AlbumAssetCount>, { albumId, assetCount, startDate, endDate }) => {
obj[albumId] = {
albumId,
assetCount,
startDate,
endDate,
};
return obj;
},
{},
);

return Promise.all(
albums.map(async (album) => {
const lastModifiedAsset = await this.assetRepository.getLastUpdatedAssetForAlbumId(album.id);
return {
...mapAlbumWithoutAssets(album),
sharedLinks: undefined,
assetCount: albumsAssetCountObj[album.id],
startDate: albumMetadataForIdsObj[album.id].startDate,
endDate: albumMetadataForIdsObj[album.id].endDate,
assetCount: albumMetadataForIdsObj[album.id].assetCount,
lastModifiedAssetTimestamp: lastModifiedAsset?.fileModifiedAt,
};
}),
Expand All @@ -91,7 +102,16 @@ export class AlbumService {
async get(authUser: AuthUserDto, id: string, dto: AlbumInfoDto): Promise<AlbumResponseDto> {
await this.access.requirePermission(authUser, Permission.ALBUM_READ, id);
await this.albumRepository.updateThumbnails();
return mapAlbum(await this.findOrFail(id, { withAssets: true }), !dto.withoutAssets);
const withAssets = dto.withoutAssets === undefined ? false : !dto.withoutAssets;
const album = await this.findOrFail(id, { withAssets });
const [albumMetadataForIds] = await this.albumRepository.getMetadataForIds([album.id]);

return {
...mapAlbum(album, withAssets),
startDate: albumMetadataForIds.startDate,
endDate: albumMetadataForIds.endDate,
assetCount: albumMetadataForIds.assetCount,
};
}

async create(authUser: AuthUserDto, dto: CreateAlbumDto): Promise<AlbumResponseDto> {
Expand Down
4 changes: 3 additions & 1 deletion server/src/domain/repositories/album.repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export const IAlbumRepository = 'IAlbumRepository';
export interface AlbumAssetCount {
albumId: string;
assetCount: number;
startDate: Date | undefined;
endDate: Date | undefined;
}

export interface AlbumInfoOptions {
Expand All @@ -30,7 +32,7 @@ export interface IAlbumRepository {
hasAsset(asset: AlbumAsset): Promise<boolean>;
removeAsset(assetId: string): Promise<void>;
removeAssets(assets: AlbumAssets): Promise<void>;
getAssetCountForIds(ids: string[]): Promise<AlbumAssetCount[]>;
getMetadataForIds(ids: string[]): Promise<AlbumAssetCount[]>;
getInvalidThumbnail(): Promise<string[]>;
getOwned(ownerId: string): Promise<AlbumEntity[]>;
getShared(ownerId: string): Promise<AlbumEntity[]>;
Expand Down
19 changes: 12 additions & 7 deletions server/src/infra/repositories/album.repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,30 @@ export class AlbumRepository implements IAlbumRepository {
});
}

async getAssetCountForIds(ids: string[]): Promise<AlbumAssetCount[]> {
async getMetadataForIds(ids: string[]): Promise<AlbumAssetCount[]> {
// Guard against running invalid query when ids list is empty.
if (!ids.length) {
return [];
}

// Only possible with query builder because of GROUP BY.
const countByAlbums = await this.repository
const albumMetadatas = await this.repository
.createQueryBuilder('album')
.select('album.id')
.addSelect('COUNT(albums_assets.assetsId)', 'asset_count')
.leftJoin('albums_assets_assets', 'albums_assets', 'albums_assets.albumsId = album.id')
.addSelect('MIN(assets.fileCreatedAt)', 'start_date')
.addSelect('MAX(assets.fileCreatedAt)', 'end_date')
.addSelect('COUNT(album_assets.assetsId)', 'asset_count')
.leftJoin('albums_assets_assets', 'album_assets', 'album_assets.albumsId = album.id')
.leftJoin('assets', 'assets', 'assets.id = album_assets.assetsId')
.where('album.id IN (:...ids)', { ids })
.groupBy('album.id')
.getRawMany();

return countByAlbums.map<AlbumAssetCount>((albumCount) => ({
albumId: albumCount['album_id'],
assetCount: Number(albumCount['asset_count']),
return albumMetadatas.map<AlbumAssetCount>((metadatas) => ({
albumId: metadatas['album_id'],
assetCount: Number(metadatas['asset_count']),
startDate: metadatas['end_date'] ? new Date(metadatas['start_date']) : undefined,
endDate: metadatas['end_date'] ? new Date(metadatas['end_date']) : undefined,
}));
}

Expand Down
4 changes: 2 additions & 2 deletions server/test/e2e/album.e2e-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ describe(`${AlbumController.name} (e2e)`, () => {

it('should return album info for own album', async () => {
const { status, body } = await request(server)
.get(`/album/${user1Albums[0].id}`)
.get(`/album/${user1Albums[0].id}?withoutAssets=false`)
.set('Authorization', `Bearer ${user1.accessToken}`);

expect(status).toBe(200);
Expand All @@ -255,7 +255,7 @@ describe(`${AlbumController.name} (e2e)`, () => {

it('should return album info for shared album', async () => {
const { status, body } = await request(server)
.get(`/album/${user2Albums[0].id}`)
.get(`/album/${user2Albums[0].id}?withoutAssets=false`)
.set('Authorization', `Bearer ${user1.accessToken}`);

expect(status).toBe(200);
Expand Down
2 changes: 1 addition & 1 deletion server/test/repositories/album.repository.mock.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const newAlbumRepositoryMock = (): jest.Mocked<IAlbumRepository> => {
getById: jest.fn(),
getByIds: jest.fn(),
getByAssetId: jest.fn(),
getAssetCountForIds: jest.fn(),
getMetadataForIds: jest.fn(),
getInvalidThumbnail: jest.fn(),
getOwned: jest.fn(),
getShared: jest.fn(),
Expand Down
8 changes: 4 additions & 4 deletions web/src/lib/components/elements/table-header.svelte
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
export let option: Sort;
const handleSort = () => {
if (albumViewSettings === option.sortTitle) {
if (albumViewSettings === option.title) {
option.sortDesc = !option.sortDesc;
} else {
albumViewSettings = option.sortTitle;
albumViewSettings = option.title;
}
};
</script>
Expand All @@ -18,12 +18,12 @@
class="rounded-lg p-2 hover:bg-immich-dark-primary hover:dark:bg-immich-dark-primary/50"
on:click={() => handleSort()}
>
{#if albumViewSettings === option.sortTitle}
{#if albumViewSettings === option.title}
{#if option.sortDesc}
&#8595;
{:else}
&#8593;
{/if}
{/if}{option.table}</button
{/if}{option.title}</button
></th
>
27 changes: 19 additions & 8 deletions web/src/lib/components/sharedlinks-page/shared-link-card.svelte
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@
import { createEventDispatcher } from 'svelte';
import { goto } from '$app/navigation';
import { mdiCircleEditOutline, mdiContentCopy, mdiDelete, mdiOpenInNew } from '@mdi/js';
import noThumbnailUrl from '$lib/assets/no-thumbnail.png';
export let link: SharedLinkResponseDto;
let expirationCountdown: luxon.DurationObjectUnits;
const dispatch = createEventDispatcher();
const getAssetInfo = async (): Promise<AssetResponseDto> => {
const getThumbnail = async (): Promise<AssetResponseDto> => {
let assetId = '';
if (link.album?.albumThumbnailAssetId) {
Expand Down Expand Up @@ -60,18 +61,28 @@
class="flex w-full gap-4 border-b border-gray-200 py-4 transition-all hover:border-immich-primary dark:border-gray-600 dark:text-immich-gray dark:hover:border-immich-dark-primary"
>
<div>
{#await getAssetInfo()}
<LoadingSpinner />
{:then asset}
{#if link?.album?.albumThumbnailAssetId || link.assets.length > 0}
{#await getThumbnail()}
<LoadingSpinner />
{:then asset}
<img
id={asset.id}
src={api.getAssetThumbnailUrl(asset.id, ThumbnailFormat.Webp)}
alt={asset.id}
class="h-[100px] w-[100px] rounded-lg object-cover"
loading="lazy"
draggable="false"
/>
{/await}
{:else}
<img
id={asset.id}
src={api.getAssetThumbnailUrl(asset.id, ThumbnailFormat.Webp)}
alt={asset.id}
src={noThumbnailUrl}
alt={'Album without assets'}
class="h-[100px] w-[100px] rounded-lg object-cover"
loading="lazy"
draggable="false"
/>
{/await}
{/if}
</div>

<div class="flex flex-col justify-between">
Expand Down
Loading

0 comments on commit 3aa2927

Please sign in to comment.