Skip to content

fix(api-service): move workflow dto from shared to api #8009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

tatarco
Copy link
Contributor

@tatarco tatarco commented Mar 26, 2025

Resolve Workflow DTO Issues

What changed? Why was the change needed?

DTO Migration and Enhancements

  • All DTOs were moved from shared into API, converted to classes, and assigned the correct OpenAPI decorators.
  • All of the workflow V2 DTOs are now generated under the @novu-api. All of our E2E tests are now using the SDK and dogfooding the OpenAPI structure.
  • Error DTOs have been enhanced due to issues found in implementation.
  • Class validators are now working on DTOs, enforcing bad data from getting into the service.
  • The filter has been updated to parse those errors elegantly as well.
  • Decoupled the interfaces created in shared from entities; they should not be used anymore. We generate DTOs now, and the client should not care about our database entity; all is clearly presented to him.
  • JSON schema was a complete mess; aligned it all the way to the DAL layer. Some as unknown had to be added in the framework to minimize scope.
  • The preference controller has been deprecated along with all its DTOs since naming was a big confusion and duplication. Deprecated them all in order to know what is new and what is old.
  • Separated DTOs into separate properly named files.

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Mar 26, 2025

Copy link

pkg-pr-new bot commented Mar 26, 2025

Open in StackBlitz

npm i https://pkg.pr.new/novuhq/novu@8009
npm i https://pkg.pr.new/novuhq/novu/@novu/providers@8009
npm i https://pkg.pr.new/novuhq/novu/@novu/shared@8009

commit: 0b93f24

Copy link

netlify bot commented Mar 31, 2025

👷 Deploy Preview for dashboard-v2-novu-staging processing.

Name Link
🔨 Latest commit 0b93f24
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/68007a7a0591fb0008d1787d

@tatarco tatarco force-pushed the nv-5295-align-workflow-controller-to-sdk branch from 1b85ee4 to 345883d Compare March 31, 2025 10:31
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an initial high level review. We are in the right direction and I am noting down the next actions in order to wrap this effort up:

  1. We need to do another pass around the Preference and Workflow V2 DTOs. There still a lot of duplication between these DTOs for request and responses.
  2. We need to solve the dual export issue for the internal SDK generated by speakeasy.

@@ -54,7 +54,7 @@ export class ConstructFrameworkWorkflow {
dbWorkflow.triggers[0].identifier,
async ({ step, payload, subscriber }) => {
const fullPayloadForRender: FullPayloadForRender = { payload, subscriber, steps: {} };
for await (const staticStep of dbWorkflow.steps) {
for (const staticStep of dbWorkflow.steps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tatarco, why did we remove the await here? The dynamic workflow generation should be an async iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't make any sense, it's an object not a promise, why async iterator?

@@ -82,3 +118,32 @@ export async function expectSdkValidationExceptionGeneric<U>(
return { error: handleValidationErrorDto(e) };
}
}
export class CustomHeaderHTTPClient extends HTTPClient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to extend the Speakeasy client at this point? Doesn't it allow us to override its headers on the spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it allows us to do it only at the calls signature, I want to have it injected in all calls done with the instance, hence the requirement for this change.

@@ -0,0 +1,22 @@
import { ApiProperty } from '@nestjs/swagger';

export class SubscriberPreferenceTemplateResponseDto {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template is the internal modelling of workflows. Some preference DTOs use the term workflow, others the term template. We should stick to workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't want to make any additional changes besides whats a must in this huge PR, the purpose here is to document the existing state, lets move this to a seperate task

ChmaraX and others added 5 commits April 16, 2025 16:22
fixed wierd msg test

fix test

rebase fixes

fixed wierd msg test

remove bail

fix excpetion mapper

topics fixed.

topics fixed.

Revert "test skips"

This reverts commit f9d2c46.

test skips

fix failing test on usage reports

fix failing test on usage reports

fix failing test on usage reports

Refactor WorkflowController to use internal SDK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants