fix: code review findings

Signed-off-by: Heikki Hellgren <heikki.hellgren@op.fi>

plugins/notifications-node/src/extensions.ts

@@ -0,0 +1,54 @@
+/*
+ * Copyright 2024 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { createExtensionPoint } from '@backstage/backend-plugin-api';
+import { Notification } from '@backstage/plugin-notifications-common';
+
+/**
+ * @public
+ */
+export interface NotificationProcessor {
+  /**
+   * Decorate notification before sending it
+   *
+   * @param notification - The notification to decorate
+   * @returns The same notification or a modified version of it
+   */
+  decorate?(notification: Notification): Promise<Notification>;
+
+  /**
+   * Send notification using this processor.
+   *
+   * @param notification - The notification to send
+   */
+  send?(notification: Notification): Promise<void>;
+}
+
+/**
+ * @public
+ */
+export interface NotificationsProcessingExtensionPoint {
+  addProcessor(
+    ...processors: Array<NotificationProcessor | Array<NotificationProcessor>>
+  ): void;
+}
+
+/**
+ * @public
+ */
+export const notificationsProcessingExtensionPoint =
+  createExtensionPoint<NotificationsProcessingExtensionPoint>({
+    id: 'notifications.processing',
+  });

plugins/notifications-node/src/index.ts

@@ -22,3 +22,4 @@
 
 export * from './service';
 export * from './lib';
+export * from './extensions';

plugins/notifications-node/src/lib.ts

@@ -20,7 +20,6 @@ import {
 } from '@backstage/backend-plugin-api';
 import { DefaultNotificationService } from './service';
 import { NotificationService } from './service/NotificationService';
-import { signalService } from '@backstage/plugin-signals-node';
 
 /** @public */
 export const notificationService = createServiceRef<NotificationService>({
@@ -30,20 +29,16 @@ export const notificationService = createServiceRef<NotificationService>({
     createServiceFactory({
       service,
       deps: {
-        logger: coreServices.rootLogger,
         discovery: coreServices.discovery,
         tokenManager: coreServices.tokenManager,
         pluginMetadata: coreServices.pluginMetadata,
-        signals: signalService,
       },
-      factory({ logger, discovery, tokenManager, signals, pluginMetadata }) {
-        // TODO: Convert to use createRootContext
+      factory({ discovery, tokenManager, pluginMetadata }) {
         return DefaultNotificationService.create({
-          logger,
           discovery,
           tokenManager,
-          signalService: signals,
-        }).forPlugin(pluginMetadata.getId());
+          pluginId: pluginMetadata.getId(),
+        });
       },
     }),
 });

plugins/notifications-node/src/service/DefaultNotificationService.ts

@@ -15,16 +15,14 @@
  */
 import { TokenManager } from '@backstage/backend-common';
 import { NotificationService } from './NotificationService';
-import { DiscoveryService, LoggerService } from '@backstage/backend-plugin-api';
-import { SignalService } from '@backstage/plugin-signals-node';
+import { DiscoveryService } from '@backstage/backend-plugin-api';
 import { NotificationPayload } from '@backstage/plugin-notifications-common';
 
 /** @public */
 export type NotificationServiceOptions = {
-  logger: LoggerService;
   discovery: DiscoveryService;
   tokenManager: TokenManager;
-  signalService: SignalService;
+  pluginId: string;
 };
 
 /** @public */
@@ -45,34 +43,20 @@ export type NotificationSendOptions = {
 /** @public */
 export class DefaultNotificationService implements NotificationService {
   private constructor(
-    private readonly logger: LoggerService,
     private readonly discovery: DiscoveryService,
     private readonly tokenManager: TokenManager,
-    private readonly pluginId?: string,
+    private readonly pluginId: string,
   ) {}
 
   static create({
-    logger,
     tokenManager,
     discovery,
+    pluginId,
   }: NotificationServiceOptions): DefaultNotificationService {
-    return new DefaultNotificationService(logger, discovery, tokenManager);
-  }
-
-  forPlugin(pluginId: string): NotificationService {
-    return new DefaultNotificationService(
-      this.logger,
-      this.discovery,
-      this.tokenManager,
-      pluginId,
-    );
+    return new DefaultNotificationService(discovery, tokenManager, pluginId);
   }
 
   async send(notification: NotificationSendOptions): Promise<void> {
-    if (!this.pluginId) {
-      throw new Error('Invalid initialization of the NotificationService');
-    }
-
     try {
       const baseUrl = await this.discovery.getBaseUrl('notifications');
       const { token } = await this.tokenManager.getToken();
@@ -80,6 +64,7 @@ export class DefaultNotificationService implements NotificationService {
         method: 'POST',
         body: JSON.stringify({
           ...notification,
+          // TODO: Should retrieve this in the backend from service auth instead
           origin: `plugin-${this.pluginId}`,
         }),
         headers: {
@@ -88,7 +73,8 @@ export class DefaultNotificationService implements NotificationService {
         },
       });
     } catch (error) {
-      this.logger.error(`Failed to send notifications: ${error}`);
+      // TODO: Should not throw in optimal case, see BEP
+      throw new Error(`Failed to send notifications: ${error}`);
     }
   }
 }

plugins/notifications-node/src/service/NotificationService.ts

@@ -18,7 +18,5 @@ import { NotificationSendOptions } from './DefaultNotificationService';
 
 /** @public */
 export interface NotificationService {
-  forPlugin(pluginId: string): NotificationService;
-
   send(options: NotificationSendOptions): Promise<void>;
 }