From 0ab98930cd0d30619f00e4e388f98587a23a98aa Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 11:08:09 +0200 Subject: [PATCH 1/8] fix(notification): pervent long running transaction --- .../services/notification-module-service.ts | 148 +++++++++++++----- .../src/services/notification-provider.ts | 17 +- 2 files changed, 117 insertions(+), 48 deletions(-) diff --git a/packages/modules/notification/src/services/notification-module-service.ts b/packages/modules/notification/src/services/notification-module-service.ts index 39bb5e05e756d..da00ec9a2f616 100644 --- a/packages/modules/notification/src/services/notification-module-service.ts +++ b/packages/modules/notification/src/services/notification-module-service.ts @@ -8,9 +8,10 @@ import { NotificationTypes, } from "@medusajs/types" import { + arrayDifference, EmitEvents, + generateEntityId, InjectManager, - InjectTransactionManager, MedusaContext, MedusaError, MedusaService, @@ -94,7 +95,7 @@ export default class NotificationModuleService return Array.isArray(data) ? serialized : serialized[0] } - @InjectTransactionManager("baseRepository_") + @InjectManager("baseRepository_") protected async createNotifications_( data: NotificationTypes.CreateNotificationDTO[], @MedusaContext() sharedContext: Context = {} @@ -108,57 +109,118 @@ export default class NotificationModuleService const idempotencyKeys = data .map((entry) => entry.idempotency_key) .filter(Boolean) - const alreadySentNotifications = await this.notificationService_.list( - { - idempotency_key: idempotencyKeys, - }, - { take: null }, - sharedContext - ) - const existsMap = new Map( - alreadySentNotifications.map((n) => [n.idempotency_key as string, true]) - ) + let { notificationsToProcess, createdNotifications } = + await this.baseRepository_.transaction(async (txManager) => { + const context = { + ...sharedContext, + transactionManager: txManager, + } - const notificationsToProcess = data.filter( - (entry) => !entry.idempotency_key || !existsMap.has(entry.idempotency_key) - ) + const alreadySentNotifications = await this.notificationService_.list( + { + idempotency_key: idempotencyKeys, + }, + { take: null }, + context + ) - const notificationsToCreate = await promiseAll( - notificationsToProcess.map(async (entry) => { - const provider = - await this.notificationProviderService_.getProviderForChannel( - entry.channel - ) + const existsMap = new Map( + alreadySentNotifications.map((n) => [ + n.idempotency_key as string, + true, + ]) + ) - if (!provider) { - throw new MedusaError( - MedusaError.Types.NOT_FOUND, - `Could not find a notification provider for channel: ${entry.channel}` - ) - } + const notificationsToProcess = data.filter( + (entry) => + !entry.idempotency_key || !existsMap.has(entry.idempotency_key) + ) - if (!provider.is_enabled) { - throw new MedusaError( - MedusaError.Types.NOT_FOUND, - `Notification provider ${provider.id} is not enabled. To enable it, configure it as a provider in the notification module options.` + const channels = notificationsToProcess.map((not) => not.channel) + const providers = + await this.notificationProviderService_.getProviderForChannels( + channels ) - } - const res = await this.notificationProviderService_.send( - provider, - entry + // Create the notifications to be sent to prevent concurrent actions listing the same notifications + const normalizedNotificationsToProcess = notificationsToProcess.map( + (entry) => { + const provider = providers.find((provider) => + provider.channels.includes(entry.channel) + ) + + return { + provider, + data: { + id: generateEntityId(undefined, "noti"), + ...entry, + provider_id: provider?.id, + }, + } + } + ) + + const createdNotifications = await this.notificationService_.create( + normalizedNotificationsToProcess + .filter((entry) => entry.provider?.is_enabled) + .map((e) => e.data), + context ) - return { ...entry, provider_id: provider.id, external_id: res.id } + + return { + notificationsToProcess: normalizedNotificationsToProcess, + createdNotifications, + } }) - ) - // Currently we store notifications after they are sent, which might result in a notification being sent that is not registered in the database. - // If necessary, we can switch to a two-step process where we first create the notification, send it, and update it after it being sent. - const createdNotifications = await this.notificationService_.create( - notificationsToCreate, - sharedContext - ) + const notificationToUpdate: { id: string; external_id?: string }[] = [] + + try { + await promiseAll( + notificationsToProcess.map(async (entry) => { + const provider = entry.provider + + if (!provider) { + throw new MedusaError( + MedusaError.Types.NOT_FOUND, + `Could not find a notification provider for channel: ${entry.channel}` + ) + } + + if (!provider.is_enabled) { + throw new MedusaError( + MedusaError.Types.NOT_FOUND, + `Notification provider ${provider.id} is not enabled. To enable it, configure it as a provider in the notification module options.` + ) + } + + const res = await this.notificationProviderService_.send( + provider, + entry.data + ) + + entry.data.external_id = res.id + notificationToUpdate.push(entry.data) + }) + ) + } catch (e) { + const notificationIdsToDelete = arrayDifference( + createdNotifications.map((n) => n.id), + notificationToUpdate.map((n) => n.id) + ) + await this.notificationService_.delete( + { id: notificationIdsToDelete }, + sharedContext + ) + + throw e + } finally { + createdNotifications = await this.notificationService_.update( + notificationToUpdate, + sharedContext + ) + } return createdNotifications } diff --git a/packages/modules/notification/src/services/notification-provider.ts b/packages/modules/notification/src/services/notification-provider.ts index 9ce377e5b1e65..7cd0ad934bb2e 100644 --- a/packages/modules/notification/src/services/notification-provider.ts +++ b/packages/modules/notification/src/services/notification-provider.ts @@ -12,6 +12,8 @@ type InjectedDependencies = { ]: NotificationTypes.INotificationProvider } +type Provider = InferEntityType + export default class NotificationProviderService extends ModulesSdkUtils.MedusaInternalService< InjectedDependencies, typeof NotificationProvider @@ -46,13 +48,13 @@ export default class NotificationProviderService extends ModulesSdkUtils.MedusaI } } - async getProviderForChannel( - channel: string - ): Promise | undefined> { + async getProviderForChannels< + TChannel = string | string[], + TOutput = TChannel extends string[] ? Provider[] : Provider | undefined + >(channels: TChannel): Promise { if (!this.providersCache) { const providers = await this.notificationProviderRepository_.find() - type name = (typeof NotificationProvider)["name"] this.providersCache = new Map( providers.flatMap((provider) => provider.channels.map((c) => [c, provider]) @@ -60,7 +62,12 @@ export default class NotificationProviderService extends ModulesSdkUtils.MedusaI ) } - return this.providersCache.get(channel) + const normalizedChannels = Array.isArray(channels) ? channels : [channels] + const results = normalizedChannels.map((channel) => + this.providersCache.get(channel) + ) + + return (Array.isArray(channels) ? results : results[0]) as TOutput } async send( From 8f387c66a52ec67825c11cae4108bb1a1895a918 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 12:15:41 +0200 Subject: [PATCH 2/8] Add status management --- .../core/types/src/notification/common.ts | 4 ++ .../core/utils/src/notification/common.ts | 5 ++ packages/core/utils/src/notification/index.ts | 1 + .../notification-module-service/index.spec.ts | 68 +++++++++++++++++-- .../.snapshot-medusa-notification.json | 15 ++++ .../src/migrations/Migration20240830094712.ts | 13 ++++ .../notification/src/models/notification.ts | 5 +- .../services/notification-module-service.ts | 58 ++++++++-------- .../src/services/notification-provider.ts | 6 +- 9 files changed, 134 insertions(+), 41 deletions(-) create mode 100644 packages/core/utils/src/notification/common.ts create mode 100644 packages/modules/notification/src/migrations/Migration20240830094712.ts diff --git a/packages/core/types/src/notification/common.ts b/packages/core/types/src/notification/common.ts index 251d1aad28ba9..963eb85347afb 100644 --- a/packages/core/types/src/notification/common.ts +++ b/packages/core/types/src/notification/common.ts @@ -99,6 +99,10 @@ export interface NotificationDTO { * The date and time the notification was created. */ created_at: Date + /** + * The status of the notification + */ + status: "pending" | "success" | "failure" } /** diff --git a/packages/core/utils/src/notification/common.ts b/packages/core/utils/src/notification/common.ts new file mode 100644 index 0000000000000..f4d533e906357 --- /dev/null +++ b/packages/core/utils/src/notification/common.ts @@ -0,0 +1,5 @@ +export enum NotificationStatus { + PENDING = "pending", + SUCCESS = "success", + FAILURE = "failure", +} diff --git a/packages/core/utils/src/notification/index.ts b/packages/core/utils/src/notification/index.ts index 295cc03deb451..0ea6696c00952 100644 --- a/packages/core/utils/src/notification/index.ts +++ b/packages/core/utils/src/notification/index.ts @@ -1,2 +1,3 @@ export * from "./abstract-notification-provider" export * from "./events" +export * from "./common" diff --git a/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts b/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts index 392ce19f060cb..612fe01d653ab 100644 --- a/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts +++ b/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts @@ -5,11 +5,11 @@ import { Module, Modules, NotificationEvents, + NotificationStatus, } from "@medusajs/utils" import { MockEventBusService, moduleIntegrationTestRunner, - SuiteOptions, } from "medusa-test-utils" import { resolve } from "path" import { NotificationModuleService } from "@services" @@ -32,10 +32,10 @@ let moduleOptions = { jest.setTimeout(30000) -moduleIntegrationTestRunner({ +moduleIntegrationTestRunner({ moduleName: Modules.NOTIFICATION, moduleOptions, - testSuite: ({ service }: SuiteOptions) => + testSuite: ({ service }) => describe("Notification Module Service", () => { let eventBusEmitSpy @@ -66,7 +66,7 @@ moduleIntegrationTestRunner({ }) }) - it("sends a notification and stores it in the database", async () => { + it("should send a notification and stores it in the database", async () => { const notification = { to: "admin@medusa.com", template: "some-template", @@ -79,11 +79,12 @@ moduleIntegrationTestRunner({ expect.objectContaining({ provider_id: "test-provider", external_id: "external_id", + status: NotificationStatus.SUCCESS, }) ) }) - it("emits an event when a notification is created", async () => { + it("should emit an event when a notification is created", async () => { const notification = { to: "admin@medusa.com", template: "some-template", @@ -109,7 +110,7 @@ moduleIntegrationTestRunner({ ) }) - it("ensures the same notification is not sent twice", async () => { + it("should ensures the same notification is not sent twice", async () => { const notification = { to: "admin@medusa.com", template: "some-template", @@ -123,11 +124,66 @@ moduleIntegrationTestRunner({ expect.objectContaining({ provider_id: "test-provider", external_id: "external_id", + status: NotificationStatus.SUCCESS, }) ) const secondResult = await service.createNotifications(notification) expect(secondResult).toBe(undefined) }) + + it("should manage the status of multiple notification properly in any scenarios", async () => { + const notification1 = { + to: "admin@medusa.com", + template: "some-template", + channel: "email", + data: {}, + idempotency_key: "idempotency-key", + } + + const notification2 = { + to: "0000000000", + template: "some-template", + channel: "sms", + data: {}, + idempotency_key: "idempotency-key-2", + } + + const notification3 = { + to: "admin@medusa.com", + template: "some-template", + channel: "email", + data: {}, + idempotency_key: "idempotency-key-3", + } + + const err = await service + .createNotifications([notification1, notification2, notification3]) + .catch((e) => e) + + expect(err).toBeTruthy() + expect(err.message).toEqual( + "Could not find a notification provider for channel: sms" + ) + + const notifications = await service.listNotifications() + + expect(notifications).toHaveLength(3) + + const notification1Result = notifications.find( + (n) => n.idempotency_key === "idempotency-key" + )! + expect(notification1Result.status).toEqual(NotificationStatus.SUCCESS) + + const notification2Result = notifications.find( + (n) => n.idempotency_key === "idempotency-key-2" + )! + expect(notification2Result.status).toEqual(NotificationStatus.FAILURE) + + const notification3Result = notifications.find( + (n) => n.idempotency_key === "idempotency-key-3" + )! + expect(notification3Result.status).toEqual(NotificationStatus.SUCCESS) + }) }), }) diff --git a/packages/modules/notification/src/migrations/.snapshot-medusa-notification.json b/packages/modules/notification/src/migrations/.snapshot-medusa-notification.json index d3338923df240..57b5f9fd6c040 100644 --- a/packages/modules/notification/src/migrations/.snapshot-medusa-notification.json +++ b/packages/modules/notification/src/migrations/.snapshot-medusa-notification.json @@ -212,6 +212,21 @@ "nullable": true, "mappedType": "text" }, + "status": { + "name": "status", + "type": "text", + "unsigned": false, + "autoincrement": false, + "primary": false, + "nullable": false, + "default": "'pending'", + "enumItems": [ + "pending", + "success", + "failure" + ], + "mappedType": "enum" + }, "provider_id": { "name": "provider_id", "type": "text", diff --git a/packages/modules/notification/src/migrations/Migration20240830094712.ts b/packages/modules/notification/src/migrations/Migration20240830094712.ts new file mode 100644 index 0000000000000..240579cacfb13 --- /dev/null +++ b/packages/modules/notification/src/migrations/Migration20240830094712.ts @@ -0,0 +1,13 @@ +import { Migration } from '@mikro-orm/migrations'; + +export class Migration20240830094712 extends Migration { + + async up(): Promise { + this.addSql('alter table if exists "notification" add column if not exists "status" text check ("status" in (\'pending\', \'success\', \'failure\')) not null default \'pending\';'); + } + + async down(): Promise { + this.addSql('alter table if exists "notification" drop column if exists "status";'); + } + +} diff --git a/packages/modules/notification/src/models/notification.ts b/packages/modules/notification/src/models/notification.ts index a03294d41eb77..b8c6f502155a0 100644 --- a/packages/modules/notification/src/models/notification.ts +++ b/packages/modules/notification/src/models/notification.ts @@ -1,4 +1,4 @@ -import { model } from "@medusajs/utils" +import { model, NotificationStatus } from "@medusajs/utils" import { NotificationProvider } from "./notification-provider" // We probably want to have a TTL for each entry, so we don't bloat the DB (and also for GDPR reasons if TTL < 30 days). @@ -24,6 +24,9 @@ export const Notification = model.define("notification", { idempotency_key: model.text().unique().nullable(), // The ID of the notification in the external system, if applicable external_id: model.text().nullable(), + // Th status of the notification + status: model.enum(NotificationStatus).default(NotificationStatus.PENDING), + provider: model .belongsTo(() => NotificationProvider, { mappedBy: "notifications" }) .nullable(), diff --git a/packages/modules/notification/src/services/notification-module-service.ts b/packages/modules/notification/src/services/notification-module-service.ts index da00ec9a2f616..404cec8435aa7 100644 --- a/packages/modules/notification/src/services/notification-module-service.ts +++ b/packages/modules/notification/src/services/notification-module-service.ts @@ -8,13 +8,13 @@ import { NotificationTypes, } from "@medusajs/types" import { - arrayDifference, EmitEvents, generateEntityId, InjectManager, MedusaContext, MedusaError, MedusaService, + NotificationStatus, promiseAll, } from "@medusajs/utils" import { Notification } from "@models" @@ -126,15 +126,17 @@ export default class NotificationModuleService ) const existsMap = new Map( - alreadySentNotifications.map((n) => [ - n.idempotency_key as string, - true, - ]) + alreadySentNotifications.map((n) => [n.idempotency_key as string, n]) ) const notificationsToProcess = data.filter( (entry) => - !entry.idempotency_key || !existsMap.has(entry.idempotency_key) + !entry.idempotency_key || + !existsMap.has(entry.idempotency_key) || + (existsMap.has(entry.idempotency_key) && + [NotificationStatus.PENDING, NotificationStatus.FAILURE].includes( + existsMap.get(entry.idempotency_key)!.status + )) ) const channels = notificationsToProcess.map((not) => not.channel) @@ -147,11 +149,14 @@ export default class NotificationModuleService const normalizedNotificationsToProcess = notificationsToProcess.map( (entry) => { const provider = providers.find((provider) => - provider.channels.includes(entry.channel) + provider?.channels.includes(entry.channel) ) return { provider, + alreadyExists: !!( + entry.idempotency_key && existsMap.has(entry.idempotency_key) + ), data: { id: generateEntityId(undefined, "noti"), ...entry, @@ -163,7 +168,7 @@ export default class NotificationModuleService const createdNotifications = await this.notificationService_.create( normalizedNotificationsToProcess - .filter((entry) => entry.provider?.is_enabled) + .filter((e) => !e.alreadyExists) .map((e) => e.data), context ) @@ -181,18 +186,15 @@ export default class NotificationModuleService notificationsToProcess.map(async (entry) => { const provider = entry.provider - if (!provider) { - throw new MedusaError( - MedusaError.Types.NOT_FOUND, - `Could not find a notification provider for channel: ${entry.channel}` - ) - } + if (!provider?.is_enabled) { + entry.data.status = NotificationStatus.FAILURE + notificationToUpdate.push(entry.data) - if (!provider.is_enabled) { - throw new MedusaError( - MedusaError.Types.NOT_FOUND, - `Notification provider ${provider.id} is not enabled. To enable it, configure it as a provider in the notification module options.` - ) + const errorMessage = !provider + ? `Could not find a notification provider for channel: ${entry.data.channel}` + : `Notification provider ${provider.id} is not enabled. To enable it, configure it as a provider in the notification module options.` + + throw new MedusaError(MedusaError.Types.NOT_FOUND, errorMessage) } const res = await this.notificationProviderService_.send( @@ -201,20 +203,14 @@ export default class NotificationModuleService ) entry.data.external_id = res.id + entry.data.status = NotificationStatus.SUCCESS + notificationToUpdate.push(entry.data) - }) - ) - } catch (e) { - const notificationIdsToDelete = arrayDifference( - createdNotifications.map((n) => n.id), - notificationToUpdate.map((n) => n.id) - ) - await this.notificationService_.delete( - { id: notificationIdsToDelete }, - sharedContext + }), + { + aggregateErrors: true, + } ) - - throw e } finally { createdNotifications = await this.notificationService_.update( notificationToUpdate, diff --git a/packages/modules/notification/src/services/notification-provider.ts b/packages/modules/notification/src/services/notification-provider.ts index 7cd0ad934bb2e..f1d39aec2a1b2 100644 --- a/packages/modules/notification/src/services/notification-provider.ts +++ b/packages/modules/notification/src/services/notification-provider.ts @@ -63,9 +63,9 @@ export default class NotificationProviderService extends ModulesSdkUtils.MedusaI } const normalizedChannels = Array.isArray(channels) ? channels : [channels] - const results = normalizedChannels.map((channel) => - this.providersCache.get(channel) - ) + const results = normalizedChannels + .map((channel) => this.providersCache.get(channel)) + .filter(Boolean) return (Array.isArray(channels) ? results : results[0]) as TOutput } From 2f2dbad7b6185fb4526582bec3a8380ec80344de Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 12:19:33 +0200 Subject: [PATCH 3/8] fix type in tsdoc --- packages/modules/notification/src/models/notification.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/modules/notification/src/models/notification.ts b/packages/modules/notification/src/models/notification.ts index b8c6f502155a0..451c564011f90 100644 --- a/packages/modules/notification/src/models/notification.ts +++ b/packages/modules/notification/src/models/notification.ts @@ -24,7 +24,7 @@ export const Notification = model.define("notification", { idempotency_key: model.text().unique().nullable(), // The ID of the notification in the external system, if applicable external_id: model.text().nullable(), - // Th status of the notification + // The status of the notification status: model.enum(NotificationStatus).default(NotificationStatus.PENDING), provider: model From 78048bf0c854aab1f84d9af58b26bd7b0303f256 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 12:43:31 +0200 Subject: [PATCH 4/8] fix integration + return order --- .../__tests__/notification/admin/notification.spec.ts | 3 ++- .../src/services/notification-module-service.ts | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts index 33de1f9815122..bb31d3cd95d53 100644 --- a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts +++ b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts @@ -8,7 +8,7 @@ import { ContainerRegistrationKeys, ModuleRegistrationName, } from "@medusajs/utils" -import { TestEventUtils, medusaIntegrationTestRunner } from "medusa-test-utils" +import { medusaIntegrationTestRunner, TestEventUtils } from "medusa-test-utils" jest.setTimeout(50000) @@ -91,6 +91,7 @@ medusaIntegrationTestRunner({ it("should throw an exception if there is no provider for the channel", async () => { const notification = { to: "test@medusajs.com", + template: "order-created", channel: "sms", } as CreateNotificationDTO diff --git a/packages/modules/notification/src/services/notification-module-service.ts b/packages/modules/notification/src/services/notification-module-service.ts index 404cec8435aa7..7ce9ea0ac96c5 100644 --- a/packages/modules/notification/src/services/notification-module-service.ts +++ b/packages/modules/notification/src/services/notification-module-service.ts @@ -212,10 +212,18 @@ export default class NotificationModuleService } ) } finally { - createdNotifications = await this.notificationService_.update( + const updatedNotifications = await this.notificationService_.update( notificationToUpdate, sharedContext ) + const updatedNotificationsMap = new Map( + updatedNotifications.map((n) => [n.id, n]) + ) + + // Maintain the order of the notifications + createdNotifications = createdNotifications.map((notification) => { + return updatedNotificationsMap.get(notification.id) || notification + }) } return createdNotifications From 36897c5ea04d6bb8dacd89557b2ec6c9145431d7 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 13:08:49 +0200 Subject: [PATCH 5/8] include feedback --- .../services/notification-module-service.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/modules/notification/src/services/notification-module-service.ts b/packages/modules/notification/src/services/notification-module-service.ts index 7ce9ea0ac96c5..3c6748028acd1 100644 --- a/packages/modules/notification/src/services/notification-module-service.ts +++ b/packages/modules/notification/src/services/notification-module-service.ts @@ -134,9 +134,8 @@ export default class NotificationModuleService !entry.idempotency_key || !existsMap.has(entry.idempotency_key) || (existsMap.has(entry.idempotency_key) && - [NotificationStatus.PENDING, NotificationStatus.FAILURE].includes( - existsMap.get(entry.idempotency_key)!.status - )) + existsMap.get(entry.idempotency_key)!.status === + NotificationStatus.FAILURE) ) const channels = notificationsToProcess.map((not) => not.channel) @@ -154,9 +153,6 @@ export default class NotificationModuleService return { provider, - alreadyExists: !!( - entry.idempotency_key && existsMap.has(entry.idempotency_key) - ), data: { id: generateEntityId(undefined, "noti"), ...entry, @@ -166,12 +162,16 @@ export default class NotificationModuleService } ) - const createdNotifications = await this.notificationService_.create( - normalizedNotificationsToProcess - .filter((e) => !e.alreadyExists) - .map((e) => e.data), - context - ) + const toCreate = normalizedNotificationsToProcess + .filter( + (e) => + !e.data.idempotency_key || !existsMap.has(e.data.idempotency_key) + ) + .map((e) => e.data) + + const createdNotifications = toCreate.length + ? await this.notificationService_.create(toCreate, context) + : [] return { notificationsToProcess: normalizedNotificationsToProcess, From 96af36d5dba766e9513e11c0df102a156347bc62 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 14:13:23 +0200 Subject: [PATCH 6/8] properly handle send failure error and improve error messages --- .../providers/default-provider.ts | 3 ++ .../notification-module-service/index.spec.ts | 34 +++++++++++++++---- .../services/notification-module-service.ts | 16 ++++++--- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/packages/modules/notification/integration-tests/__fixtures__/providers/default-provider.ts b/packages/modules/notification/integration-tests/__fixtures__/providers/default-provider.ts index 829bb20357717..17620ee7a54f8 100644 --- a/packages/modules/notification/integration-tests/__fixtures__/providers/default-provider.ts +++ b/packages/modules/notification/integration-tests/__fixtures__/providers/default-provider.ts @@ -7,6 +7,9 @@ export class NotificationProviderServiceFixtures extends AbstractNotificationPro async send( notification: NotificationTypes.ProviderSendNotificationDTO ): Promise { + if (notification.to === "fail") { + throw new Error("Failed to send notification") + } return { id: "external_id" } } } diff --git a/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts b/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts index 612fe01d653ab..a52c1ec20007d 100644 --- a/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts +++ b/packages/modules/notification/integration-tests/__tests__/notification-module-service/index.spec.ts @@ -157,18 +157,26 @@ moduleIntegrationTestRunner({ idempotency_key: "idempotency-key-3", } + const notification4 = { + to: "fail", + template: "some-template", + channel: "email", + data: {}, + idempotency_key: "idempotency-key-4", + } + const err = await service - .createNotifications([notification1, notification2, notification3]) + .createNotifications([ + notification1, + notification2, + notification3, + notification4, + ]) .catch((e) => e) - expect(err).toBeTruthy() - expect(err.message).toEqual( - "Could not find a notification provider for channel: sms" - ) - const notifications = await service.listNotifications() - expect(notifications).toHaveLength(3) + expect(notifications).toHaveLength(4) const notification1Result = notifications.find( (n) => n.idempotency_key === "idempotency-key" @@ -184,6 +192,18 @@ moduleIntegrationTestRunner({ (n) => n.idempotency_key === "idempotency-key-3" )! expect(notification3Result.status).toEqual(NotificationStatus.SUCCESS) + + const notification4Result = notifications.find( + (n) => n.idempotency_key === "idempotency-key-4" + )! + expect(notification4Result.status).toEqual(NotificationStatus.FAILURE) + + expect(err).toBeTruthy() + expect(err.message).toEqual( + `Could not find a notification provider for channel: sms for notification id ${notification2Result.id} +Failed to send notification with id ${notification4Result.id}: +Failed to send notification` + ) }) }), }) diff --git a/packages/modules/notification/src/services/notification-module-service.ts b/packages/modules/notification/src/services/notification-module-service.ts index 3c6748028acd1..a0f327881938b 100644 --- a/packages/modules/notification/src/services/notification-module-service.ts +++ b/packages/modules/notification/src/services/notification-module-service.ts @@ -191,16 +191,22 @@ export default class NotificationModuleService notificationToUpdate.push(entry.data) const errorMessage = !provider - ? `Could not find a notification provider for channel: ${entry.data.channel}` + ? `Could not find a notification provider for channel: ${entry.data.channel} for notification id ${entry.data.id}` : `Notification provider ${provider.id} is not enabled. To enable it, configure it as a provider in the notification module options.` throw new MedusaError(MedusaError.Types.NOT_FOUND, errorMessage) } - const res = await this.notificationProviderService_.send( - provider, - entry.data - ) + const res = await this.notificationProviderService_ + .send(provider, entry.data) + .catch((e) => { + entry.data.status = NotificationStatus.FAILURE + notificationToUpdate.push(entry.data) + throw new MedusaError( + MedusaError.Types.UNEXPECTED_STATE, + `Failed to send notification with id ${entry.data.id}:\n${e.message}` + ) + }) entry.data.external_id = res.id entry.data.status = NotificationStatus.SUCCESS From 47918579af69917924e18a58177df5cc311cf202 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 14:27:44 +0200 Subject: [PATCH 7/8] fix tests --- .../__tests__/notification/admin/notification.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts index bb31d3cd95d53..24ceb0dc1d606 100644 --- a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts +++ b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts @@ -98,8 +98,11 @@ medusaIntegrationTestRunner({ const error = await service .createNotifications(notification) .catch((e) => e) + + const [notificationResult] = await service.listNotifications() + expect(error.message).toEqual( - "Could not find a notification provider for channel: sms" + `Could not find a notification provider for channel: sms for notification: ${notificationResult.id}` ) }) From 3eb45cf185965b001302e9837a9f5458a09c26f0 Mon Sep 17 00:00:00 2001 From: adrien2p Date: Fri, 30 Aug 2024 14:42:50 +0200 Subject: [PATCH 8/8] fix tests --- .../modules/__tests__/notification/admin/notification.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts index 24ceb0dc1d606..0c568bc49fecc 100644 --- a/integration-tests/modules/__tests__/notification/admin/notification.spec.ts +++ b/integration-tests/modules/__tests__/notification/admin/notification.spec.ts @@ -102,7 +102,7 @@ medusaIntegrationTestRunner({ const [notificationResult] = await service.listNotifications() expect(error.message).toEqual( - `Could not find a notification provider for channel: sms for notification: ${notificationResult.id}` + `Could not find a notification provider for channel: sms for notification id ${notificationResult.id}` ) })