feat(api): add workflow trigger identifier parity #6657

djabarovgeorge · 2024-10-09T07:51:39Z

What changed? Why was the change needed?

Add trigger identifier parity with the v2 Workflows API CRUD.

Why? (Context)

Currently, we're missing some functionality that was available in v1, the identifier was missing in the DTO meaning the frontend was not able to provide it in the client.

This ticket aims to address that gap.

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

linear · 2024-10-09T07:51:42Z

NV-4468 Create trigger identifier parity with v2 workflows

djabarovgeorge · 2024-10-09T08:01:07Z

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

The test "should not allow creating two workflows for the same user with the same name" will fail because we do allow workflows with the same name.
We need to clarify the terms: name is not unique and can be duplicated, while workflowIdentifier (based on the workflow name) cannot be duplicated.

Please note, that I’ve seen discussions across the product regarding this topic, but the current logic in the product is as follows:

On the first workflow creation, say "Password Reset," if the user hasn't provided a custom trigger identifier, we generate a slug from the workflow name.

On the second creation of a workflow with the same name ("Password Reset"), if the user hasn't provided a custom trigger identifier, we gracefully allow the same name but append a short ID to the slug.

When updating a workflow, we will throw an error if the user provides a trigger identifier that already exists in the system. This is because having two workflows with the same trigger identifier is invalid. We enforce this rule both in the application and the database layer, where MongoDB will throw an exception if we don’t validate it.

netlify · 2024-10-09T08:01:08Z

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name	Link
🔨 Latest commit	`48c0c00`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/670e923f6a52a60008ac6217
😎 Deploy Preview	https://deploy-preview-6657--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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

djabarovgeorge · 2024-10-09T08:02:38Z

packages/shared/src/dto/workflows/workflow-commons-fields.ts

