Skip to content
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.

Sign up for GitHub

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

feat(api): add full step data to workflow dto; refactor #7235

Merged
merged 4 commits into from
Dec 10, 2024
+285 −302
Merged

feat(api): add full step data to workflow dto; refactor #7235

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d829484
feat(api): add full step data to workflow dto; refactor
ChmaraX Dec 7, 2024
546801d
Merge branch 'next' into nv-4974-add-step-data-to-get-workflow-endpoint
ChmaraX Dec 7, 2024
0302cd8
feat(dashboard): minor fixes and improvements
ChmaraX Dec 9, 2024
01e4a62
feat(dashboard): improvements & fixes
ChmaraX Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 6 additions & 26 deletions apps/api/src/app/workflows-v2/mappers/notification-template-mapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {
PreferencesResponseDto,
RuntimeIssue,
ShortIsPrefixEnum,
StepResponseDto,
StepDataDto,
StepTypeEnum,
WorkflowCreateAndUpdateKeys,
WorkflowListResponseDto,
Expand All @@ -15,7 +15,10 @@ import { NotificationStepEntity, NotificationTemplateEntity } from '@novu/dal';
import { WorkflowInternalResponseDto } from '@novu/application-generic';
import { buildSlug } from '../../shared/helpers/build-slug';

export function toResponseWorkflowDto(workflow: WorkflowInternalResponseDto): WorkflowResponseDto {
export function toResponseWorkflowDto(
Copy link
Contributor

Choose a reason for hiding this comment

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

the point was letting this map to dto, if you send the DTO pre cooked I think it's taking the string out of the funciton no? I would expect to get the entity list and maps of the additinoal data you want to bake in

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 still maps the workflow to DTO as name of the method suggests, only the steps are coming in already in "DTO form".

We simply don't need to do any mapping for the steps for now, its already in viable form, which is good. Each layer of mapping just adds an overhead

workflow: WorkflowInternalResponseDto,
fullSteps: StepDataDto[]
ChmaraX marked this conversation as resolved.
Show resolved Hide resolved
): WorkflowResponseDto {
const preferencesDto: PreferencesResponseDto = {
user: workflow.userPreferences,
default: workflow.defaultPreferences,
Expand All @@ -30,7 +33,7 @@ export function toResponseWorkflowDto(workflow: WorkflowInternalResponseDto): Wo
tags: workflow.tags,
active: workflow.active,
preferences: preferencesDto,
steps: getSteps(workflow),
steps: fullSteps,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should keep the mapping here for two key reasons:

  1. We’re adding extra parameters, such as slug, which are only exposed to the API client.
  2. We’re mapping the types—for instance, if the entity allows undefined, we convert those to stricter types in the DTO response.

Copy link
Contributor

Choose a reason for hiding this comment

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

the first point is not relevant, i see that you added the slug to the BuildStepDataUsecase.

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 see your point but the stricter types in this case are already coming from the BuildStepDataUsecase.

The return type from the BuildStepDataUsecase doesn't allow undefined and does explicit checks for missing values, therefore having additional response type for step and mapping, e.g. like this:

name: step.name || 'Missing step name'

would be redundant . I think we should keep it simple and when the usecase emerges then we can have some mapping.

description: workflow.description,
origin: computeOrigin(workflow),
updatedAt: workflow.updatedAt || 'Missing Updated At',
Expand All @@ -40,16 +43,6 @@ export function toResponseWorkflowDto(workflow: WorkflowInternalResponseDto): Wo
};
}

function getSteps(template: NotificationTemplateEntity) {
const steps: StepResponseDto[] = [];
for (const step of template.steps) {
const stepResponseDto = toStepResponseDto(step);
steps.push(stepResponseDto);
}

return steps;
}

function toMinifiedWorkflowDto(template: NotificationTemplateEntity): WorkflowListResponseDto {
const workflowName = template.name || 'Missing Name';

Expand All @@ -71,19 +64,6 @@ export function toWorkflowsMinifiedDtos(templates: NotificationTemplateEntity[])
return templates.map(toMinifiedWorkflowDto);
}

function toStepResponseDto(persistedStep: NotificationStepEntity): StepResponseDto {
const stepName = persistedStep.name || 'Missing Name';

return {
_id: persistedStep._templateId,
slug: buildSlug(stepName, ShortIsPrefixEnum.STEP, persistedStep._templateId),
name: stepName,
stepId: persistedStep.stepId || 'Missing Step Id',
type: persistedStep.template?.type || StepTypeEnum.EMAIL,
issues: persistedStep.issues,
} satisfies StepResponseDto;
}
Comment on lines -74 to -85
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 moved this directly to the build-step-data use-case, because its always used together with the use-case.

If the usecase for unsanitized step data emerges, then we can split it again or create GetStepDataInternal usecase.

Now I don't think its even possible to create step without name or stepId or without type, it seems to be here only because of tests. These things should be validated upon creation and not rely on fallbacks anyway.


function buildStepTypeOverview(step: NotificationStepEntity): StepTypeEnum | undefined {
return step.template?.type;
}
Expand Down
4 changes: 3 additions & 1 deletion apps/api/src/app/workflows-v2/usecases/build-step-data/build-step-data.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { BadRequestException, Injectable } from '@nestjs/common';
import { ControlValuesLevelEnum, StepDataDto, WorkflowOriginEnum } from '@novu/shared';
import { ControlValuesLevelEnum, ShortIsPrefixEnum, StepDataDto, WorkflowOriginEnum } from '@novu/shared';
import { ControlValuesRepository, NotificationStepEntity, NotificationTemplateEntity } from '@novu/dal';
import { GetWorkflowByIdsUseCase, Instrument, InstrumentUsecase } from '@novu/application-generic';
import { BuildStepDataCommand } from './build-step-data.command';
import { InvalidStepException } from '../../exceptions/invalid-step.exception';
import { BuildAvailableVariableSchemaUsecase } from '../build-variable-schema';
import { buildSlug } from '../../../shared/helpers/build-slug';

@Injectable()
export class BuildStepDataUsecase {
Expand Down Expand Up @@ -43,6 +44,7 @@ export class BuildStepDataUsecase {
workflow,
}),
name: currentStep.name,
slug: buildSlug(currentStep.name, ShortIsPrefixEnum.STEP, currentStep._templateId),
_id: currentStep._templateId,
stepId: currentStep.stepId,
type: currentStep.template?.type,
Expand Down
42 changes: 38 additions & 4 deletions apps/api/src/app/workflows-v2/usecases/get-workflow/get-workflow.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
import { Injectable } from '@nestjs/common';

import { WorkflowResponseDto } from '@novu/shared';
import { GetWorkflowByIdsCommand, GetWorkflowByIdsUseCase, InstrumentUsecase } from '@novu/application-generic';
import { StepDataDto, UserSessionData, WorkflowResponseDto } from '@novu/shared';
import {
GetWorkflowByIdsCommand,
GetWorkflowByIdsUseCase,
InstrumentUsecase,
WorkflowInternalResponseDto,
} from '@novu/application-generic';

import { GetWorkflowCommand } from './get-workflow.command';
import { toResponseWorkflowDto } from '../../mappers/notification-template-mapper';
import { BuildStepDataUsecase } from '../build-step-data/build-step-data.usecase';
import { BuildStepDataCommand } from '../build-step-data/build-step-data.command';
import { NotificationStepEntity } from '@novu/dal';

@Injectable()
export class GetWorkflowUseCase {
constructor(private getWorkflowByIdsUseCase: GetWorkflowByIdsUseCase) {}
constructor(
private getWorkflowByIdsUseCase: GetWorkflowByIdsUseCase,
private buildStepDataUsecase: BuildStepDataUsecase
) {}

@InstrumentUsecase()
async execute(command: GetWorkflowCommand): Promise<WorkflowResponseDto> {
Expand All @@ -21,6 +32,29 @@ export class GetWorkflowUseCase {
})
);

return toResponseWorkflowDto(workflowEntity);
const fullSteps = await this.getFullWorkflowSteps(workflowEntity, command.user);

return toResponseWorkflowDto(workflowEntity, fullSteps);
}

private async getFullWorkflowSteps(
workflow: WorkflowInternalResponseDto,
user: UserSessionData
tatarco marked this conversation as resolved.
Show resolved Hide resolved
): Promise<StepDataDto[]> {
const stepPromises = workflow.steps.map((step: NotificationStepEntity & { _id: string }) =>
this.buildStepDataUsecase.execute(
BuildStepDataCommand.create({
workflowIdOrInternalId: workflow._id,
stepIdOrInternalId: step._id,
user,
})
)
);
ChmaraX marked this conversation as resolved.
Show resolved Hide resolved

try {
return Promise.all(stepPromises);
} catch (error) {
throw new Error(`Failed to build full workflow steps: ${error.message}`);
}
}
}
84 changes: 24 additions & 60 deletions apps/api/src/app/workflows-v2/usecases/sync-to-environment/sync-to-environment.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
PreferencesTypeEnum,
StepCreateDto,
StepDataDto,
StepResponseDto,
StepUpdateDto,
UpdateWorkflowDto,
WorkflowCreationSourceEnum,
Expand All @@ -16,7 +15,6 @@ import { Instrument, InstrumentUsecase } from '@novu/application-generic';
import { SyncToEnvironmentCommand } from './sync-to-environment.command';
import { GetWorkflowCommand, GetWorkflowUseCase } from '../get-workflow';
import { UpsertWorkflowCommand, UpsertWorkflowUseCase } from '../upsert-workflow';
import { BuildStepDataUsecase } from '../build-step-data';

