Migrate project transition emails into Notifications by CarsonF · Pull Request #3660 · SeedCompany/cord-api-v3

CarsonF · 2026-02-20T22:38:14Z

Goal: To move project workflow transition notifications into our user notification system.
This will allow people to get in-app notifications & also for them to opt-out of emails if they want.

Prompt:

Refactor/reimplement this ProjectWorkflowNotifcation feature. I want to build it out on top of our new user notifications. Look at CommentViaMentionNotification & its associated strategy class for that. It looks like we have 3 groups of notifiers: project team members, financial approvers, and email distros (group email addresses, not users). Those email distros have to stay implemented here as they are: they do not fit into the new user notification functionality. But for the other two cases I think we could have a ProjectTransitionViaMembershipNotification and a ProjectTransitionRequiringFinancialApprovalNotification. Be sure you understand the notification system first. Then build a out those two new notification concrete modules. Then refactor this existing handler & workflow to call the notification system.

CarsonF · 2026-02-21T21:42:47Z

Things to consider:

The financial approver email template should be unique to include verbiage about the user needing to take action. The UI should include the same verbiage and could have custom UX to push into that.
Financial approvers could be project members as well. If so, they probably shouldn't get the membership notification, that duplicate would be annoying. I'm not sure how this could function.
Permission to view the WorkflowEvent is required to show the notification in the UI. Consider checking if the recipient has that permission before assigning OR ensure that all members and financial approvers have permission to view workflow events.
All of the work in the pre-existing handler could be moved to a new queue. That would speed up the transition mutation since all of the notification work can be async (in background)

NestJS was orphaning these types since there were no references to them or the ProjectTransitionNotification interface. Declaring that they implement the Notification interface un-orphans them, since there are queries/mutations/other-refs for that interface.

This time it is just the default preferences, users can override.

Copilot

Pull request overview

This PR migrates project workflow transition notifications from direct email delivery to the new user notification system, enabling in-app notifications and user opt-out preferences. The implementation creates two notification types: one for team members and one for financial approvers, while preserving direct email delivery for distribution lists that don't map to individual users.

Changes:

Added database schema and DTOs for ProjectTransitionViaMembership and ProjectTransitionRequiringFinancialApproval notification types
Implemented notification strategies with shared base class for common functionality (email rendering, database persistence)
Refactored ProjectWorkflowNotificationHandler to route team members and financial approvers through the notification service while maintaining direct emails for distribution lists
Added resolver for loading full Project object on ProjectWorkflowEvent and resolver for hydrating workflowEvent link on transition notifications
Added configuration option NOTIFY_PROJECT_STEP_CHANGES to control default email notification preferences

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
dbschema/notifications.gel	Defines abstract `ProjectTransition` notification type with concrete implementations for membership and financial approval scenarios
dbschema/migrations/00031-m1ofy7r.edgeql	Database migration creating the new notification types
src/components/project/workflow/notifications/*.dto.ts	Notification DTO definitions with resource registration and GraphQL type declarations
src/components/project/workflow/notifications/*.strategy.tsx	Notification strategy implementations for database persistence and email rendering
src/components/project/workflow/notifications/project-transition-notification-links.resolver.ts	GraphQL resolver for hydrating workflowEvent links on notifications
src/components/project/workflow/handlers/project-workflow-notification.handler.tsx	Refactored handler routing different notifier types to appropriate delivery mechanisms
src/components/project/workflow/emails/project-step-changed-notification.email.tsx	New async email component that loads required data for rendering within recipient context
src/components/project/workflow/resolvers/project-workflow-event.resolver.ts	Added project field resolver to load full Project object
src/components/project/workflow/project-workflow.module.ts	Module configuration adding notification module import and new providers
src/components/notifications/notification.module.ts	Added forwardRef for UserModule import to prevent circular dependency
src/core/config/config.service.ts	Added documentation for `notifyProjectStepChanges` configuration option

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-24T23:12:30Z

src/components/project/workflow/handlers/project-workflow-notification.handler.tsx

@@ -44,28 +49,87 @@ export class ProjectWorkflowNotificationHandler {
      previousStep,
      moduleRef: this.moduleRef,
    };
-    const notifyees = (
-      await Promise.all(notifiers.map((notifier) => notifier.resolve(params)))
-    )
-      .flat()
-      .filter(
-        (n) =>
-          // Not current user
-          n.id !== session.userId &&
-          // Only email notifications right now
-          n.email,
-      );

-    if (notifyees.length === 0) {
-      return;
-    }
+    const notificationInput = {
+      workflowEvent: workflowEvent.id,
+      previousStep,
+    };
+
+    // Resolve each notifier separately so we can categorize them
+    await Promise.all(
+      notifiers.map(async (notifier) => {
+        const resolved = many(await notifier.resolve(params));

-    this.logger.info('Notifying', {
-      emails: notifyees.flatMap((r) => r.email ?? []),
-      projectId: event.project.id,
-      previousStep: event.previousStep,
-      toStep: event.workflowEvent.to,
-    });
+        if (notifier === TeamMembers) {
+          const userIds = resolved
+            .filter((n): n is Notifier & { id: ID<'User'> } => !!n.id)
+            .map((n) => n.id)
+            .filter((id) => id !== currentUserId);
+          if (userIds.length > 0) {
+            this.logger.info('Notifying team members via notification system', {
+              userIds,
+              projectId: event.project.id,
+              previousStep,
+              toStep: workflowEvent.to,
+            });
+            await this.notifier.create(
+              ProjectTransitionViaMembershipNotification,
+              userIds,
+              notificationInput,
+            );
+          }
+          return;
+        }
+
+        if (notifier === FinancialApprovers) {
+          const userIds = resolved
+            .filter((n): n is Notifier & { id: ID<'User'> } => !!n.id)
+            .map((n) => n.id)
+            .filter((id) => id !== currentUserId);
+          if (userIds.length > 0) {
+            this.logger.info(
+              'Notifying financial approvers via notification system',
+              {
+                userIds,
+                projectId: event.project.id,
+                previousStep,
+                toStep: workflowEvent.to,
+              },
+            );
+            await this.notifier.create(
+              ProjectTransitionRequiringFinancialApprovalNotification,
+              userIds,
+              notificationInput,
+            );
+          }
+          return;
+        }
+
+        // Email distros and other custom notifiers — send emails directly
+        const emailNotifyees = resolved.filter(
+          (n) => n.email && n.id !== currentUserId,
+        );
+        if (emailNotifyees.length === 0) {
+          return;
+        }
+
+        this.logger.info('Notifying via direct email', {
+          emails: emailNotifyees.map((n) => n.email),
+          projectId: event.project.id,
+          previousStep,
+          toStep: workflowEvent.to,
+        });
+
+        await this.sendDirectEmails(emailNotifyees, event);
+      }),
+    );
+  }


This PR introduces a significant refactoring of project workflow notifications without adding test coverage. The new notification system routes team members and financial approvers through the notification service, while distribution lists use direct emails. Tests should be added to verify:

Team members receive notifications when a project they're on transitions

Financial approvers receive notifications for transitions requiring approval

Distribution lists still receive direct emails

Current user is correctly excluded from all notification types

Email preferences are respected (opt-out functionality)

Consider adding tests in the existing test/project-workflow.e2e-spec.ts or creating a new test file for notification-specific scenarios.

Copilot · 2026-02-24T23:12:30Z

src/components/project/workflow/notifications/project-transition-notification.strategy.tsx

+        ])
+        .return(
+          exp({
+            workflowEvent: 'workflowEvent { .id }',


The hydrateExtraForNeo4j method only hydrates the workflowEvent field but doesn't hydrate previousStep. While previousStep is a scalar property that gets automatically hydrated by the base implementation, it would be more consistent to include it explicitly in the return statement for clarity, or add a comment explaining why it's omitted. This would help future developers understand the complete shape of the hydrated object.

Suggested change

workflowEvent: 'workflowEvent { .id }',

workflowEvent: 'workflowEvent { .id }',

previousStep: 'node.previousStep',

Yeah having that symmetry could be a better design. But there's no bug here, the previousStep is automatically hydrated with the other default/base notification properties.

Copilot · 2026-02-24T23:12:30Z

src/components/project/workflow/notifications/project-transition-notification.strategy.tsx

+  insertForGel(input: ProjectTransitionInput) {
+    return e.insert(
+      EnhancedResource.resolve(this.dtoClass)
+        .db as unknown as typeof e.Notification.ProjectTransition,
+      {
+        workflowEvent: e.cast(
+          e.Project.WorkflowEvent,
+          e.uuid(input.workflowEvent),
+        ),
+        previousStep: e.cast(e.Project.Step, input.previousStep),
+      },
+    );
+  }


The type cast to typeof e.Notification.ProjectTransition is fragile and may cause type safety issues. The insertForGel method is supposed to return an expression for the specific concrete type (e.g., ProjectTransitionViaMembership), but this casts it to the abstract parent type. This could lead to runtime issues where the wrong database type is inserted.

Consider using the concrete type directly, similar to how saveForNeo4j uses this.dtoClass. You could retrieve the specific database type for each concrete class instead of using the abstract parent type.

Both of the concretes have exactly the same shape, without any extra properties so this cast is safe.

Copilot · 2026-02-24T23:12:31Z

src/components/project/workflow/emails/project-step-changed-notification.email.tsx

+  const [changedBy, project, primaryPartnerName, recipient] = await Promise.all(
+    [
+      users.readOne(workflowEvent.who.value!.id),
+      projects.readOne(projectId),
+      projects.getPrimaryOrganizationName(projectId),
+      users.readOne(identity.current.userId),
+    ],
+  );
+


Using a non-null assertion on workflowEvent.who.value!.id is potentially unsafe. If who.value is null, this will cause a runtime error during email rendering. Consider adding a null check or providing a fallback value to prevent email delivery failures.

Suggested change

const [changedBy, project, primaryPartnerName, recipient] = await Promise.all(

[

users.readOne(workflowEvent.who.value!.id),

projects.readOne(projectId),

projects.getPrimaryOrganizationName(projectId),

users.readOne(identity.current.userId),

],

);

const [project, primaryPartnerName, recipient] = await Promise.all([

projects.readOne(projectId),

projects.getPrimaryOrganizationName(projectId),

users.readOne(identity.current.userId),

]);

// Fall back to recipient if workflowEvent.who is not set to avoid render failures.

const changedBy =

workflowEvent.who?.value != null

? await users.readOne(workflowEvent.who.value.id)

: recipient;

Copilot · 2026-02-24T23:12:31Z

src/components/project/workflow/handlers/project-workflow-notification.handler.tsx

+
+        // Email distros and other custom notifiers — send emails directly
+        const emailNotifyees = resolved.filter(
+          (n) => n.email && n.id !== currentUserId,


The filter logic n.id !== currentUserId will incorrectly exclude email distribution lists. Distribution list notifiers have email but no id (or a null/undefined id). This condition will evaluate to true for all distribution lists (since undefined !== currentUserId), so they won't actually be excluded. However, the logic is unclear and could break if the Notifier type changes. Consider explicitly checking for the absence of an id: !n.id || n.id !== currentUserId to make the intent clear.

Suggested change

(n) => n.email && n.id !== currentUserId,

(n) => n.email && (!n.id || n.id !== currentUserId),

Copilot · 2026-02-24T23:12:31Z

src/components/project/workflow/notifications/project-transition-notification.strategy.tsx

+
+@Injectable()
+export abstract class ProjectTransitionNotificationStrategy<
+  T extends ProjectTransitionViaMembershipNotification,


The generic constraint T extends ProjectTransitionViaMembershipNotification is too restrictive. This base class is used by both ProjectTransitionViaMembershipNotificationStrategy and ProjectTransitionRequiringFinancialApprovalNotificationStrategy, but the latter doesn't extend ProjectTransitionViaMembershipNotification. The constraint should be T extends ProjectTransitionNotification instead to properly accommodate both concrete notification types.

Suggested change

T extends ProjectTransitionViaMembershipNotification,

T extends ProjectTransitionNotification,

CarsonF marked this pull request as ready for review February 21, 2026 16:20

CarsonF force-pushed the notifications/queue branch from e5f7188 to a890a15 Compare February 22, 2026 16:55

CarsonF added 8 commits February 22, 2026 10:56

AI to refactor project transition notifications

146823f

Adjust project transition notification schema

c9f0773

AI adapts to schema change

d51972b

DRY & clean up

eb95c78

Fix saving for transition notification for neo4j

c9c3b98

Expose WorkflowEvent.project

3c50d4e

Re-introduce config/env flag for project transition email notifications

04fd4c2

This time it is just the default preferences, users can override.

CarsonF force-pushed the notifications/project-workflow branch from 6ad557a to 04fd4c2 Compare February 22, 2026 16:58

CarsonF changed the title ~~AI to refactor project transition notifications~~ Migrate project transition emails into Notifications Feb 22, 2026

CarsonF mentioned this pull request Feb 22, 2026

Add new project transition notifications SeedCompany/cord-field#1773

Open

sethmcknight requested a review from Copilot February 24, 2026 23:06

Copilot started reviewing on behalf of sethmcknight February 24, 2026 23:06 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate project transition emails into Notifications #3660

Migrate project transition emails into Notifications #3660
CarsonF wants to merge 8 commits intonotifications/queuefrom
notifications/project-workflow

CarsonF commented Feb 20, 2026

Uh oh!

CarsonF commented Feb 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

CarsonF Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

CarsonF Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Copilot AI Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	workflowEvent: 'workflowEvent { .id }',
	workflowEvent: 'workflowEvent { .id }',
	previousStep: 'node.previousStep',

	(n) => n.email && n.id !== currentUserId,
	(n) => n.email && (!n.id \|\| n.id !== currentUserId),

	T extends ProjectTransitionViaMembershipNotification,
	T extends ProjectTransitionNotification,

Conversation

CarsonF commented Feb 20, 2026

Uh oh!

CarsonF commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

CarsonF Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

CarsonF Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CarsonF commented Feb 21, 2026 •

edited

Loading