chore(api): Refactor workflows v2 #8069

SokratisVidros · 2025-04-04T23:48:24Z

What changed? Why was the change needed?

This PR started as an attempt to improve the UX when a step was created. The goal was to create a step, open the step template form, and not show issues immediately until the first edit.

Then, one thing led to another, and a major cleanup was conducted.

Please refer to the commit messages.

netlify · 2025-04-04T23:48:45Z

✅ Deploy Preview for dashboard-v2-novu-staging canceled.

Name	Link
🔨 Latest commit	`c3661e7`
🔍 Latest deploy log	https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/67f39b396933be0008125e1d

SokratisVidros · 2025-04-04T23:48:51Z

.github/workflows/reusable-api-e2e.yml

@@ -52,7 +52,25 @@ jobs:
        shell: bash
        env:
          NOVU_ENTERPRISE: ${{ inputs.ee }}
-        run: cd apps/worker && pnpm start:test 2>&1 &
+        run: |


An attempt to make E2E more robust when the worker hangs

feels a bit hacky i wonder if there is a more robust way to fix it within the worker service.

I would maybe put it to a bash script than having it in yaml directly, so the workflow is less overwhelming.

pnpm start:worker:test 2>&1 & ./monitor-worker.sh

SokratisVidros · 2025-04-04T23:49:28Z

apps/api/src/app/workflows-v2/e2e/workflow-test-data.e2e.ts

      })),
    };

    const { body } = await session.testAgent.post('/v2/workflows').send(createWorkflowDto);
-    const workflow = body.data;
-
-    for (const [index, step] of steps.entries()) {


This endpoint doesn't exist anymore. So the data generator of the E2E had to be updated

SokratisVidros · 2025-04-04T23:49:54Z

apps/api/src/app/workflows-v2/generate-preview.e2e.ts

@@ -1,8 +1,9 @@
+// TODO: Move this file under e2e folder and merge it with the one that has the same name


I didn't do it as part of this PR to avoid losing the diff.

SokratisVidros · 2025-04-04T23:50:28Z

apps/api/src/app/workflows-v2/usecases/build-step-issues/build-step-issues.command.ts

+
+  @IsBoolean()
+  @IsOptional()
+  skipControlValueIssues?: boolean;


Control if control value issues should be enforced or not.

SokratisVidros · 2025-04-04T23:51:02Z

apps/api/src/app/workflows-v2/generate-preview.e2e.ts


-  async function patchStepWithControlValues(workflowSlug: string, stepSlug: string, controlValues: InAppControlType) {


Ditto, there is no patch step endpoint anymore.

SokratisVidros · 2025-04-04T23:52:56Z

apps/api/src/app/workflows-v2/workflow.controller.ts

@@ -208,24 +223,8 @@ export class WorkflowController {
    );
  }

-  @Patch('/:workflowId/steps/:stepId')


👋 The PATCH step endpoint wasn't used in the Dashboard anymore. Most importantly, it's a problematic endpoint that couldn't survive our modelling.

Steps depend on each other through variables. So, PATCHing a single step without considering the other steps shouldn't be allowed. Imagine patching a digest step that affects all the other steps that follow.

SokratisVidros · 2025-04-04T23:53:26Z

apps/api/src/app/workflows-v2/workflow.controller.ts

    private patchWorkflowUsecase: PatchWorkflowUsecase,
    private duplicateWorkflowUseCase: DuplicateWorkflowUseCase
  ) {}

  @Post('')
+  @ExternalApiAccessible()


Expose the basic CRUD endpoints for API access. We won't document them yet.

im not against this, but as far as i remember, we agreed that this route would be internal.
when needed, we would introduce a lean route specifically for api usage.

SokratisVidros · 2025-04-04T23:53:59Z

apps/dashboard/src/components/side-navigation/sidebar.tsx

