From 6a40aa83b723da300c453b531a583b4cda05fb28 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Mon, 17 Mar 2025 15:32:12 -0400 Subject: [PATCH] refactor: better types for getList and getDeletedAfter (#16926) --- server/src/database.ts | 38 +++- server/src/dtos/user.dto.ts | 23 +- server/src/queries/user.repository.sql | 205 +++++++++++++----- .../src/repositories/activity.repository.ts | 2 +- server/src/repositories/partner.repository.ts | 7 +- server/src/repositories/user.repository.ts | 55 ++--- server/src/services/partner.service.ts | 4 +- server/src/services/user.service.spec.ts | 95 ++++---- server/src/services/user.service.ts | 10 +- server/test/factory.ts | 3 + server/test/fixtures/system-config.stub.ts | 5 - server/test/medium/specs/user.service.spec.ts | 64 +++++- server/test/small.factory.ts | 25 ++- 13 files changed, 342 insertions(+), 194 deletions(-) diff --git a/server/src/database.ts b/server/src/database.ts index 9caa0c196e..7fd791c59c 100644 --- a/server/src/database.ts +++ b/server/src/database.ts @@ -1,5 +1,5 @@ -import { sql } from 'kysely'; -import { AssetStatus, AssetType, Permission } from 'src/enum'; +import { UserMetadataEntity } from 'src/entities/user-metadata.entity'; +import { AssetStatus, AssetType, Permission, UserStatus } from 'src/enum'; export type AuthUser = { id: string; @@ -46,6 +46,20 @@ export type User = { profileChangedAt: Date; }; +export type UserAdmin = User & { + storageLabel: string | null; + shouldChangePassword: boolean; + isAdmin: boolean; + createdAt: Date; + updatedAt: Date; + deletedAt: Date | null; + oauthId: string; + quotaSizeInBytes: number | null; + quotaUsageInBytes: number; + status: UserStatus; + metadata: UserMetadataEntity[]; +}; + export type Asset = { createdAt: Date; updatedAt: Date; @@ -103,9 +117,9 @@ export type Partner = { inTimeline: boolean; }; +const userColumns = ['id', 'name', 'email', 'profileImagePath', 'profileChangedAt'] as const; + export const columns = { - ackEpoch: (columnName: 'createdAt' | 'updatedAt' | 'deletedAt') => - sql.raw(`extract(epoch from "${columnName}")::text`).as('ackEpoch'), authUser: [ 'users.id', 'users.name', @@ -125,7 +139,21 @@ export const columns = { 'shared_links.allowDownload', 'shared_links.password', ], - userDto: ['id', 'name', 'email', 'profileImagePath', 'profileChangedAt'], + user: userColumns, + userAdmin: [ + ...userColumns, + 'createdAt', + 'updatedAt', + 'deletedAt', + 'isAdmin', + 'status', + 'oauthId', + 'profileImagePath', + 'shouldChangePassword', + 'storageLabel', + 'quotaSizeInBytes', + 'quotaUsageInBytes', + ], tagDto: ['id', 'value', 'createdAt', 'updatedAt', 'color', 'parentId'], apiKey: ['id', 'name', 'userId', 'createdAt', 'updatedAt', 'permissions'], syncAsset: [ diff --git a/server/src/dtos/user.dto.ts b/server/src/dtos/user.dto.ts index 5b7784aa3d..0177e9b475 100644 --- a/server/src/dtos/user.dto.ts +++ b/server/src/dtos/user.dto.ts @@ -1,8 +1,8 @@ import { ApiProperty } from '@nestjs/swagger'; import { Transform } from 'class-transformer'; import { IsBoolean, IsEmail, IsNotEmpty, IsNumber, IsPositive, IsString } from 'class-validator'; -import { User } from 'src/database'; -import { UserMetadataEntity } from 'src/entities/user-metadata.entity'; +import { User, UserAdmin } from 'src/database'; +import { UserMetadataEntity, UserMetadataItem } from 'src/entities/user-metadata.entity'; import { UserEntity } from 'src/entities/user.entity'; import { UserAvatarColor, UserMetadataKey, UserStatus } from 'src/enum'; import { getPreferences } from 'src/utils/preferences'; @@ -42,28 +42,17 @@ export class UserLicense { activatedAt!: Date; } -export const mapUser = (entity: UserEntity): UserResponseDto => { +export const mapUser = (entity: UserEntity | User): UserResponseDto => { return { id: entity.id, email: entity.email, name: entity.name, profileImagePath: entity.profileImagePath, - avatarColor: getPreferences(entity.email, entity.metadata || []).avatar.color, + avatarColor: getPreferences(entity.email, (entity as UserEntity).metadata || []).avatar.color, profileChangedAt: entity.profileChangedAt, }; }; -export const mapDatabaseUser = (user: User): UserResponseDto => { - return { - id: user.id, - email: user.email, - name: user.name, - profileImagePath: user.profileImagePath, - avatarColor: getPreferences(user.email, []).avatar.color, - profileChangedAt: user.profileChangedAt, - }; -}; - export class UserAdminSearchDto { @ValidateBoolean({ optional: true }) withDeleted?: boolean; @@ -153,8 +142,8 @@ export class UserAdminResponseDto extends UserResponseDto { license!: UserLicense | null; } -export function mapUserAdmin(entity: UserEntity): UserAdminResponseDto { - const license = entity.metadata?.find( +export function mapUserAdmin(entity: UserEntity | UserAdmin): UserAdminResponseDto { + const license = (entity.metadata as UserMetadataItem[])?.find( (item): item is UserMetadataEntity => item.key === UserMetadataKey.LICENSE, )?.value; return { diff --git a/server/src/queries/user.repository.sql b/server/src/queries/user.repository.sql index 7ae8003a09..a726ab46e9 100644 --- a/server/src/queries/user.repository.sql +++ b/server/src/queries/user.repository.sql @@ -3,20 +3,21 @@ -- UserRepository.get select "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", "name", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", + "status", + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", "quotaSizeInBytes", "quotaUsageInBytes", - "status", - "profileChangedAt", ( select coalesce(json_agg(agg), '[]') @@ -39,20 +40,21 @@ where -- UserRepository.getAdmin select "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", "name", - "quotaSizeInBytes", - "quotaUsageInBytes", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", "status", - "profileChangedAt" + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes" from "users" where @@ -71,20 +73,21 @@ where -- UserRepository.getByEmail select "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", "name", - "quotaSizeInBytes", - "quotaUsageInBytes", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", "status", - "profileChangedAt" + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes" from "users" where @@ -94,20 +97,21 @@ where -- UserRepository.getByStorageLabel select "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", "name", - "quotaSizeInBytes", - "quotaUsageInBytes", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", "status", - "profileChangedAt" + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes" from "users" where @@ -117,26 +121,109 @@ where -- UserRepository.getByOAuthId select "id", - "email", - "createdAt", - "profileImagePath", - "isAdmin", - "shouldChangePassword", - "deletedAt", - "oauthId", - "updatedAt", - "storageLabel", "name", - "quotaSizeInBytes", - "quotaUsageInBytes", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", "status", - "profileChangedAt" + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes" from "users" where "users"."oauthId" = $1 and "users"."deletedAt" is null +-- UserRepository.getDeletedAfter +select + "id" +from + "users" +where + "users"."deletedAt" < $1 + +-- UserRepository.getList (with deleted) +select + "id", + "name", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", + "status", + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes", + ( + select + coalesce(json_agg(agg), '[]') + from + ( + select + "user_metadata".* + from + "user_metadata" + where + "users"."id" = "user_metadata"."userId" + ) as agg + ) as "metadata" +from + "users" +order by + "createdAt" desc + +-- UserRepository.getList (without deleted) +select + "id", + "name", + "email", + "profileImagePath", + "profileChangedAt", + "createdAt", + "updatedAt", + "deletedAt", + "isAdmin", + "status", + "oauthId", + "profileImagePath", + "shouldChangePassword", + "storageLabel", + "quotaSizeInBytes", + "quotaUsageInBytes", + ( + select + coalesce(json_agg(agg), '[]') + from + ( + select + "user_metadata".* + from + "user_metadata" + where + "users"."id" = "user_metadata"."userId" + ) as agg + ) as "metadata" +from + "users" +where + "users"."deletedAt" is null +order by + "createdAt" desc + -- UserRepository.getUserStats select "users"."id" as "userId", diff --git a/server/src/repositories/activity.repository.ts b/server/src/repositories/activity.repository.ts index d998fea23c..48def82f49 100644 --- a/server/src/repositories/activity.repository.ts +++ b/server/src/repositories/activity.repository.ts @@ -18,7 +18,7 @@ const withUser = (eb: ExpressionBuilder) => { return jsonObjectFrom( eb .selectFrom('users') - .select(columns.userDto) + .select(columns.user) .whereRef('users.id', '=', 'activity.userId') .where('users.deletedAt', 'is', null), ).as('user'); diff --git a/server/src/repositories/partner.repository.ts b/server/src/repositories/partner.repository.ts index 8877185d31..489793aa77 100644 --- a/server/src/repositories/partner.repository.ts +++ b/server/src/repositories/partner.repository.ts @@ -18,16 +18,13 @@ export enum PartnerDirection { const withSharedBy = (eb: ExpressionBuilder) => { return jsonObjectFrom( - eb.selectFrom('users as sharedBy').select(columns.userDto).whereRef('sharedBy.id', '=', 'partners.sharedById'), + eb.selectFrom('users as sharedBy').select(columns.user).whereRef('sharedBy.id', '=', 'partners.sharedById'), ).as('sharedBy'); }; const withSharedWith = (eb: ExpressionBuilder) => { return jsonObjectFrom( - eb - .selectFrom('users as sharedWith') - .select(columns.userDto) - .whereRef('sharedWith.id', '=', 'partners.sharedWithId'), + eb.selectFrom('users as sharedWith').select(columns.user).whereRef('sharedWith.id', '=', 'partners.sharedWithId'), ).as('sharedWith'); }; diff --git a/server/src/repositories/user.repository.ts b/server/src/repositories/user.repository.ts index 1387828be6..055df9dfdc 100644 --- a/server/src/repositories/user.repository.ts +++ b/server/src/repositories/user.repository.ts @@ -1,6 +1,8 @@ import { Injectable } from '@nestjs/common'; import { Insertable, Kysely, sql, Updateable } from 'kysely'; +import { DateTime } from 'luxon'; import { InjectKysely } from 'nestjs-kysely'; +import { columns, UserAdmin } from 'src/database'; import { DB, UserMetadata as DbUserMetadata, Users } from 'src/db'; import { DummyValue, GenerateSql } from 'src/decorators'; import { UserMetadata, UserMetadataItem } from 'src/entities/user-metadata.entity'; @@ -8,24 +10,6 @@ import { UserEntity, withMetadata } from 'src/entities/user.entity'; import { AssetType, UserStatus } from 'src/enum'; import { asUuid } from 'src/utils/database'; -const columns = [ - 'id', - 'email', - 'createdAt', - 'profileImagePath', - 'isAdmin', - 'shouldChangePassword', - 'deletedAt', - 'oauthId', - 'updatedAt', - 'storageLabel', - 'name', - 'quotaSizeInBytes', - 'quotaUsageInBytes', - 'status', - 'profileChangedAt', -] as const; - type Upsert = Insertable; export interface UserListFilter { @@ -57,7 +41,7 @@ export class UserRepository { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .select(withMetadata) .where('users.id', '=', userId) .$if(!options.withDeleted, (eb) => eb.where('users.deletedAt', 'is', null)) @@ -76,7 +60,7 @@ export class UserRepository { getAdmin(): Promise { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .where('users.isAdmin', '=', true) .where('users.deletedAt', 'is', null) .executeTakeFirst() as Promise; @@ -98,7 +82,7 @@ export class UserRepository { getByEmail(email: string, withPassword?: boolean): Promise { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .$if(!!withPassword, (eb) => eb.select('password')) .where('email', '=', email) .where('users.deletedAt', 'is', null) @@ -109,7 +93,7 @@ export class UserRepository { getByStorageLabel(storageLabel: string): Promise { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .where('users.storageLabel', '=', storageLabel) .where('users.deletedAt', 'is', null) .executeTakeFirst() as Promise; @@ -119,35 +103,36 @@ export class UserRepository { getByOAuthId(oauthId: string): Promise { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .where('users.oauthId', '=', oauthId) .where('users.deletedAt', 'is', null) .executeTakeFirst() as Promise; } - getDeletedUsers(): Promise { - return this.db - .selectFrom('users') - .select(columns) - .where('users.deletedAt', 'is not', null) - .execute() as unknown as Promise; + @GenerateSql({ params: [DateTime.now().minus({ years: 1 })] }) + getDeletedAfter(target: DateTime) { + return this.db.selectFrom('users').select(['id']).where('users.deletedAt', '<', target.toJSDate()).execute(); } - getList({ withDeleted }: UserListFilter = {}): Promise { + @GenerateSql( + { name: 'with deleted', params: [{ withDeleted: true }] }, + { name: 'without deleted', params: [{ withDeleted: false }] }, + ) + getList({ withDeleted }: UserListFilter = {}) { return this.db .selectFrom('users') - .select(columns) + .select(columns.userAdmin) .select(withMetadata) .$if(!withDeleted, (eb) => eb.where('users.deletedAt', 'is', null)) .orderBy('createdAt', 'desc') - .execute() as unknown as Promise; + .execute() as Promise; } async create(dto: Insertable): Promise { return this.db .insertInto('users') .values(dto) - .returning(columns) + .returning(columns.userAdmin) .executeTakeFirst() as unknown as Promise; } @@ -157,7 +142,7 @@ export class UserRepository { .set(dto) .where('users.id', '=', asUuid(id)) .where('users.deletedAt', 'is', null) - .returning(columns) + .returning(columns.userAdmin) .returning(withMetadata) .executeTakeFirst() as unknown as Promise; } @@ -167,7 +152,7 @@ export class UserRepository { .updateTable('users') .set({ status: UserStatus.ACTIVE, deletedAt: null }) .where('users.id', '=', asUuid(id)) - .returning(columns) + .returning(columns.userAdmin) .returning(withMetadata) .executeTakeFirst() as unknown as Promise; } diff --git a/server/src/services/partner.service.ts b/server/src/services/partner.service.ts index 28ceab470e..3723634948 100644 --- a/server/src/services/partner.service.ts +++ b/server/src/services/partner.service.ts @@ -2,7 +2,7 @@ import { BadRequestException, Injectable } from '@nestjs/common'; import { Partner } from 'src/database'; import { AuthDto } from 'src/dtos/auth.dto'; import { PartnerResponseDto, PartnerSearchDto, UpdatePartnerDto } from 'src/dtos/partner.dto'; -import { mapDatabaseUser } from 'src/dtos/user.dto'; +import { mapUser } from 'src/dtos/user.dto'; import { Permission } from 'src/enum'; import { PartnerDirection, PartnerIds } from 'src/repositories/partner.repository'; import { BaseService } from 'src/services/base.service'; @@ -49,7 +49,7 @@ export class PartnerService extends BaseService { private mapPartner(partner: Partner, direction: PartnerDirection): PartnerResponseDto { // this is opposite to return the non-me user of the "partner" - const user = mapDatabaseUser( + const user = mapUser( direction === PartnerDirection.SharedBy ? partner.sharedWith : partner.sharedBy, ) as PartnerResponseDto; diff --git a/server/src/services/user.service.spec.ts b/server/src/services/user.service.spec.ts index b9fa39a8c2..c2433f13cb 100644 --- a/server/src/services/user.service.spec.ts +++ b/server/src/services/user.service.spec.ts @@ -6,6 +6,7 @@ import { ImmichFileResponse } from 'src/utils/file'; import { authStub } from 'test/fixtures/auth.stub'; import { systemConfigStub } from 'test/fixtures/system-config.stub'; import { userStub } from 'test/fixtures/user.stub'; +import { factory } from 'test/small.factory'; import { newTestService, ServiceMocks } from 'test/utils'; const makeDeletedAt = (daysAgo: number) => { @@ -20,7 +21,6 @@ describe(UserService.name, () => { beforeEach(() => { ({ sut, mocks } = newTestService(UserService)); - mocks.user.get.mockImplementation((userId) => Promise.resolve([userStub.admin, userStub.user1].find((user) => user.id === userId) ?? undefined), ); @@ -28,36 +28,40 @@ describe(UserService.name, () => { describe('getAll', () => { it('admin should get all users', async () => { - mocks.user.getList.mockResolvedValue([userStub.admin]); - await expect(sut.search(authStub.admin)).resolves.toEqual([ - expect.objectContaining({ - id: authStub.admin.user.id, - email: authStub.admin.user.email, - }), - ]); + const user = factory.userAdmin(); + const auth = factory.auth(user); + + mocks.user.getList.mockResolvedValue([user]); + + await expect(sut.search(auth)).resolves.toEqual([expect.objectContaining({ id: user.id, email: user.email })]); + expect(mocks.user.getList).toHaveBeenCalledWith({ withDeleted: false }); }); it('non-admin should get all users when publicUsers enabled', async () => { mocks.user.getList.mockResolvedValue([userStub.user1]); + await expect(sut.search(authStub.user1)).resolves.toEqual([ expect.objectContaining({ id: authStub.user1.user.id, email: authStub.user1.user.email, }), ]); + expect(mocks.user.getList).toHaveBeenCalledWith({ withDeleted: false }); }); it('non-admin user should only receive itself when publicUsers is disabled', async () => { mocks.user.getList.mockResolvedValue([userStub.user1]); mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.publicUsersDisabled); + await expect(sut.search(authStub.user1)).resolves.toEqual([ expect.objectContaining({ id: authStub.user1.user.id, email: authStub.user1.user.email, }), ]); + expect(mocks.user.getList).not.toHaveBeenCalledWith({ withDeleted: false }); }); }); @@ -65,13 +69,17 @@ describe(UserService.name, () => { describe('get', () => { it('should get a user by id', async () => { mocks.user.get.mockResolvedValue(userStub.admin); + await sut.get(authStub.admin.user.id); + expect(mocks.user.get).toHaveBeenCalledWith(authStub.admin.user.id, { withDeleted: false }); }); it('should throw an error if a user is not found', async () => { mocks.user.get.mockResolvedValue(void 0); + await expect(sut.get(authStub.admin.user.id)).rejects.toBeInstanceOf(BadRequestException); + expect(mocks.user.get).toHaveBeenCalledWith(authStub.admin.user.id, { withDeleted: false }); }); }); @@ -79,6 +87,7 @@ describe(UserService.name, () => { describe('getMe', () => { it("should get the auth user's info", async () => { const user = authStub.admin.user; + await expect(sut.getMe(authStub.admin)).resolves.toMatchObject({ id: user.id, email: user.email, @@ -89,6 +98,7 @@ describe(UserService.name, () => { describe('createProfileImage', () => { it('should throw an error if the user does not exist', async () => { const file = { path: '/profile/path' } as Express.Multer.File; + mocks.user.get.mockResolvedValue(void 0); mocks.user.update.mockResolvedValue({ ...userStub.admin, profileImagePath: file.path }); @@ -105,20 +115,24 @@ describe(UserService.name, () => { it('should delete the previous profile image', async () => { const file = { path: '/profile/path' } as Express.Multer.File; - mocks.user.get.mockResolvedValue(userStub.profilePath); const files = [userStub.profilePath.profileImagePath]; + + mocks.user.get.mockResolvedValue(userStub.profilePath); mocks.user.update.mockResolvedValue({ ...userStub.admin, profileImagePath: file.path }); await sut.createProfileImage(authStub.admin, file); + expect(mocks.job.queue.mock.calls).toEqual([[{ name: JobName.DELETE_FILES, data: { files } }]]); }); it('should not delete the profile image if it has not been set', async () => { const file = { path: '/profile/path' } as Express.Multer.File; + mocks.user.get.mockResolvedValue(userStub.admin); mocks.user.update.mockResolvedValue({ ...userStub.admin, profileImagePath: file.path }); await sut.createProfileImage(authStub.admin, file); + expect(mocks.job.queue).not.toHaveBeenCalled(); expect(mocks.job.queueAll).not.toHaveBeenCalled(); }); @@ -129,6 +143,7 @@ describe(UserService.name, () => { mocks.user.get.mockResolvedValue(userStub.admin); await expect(sut.deleteProfileImage(authStub.admin)).rejects.toBeInstanceOf(BadRequestException); + expect(mocks.job.queue).not.toHaveBeenCalled(); expect(mocks.job.queueAll).not.toHaveBeenCalled(); }); @@ -138,6 +153,7 @@ describe(UserService.name, () => { const files = [userStub.profilePath.profileImagePath]; await sut.deleteProfileImage(authStub.admin); + expect(mocks.job.queue.mock.calls).toEqual([[{ name: JobName.DELETE_FILES, data: { files } }]]); }); }); @@ -176,53 +192,22 @@ describe(UserService.name, () => { describe('handleQueueUserDelete', () => { it('should skip users not ready for deletion', async () => { - mocks.user.getDeletedUsers.mockResolvedValue([ - {}, - { deletedAt: undefined }, - { deletedAt: null }, - { deletedAt: makeDeletedAt(5) }, - ] as UserEntity[]); + mocks.user.getDeletedAfter.mockResolvedValue([]); await sut.handleUserDeleteCheck(); - expect(mocks.user.getDeletedUsers).toHaveBeenCalled(); - expect(mocks.job.queue).not.toHaveBeenCalled(); - expect(mocks.job.queueAll).toHaveBeenCalledWith([]); - }); - - it('should skip users not ready for deletion - deleteDelay30', async () => { - mocks.systemMetadata.get.mockResolvedValue(systemConfigStub.deleteDelay30); - mocks.user.getDeletedUsers.mockResolvedValue([ - {}, - { deletedAt: undefined }, - { deletedAt: null }, - { deletedAt: makeDeletedAt(15) }, - ] as UserEntity[]); - - await sut.handleUserDeleteCheck(); - - expect(mocks.user.getDeletedUsers).toHaveBeenCalled(); + expect(mocks.user.getDeletedAfter).toHaveBeenCalled(); expect(mocks.job.queue).not.toHaveBeenCalled(); expect(mocks.job.queueAll).toHaveBeenCalledWith([]); }); it('should queue user ready for deletion', async () => { - const user = { id: 'deleted-user', deletedAt: makeDeletedAt(10) }; - mocks.user.getDeletedUsers.mockResolvedValue([user] as UserEntity[]); + const user = factory.user(); + mocks.user.getDeletedAfter.mockResolvedValue([{ id: user.id }]); await sut.handleUserDeleteCheck(); - expect(mocks.user.getDeletedUsers).toHaveBeenCalled(); - expect(mocks.job.queueAll).toHaveBeenCalledWith([{ name: JobName.USER_DELETION, data: { id: user.id } }]); - }); - - it('should queue user ready for deletion - deleteDelay30', async () => { - const user = { id: 'deleted-user', deletedAt: makeDeletedAt(31) }; - mocks.user.getDeletedUsers.mockResolvedValue([user] as UserEntity[]); - - await sut.handleUserDeleteCheck(); - - expect(mocks.user.getDeletedUsers).toHaveBeenCalled(); + expect(mocks.user.getDeletedAfter).toHaveBeenCalled(); expect(mocks.job.queueAll).toHaveBeenCalledWith([{ name: JobName.USER_DELETION, data: { id: user.id } }]); }); }); @@ -230,6 +215,7 @@ describe(UserService.name, () => { describe('handleUserDelete', () => { it('should skip users not ready for deletion', async () => { const user = { id: 'user-1', deletedAt: makeDeletedAt(5) } as UserEntity; + mocks.user.get.mockResolvedValue(user); await sut.handleUserDelete({ id: user.id }); @@ -240,12 +226,12 @@ describe(UserService.name, () => { it('should delete the user and associated assets', async () => { const user = { id: 'deleted-user', deletedAt: makeDeletedAt(10) } as UserEntity; + const options = { force: true, recursive: true }; + mocks.user.get.mockResolvedValue(user); await sut.handleUserDelete({ id: user.id }); - const options = { force: true, recursive: true }; - expect(mocks.storage.unlinkDir).toHaveBeenCalledWith('upload/library/deleted-user', options); expect(mocks.storage.unlinkDir).toHaveBeenCalledWith('upload/upload/deleted-user', options); expect(mocks.storage.unlinkDir).toHaveBeenCalledWith('upload/profile/deleted-user', options); @@ -257,6 +243,7 @@ describe(UserService.name, () => { it('should delete the library path for a storage label', async () => { const user = { id: 'deleted-user', deletedAt: makeDeletedAt(10), storageLabel: 'admin' } as UserEntity; + mocks.user.get.mockResolvedValue(user); await sut.handleUserDelete({ id: user.id }); @@ -269,9 +256,10 @@ describe(UserService.name, () => { describe('setLicense', () => { it('should save client license if valid', async () => { + const license = { licenseKey: 'IMCL-license-key', activationKey: 'activation-key' }; + mocks.user.upsertMetadata.mockResolvedValue(); - const license = { licenseKey: 'IMCL-license-key', activationKey: 'activation-key' }; await sut.setLicense(authStub.user1, license); expect(mocks.user.upsertMetadata).toHaveBeenCalledWith(authStub.user1.user.id, { @@ -281,9 +269,10 @@ describe(UserService.name, () => { }); it('should save server license as client if valid', async () => { + const license = { licenseKey: 'IMSV-license-key', activationKey: 'activation-key' }; + mocks.user.upsertMetadata.mockResolvedValue(); - const license = { licenseKey: 'IMSV-license-key', activationKey: 'activation-key' }; await sut.setLicense(authStub.user1, license); expect(mocks.user.upsertMetadata).toHaveBeenCalledWith(authStub.user1.user.id, { @@ -293,11 +282,13 @@ describe(UserService.name, () => { }); it('should not save license if invalid', async () => { - mocks.user.upsertMetadata.mockResolvedValue(); - const license = { licenseKey: 'license-key', activationKey: 'activation-key' }; const call = sut.setLicense(authStub.admin, license); + + mocks.user.upsertMetadata.mockResolvedValue(); + await expect(call).rejects.toThrowError('Invalid license key'); + expect(mocks.user.upsertMetadata).not.toHaveBeenCalled(); }); }); @@ -307,6 +298,7 @@ describe(UserService.name, () => { mocks.user.upsertMetadata.mockResolvedValue(); await sut.deleteLicense(authStub.admin); + expect(mocks.user.upsertMetadata).not.toHaveBeenCalled(); }); }); @@ -314,6 +306,7 @@ describe(UserService.name, () => { describe('handleUserSyncUsage', () => { it('should sync usage', async () => { await sut.handleUserSyncUsage(); + expect(mocks.user.syncUsage).toHaveBeenCalledTimes(1); }); }); diff --git a/server/src/services/user.service.ts b/server/src/services/user.service.ts index f7d6018207..ef06b6f4b1 100644 --- a/server/src/services/user.service.ts +++ b/server/src/services/user.service.ts @@ -188,15 +188,9 @@ export class UserService extends BaseService { @OnJob({ name: JobName.USER_DELETE_CHECK, queue: QueueName.BACKGROUND_TASK }) async handleUserDeleteCheck(): Promise { - const users = await this.userRepository.getDeletedUsers(); const config = await this.getConfig({ withCache: false }); - await this.jobRepository.queueAll( - users.flatMap((user) => - this.isReadyForDeletion(user, config.user.deleteDelay) - ? [{ name: JobName.USER_DELETION, data: { id: user.id } }] - : [], - ), - ); + const users = await this.userRepository.getDeletedAfter(DateTime.now().minus({ days: config.user.deleteDelay })); + await this.jobRepository.queueAll(users.map((user) => ({ name: JobName.USER_DELETION, data: { id: user.id } }))); return JobStatus.SUCCESS; } diff --git a/server/test/factory.ts b/server/test/factory.ts index 520119fc3e..69160aa8a4 100644 --- a/server/test/factory.ts +++ b/server/test/factory.ts @@ -29,6 +29,7 @@ import { SharedLinkRepository } from 'src/repositories/shared-link.repository'; import { StackRepository } from 'src/repositories/stack.repository'; import { StorageRepository } from 'src/repositories/storage.repository'; import { SyncRepository } from 'src/repositories/sync.repository'; +import { SystemMetadataRepository } from 'src/repositories/system-metadata.repository'; import { TelemetryRepository } from 'src/repositories/telemetry.repository'; import { TrashRepository } from 'src/repositories/trash.repository'; import { UserRepository } from 'src/repositories/user.repository'; @@ -205,6 +206,7 @@ export class TestContext { sharedLink: SharedLinkRepository; stack: StackRepository; storage: StorageRepository; + systemMetadata: SystemMetadataRepository; sync: SyncRepository; telemetry: TelemetryRepository; trash: TrashRepository; @@ -241,6 +243,7 @@ export class TestContext { this.stack = new StackRepository(this.db); this.storage = new StorageRepository(logger); this.sync = new SyncRepository(this.db); + this.systemMetadata = new SystemMetadataRepository(this.db); this.telemetry = newTelemetryRepositoryMock() as unknown as TelemetryRepository; this.trash = new TrashRepository(this.db); this.user = new UserRepository(this.db); diff --git a/server/test/fixtures/system-config.stub.ts b/server/test/fixtures/system-config.stub.ts index 89828d781f..355f2cc1a3 100644 --- a/server/test/fixtures/system-config.stub.ts +++ b/server/test/fixtures/system-config.stub.ts @@ -47,11 +47,6 @@ export const systemConfigStub = { defaultStorageQuota: 1, }, }, - deleteDelay30: { - user: { - deleteDelay: 30, - }, - }, libraryWatchEnabled: { library: { scan: { diff --git a/server/test/medium/specs/user.service.spec.ts b/server/test/medium/specs/user.service.spec.ts index 6750dd38d8..fded6ec3b2 100644 --- a/server/test/medium/specs/user.service.spec.ts +++ b/server/test/medium/specs/user.service.spec.ts @@ -1,15 +1,25 @@ +import { Kysely } from 'kysely'; +import { DateTime } from 'luxon'; +import { DB } from 'src/db'; +import { JobName, JobStatus } from 'src/enum'; import { UserService } from 'src/services/user.service'; import { TestContext, TestFactory } from 'test/factory'; -import { getKyselyDB, newTestService } from 'test/utils'; +import { getKyselyDB, newTestService, ServiceMocks } from 'test/utils'; + +const setup = async (db: Kysely) => { + const context = await TestContext.from(db).withUser({ isAdmin: true }).create(); + const { sut, mocks } = newTestService(UserService, context); + + return { sut, mocks, context }; +}; describe.concurrent(UserService.name, () => { let sut: UserService; let context: TestContext; + let mocks: ServiceMocks; beforeAll(async () => { - const db = await getKyselyDB(); - context = await TestContext.from(db).withUser({ isAdmin: true }).create(); - ({ sut } = newTestService(UserService, context)); + ({ sut, context, mocks } = await setup(await getKyselyDB())); }); describe('create', () => { @@ -113,4 +123,50 @@ describe.concurrent(UserService.name, () => { expect(getResponse).toEqual(after); }); }); + + describe('handleUserDeleteCheck', () => { + it('should work when there are no deleted users', async () => { + await expect(sut.handleUserDeleteCheck()).resolves.toEqual(JobStatus.SUCCESS); + + expect(mocks.job.queueAll).toHaveBeenCalledWith([]); + }); + + it('should work when there is a user to delete', async () => { + const { sut, context, mocks } = await setup(await getKyselyDB()); + const user = TestFactory.user({ deletedAt: DateTime.now().minus({ days: 25 }).toJSDate() }); + + await context.createUser(user); + + await expect(sut.handleUserDeleteCheck()).resolves.toEqual(JobStatus.SUCCESS); + + expect(mocks.job.queueAll).toHaveBeenCalledWith([{ name: JobName.USER_DELETION, data: { id: user.id } }]); + }); + + it('should skip a recently deleted user', async () => { + const { sut, context, mocks } = await setup(await getKyselyDB()); + const user = TestFactory.user({ deletedAt: DateTime.now().minus({ days: 5 }).toJSDate() }); + + await context.createUser(user); + + await expect(sut.handleUserDeleteCheck()).resolves.toEqual(JobStatus.SUCCESS); + + expect(mocks.job.queueAll).toHaveBeenCalledWith([]); + }); + + it('should respect a custom user delete delay', async () => { + const db = await getKyselyDB(); + const { sut, context, mocks } = await setup(db); + const user = TestFactory.user({ deletedAt: DateTime.now().minus({ days: 25 }).toJSDate() }); + await context.createUser(user); + + const config = await sut.getConfig({ withCache: false }); + config.user.deleteDelay = 30; + + await sut.updateConfig(config); + + await expect(sut.handleUserDeleteCheck()).resolves.toEqual(JobStatus.SUCCESS); + + expect(mocks.job.queueAll).toHaveBeenCalledWith([]); + }); + }); }); diff --git a/server/test/small.factory.ts b/server/test/small.factory.ts index 1faedf311a..5b228d3afb 100644 --- a/server/test/small.factory.ts +++ b/server/test/small.factory.ts @@ -1,8 +1,8 @@ import { randomUUID } from 'node:crypto'; -import { ApiKey, Asset, AuthApiKey, AuthUser, Library, Partner, User } from 'src/database'; +import { ApiKey, Asset, AuthApiKey, AuthUser, Library, Partner, User, UserAdmin } from 'src/database'; import { AuthDto } from 'src/dtos/auth.dto'; import { OnThisDayData } from 'src/entities/memory.entity'; -import { AssetStatus, AssetType, MemoryType, Permission } from 'src/enum'; +import { AssetStatus, AssetType, MemoryType, Permission, UserStatus } from 'src/enum'; import { ActivityItem, MemoryItem } from 'src/types'; export const newUuid = () => randomUUID() as string; @@ -85,6 +85,26 @@ const userFactory = (user: Partial = {}) => ({ ...user, }); +const userAdminFactory = (user: Partial = {}) => ({ + id: newUuid(), + name: 'Test User', + email: 'test@immich.cloud', + profileImagePath: '', + profileChangedAt: newDate(), + storageLabel: null, + shouldChangePassword: false, + isAdmin: false, + createdAt: newDate(), + updatedAt: newDate(), + deletedAt: null, + oauthId: '', + quotaSizeInBytes: null, + quotaUsageInBytes: 0, + status: UserStatus.ACTIVE, + metadata: [], + ...user, +}); + const assetFactory = (asset: Partial = {}) => ({ id: newUuid(), createdAt: newDate(), @@ -198,5 +218,6 @@ export const factory = { session: sessionFactory, stack: stackFactory, user: userFactory, + userAdmin: userAdminFactory, versionHistory: versionHistoryFactory, };