Multiple operations on webhooks #7807

ehconitin · 2024-10-17T20:53:57Z

WIP :)

webhook.mp4

ehconitin · 2024-10-17T20:58:05Z

...es/twenty-server/src/engine/api/graphql/workspace-query-runner/jobs/call-webhook-jobs.job.ts

+      );
+    } else {
+      this.logger.debug(
+        `CallWebhookJobsJob on eventName '${eventName}' did not trigger any webhooks.`,
      );


we dont really need this log right?

think we can remove those 'not trigger any ...' logs

martmull

Hey @ehconitin thanks for your PR. The code looks good to me but I think we must take care of the data migrations. I suggest a 2 steps migration strategy:

First PR

leave operation column like it exists now and create a new column operations
create the command to copy data from operation to operations with arrays of lenght 1
duplicate operation updates into the operations column

Second PR

update the frontend and backend so it uses operations instead of operation
add the frontend code that enable operations arrays of length > 1
remove the operation column

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

martmull · 2024-10-18T07:36:04Z

...es/twenty-server/src/engine/api/graphql/workspace-query-runner/jobs/call-webhook-jobs.job.ts

+      );
+    } else {
+      this.logger.debug(
+        `CallWebhookJobsJob on eventName '${eventName}' did not trigger any webhooks.`,
      );


think we can remove those 'not trigger any ...' logs

martmull · 2024-10-18T07:44:51Z

packages/twenty-server/src/modules/webhook/standard-objects/webhook.workspace-entity.ts

  @WorkspaceField({
    standardId: WEBHOOK_STANDARD_FIELD_IDS.operation,
-    type: FieldMetadataType.TEXT,
+    type: FieldMetadataType.ARRAY,
    label: 'Operation',
    description: 'Webhook operation',
    icon: 'IconCheckbox',
  })
-  operation: string;
+  operations: string[];


I think we should proceed with a 2 steps strategy to migrate data properly:

First PR

leave operation column like it exists now and create a new column operations

create the command to copy data from operation to operations with arrays of lenght 1

duplicate operation updates into the operations column

Second PR

update the frontend and backend so it uses operations instead of operation

add the frontend code that enable operations arrays of length > 1

remove the operation column

ehconitin · 2024-10-18T09:44:31Z

@Bonapara @martmull
How about this?

update - just saw #7807 (comment)
working on it

webhook_new.mp4

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

martmull · 2024-10-18T10:20:35Z

@ehconitin
If the first operation is *.*, there is no need to add another operation as it will be necessarily included in the *.*
So we should allow adding the second operation only when the first operation is updated
And so on
I suggest to do that like @Bonapara designed it on Figma, without any "Add operation" button, secondary operations can be added automatically with the criteria details above (first operation is updated => add another operation, second operation is updated => add a 3rd operation, ...)

ehconitin · 2024-10-18T10:23:27Z

@ehconitin If the first operation is *.*, there is no need to add another operation as it will be necessarily included in the *.* So we should allow adding the second operation only when the first operation is updated And so on I suggest to do that like @Bonapara designed it on Figma, without any "Add operation" button, secondary operations can be added automatically with the criteria details above (first operation is updated => add another operation, second operation is updated => add a 3rd operation, ...)

Yep! missed your comment explaining this behavior in the above comments! Understood! Thanks

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

martmull · 2024-10-18T10:25:37Z

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

+    { value: '*', label: 'All Actions', Icon: IconNorthStar },
+    { value: 'create', label: 'Create', Icon: IconPlus },
+    { value: 'update', label: 'Update', Icon: IconRefresh },
+    { value: 'delete', label: 'Delete', Icon: IconTrash },


@charlesBochet @Bonapara should we add destroy there?
@ehconitin you can add it if needed

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

ehconitin · 2024-10-18T12:46:33Z

All requested changes from the review have been addressed!

TODO:

On initial render, if the operation is not *.*, insert a WEBHOOK_EMPTY_OPERATION below.
Find a better way to remove the existing filter.
@Bonapara suggested adding a 'No object' or 'Choose object' option, which could be treated as null and filtered out in cleanAndFormatOperations.
I’m still a bit uncertain about this approach but will give it a try.
@martmull, do you have any suggestions for the second TODO? We want to avoid having a delete (trash) button here.

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

- Mark operation as deprecated - Add operations migration command - Avoid breaking changes - Fix ellipsis display - Improve empty operation apearance

greptile-apps

PR Summary

This PR implements multiple operations for webhooks, addressing issue #7792. The changes include updating both frontend and backend components to support an array of operations instead of a single operation.

Added 'operations' field to WebhookWorkspaceEntity and deprecated 'operation' field
Updated SettingsDevelopersWebhookDetail component to handle multiple operations
Implemented CopyWebhookOperationIntoOperationsCommand for data migration
Modified CallWebhookJobsJob to query and process multiple operations
Added new icons (IconHandClick and IconNorthStar) to support the UI changes

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

greptile-apps · 2024-10-22T16:50:31Z

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

+      const baseOperations = data?.operations
+        ? data.operations.map((op: string) => {
+            const [object, action] = op.split('.');
+            return { object, action };
+          })
+        : data?.operation
+          ? [
+              {
+                object: data.operation.split('.')[0],
+                action: data.operation.split('.')[1],
+              },
+            ]
+          : [];


style: This logic might be better placed in a separate function for clarity

Suggested change

const baseOperations = data?.operations

? data.operations.map((op: string) => {

const [object, action] = op.split('.');

return { object, action };

})

: data?.operation

? [

{

object: data.operation.split('.')[0],

action: data.operation.split('.')[1],

},

]

: [];

const mapOperations = (data) => {

return data?.operations

? data.operations.map((op: string) => {

const [object, action] = op.split('.');

return { object, action };

})

: data?.operation

? [

{

object: data.operation.split('.')[0],

action: data.operation.split('.')[1],

},

]

: [];

};

const baseOperations = mapOperations(data);

greptile-apps · 2024-10-22T16:50:32Z

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

+        operation: cleanedOperations?.[0],
+        operations: cleanedOperations,


logic: Saving both 'operation' and 'operations' might lead to inconsistencies. Consider removing 'operation'

Suggested change

operation: cleanedOperations?.[0],

operations: cleanedOperations,

await updateOneRecord({

idToUpdate: webhookId,

updateOneRecordInput: {

operations: cleanedOperations,

description: description,

},

});

greptile-apps · 2024-10-22T16:50:32Z

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx

+    if (
+      !newOperations.some((op) => op.object === '*' && op.action === '*') &&
+      !newOperations.some((op) => op.object === null || op.action === null)
+    ) {
+      return [...newOperations, WEBHOOK_EMPTY_OPERATION];
+    }


style: This logic might add unnecessary empty operations. Consider simplifying

Suggested change

if (

!newOperations.some((op) => op.object === '*' && op.action === '*') &&

!newOperations.some((op) => op.object === null || op.action === null)

) {

return [...newOperations, WEBHOOK_EMPTY_OPERATION];

}

if (!newOperations.some((op) => op.object === '*' && op.action === '*')) {

return [...newOperations, WEBHOOK_EMPTY_OPERATION];

}

greptile-apps · 2024-10-22T16:51:07Z

...ty-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhooksNew.tsx

  const [formValues, setFormValues] = useState<{
    targetUrl: string;
    operation: string;
+    operations: string[];
  }>({
    targetUrl: '',
    operation: '*.*',
+    operations: ['*.*'],
  });


logic: Consider removing the 'operation' field from the state as it's being replaced by 'operations' array. This will prevent potential inconsistencies.

Suggested change

const [formValues, setFormValues] = useState<{

targetUrl: string;

operation: string;

operations: string[];

}>({

targetUrl: '',

operation: '*.*',

operations: ['*.*'],

});

const [formValues, setFormValues] = useState<{

targetUrl: string;

operations: string[];

}>({

targetUrl: '',

operations: ['*.*'],

});

greptile-apps · 2024-10-22T16:52:17Z

...atabase/commands/upgrade-version/0-32/0-32-copy-webhook-operation-into-operations-command.ts

+        if ('operation' in webhook) {
+          await webhookRepository.update(webhook.id, {
+            operations: [webhook.operation],
+          });


logic: Consider handling the case where 'operations' already exists to avoid overwriting existing data

greptile-apps · 2024-10-22T16:52:18Z

...atabase/commands/upgrade-version/0-32/0-32-copy-webhook-operation-into-operations-command.ts

+          await webhookRepository.update(webhook.id, {
+            operations: [webhook.operation],
+          });


logic: Ensure webhook.operation is not null or undefined before adding to array

greptile-apps · 2024-10-22T16:52:18Z

...atabase/commands/upgrade-version/0-32/0-32-copy-webhook-operation-into-operations-command.ts

+          'webhook',
+        );
+
+      const webhooks = await webhookRepository.find();


style: Consider using pagination for large datasets to avoid potential memory issues

greptile-apps · 2024-10-22T16:52:19Z

...atabase/commands/upgrade-version/0-32/0-32-copy-webhook-operation-into-operations-command.ts

+    options: BaseCommandOptions,
+    activeWorkspaceIds: string[],
+  ): Promise<void> {
+    this.logger.log('Running command to copy operation to operations');


style: Log the total number of workspaces being processed for better progress tracking

greptile-apps · 2024-10-22T16:52:20Z

...atabase/commands/upgrade-version/0-32/0-32-copy-webhook-operation-into-operations-command.ts

+          this.logger.log(
+            chalk.yellow(`Copied webhook operation to operations`),
+          );


style: Log the webhook ID for better traceability

greptile-apps · 2024-10-22T16:54:42Z

packages/twenty-server/src/modules/webhook/standard-objects/webhook.workspace-entity.ts

+    icon: 'IconCheckbox',
+    defaultValue: ['*.*'],
+  })
+  operations: string[];