@@ -32,5 +32,5 @@ type SidebarFooterProps = HTMLAttributes<HTMLDivElement>;

 export const SidebarFooter = (props: SidebarFooterProps) => {
  const { className, ...rest } = props;
-  return <div className={cn('mt-auto space-y-2.5 px-2 py-3.5', className)} {...rest} />;
+  return <div className={cn('border-t-border-weak mt-auto space-y-2.5 border-t p-2', className)} {...rest} />;


Minor UI fix for the Step editor footer to ensure the border has 100% width.

SokratisVidros · 2025-04-04T23:54:24Z

apps/dashboard/src/pages/workflows.tsx

@@ -202,7 +202,7 @@ export const WorkflowsPage = () => {
                      }}
                    >
                      <RiFileMarkedLine />
-                      View Workflow Gallery


Copywriting fix: It's the Workflow Template store, we don't use the term Gallery.

SokratisVidros · 2025-04-04T23:54:43Z

apps/dashboard/src/routes/protected-route.tsx

@@ -1,5 +1,4 @@
 import { RedirectToSignIn, SignedIn, SignedOut } from '@clerk/clerk-react';
-import { ROUTES } from '@/utils/routes';


Oops, I forge this in next

SokratisVidros · 2025-04-04T23:55:23Z

libs/application-generic/src/utils/sanitize-control-values.ts

@@ -21,6 +21,7 @@ function sanitizeEmptyInput<T_Type>(input: T_Type, defaultValue: T_Type = undefi
 }

 export function sanitizeRedirect(redirect: InAppRedirectType | undefined) {
+  // TODO: There is a bug here, if the redirect doesn't contain both a url and a target it is removed from the new controlValues


We must fix this so as not to have issues if the endpoints are used by the API and not the Dashboard that always sends a full object for redirectURls and actions.

can you please elaborate on the exact problem we’re facing here?
and yes, this sanitization was explicitly designed to sanitize only dashboard values; it’s even in the name: dashboardSanitizeControlValues.
this was intentional because we decided that the v2 api would be client-first.

SokratisVidros · 2025-04-04T23:55:50Z

packages/shared/src/dto/workflows/step.dto.ts

@@ -104,7 +100,7 @@ export class UiSchema {
  properties?: Record<string, UiSchemaProperty>;
 }

-export class ControlsMetadata {
+export class Controls {


Just a shorter type name.

SokratisVidros · 2025-04-04T23:57:45Z

apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts

-    commandWorkflowSteps: Array<StepCreateDto | StepUpdateDto>,
-    persistedWorkflow?: NotificationTemplateEntity | undefined
+  private async buildSteps(
+    command: UpsertWorkflowCommand,


Notice that too avoid multiple arguments for each private function and ad-hoc types, the command object is passe to all of them so that each function can pick whatever it needs from it.

SokratisVidros · 2025-04-04T23:58:53Z

apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts

    private notificationGroupRepository: NotificationGroupRepository,
    private getWorkflowByIdsUseCase: GetWorkflowByIdsUseCase,
    private getWorkflowUseCase: GetWorkflowUseCase,
    private buildStepIssuesUsecase: BuildStepIssuesUsecase,
    private controlValuesRepository: ControlValuesRepository,
    private upsertControlValuesUseCase: UpsertControlValuesUseCase,
-    private analyticsService: AnalyticsService,
-    private notificationTemplateRepository: NotificationTemplateRepository
+    private analyticsService: AnalyticsService
  ) {}

  @InstrumentUsecase()
  async execute(command: UpsertWorkflowCommand): Promise<WorkflowResponseDto> {


The primary execute function's goal is to contain all the logic in one go without too much indirection. Moreover, the use case is symmetrical, one path for create, another path for update with identical function names.

Oh boy, this started as a small UI improvement but then I just couldn't stop. So here is the list of the fixes: - Expose the CRUD endpoints for API Key access - Remove the PATCH step endpoint as it can be used autonomously since workflow steps depend on each other. For example, you can't patch a digest step as it will affect all the following steps that use its outputs. - Simplify drastically the upsert workflow usecase to make it more robust and readable. - Simplify drastically the workflow E2E tests - Ensure that step content issues are not computed immediately after a step is created when the request comes from the Dashboard. This results in better UX upon step creation, as the step form opens without step issues initially.

Worker often hangs in CI during E2E execution. This is an attempt to restart it when that happens.

Nest.js keeps us hostages with Next.js until we upgrade to a Node version that allows require (ESM). So we had to use a much-ordered version of the get-port, which is CJS and not ESM.

…d yet

…ersisted yet

djabarovgeorge · 2025-04-07T08:52:04Z

apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts

-      })
-    );
-  }
+    // TODO: use transaction to ensure that the workflows, steps and controls are upserted atomically


