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-service): polish action steps #7389

Merged
merged 6 commits into from
Dec 26, 2024
Merged

feat(api-service): polish action steps #7389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c0dfe2
feat: polish action steps
djabarovgeorge Dec 26, 2024
106ff08
fix: amount param
djabarovgeorge Dec 26, 2024
802f726
chore(dashboard): update amount
djabarovgeorge Dec 26, 2024
597c717
chore(dashboard): logs
djabarovgeorge Dec 26, 2024
0b508ec
chore(dashboard): polish sanitize
djabarovgeorge Dec 26, 2024
0ba3b78
fix(dashboard): sms push chat sanitize
djabarovgeorge Dec 26, 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
1 change: 1 addition & 0 deletions apps/api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"scripts": {
"prebuild": "rimraf dist",
"build": "pnpm build:metadata && nest build",
"build:watch": "pnpm build:metadata && nest build --watch",
"format": "prettier --write \"src/**/*.ts\"",
"docker:build": "pnpm --silent --workspace-root pnpm-context -- apps/api/Dockerfile | BULL_MQ_PRO_NPM_TOKEN=${BULL_MQ_PRO_NPM_TOKEN} docker buildx build --load -t novu-api --secret id=BULL_MQ_PRO_NPM_TOKEN --build-arg PACKAGE_PATH=apps/api - $DOCKER_BUILD_ARGUMENTS",
"docker:build:depot": "pnpm --silent --workspace-root pnpm-context -- apps/api/Dockerfile | depot build --build-arg PACKAGE_PATH=apps/api - -t novu-api --load",
Expand Down
2 changes: 1 addition & 1 deletion apps/api/src/app/workflows-v2/usecases/generate-preview/generate-preview.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export class GeneratePreviewUsecase {
}

private sanitizeControlsForPreview(initialControlValues: Record<string, unknown>, stepData: StepDataDto) {
const sanitizedValues = dashboardSanitizeControlValues(initialControlValues, stepData.type);
const sanitizedValues = dashboardSanitizeControlValues(this.logger, initialControlValues, stepData.type);

return sanitizeControlValuesByOutputSchema(sanitizedValues || {}, stepData.type);
}
Expand Down
39 changes: 13 additions & 26 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 @@ -11,15 +11,13 @@ import {
} from '@novu/dal';
import {
ContentIssue,
CreateWorkflowDto,
JSONSchemaDto,
DEFAULT_WORKFLOW_PREFERENCES,
slugify,
StepContentIssueEnum,
StepCreateDto,
StepIssuesDto,
StepUpdateDto,
UpdateWorkflowDto,
UserSessionData,
StepTypeEnum,
WorkflowCreationSourceEnum,
Expand Down Expand Up @@ -50,6 +48,7 @@ import {
DeleteControlValuesUseCase,
TierRestrictionsValidateCommand,
dashboardSanitizeControlValues,
PinoLogger,
} from '@novu/application-generic';

import { UpsertWorkflowCommand, UpsertWorkflowDataCommand } from './upsert-workflow.command';
Expand All @@ -70,7 +69,8 @@ export class UpsertWorkflowUseCase {
private controlValuesRepository: ControlValuesRepository,
private upsertControlValuesUseCase: UpsertControlValuesUseCase,
private deleteControlValuesUseCase: DeleteControlValuesUseCase,
private tierRestrictionsValidateUsecase: TierRestrictionsValidateUsecase
private tierRestrictionsValidateUsecase: TierRestrictionsValidateUsecase,
private logger: PinoLogger
) {}

@InstrumentUsecase()
Expand Down Expand Up @@ -316,12 +316,12 @@ export class UpsertWorkflowUseCase {

const sanitizedControlValues =
controlValueLocal && workflowOrigin === WorkflowOriginEnum.NOVU_CLOUD
? dashboardSanitizeControlValues(controlValueLocal, step.type) || {}
: convertEmptyStringsToNull(controlValueLocal) || {};
? dashboardSanitizeControlValues(this.logger, controlValueLocal, step.type) || {}
: frameworkSanitizeEmptyStringsToNull(controlValueLocal) || {};

const controlIssues = processControlValuesBySchema(controlSchemas?.schema, sanitizedControlValues || {});
const liquidTemplateIssues = processControlValuesByLiquid(variableSchema, controlValueLocal || {});
const customIssues = await this.processControlValuesByRules(user, step.type, controlValueLocal || {});
const customIssues = await this.processControlValuesByRules(user, step.type, sanitizedControlValues || {});
const customControlIssues = _.isEmpty(customIssues) ? {} : { controls: customIssues };

return _.merge(controlIssues, liquidTemplateIssues, customControlIssues);
Expand Down Expand Up @@ -432,13 +432,11 @@ export class UpsertWorkflowUseCase {
stepType: StepTypeEnum,
controlValues: Record<string, unknown> | null
): Promise<StepIssuesDto> {
const cleanedControlValues = controlValues ? cleanObject(controlValues) : {};

const restrictionsErrors = await this.tierRestrictionsValidateUsecase.execute(
TierRestrictionsValidateCommand.create({
amount: cleanedControlValues.amount as string | undefined,
unit: cleanedControlValues.unit as string | undefined,
cron: cleanedControlValues.cron as string | undefined,
amount: controlValues?.amount as number | undefined,
unit: controlValues?.unit as string | undefined,
cron: controlValues?.cron as string | undefined,
organizationId: user.organizationId,
stepType,
})
Expand Down Expand Up @@ -571,20 +569,9 @@ function getErrorPath(error: ErrorObject): string {
return fullPath?.replace(/\//g, '.');
}

function cleanObject(
obj: Record<string, any>,
valuesToClean: Array<string | null | undefined> = ['', null, undefined]
) {
if (typeof obj !== 'object' || obj === null) return obj;

return Object.fromEntries(
Object.entries(obj)
.filter(([unused, value]) => !valuesToClean.includes(value as string | null | undefined))
.map(([key, value]) => [key, cleanObject(value, valuesToClean)])
);
}

function convertEmptyStringsToNull(obj: Record<string, unknown> | undefined): Record<string, unknown> | undefined {
function frameworkSanitizeEmptyStringsToNull(
obj: Record<string, unknown> | undefined | null
): Record<string, unknown> | undefined | null {
if (typeof obj !== 'object' || obj === null || obj === undefined) return obj;

return Object.fromEntries(
Expand All @@ -593,7 +580,7 @@ function convertEmptyStringsToNull(obj: Record<string, unknown> | undefined): Re
return [key, null];
}
if (typeof value === 'object') {
return [key, convertEmptyStringsToNull(value as Record<string, unknown>)];
return [key, frameworkSanitizeEmptyStringsToNull(value as Record<string, unknown>)];
}

return [key, value];
Expand Down
2 changes: 1 addition & 1 deletion ...ses/validate-content/prepare-and-validate-content/prepare-and-validate-content.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ export class PrepareAndValidateContentUsecase {
): Promise<Record<string, ContentIssue[]>> {
const restrictionsErrors = await this.tierRestrictionsValidateUsecase.execute(
TierRestrictionsValidateCommand.create({
amount: defaultControlValues.amount as string | undefined,
amount: defaultControlValues.amount as number | undefined,
unit: defaultControlValues.unit as string | undefined,
organizationId: user.organizationId,
stepType,
Expand Down
44 changes: 34 additions & 10 deletions apps/dashboard/src/components/workflow-editor/step-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { flatten } from 'flat';
import type {
ContentIssue,
StepCreateDto,
StepIssuesDto,
StepTypeEnum,
StepUpdateDto,
UpdateWorkflowDto,
WorkflowResponseDto,
} from '@novu/shared';
import { Step } from '@/utils/types';
import { STEP_TYPE_LABELS } from '@/utils/constants';
import { StepTypeEnum } from '@novu/shared';
import {
DEFAULT_CONTROL_DELAY_AMOUNT,
DEFAULT_CONTROL_DELAY_TYPE,
DEFAULT_CONTROL_DELAY_UNIT,
DEFAULT_CONTROL_DIGEST_AMOUNT,
DEFAULT_CONTROL_DIGEST_CRON,
DEFAULT_CONTROL_DIGEST_DIGEST_KEY,
DEFAULT_CONTROL_DIGEST_UNIT,
STEP_TYPE_LABELS,
} from '@/utils/constants';

export const getFirstBodyErrorMessage = (issues?: StepIssuesDto) => {
const stepIssuesArray = Object.entries({ ...issues?.body });
Expand Down Expand Up @@ -58,10 +67,25 @@ export const updateStepInWorkflow = (
};
};

export const createStep = (type: StepTypeEnum): Step => ({
name: STEP_TYPE_LABELS[type] + ' Step',
stepId: '',
slug: '_st_',
type,
_id: crypto.randomUUID(),
});
export const createStep = (type: StepTypeEnum): StepCreateDto => {
Copy link
Contributor Author

@djabarovgeorge djabarovgeorge Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we used the wrong type here, this is why we passed redundant data to the API such as slug prefix

const controlValue: Record<string, unknown> = {};

if (type === StepTypeEnum.DIGEST) {
controlValue.amount = DEFAULT_CONTROL_DIGEST_AMOUNT;
controlValue.unit = DEFAULT_CONTROL_DIGEST_UNIT;
controlValue.digestKey = DEFAULT_CONTROL_DIGEST_DIGEST_KEY;
controlValue.cron = DEFAULT_CONTROL_DIGEST_CRON;
}

if (type === StepTypeEnum.DELAY) {
controlValue.amount = DEFAULT_CONTROL_DELAY_AMOUNT;
controlValue.unit = DEFAULT_CONTROL_DELAY_UNIT;
controlValue.type = DEFAULT_CONTROL_DELAY_TYPE;
}

return {
name: STEP_TYPE_LABELS[type] + ' Step',
type,
controlValues: controlValue,
};
};
Comment on lines +70 to +91
Copy link
Contributor Author

@djabarovgeorge djabarovgeorge Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The summary of the findings of the best approach, why the change is necessary, and what is happening in the current implementation.

The main reason for this change is that we cannot create a step with control values during step creation. This limitation causes unexpected behavior, like the bug we are experiencing.

Currently, we cannot move schemas with default values to the Shared package and reuse them in the Dashboard because these schemas depend on Zod and Zod-to-JSON-Schema.

The solution we originally wanted—to move default values from the UiSchema to the dataSchema—is not supported natively. The solution should be Zod schema with default values (used by the Dashboard) and make certain attributes, like body, required for generating issues. However, this approach fails because zod-to-json-schema removes the required attributes if there is a default value meaning we will need to manipulate the schema manually.

This solution is reducing the amount of code. In addition once we store the default values during step creation, we wouldn't need to handle parsing them (placeholders/defaults) in the Dashboard or maintain "placeholders" or default values in the API.

Next Steps

  • Remove placeholders from the API.
  • Eliminate placeholder parsing in the Dashboard.

11 changes: 10 additions & 1 deletion apps/dashboard/src/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { StepTypeEnum } from '@novu/shared';
import { StepTypeEnum, TimeUnitEnum } from '@novu/shared';

export const AUTOCOMPLETE_PASSWORD_MANAGERS_OFF = {
autoComplete: 'off',
Expand Down Expand Up @@ -27,3 +27,12 @@ export const STEP_TYPE_LABELS: Record<StepTypeEnum, string> = {
[StepTypeEnum.TRIGGER]: 'Trigger',
[StepTypeEnum.CUSTOM]: 'Custom',
};

export const DEFAULT_CONTROL_DELAY_AMOUNT = 30;
export const DEFAULT_CONTROL_DELAY_UNIT = TimeUnitEnum.SECONDS;
export const DEFAULT_CONTROL_DELAY_TYPE = 'regular';

export const DEFAULT_CONTROL_DIGEST_AMOUNT = 30;
export const DEFAULT_CONTROL_DIGEST_UNIT = TimeUnitEnum.SECONDS;
export const DEFAULT_CONTROL_DIGEST_CRON = '';
export const DEFAULT_CONTROL_DIGEST_DIGEST_KEY = '';
9 changes: 5 additions & 4 deletions apps/worker/src/app/workflow/usecases/execute-bridge-job/execute-bridge-job.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Injectable, Logger } from '@nestjs/common';
import { Injectable } from '@nestjs/common';

import {
ControlValuesRepository,
Expand All @@ -15,7 +15,6 @@ import {
ExecutionDetailsStatusEnum,
ITriggerPayload,
JobStatusEnum,
StepTypeEnum,
WorkflowOriginEnum,
WorkflowTypeEnum,
} from '@novu/shared';
Expand All @@ -30,6 +29,7 @@ import {
ExecuteBridgeRequestCommand,
Instrument,
InstrumentUsecase,
PinoLogger,
} from '@novu/application-generic';
import { ExecuteBridgeJobCommand } from './execute-bridge-job.command';

Expand All @@ -44,7 +44,8 @@ export class ExecuteBridgeJob {
private environmentRepository: EnvironmentRepository,
private controlValuesRepository: ControlValuesRepository,
private createExecutionDetails: CreateExecutionDetails,
private executeBridgeRequest: ExecuteBridgeRequest
private executeBridgeRequest: ExecuteBridgeRequest,
private logger: PinoLogger
) {}

@InstrumentUsecase()
Expand Down Expand Up @@ -152,7 +153,7 @@ export class ExecuteBridgeJob {

if (workflow?.origin === WorkflowOriginEnum.NOVU_CLOUD) {
return controls?.controls
? dashboardSanitizeControlValues(controls.controls, command.job?.step?.template?.type)
? dashboardSanitizeControlValues(this.logger, controls.controls, command.job?.step?.template?.type)
: {};
}

Expand Down
4 changes: 2 additions & 2 deletions libs/application-generic/src/schemas/control/delay-control.schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import { defaultOptions, skipStepUiSchema, skipZodSchema } from './shared';
export const delayControlZodSchema = z
.object({
skip: skipZodSchema,
type: z.enum(['regular']).default('regular'),
amount: z.union([z.number().min(1), z.string()]),
type: z.enum(['regular']),
amount: z.number().min(1),
unit: z.nativeEnum(TimeUnitEnum),
})
.strict();
Expand Down
4 changes: 2 additions & 2 deletions libs/application-generic/src/schemas/control/digest-control.schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ const lookBackWindowZodSchema = z
const digestRegularControlZodSchema = z
.object({
skip: skipZodSchema,
amount: z.union([z.number().min(1), z.string().min(1)]),
amount: z.number().min(1),
unit: z.nativeEnum(TimeUnitEnum),
digestKey: z.string().optional(),
lookBackWindow: lookBackWindowZodSchema.optional(),
})
.strict();
const digestTimedControlZodSchema = z
.object({
skip: z.object({}).catchall(z.unknown()).optional(),
skip: skipZodSchema,
cron: z.string().min(1),
digestKey: z.string().optional(),
})
Expand Down
6 changes: 3 additions & 3 deletions ...ion-generic/src/usecases/tier-restrictions-validate/tier-restrictions-validate.command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import { OrganizationLevelCommand } from '../../commands';

export class TierRestrictionsValidateCommand extends OrganizationLevelCommand {
@IsOptional()
@Transform(({ value }) => (value ? String(value) : value))
@IsString()
amount?: string;
@Transform(({ value }) => (value ? Number(value) : value))
@IsNumber()
amount?: number;
Comment on lines +8 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be a number as we use it as number in the usecase


@IsString()
@IsOptional()
Expand Down
Loading
Loading