Create record action #8460
Create record action #8460
Conversation
thomtrp
commented
Nov 12, 2024
thomtrp
commented
- Add create record action
- Migrate all step name => action in a common folder
- Separate types
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new CreateRecord workflow action and reorganizes workflow-related code for better maintainability. Here's a concise summary of the key changes:
- Added new CreateRecordWorkflowAction for creating database records through workflows using TwentyORMManager
- Migrated all "step" terminology to "action" across the codebase for consistency
- Reorganized workflow action types and modules into a common /workflow-actions directory structure
- Added proper type definitions for the new create record action with WorkflowCreateRecordActionInput and settings types
- Refactored workflow executor module to use more specific action modules (CodeActionModule, SendEmailActionModule, CreateRecordActionModule)
Note: The CreateRecordWorkflowAction implementation could benefit from additional error handling for invalid object names and failed record operations.
27 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -1,4 +1,4 @@ | |||
| import { WorkflowActionResult } from 'src/modules/workflow/workflow-executor/types/workflow-action-result.type'; | |||
| import { WorkflowActionResult } from 'src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action-result.type'; | |||
|
|
|||
| export interface WorkflowAction { | |||
| execute(workflowStepInput: unknown): Promise<WorkflowActionResult>; | |||
There was a problem hiding this comment.
style: Consider adding type safety by making workflowStepInput generic instead of unknown
There was a problem hiding this comment.
logic: The WorkflowActionType enum only contained CODE and SEND_EMAIL, but the PR adds CREATE_RECORD. Make sure the new enum location includes all three action types.
...-server/src/modules/workflow/workflow-executor/workflow-actions/code/code.workflow-action.ts
Show resolved
Hide resolved
...es/workflow/workflow-executor/workflow-actions/code/types/workflow-code-action-input.type.ts
Show resolved
Hide resolved
...s/workflow/workflow-executor/workflow-actions/create-record/create-record.workflow-action.ts
Outdated
Show resolved
Hide resolved
...orkflow-executor/workflow-actions/mail-sender/types/workflow-send-email-action-input.type.ts
Show resolved
Hide resolved
...c/modules/workflow/workflow-executor/workflow-actions/types/workflow-action-settings.type.ts
Show resolved
Hide resolved
...c/modules/workflow/workflow-executor/workflow-actions/types/workflow-action-settings.type.ts
Show resolved
Hide resolved
...c/modules/workflow/workflow-executor/workflow-actions/types/workflow-action-settings.type.ts
Show resolved
Hide resolved
...server/src/modules/workflow/workflow-executor/workflow-actions/types/workflow-action.type.ts
Show resolved
Hide resolved
thomtrp
force-pushed
the
tt-create-record-action
branch
from
8435b54 to
4b72d8f
Compare
thomtrp
force-pushed
the
tt-create-record-action
branch
from
4b72d8f to
d2a070a
Compare
| @@ -98,7 +98,7 @@ export class WorkflowVersionWorkspaceEntity extends BaseWorkspaceEntity { | |||
| icon: 'IconSettingsAutomation', | |||
| }) | |||
| @WorkspaceIsNullable() | |||
| steps: WorkflowStep[] | null; | |||
| steps: WorkflowAction[] | null; | |||
There was a problem hiding this comment.
do you think it worth renaming steps into actions?
There was a problem hiding this comment.
I don't think branches, conditions, loops will be actions. There would be better as steps. Wdyt?
| CREATE = 'Create', | ||
| UPDATE = 'Update', | ||
| DELETE = 'Delete', |
There was a problem hiding this comment.
| CREATE = 'Create', | |
| UPDATE = 'Update', | |
| DELETE = 'Delete', | |
| CREATE = 'create', | |
| UPDATE = 'update', | |
| DELETE = 'delete', |
| export enum WorkflowActionType { | ||
| CODE = 'CODE', | ||
| SEND_EMAIL = 'SEND_EMAIL', | ||
| RECORD_OPERATION = 'RECORD_OPERATION', |
There was a problem hiding this comment.
| RECORD_OPERATION = 'RECORD_OPERATION', | |
| CRUD_RECORD = 'CRUD_RECORD', |
There was a problem hiding this comment.
alright renaming everything
| input: object; | ||
| outputSchema: OutputSchema; | ||
| errorHandlingOptions: { | ||
| retryOnFailure: { |
There was a problem hiding this comment.
seems both bool are mutually exclusive, right?
If so, should we create a strategy enum instead? @thomtrp
If both are true, does that mean it will retry once and continue if still fails? If so you can ignore my comment. You could take example on queue system libraries such as bullmq, usually we want to provide the number of retry etc.
Up to you!
There was a problem hiding this comment.
If both are true, does that mean it will retry once and continue if still fails?
Yes, it will retryMAX_RETRIES_ON_FAILUREwhich is a repo constant for now, and then continue if it fails
