Execute variables in action input #7715

thomtrp · 2024-10-15T11:32:31Z

send context from all previous steps rather than unique payload
wrap input data in settings into input field
add email into send email action settings
update output shape

martmull

Hey, good job, some comments

packages/twenty-server/src/modules/mail-sender/workflow-actions/send-email.workflow-action.ts

packages/twenty-server/src/modules/workflow/workflow-executor/utils/variable-resolver.util.ts

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

greptile-apps

PR Summary

This pull request introduces significant changes to the workflow execution process, focusing on variable handling and input structure for workflow actions. Here's a concise summary of the major changes:

Restructured workflow step settings to wrap input data in an 'input' field for both code and email actions
Added email field to send email action settings for improved flexibility
Implemented variable resolution using Handlebars templating for dynamic content in action inputs
Updated workflow execution to use a context object containing data from all previous steps
Modified workflow run output structure to track multiple execution attempts per step
Introduced a new variable resolver utility for handling different input types and error scenarios

Key points to consider:

The changes may require updates to existing code that relies on the previous input structure
New error handling for variable evaluation failures has been added
The modifications aim to enhance flexibility and tracking in workflow execution

_{20 file(s) reviewed, 25 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-front/src/modules/workflow/components/WorkflowEditActionFormSendEmail.tsx

...es/twenty-front/src/modules/workflow/components/WorkflowEditActionFormServerlessFunction.tsx

packages/twenty-server/src/modules/mail-sender/workflow-actions/send-email.workflow-action.ts

packages/twenty-server/src/modules/workflow/workflow-executor/utils/variable-resolver.util.ts

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

packages/twenty-server/src/modules/workflow/workflow-runner/jobs/run-workflow.job.ts

Devessier

Great job, @thomtrp! I left some comments to discuss a few things in your implementation. That looks great! 😉

Devessier · 2024-10-15T14:08:37Z

.../twenty-server/src/modules/workflow/common/standard-objects/workflow-run.workspace-entity.ts

    attemptCount: number;
    result: object | undefined;
    error: string | undefined;
  }[];
 };

+export type WorkflowRunOutput = {
+  steps: Record<string, StepRunOutput>;


The consequence of storing the output of the steps in an object is that their order is not guaranteed to be preserved. That's not an issue on its own, but the frontend will have to fetch the steps definition to print the output in order.

Indeed. In V1, the frontend may have to re-order the output if we go with the figma. But most likely, we will end with a drawer and each step will look for its output. That's where a map with the step id as key will be useful!

Devessier · 2024-10-15T14:09:54Z

...twenty-server/src/modules/workflow/workflow-executor/interfaces/workflow-action.interface.ts

  }: {
    step: WorkflowStep;
-    payload?: object;
+    context?: Record<string, any>;


I like to use unknown instead of any when I don't know the data type that will be there. It forces me to type-check it explicitly later.

Also, we might want to create a specific type for the context so we can reuse it easily.

We use Record<string, any> in a lot of places actually. This is because we do not know the shape of the objects. And same goes for context, since it could contain anything. Not sure what specific type we would create...

About the unknown you are right, I updated the code 👍

Perfect for unknown 👌
I wasn't clear, but I meant that we could create a WorkflowStepInputContext type or another name not to repeat Record<string, unknown> many times.

...es/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step-settings.type.ts

Devessier · 2024-10-15T14:12:21Z

packages/twenty-server/src/modules/workflow/workflow-executor/utils/variable-resolver.util.ts

+const VARIABLE_PATTERN = RegExp('\\{\\{(.*?)\\}\\}', 'g');
+
+export const resolve = (
+  unresolvedInput: any,


Is it possible for us to predict a more specific type for the unresolvedInput parameter? If it's not, I would use unknown to be forced to explicitly check the type of the arguments.

nope as explained above. So using unknown!

Devessier · 2024-10-15T14:23:36Z

packages/twenty-server/src/modules/workflow/workflow-executor/utils/variable-resolver.util.ts

+  return input.replace(VARIABLE_PATTERN, (matchedToken, _) => {
+    const processedToken = evalFromContext(matchedToken, context);
+
+    return processedToken;
+  });


Aren't we redoing the job of Handlebars here? Couldn't we call evalFromContext on the whole input string and let Handlebars replace the variables? i might misunderstand something.

I want to focus on variables only there. Giving the whole string to handlebars can make it translate pretty much anything. See different templates here https://handlebarsjs.com/guide/#nested-input-objects

Indeed. Having loops and conditions might be interesting when writing the body of the email, though!

martmull

Two small comment about naming and typing but LGTM though. Nice work!

martmull · 2024-10-15T17:03:32Z

...twenty-server/src/modules/workflow/workflow-executor/interfaces/workflow-action.interface.ts

-    step: WorkflowStep;
-    payload?: object;
-  }): Promise<WorkflowActionResult>;
+  execute(payload: unknown): Promise<WorkflowActionResult>;


we know that payload is at least an object isn't it?

We cannot be sure. The step could have an input null. Or the inferred variables could provide anything

martmull · 2024-10-15T17:04:59Z

packages/twenty-server/src/modules/serverless/workflow-actions/code.workflow-action.ts

-    step: WorkflowCodeStep;
-    payload?: object;
-  }): Promise<WorkflowActionResult> {
+  async execute(payload: WorkflowCodeStepInput): Promise<WorkflowActionResult> {


payload might not be the proper naming finally. What about workflowStepInput or actionInput or just input?

I will go with workflowStepInput there!

martmull · 2024-10-15T17:06:04Z

...server/src/modules/workflow/workflow-executor/utils/__tests__/variable-resolver.util.spec.ts

+  it('should throw an error for invalid expressions', () => {
+    expect(() => resolveInput('{{invalidFunction()}}', context)).toThrow();
+  });
+});


Devessier

Great job, @thomtrp, as usual! We can merge.

Devessier · 2024-10-16T08:46:31Z

packages/twenty-server/src/modules/workflow/workflow-executor/utils/variable-resolver.util.ts

+  return input.replace(VARIABLE_PATTERN, (matchedToken, _) => {
+    const processedToken = evalFromContext(matchedToken, context);
+
+    return processedToken;
+  });


Indeed. Having loops and conditions might be interesting when writing the body of the email, though!

thomtrp changed the title ~~Use context in workflows rather than payload~~ Execute variables in action input Oct 15, 2024

thomtrp force-pushed the tt-execute-variable-in-workflow branch 2 times, most recently from 48367c5 to 126dcea Compare October 15, 2024 13:28

thomtrp marked this pull request as ready for review October 15, 2024 13:29

martmull reviewed Oct 15, 2024

View reviewed changes

greptile-apps bot reviewed Oct 15, 2024

View reviewed changes

Devessier reviewed Oct 15, 2024

View reviewed changes

Devessier mentioned this pull request Oct 15, 2024

Add Workflow Run show page #7719

Merged

thomtrp force-pushed the tt-execute-variable-in-workflow branch 3 times, most recently from 9dc1752 to 0906d12 Compare October 15, 2024 16:52

martmull approved these changes Oct 15, 2024

View reviewed changes

Devessier self-requested a review October 16, 2024 08:42

Devessier approved these changes Oct 16, 2024

View reviewed changes

thomtrp added 5 commits October 16, 2024 14:16

Wrap into input + use context + add email field

17622d7

Resolve variables

ef0e589

Send payload rather than step to action

cd6ffd0

use uknown in separated functions + add tests

da991d6

Rename input

a72ebcc

thomtrp force-pushed the tt-execute-variable-in-workflow branch from c42303c to a72ebcc Compare October 16, 2024 12:16

thomtrp merged commit e811bae into main Oct 16, 2024
16 checks passed

thomtrp deleted the tt-execute-variable-in-workflow branch October 16, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute variables in action input #7715

Execute variables in action input #7715

thomtrp commented Oct 15, 2024 •

edited

Loading

martmull left a comment

greptile-apps bot left a comment

Devessier left a comment

Devessier Oct 15, 2024

thomtrp Oct 15, 2024

Devessier Oct 15, 2024

Devessier Oct 15, 2024

thomtrp Oct 15, 2024

Devessier Oct 16, 2024

Devessier Oct 15, 2024

thomtrp Oct 15, 2024

Devessier Oct 15, 2024

thomtrp Oct 15, 2024

Devessier Oct 16, 2024

martmull left a comment

martmull Oct 15, 2024

thomtrp Oct 15, 2024

martmull Oct 15, 2024

thomtrp Oct 15, 2024 •

edited

Loading

martmull Oct 15, 2024

Devessier left a comment

Devessier Oct 16, 2024

Execute variables in action input #7715

Execute variables in action input #7715

Conversation

thomtrp commented Oct 15, 2024 • edited Loading

martmull left a comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Devessier left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martmull left a 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

thomtrp Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devessier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomtrp commented Oct 15, 2024 •

edited

Loading

thomtrp Oct 15, 2024 •

edited

Loading