refactor: better types for getList and getDeletedAfter (#16926)

server/src/database.ts

@@ -1,5 +1,5 @@
-import { sql } from 'kysely';
+import { UserMetadataEntity } from 'src/entities/user-metadata.entity';
-import { AssetStatus, AssetType, Permission } from 'src/enum';
+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<string>(`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: [

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 { User, UserAdmin } from 'src/database';
-import { UserMetadataEntity } from 'src/entities/user-metadata.entity';
+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 {
+export function mapUserAdmin(entity: UserEntity | UserAdmin): UserAdminResponseDto {
-  const license = entity.metadata?.find(
+  const license = (entity.metadata as UserMetadataItem[])?.find(
     (item): item is UserMetadataEntity<UserMetadataKey.LICENSE> => item.key === UserMetadataKey.LICENSE,
   )?.value;
   return {

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",
+  "email",
-  "quotaUsageInBytes",
+  "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",
+  "email",
-  "quotaUsageInBytes",
+  "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",
+  "email",
-  "quotaUsageInBytes",
+  "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",
+  "email",
-  "quotaUsageInBytes",
+  "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",

server/src/repositories/activity.repository.ts

@@ -18,7 +18,7 @@ const withUser = (eb: ExpressionBuilder<DB, 'activity'>) => {
   return jsonObjectFrom(
     eb
       .selectFrom('users')
-      .select(columns.userDto)
+      .select(columns.user)
       .whereRef('users.id', '=', 'activity.userId')
       .where('users.deletedAt', 'is', null),
   ).as('user');

server/src/repositories/partner.repository.ts

@@ -18,16 +18,13 @@ export enum PartnerDirection {
 
 const withSharedBy = (eb: ExpressionBuilder<DB, 'partners'>) => {
   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<DB, 'partners'>) => {
   return jsonObjectFrom(
-    eb
+    eb.selectFrom('users as sharedWith').select(columns.user).whereRef('sharedWith.id', '=', 'partners.sharedWithId'),
-      .selectFrom('users as sharedWith')
-      .select(columns.userDto)
-      .whereRef('sharedWith.id', '=', 'partners.sharedWithId'),
   ).as('sharedWith');
 };
 

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<DbUserMetadata>;
 
 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<UserEntity | undefined> {
     return this.db
       .selectFrom('users')
-      .select(columns)
+      .select(columns.userAdmin)
       .where('users.isAdmin', '=', true)
       .where('users.deletedAt', 'is', null)
       .executeTakeFirst() as Promise<UserEntity | undefined>;
@@ -98,7 +82,7 @@ export class UserRepository {
   getByEmail(email: string, withPassword?: boolean): Promise<UserEntity | undefined> {
     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<UserEntity | undefined> {
     return this.db
       .selectFrom('users')
-      .select(columns)
+      .select(columns.userAdmin)
       .where('users.storageLabel', '=', storageLabel)
       .where('users.deletedAt', 'is', null)
       .executeTakeFirst() as Promise<UserEntity | undefined>;
@@ -119,35 +103,36 @@ export class UserRepository {
   getByOAuthId(oauthId: string): Promise<UserEntity | undefined> {
     return this.db
       .selectFrom('users')
-      .select(columns)
+      .select(columns.userAdmin)
       .where('users.oauthId', '=', oauthId)
       .where('users.deletedAt', 'is', null)
       .executeTakeFirst() as Promise<UserEntity | undefined>;
   }
 
-  getDeletedUsers(): Promise<UserEntity[]> {
+  @GenerateSql({ params: [DateTime.now().minus({ years: 1 })] })
-    return this.db
+  getDeletedAfter(target: DateTime) {
-      .selectFrom('users')
+    return this.db.selectFrom('users').select(['id']).where('users.deletedAt', '<', target.toJSDate()).execute();
-      .select(columns)
-      .where('users.deletedAt', 'is not', null)
-      .execute() as unknown as Promise<UserEntity[]>;
   }
 
-  getList({ withDeleted }: UserListFilter = {}): Promise<UserEntity[]> {
+  @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<UserEntity[]>;
+      .execute() as Promise<UserAdmin[]>;
   }
 
   async create(dto: Insertable<Users>): Promise<UserEntity> {
     return this.db
       .insertInto('users')
       .values(dto)
-      .returning(columns)
+      .returning(columns.userAdmin)
       .executeTakeFirst() as unknown as Promise<UserEntity>;
   }
 
@@ -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<UserEntity>;
   }
@@ -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<UserEntity>;
   }

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;
 

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]);
+      const user = factory.userAdmin();
-      await expect(sut.search(authStub.admin)).resolves.toEqual([
+      const auth = factory.auth(user);
-        expect.objectContaining({
+
-          id: authStub.admin.user.id,
+      mocks.user.getList.mockResolvedValue([user]);
-          email: authStub.admin.user.email,
+
-        }),
+      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([
+      mocks.user.getDeletedAfter.mockResolvedValue([]);
-        {},
-        { deletedAt: undefined },
-        { deletedAt: null },
-        { deletedAt: makeDeletedAt(5) },
-      ] 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 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.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) };
+      const user = factory.user();
-      mocks.user.getDeletedUsers.mockResolvedValue([user] as UserEntity[]);
+      mocks.user.getDeletedAfter.mockResolvedValue([{ id: user.id }]);
 
       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 } }]);
-    });
-
-    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.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);
     });
   });

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<JobStatus> {
-    const users = await this.userRepository.getDeletedUsers();
     const config = await this.getConfig({ withCache: false });
-    await this.jobRepository.queueAll(
+    const users = await this.userRepository.getDeletedAfter(DateTime.now().minus({ days: config.user.deleteDelay }));
-      users.flatMap((user) =>
+    await this.jobRepository.queueAll(users.map((user) => ({ name: JobName.USER_DELETION, data: { id: user.id } })));
-        this.isReadyForDeletion(user, config.user.deleteDelay)
-          ? [{ name: JobName.USER_DELETION, data: { id: user.id } }]
-          : [],
-      ),
-    );
     return JobStatus.SUCCESS;
   }
 

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);

server/test/fixtures/system-config.stub.ts

@@ -47,11 +47,6 @@ export const systemConfigStub = {
       defaultStorageQuota: 1,
     },
   },
-  deleteDelay30: {
-    user: {
-      deleteDelay: 30,
-    },
-  },
   libraryWatchEnabled: {
     library: {
       scan: {

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<DB>) => {
+  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();
+    ({ sut, context, mocks } = await setup(await getKyselyDB()));
-    context = await TestContext.from(db).withUser({ isAdmin: true }).create();
-    ({ sut } = newTestService(UserService, context));
   });
 
   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([]);
+    });
+  });
 });

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> = {}) => ({
   ...user,
 });
 
+const userAdminFactory = (user: Partial<UserAdmin> = {}) => ({
+  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<Asset> = {}) => ({
   id: newUuid(),
   createdAt: newDate(),
@@ -198,5 +218,6 @@ export const factory = {
   session: sessionFactory,
   stack: stackFactory,
   user: userFactory,
+  userAdmin: userAdminFactory,
   versionHistory: versionHistoryFactory,
 };