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

bug fix webhook response not sending data to tinybird #7952

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

anamarn
Copy link
Contributor

@anamarn anamarn commented Oct 22, 2024

Solves https://github.com/twentyhq/private-issues/issues/118

TLDR

Fix webhook response not sending data to tinybird when the url is not a link.

Changes in Tinybird:

  • Add column Success to webhook payload (boolean)
  • Changed the parameter WebhookIdRequest to WebhookId in the getWebhooksResponse api point.
  • Those changes can be seen in the tinybird workspace twenty_analytics_playground

In order to test

  1. Set ANALYTICS_ENABLED to true
  2. Set TINYBIRD_INGEST_TOKEN to your token from the workspace twenty_analytics_playground
  3. Set TINYBIRD_GENERATE_JWT_TOKEN to the admin kwt token from the workspace twenty_analytics_playground
  4. Set TINYBIRD_WORKSPACE_UUID to the UUID of twenty_analytics_playground
  5. Create a Webhook in twenty and set wich events it needs to track
  6. Run twenty-worker in order to make the webhooks work.
  7. Do your tasks in order to populate the data
  8. Look at your webhooks in settings>api and webhooks> your webhook and the statistics should be displayed

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 addresses a bug in webhook response handling, ensuring proper data transmission to Tinybird for analytics.

  • Updated fetchGraphDataOrThrow.ts to use 'webhookId' instead of 'webhookIdRequest' in API calls
  • Modified call-webhook.job.ts to include a 'success' boolean field in the analytics payload
  • Improved error handling in call-webhook.job.ts to conditionally include status code for failed requests
  • These changes enhance webhook analytics data quality and accuracy in Tinybird

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

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.

Left some nit comments but LGTM!

@@ -14,7 +14,7 @@ export const fetchGraphDataOrThrow = async ({
}: fetchGraphDataOrThrowProps) => {
const queryString = new URLSearchParams({
...WEBHOOK_GRAPH_API_OPTIONS_MAP[windowLength],
webhookIdRequest: webhookId,
webhookId: webhookId,
Copy link
Member

Choose a reason for hiding this comment

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

redundant

@@ -46,10 +47,11 @@ export class CallWebhookJob {
const eventInput = {
Copy link
Member

Choose a reason for hiding this comment

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

This part seems a bit redundant as well, shouldn't success be dependant on the response status / call failure? The rest seems very similar so I think it could be refactored a bit

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Thank you!

@anamarn anamarn merged commit 18cfe79 into main Oct 22, 2024
18 checks passed
@anamarn anamarn deleted the bug/analytics-webhooks branch October 22, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants