Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webhook wip #6371

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Webhook wip #6371

merged 6 commits into from
Aug 5, 2024

Conversation

charlesBochet
Copy link
Member

@charlesBochet charlesBochet commented Jul 23, 2024

This PR introduces the following changes:

  • Add the ability to filter webhooks by objectSingularName and Actions
  • Refactor SettingsWebhookDetails edition to not use react-hook-form (which will be deprecated on the whole project)
  • Updating the tests with a complex set of mock (we just need to fix ~30 of them now :D)
image

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This pull request introduces a new section for filtering webhook events and reorganizes import statements in a specific file.

  • Reorganized Imports: packages/twenty-front/src/modules/settings/data-model/fields/forms/components/SettingsDataModelFieldTypeSelect.tsx - Reordered import statements, no functional changes.
  • Webhook Event Filtering: packages/twenty-front/src/pages/settings/developers/webhooks/SettingsDevelopersWebhookDetail.tsx - Added a Select component for filtering webhook events.
  • New Hook Integration: packages/twenty-front/src/pages/settings/developers/webhooks/SettingsDevelopersWebhookDetail.tsx - Introduced useUpdateOneRecord hook to update webhook operations based on selected filters.
  • Potential Pitfalls: Ensure updateOneRecord function works correctly and objectMetadataItems are properly fetched and mapped. Thorough testing is recommended.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@@ -4,7 +4,6 @@ import { Link } from 'react-router-dom';

const StyledUndecoratedLink = styled(Link)`
text-decoration: none;
width: 100%;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regression introduced in a recent PR

@@ -89,7 +89,7 @@ export class WorkspaceMigrationRunnerService {

await queryRunner.commitTransaction();
} catch (error) {
this.logger.error('Error executing migration', error);
console.error('Error executing migration', error);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger swallow the errors too much which is a problem to troubleshoot in prod

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if an object is deleted? Should we delete associated webhooks? Should we add a "isDeactivated" bool on the webhook and invalidate it until the user update it? Otherwise LGTM! @charlesBochet

@charlesBochet charlesBochet merged commit 8373dfd into main Aug 5, 2024
8 of 14 checks passed
@charlesBochet charlesBochet deleted the webhooks branch August 5, 2024 21:14
@Weiko
Copy link
Member

Weiko commented Aug 6, 2024

FYI @FelixMalfait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants