fix: skipped user DMs when group is present

Closes #32584

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

.changeset/curvy-socks-punch.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-notifications-backend-module-slack': patch
+---
+
+Fix skipped slack DMS for users when a group entity is a recipient

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

@@ -16,6 +16,7 @@
 
 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';
@@ -318,6 +319,82 @@ describe('SlackNotificationProcessor', () => {
     });
   });
 
+  describe('when group and user recipients are both provided', () => {
+    it('should send a group message and only DM explicit users', 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],
+        }),
+        slack,
+      })[0];
+
+      const processedOptions = await processor.processOptions({
+        recipients: {
+          type: 'entity',
+          entityRef: ['group:default/mock', 'user:default/mock'],
+        },
+        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',
+          },
+        },
+        processedOptions,
+      );
+
+      expect(slack.chat.postMessage).toHaveBeenCalledTimes(2);
+      expect(slack.chat.postMessage).toHaveBeenLastCalledWith(
+        expect.objectContaining({ channel: 'U12345678' }),
+      );
+    });
+  });
+
   describe('when broadcast channels are not configured', () => {
     it('should not send broadcast messages', async () => {
       const slack = new WebClient();

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

@@ -19,6 +19,7 @@ import {
   Entity,
   isUserEntity,
   parseEntityRef,
+  stringifyEntityRef,
   UserEntity,
 } from '@backstage/catalog-model';
 import { Config, readDurationFromConfig } from '@backstage/config';
@@ -33,7 +34,10 @@ 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 } from './constants';
+import {
+  ANNOTATION_SLACK_BOT_NOTIFY,
+  USER_REFS_FROM_REQUEST_KEY,
+} from './constants';
 import { BroadcastRoute } from './types';
 import { ExpiryMap, toChatPostMessageArgs } from './util';
 import { CatalogService } from '@backstage/plugin-catalog-node';
@@ -210,12 +214,14 @@ 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;
         }
 
@@ -257,7 +263,18 @@ export class SlackNotificationProcessor implements NotificationProcessor {
 
     await this.sendNotifications(outbound);
 
-    return options;
+    const enrichedPayload = {
+      ...options.payload,
+      metadata: {
+        ...options.payload.metadata,
+        [USER_REFS_FROM_REQUEST_KEY]: userEntityRefs,
+      },
+    };
+
+    return {
+      ...options,
+      payload: enrichedPayload,
+    };
   }
 
   async postProcess(
@@ -272,10 +289,25 @@ export class SlackNotificationProcessor implements NotificationProcessor {
       destinations.push(...routedChannels);
     } else if (options.recipients.type === 'entity') {
       // Handle user-specific notification
-      const entityRefs = [options.recipients.entityRef].flat();
-      if (entityRefs.some(e => parseEntityRef(e).kind === 'group')) {
-        // We've already dispatched a slack channel message, so let's not send a DM.
-        return;
+      const rawUserRefs =
+        options.payload.metadata?.[USER_REFS_FROM_REQUEST_KEY];
+
+      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;
+        }
       }
 
       const destination = await this.getSlackNotificationTarget(

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

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