Skip to content

Commit

Permalink
feat(server): wait 60 seconds before sending email on new album item
Browse files Browse the repository at this point in the history
Album update jobs will now wait a minute to send. If a new image is added while that job is pending, the old job will be cancelled, and a new one will be enqueued for a minute.

This is to prevent a flood of notifications by dragging in images directly to the album, which adds them to the album one at a time.

Album updates now include a list of users to email, which is generally everybody except the updater. If somebody else updates the album within that minute, both people will get an album update email in a minute, as they both added images and the other should be notified.
  • Loading branch information
HeyBanditoz committed Sep 7, 2024
1 parent 28bc7f3 commit 6a50955
Show file tree
Hide file tree
Showing 10 changed files with 99 additions and 44 deletions.
1 change: 0 additions & 1 deletion e2e/test-assets
Submodule test-assets deleted from 4e9731
2 changes: 1 addition & 1 deletion server/src/interfaces/event.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 9 additions & 2 deletions server/src/interfaces/job.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -292,6 +297,7 @@ export enum JobStatus {

export type JobHandler<T = any> = (data: T) => Promise<JobStatus>;
export type JobItemHandler = (item: JobItem) => Promise<void>;
export type DataTransformer = (fromExistingJob: any, fromEnqueueingJob: any) => any;

export const IJobRepository = 'IJobRepository';

Expand All @@ -310,4 +316,5 @@ export interface IJobRepository {
getQueueStatus(name: QueueName): Promise<QueueStatus>;
getJobCounts(name: QueueName): Promise<JobCounts>;
waitForQueueCompletion(...queues: QueueName[]): Promise<void>;
removeJob(jobId: string, name: JobName): Promise<IEntityJob | undefined>;
}
33 changes: 26 additions & 7 deletions server/src/repositories/job.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<Queue>(getQueueToken(queue), { strict: false });
}

public async removeJob(jobId: string, name: JobName): Promise<IEntityJob | undefined> {
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;
}
}
4 changes: 2 additions & 2 deletions server/src/services/album.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
});
});

Expand Down Expand Up @@ -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'],
});
});

Expand Down
6 changes: 5 additions & 1 deletion server/src/services/album.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
49 changes: 23 additions & 26 deletions server/src/services/notification.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<SystemConfig>({
Expand Down Expand Up @@ -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 },
});
});
});
Expand Down Expand Up @@ -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,
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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-ignore
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', () => {
Expand Down
35 changes: 31 additions & 4 deletions server/src/services/notification.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down Expand Up @@ -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' })
Expand Down Expand Up @@ -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) {
Expand All @@ -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 });
Expand Down
1 change: 1 addition & 0 deletions server/test/fixtures/user.stub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const userStub = {
...authStub.admin.user,
password: 'admin_password',
name: 'admin_name',
id: 'admin_id',
storageLabel: 'admin',
oauthId: '',
shouldChangePassword: false,
Expand Down
1 change: 1 addition & 0 deletions server/test/repositories/job.repository.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ export const newJobRepositoryMock = (): Mocked<IJobRepository> => {
getJobCounts: vitest.fn(),
clear: vitest.fn(),
waitForQueueCompletion: vitest.fn(),
removeJob: vitest.fn(),
};
};

0 comments on commit 6a50955

Please sign in to comment.