diff --git a/server/src/interfaces/event.interface.ts b/server/src/interfaces/event.interface.ts index bb2b0d9ab4bc9..4abd73bf370da 100644 --- a/server/src/interfaces/event.interface.ts +++ b/server/src/interfaces/event.interface.ts @@ -14,7 +14,7 @@ type EmitEventMap = { 'config.validate': [{ newConfig: SystemConfig; oldConfig: SystemConfig }]; // album events - 'album.update': [{ id: string; updatedBy: string }]; + 'album.update': [{ id: string; recipientIds: string[] }]; 'album.invite': [{ id: string; userId: string }]; // tag events diff --git a/server/src/interfaces/job.interface.ts b/server/src/interfaces/job.interface.ts index bc780398eaf05..ea823e3bea16b 100644 --- a/server/src/interfaces/job.interface.ts +++ b/server/src/interfaces/job.interface.ts @@ -117,6 +117,11 @@ export interface IBaseJob { force?: boolean; } +export interface IDelayedJob extends IBaseJob { + /** The minimum time to wait to execute this job, in milliseconds. */ + delay?: number; +} + export interface IEntityJob extends IBaseJob { id: string; source?: 'upload' | 'sidecar-write' | 'copy'; @@ -182,8 +187,8 @@ export interface INotifyAlbumInviteJob extends IEntityJob { recipientId: string; } -export interface INotifyAlbumUpdateJob extends IEntityJob { - senderId: string; +export interface INotifyAlbumUpdateJob extends IEntityJob, IDelayedJob { + recipientIds: string[]; } export interface JobCounts { @@ -292,6 +297,7 @@ export enum JobStatus { export type JobHandler = (data: T) => Promise; export type JobItemHandler = (item: JobItem) => Promise; +export type DataTransformer = (fromExistingJob: any, fromEnqueueingJob: any) => any; export const IJobRepository = 'IJobRepository'; @@ -310,4 +316,5 @@ export interface IJobRepository { getQueueStatus(name: QueueName): Promise; getJobCounts(name: QueueName): Promise; waitForQueueCompletion(...queues: QueueName[]): Promise; + removeJob(jobId: string, name: JobName): Promise; } diff --git a/server/src/repositories/job.repository.ts b/server/src/repositories/job.repository.ts index f64e5175e5127..6c0f4e266e18d 100644 --- a/server/src/repositories/job.repository.ts +++ b/server/src/repositories/job.repository.ts @@ -7,6 +7,7 @@ import { CronJob, CronTime } from 'cron'; import { setTimeout } from 'node:timers/promises'; import { bullConfig } from 'src/config'; import { + IEntityJob, IJobRepository, JobCounts, JobItem, @@ -249,24 +250,42 @@ export class JobRepository implements IJobRepository { } private getJobOptions(item: JobItem): JobsOptions | null { + let opts = {}; switch (item.name) { + case JobName.NOTIFY_ALBUM_UPDATE: { + opts = { jobId: item.data.id, delay: item.data?.delay }; + break; + } case JobName.STORAGE_TEMPLATE_MIGRATION_SINGLE: { - return { jobId: item.data.id }; + opts = { jobId: item.data.id }; + break; } case JobName.GENERATE_PERSON_THUMBNAIL: { - return { priority: 1 }; + opts = { priority: 1 }; + break; } case JobName.QUEUE_FACIAL_RECOGNITION: { - return { jobId: JobName.QUEUE_FACIAL_RECOGNITION }; - } - - default: { - return null; + opts = { jobId: JobName.QUEUE_FACIAL_RECOGNITION }; + break; } } + return opts; } private getQueue(queue: QueueName): Queue { return this.moduleReference.get(getQueueToken(queue), { strict: false }); } + + public async removeJob(jobId: string, name: JobName): Promise { + const existingJob = await this.getQueue(JOBS_TO_QUEUE[name]).getJob(jobId); + if (existingJob === undefined) { + return undefined; + } + try { + await existingJob.remove(); + } catch { + return undefined; + } + return existingJob.data; + } } diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 164e823336878..1af784e379c2d 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -574,7 +574,7 @@ describe(AlbumService.name, () => { expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']); expect(eventMock.emit).toHaveBeenCalledWith('album.update', { id: 'album-123', - updatedBy: authStub.admin.user.id, + recipientIds: [], }); }); @@ -618,7 +618,7 @@ describe(AlbumService.name, () => { expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']); expect(eventMock.emit).toHaveBeenCalledWith('album.update', { id: 'album-123', - updatedBy: authStub.user1.user.id, + recipientIds: ['admin_id'], }); }); diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index b59364af9fb6e..0bded6bebe082 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -192,7 +192,11 @@ export class AlbumService { albumThumbnailAssetId: album.albumThumbnailAssetId ?? firstNewAssetId, }); - await this.eventRepository.emit('album.update', { id, updatedBy: auth.user.id }); + const allUsersExceptUs = [...album.albumUsers.map((au) => au.user.id), album.owner.id].filter( + (userId) => userId !== auth.user.id, + ); + + await this.eventRepository.emit('album.update', { id, recipientIds: allUsersExceptUs }); } return results; diff --git a/server/src/services/notification.service.spec.ts b/server/src/services/notification.service.spec.ts index 5bcead0ff31ae..d57426ea95cbe 100644 --- a/server/src/services/notification.service.spec.ts +++ b/server/src/services/notification.service.spec.ts @@ -22,7 +22,7 @@ import { newLoggerRepositoryMock } from 'test/repositories/logger.repository.moc import { newNotificationRepositoryMock } from 'test/repositories/notification.repository.mock'; import { newSystemMetadataRepositoryMock } from 'test/repositories/system-metadata.repository.mock'; import { newUserRepositoryMock } from 'test/repositories/user.repository.mock'; -import { Mocked } from 'vitest'; +import { expect, Mocked } from 'vitest'; const configs = { smtpDisabled: Object.freeze({ @@ -148,10 +148,10 @@ describe(NotificationService.name, () => { describe('onAlbumUpdateEvent', () => { it('should queue notify album update event', async () => { - await sut.onAlbumUpdate({ id: '', updatedBy: '42' }); + await sut.onAlbumUpdate({ id: 'album', recipientIds: ['42'] }); expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.NOTIFY_ALBUM_UPDATE, - data: { id: '', senderId: '42' }, + data: { id: 'album', recipientIds: ['42'], delay: 60_000 }, }); }); }); @@ -422,34 +422,17 @@ describe(NotificationService.name, () => { describe('handleAlbumUpdate', () => { it('should skip if album could not be found', async () => { - await expect(sut.handleAlbumUpdate({ id: '', senderId: '' })).resolves.toBe(JobStatus.SKIPPED); + await expect(sut.handleAlbumUpdate({ id: '', recipientIds: ['1'] })).resolves.toBe(JobStatus.SKIPPED); expect(userMock.get).not.toHaveBeenCalled(); }); it('should skip if owner could not be found', async () => { albumMock.getById.mockResolvedValue(albumStub.emptyWithValidThumbnail); - await expect(sut.handleAlbumUpdate({ id: '', senderId: '' })).resolves.toBe(JobStatus.SKIPPED); + await expect(sut.handleAlbumUpdate({ id: '', recipientIds: ['1'] })).resolves.toBe(JobStatus.SKIPPED); expect(systemMock.get).not.toHaveBeenCalled(); }); - it('should filter out the sender', async () => { - albumMock.getById.mockResolvedValue({ - ...albumStub.emptyWithValidThumbnail, - albumUsers: [ - { user: { id: userStub.user1.id } } as AlbumUserEntity, - { user: { id: userStub.user2.id } } as AlbumUserEntity, - ], - }); - userMock.get.mockResolvedValue(userStub.user1); - notificationMock.renderEmail.mockResolvedValue({ html: '', text: '' }); - - await sut.handleAlbumUpdate({ id: '', senderId: userStub.user1.id }); - expect(userMock.get).not.toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false }); - expect(userMock.get).toHaveBeenCalledWith(userStub.user2.id, { withDeleted: false }); - expect(notificationMock.renderEmail).toHaveBeenCalledOnce(); - }); - it('should skip recipient that could not be looked up', async () => { albumMock.getById.mockResolvedValue({ ...albumStub.emptyWithValidThumbnail, @@ -458,7 +441,7 @@ describe(NotificationService.name, () => { userMock.get.mockResolvedValueOnce(userStub.user1); notificationMock.renderEmail.mockResolvedValue({ html: '', text: '' }); - await sut.handleAlbumUpdate({ id: '', senderId: '' }); + await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] }); expect(userMock.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false }); expect(notificationMock.renderEmail).not.toHaveBeenCalled(); }); @@ -481,7 +464,7 @@ describe(NotificationService.name, () => { }); notificationMock.renderEmail.mockResolvedValue({ html: '', text: '' }); - await sut.handleAlbumUpdate({ id: '', senderId: '' }); + await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] }); expect(userMock.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false }); expect(notificationMock.renderEmail).not.toHaveBeenCalled(); }); @@ -504,7 +487,7 @@ describe(NotificationService.name, () => { }); notificationMock.renderEmail.mockResolvedValue({ html: '', text: '' }); - await sut.handleAlbumUpdate({ id: '', senderId: '' }); + await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] }); expect(userMock.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false }); expect(notificationMock.renderEmail).not.toHaveBeenCalled(); }); @@ -517,11 +500,25 @@ describe(NotificationService.name, () => { userMock.get.mockResolvedValue(userStub.user1); notificationMock.renderEmail.mockResolvedValue({ html: '', text: '' }); - await sut.handleAlbumUpdate({ id: '', senderId: '' }); + await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] }); expect(userMock.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false }); expect(notificationMock.renderEmail).toHaveBeenCalled(); expect(jobMock.queue).toHaveBeenCalled(); }); + + it('should add new recipients for new images if job is already queued', async () => { + // @ts-expect-error needed to force a INotifyAlbumUpdateJob + jobMock.removeJob.mockResolvedValue({ id: '1', recipientIds: ['2', '3', '4'] }); + await sut.onAlbumUpdate({ id: '1', recipientIds: ['1', '2', '3'] }); + expect(jobMock.queue).toHaveBeenCalledWith({ + name: JobName.NOTIFY_ALBUM_UPDATE, + data: { + id: '1', + delay: 60_000, + recipientIds: ['1', '2', '3', '4'], + }, + }); + }); }); describe('handleSendEmail', () => { diff --git a/server/src/services/notification.service.ts b/server/src/services/notification.service.ts index 274c91661ca2b..e6a8cdd18d30d 100644 --- a/server/src/services/notification.service.ts +++ b/server/src/services/notification.service.ts @@ -9,10 +9,12 @@ import { IAssetRepository } from 'src/interfaces/asset.interface'; import { ArgOf } from 'src/interfaces/event.interface'; import { IEmailJob, + IEntityJob, IJobRepository, INotifyAlbumInviteJob, INotifyAlbumUpdateJob, INotifySignupJob, + JobItem, JobName, JobStatus, } from 'src/interfaces/job.interface'; @@ -28,6 +30,7 @@ import { getPreferences } from 'src/utils/preferences'; @Injectable() export class NotificationService { private configCore: SystemConfigCore; + private static albumUpdateEmailDelayMs = 60_000; constructor( @Inject(ISystemMetadataRepository) systemMetadataRepository: ISystemMetadataRepository, @@ -65,8 +68,30 @@ export class NotificationService { } @OnEmit({ event: 'album.update' }) - async onAlbumUpdate({ id, updatedBy }: ArgOf<'album.update'>) { - await this.jobRepository.queue({ name: JobName.NOTIFY_ALBUM_UPDATE, data: { id, senderId: updatedBy } }); + async onAlbumUpdate({ id, recipientIds }: ArgOf<'album.update'>) { + // if deliverTo is empty, album likely only has one user part of it, don't queue notification if so + if (!recipientIds) { + return; + } + + const job: JobItem = { + name: JobName.NOTIFY_ALBUM_UPDATE, + data: { id, recipientIds, delay: NotificationService.albumUpdateEmailDelayMs }, + }; + + const previousJobData = await this.jobRepository.removeJob(id, JobName.NOTIFY_ALBUM_UPDATE); + if (previousJobData !== undefined && this.isAlbumUpdateJob(previousJobData)) { + for (const string of previousJobData.recipientIds) { + if (!recipientIds.includes(string)) { + recipientIds.push(string); + } + } + } + await this.jobRepository.queue(job); + } + + private isAlbumUpdateJob(job: IEntityJob): job is INotifyAlbumUpdateJob { + return 'recipientIds' in job; } @OnEmit({ event: 'album.invite' }) @@ -182,7 +207,7 @@ export class NotificationService { return JobStatus.SUCCESS; } - async handleAlbumUpdate({ id, senderId }: INotifyAlbumUpdateJob) { + async handleAlbumUpdate({ id, recipientIds }: INotifyAlbumUpdateJob) { const album = await this.albumRepository.getById(id, { withAssets: false }); if (!album) { @@ -194,7 +219,9 @@ export class NotificationService { return JobStatus.SKIPPED; } - const recipients = [...album.albumUsers.map((user) => user.user), owner].filter((user) => user.id !== senderId); + const recipients = [...album.albumUsers.map((user) => user.user), owner].filter((user) => + recipientIds.includes(user.id), + ); const attachment = await this.getAlbumThumbnailAttachment(album); const { server } = await this.configCore.getConfig({ withCache: false }); diff --git a/server/test/fixtures/user.stub.ts b/server/test/fixtures/user.stub.ts index 6f3a819eef80e..914b09c10ff2d 100644 --- a/server/test/fixtures/user.stub.ts +++ b/server/test/fixtures/user.stub.ts @@ -31,6 +31,7 @@ export const userStub = { ...authStub.admin.user, password: 'admin_password', name: 'admin_name', + id: 'admin_id', storageLabel: 'admin', oauthId: '', shouldChangePassword: false, diff --git a/server/test/repositories/job.repository.mock.ts b/server/test/repositories/job.repository.mock.ts index 6bffe184fda57..44f5c3f8318c6 100644 --- a/server/test/repositories/job.repository.mock.ts +++ b/server/test/repositories/job.repository.mock.ts @@ -17,5 +17,6 @@ export const newJobRepositoryMock = (): Mocked => { getJobCounts: vitest.fn(), clear: vitest.fn(), waitForQueueCompletion: vitest.fn(), + removeJob: vitest.fn(), }; };