Set statuses on workflows #60

vkameswaran · 2025-01-28T17:12:42Z

Add listener to keep status on workflows up to date:
• version draft => statuses should contain draft
• version active => statuses should contain active
• version deactivated => if no version active, statuses should contain deactivated
Renaming also the endpoints because it was not reflecting the full behaviour.
Finally, adding a new status Archived for versions. Will be used when a version is deactivated, but is not the last published version anymore. It means this version cannot be re-activated.

greptile-apps

PR Summary

Hi! Looks like you've reached your API usage limit. You can increase it from your account settings page here: app.greptile.com/settings/billing/code-review

_{💡 (5/5) You can turn off certain types of comments like style here!}

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

greptile-apps-local

PR Summary

This PR introduces significant changes to the workflow status management system, focusing on maintaining accurate statuses for workflows and their versions. Key updates include:

Added WorkflowStatusModule to handle workflow status updates
Introduced new 'ARCHIVED' status for workflow versions
Renamed endpoints to better reflect their functionality
Implemented WorkflowStatusesUpdateJob for processing status changes
Created WorkflowVersionStatusListener to handle version-related events

Key points to consider:

WorkflowStatusModule is not explicitly imported in the main WorkflowModule
New WorkflowStatusesUpdateJob handles various scenarios but may benefit from additional error handling
WorkflowVersionStatusListener implementation is sound but could be improved with more robust error handling
Renaming of methods in workflow-trigger.workspace-service.ts aligns with new status management approach
Comprehensive unit tests added for WorkflowStatusesUpdateJob, ensuring proper functionality

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

greptile-apps-local · 2025-01-28T17:14:00Z

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

