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.

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

fix(framework): Stop validating controls for non previewed step #6876

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 68 additions & 1 deletion packages/framework/src/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1350,7 +1350,15 @@ describe('Novu Client', () => {
workflowId: 'test-workflow',
stepId: 'active-step-id',
subscriber: {},
state: [],
state: [
{
stepId: 'skipped-step-id',
outputs: {},
state: {
status: 'success',
},
},
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An empty outputs should have been supplied for this test, ensuring that a hydration can still occur for skipped steps. This fixes the assertion.

payload: {},
controls: {},
};
Expand Down Expand Up @@ -1433,6 +1441,65 @@ describe('Novu Client', () => {
expect(metadata.duration).toEqual(expect.any(Number));
});

it('should preview a non-first step in a workflow successfully when action is preview', async () => {
const newWorkflow = workflow('test-workflow', async ({ step }) => {
await step.delay(
'delay-step',
async (controls) => ({
amount: controls.amount,
unit: controls.unit,
}),
{
controlSchema: {
type: 'object',
properties: {
amount: { type: 'number' },
unit: {
type: 'string',
enum: ['seconds', 'minutes', 'hours', 'days', 'weeks', 'months'],
},
},
required: ['amount', 'unit'],
additionalProperties: false,
} as const,
}
);

await step.inApp('send-in-app', async () => ({ body: 'Test Body', subject: 'Subject' }));
});

client.addWorkflows([newWorkflow]);

const event: Event = {
action: PostActionEnum.PREVIEW,
workflowId: 'test-workflow',
stepId: 'send-in-app',
subscriber: {},
state: [],
payload: {},
controls: {},
};

const executionResult = await client.executeWorkflow(event);

expect(executionResult).toBeDefined();
expect(executionResult.outputs).toBeDefined();
if (!executionResult.outputs) throw new Error('executionResult.outputs is undefined');

const { body } = executionResult.outputs;
expect(body).toBe('Test Body');

const { subject } = executionResult.outputs;
expect(subject).toBe('Subject');

expect(executionResult.providers).toEqual({});

const { metadata } = executionResult;
expect(metadata.status).toBe('success');
expect(metadata.error).toBe(false);
expect(metadata.duration).toEqual(expect.any(Number));
});

it('should preview workflow successfully when action is preview and skipped', async () => {
const newWorkflow = workflow('test-workflow', async ({ step }) => {
await step.email('send-email', async () => ({ body: 'Test Body', subject: 'Subject' }), {
Expand Down
24 changes: 13 additions & 11 deletions packages/framework/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,25 +277,27 @@ export class Client {
}

const step = this.getStep(event.workflowId, stepId);
const controls = await this.createStepControls(step, event);
const isPreview = event.action === PostActionEnum.PREVIEW;

if (!isPreview && (await this.shouldSkip(options?.skip as typeof step.options.skip, controls))) {
if (stepId === event.stepId) {
// Only set the result when the step is the current step.
// Only evaluate a skip condition when the step is the current step and not in preview mode.
if (!isPreview && stepId === event.stepId) {
const controls = await this.createStepControls(step, event);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, we ensure step controls are only validated during execution where the controls are guaranteed to be available for the current step being executed.

const shouldSkip = await this.shouldSkip(options?.skip as typeof step.options.skip, controls);

if (shouldSkip) {
setResult({
options: { skip: true },
outputs: {},
providers: {},
});
}

/*
* Return an empty object for results when a step is skipped.
* TODO: fix typings when `skip` is specified to return `Partial<T_Result>`
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return {} as any;
/*
* Return an empty object for results when a step is skipped.
* TODO: fix typings when `skip` is specified to return `Partial<T_Result>`
Copy link
Contributor

Choose a reason for hiding this comment

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

just to make sure it doesn't bypass the second PR and results will be supplied if i provide them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this code branch is only for the non-preview case.

*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return {} as any;
}
}

const previewStepHandler = this.previewStep.bind(this);
Expand Down
Loading