dont we have the Idempotency header key for this?

Let's take this offline.

djabarovgeorge · 2025-04-07T08:52:57Z

apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts

@@ -2,26 +2,24 @@ import { BadRequestException, Injectable } from '@nestjs/common';

 import {
  AnalyticsService,
-  CreateWorkflow as CreateWorkflowGeneric,
+  CreateWorkflow as CreateWorkflowV0Usecase,


djabarovgeorge · 2025-04-07T09:17:09Z

apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts

    existingWorkflow: NotificationTemplateEntity
  ): Promise<UpdateWorkflowCommand> {
-    const steps = await this.mapSteps(workflowDto.origin, user, workflowDto.steps, existingWorkflow);
+    const { workflowDto, user } = command;
+    const steps = await this.buildSteps(command, existingWorkflow);


~~sorry i dont get it.~~
~~wont we create issues in here for the newly created workflow? (in this.buildSteps =>~~ ~~this.buildStepIssuesUsecase.execute)~~

never mind i just saw the Dashboard implementation

djabarovgeorge · 2025-04-07T09:24:12Z

apps/dashboard/src/components/workflow-editor/nodes.tsx

-                    })
+                    }),
+                    {
+                      state: { hideValidationErrorsOnFirstRender: true },


ohhhhh i see, so we create issues on create but not render them.
i thought the ticket was about not creating the issues on create.

Correct! Issues must be there in the modelling but this is mostly a UX fix.

djabarovgeorge

Looks awesome on my end, left a couple of comments with my concerns.

ChmaraX · 2025-04-07T09:38:18Z

.github/workflows/reusable-api-e2e.yml

@@ -52,7 +52,25 @@ jobs:
        shell: bash
        env:
          NOVU_ENTERPRISE: ${{ inputs.ee }}
-        run: cd apps/worker && pnpm start:test 2>&1 &
+        run: |


I would maybe put it to a bash script than having it in yaml directly, so the workflow is less overwhelming.

pnpm start:worker:test 2>&1 & ./monitor-worker.sh

ChmaraX · 2025-04-07T09:43:39Z

apps/api/src/app/workflows-v2/workflow.controller.e2e.ts

Not related to this PR but as a sidenote, its really tough to work with this test suite cause it has like 800 lines and many functions do the same but due to scale its probably hard to notice that something already exists.

I suggest we should do unit test in each usecase folder and then for actual E2E we should just briefly test if the endpoint is reachable and returns something, just a happy path scenario. Because we always use 1 entry usecase per 1 controller anyway.

I agree 100%. It needs to be split in a file per controller endpoint.

SokratisVidros requested review from ChmaraX, tatarco and djabarovgeorge April 4, 2025 23:48

SokratisVidros self-assigned this Apr 4, 2025

SokratisVidros temporarily deployed to Linting April 4, 2025 23:48 — with GitHub Actions Inactive

github-actions bot added @novu/shared CI/CD @novu/dashboard @novu/api-service labels Apr 4, 2025

SokratisVidros commented Apr 4, 2025

View reviewed changes

SokratisVidros changed the title ~~Refactor workflows v2~~ chore(api): Refactor workflows v2 Apr 4, 2025

SokratisVidros commented Apr 4, 2025

View reviewed changes

SokratisVidros temporarily deployed to Linting April 5, 2025 00:04 — with GitHub Actions Inactive

SokratisVidros added 10 commits April 7, 2025 11:06

Update sanitize-control-values.ts

7c501e3

Remove unused usecase for patch step

33cacd1

Update step.dto.ts

e9c5ce0

Update workflows.tsx

939f6f2

Update workflows-client.ts

0b3dfa2

fix(root): Attempt to make API E2E more robust

17f9777

Worker often hangs in CI during E2E execution. This is an attempt to restart it when that happens.

fix(api): Use a CJS get-port

cbc4f1f

Nest.js keeps us hostages with Next.js until we upgrade to a Node version that allows require (ESM). So we had to use a much-ordered version of the get-port, which is CJS and not ESM.

fixup! fix(api): Do not build issues for a step that has not persiste…

a2d67b1

…d yet

fixup! fixup! fix(api): Do not build issues for a step that has not p…

ba6d9b2

…ersisted yet

SokratisVidros force-pushed the refactor_workflows_v2 branch from 69e2a7c to ba6d9b2 Compare April 7, 2025 08:07

SokratisVidros temporarily deployed to Linting April 7, 2025 08:07 — with GitHub Actions Inactive

Update bridge-trigger.e2e.ts

c0443d6

SokratisVidros temporarily deployed to Linting April 7, 2025 08:39 — with GitHub Actions Inactive

djabarovgeorge reviewed Apr 7, 2025

View reviewed changes

fix(api-service): e2e tests

c3661e7

ChmaraX temporarily deployed to Linting April 7, 2025 09:30 — with GitHub Actions Inactive

djabarovgeorge reviewed Apr 7, 2025

View reviewed changes

ChmaraX approved these changes Apr 7, 2025

View reviewed changes

SokratisVidros merged commit 526d8f1 into next Apr 7, 2025
32 of 33 checks passed

SokratisVidros deleted the refactor_workflows_v2 branch April 7, 2025 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(api): Refactor workflows v2 #8069

chore(api): Refactor workflows v2 #8069

SokratisVidros commented Apr 4, 2025

netlify bot commented Apr 4, 2025 •

edited

Loading

SokratisVidros Apr 4, 2025

djabarovgeorge Apr 6, 2025

ChmaraX Apr 7, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

djabarovgeorge Apr 6, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

djabarovgeorge Apr 6, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

SokratisVidros Apr 4, 2025

djabarovgeorge Apr 7, 2025

djabarovgeorge Apr 7, 2025

SokratisVidros Apr 7, 2025

djabarovgeorge Apr 7, 2025

djabarovgeorge Apr 7, 2025 •

edited

Loading

djabarovgeorge Apr 7, 2025

SokratisVidros Apr 7, 2025

djabarovgeorge left a comment

ChmaraX Apr 7, 2025

ChmaraX Apr 7, 2025

SokratisVidros Apr 7, 2025

		@@ -1,8 +1,9 @@
		// TODO: Move this file under e2e folder and merge it with the one that has the same name


		async function patchStepWithControlValues(workflowSlug: string, stepSlug: string, controlValues: InAppControlType) {

		@@ -1,5 +1,4 @@
		import { RedirectToSignIn, SignedIn, SignedOut } from '@clerk/clerk-react';
		import { ROUTES } from '@/utils/routes';

chore(api): Refactor workflows v2 #8069

chore(api): Refactor workflows v2 #8069

Conversation

SokratisVidros commented Apr 4, 2025

What changed? Why was the change needed?

netlify bot commented Apr 4, 2025 • edited Loading

✅ Deploy Preview for dashboard-v2-novu-staging canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Apr 4, 2025 •

edited

Loading

djabarovgeorge Apr 7, 2025 •

edited

Loading