+
+const WorkflowStatusOptions = [
+  {
+    value: WorkflowVersionStatus.DRAFT,


logic: WorkflowVersionStatus is used here instead of WorkflowStatus

Suggested change

value: WorkflowVersionStatus.DRAFT,

value: WorkflowStatus.DRAFT,

greptile-apps-local · 2025-01-28T17:14:01Z

...ver/src/modules/workflow/workflow-status/jobs/__tests__/workflow-statuses-update.job.spec.ts

+  describe('handle', () => {
+    beforeEach(() => {
+      // make twentyORMManager.getRepository return a mock object
+      TwentyORMManager.prototype.getRepository = jest.fn();


style: Consider using jest.spyOn instead of directly mocking prototype

greptile-apps-local · 2025-01-28T17:14:04Z

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

+    color: 'yellow',
+  },
+  {
+    value: WorkflowVersionStatus.ACTIVE,


logic: WorkflowVersionStatus is used here instead of WorkflowStatus

Suggested change

value: WorkflowVersionStatus.ACTIVE,

value: WorkflowStatus.ACTIVE,

greptile-apps-local · 2025-01-28T17:14:06Z

...ages/twenty-server/src/modules/workflow/workflow-status/jobs/workflow-statuses-update.job.ts

+  private async hasWorkflowVersionByStatus(
+    workflowId: string,
+    status: WorkflowVersionStatus,
+  ): Promise<boolean> {
+    const workflowVersionRepository =
+      await this.twentyORMManager.getRepository<WorkflowVersionWorkspaceEntity>(
+        'workflowVersion',
+      );
+
+    const workflowVersions = await workflowVersionRepository.find({
+      where: {
+        workflowId,
+        status,
+      },
+    });
+
+    return workflowVersions.length > 0;
+  }


style: method could be optimized to use count instead of fetching all versions

Suggested change

private async hasWorkflowVersionByStatus(

workflowId: string,

status: WorkflowVersionStatus,

): Promise<boolean> {

const workflowVersionRepository =

await this.twentyORMManager.getRepository<WorkflowVersionWorkspaceEntity>(

'workflowVersion',

);

const workflowVersions = await workflowVersionRepository.find({

where: {

workflowId,

status,

},

});

return workflowVersions.length > 0;

}

private async hasWorkflowVersionByStatus(

workflowId: string,

status: WorkflowVersionStatus,

): Promise<boolean> {

const workflowVersionRepository =

await this.twentyORMManager.getRepository<WorkflowVersionWorkspaceEntity>(

'workflowVersion',

);

const count = await workflowVersionRepository.count({

where: {

workflowId,

status,

},

});

return count > 0;

}

greptile-apps-local · 2025-01-28T17:14:06Z

...ver/src/modules/workflow/workflow-status/jobs/__tests__/workflow-statuses-update.job.spec.ts

+        expect(mockWorkflowRepository.update).toHaveBeenCalledTimes(0);
+      });
+
+      test('when update that should be impossible, do not do anything', async () => {


style: Test name could be more specific about the impossible update

greptile-apps-local · 2025-01-28T17:14:06Z

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

+    color: 'green',
+  },
+  {
+    value: WorkflowVersionStatus.DEACTIVATED,


logic: WorkflowVersionStatus is used here instead of WorkflowStatus

Suggested change

value: WorkflowVersionStatus.DEACTIVATED,

value: WorkflowStatus.DEACTIVATED,

greptile-apps-local · 2025-01-28T17:14:18Z

...es/twenty-server/src/modules/workflow/workflow-trigger/workflow-trigger.workspace-service.ts

+  private emitStatusUpdateEventOrThrow(
+    workflowId: string,
+    previousStatus: WorkflowVersionStatus,
+    newStatus: WorkflowVersionStatus,
+  ) {
+    const workspaceId = this.scopedWorkspaceContextFactory.create().workspaceId;

-    if (activeWorkflowVersions.length > 0) {
+    if (!workspaceId) {
      throw new WorkflowTriggerException(
-        'Cannot have more than one active workflow version',
-        WorkflowTriggerExceptionCode.FORBIDDEN,
+        'No workspace id found',
+        WorkflowTriggerExceptionCode.INTERNAL_ERROR,
      );
    }

-    await workflowVersionRepository.update(
-      { id: workflowVersionId },
-      { status: WorkflowVersionStatus.ACTIVE },
-      manager,
+    this.workspaceEventEmitter.emit(
+      'workflowVersion.statusUpdated',
+      [
+        {
+          workflowId,
+          previousStatus,
+          newStatus,
+        } satisfies WorkflowVersionStatusUpdate,
+      ],
+      workspaceId,
    );


logic: Event emission could potentially fail silently. Consider adding error handling

greptile-apps-staging

PR Summary

This PR continues to enhance the workflow status management system with several important changes:

Added new getValidWorkflowVersionOrFail method in workflow-common.workspace-service.ts to improve validation logic separation
Modified WorkflowStatus enum in workflow.workspace-entity.ts to include DRAFT, ACTIVE, and DEACTIVATED states
Changed DEACTIVATED status color from gray to red in workflow-version.workspace-entity.ts
Introduced comprehensive test coverage in workflow-statuses-update.job.spec.ts for status transitions

Key points to review:

Type mismatch in WorkflowStatusOptions using WorkflowVersionStatus instead of WorkflowStatus enum
Missing implementation of ARCHIVED status handling in workflow-statuses-update.job.ts
Potential race conditions during concurrent status updates need addressing
No explicit error handling for missing workflows in status update logic

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

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

greptile-apps-staging · 2025-01-29T04:20:02Z

packages/twenty-server/src/modules/workflow/workflow-status/workflow-status.module.ts

+import { WorkflowVersionStatusListener } from 'src/modules/workflow/workflow-status/listeners/workflow-version-status.listener';
+
+@Module({
+  providers: [WorkflowStatusesUpdateJob, WorkflowVersionStatusListener],


style: Consider adding exports array to make these providers available to other modules that import WorkflowStatusModule

greptile-apps-staging · 2025-01-29T04:20:10Z

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

+
+const WorkflowStatusOptions = [
+  {
+    value: WorkflowVersionStatus.DRAFT,


logic: Using WorkflowVersionStatus.DRAFT instead of WorkflowStatus.DRAFT will cause type mismatch errors

Suggested change

value: WorkflowVersionStatus.DRAFT,

value: WorkflowStatus.DRAFT,

greptile-apps-staging · 2025-01-29T04:20:14Z

...ver/src/modules/workflow/workflow-status/jobs/__tests__/workflow-statuses-update.job.spec.ts

+  const mockWorkflowVersionRepository = {
+    find: jest.fn(),
+  };


logic: Repository mock is missing error cases - should test behavior when find() rejects

greptile-apps-staging · 2025-01-29T04:20:15Z

...ver/src/modules/workflow/workflow-status/jobs/__tests__/workflow-statuses-update.job.spec.ts

+      test('when WorkflowVersionStatus.ACTIVE to WorkflowVersionStatus.DEACTIVATED, should deactivate', async () => {
+        // Arrange
+        const event: WorkflowVersionBatchEvent = {
+          workspaceId: '1',
+          type: WorkflowVersionEventType.STATUS_UPDATE,
+          statusUpdates: [
+            {
+              workflowId: '1',
+              previousStatus: WorkflowVersionStatus.ACTIVE,
+              newStatus: WorkflowVersionStatus.DEACTIVATED,
+            },
+          ],
+        };
+
+        const mockWorkflow = {
+          statuses: [WorkflowStatus.ACTIVE, WorkflowStatus.DRAFT],
+        };
+
+        mockWorkflowRepository.findOneOrFail.mockResolvedValue(mockWorkflow);
+        mockWorkflowVersionRepository.find.mockResolvedValue([]);
+
+        // Act
+        await job.handle(event);
+
+        // Assert
+        expect(mockWorkflowRepository.findOneOrFail).toHaveBeenCalledTimes(1);
+        expect(mockWorkflowVersionRepository.find).toHaveBeenCalledWith({
+          where: {
+            workflowId: '1',
+            status: WorkflowVersionStatus.ACTIVE,
+          },
+        });
+        expect(mockWorkflowRepository.update).toHaveBeenCalledWith(
+          { id: '1' },
+          { statuses: [WorkflowStatus.DEACTIVATED, WorkflowStatus.DRAFT] },
+        );
+      });


logic: Test case doesn't verify behavior when there are multiple active versions during deactivation

greptile-apps-staging · 2025-01-29T04:20:15Z

...ty-server/src/modules/workflow/workflow-status/listeners/workflow-version-status.listener.ts

+        (event) =>
+          !event.properties.after.status ||
+          event.properties.after.status === WorkflowVersionStatus.DRAFT,


style: This condition allows undefined status to be treated as DRAFT. Consider explicitly checking for undefined to make the logic more clear.

greptile-apps-staging · 2025-01-29T04:20:23Z

...ages/twenty-server/src/modules/workflow/workflow-status/jobs/workflow-statuses-update.job.ts

+    const workflow = await workflowRepository.findOneOrFail({
+      where: {
+        id: workflowId,
+      },
+    });


style: findOneOrFail could throw an exception if workflow doesn't exist. Consider adding explicit error handling to provide better error messages.

greptile-apps-staging · 2025-01-29T04:20:29Z

...es/twenty-server/src/modules/workflow/workflow-trigger/workflow-trigger.workspace-service.ts

+    this.emitStatusUpdateEventOrThrow(
+      workflowVersion.workflowId,
+      workflowVersion.status,
+      WorkflowVersionStatus.DEACTIVATED,
+    );


logic: The status update event is emitted with the old status (workflowVersion.status) instead of WorkflowVersionStatus.ACTIVE as the previousStatus. This will cause incorrect status tracking.

Suggested change

this.emitStatusUpdateEventOrThrow(

workflowVersion.workflowId,

workflowVersion.status,

WorkflowVersionStatus.DEACTIVATED,

);

this.emitStatusUpdateEventOrThrow(

workflowVersion.workflowId,

WorkflowVersionStatus.ACTIVE,

WorkflowVersionStatus.DEACTIVATED,

);

greptile-apps-staging · 2025-01-29T04:20:29Z

...es/twenty-server/src/modules/workflow/workflow-trigger/workflow-trigger.workspace-service.ts

+    if (workflow.lastPublishedVersionId) {
+      await workflowVersionRepository.update(
+        { id: workflow.lastPublishedVersionId },
+        { status: WorkflowVersionStatus.ARCHIVED },
+        manager,
+      );
+    }


logic: No status update event is emitted when archiving a version. This could lead to inconsistencies in status tracking across the system.

greptile-apps-staging · 2025-01-29T04:20:30Z

...es/twenty-server/src/modules/workflow/workflow-trigger/workflow-trigger.workspace-service.ts

+    if (
+      workflow.lastPublishedVersionId &&
+      workflowVersion.id !== workflow.lastPublishedVersionId
+    ) {
+      await this.performDeactivationSteps(
+        workflow.lastPublishedVersionId,
+        workflowVersionRepository,
+        manager,
      );
+    }


style: Consider checking if the lastPublishedVersionId version exists before attempting deactivation steps. Could throw if version was deleted.

greptile-apps-staging · 2025-01-29T04:20:30Z

...es/twenty-server/src/modules/workflow/workflow-trigger/workflow-trigger.workspace-service.ts

+    const activeWorkflowVersions = await workflowVersionRepository.find(
+      {
+        where: {
+          workflowId: workflowVersion.workflowId,
+          status: WorkflowVersionStatus.ACTIVE,
+        },
+      },
+      manager,
+    );


logic: The active version check should include workflowVersion.id in the where clause to exclude the current version. Currently prevents reactivating a deactivated version.

Suggested change

const activeWorkflowVersions = await workflowVersionRepository.find(

{

where: {

workflowId: workflowVersion.workflowId,

status: WorkflowVersionStatus.ACTIVE,

},

},

manager,

);

const activeWorkflowVersions = await workflowVersionRepository.find(

{

where: {

workflowId: workflowVersion.workflowId,

status: WorkflowVersionStatus.ACTIVE,

id: { $ne: workflowVersion.id }

},

},

manager,

);

thomtrp added 4 commits August 29, 2024 16:53

Add listener to keep statuses up to date

0b16d20

Split into functions + rename endpoints

ccf196f

Use batch events

fcda756

Build new events

04cbc4b

greptile-apps bot reviewed Jan 28, 2025

View reviewed changes

greptile-apps-local bot reviewed Jan 28, 2025

View reviewed changes

greptile-apps-staging bot reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set statuses on workflows #60

Set statuses on workflows #60

vkameswaran commented Jan 28, 2025

greptile-apps bot left a comment

greptile-apps-local bot left a comment

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-local bot Jan 28, 2025

greptile-apps-staging bot left a comment

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

greptile-apps-staging bot Jan 29, 2025

	value: WorkflowVersionStatus.DRAFT,
	value: WorkflowStatus.DRAFT,

	value: WorkflowVersionStatus.ACTIVE,
	value: WorkflowStatus.ACTIVE,

	value: WorkflowVersionStatus.DEACTIVATED,
	value: WorkflowStatus.DEACTIVATED,

Set statuses on workflows #60

Are you sure you want to change the base?

Set statuses on workflows #60

Conversation

vkameswaran commented Jan 28, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps-local bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-local bot Jan 28, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps-staging bot Jan 29, 2025

Choose a reason for hiding this comment