Merge pull request #1372 from assemblycom/OUT-3928-send-task-marked-as-done-emails-to-ius

OUT-3928 | Send task-marked-as-done email notifications to IUs

Author: arpandhakal

src/app/api/notification/notification.helpers.test.ts

@@ -1,7 +1,7 @@
 import { NotificationTaskActions } from '@api/core/types/tasks'
 import { WorkspaceResponse } from '@/types/common'
 import { getEmailDetails, getReminderEmailDetails } from './notification.helpers'
-import { TaskReminderType } from '@prisma/client'
+import { Task, TaskReminderType } from '@prisma/client'
 
 const workspace: WorkspaceResponse = {
   id: 'ws_1',
@@ -81,13 +81,30 @@ describe('getReminderEmailDetails', () => {
 describe('getEmailDetails', () => {
   // Actions that email an IU recipient must have a template here, or the grouped
   // buffer silently skips them (in-product fires but no email is ever flushed).
-  it.each([NotificationTaskActions.Assigned, NotificationTaskActions.ReassignedToIU])(
-    'defines an email template for IU-recipient action %s',
-    (action) => {
-      const details = getEmailDetails(workspace, 'Arpan Two')[action]
-      expect(details).toBeDefined()
-      expect(details?.subject).toBeTruthy()
-      expect(details?.body).toBeTruthy()
-    },
-  )
+  it.each([
+    NotificationTaskActions.Assigned,
+    NotificationTaskActions.ReassignedToIU,
+    NotificationTaskActions.Completed,
+    NotificationTaskActions.CompletedByIU,
+    NotificationTaskActions.CompletedByCompanyMember,
+    NotificationTaskActions.CompletedForCompanyByIU,
+  ])('defines an email template for IU-recipient action %s', (action) => {
+    const details = getEmailDetails(workspace, 'Arpan Two')[action]
+    expect(details).toBeDefined()
+    expect(details?.subject).toBeTruthy()
+    expect(details?.body).toBeTruthy()
+  })
+
+  it.each([
+    NotificationTaskActions.Completed,
+    NotificationTaskActions.CompletedByIU,
+    NotificationTaskActions.CompletedByCompanyMember,
+    NotificationTaskActions.CompletedForCompanyByIU,
+  ])('uses the task-marked-as-done copy for completion action %s', (action) => {
+    const details = getEmailDetails(workspace, 'Casey Client', task as unknown as Task)[action]
+    expect(details?.subject).toBe('Task marked as done')
+    expect(details?.header).toBe('A task has been completed')
+    expect(details?.body).toContain('has been marked as done by Casey Client')
+    expect(details?.title).toBe('View task')
+  })
 })

src/app/api/notification/notification.helpers.ts

@@ -166,6 +166,14 @@ export const getEmailDetails = (
         }
       : undefined
 
+  const completedDetail = {
+    subject: 'Task marked as done',
+    header: 'A task has been completed',
+    title: 'View task',
+    body: `The task ‘${task?.title}’ has been marked as done by ${actionUser}.\n\nTo see details about the task, open it below.`,
+    ctaParams,
+  }
+
   return {
     [NotificationTaskActions.Assigned]: {
       subject: 'A task was assigned to you',
@@ -181,13 +189,10 @@ export const getEmailDetails = (
       title: 'View task',
       ctaParams,
     },
-    //! Currently disable all IU email notifications
-    // [NotificationTaskActions.Completed]: {
-    //   title: 'A client completed a task',
-    //   subject: 'A client completed a task',
-    //   header: 'A client completed a task',
-    //   body: `A new task was completed by ${actionUser}. You are receiving this notification because you have access to the client.`,
-    // },
+    [NotificationTaskActions.Completed]: completedDetail,
+    [NotificationTaskActions.CompletedByIU]: completedDetail,
+    [NotificationTaskActions.CompletedByCompanyMember]: completedDetail,
+    [NotificationTaskActions.CompletedForCompanyByIU]: completedDetail,
     [NotificationTaskActions.Commented]: {
       subject: 'Comment was added',
       header: 'Comment was added',

src/app/api/notification/notification.service.test.ts

@@ -292,9 +292,13 @@ describe('guard: CU wiring boundaries', () => {
     expect(svc.groupedEventTypeFor(NotificationTaskActions.Shared)).toBe(GroupedEmailEventType.SHARED)
     expect(svc.groupedEventTypeFor(NotificationTaskActions.SharedToCompany)).toBe(GroupedEmailEventType.SHARED)
     expect(svc.groupedEventTypeFor(NotificationTaskActions.Commented)).toBe(GroupedEmailEventType.COMMENT)
+    expect(svc.groupedEventTypeFor(NotificationTaskActions.Completed)).toBe(GroupedEmailEventType.COMPLETED)
+    expect(svc.groupedEventTypeFor(NotificationTaskActions.CompletedByIU)).toBe(GroupedEmailEventType.COMPLETED)
+    expect(svc.groupedEventTypeFor(NotificationTaskActions.CompletedByCompanyMember)).toBe(GroupedEmailEventType.COMPLETED)
+    expect(svc.groupedEventTypeFor(NotificationTaskActions.CompletedForCompanyByIU)).toBe(GroupedEmailEventType.COMPLETED)
   })
 
-  it('returns null for every action that must not be buffered', () => {
+  it('returns null for every action that must not be buffered (incl. shared-CU completion emails)', () => {
     const svc = buildService() as unknown as {
       groupedEventTypeFor: (a: NotificationTaskActions) => GroupedEmailEventType | null
     }
@@ -305,23 +309,17 @@ describe('guard: CU wiring boundaries', () => {
       NotificationTaskActions.Shared,
       NotificationTaskActions.SharedToCompany,
       NotificationTaskActions.Commented,
+      NotificationTaskActions.Completed,
+      NotificationTaskActions.CompletedByIU,
+      NotificationTaskActions.CompletedByCompanyMember,
+      NotificationTaskActions.CompletedForCompanyByIU,
     ]
     const unmapped = Object.values(NotificationTaskActions).filter((a) => !mapped.includes(a))
     for (const action of unmapped) {
       expect(svc.groupedEventTypeFor(action)).toBeNull()
     }
   })
 
-  it('never returns COMPLETED — that type is reserved for the deferred IU milestone', () => {
-    const svc = buildService() as unknown as {
-      groupedEventTypeFor: (a: NotificationTaskActions) => GroupedEmailEventType | null
-    }
-    const allActions = Object.values(NotificationTaskActions)
-    for (const action of allActions) {
-      expect(svc.groupedEventTypeFor(action)).not.toBe(GroupedEmailEventType.COMPLETED)
-    }
-  })
-
   it('never writes recipientIuId in a CU grouped event row', async () => {
     await buildService().create(NotificationTaskActions.Assigned, makeTask())
     const row = mockGroupedCreateMany.mock.calls[0][0].data[0]
@@ -372,3 +370,94 @@ describe('guard: IU wiring boundaries', () => {
     expect(mockEnqueueFlush).not.toHaveBeenCalled()
   })
 })
+
+describe('guard: IU completion emails', () => {
+  // Every completion action routes to an IU; create() must buffer them as IU rows regardless of
+  // which one is passed, so the guard stays consistent with groupedEventTypeFor.
+  it.each([
+    NotificationTaskActions.CompletedByIU,
+    NotificationTaskActions.CompletedForCompanyByIU,
+    NotificationTaskActions.Completed,
+    NotificationTaskActions.CompletedByCompanyMember,
+  ])('buffers a %s email as a COMPLETED IU event and keeps the in-product notification immediate', async (action) => {
+    await buildService().create(action, makeTask({ assigneeType: AssigneeType.internalUser, clientId: null }))
+
+    expect(mockGroupedCreateMany).toHaveBeenCalledTimes(1)
+    const row = mockGroupedCreateMany.mock.calls[0][0].data[0]
+    expect(row).toMatchObject({
+      recipientIuId: '33333333-3333-3333-3333-333333333333',
+      recipientClientId: null,
+      recipientCompanyId: null,
+      eventType: GroupedEmailEventType.COMPLETED,
+    })
+    expect(row.individualEmail.recipientInternalUserId).toBe('33333333-3333-3333-3333-333333333333')
+
+    expect(mockCreateNotification).toHaveBeenCalledTimes(1)
+    const sent = mockCreateNotification.mock.calls[0][0]
+    expect(sent.recipientInternalUserId).toBe('33333333-3333-3333-3333-333333333333')
+    expect(sent.recipientClientId).toBeUndefined()
+    expect(deliveryTargetsOf(0).inProduct).toBeDefined()
+    expect(deliveryTargetsOf(0).email).toBeUndefined()
+  })
+
+  it('does not buffer and still routes the in-product CompletedByIU notification to the IU when email is disabled', async () => {
+    const task = makeTask({ assigneeType: AssigneeType.internalUser, clientId: null })
+    await buildService().create(NotificationTaskActions.CompletedByIU, task, { disableEmail: true })
+
+    expect(mockGroupedCreateMany).not.toHaveBeenCalled()
+    const sent = mockCreateNotification.mock.calls[0][0]
+    expect(sent.recipientInternalUserId).toBe('33333333-3333-3333-3333-333333333333')
+    expect(sent.recipientClientId).toBeUndefined()
+    expect(deliveryTargetsOf(0).email).toBeUndefined()
+  })
+
+  it('is not blocked by the client-notification dedup guard on a client-assigned task', async () => {
+    // A client-assigned task still has an unread ClientNotification from its assignment when the
+    // IU completes it; the CompletedByIU recipient is the creator IU, so the guard must not fire.
+    mockFindFirst.mockResolvedValue({ id: 'existing-client-notif' })
+    const task = makeTask({ assigneeType: AssigneeType.client, clientId: '33333333-3333-3333-3333-333333333333' })
+
+    await buildService().create(NotificationTaskActions.CompletedByIU, task, { disableEmail: false })
+
+    // guard skipped: the completion email buffers and the IU notification dispatches
+    expect(mockFindFirst).not.toHaveBeenCalled()
+    expect(mockGroupedCreateMany).toHaveBeenCalledTimes(1)
+    expect(mockGroupedCreateMany.mock.calls[0][0].data[0].recipientIuId).toBe('33333333-3333-3333-3333-333333333333')
+    expect(mockCreateNotification).toHaveBeenCalledTimes(1)
+    expect(mockCreateNotification.mock.calls[0][0].recipientInternalUserId).toBe('33333333-3333-3333-3333-333333333333')
+  })
+
+  it('bulk Completed buffers one COMPLETED IU row per recipient and strips the email from dispatch', async () => {
+    await buildService().createBulkNotification(NotificationTaskActions.Completed, makeTask(), ['iu_a', 'iu_b'], {
+      email: true,
+    })
+
+    expect(mockGroupedCreateMany).toHaveBeenCalledTimes(2)
+    const rows = mockGroupedCreateMany.mock.calls.map((c) => c[0].data[0])
+    expect(rows.map((r) => r.recipientIuId)).toEqual(['iu_a', 'iu_b'])
+    for (const row of rows) {
+      expect(row.eventType).toBe(GroupedEmailEventType.COMPLETED)
+      expect(row.recipientClientId).toBeNull()
+      expect(row.individualEmail.recipientInternalUserId).toBeDefined()
+    }
+
+    expect(mockCreateNotification).toHaveBeenCalledTimes(2)
+    const sent = mockCreateNotification.mock.calls.map((c) => c[0])
+    expect(sent.map((s) => s.recipientInternalUserId)).toEqual(['iu_a', 'iu_b'])
+    for (const s of sent) {
+      expect(s.recipientClientId).toBeUndefined()
+      expect(s.deliveryTargets.email).toBeUndefined()
+    }
+  })
+
+  it('bulk CompletedByCompanyMember neither buffers nor emails when the flag is off (email opt falsy)', async () => {
+    await buildService().createBulkNotification(NotificationTaskActions.CompletedByCompanyMember, makeTask(), ['iu_a'], {
+      email: false,
+    })
+
+    expect(mockGroupedCreateMany).not.toHaveBeenCalled()
+    expect(mockCreateNotification).toHaveBeenCalledTimes(1)
+    expect(deliveryTargetsOf(0).email).toBeUndefined()
+    expect(mockCreateNotification.mock.calls[0][0].recipientInternalUserId).toBe('iu_a')
+  })
+})

src/app/api/notification/notification.service.ts

@@ -36,13 +36,22 @@ export class NotificationService extends BaseService {
     } = { disableEmail: false },
   ) {
     try {
-      // 1.Check for existing notification. Skip if duplicate
-      const existingNotification = task.clientId
-        ? await this.db.clientNotification.findFirst({
-            where: { taskId: task.id, clientId: task.clientId, companyId: task.companyId },
-          })
-        : null
-      if (task.clientId && existingNotification && !opts.commentId) {
+      const isAssignedToIu =
+        task.assigneeType === AssigneeType.internalUser &&
+        (action === NotificationTaskActions.Assigned || action === NotificationTaskActions.ReassignedToIU)
+      // Completion notifications always go to IUs (task creator, or IUs with access)
+      const isRecipientIu = isAssignedToIu || this.isCompletionAction(action)
+
+      // 1. Check for existing notification. Skip if duplicate. This dedup is keyed on the client
+      // assignee, so it must not gate IU-recipient notifications (e.g. CompletedByIU on a
+      // client-assigned task, whose recipient is the creator IU, not the client).
+      const existingNotification =
+        task.clientId && !isRecipientIu
+          ? await this.db.clientNotification.findFirst({
+              where: { taskId: task.id, clientId: task.clientId, companyId: task.companyId },
+            })
+          : null
+      if (existingNotification && !opts.commentId) {
         console.error(`NotificationService#create | Found existing notification for ${task.clientId}`, existingNotification)
         return
       }
@@ -55,10 +64,6 @@ export class NotificationService extends BaseService {
         action,
       )
 
-      const isAssignedToIu =
-        task.assigneeType === AssigneeType.internalUser &&
-        (action === NotificationTaskActions.Assigned || action === NotificationTaskActions.ReassignedToIU)
-
       const inProduct = opts.disableInProduct
         ? undefined
         : getInProductNotificationDetails(workspace, actionUser, task, { companyName, commentId: opts?.commentId })[action]
@@ -74,7 +79,7 @@ export class NotificationService extends BaseService {
           task,
           recipientId,
           companyId: task.companyId ?? association?.companyId ?? undefined,
-          isRecipientIu: isAssignedToIu,
+          isRecipientIu,
           eventType: groupedType,
           commentId: opts.commentId,
           individualEmail: this.buildNotificationDetails(
@@ -83,7 +88,7 @@ export class NotificationService extends BaseService {
             recipientId,
             { email },
             senderCompanyId,
-            isAssignedToIu,
+            isRecipientIu,
           ),
         })
       }
@@ -96,7 +101,7 @@ export class NotificationService extends BaseService {
         recipientId,
         { inProduct, email },
         senderCompanyId,
-        isAssignedToIu,
+        isRecipientIu,
       )
       if (groupedType) notificationDetails.deliveryTargets = { inProduct }
       if (!inProduct && !notificationDetails.deliveryTargets?.email) return
@@ -188,6 +193,12 @@ export class NotificationService extends BaseService {
       const association = AssociationsSchema.parse(task.associations)?.[0]
       // Non-null only when these CU emails should be diverted into the grouped buffer.
       const groupedType = email ? this.groupedEventTypeFor(action) : null
+      // Completion recipients are IUs; undefined (not false) elsewhere so paths like the
+      // Commented-to-IU job keep the absence-of-email inference in buildNotificationDetails
+      const isRecipientIu =
+        action === NotificationTaskActions.Completed || action === NotificationTaskActions.CompletedByCompanyMember
+          ? true
+          : undefined
 
       // NOTE: The reason we are skipping using NotificationService#create and implementing notification dispatch + save manually is because
       // we can just do one `createMany` DB call instead of one per notification, saving a ton of DB calls
@@ -208,9 +219,17 @@ export class NotificationService extends BaseService {
               task,
               recipientId,
               companyId: task.companyId ?? association?.companyId ?? undefined,
+              isRecipientIu,
               eventType: groupedType,
               commentId: opts?.commentId,
-              individualEmail: this.buildNotificationDetails(task, senderId, recipientId, { email }, opts?.senderCompanyId),
+              individualEmail: this.buildNotificationDetails(
+                task,
+                senderId,
+                recipientId,
+                { email },
+                opts?.senderCompanyId,
+                isRecipientIu,
+              ),
             })
             if (!inProduct) continue
           }
@@ -223,6 +242,7 @@ export class NotificationService extends BaseService {
             recipientId,
             { inProduct, email },
             opts?.senderCompanyId,
+            isRecipientIu,
           )
           if (groupedType) notificationDetails.deliveryTargets = { inProduct }
 
@@ -593,7 +613,17 @@ export class NotificationService extends BaseService {
     })
   }
 
+  private isCompletionAction(action: NotificationTaskActions): boolean {
+    return (
+      action === NotificationTaskActions.Completed ||
+      action === NotificationTaskActions.CompletedByIU ||
+      action === NotificationTaskActions.CompletedByCompanyMember ||
+      action === NotificationTaskActions.CompletedForCompanyByIU
+    )
+  }
+
   private groupedEventTypeFor(action: NotificationTaskActions): GroupedEmailEventType | null {
+    if (this.isCompletionAction(action)) return GroupedEmailEventType.COMPLETED
     switch (action) {
       case NotificationTaskActions.Assigned:
       case NotificationTaskActions.AssignedToCompany:

src/app/api/tasks/task-notifications.service.ts

@@ -289,12 +289,14 @@ export class TaskNotificationsService extends BaseService {
       // Don't do this in parallel since this can cause rate-limits, each of them has their own bottlenecks for avoiding ratelimits
       shouldCreateNotification &&
         (await this.notificationService.create(NotificationTaskActions.CompletedForCompanyByIU, updatedTask, {
-          disableEmail: true,
+          disableEmail: !isIuEmailEnabled(),
         }))
       await this.notificationService.markAsReadForAllRecipients(updatedTask)
     } else if (updatedTask.assigneeType === AssigneeType.client) {
       shouldCreateNotification &&
-        (await this.notificationService.create(NotificationTaskActions.CompletedByIU, updatedTask, { disableEmail: true }))
+        (await this.notificationService.create(NotificationTaskActions.CompletedByIU, updatedTask, {
+          disableEmail: !isIuEmailEnabled(),
+        }))
       try {
         await this.notificationService.markClientNotificationAsRead(updatedTask)
         return
@@ -303,7 +305,9 @@ export class TaskNotificationsService extends BaseService {
       }
     } else if (updatedTask.assigneeType === AssigneeType.internalUser) {
       shouldCreateNotification &&
-        (await this.notificationService.create(NotificationTaskActions.CompletedByIU, updatedTask, { disableEmail: true }))
+        (await this.notificationService.create(NotificationTaskActions.CompletedByIU, updatedTask, {
+          disableEmail: !isIuEmailEnabled(),
+        }))
     }
   }
 
@@ -368,7 +372,7 @@ export class TaskNotificationsService extends BaseService {
         NotificationTaskActions.CompletedByCompanyMember,
         updatedTask,
         recipientIds,
-        { senderCompanyId },
+        { senderCompanyId, email: isIuEmailEnabled() },
       )
       await this.notificationService.markAsReadForAllRecipients(updatedTask)
     } else {
@@ -379,6 +383,7 @@ export class TaskNotificationsService extends BaseService {
       )
       await this.notificationService.createBulkNotification(NotificationTaskActions.Completed, updatedTask, recipientIds, {
         senderCompanyId,
+        email: isIuEmailEnabled(),
       })
       await this.notificationService.markClientNotificationAsRead(updatedTask)
     }