style: operations field should have a more specific type than string[], e.g., string[] as WebhookOperation[]

Suggested change

operations: string[];

type WebhookOperation = string; // Define a specific type for operations

operations: WebhookOperation[];

martmull · 2024-10-22T17:16:39Z

add migration command
avoid breaking change
fix dropdown selected option ellipsis display
fix empty operation option appearance

Bonapara · 2024-10-23T12:32:06Z

@martmull @ehconitin Let me know when you're finished with this so I can do a final Product / Design review ;)

martmull · 2024-10-23T12:33:05Z

Finished

martmull · 2024-10-23T12:33:21Z

@Bonapara

WIP

c861aa1

ehconitin added the -PR: wip label Oct 17, 2024

ehconitin requested a review from martmull October 17, 2024 20:53

ehconitin assigned martmull Oct 17, 2024

ehconitin commented Oct 17, 2024

View reviewed changes

charlesBochet added the -PR: awaiting author label Oct 17, 2024

martmull reviewed Oct 18, 2024

View reviewed changes

ehconitin added 2 commits October 18, 2024 14:52

icons and log fixes

e7642ac

Merge remote-tracking branch 'upstream/main' into webhook-fix

ad25cc3

ehconitin commented Oct 18, 2024

View reviewed changes

...-front/src/pages/settings/developers/webhooks/components/SettingsDevelopersWebhookDetail.tsx Outdated Show resolved Hide resolved

