diff --git a/e2e/src/specs/server/api/album.e2e-spec.ts b/e2e/src/specs/server/api/album.e2e-spec.ts index e1e5178476..55b9c44b70 100644 --- a/e2e/src/specs/server/api/album.e2e-spec.ts +++ b/e2e/src/specs/server/api/album.e2e-spec.ts @@ -146,7 +146,7 @@ describe('/albums', () => { it('should not return shared albums with a deleted owner', async () => { const { status, body } = await request(app) - .get('/albums?shared=true') + .get('/albums?isShared=true') .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); @@ -188,7 +188,7 @@ describe('/albums', () => { it('should return the album collection including owned and shared', async () => { const { status, body } = await request(app).get('/albums').set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); - expect(body).toHaveLength(4); + expect(body).toHaveLength(5); expect(body).toEqual( expect.arrayContaining([ expect.objectContaining({ @@ -219,13 +219,20 @@ describe('/albums', () => { ]), shared: false, }), + expect.objectContaining({ + albumName: user2SharedUser, + albumUsers: expect.arrayContaining([ + { role: AlbumUserRole.Owner, user: expect.objectContaining({ id: user2.userId }) }, + ]), + shared: true, + }), ]), ); }); - it('should return the album collection filtered by shared', async () => { + it('should return the album collection filtered by isShared', async () => { const { status, body } = await request(app) - .get('/albums?shared=true') + .get('/albums?isShared=true') .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); expect(body).toHaveLength(4); @@ -263,9 +270,9 @@ describe('/albums', () => { ); }); - it('should return the album collection filtered by NOT shared', async () => { + it('should return the album collection filtered by NOT isShared', async () => { const { status, body } = await request(app) - .get('/albums?shared=false') + .get('/albums?isShared=false') .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); expect(body).toHaveLength(1); @@ -282,6 +289,63 @@ describe('/albums', () => { ); }); + it('should return only owned albums when filtered by isOwned=true', async () => { + const { status, body } = await request(app) + .get('/albums?isOwned=true') + .set('Authorization', `Bearer ${user1.accessToken}`); + expect(status).toBe(200); + expect(body).toHaveLength(4); + expect(body).toEqual( + expect.arrayContaining([ + expect.objectContaining({ albumName: user1SharedEditorUser }), + expect.objectContaining({ albumName: user1SharedViewerUser }), + expect.objectContaining({ albumName: user1SharedLink }), + expect.objectContaining({ albumName: user1NotShared }), + ]), + ); + }); + + it('should return only shared-with-me albums when filtered by isOwned=false', async () => { + const { status, body } = await request(app) + .get('/albums?isOwned=false') + .set('Authorization', `Bearer ${user1.accessToken}`); + expect(status).toBe(200); + expect(body).toHaveLength(1); + expect(body).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + albumName: user2SharedUser, + albumUsers: expect.arrayContaining([ + { role: AlbumUserRole.Owner, user: expect.objectContaining({ id: user2.userId }) }, + ]), + }), + ]), + ); + }); + + it('should return owned shared-out albums when filtered by isOwned=true&ishared=true', async () => { + const { status, body } = await request(app) + .get('/albums?isOwned=true&isShared=true') + .set('Authorization', `Bearer ${user1.accessToken}`); + expect(status).toBe(200); + expect(body).toHaveLength(3); + expect(body).toEqual( + expect.arrayContaining([ + expect.objectContaining({ albumName: user1SharedEditorUser }), + expect.objectContaining({ albumName: user1SharedViewerUser }), + expect.objectContaining({ albumName: user1SharedLink }), + ]), + ); + }); + + it('should return empty list when filtered by isOwned=false&isShared=false', async () => { + const { status, body } = await request(app) + .get('/albums?isOwned=false&isShared=false') + .set('Authorization', `Bearer ${user1.accessToken}`); + expect(status).toBe(200); + expect(body).toHaveLength(0); + }); + it('should return the album collection filtered by assetId', async () => { const { status, body } = await request(app) .get(`/albums?assetId=${user1Asset2.id}`) @@ -290,17 +354,17 @@ describe('/albums', () => { expect(body).toHaveLength(2); }); - it('should return the album collection filtered by assetId and ignores shared=true', async () => { + it('should return the album collection filtered by assetId and ignores isShared=true', async () => { const { status, body } = await request(app) - .get(`/albums?shared=true&assetId=${user1Asset1.id}`) + .get(`/albums?isShared=true&assetId=${user1Asset1.id}`) .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); expect(body).toHaveLength(5); }); - it('should return the album collection filtered by assetId and ignores shared=false', async () => { + it('should return the album collection filtered by assetId and ignores isShared=false', async () => { const { status, body } = await request(app) - .get(`/albums?shared=false&assetId=${user1Asset1.id}`) + .get(`/albums?isShared=false&assetId=${user1Asset1.id}`) .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); expect(body).toHaveLength(5); diff --git a/e2e/src/ui/mock-network/base-network.ts b/e2e/src/ui/mock-network/base-network.ts index 3dc3580396..6680b83dd1 100644 --- a/e2e/src/ui/mock-network/base-network.ts +++ b/e2e/src/ui/mock-network/base-network.ts @@ -240,7 +240,8 @@ export const setupBaseMockApiRoutes = async (context: BrowserContext, adminUserI }); }); await context.route('**/api/albums*', async (route, request) => { - if (request.url().endsWith('albums?shared=true') || request.url().endsWith('albums')) { + const url = request.url(); + if (url.endsWith('albums?isShared=true') || url.endsWith('albums?isOwned=true') || url.endsWith('albums')) { return route.fulfill({ status: 200, contentType: 'application/json', diff --git a/mobile/openapi/lib/api/albums_api.dart b/mobile/openapi/lib/api/albums_api.dart index d08d1cba9d..e0fc383c1d 100644 --- a/mobile/openapi/lib/api/albums_api.dart +++ b/mobile/openapi/lib/api/albums_api.dart @@ -506,11 +506,14 @@ class AlbumsApi { /// Parameters: /// /// * [String] assetId: - /// Filter albums containing this asset ID (ignores shared parameter) + /// Filter albums containing this asset ID (ignores other parameters) /// - /// * [bool] shared: - /// Filter by shared status: true = only shared, false = not shared, undefined = all owned albums - Future getAllAlbumsWithHttpInfo({ String? assetId, bool? shared, }) async { + /// * [bool] isOwned: + /// Filter by ownership: true = only owned, false = only shared-with-me, undefined = no filter + /// + /// * [bool] isShared: + /// Filter by shared status: true = only shared, false = not shared, undefined = no filter + Future getAllAlbumsWithHttpInfo({ String? assetId, bool? isOwned, bool? isShared, }) async { // ignore: prefer_const_declarations final apiPath = r'/albums'; @@ -524,8 +527,11 @@ class AlbumsApi { if (assetId != null) { queryParams.addAll(_queryParams('', 'assetId', assetId)); } - if (shared != null) { - queryParams.addAll(_queryParams('', 'shared', shared)); + if (isOwned != null) { + queryParams.addAll(_queryParams('', 'isOwned', isOwned)); + } + if (isShared != null) { + queryParams.addAll(_queryParams('', 'isShared', isShared)); } const contentTypes = []; @@ -549,12 +555,15 @@ class AlbumsApi { /// Parameters: /// /// * [String] assetId: - /// Filter albums containing this asset ID (ignores shared parameter) + /// Filter albums containing this asset ID (ignores other parameters) /// - /// * [bool] shared: - /// Filter by shared status: true = only shared, false = not shared, undefined = all owned albums - Future?> getAllAlbums({ String? assetId, bool? shared, }) async { - final response = await getAllAlbumsWithHttpInfo( assetId: assetId, shared: shared, ); + /// * [bool] isOwned: + /// Filter by ownership: true = only owned, false = only shared-with-me, undefined = no filter + /// + /// * [bool] isShared: + /// Filter by shared status: true = only shared, false = not shared, undefined = no filter + Future?> getAllAlbums({ String? assetId, bool? isOwned, bool? isShared, }) async { + final response = await getAllAlbumsWithHttpInfo( assetId: assetId, isOwned: isOwned, isShared: isShared, ); if (response.statusCode >= HttpStatus.badRequest) { throw ApiException(response.statusCode, await _decodeBodyBytes(response)); } diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 0cb47f4021..960d90d91c 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -1641,7 +1641,7 @@ "name": "assetId", "required": false, "in": "query", - "description": "Filter albums containing this asset ID (ignores shared parameter)", + "description": "Filter albums containing this asset ID (ignores other parameters)", "schema": { "format": "uuid", "pattern": "^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-4[0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12})$", @@ -1649,10 +1649,19 @@ } }, { - "name": "shared", + "name": "isOwned", "required": false, "in": "query", - "description": "Filter by shared status: true = only shared, false = not shared, undefined = all owned albums", + "description": "Filter by ownership: true = only owned, false = only shared-with-me, undefined = no filter", + "schema": { + "type": "boolean" + } + }, + { + "name": "isShared", + "required": false, + "in": "query", + "description": "Filter by shared status: true = only shared, false = not shared, undefined = no filter", "schema": { "type": "boolean" } diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index d2bb6304b1..6b3e9fe26f 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -3657,16 +3657,18 @@ export function getUserStatisticsAdmin({ id, isFavorite, isTrashed, visibility } /** * List all albums */ -export function getAllAlbums({ assetId, shared }: { +export function getAllAlbums({ assetId, isOwned, isShared }: { assetId?: string; - shared?: boolean; + isOwned?: boolean; + isShared?: boolean; }, opts?: Oazapfts.RequestOpts) { return oazapfts.ok(oazapfts.fetchJson<{ status: 200; data: AlbumResponseDto[]; }>(`/albums${QS.query(QS.explode({ assetId, - shared + isOwned, + isShared }))}`, { ...opts })); diff --git a/server/src/controllers/album.controller.spec.ts b/server/src/controllers/album.controller.spec.ts index 0c7a4eb09f..2ab7b08ceb 100644 --- a/server/src/controllers/album.controller.spec.ts +++ b/server/src/controllers/album.controller.spec.ts @@ -25,11 +25,11 @@ describe(AlbumController.name, () => { }); it('should reject an invalid shared param', async () => { - const { status, body } = await request(ctx.getHttpServer()).get('/albums?shared=invalid'); + const { status, body } = await request(ctx.getHttpServer()).get('/albums?isShared=invalid'); expect(status).toEqual(400); expect(body).toEqual( factory.responses.validationError([ - { path: ['shared'], message: 'Invalid option: expected one of "true"|"false"' }, + { path: ['isShared'], message: 'Invalid option: expected one of "true"|"false"' }, ]), ); }); diff --git a/server/src/dtos/album.dto.ts b/server/src/dtos/album.dto.ts index 33870cd6fc..095e399b96 100644 --- a/server/src/dtos/album.dto.ts +++ b/server/src/dtos/album.dto.ts @@ -65,10 +65,13 @@ const UpdateAlbumSchema = z const GetAlbumsSchema = z .object({ - shared: stringToBool + isOwned: stringToBool .optional() - .describe('Filter by shared status: true = only shared, false = not shared, undefined = all owned albums'), - assetId: z.uuidv4().optional().describe('Filter albums containing this asset ID (ignores shared parameter)'), + .describe('Filter by ownership: true = only owned, false = only shared-with-me, undefined = no filter'), + isShared: stringToBool + .optional() + .describe('Filter by shared status: true = only shared, false = not shared, undefined = no filter'), + assetId: z.uuidv4().optional().describe('Filter albums containing this asset ID (ignores other parameters)'), }) .meta({ id: 'GetAlbumsDto' }); diff --git a/server/src/queries/album.repository.sql b/server/src/queries/album.repository.sql index f5bf79ac9e..35c82fdb73 100644 --- a/server/src/queries/album.repository.sql +++ b/server/src/queries/album.repository.sql @@ -185,7 +185,7 @@ where group by "album_asset"."albumId" --- AlbumRepository.getOwned +-- AlbumRepository.getAll select "album".*, ( @@ -242,172 +242,55 @@ from "album" inner join "album_user" on "album_user"."albumId" = "album"."id" and "album_user"."userId" = $2 - and "album_user"."role" = 'owner' where "album"."deletedAt" is null -order by - "album"."createdAt" desc - --- AlbumRepository.getShared -select - "album".*, - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "album_user"."role", - ( - select - to_json(obj) - from - ( - select - "id", - "name", - "email", - "avatarColor", - "profileImagePath", - "profileChangedAt" - from - ( - select - 1 - ) as "dummy" - ) as obj - ) as "user" - from - "album_user" - inner join "user" on "user"."id" = "album_user"."userId" - where - "album_user"."albumId" = "album"."id" - order by - "album_user"."role", - "album_user"."userId" = $1 desc, - "user"."name" asc - ) as agg - ) as "albumUsers", - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "shared_link".* - from - "shared_link" - where - "shared_link"."albumId" = "album"."id" - ) as agg - ) as "sharedLinks" -from - "album" - inner join ( - select - "album_user"."albumId" as "id" - from - "album_user" - where - "album_user"."userId" = $2 - and "album_user"."albumId" in ( - select - "album_user"."albumId" - from - "album_user" - where - "album_user"."role" != 'owner' - ) - union - select - "shared_link"."albumId" as "id" - from - "shared_link" - where - "shared_link"."userId" = $3 - and "shared_link"."albumId" is not null - ) as "matching" on "matching"."id" = "album"."id" - inner join "album_user" on "album_user"."albumId" = "album"."id" and "album_user"."role" = 'owner' -where - "album"."deletedAt" is null -order by - "album"."createdAt" desc - --- AlbumRepository.getNotShared -select - "album".*, - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "shared_link".* - from - "shared_link" - where - "shared_link"."albumId" = "album"."id" - ) as agg - ) as "sharedLinks", - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "album_user"."role", - ( - select - to_json(obj) - from - ( - select - "id", - "name", - "email", - "avatarColor", - "profileImagePath", - "profileChangedAt" - from - ( - select - 1 - ) as "dummy" - ) as obj - ) as "user" - from - "album_user" - inner join "user" on "user"."id" = "album_user"."userId" - where - "album_user"."albumId" = "album"."id" - order by - "album_user"."role", - "album_user"."userId" = $1 desc, - "user"."name" asc - ) as agg - ) as "albumUsers" -from - "album" - inner join "album_user" on "album_user"."albumId" = "album"."id" - and "album_user"."userId" = $2 - and "album_user"."role" = 'owner' -where - "album"."deletedAt" is null - and not exists ( - select - from - "album_user" as "au" - where - "au"."albumId" = "album"."id" - and "au"."role" != 'owner' + and ( + exists ( + select + from + "album_user" as "au" + where + "au"."albumId" = "album"."id" + and "au"."role" != 'owner' + ) + or exists ( + select + from + "shared_link" + where + "shared_link"."albumId" = "album"."id" + ) ) - and not exists ( - select - from - "shared_link" - where - "shared_link"."albumId" = "album"."id" +order by + "album"."createdAt" desc + +-- AlbumRepository.getAllIds +select + "album"."id" +from + "album" + inner join "album_user" on "album_user"."albumId" = "album"."id" + and "album_user"."userId" = $1 +where + "album"."deletedAt" is null + and "album_user"."role" = 'owner' + and ( + exists ( + select + from + "album_user" as "au" + where + "au"."albumId" = "album"."id" + and "au"."role" != 'owner' + ) + or exists ( + select + from + "shared_link" + where + "shared_link"."albumId" = "album"."id" + ) ) order by "album"."createdAt" desc diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index a910673c62..a712151355 100644 --- a/server/src/repositories/album.repository.ts +++ b/server/src/repositories/album.repository.ts @@ -13,7 +13,7 @@ import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres'; import { InjectKysely } from 'nestjs-kysely'; import { columns } from 'src/database'; import { Chunked, ChunkedArray, ChunkedSet, DummyValue, GenerateSql } from 'src/decorators'; -import { AlbumUserCreateDto } from 'src/dtos/album.dto'; +import { AlbumUserCreateDto, MapAlbumDto } from 'src/dtos/album.dto'; import { AlbumUserRole } from 'src/enum'; import { DB } from 'src/schema'; import { AlbumTable } from 'src/schema/tables/album.table'; @@ -183,98 +183,48 @@ export class AlbumRepository { ); } - @GenerateSql({ params: [DummyValue.UUID] }) - getOwned(ownerId: string) { + private buildAlbumBaseQuery(ownerId: string, { isOwned, isShared }: { isOwned?: boolean; isShared?: boolean }) { return this.db .selectFrom('album') - .selectAll('album') .innerJoin('album_user', (join) => - join - .onRef('album_user.albumId', '=', 'album.id') - .on('album_user.userId', '=', ownerId) - .on('album_user.role', '=', sql.lit(AlbumUserRole.Owner)), + join.onRef('album_user.albumId', '=', 'album.id').on('album_user.userId', '=', ownerId), ) .where('album.deletedAt', 'is', null) + .$if(isOwned === true, (qb) => qb.where('album_user.role', '=', sql.lit(AlbumUserRole.Owner))) + .$if(isOwned === false, (qb) => qb.where('album_user.role', '!=', sql.lit(AlbumUserRole.Owner))) + .$if(isShared !== undefined, (qb) => + qb.where((eb) => { + const isSharedAlbum = eb.or([ + eb.exists( + eb + .selectFrom('album_user as au') + .whereRef('au.albumId', '=', 'album.id') + .where('au.role', '!=', sql.lit(AlbumUserRole.Owner)), + ), + eb.exists(eb.selectFrom('shared_link').whereRef('shared_link.albumId', '=', 'album.id')), + ]); + return isShared ? isSharedAlbum : eb.not(isSharedAlbum); + }), + ); + } + + @GenerateSql({ params: [DummyValue.UUID, { isOwned: true, isShared: true }] }) + getAll(ownerId: string, options: { isOwned?: boolean; isShared?: boolean } = {}): Promise { + return this.buildAlbumBaseQuery(ownerId, options) + .selectAll('album') .select(withAlbumUsers(ownerId)) .select(withSharedLink) .orderBy('album.createdAt', 'desc') .execute(); } - /** - * Get albums shared with and shared by owner. - */ - @GenerateSql({ params: [DummyValue.UUID] }) - getShared(ownerId: string) { - return this.db - .selectFrom('album') - .selectAll('album') - .innerJoin( - (eb) => - eb - .selectFrom('album_user') - .select('album_user.albumId as id') - .where('album_user.userId', '=', ownerId) - .where( - 'album_user.albumId', - 'in', - eb - .selectFrom('album_user') - .select('album_user.albumId') - .where('album_user.role', '!=', sql.lit(AlbumUserRole.Owner)), - ) - .union( - eb - .selectFrom('shared_link') - .where('shared_link.userId', '=', ownerId) - .where('shared_link.albumId', 'is not', null) - .select('shared_link.albumId as id') - .$narrowType<{ id: NotNull }>(), - ) - .as('matching'), - (join) => join.onRef('matching.id', '=', 'album.id'), - ) - .innerJoin('album_user', (join) => - join.onRef('album_user.albumId', '=', 'album.id').on('album_user.role', '=', sql.lit(AlbumUserRole.Owner)), - ) - .where('album.deletedAt', 'is', null) - .select(withAlbumUsers(ownerId)) - .select(withSharedLink) - .orderBy('album.createdAt', 'desc') - .execute(); - } - - /** - * Get albums of owner that are _not_ shared - */ - @GenerateSql({ params: [DummyValue.UUID] }) - getNotShared(ownerId: string) { - return this.db - .selectFrom('album') - .selectAll('album') - .innerJoin('album_user', (join) => - join - .onRef('album_user.albumId', '=', 'album.id') - .on('album_user.userId', '=', ownerId) - .on('album_user.role', '=', sql.lit(AlbumUserRole.Owner)), - ) - .where('album.deletedAt', 'is', null) - .where(({ not, exists, selectFrom }) => - not( - exists( - selectFrom('album_user as au') - .whereRef('au.albumId', '=', 'album.id') - .where('au.role', '!=', sql.lit(AlbumUserRole.Owner)), - ), - ), - ) - .where(({ not, exists, selectFrom }) => - not(exists(selectFrom('shared_link').whereRef('shared_link.albumId', '=', 'album.id'))), - ) - .select(withSharedLink) - .select(withAlbumUsers(ownerId)) + @GenerateSql({ params: [DummyValue.UUID, { isOwned: true, isShared: true }] }) + async getAllIds(ownerId: string, options: { isOwned?: boolean; isShared?: boolean } = {}): Promise { + const rows = await this.buildAlbumBaseQuery(ownerId, options) + .select('album.id') .orderBy('album.createdAt', 'desc') .execute(); + return rows.map((r) => r.id); } async restoreAll(userId: string): Promise { diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 24e28a9701..288c3c1d3c 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -26,18 +26,16 @@ describe(AlbumService.name, () => { describe('getStatistics', () => { it('should get the album count', async () => { - mocks.album.getOwned.mockResolvedValue([]); - mocks.album.getShared.mockResolvedValue([]); - mocks.album.getNotShared.mockResolvedValue([]); + mocks.album.getAll.mockResolvedValue([]); await expect(sut.getStatistics(authStub.admin)).resolves.toEqual({ owned: 0, shared: 0, notShared: 0, }); - expect(mocks.album.getOwned).toHaveBeenCalledWith(authStub.admin.user.id); - expect(mocks.album.getShared).toHaveBeenCalledWith(authStub.admin.user.id); - expect(mocks.album.getNotShared).toHaveBeenCalledWith(authStub.admin.user.id); + expect(mocks.album.getAll).toHaveBeenCalledWith(authStub.admin.user.id, { isOwned: true }); + expect(mocks.album.getAll).toHaveBeenCalledWith(authStub.admin.user.id, { isShared: true }); + expect(mocks.album.getAll).toHaveBeenCalledWith(authStub.admin.user.id, { isOwned: true, isShared: false }); }); }); @@ -46,7 +44,7 @@ describe(AlbumService.name, () => { const album = AlbumFactory.from().albumUser().build(); const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; const sharedWithUserAlbum = AlbumFactory.from().owner(owner).albumUser().build(); - mocks.album.getOwned.mockResolvedValue([getForAlbum(album), getForAlbum(sharedWithUserAlbum)]); + mocks.album.getAll.mockResolvedValue([getForAlbum(album), getForAlbum(sharedWithUserAlbum)]); mocks.album.getMetadataForIds.mockResolvedValue([ { albumId: album.id, @@ -68,6 +66,7 @@ describe(AlbumService.name, () => { expect(result).toHaveLength(2); expect(result[0].id).toEqual(album.id); expect(result[1].id).toEqual(sharedWithUserAlbum.id); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, { isOwned: undefined, isShared: undefined }); }); it('gets list of albums that have a specific asset', async () => { @@ -98,7 +97,7 @@ describe(AlbumService.name, () => { it('gets list of albums that are shared', async () => { const album = AlbumFactory.from().albumUser().build(); const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; - mocks.album.getShared.mockResolvedValue([getForAlbum(album)]); + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); mocks.album.getMetadataForIds.mockResolvedValue([ { albumId: album.id, @@ -109,16 +108,16 @@ describe(AlbumService.name, () => { }, ]); - const result = await sut.getAll(AuthFactory.create(owner), { shared: true }); + const result = await sut.getAll(AuthFactory.create(owner), { isShared: true }); expect(result).toHaveLength(1); expect(result[0].id).toEqual(album.id); - expect(mocks.album.getShared).toHaveBeenCalledTimes(1); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, expect.objectContaining({ isShared: true })); }); it('gets list of albums that are NOT shared', async () => { const album = AlbumFactory.create(); const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; - mocks.album.getNotShared.mockResolvedValue([getForAlbum(album)]); + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); mocks.album.getMetadataForIds.mockResolvedValue([ { albumId: album.id, @@ -129,17 +128,66 @@ describe(AlbumService.name, () => { }, ]); - const result = await sut.getAll(AuthFactory.create(owner), { shared: false }); + const result = await sut.getAll(AuthFactory.create(owner), { isShared: false }); expect(result).toHaveLength(1); expect(result[0].id).toEqual(album.id); - expect(mocks.album.getNotShared).toHaveBeenCalledTimes(1); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, expect.objectContaining({ isShared: false })); + }); + + it('gets only owned albums when isOwned=true', async () => { + const album = AlbumFactory.create(); + const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); + mocks.album.getMetadataForIds.mockResolvedValue([ + { albumId: album.id, assetCount: 0, startDate: null, endDate: null, lastModifiedAssetTimestamp: null }, + ]); + + const result = await sut.getAll(AuthFactory.create(owner), { isOwned: true }); + expect(result).toHaveLength(1); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, expect.objectContaining({ isOwned: true })); + }); + + it('gets only shared-with-me albums when isOwned=false', async () => { + const album = AlbumFactory.create(); + const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); + mocks.album.getMetadataForIds.mockResolvedValue([ + { albumId: album.id, assetCount: 0, startDate: null, endDate: null, lastModifiedAssetTimestamp: null }, + ]); + + const result = await sut.getAll(AuthFactory.create(owner), { isOwned: false }); + expect(result).toHaveLength(1); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, expect.objectContaining({ isOwned: false })); + }); + + it('gets owned shared-out albums when isOwned=true and isShared=true', async () => { + const album = AlbumFactory.create(); + const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); + mocks.album.getMetadataForIds.mockResolvedValue([ + { albumId: album.id, assetCount: 0, startDate: null, endDate: null, lastModifiedAssetTimestamp: null }, + ]); + + const result = await sut.getAll(AuthFactory.create(owner), { isOwned: true, isShared: true }); + expect(result).toHaveLength(1); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, { isOwned: true, isShared: true }); + }); + + it('returns empty list when isOwned=false and isShared=false', async () => { + const album = AlbumFactory.create(); + const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + mocks.album.getAll.mockResolvedValue([]); + + const result = await sut.getAll(AuthFactory.create(owner), { isOwned: false, isShared: false }); + expect(result).toHaveLength(0); + expect(mocks.album.getAll).toHaveBeenCalledWith(owner.id, { isOwned: false, isShared: false }); }); }); it('counts assets correctly', async () => { const album = AlbumFactory.create(); const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; - mocks.album.getOwned.mockResolvedValue([getForAlbum(album)]); + mocks.album.getAll.mockResolvedValue([getForAlbum(album)]); mocks.album.getMetadataForIds.mockResolvedValue([ { albumId: album.id, @@ -153,7 +201,7 @@ describe(AlbumService.name, () => { const result = await sut.getAll(AuthFactory.create(owner), {}); expect(result).toHaveLength(1); expect(result[0].assetCount).toEqual(1); - expect(mocks.album.getOwned).toHaveBeenCalledTimes(1); + expect(mocks.album.getAll).toHaveBeenCalledTimes(1); }); describe('create', () => { diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 7bfc0bdcc2..723288e5b5 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -8,7 +8,6 @@ import { CreateAlbumDto, GetAlbumsDto, mapAlbum, - MapAlbumDto, UpdateAlbumDto, UpdateAlbumUserDto, } from 'src/dtos/album.dto'; @@ -26,9 +25,9 @@ import { getPreferences } from 'src/utils/preferences'; export class AlbumService extends BaseService { async getStatistics(auth: AuthDto): Promise { const [owned, shared, notShared] = await Promise.all([ - this.albumRepository.getOwned(auth.user.id), - this.albumRepository.getShared(auth.user.id), - this.albumRepository.getNotShared(auth.user.id), + this.albumRepository.getAll(auth.user.id, { isOwned: true }), + this.albumRepository.getAll(auth.user.id, { isShared: true }), + this.albumRepository.getAll(auth.user.id, { isOwned: true, isShared: false }), ]); return { @@ -38,18 +37,18 @@ export class AlbumService extends BaseService { }; } - async getAll({ user: { id: ownerId } }: AuthDto, { assetId, shared }: GetAlbumsDto): Promise { + async getAll( + { user: { id: ownerId } }: AuthDto, + { assetId, isOwned, isShared }: GetAlbumsDto, + ): Promise { await this.albumRepository.updateThumbnails(); - let albums: MapAlbumDto[]; - if (assetId) { - albums = await this.albumRepository.getByAssetId(ownerId, assetId); - } else if (shared === true) { - albums = await this.albumRepository.getShared(ownerId); - } else if (shared === false) { - albums = await this.albumRepository.getNotShared(ownerId); - } else { - albums = await this.albumRepository.getOwned(ownerId); + const albums = assetId + ? await this.albumRepository.getByAssetId(ownerId, assetId) + : await this.albumRepository.getAll(ownerId, { isOwned, isShared }); + + if (albums.length === 0) { + return []; } // Get asset count for each album. Then map the result to an object: diff --git a/server/src/services/map.service.spec.ts b/server/src/services/map.service.spec.ts index fdf7aee68b..6ef94cd40c 100644 --- a/server/src/services/map.service.spec.ts +++ b/server/src/services/map.service.spec.ts @@ -4,7 +4,7 @@ import { AssetFactory } from 'test/factories/asset.factory'; import { AuthFactory } from 'test/factories/auth.factory'; import { PartnerFactory } from 'test/factories/partner.factory'; import { userStub } from 'test/fixtures/user.stub'; -import { getForAlbum, getForPartner } from 'test/mappers'; +import { getForPartner } from 'test/mappers'; import { newTestService, ServiceMocks } from 'test/utils'; describe(MapService.name, () => { @@ -82,15 +82,15 @@ describe(MapService.name, () => { }; mocks.partner.getAll.mockResolvedValue([]); mocks.map.getMapMarkers.mockResolvedValue([marker]); - mocks.album.getOwned.mockResolvedValue([getForAlbum(AlbumFactory.create())]); - mocks.album.getShared.mockResolvedValue([ - getForAlbum(AlbumFactory.from().albumUser({ userId: userStub.user1.id }).build()), - ]); + const album1 = AlbumFactory.create(); + const album2 = AlbumFactory.from().albumUser({ userId: userStub.user1.id }).build(); + mocks.album.getAllIds.mockResolvedValue([album1.id, album2.id]); const markers = await sut.getMapMarkers(auth, { withSharedAlbums: true }); expect(markers).toHaveLength(1); expect(markers[0]).toEqual(marker); + expect(mocks.album.getAllIds).toHaveBeenCalledWith(auth.user.id); }); }); diff --git a/server/src/services/map.service.ts b/server/src/services/map.service.ts index 94eca77a60..3a825697b4 100644 --- a/server/src/services/map.service.ts +++ b/server/src/services/map.service.ts @@ -13,15 +13,7 @@ export class MapService extends BaseService { userIds.push(...partnerIds); } - // TODO convert to SQL join - const albumIds: string[] = []; - if (options.withSharedAlbums) { - const [ownedAlbums, sharedAlbums] = await Promise.all([ - this.albumRepository.getOwned(auth.user.id), - this.albumRepository.getShared(auth.user.id), - ]); - albumIds.push(...ownedAlbums.map((album) => album.id), ...sharedAlbums.map((album) => album.id)); - } + const albumIds = options.withSharedAlbums ? await this.albumRepository.getAllIds(auth.user.id) : []; return this.mapRepository.getMapMarkers(userIds, albumIds, options); } diff --git a/web/src/lib/modals/AlbumPickerModal.svelte b/web/src/lib/modals/AlbumPickerModal.svelte index 34bf3c0490..5d0d5692da 100644 --- a/web/src/lib/modals/AlbumPickerModal.svelte +++ b/web/src/lib/modals/AlbumPickerModal.svelte @@ -28,11 +28,8 @@ let { onClose }: Props = $props(); onMount(async () => { - // TODO the server should *really* just return all albums (paginated ideally) - const ownedAlbums = await getAllAlbums({ shared: false }); - ownedAlbums.push.apply(ownedAlbums, await getAllAlbums({ shared: true })); - albums = ownedAlbums; - recentAlbums = albums.sort((a, b) => (new Date(a.updatedAt) > new Date(b.updatedAt) ? -1 : 1)).slice(0, 3); + albums = await getAllAlbums({}); + recentAlbums = [...albums].sort((a, b) => (new Date(a.updatedAt) > new Date(b.updatedAt) ? -1 : 1)).slice(0, 3); loading = false; }); diff --git a/web/src/routes/(user)/albums/+page.ts b/web/src/routes/(user)/albums/+page.ts index 8a9a5ef66d..43fe1f712f 100644 --- a/web/src/routes/(user)/albums/+page.ts +++ b/web/src/routes/(user)/albums/+page.ts @@ -5,8 +5,8 @@ import type { PageLoad } from './$types'; export const load = (async ({ url }) => { await authenticate(url); - const sharedAlbums = await getAllAlbums({ shared: true }); - const albums = await getAllAlbums({}); + const sharedAlbums = await getAllAlbums({ isShared: true }); + const albums = await getAllAlbums({ isOwned: true }); const $t = await getFormatter(); return { diff --git a/web/src/routes/(user)/sharing/+page.ts b/web/src/routes/(user)/sharing/+page.ts index 19008038ab..aeb6d431e3 100644 --- a/web/src/routes/(user)/sharing/+page.ts +++ b/web/src/routes/(user)/sharing/+page.ts @@ -5,7 +5,7 @@ import type { PageLoad } from './$types'; export const load = (async ({ url }) => { await authenticate(url); - const sharedAlbums = await getAllAlbums({ shared: true }); + const sharedAlbums = await getAllAlbums({ isShared: true }); const partners = await getPartners({ direction: PartnerDirection.SharedWith }); const $t = await getFormatter();