@@ -64,6 +64,10 @@ export class WorkflowCommonsFields {
  @IsDefined()
  name: string;

+  @IsString()
+  @IsDefined()
+  triggerIdentifier: string;


IMO we can refactor the legacy term and make it simplifier with identifier what do you think?

djabarovgeorge · 2024-10-09T08:05:08Z

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

-  private async workflowExistByExternalId(upsertCommand: UpsertWorkflowCommand) {
-    const { environmentId } = upsertCommand.user;
-    const workflowByDbId = await this.notificationTemplateRepository.findByTriggerIdentifier(
-      environmentId,
-      upsertCommand.workflowDto.name
-    );
-
-    return !!workflowByDbId;
-  }


we don't need to validate because this logic is handled in the Create/Update workflow application-generic logic. so this call is duplicated.
(small note we enforce it on the database layer as well)

djabarovgeorge · 2024-10-09T08:06:41Z

libs/application-generic/src/usecases/create-workflow/create-workflow.usecase.ts

+    if (command.triggerIdentifier) {
+      return command.triggerIdentifier;
+    }


If provided, we will skip the internal generation. This is somewhat of a risk area, which is why I added a comment on the command parameter.

libs/application-generic/src/usecases/create-workflow/create-workflow.usecase.ts

…h-v2-workflows

pkg-pr-new · 2024-10-10T13:14:52Z

Open in Stackblitz

novu

pnpm add https://pkg.pr.new/novuhq/novu@6657

@novu/client

pnpm add https://pkg.pr.new/novuhq/novu/@novu/client@6657

@novu/framework

pnpm add https://pkg.pr.new/novuhq/novu/@novu/framework@6657

@novu/headless

pnpm add https://pkg.pr.new/novuhq/novu/@novu/headless@6657

@novu/js

pnpm add https://pkg.pr.new/novuhq/novu/@novu/js@6657

@novu/node

pnpm add https://pkg.pr.new/novuhq/novu/@novu/node@6657

@novu/notification-center

pnpm add https://pkg.pr.new/novuhq/novu/@novu/notification-center@6657

@novu/providers

pnpm add https://pkg.pr.new/novuhq/novu/@novu/providers@6657

@novu/react

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react@6657

@novu/react-native

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react-native@6657

@novu/shared

pnpm add https://pkg.pr.new/novuhq/novu/@novu/shared@6657

commit: 48c0c00

…h-v2-workflows

SokratisVidros · 2024-10-15T12:11:43Z

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

@@ -188,28 +189,13 @@ export class UpsertWorkflowUseCase {
      description: workflowDto.description || '',
      tags: workflowDto.tags || [],
      critical: false,
+      triggerIdentifier: `${slugify(workflowDto.name, {


This introduces a new term called triggerIdentifier. Isn't this the same as the workflowId? If yes, please rename accordingly.

Moreover we need to use the same help slugifyIdentifier helper

This introduces a new term called triggerIdentifier.

this is not true, under the hood CreateWorkflow usecase is responsible for creation of
workflow.triggers.identifier
where workflow is NotificationTemplateEntity)
and triggers is NotificationTriggerEntity[]

In CreateWorkflow usecase context, i think we should provide it with more explicit terms if possible, until/if we decide to refactor our workflow entity

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

libs/application-generic/src/usecases/create-workflow/create-workflow.usecase.ts

libs/application-generic/src/utils/slugify-identifier.ts

packages/shared/src/dto/workflows/workflow-commons-fields.ts

rifont · 2024-10-15T19:50:19Z

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

@@ -243,6 +245,7 @@ function buildCreateWorkflowDto(nameSuffix: string): CreateWorkflowDto {
  return {
    __source: WorkflowCreationSourceEnum.EDITOR,
    name: TEST_WORKFLOW_NAME + nameSuffix,
+    workflowId: `${slugifyName(TEST_WORKFLOW_NAME + nameSuffix)}`,


I think it's an antipattern to supply a slugified workflowId during workflow creation, that concern should be handled in the API. Am I missing something?

Good point i made it like this because of the following design where the client has validation.

feat(api): add workflow trigger identifier parity

a02a135

djabarovgeorge temporarily deployed to Linting October 9, 2024 07:51 — with GitHub Actions Inactive

github-actions bot added @novu/api @novu/shared labels Oct 9, 2024

djabarovgeorge commented Oct 9, 2024

View reviewed changes

apps/api/src/app/workflows-v2/workflow.controller.e2e.ts Outdated Show resolved Hide resolved

djabarovgeorge commented Oct 9, 2024

View reviewed changes

rifont reviewed Oct 9, 2024

View reviewed changes

libs/application-generic/src/usecases/create-workflow/create-workflow.usecase.ts Show resolved Hide resolved

djabarovgeorge requested a review from tatarco October 10, 2024 08:41

Merge branch 'next' into nv-4468-create-trigger-identifier-parity-wit…

b78ea4c

…h-v2-workflows

djabarovgeorge temporarily deployed to Linting October 10, 2024 12:43 — with GitHub Actions Inactive

refactor(api): extract slugify to reusable function

9d04dba

djabarovgeorge had a problem deploying to Linting October 10, 2024 13:09 — with GitHub Actions Failure

djabarovgeorge temporarily deployed to Linting October 10, 2024 13:09 — with GitHub Actions Inactive

refactor(api): rename triggerIdentifier to identifier

0903949

djabarovgeorge temporarily deployed to Linting October 10, 2024 13:24 — with GitHub Actions Inactive

djabarovgeorge had a problem deploying to Linting October 10, 2024 13:24 — with GitHub Actions Failure

djabarovgeorge temporarily deployed to Linting October 10, 2024 13:24 — with GitHub Actions Inactive

Merge branch 'next' into nv-4468-create-trigger-identifier-parity-wit…

4224f66

…h-v2-workflows

djabarovgeorge temporarily deployed to Linting October 13, 2024 08:32 — with GitHub Actions Inactive

djabarovgeorge had a problem deploying to Linting October 13, 2024 08:32 — with GitHub Actions Failure

djabarovgeorge temporarily deployed to Linting October 13, 2024 08:32 — with GitHub Actions Inactive

Merge branch 'next' into nv-4468-create-trigger-identifier-parity-wit…

c0da414

…h-v2-workflows

djabarovgeorge temporarily deployed to Linting October 14, 2024 09:11 — with GitHub Actions Inactive

djabarovgeorge had a problem deploying to Linting October 14, 2024 09:11 — with GitHub Actions Failure

Merge branch 'next' into nv-4468-create-trigger-identifier-parity-wit…

2419fdb

…h-v2-workflows

djabarovgeorge temporarily deployed to Linting October 14, 2024 12:51 — with GitHub Actions Inactive

djabarovgeorge had a problem deploying to Linting October 14, 2024 12:51 — with GitHub Actions Failure

djabarovgeorge temporarily deployed to Linting October 14, 2024 12:51 — with GitHub Actions Inactive

fix(api): failing tests and spell

67695bf

djabarovgeorge temporarily deployed to Linting October 14, 2024 13:19 — with GitHub Actions Inactive

SokratisVidros requested changes Oct 15, 2024

View reviewed changes

refactor(api): after pr comments

688aff0

djabarovgeorge temporarily deployed to Linting October 15, 2024 15:16 — with GitHub Actions Inactive

fix(api): test files

48c0c00

djabarovgeorge temporarily deployed to Linting October 15, 2024 16:03 — with GitHub Actions Inactive

SokratisVidros approved these changes Oct 15, 2024

View reviewed changes

djabarovgeorge merged commit 2e87ab7 into next Oct 15, 2024
41 checks passed

djabarovgeorge deleted the nv-4468-create-trigger-identifier-parity-with-v2-workflows branch October 15, 2024 17:37

rifont reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add workflow trigger identifier parity #6657

feat(api): add workflow trigger identifier parity #6657

djabarovgeorge commented Oct 9, 2024 •

edited

Loading

linear bot commented Oct 9, 2024

djabarovgeorge Oct 9, 2024

netlify bot commented Oct 9, 2024 •

edited

Loading

djabarovgeorge Oct 9, 2024

rifont Oct 9, 2024

djabarovgeorge Oct 9, 2024

djabarovgeorge Oct 9, 2024

pkg-pr-new bot commented Oct 10, 2024 •

edited

Loading

SokratisVidros Oct 15, 2024

SokratisVidros Oct 15, 2024

djabarovgeorge Oct 15, 2024

rifont Oct 15, 2024

djabarovgeorge Oct 16, 2024

feat(api): add workflow trigger identifier parity #6657

feat(api): add workflow trigger identifier parity #6657

Conversation

djabarovgeorge commented Oct 9, 2024 • edited Loading

What changed? Why was the change needed?

Why? (Context)

Screenshots

Related enterprise PR

Special notes for your reviewer

linear bot commented Oct 9, 2024

Choose a reason for hiding this comment

netlify bot commented Oct 9, 2024 • edited Loading

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

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

pkg-pr-new bot commented Oct 10, 2024 • edited Loading

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 commented Oct 9, 2024 •

edited

Loading

netlify bot commented Oct 9, 2024 •

edited

Loading

pkg-pr-new bot commented Oct 10, 2024 •

edited

Loading