martmull reviewed Oct 18, 2024

View reviewed changes

refactor and removed add opearation button

da5a47d

martmull reviewed Oct 18, 2024

View reviewed changes

ehconitin added 4 commits October 18, 2024 22:24

fix

f010de0

Merge remote-tracking branch 'upstream/main' into webhook-fix

e88cc53

Merge remote-tracking branch 'upstream/main' into webhook-fix

33ce77d

lint

8e6933b

charlesBochet force-pushed the main branch from 1c484d5 to 2f72e22 Compare October 21, 2024 11:19

ehconitin and others added 2 commits October 21, 2024 17:25

Merge branch 'twentyhq:main' into webhook-fix

6358f6f

Merge branch 'main' into webhook-fix

3f30764

martmull force-pushed the webhook-fix branch from 3ca4961 to 1eb316a Compare October 22, 2024 16:45

Martin's changes

d37b084

- Mark operation as deprecated - Add operations migration command - Avoid breaking changes - Fix ellipsis display - Improve empty operation apearance

martmull force-pushed the webhook-fix branch from 1eb316a to d37b084 Compare October 22, 2024 16:47

martmull marked this pull request as ready for review October 22, 2024 16:49

Merge branch 'main' into webhook-fix

ad98970

greptile-apps bot reviewed Oct 22, 2024