/**
* This usecase is used to sync a workflow from one environment to another.
Expand All @@ -32,8 +30,7 @@ export class SyncToEnvironmentUseCase {
constructor(
private getWorkflowUseCase: GetWorkflowUseCase,
private preferencesRepository: PreferencesRepository,
private upsertWorkflowUseCase: UpsertWorkflowUseCase,
private buildStepDataUsecase: BuildStepDataUsecase
private upsertWorkflowUseCase: UpsertWorkflowUseCase
) {}

@InstrumentUsecase()
Expand All @@ -46,7 +43,7 @@ export class SyncToEnvironmentUseCase {
const preferencesToClone = await this.getWorkflowPreferences(originWorkflow._id, command.user.environmentId);
const externalId = originWorkflow.workflowId;
const targetWorkflow = await this.findWorkflowInTargetEnvironment(command, externalId);
const workflowDto = await this.buildRequestDto(originWorkflow, preferencesToClone, command, targetWorkflow);
const workflowDto = await this.buildRequestDto(originWorkflow, preferencesToClone, targetWorkflow);

return await this.upsertWorkflowUseCase.execute(
UpsertWorkflowCommand.create({
Expand All @@ -61,14 +58,13 @@ export class SyncToEnvironmentUseCase {
private async buildRequestDto(
originWorkflow: WorkflowResponseDto,
preferencesToClone: PreferencesEntity[],
command: SyncToEnvironmentCommand,
targetWorkflow?: WorkflowResponseDto
) {
if (targetWorkflow) {
return await this.mapWorkflowToUpdateWorkflowDto(originWorkflow, targetWorkflow, preferencesToClone, command);
return this.mapWorkflowToUpdateWorkflowDto(originWorkflow, targetWorkflow, preferencesToClone);
ChmaraX marked this conversation as resolved.
Show resolved Hide resolved
}

return await this.mapWorkflowToCreateWorkflowDto(originWorkflow, preferencesToClone, command);
return this.mapWorkflowToCreateWorkflowDto(originWorkflow, preferencesToClone);
}

@Instrument()
Expand All @@ -87,7 +83,7 @@ export class SyncToEnvironmentUseCase {
externalId: string
): Promise<WorkflowResponseDto | undefined> {
try {
return await this.getWorkflowUseCase.execute(
return this.getWorkflowUseCase.execute(
GetWorkflowCommand.create({
user: { ...command.user, environmentId: command.targetEnvironmentId },
workflowIdOrInternalId: externalId,
Expand All @@ -101,8 +97,7 @@ export class SyncToEnvironmentUseCase {
@Instrument()
private async mapWorkflowToCreateWorkflowDto(
originWorkflow: WorkflowResponseDto,
preferences: PreferencesEntity[],
command: SyncToEnvironmentCommand
preferences: PreferencesEntity[]
): Promise<CreateWorkflowDto> {
return {
workflowId: originWorkflow.workflowId,
Expand All @@ -111,83 +106,52 @@ export class SyncToEnvironmentUseCase {
tags: originWorkflow.tags,
description: originWorkflow.description,
__source: WorkflowCreationSourceEnum.DASHBOARD,
steps: await this.mapStepsToDto(originWorkflow.steps, command),
steps: await this.mapStepsToCreateOrUpdateDto(originWorkflow.steps),
preferences: this.mapPreferences(preferences),
};
}

@Instrument()
private async mapWorkflowToUpdateWorkflowDto(
originWorkflow: WorkflowResponseDto,
existingWorkflowInProd: WorkflowResponseDto | undefined,
preferencesToClone: PreferencesEntity[],
command: SyncToEnvironmentCommand
existingTargetEnvWorkflow: WorkflowResponseDto | undefined,
preferencesToClone: PreferencesEntity[]
): Promise<UpdateWorkflowDto> {
return {
workflowId: originWorkflow.workflowId,
name: originWorkflow.name,
active: originWorkflow.active,
tags: originWorkflow.tags,
description: originWorkflow.description,
steps: await this.mapStepsToDto(originWorkflow.steps, command, existingWorkflowInProd?.steps),
steps: await this.mapStepsToCreateOrUpdateDto(originWorkflow.steps, existingTargetEnvWorkflow?.steps),
preferences: this.mapPreferences(preferencesToClone),
};
}

@Instrument()
private async mapStepsToDto(
originSteps: StepResponseDto[],
command: SyncToEnvironmentCommand,
targetWorkflowSteps?: StepResponseDto[]
private async mapStepsToCreateOrUpdateDto(
originSteps: StepDataDto[],
targetEnvSteps?: StepDataDto[]
): Promise<(StepUpdateDto | StepCreateDto)[]> {
const augmentedSteps: (StepUpdateDto | StepCreateDto)[] = [];
for (const originStep of originSteps) {
const idAsOptionalObject = this.prodDbIdAsOptionalObject(originStep, targetWorkflowSteps);
const stepDataDto = await this.buildStepDataUsecase.execute({
workflowIdOrInternalId: command.workflowIdOrInternalId,
stepIdOrInternalId: originStep._id || originStep.stepId,
user: command.user,
});

augmentedSteps.push(this.buildSingleStepRequest(idAsOptionalObject, originStep, stepDataDto));
}
return originSteps.map((sourceStep) => {
// if we find matching step in target environment, we are updating
const targetEnvStepInternalId = targetEnvSteps?.find(
(targetStep) => targetStep.stepId === sourceStep.stepId
)?._id;

return augmentedSteps;
return this.buildStepCreateOrUpdateDto(sourceStep, targetEnvStepInternalId);
});
}
/*
* If we are updating an existing workflow, we need to map the updated steps to the existing steps
* (!) 'existingWorkflowSteps' are from a different environment than 'steps' - the only thing that doesn't change
* in steps across environments is the stepId (TODO)
*/
private buildSingleStepRequest(
idAsOptionalObject: { _id: string } | {},
step: StepResponseDto,
stepDataDto: StepDataDto
): StepUpdateDto | StepCreateDto {

private buildStepCreateOrUpdateDto(step: StepDataDto, existingInternalId?: string): StepUpdateDto | StepCreateDto {
return {
...idAsOptionalObject,
...(existingInternalId && { _id: existingInternalId }),
name: step.name ?? '',
type: step.type,
controlValues: stepDataDto.controls.values ?? {},
controlValues: step.controls.values ?? {},
};
}

private prodDbIdAsOptionalObject(originStep: StepResponseDto, targetWorkflowSteps?: StepResponseDto[]) {
const prodDatabaseId = this.findDatabaseIdInProdByExternalId(originStep, targetWorkflowSteps);

if (prodDatabaseId) {
return {
_id: prodDatabaseId,
};
} else {
return {};
}
}

private findDatabaseIdInProdByExternalId(originStep: StepResponseDto, targetWorkflowSteps?: StepResponseDto[]) {
return targetWorkflowSteps?.find((targetStep) => targetStep.stepId === originStep.stepId)?._id;
}

private mapPreferences(preferences: PreferencesEntity[]): {
user: WorkflowPreferences | null;
workflow: WorkflowPreferences | null;
Expand Down
14 changes: 11 additions & 3 deletions apps/api/src/app/workflows-v2/usecases/upsert-workflow/upsert-workflow.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ import {
} from '@novu/application-generic';
import { BadRequestException, Injectable } from '@nestjs/common';
import { UpsertWorkflowCommand } from './upsert-workflow.command';
import { toResponseWorkflowDto } from '../../mappers/notification-template-mapper';
import { stepTypeToDefaultDashboardControlSchema } from '../../shared';
import { PatchStepUsecase } from '../patch-step-data';
import { PostProcessWorkflowUpdate } from '../post-process-workflow-update';
import { GetWorkflowUseCase } from '../get-workflow/get-workflow.usecase';
import { GetWorkflowCommand } from '../get-workflow/get-workflow.command';

@Injectable()
export class UpsertWorkflowUseCase {
Expand All @@ -41,6 +42,7 @@ export class UpsertWorkflowUseCase {
private notificationGroupRepository: NotificationGroupRepository,
private workflowUpdatePostProcess: PostProcessWorkflowUpdate,
private getWorkflowByIdsUseCase: GetWorkflowByIdsUseCase,
private getWorkflowUseCase: GetWorkflowUseCase,
private patchStepDataUsecase: PatchStepUsecase
) {}

Expand All @@ -54,9 +56,15 @@ export class UpsertWorkflowUseCase {
workflow: persistedWorkflow,
});
await this.persistWorkflow(validatedWorkflowWithIssues);
persistedWorkflow = await this.getWorkflow(validatedWorkflowWithIssues._id, command);

return toResponseWorkflowDto(persistedWorkflow);
const workflow = await this.getWorkflowUseCase.execute(
GetWorkflowCommand.create({
workflowIdOrInternalId: validatedWorkflowWithIssues._id,
user: command.user,
})
);

return workflow;
}

@Instrument()
Expand Down
Loading
Loading