chore(server): split album update notifications into multiple jobs (#17879)

We would like to move away from the concept of finding and removing pending
jobs. The only place this is used is for album update notifications, and this
is done so that users who initially uploaded assets to an album will also
receive a notification if someone else then adds assets to the same album. This
can also be achieved with a job for each recipient. Multiple jobs also has the
advantage that it will scale better for albums with many users, it's possible
to send notifications concurrently, retries are possible without sending
duplicate notifications, and it's clear what recipient a job failed for.
This commit is contained in:
Thomas 2025-04-30 22:45:35 +01:00 committed by GitHub
parent becdc3dcf5
commit da7a81b752
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 74 additions and 143 deletions

View file

@ -154,10 +154,10 @@ describe(NotificationService.name, () => {
describe('onAlbumUpdateEvent', () => {
it('should queue notify album update event', async () => {
await sut.onAlbumUpdate({ id: 'album', recipientIds: ['42'] });
await sut.onAlbumUpdate({ id: 'album', recipientId: '42' });
expect(mocks.job.queue).toHaveBeenCalledWith({
name: JobName.NOTIFY_ALBUM_UPDATE,
data: { id: 'album', recipientIds: ['42'], delay: 300_000 },
data: { id: 'album', recipientId: '42', delay: 300_000 },
});
});
});
@ -414,14 +414,14 @@ describe(NotificationService.name, () => {
describe('handleAlbumUpdate', () => {
it('should skip if album could not be found', async () => {
await expect(sut.handleAlbumUpdate({ id: '', recipientIds: ['1'] })).resolves.toBe(JobStatus.SKIPPED);
await expect(sut.handleAlbumUpdate({ id: '', recipientId: '1' })).resolves.toBe(JobStatus.SKIPPED);
expect(mocks.user.get).not.toHaveBeenCalled();
});
it('should skip if owner could not be found', async () => {
mocks.album.getById.mockResolvedValue(albumStub.emptyWithValidThumbnail);
await expect(sut.handleAlbumUpdate({ id: '', recipientIds: ['1'] })).resolves.toBe(JobStatus.SKIPPED);
await expect(sut.handleAlbumUpdate({ id: '', recipientId: '1' })).resolves.toBe(JobStatus.SKIPPED);
expect(mocks.systemMetadata.get).not.toHaveBeenCalled();
});
@ -434,7 +434,7 @@ describe(NotificationService.name, () => {
mocks.email.renderEmail.mockResolvedValue({ html: '', text: '' });
mocks.assetJob.getAlbumThumbnailFiles.mockResolvedValue([]);
await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] });
await sut.handleAlbumUpdate({ id: '', recipientId: userStub.user1.id });
expect(mocks.user.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false });
expect(mocks.email.renderEmail).not.toHaveBeenCalled();
});
@ -456,7 +456,7 @@ describe(NotificationService.name, () => {
mocks.email.renderEmail.mockResolvedValue({ html: '', text: '' });
mocks.assetJob.getAlbumThumbnailFiles.mockResolvedValue([]);
await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] });
await sut.handleAlbumUpdate({ id: '', recipientId: userStub.user1.id });
expect(mocks.user.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false });
expect(mocks.email.renderEmail).not.toHaveBeenCalled();
});
@ -478,7 +478,7 @@ describe(NotificationService.name, () => {
mocks.email.renderEmail.mockResolvedValue({ html: '', text: '' });
mocks.assetJob.getAlbumThumbnailFiles.mockResolvedValue([]);
await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] });
await sut.handleAlbumUpdate({ id: '', recipientId: userStub.user1.id });
expect(mocks.user.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false });
expect(mocks.email.renderEmail).not.toHaveBeenCalled();
});
@ -492,21 +492,21 @@ describe(NotificationService.name, () => {
mocks.email.renderEmail.mockResolvedValue({ html: '', text: '' });
mocks.assetJob.getAlbumThumbnailFiles.mockResolvedValue([]);
await sut.handleAlbumUpdate({ id: '', recipientIds: [userStub.user1.id] });
await sut.handleAlbumUpdate({ id: '', recipientId: userStub.user1.id });
expect(mocks.user.get).toHaveBeenCalledWith(userStub.user1.id, { withDeleted: false });
expect(mocks.email.renderEmail).toHaveBeenCalled();
expect(mocks.job.queue).toHaveBeenCalled();
});
it('should add new recipients for new images if job is already queued', async () => {
mocks.job.removeJob.mockResolvedValue({ id: '1', recipientIds: ['2', '3', '4'] } as INotifyAlbumUpdateJob);
await sut.onAlbumUpdate({ id: '1', recipientIds: ['1', '2', '3'] } as INotifyAlbumUpdateJob);
await sut.onAlbumUpdate({ id: '1', recipientId: '2' } as INotifyAlbumUpdateJob);
expect(mocks.job.removeJob).toHaveBeenCalledWith(JobName.NOTIFY_ALBUM_UPDATE, '1/2');
expect(mocks.job.queue).toHaveBeenCalledWith({
name: JobName.NOTIFY_ALBUM_UPDATE,
data: {
id: '1',
delay: 300_000,
recipientIds: ['1', '2', '3', '4'],
recipientId: '2',
},
});
});