From 265c59f970739ea497b7af8949aee6214888ebeb Mon Sep 17 00:00:00 2001 From: Klaudiusz Dembler Date: Mon, 4 Sep 2023 13:38:11 +0200 Subject: [PATCH] add email send retries --- packages/server/README.md | 2 +- .../migration.sql | 13 ++ packages/server/prisma/schema.prisma | 24 +- packages/server/src/common/config.ts | 2 + .../server/src/notifier/api/notification.ts | 7 +- packages/server/src/notifier/index.ts | 4 +- .../server/src/notifier/scripts/mockEmail.ts | 3 +- .../server/src/notifier/sendNotifications.ts | 43 +++- packages/server/test/api/auth.test.ts | 8 +- packages/server/test/api/notifier.test.ts | 5 +- packages/server/test/notifier.test.ts | 218 +++++++++++++++--- packages/server/test/setup.ts | 19 +- 12 files changed, 286 insertions(+), 62 deletions(-) create mode 100644 packages/server/prisma/migrations/20230904104915_notification_status/migration.sql diff --git a/packages/server/README.md b/packages/server/README.md index cf35866d6e..a3260199dc 100644 --- a/packages/server/README.md +++ b/packages/server/README.md @@ -195,7 +195,7 @@ query { notifications { kind entityId - isSent + status } } ``` diff --git a/packages/server/prisma/migrations/20230904104915_notification_status/migration.sql b/packages/server/prisma/migrations/20230904104915_notification_status/migration.sql new file mode 100644 index 0000000000..9ae40c8134 --- /dev/null +++ b/packages/server/prisma/migrations/20230904104915_notification_status/migration.sql @@ -0,0 +1,13 @@ +/* + Warnings: + + - You are about to drop the column `isSent` on the `Notification` table. All the data in the column will be lost. + +*/ +-- CreateEnum +CREATE TYPE "NotificationStatus" AS ENUM ('PENDING', 'SENT', 'FAILED'); + +-- AlterTable +ALTER TABLE "Notification" DROP COLUMN "isSent", +ADD COLUMN "retryCount" INTEGER NOT NULL DEFAULT 0, +ADD COLUMN "status" "NotificationStatus" NOT NULL DEFAULT 'PENDING'; diff --git a/packages/server/prisma/schema.prisma b/packages/server/prisma/schema.prisma index 6abb81f959..9579b2e513 100644 --- a/packages/server/prisma/schema.prisma +++ b/packages/server/prisma/schema.prisma @@ -38,14 +38,15 @@ model Subscription { } model Notification { - id Int @id @default(autoincrement()) - member Member @relation(fields: [memberId], references: [id]) - memberId Int - kind NotificationKind - eventId String - entityId String? - isSent Boolean @default(false) - isRead Boolean @default(false) + id Int @id @default(autoincrement()) + member Member @relation(fields: [memberId], references: [id]) + memberId Int + kind NotificationKind + eventId String + entityId String? + status NotificationStatus @default(PENDING) + retryCount Int @default(0) + isRead Boolean @default(false) @@unique([memberId, eventId]) @@index(memberId) @@ -92,7 +93,6 @@ enum NotificationKind { ELECTION_ANNOUNCING_STARTED ELECTION_VOTING_STARTED ELECTION_REVEALING_STARTED - // ------------------ // Entity specific @@ -108,3 +108,9 @@ enum NotificationKind { // PROPOSAL_ENTITY_VOTE // PROPOSAL_ENTITY_DISCUSSION } + +enum NotificationStatus { + PENDING + SENT + FAILED +} diff --git a/packages/server/src/common/config.ts b/packages/server/src/common/config.ts index 1487726ab6..0045d4cbfc 100644 --- a/packages/server/src/common/config.ts +++ b/packages/server/src/common/config.ts @@ -27,3 +27,5 @@ export { PORT, APP_SECRET_KEY, QUERY_NODE_ENDPOINT, PIONEER_URL, EMAIL_SENDER, S export const STARTING_BLOCK = Number(_STARTING_BLOCK ?? 0) export const INITIAL_MEMBERSHIPS = _INITIAL_MEMBERSHIPS ? [JSON.parse(_INITIAL_MEMBERSHIPS)].flat() : [] + +export const EMAIL_MAX_RETRY_COUNT = 3 diff --git a/packages/server/src/notifier/api/notification.ts b/packages/server/src/notifier/api/notification.ts index deff06aaad..72b2b5f211 100644 --- a/packages/server/src/notifier/api/notification.ts +++ b/packages/server/src/notifier/api/notification.ts @@ -1,11 +1,12 @@ import * as Prisma from '@prisma/client' import { arg, booleanArg, enumType, list, objectType, queryField, stringArg } from 'nexus' -import { Notification, NotificationKind } from 'nexus-prisma' +import { Notification, NotificationKind, NotificationStatus } from 'nexus-prisma' import { authMemberId } from '@/auth/model/token' import { Context } from '@/common/api' export const NotificationKindEnum = enumType(NotificationKind) +export const NotificationStatusEnum = enumType(NotificationStatus) export const NotificationFields = objectType({ name: Notification.$name, @@ -15,7 +16,7 @@ export const NotificationFields = objectType({ t.field(Notification.kind) t.field(Notification.eventId) t.field(Notification.entityId) - t.field(Notification.isSent) + t.field(Notification.status) t.field(Notification.isRead) }, }) @@ -29,7 +30,7 @@ export const notificationsQuery = queryField('notifications', { kind: arg({ type: NotificationKind.name }), eventId: stringArg(), entityId: stringArg(), - isSent: booleanArg(), + status: arg({ type: NotificationStatus.name }), isRead: booleanArg(), }, diff --git a/packages/server/src/notifier/index.ts b/packages/server/src/notifier/index.ts index 1325af59b1..7bf914c8b4 100644 --- a/packages/server/src/notifier/index.ts +++ b/packages/server/src/notifier/index.ts @@ -1,7 +1,7 @@ import { createNotifications } from './createNotifications' -import { sendNotifications } from './sendNotifications' +import { processNotifications } from './sendNotifications' export const run = async () => { await createNotifications() - await sendNotifications() + await processNotifications() } diff --git a/packages/server/src/notifier/scripts/mockEmail.ts b/packages/server/src/notifier/scripts/mockEmail.ts index d157d36e77..856eb2dd39 100644 --- a/packages/server/src/notifier/scripts/mockEmail.ts +++ b/packages/server/src/notifier/scripts/mockEmail.ts @@ -24,7 +24,8 @@ const sendEmail = async () => { }, memberId: 1, isRead: false, - isSent: false, + retryCount: 0, + status: 'PENDING' as const, } const notification = { diff --git a/packages/server/src/notifier/sendNotifications.ts b/packages/server/src/notifier/sendNotifications.ts index e0f45dcb71..7241719c45 100644 --- a/packages/server/src/notifier/sendNotifications.ts +++ b/packages/server/src/notifier/sendNotifications.ts @@ -1,35 +1,58 @@ -import { pick } from 'lodash' +import { Member, Notification } from '@prisma/client' import { error, info, warn } from 'npmlog' +import { EMAIL_MAX_RETRY_COUNT } from '@/common/config' import { prisma } from '@/common/prisma' import { EmailProvider, createEmailProvider, errorMessage } from '@/common/utils' import { createEmailNotifier } from './model/email' -export const sendNotifications = async (): Promise => { +export const processNotifications = async (): Promise => { let emailProvider: EmailProvider try { emailProvider = createEmailProvider() } catch (err) { warn('Email notifications', 'Failed to configure email provider with error:', errorMessage(err)) - return [] + return } - const notifications = await prisma.notification.findMany({ where: { isSent: false }, include: { member: true } }) - const notifyViaEmail = createEmailNotifier(emailProvider) + const notifications = await prisma.notification.findMany({ where: { status: 'PENDING' }, include: { member: true } }) + await sendNotifications(notifications, emailProvider) +} + +type NotificationWithMember = Notification & { member: Member } +export const sendNotifications = async ( + notifications: NotificationWithMember[], + emailProvider: EmailProvider +): Promise => { + const notifyViaEmail = createEmailNotifier(emailProvider) info('Email notifications', 'Attempt to email', notifications.length, 'notifications') - return Promise.all( + await Promise.all( notifications.map(async (notification) => { + const { id, retryCount } = notification + try { await notifyViaEmail(notification) - return await prisma.notification.update({ where: pick(notification, 'id'), data: { isSent: true } }) + return await prisma.notification.update({ where: { id }, data: { status: 'SENT' } }) } catch (errData) { - error('Email notification failure', `Failed to email ${notification.id} with error:`, errorMessage(errData)) + if (retryCount >= EMAIL_MAX_RETRY_COUNT) { + error( + 'Email notification failure', + `Failed to email ${notification.id} with ${EMAIL_MAX_RETRY_COUNT} retries. Error:`, + errorMessage(errData) + ) + return await prisma.notification.update({ where: { id }, data: { status: 'FAILED' } }) + } else { + warn( + 'Email notification failure', + `Failed to email ${notification.id}. Will retry. Error:`, + errorMessage(errData) + ) + return await prisma.notification.update({ where: { id }, data: { retryCount: retryCount + 1 } }) + } } - // TODO: update a fail counter instead so it can be retried N time later - await prisma.notification.update({ where: pick(notification, 'id'), data: { isSent: true } }) }) ) } diff --git a/packages/server/test/api/auth.test.ts b/packages/server/test/api/auth.test.ts index a4580c2032..002f4a7f0c 100644 --- a/packages/server/test/api/auth.test.ts +++ b/packages/server/test/api/auth.test.ts @@ -3,7 +3,7 @@ import { isUndefined } from 'lodash' import { prisma } from '@/common/prisma' -import { clearDb, mockRequest, mockSendEmail } from '../setup' +import { clearDb, mockRequest, mockEmailProvider } from '../setup' import { api, authApi, gql, jwtRegex, keyring, Member, signWith, verifyEmailLinkRegex } from './utils' @@ -30,7 +30,7 @@ describe('API: Authentication', () => { }) it('Member signs up ', async () => { - mockSendEmail.mockReset() + mockEmailProvider.reset() mockRequest.mockReset() mockRequest.mockReturnValue({ membershipByUniqueInput: { controllerAccount: ALICE.controller.address } }) @@ -90,8 +90,8 @@ describe('API: Authentication', () => { authToken = success?.signup - expect(mockSendEmail).toHaveBeenCalledTimes(1) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails.length).toBe(1) + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: ALICE.email, subject: 'Confirm your email for Pioneer', diff --git a/packages/server/test/api/notifier.test.ts b/packages/server/test/api/notifier.test.ts index 8b879e6d39..4146d942f7 100644 --- a/packages/server/test/api/notifier.test.ts +++ b/packages/server/test/api/notifier.test.ts @@ -241,7 +241,7 @@ describe('API: notifier', () => { kind eventId entityId - isSent + status isRead } } @@ -254,7 +254,6 @@ describe('API: notifier', () => { kind: 'FORUM_POST_ALL', eventId: 'post_creation:1', entityId: 'post:1', - isSent: false, isRead: false, memberId: ALICE.id, }, @@ -268,7 +267,7 @@ describe('API: notifier', () => { kind: 'FORUM_POST_ALL', eventId: 'post_creation:1', entityId: 'post:1', - isSent: false, + status: 'PENDING', isRead: false, }, ], diff --git a/packages/server/test/notifier.test.ts b/packages/server/test/notifier.test.ts index 6a4257d0c3..5902a13a86 100644 --- a/packages/server/test/notifier.test.ts +++ b/packages/server/test/notifier.test.ts @@ -1,3 +1,4 @@ +import { EMAIL_MAX_RETRY_COUNT } from '@/common/config' import { prisma } from '@/common/prisma' import { GetForumCategoryDocument, GetNotificationEventsDocument, GetThreadDocument } from '@/common/queries' import { run } from '@/notifier' @@ -5,13 +6,13 @@ import { run } from '@/notifier' import { createMember } from './_mocks/notifier/createMember' import { postAddedEvent, threadCreatedEvent } from './_mocks/notifier/events' import { electionAnnouncingEvent, electionRevealingEvent, electionVotingEvent } from './_mocks/notifier/events/election' -import { clearDb, mockRequest, mockSendEmail } from './setup' +import { clearDb, mockRequest, mockEmailProvider } from './setup' describe('Notifier', () => { beforeEach(async () => { await clearDb() mockRequest.mockReset() - mockSendEmail.mockReset() + mockEmailProvider.reset() }) describe('forum', () => { @@ -87,7 +88,8 @@ describe('Notifier', () => { kind: 'FORUM_THREAD_CREATOR', entityId: 'post:1', isRead: false, - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toContainEqual( @@ -95,7 +97,8 @@ describe('Notifier', () => { eventId: 'event:2', memberId: alice.id, kind: 'FORUM_POST_MENTION', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toContainEqual( @@ -103,7 +106,8 @@ describe('Notifier', () => { eventId: 'event:2', memberId: bob.id, kind: 'FORUM_THREAD_ENTITY_POST', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toContainEqual( @@ -111,7 +115,8 @@ describe('Notifier', () => { eventId: 'event:3', memberId: alice.id, kind: 'FORUM_CATEGORY_ENTITY_POST', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toContainEqual( @@ -119,7 +124,8 @@ describe('Notifier', () => { eventId: 'event:3', memberId: bob.id, kind: 'FORUM_POST_ALL', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toHaveLength(5) @@ -128,42 +134,42 @@ describe('Notifier', () => { // Check emails // ------------------- - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('thread:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:1/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('thread:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:2/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: bob.email, subject: expect.stringContaining('thread:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:2/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('thread:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:3/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: bob.email, subject: expect.stringContaining('thread:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:3/s), }) ) - expect(mockSendEmail).toHaveBeenCalledTimes(5) + expect(mockEmailProvider.sentEmails.length).toBe(5) }) it('ThreadCreatedEvent', async () => { @@ -256,28 +262,28 @@ describe('Notifier', () => { // Check emails // ------------------- - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('thread:1:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:1/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('thread:2:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:2/s), }) ) - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: bob.email, subject: expect.stringContaining('thread:3:title'), html: expect.stringMatching(/\/#\/forum\/thread\/thread:3/s), }) ) - expect(mockSendEmail).toHaveBeenCalledTimes(3) + expect(mockEmailProvider.sentEmails.length).toBe(3) }) }) @@ -330,7 +336,8 @@ describe('Notifier', () => { memberId: alice.id, kind: 'ELECTION_ANNOUNCING_STARTED', isRead: false, - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toHaveLength(1) @@ -339,14 +346,14 @@ describe('Notifier', () => { // Check emails // ------------------- - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('election started'), html: expect.stringMatching(/\/#\/election/s), }) ) - expect(mockSendEmail).toHaveBeenCalledTimes(1) + expect(mockEmailProvider.sentEmails.length).toBe(1) }) it('ElectionVotingStartedEvent', async () => { @@ -392,7 +399,8 @@ describe('Notifier', () => { eventId: votingId, memberId: alice.id, kind: 'ELECTION_VOTING_STARTED', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toHaveLength(1) @@ -401,14 +409,14 @@ describe('Notifier', () => { // Check emails // ------------------- - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('voting started'), html: expect.stringMatching(/\/#\/election/s), }) ) - expect(mockSendEmail).toHaveBeenCalledTimes(1) + expect(mockEmailProvider.sentEmails.length).toBe(1) }) it('ElectionRevealingStartedEvent', async () => { @@ -454,7 +462,8 @@ describe('Notifier', () => { eventId: revealingId, memberId: alice.id, kind: 'ELECTION_REVEALING_STARTED', - isSent: true, + status: 'SENT', + retryCount: 0, }) ) expect(notifications).toHaveLength(1) @@ -463,14 +472,171 @@ describe('Notifier', () => { // Check emails // ------------------- - expect(mockSendEmail).toHaveBeenCalledWith( + expect(mockEmailProvider.sentEmails).toContainEqual( expect.objectContaining({ to: alice.email, subject: expect.stringContaining('revealing period started'), html: expect.stringMatching(/\/#\/election/s), }) ) - expect(mockSendEmail).toHaveBeenCalledTimes(1) + expect(mockEmailProvider.sentEmails.length).toBe(1) + }) + }) + + describe('retries', () => { + it('should retry failed notifications', async () => { + // ------------------- + // Initialize database + // ------------------- + + const alice = await createMember(1, 'alice', [{ kind: 'FORUM_POST_MENTION' }]) + + // ------------------- + // Mock QN responses + // ------------------- + + mockRequest + .mockReturnValueOnce({ + events: [ + postAddedEvent(1, { + thread: 'foo', + threadAuthor: 'foo', + text: `Hi [@Alice](#mention?member-id=${alice.id})`, + }), + ], + }) + .mockReturnValue({ + events: [], + }) + + // Enable email provider fails + mockEmailProvider.shouldFail = true + + // Run `run()` MAX_EMAIL_RETRY_COUNT times + + for (let i = 0; i < EMAIL_MAX_RETRY_COUNT; i++) { + await run() + + // ------------------- + // Check notifications + // ------------------- + + const notifications = await prisma.notification.findMany() + + expect(notifications).toContainEqual( + expect.objectContaining({ + eventId: 'event:1', + memberId: alice.id, + kind: 'FORUM_POST_MENTION', + status: 'PENDING', + retryCount: i + 1, + }) + ) + expect(notifications).toHaveLength(1) + } + + // Expect the notification to be marked as failed after retries are exhausted + + await run() + + const notifications = await prisma.notification.findMany() + + expect(notifications).toContainEqual( + expect.objectContaining({ + eventId: 'event:1', + memberId: alice.id, + kind: 'FORUM_POST_MENTION', + status: 'FAILED', + retryCount: EMAIL_MAX_RETRY_COUNT, + }) + ) + + // Make sure no emails were sent + + expect(mockEmailProvider.sentEmails.length).toBe(0) + }) + + it('should properly send retried notifications', async () => { + // ------------------- + // Initialize database + // ------------------- + + const alice = await createMember(1, 'alice', [{ kind: 'FORUM_POST_MENTION' }]) + + // ------------------- + // Mock QN responses + // ------------------- + + mockRequest + .mockReturnValueOnce({ + events: [ + postAddedEvent(1, { + thread: 'foo', + threadAuthor: 'foo', + text: `Hi [@Alice](#mention?member-id=${alice.id})`, + }), + ], + }) + .mockReturnValue({ + events: [], + forumPostByUniqueInput: { + author: { handle: 'author:handle' }, + thread: { id: 'thread:id', title: 'thread:title' }, + text: 'Lorem Ipsum', + }, + }) + + // Enable email provider fails + mockEmailProvider.shouldFail = true + + await run() + + // ------------------- + // Check notifications + // ------------------- + + let notifications = await prisma.notification.findMany() + + expect(notifications).toContainEqual( + expect.objectContaining({ + eventId: 'event:1', + memberId: alice.id, + kind: 'FORUM_POST_MENTION', + status: 'PENDING', + retryCount: 1, + }) + ) + expect(notifications).toHaveLength(1) + + // Disable email provider fails + mockEmailProvider.shouldFail = false + + // Expect the notification to be sent + await run() + + notifications = await prisma.notification.findMany() + + expect(notifications).toContainEqual( + expect.objectContaining({ + eventId: 'event:1', + memberId: alice.id, + kind: 'FORUM_POST_MENTION', + status: 'SENT', + retryCount: 1, + }) + ) + + // Check emails + + expect(mockEmailProvider.sentEmails).toContainEqual( + expect.objectContaining({ + to: alice.email, + subject: expect.stringContaining('thread:title'), + html: expect.stringMatching(/\/#\/forum\/thread\/thread:id\?post=post:1/s), + }) + ) + + expect(mockEmailProvider.sentEmails.length).toBe(1) }) }) }) diff --git a/packages/server/test/setup.ts b/packages/server/test/setup.ts index a45653179b..7a630f4e91 100644 --- a/packages/server/test/setup.ts +++ b/packages/server/test/setup.ts @@ -23,11 +23,24 @@ jest.mock('@/common/queries/__generated__', () => ({ }, })) -export const mockSendEmail = jest.fn() class MockEmailProvider implements EmailProvider { - sendEmail = mockSendEmail + sentEmails: any[] = [] + shouldFail = false + + sendEmail = async (email: any) => { + if (this.shouldFail) { + throw new Error('MockEmailProvider sendEmail failed') + } + this.sentEmails.push(email) + } + + reset = () => { + this.sentEmails = [] + this.shouldFail = false + } } +export const mockEmailProvider = new MockEmailProvider() jest.mock('@/common/utils/email', () => ({ ...jest.requireActual('@/common/utils/email'), - createEmailProvider: () => new MockEmailProvider(), + createEmailProvider: () => mockEmailProvider, }))