View reviewed changes

Fix tests

c4cc248

martmull approved these changes Oct 23, 2024

View reviewed changes

Thomas code review

d90baf4

martmull force-pushed the webhook-fix branch from 73d3591 to d90baf4 Compare October 23, 2024 13:51

martmull added 2 commits October 23, 2024 17:39

Fix lint

1933d5a

Merge branch 'main' into webhook-fix

a31581c

martmull enabled auto-merge (squash) October 23, 2024 15:40

martmull merged commit 18778c5 into twentyhq:main Oct 23, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple operations on webhooks #7807

Multiple operations on webhooks #7807

ehconitin commented Oct 17, 2024 •

edited

Loading

ehconitin Oct 17, 2024

martmull Oct 18, 2024

martmull left a comment

martmull Oct 18, 2024

martmull Oct 18, 2024

ehconitin commented Oct 18, 2024 •

edited

Loading

martmull commented Oct 18, 2024

ehconitin commented Oct 18, 2024

martmull Oct 18, 2024

ehconitin commented Oct 18, 2024

greptile-apps bot left a comment

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

greptile-apps bot Oct 22, 2024

martmull commented Oct 22, 2024

Bonapara commented Oct 23, 2024

martmull commented Oct 23, 2024

martmull commented Oct 23, 2024

		operation: cleanedOperations?.[0],
		operations: cleanedOperations,

-        operation: cleanedOperations?.[0],
-        operations: cleanedOperations,
+await updateOneRecord({
+  idToUpdate: webhookId,
+  updateOneRecordInput: {
+    operations: cleanedOperations,
+    description: description,
+  },
+});

	operations: string[];
	type WebhookOperation = string; // Define a specific type for operations
	operations: WebhookOperation[];

Multiple operations on webhooks #7807

Multiple operations on webhooks #7807

Conversation

ehconitin commented Oct 17, 2024 • edited Loading

ehconitin Oct 17, 2024

Choose a reason for hiding this comment

martmull Oct 18, 2024

Choose a reason for hiding this comment

martmull left a comment

Choose a reason for hiding this comment

First PR

Second PR

martmull Oct 18, 2024

Choose a reason for hiding this comment

martmull Oct 18, 2024

Choose a reason for hiding this comment

First PR

Second PR

ehconitin commented Oct 18, 2024 • edited Loading

martmull commented Oct 18, 2024

ehconitin commented Oct 18, 2024

martmull Oct 18, 2024

Choose a reason for hiding this comment

ehconitin commented Oct 18, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 22, 2024

Choose a reason for hiding this comment

martmull commented Oct 22, 2024

Bonapara commented Oct 23, 2024

martmull commented Oct 23, 2024

martmull commented Oct 23, 2024

ehconitin commented Oct 17, 2024 •

edited

Loading

ehconitin commented Oct 18, 2024 •

edited

Loading