diff --git a/plugins/rw-backend-module-notifications/src/CommentNotifier.test.ts b/plugins/rw-backend-module-notifications/src/CommentNotifier.test.ts index 22349fc..ec9f3d3 100644 --- a/plugins/rw-backend-module-notifications/src/CommentNotifier.test.ts +++ b/plugins/rw-backend-module-notifications/src/CommentNotifier.test.ts @@ -36,7 +36,14 @@ describe("CommentNotifier", () => { expect(send).toHaveBeenCalledTimes(1); const arg = send.mock.calls[0][0]; - expect(arg.recipients).toEqual({ type: "entity", entityRef: ["group:default/team"] }); + // The recipient is a group and the actor is forwarded as excludeEntityRef: this is what + // lets Backstage's resolver drop a group-owning actor from the expanded set (so a member + // commenting on their own entity is not self-notified). + expect(arg.recipients).toEqual({ + type: "entity", + entityRef: ["group:default/team"], + excludeEntityRef: "user:default/alice", + }); expect(arg.payload.title).toBe("Alice Smith commented on Setup Guide · My Service"); expect(arg.payload.description).toBe("please review"); expect(arg.payload.link).toBe("/catalog/default/component/site/docs/guide/setup#comment-c1"); @@ -44,6 +51,13 @@ describe("CommentNotifier", () => { expect(arg.payload.severity).toBe("normal"); }); + it("drops a payload with no actorRef without sending (exclusion would be a no-op)", async () => { + const send = jest.fn().mockResolvedValue(undefined); + const notifier = new CommentNotifier({ notifications: { send } as any, logger }); + await notifier.handle(payload({ actorRef: "" })); + expect(send).not.toHaveBeenCalled(); + }); + it("participants/created → ' replied on '", async () => { const send = jest.fn().mockResolvedValue(undefined); const notifier = new CommentNotifier({ notifications: { send } as any, logger }); diff --git a/plugins/rw-backend-module-notifications/src/CommentNotifier.ts b/plugins/rw-backend-module-notifications/src/CommentNotifier.ts index 04bec6a..2010339 100644 --- a/plugins/rw-backend-module-notifications/src/CommentNotifier.ts +++ b/plugins/rw-backend-module-notifications/src/CommentNotifier.ts @@ -4,10 +4,12 @@ import { NotificationService } from "@backstage/plugin-notifications-node"; import { CommentEventPayload } from "@rwdocs/backstage-plugin-rw-common"; /** Subscriber-side: turns a self-contained `rw.comments` event into a native notification. - * Pure presentation + delivery — no DB, no recipient resolution. The one catalog-path - * convention (`/catalog///`) is isolated here: the backend can't resolve - * the frontend's catalog routeRef, so the app-relative link is composed from the entity ref - * by convention. Best-effort: catches + logs, always resolves. */ + * Presentation + delivery — no DB. The publisher emits raw owner/participant recipients (which may + * include the actor); this is the single point of actor exclusion: the actor is forwarded as + * `excludeEntityRef` so Backstage's resolver drops them after expanding groups. The one catalog-path + * convention (`/catalog///`) is isolated here: the backend can't resolve the + * frontend's catalog routeRef, so the app-relative link is composed from the entity ref by + * convention. Best-effort: catches + logs, always resolves. */ export class CommentNotifier { private readonly notifications: NotificationService; private readonly logger: LoggerService; @@ -26,7 +28,13 @@ export class CommentNotifier { } try { await this.notifications.send({ - recipients: { type: "entity", entityRef: payload.recipients }, + // Single point of actor exclusion (see class doc): the resolver drops the actor from + // the resolved recipients, including from an expanded group. + recipients: { + type: "entity", + entityRef: payload.recipients, + excludeEntityRef: payload.actorRef, + }, payload: { title: this.title(payload), description: this.description(payload), @@ -51,12 +59,16 @@ export class CommentNotifier { /** Defensive shape guard at the subscriber boundary: the module casts the raw event * payload to `CommentEventPayload`, so a malformed or foreign event on the topic would - * otherwise flow straight into `send`. Validates the fields this notifier relies on. */ + * otherwise flow straight into `send`. Validates the fields this notifier relies on — + * including `actorRef`, the sole basis for actor exclusion (passed as `excludeEntityRef`); + * without it the exclusion silently becomes a no-op and the actor self-notifies. */ private isValid(payload: CommentEventPayload): boolean { return ( !!payload && (payload.kind === "created" || payload.kind === "resolved") && typeof payload.rootId === "string" && + typeof payload.actorRef === "string" && + payload.actorRef.length > 0 && Array.isArray(payload.recipients) && payload.recipients.length > 0 ); diff --git a/plugins/rw-backend-module-notifications/src/module.test.ts b/plugins/rw-backend-module-notifications/src/module.test.ts index e41d758..4fa41a0 100644 --- a/plugins/rw-backend-module-notifications/src/module.test.ts +++ b/plugins/rw-backend-module-notifications/src/module.test.ts @@ -101,6 +101,7 @@ describe("rw notifications module — real startTestBackend integration", () => expect(send.mock.calls[0][0].recipients).toEqual({ type: "entity", entityRef: ["group:default/team"], + excludeEntityRef: "user:default/alice", }); }); diff --git a/plugins/rw-backend/src/comments/CommentEventPublisher.test.ts b/plugins/rw-backend/src/comments/CommentEventPublisher.test.ts index b830684..35d08e3 100644 --- a/plugins/rw-backend/src/comments/CommentEventPublisher.test.ts +++ b/plugins/rw-backend/src/comments/CommentEventPublisher.test.ts @@ -77,6 +77,7 @@ describe("CommentEventPublisher", () => { kind: "created", audience: "owner", recipients: ["group:default/team"], + actorRef: "user:default/alice", // forwarded as excludeEntityRef by the notifier entityRef: "component:default/site", pageRef: "sec-1#setup", deepLinkSuffix: "/docs/guide/setup#comment-c1", @@ -187,7 +188,43 @@ describe("CommentEventPublisher", () => { expect(published).toHaveLength(0); }); - it("commenter-side: reply notifies prior participants minus the actor", async () => { + it("owner-side: still publishes when the section owner IS the actor (recipients=[actor], dropped at delivery)", async () => { + // Guards the refactor: the publisher no longer strips the actor from owner recipients, so an + // owner commenting on their own section still emits an owner-side event (recipients=[actor]); + // the notifier's excludeEntityRef drops them at delivery. If the literal-ref filter were + // re-added here, recipients would empty out and this event would silently stop publishing — + // re-opening the self-notify hole. (The other owner-side fixtures use owner=group != actor, + // so the deleted filter was a no-op in them and would NOT catch such a regression.) + const { events, published } = fakeEvents(); + const sections = { + getSection: async () => ({ + site_ref: "component:default/site", + section_ref: "sec-1", + section_path: "guide", + parent_section_ref: null, + entity_ref: "component:default/site", + entity_owner_ref: "user:default/alice", + }), + } as any; + const pub = new CommentEventPublisher({ + events, + sections, + comments: { participantsOf: async () => [] } as any, + logger, + pages: fakePages(), + }); + + await pub.onCommentCreated(row({ parent_id: null }), "user:default/alice"); + + expect(published).toHaveLength(1); + expect(published[0].eventPayload).toMatchObject({ + audience: "owner", + recipients: ["user:default/alice"], + actorRef: "user:default/alice", + }); + }); + + it("commenter-side: reply recipients include all thread participants (actor not stripped here)", async () => { const { events, published } = fakeEvents(); const sections = { getSection: async () => ({ @@ -221,14 +258,19 @@ describe("CommentEventPublisher", () => { audience: "participants", commentId: "c2", rootId: "c1", - recipients: ["user:default/alice"], + // The actor (bob) is NOT stripped here — recipients are the raw participant set; the + // notifier forwards the actor as excludeEntityRef so the resolver drops them at delivery. + recipients: ["user:default/alice", "user:default/bob"], // deeplink anchors on the thread root (c1), not the reply (c2), // so opening the notification lands on the whole thread. deepLinkSuffix: "/docs/guide/setup#comment-c1", }); }); - it("commenter-side: no publish when the actor is the only participant", async () => { + it("commenter-side: still publishes when the actor is the only participant (resolver drops them at delivery)", async () => { + // The publisher no longer short-circuits the actor-only case; it emits the raw participant + // set and the notifier's excludeEntityRef makes the resolver drop the actor, yielding no + // notification. (Slightly more work than the old early-exit, but a single exclusion path.) const { events, published } = fakeEvents(); const comments = { participantsOf: async () => ["user:default/bob"] } as any; const pub = new CommentEventPublisher({ @@ -242,7 +284,8 @@ describe("CommentEventPublisher", () => { row({ id: "c2", parent_id: "c1", author_ref: "user:default/bob" }), "user:default/bob", ); - expect(published).toHaveLength(0); + expect(published).toHaveLength(1); + expect(published[0].eventPayload).toMatchObject({ recipients: ["user:default/bob"] }); }); it("owner-side: actorName comes from author_profile in the row", async () => { @@ -271,7 +314,7 @@ describe("CommentEventPublisher", () => { expect(published[0].eventPayload).toMatchObject({ actorName: "Alice Smith" }); }); - it("resolve: notifies all participants minus the resolver; uses passed-in actorName", async () => { + it("resolve: recipients include all participants (resolver not stripped here); uses passed-in actorName", async () => { const { events, published } = fakeEvents(); const sections = { getSection: async () => undefined } as any; // degraded link OK const comments = { @@ -295,7 +338,8 @@ describe("CommentEventPublisher", () => { expect(published[0].eventPayload).toMatchObject({ kind: "resolved", audience: "participants", - recipients: ["user:default/alice"], + // resolver (bob) is not stripped here; excludeEntityRef drops them at delivery. + recipients: ["user:default/alice", "user:default/bob"], entityRef: null, // degraded: no section row actorName: "Bob Builder", }); diff --git a/plugins/rw-backend/src/comments/CommentEventPublisher.ts b/plugins/rw-backend/src/comments/CommentEventPublisher.ts index 66f1852..7010486 100644 --- a/plugins/rw-backend/src/comments/CommentEventPublisher.ts +++ b/plugins/rw-backend/src/comments/CommentEventPublisher.ts @@ -16,10 +16,13 @@ import { authorFromRow } from "./author"; import { CommentRow, subpathOf } from "./types"; /** Publishes self-contained `rw.comments` domain events after a comment write commits. - * Owns recipient + deep-link resolution (it has the comments + sections tables) so the - * notifications module can stay a thin sender. Best-effort: every method catches and - * logs, and always resolves, so a publish failure can never affect the comment write. - * Callers invoke fire-and-forget. */ + * Owns recipient + deep-link resolution from the comments + sections tables, so the + * notifications module stays a thin sender. `recipients` are the raw section owner / thread + * participants and may include the actor; actor exclusion is done entirely subscriber-side via + * `excludeEntityRef` (see CommentNotifier), which the recipient resolver applies after expanding + * groups — the one place that also catches an actor who owns the section through a group. + * Best-effort: every method catches and logs, and always resolves, so a publish failure can + * never affect the comment write. Callers invoke fire-and-forget. */ export class CommentEventPublisher { private readonly events: EventsService; private readonly sections: SectionsReader; @@ -78,8 +81,8 @@ export class CommentEventPublisher { private async publishOwnerSide(row: CommentRow, actorRef: string): Promise { const section = await this.sections.getSection(row.site_ref, row.section_ref); if (!section || !section.entity_owner_ref) return; // new/unowned section: inbox catches it - const recipients = [section.entity_owner_ref].filter((ref) => ref !== actorRef); - if (recipients.length === 0) return; + // Actor left in even when they own the section; excluded at delivery (see class doc). + const recipients = [section.entity_owner_ref]; const subpath = subpathOf(row.page_ref); const viewerPath = joinNonEmpty([section.section_path, subpath], "/"); const actorName = authorFromRow(row).name; @@ -103,10 +106,10 @@ export class CommentEventPublisher { actorRef: string, resolvedActorName?: string, ): Promise { - // participantsOf already returns distinct refs, so no extra dedup is needed here. - const participants = await this.comments.participantsOf(rootId); - const recipients = participants.filter((ref) => ref !== actorRef); - if (recipients.length === 0) return; + // participantsOf returns distinct refs (no extra dedup); the actor is left in and excluded + // at delivery (see class doc). + const recipients = await this.comments.participantsOf(rootId); + if (recipients.length === 0) return; // defensive: thread with no live participants (e.g. root deleted) // The section row provides entityRef (link target) + section_path (path prefix); degrade gracefully if absent: entityRef -> null (module emits no link), viewerPath -> bare subpath. const section = await this.sections.getSection(row.site_ref, row.section_ref); const subpath = subpathOf(row.page_ref); diff --git a/plugins/rw-common/src/commentEvents.ts b/plugins/rw-common/src/commentEvents.ts index ff9c554..00d5c50 100644 --- a/plugins/rw-common/src/commentEvents.ts +++ b/plugins/rw-common/src/commentEvents.ts @@ -13,8 +13,8 @@ export interface CommentEventPayload { siteRef: string; sectionRef: string; pageRef: string; // identifies the page within the section ("#") - actorRef: string; // already removed from `recipients` - recipients: string[]; // catalog entity refs, non-empty + actorRef: string; // the user who triggered the event; forwarded as excludeEntityRef so the resolver drops them (see CommentNotifier) + recipients: string[]; // catalog entity refs, non-empty; raw owner/participants — may include actorRef entityRef: string | null; // owning entity (sections.entity_ref); null = degraded link // prefix-free deep-link suffix, always "/docs/#comment-". Normal: viewerPath = section_path + "/" + subpath. Degraded (no section row): viewerPath = subpath only (section prefix omitted). deepLinkSuffix: string;