check explicit user refs from options.recipients

Signed-off-by: Kai Dubauskas <kai.dubauskas@doordash.com>

plugins/notifications-backend-module-slack/src/lib/SlackNotificationProcessor.test.ts

@@ -16,7 +16,6 @@
 
 import { mockServices } from '@backstage/backend-test-utils';
 import { SlackNotificationProcessor } from './SlackNotificationProcessor';
-import { USER_REFS_FROM_REQUEST_KEY } from './constants';
 import { catalogServiceMock } from '@backstage/plugin-catalog-node/testUtils';
 import { WebClient } from '@slack/web-api';
 import { Entity } from '@backstage/catalog-model';
@@ -319,32 +318,20 @@ describe('SlackNotificationProcessor', () => {
     });
   });
 
-  describe('when group and user recipients are both provided', () => {
-    it('should send a group message and only DM explicit users', async () => {
+  describe('when recipients include both users and a group', () => {
+    it('should still DM explicit user recipients', async () => {
       const slack = new WebClient();
-      const extraUser: Entity = {
-        apiVersion: 'backstage.io/v1alpha1',
-        kind: 'User',
-        metadata: {
-          name: 'other',
-          namespace: 'default',
-          annotations: {
-            'slack.com/bot-notify': 'U99999999',
-          },
-        },
-        spec: {},
-      };
 
       const processor = SlackNotificationProcessor.fromConfig(config, {
         auth,
         logger,
         catalog: catalogServiceMock({
-          entities: [...DEFAULT_ENTITIES_RESPONSE.items, extraUser],
+          entities: DEFAULT_ENTITIES_RESPONSE.items,
         }),
         slack,
       })[0];
 
-      const processedOptions = await processor.processOptions({
+      await processor.processOptions({
         recipients: {
           type: 'entity',
           entityRef: ['group:default/mock', 'user:default/mock'],
@@ -352,44 +339,25 @@ describe('SlackNotificationProcessor', () => {
         payload: { title: 'notification' },
       });
 
-      expect(processedOptions.payload.metadata).toEqual(
-        expect.objectContaining({
-          [USER_REFS_FROM_REQUEST_KEY]: ['user:default/mock'],
-        }),
-      );
-
-      await processor.postProcess(
-        {
-          origin: 'plugin',
-          id: 'group-user-1',
-          user: 'user:default/other',
-          created: new Date(),
-          payload: {
-            title: 'notification',
-            link: '/catalog/user/default/other',
-          },
-        },
-        processedOptions,
-      );
-
-      expect(slack.chat.postMessage).toHaveBeenCalledTimes(1);
-
       await processor.postProcess(
         {
           origin: 'plugin',
           id: 'explicit-user-1',
           user: 'user:default/mock',
           created: new Date(),
-          payload: {
-            title: 'notification',
-            link: '/catalog/user/default/mock',
-          },
+          payload: { title: 'notification' },
+        },
+        {
+          recipients: {
+            type: 'entity',
+            entityRef: ['group:default/mock', 'user:default/mock'],
+          },
+          payload: { title: 'notification' },
         },
-        processedOptions,
       );
 
       expect(slack.chat.postMessage).toHaveBeenCalledTimes(2);
-      expect(slack.chat.postMessage).toHaveBeenLastCalledWith(
+      expect(slack.chat.postMessage).toHaveBeenCalledWith(
         expect.objectContaining({ channel: 'U12345678' }),
       );
     });

plugins/notifications-backend-module-slack/src/lib/SlackNotificationProcessor.ts

@@ -34,10 +34,7 @@ import { Counter, metrics } from '@opentelemetry/api';
 import { ChatPostMessageArguments, WebClient } from '@slack/web-api';
 import DataLoader from 'dataloader';
 import pThrottle from 'p-throttle';
-import {
-  ANNOTATION_SLACK_BOT_NOTIFY,
-  USER_REFS_FROM_REQUEST_KEY,
-} from './constants';
+import { ANNOTATION_SLACK_BOT_NOTIFY } from './constants';
 import { BroadcastRoute } from './types';
 import { ExpiryMap, toChatPostMessageArgs } from './util';
 import { CatalogService } from '@backstage/plugin-catalog-node';
@@ -214,14 +211,12 @@ export class SlackNotificationProcessor implements NotificationProcessor {
 
     const entityRefs = [options.recipients.entityRef].flat();
 
-    const userEntityRefs: string[] = [];
     const outbound: ChatPostMessageArguments[] = [];
     await Promise.all(
       entityRefs.map(async entityRef => {
         const compoundEntityRef = parseEntityRef(entityRef);
         // skip users as they are sent direct messages
         if (compoundEntityRef.kind === 'user') {
-          userEntityRefs.push(stringifyEntityRef(compoundEntityRef));
           return;
         }
 
@@ -263,18 +258,7 @@ export class SlackNotificationProcessor implements NotificationProcessor {
 
     await this.sendNotifications(outbound);
 
-    const enrichedPayload = {
-      ...options.payload,
-      metadata: {
-        ...options.payload.metadata,
-        [USER_REFS_FROM_REQUEST_KEY]: userEntityRefs,
-      },
-    };
-
-    return {
-      ...options,
-      payload: enrichedPayload,
-    };
+    return options;
   }
 
   async postProcess(
@@ -289,25 +273,20 @@ export class SlackNotificationProcessor implements NotificationProcessor {
       destinations.push(...routedChannels);
     } else if (options.recipients.type === 'entity') {
       // Handle user-specific notification
-      const rawUserRefs =
-        options.payload.metadata?.[USER_REFS_FROM_REQUEST_KEY];
-
+      const entityRefs = [options.recipients.entityRef].flat();
+      const explicitUserEntityRefs = entityRefs
+        .filter(entityRef => parseEntityRef(entityRef).kind === 'user')
+        .map(entityRef => stringifyEntityRef(parseEntityRef(entityRef)));
       const normalizedUserRef = stringifyEntityRef(
         parseEntityRef(notification.user),
       );
-      const hasValidUserRefs =
-        Array.isArray(rawUserRefs) &&
-        rawUserRefs.every(ref => typeof ref === 'string');
-      const explicitUserEntityRefs = hasValidUserRefs
-        ? rawUserRefs.map(ref => stringifyEntityRef(parseEntityRef(ref)))
-        : undefined;
 
-      if (!explicitUserEntityRefs?.includes(normalizedUserRef)) {
-        const entityRefs = [options.recipients.entityRef].flat();
-        if (entityRefs.some(e => parseEntityRef(e).kind === 'group')) {
-          // This user was resolved from a non-user entity and we've already send a channel DM so let's not send another.
-          return;
-        }
+      if (
+        entityRefs.some(e => parseEntityRef(e).kind === 'group') &&
+        !explicitUserEntityRefs.includes(normalizedUserRef)
+      ) {
+        // This user was resolved from a non-user entity and we've already sent a group channel message.
+        return;
       }
 
       const destination = await this.getSlackNotificationTarget(

plugins/notifications-backend-module-slack/src/lib/constants.ts

@@ -27,9 +27,3 @@
  * however IDs are preferred.
  */
 export const ANNOTATION_SLACK_BOT_NOTIFY = 'slack.com/bot-notify';
-
-/**
- * @public
- * Metadata key containing a list of all user entities before resolution
- */
-export const USER_REFS_FROM_REQUEST_KEY = 'userRefsFromRequest';