Tt maintain statuses on workflows #47
Conversation
![greptile-apps-local[bot]](https://avatars.githubusercontent.com/u/140149887?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces significant changes to the workflow management system, focusing on maintaining and updating workflow statuses more effectively.
- Added new
WorkflowStatusModuleinworkflow-status.module.tsto handle status updates and version changes - Introduced
WorkflowStatusenum inworkflow.workspace-entity.ts, separating workflow statuses from version statuses - Implemented
WorkflowStatusesUpdateJobinworkflow-statuses-update.job.tsto process batch events for workflow version statuses - Refactored
WorkflowTriggerWorkspaceServiceinworkflow-trigger.workspace-service.tsto improve version management and status transitions - Added comprehensive test suite in
workflow-statuses-update.job.spec.tsto cover various scenarios for status updates
10 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| const WorkflowStatusOptions = [ | ||
| { | ||
| value: WorkflowVersionStatus.DRAFT, |
There was a problem hiding this comment.
logic: WorkflowVersionStatus is used here instead of WorkflowStatus
| constructor(private readonly twentyORMManager: TwentyORMManager) {} | ||
|
|
||
| @Process(WorkflowStatusesUpdateJob.name) | ||
| async handle(event: WorkflowVersionBatchEvent): Promise<void> { |
There was a problem hiding this comment.
logic: handle method processes events in parallel using Promise.all, which could lead to race conditions if multiple events affect the same workflow
| const statusUpdates = payload.events.filter( | ||
| (workflowVersionEvent) => | ||
| workflowVersionEvent.previousStatus !== workflowVersionEvent.newStatus, | ||
| ); |
There was a problem hiding this comment.
style: Consider adding a type annotation for workflowVersionEvent for better readability
| describe('handle', () => { | ||
| beforeEach(() => { | ||
| // make twentyORMManager.getRepository return a mock object | ||
| TwentyORMManager.prototype.getRepository = jest.fn(); |
There was a problem hiding this comment.
style: This mock might override the one set in the beforeEach block for the entire describe block. Consider moving it to individual tests if needed.
| const currentWorkflowStatuses = workflow.statuses || []; | ||
| let newWorkflowStatuses = new Set<WorkflowStatus>(currentWorkflowStatuses); |
There was a problem hiding this comment.
style: initializing newWorkflowStatuses with currentWorkflowStatuses is unnecessary as it's immediately reassigned in the following conditions
| if ( | ||
| isEqual(newWorkflowStatusesArray.sort(), currentWorkflowStatuses.sort()) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: sorting arrays for comparison may be inefficient for large status lists. Consider using a more optimized comparison method
| await this.upgradeWorflowVersion( | ||
| workflow, | ||
| workflowVersion.id, | ||
| workflowRepository, | ||
| workflowVersionRepository, | ||
| manager, | ||
| ); |
There was a problem hiding this comment.
syntax: Typo in method name: 'upgradeWorflowVersion' should be 'upgradeWorkflowVersion'
| ); | ||
| } | ||
|
|
||
| private async hasWorkflowVersionByStatus( |
There was a problem hiding this comment.
style: hasWorkflowVersionByStatus fetches all matching versions. For large datasets, consider using a count query or limit(1) for better performance
| const activeWorkflowVersions = await workflowVersionRepository.find( | ||
| { | ||
| where: { | ||
| workflowId: workflowVersion.workflowId, | ||
| status: WorkflowVersionStatus.ACTIVE, | ||
| }, | ||
| }, | ||
| manager, | ||
| ); |
There was a problem hiding this comment.
style: This query might be inefficient for large datasets. Consider using a count query instead of fetching all active versions.
| this.emitStatusUpdateEventOrThrow( | ||
| workflowVersion.workflowId, | ||
| workflowVersion.status, | ||
| WorkflowVersionStatus.ACTIVE, | ||
| ); |
There was a problem hiding this comment.
logic: Emitting the event before the database transaction is committed could lead to inconsistencies if the transaction fails.
|
@greptileai can you review this pull request? |
|
@greptileai can you review this pull request? |
There was a problem hiding this comment.
PR Summary
No major changes found since last review.
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
@greptileai can you explain what's happening in this pull request? |
There was a problem hiding this comment.
This pull request introduces several significant changes to the workflow management system. Here's a summary of the key modifications:
-
Method Renaming:
enableWorkflowTriggeris renamed toactivateWorkflowVersiondisableWorkflowTriggeris renamed todeactivateWorkflowVersion
These changes make the method names more descriptive and aligned with their functionality.
-
New Workflow Version Status:
- A new status
ARCHIVEDis added to theWorkflowVersionStatusenum. - This allows for better tracking of workflow version lifecycle.
- A new status
-
Workflow Status Management:
- Introduction of a new
WorkflowStatusenum and related options. - This helps in managing the overall status of workflows, not just individual versions.
- Introduction of a new
-
Event Listeners and Job Processing:
- New event listeners are added for workflow version creation, status updates, and deletion.
- A new job
WorkflowStatusesUpdateJobis introduced to handle these events asynchronously. - This improves the system's ability to react to changes in workflow versions and update statuses accordingly.
-
Workflow Activation Logic:
- The process of activating a workflow version is refactored and expanded.
- It now includes steps like upgrading the workflow version, setting statuses, and enabling triggers.
-
Error Handling and Validation:
- Additional checks and error handling are implemented to ensure data integrity and prevent invalid state transitions.
-
Module Updates:
- The
WorkflowStatusModuleis added to theWorkflowModule, integrating the new status management features.
- The
-
Testing:
- New tests are added for the
WorkflowStatusesUpdateJobto ensure proper functionality of the status update logic.
- New tests are added for the
These changes enhance the workflow management system by providing more granular control over workflow versions, improving status tracking, and ensuring that status changes are properly propagated and handled throughout the system.
|
@greptileai pls help me!! |
There was a problem hiding this comment.
The changes you've made involve renaming and refactoring some methods related to workflow version activation and deactivation, as well as adding a new status 'ARCHIVED' for workflow versions. Here's a summary of the key changes:
-
Method renaming:
enableWorkflowTriggeris nowactivateWorkflowVersiondisableWorkflowTriggeris nowdeactivateWorkflowVersion
-
New WorkflowVersionStatus:
- Added 'ARCHIVED' status to the WorkflowVersionStatus enum
-
New WorkflowStatus enum:
- Introduced a separate enum for workflow statuses (DRAFT, ACTIVE, DEACTIVATED)
-
Workflow status updates:
- Added a new job (WorkflowStatusesUpdateJob) to handle batch updates of workflow statuses
- Implemented a listener (WorkflowVersionStatusListener) to trigger the status update job
-
Refactoring:
- Split the activation and deactivation logic into smaller, more focused methods
- Added more granular error handling and status checks
-
New module:
- Added WorkflowStatusModule to handle the new status update functionality
These changes aim to improve the workflow management system by providing more precise control over workflow version statuses and ensuring that the overall workflow status is updated accordingly. The new job and listener system allows for more efficient batch processing of status updates.
Is there anything specific about these changes you'd like me to explain further?
|
@greptileai skibidi |
There was a problem hiding this comment.
PR Summary
This pull request continues to refine the workflow management system, focusing on improving error handling and status transitions.
- Enhanced error handling in
workflow-trigger.workspace-service.tsfor invalid workflow version transitions - Added
WorkflowVersionTransitionErrorinerrors.tsfor more specific error reporting - Implemented additional checks in
activateWorkflowVersionmethod to prevent invalid status transitions - Updated
deactivateWorkflowVersionmethod to handle ARCHIVED status correctly - Refined status update logic in
WorkflowStatusesUpdateJobto ensure consistent workflow statuses
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
@greptileai what's up |
There was a problem hiding this comment.
PR Summary
This pull request further refines the workflow management system, focusing on error handling and status transition improvements.
- Introduced
WorkflowVersionTransitionErrorinerrors.tsfor more specific error reporting in invalid transitions - Enhanced
activateWorkflowVersioninworkflow-trigger.workspace-service.tswith additional checks to prevent invalid status transitions - Updated
deactivateWorkflowVersionto properly handle the ARCHIVED status - Refined status update logic in
WorkflowStatusesUpdateJobto maintain consistent workflow statuses across versions
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
@greptileai please help |
There was a problem hiding this comment.
PR Summary
This pull request continues to refine the workflow management system, focusing on improving error handling and status transitions for workflow versions.
- Introduced
WorkflowVersionTransitionErrorinerrors.tsfor more specific error reporting during invalid transitions - Enhanced
activateWorkflowVersionmethod with additional checks to prevent invalid status transitions - Updated
deactivateWorkflowVersionmethod to properly handle the ARCHIVED status - Refined status update logic in
WorkflowStatusesUpdateJobto maintain consistent workflow statuses across versions
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
This update enhances the workflow status management system with improved error handling and validation.
- Added validation logic in
getValidWorkflowVersionOrFailto separate concerns from database operations - Implemented batch processing in
WorkflowVersionStatusListenerto efficiently handle multiple status updates - Enhanced error handling in
workflow-trigger.workspace-service.tswith transaction management for database operations - Updated method names in
workflow-trigger.resolver.tsfor better semantic clarity (e.g.,activateWorkflowVersion) - Added test coverage in
workflow-statuses-update.job.spec.tsfor status transition scenarios
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
10 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| const WorkflowStatusOptions = [ | ||
| { | ||
| value: WorkflowVersionStatus.DRAFT, |
There was a problem hiding this comment.
logic: WorkflowVersionStatus.DRAFT is used here but should be WorkflowStatus.DRAFT to match the new enum
| @Module({ | ||
| providers: [WorkflowStatusesUpdateJob, WorkflowVersionStatusListener], | ||
| }) |
There was a problem hiding this comment.
style: Module does not export its providers, which may prevent other modules from injecting WorkflowStatusesUpdateJob or WorkflowVersionStatusListener. Consider adding exports array if these services need to be used outside this module.
| color: 'yellow', | ||
| }, | ||
| { | ||
| value: WorkflowVersionStatus.ACTIVE, |
There was a problem hiding this comment.
logic: WorkflowVersionStatus.ACTIVE is used here but should be WorkflowStatus.ACTIVE to match the new enum
| import { WorkflowStatusesUpdateJob } from 'src/modules/workflow/workflow-status/jobs/workflow-statuses-update.job'; | ||
| import { WorkflowVersionStatusListener } from 'src/modules/workflow/workflow-status/listeners/workflow-version-status.listener'; |
There was a problem hiding this comment.
style: Using absolute imports with 'src/' can cause issues in production builds. Consider using relative imports or configuring path aliases properly.
| color: 'green', | ||
| }, | ||
| { | ||
| value: WorkflowVersionStatus.DEACTIVATED, |
There was a problem hiding this comment.
logic: WorkflowVersionStatus.DEACTIVATED is used here but should be WorkflowStatus.DEACTIVATED to match the new enum
| describe('handle', () => { | ||
| beforeEach(() => { | ||
| // make twentyORMManager.getRepository return a mock object | ||
| TwentyORMManager.prototype.getRepository = jest.fn(); |
There was a problem hiding this comment.
logic: mocking TwentyORMManager.prototype.getRepository is unnecessary since mockTwentyORMManager is already injected
| Omit<WorkflowVersionWorkspaceEntity, 'trigger'> & { | ||
| trigger: WorkflowTrigger; | ||
| } |
There was a problem hiding this comment.
style: return type could be simplified to WorkflowVersionWorkspaceEntity & { trigger: NonNullable<WorkflowTrigger> } for better type safety
| expect(mockWorkflowRepository.update).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('when no draft yet, update statuses', async () => { |
There was a problem hiding this comment.
logic: missing assertion to verify the actual status values being updated to in the repository call
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [ | ||
| WorkflowStatusesUpdateJob, | ||
| { | ||
| provide: TwentyORMManager, | ||
| useValue: mockTwentyORMManager, | ||
| }, | ||
| ], | ||
| }).compile(); | ||
|
|
||
| job = await module.resolve<WorkflowStatusesUpdateJob>( | ||
| WorkflowStatusesUpdateJob, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
style: reset mock call history in beforeEach to ensure clean state between tests
| const workflow = await workflowRepository.findOneOrFail({ | ||
| where: { | ||
| id: workflowId, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
logic: findOneOrFail will throw an exception if the workflow is not found. Consider handling this error gracefully with a custom exception that includes the workflowId for better debugging.
There was a problem hiding this comment.
PR Summary
This pull request further refines the workflow management system, focusing on improving error handling and status transitions.
- Implemented transaction management in
workflow-trigger.workspace-service.tsfor atomic database operations during workflow version activation and deactivation - Added
emitStatusUpdateEventOrThrowmethod to ensure consistent status update event emission - Refined
WorkflowStatusesUpdateJobto handle edge cases in status transitions, such as deactivation of non-active versions - Updated
WorkflowVersionStatusListenerto filter and process events more efficiently, reducing unnecessary processing
10 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| import { WorkflowStatusesUpdateJob } from 'src/modules/workflow/workflow-status/jobs/workflow-statuses-update.job'; | ||
| import { WorkflowVersionStatusListener } from 'src/modules/workflow/workflow-status/listeners/workflow-version-status.listener'; |
There was a problem hiding this comment.
style: consider using relative imports instead of absolute paths for better maintainability
| const workflowIds = payload.events | ||
| .filter( | ||
| (event) => | ||
| !event.properties.after.status || | ||
| event.properties.after.status === WorkflowVersionStatus.DRAFT, | ||
| ) | ||
| .map((event) => event.properties.after.workflowId); |
There was a problem hiding this comment.
style: consider using Set for unique workflowIds
| const workflowIds = payload.events | ||
| .filter( | ||
| (event) => | ||
| event.properties.before.status === WorkflowVersionStatus.DRAFT, | ||
| ) | ||
| .map((event) => event.properties.before.workflowId); |
There was a problem hiding this comment.
style: consider using Set for unique workflowIds
There was a problem hiding this comment.
PR Summary
This pull request continues to enhance the workflow management system, focusing on improving transaction handling and event processing. Key updates include:
- Implemented robust transaction management in
workflow-trigger.workspace-service.tsfor atomic database operations - Added
emitStatusUpdateEventOrThrowmethod inworkflow-trigger.workspace-service.tsfor consistent status update event emission - Refined
WorkflowStatusesUpdateJobto handle edge cases in status transitions more effectively - Optimized
WorkflowVersionStatusListenerfor more efficient event filtering and processing
10 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
This pull request further refines the workflow management system, focusing on transaction handling and event processing efficiency.
- Implemented robust transaction management in
workflow-trigger.workspace-service.tsfor atomic database operations during workflow version activation and deactivation - Added
emitStatusUpdateEventOrThrowmethod inworkflow-trigger.workspace-service.tsto ensure consistent status update event emission - Optimized
WorkflowVersionStatusListenerinworkflow-version-status.listener.tsfor more efficient event filtering and batch processing - Refined error handling in
workflow-trigger.workspace-service.tswith specific exceptions for invalid workflow version transitions
10 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| expect(mockWorkflowRepository.update).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('when no draft yet, update statuses', async () => { |
There was a problem hiding this comment.
logic: This test doesn't verify the actual update content. Consider adding an expectation for the update call arguments.
There was a problem hiding this comment.
PR Summary
This pull request continues to refine the workflow management system, focusing on improving transaction handling and error management. Key updates include:
- Enhanced transaction management in
workflow-trigger.workspace-service.tsfor atomic operations during workflow version activation and deactivation - Implemented
emitStatusUpdateEventOrThrowmethod inworkflow-trigger.workspace-service.tsto ensure consistent status update event emission - Improved error handling with specific exceptions for invalid workflow version transitions in
workflow-trigger.workspace-service.ts - Optimized
WorkflowVersionStatusListenerinworkflow-version-status.listener.tsfor more efficient event filtering and batch processing
10 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| this.workspaceEventEmitter.emit( | ||
| 'workflowVersion.statusUpdated', | ||
| [ | ||
| { | ||
| workflowId, | ||
| previousStatus, | ||
| newStatus, | ||
| } satisfies WorkflowVersionStatusUpdate, | ||
| ], | ||
| workspaceId, | ||
| ); |
There was a problem hiding this comment.
logic: Event emission is not awaited, which could lead to race conditions.
![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 update enhances the workflow status management system with improved validation and error handling.
- Added
getValidWorkflowVersionOrFailinworkflow-common.workspace-service.tsto separate validation logic from database operations - Implemented transaction rollback in
workflow-trigger.workspace-service.tsfor failed workflow version activations - Added specific error codes in
workflow-trigger.exception.tsfor invalid workflow version transitions - Enhanced test coverage in
workflow-statuses-update.job.spec.tsfor edge cases like concurrent status updates
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
10 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| const WorkflowStatusOptions = [ | ||
| { | ||
| value: WorkflowVersionStatus.DRAFT, |
There was a problem hiding this comment.
logic: Using WorkflowVersionStatus.DRAFT instead of WorkflowStatus.DRAFT - this will cause type mismatches and runtime errors
| import { WorkflowVersionStatusListener } from 'src/modules/workflow/workflow-status/listeners/workflow-version-status.listener'; | ||
|
|
||
| @Module({ | ||
| providers: [WorkflowStatusesUpdateJob, WorkflowVersionStatusListener], |
There was a problem hiding this comment.
logic: MessageQueueService is used in WorkflowVersionStatusListener but not imported/provided here. Add MessageQueueService to providers or imports.
| import { WorkflowVersionStatusListener } from 'src/modules/workflow/workflow-status/listeners/workflow-version-status.listener'; | ||
|
|
||
| @Module({ | ||
| providers: [WorkflowStatusesUpdateJob, WorkflowVersionStatusListener], |
There was a problem hiding this comment.
logic: TwentyORMManager is used in WorkflowStatusesUpdateJob but not imported/provided here. Add TwentyORMManager to providers or imports.
| const workflowIds = payload.events | ||
| .filter( | ||
| (event) => | ||
| event.properties.before.status === WorkflowVersionStatus.DRAFT, | ||
| ) |
There was a problem hiding this comment.
logic: only handling deletion of DRAFT status versions could miss cleanup needed for other statuses
| expect(mockWorkflowRepository.update).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('when no draft yet, update statuses', async () => { |
There was a problem hiding this comment.
logic: Missing assertion for the actual update values in this test. Should verify the new statuses array contains both ACTIVE and DRAFT
| export class WorkflowStatusesUpdateJob { | ||
| constructor(private readonly twentyORMManager: TwentyORMManager) {} | ||
|
|
||
| @Process(WorkflowStatusesUpdateJob.name) |
There was a problem hiding this comment.
logic: no error handling in the main handle method - failed promises in Promise.all() could cause partial updates
|
|
||
| const workflowVersion = await workflowVersionRepository.findOne({ | ||
| await this.upgradeWorflowVersion( |
There was a problem hiding this comment.
syntax: typo in method name 'upgradeWorflowVersion' (missing 'k')
| this.emitStatusUpdateEventOrThrow( | ||
| workflowVersion.workflowId, | ||
| workflowVersion.status, | ||
| WorkflowVersionStatus.DEACTIVATED, | ||
| ); |
There was a problem hiding this comment.
logic: status update event is emitted with the old status when it should be using the new DEACTIVATED status
| const activeWorkflowVersions = await workflowVersionRepository.find( | ||
| { | ||
| where: { | ||
| workflowId: workflowVersion.workflowId, | ||
| status: WorkflowVersionStatus.ACTIVE, | ||
| }, | ||
| }, | ||
| manager, | ||
| ); |
There was a problem hiding this comment.
logic: race condition possible between checking active versions and setting new active version
No description provided.