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

feat: Airtable CDP destination, creating records per-event #25656

Merged
merged 17 commits into from
Oct 17, 2024

Conversation

daniloc
Copy link
Contributor

@daniloc daniloc commented Oct 17, 2024

Problem

Allows teams invested in Airtable to integrate events from PostHog into their databases.

Changes

Screenshot 2024-10-17 at 9 32 40 AM Screenshot 2024-10-17 at 9 34 44 AM

Adapts template_webhook with the following changes:

  • Hardcode URL root at https://api.airtable.com/v0/, concatenating path components for base ID and table name
  • Fixed header content to set Content-Type and Authorization fields
  • Structured body to create fields per Airtable API spec and allow typecasting so things like event.timestamp convert tidily to Airtable field types like Date.
  • Use POST method.

Does this work well for both Cloud and self-hosted?

It should not make a difference.

How did you test this code?

Run PostHog locally, using the built-in Destination testing UI to verify it would successfully create records in an Airtable base. It does!

⚠️ ruff/pre-commit linting drama

For neither love nor money could I coerce the pre-commit linting to pass; ruff would fail without output:

[…]
[STARTED] ruff check --fix
[FAILED] ruff check --fix [ENOENT]
[FAILED] ruff check --fix [ENOENT]
[SUCCESS] Running tasks...
[STARTED] Applying modifications...
[SKIPPED] Skipped because of errors from tasks.
[STARTED] Reverting to original state because of errors...
[SUCCESS] Reverting to original state because of errors...
[STARTED] Cleaning up...
[SUCCESS] Cleaning up...

✖ ruff check --fix failed without output (ENOENT).
husky - pre-commit script failed (code 1)

I ran ruff manually against affected files, addressed reported issues, then bypassed pre-commit checks with --no-verify. Apologies if this has introduced any subtle contamination.

@daniloc daniloc changed the title add: Airtable CDP destination, creating records per-event feat: Airtable CDP destination, creating records per-event Oct 17, 2024
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work! 👍

One addition you could still do is to check if the returned request is something other than 200, and then throw an error.

I've seen code like this in other functions:

if (res.status >= 400) {
    throw Error(f'Error from api.attio.com (status {res.status}): {res.body}')
}

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Size Change: 0 B

Total Size: 1.14 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.14 MB

compressed-size-action

@daniloc
Copy link
Contributor Author

daniloc commented Oct 17, 2024

Looks good to me, great work! 👍

One addition you could still do is to check if the returned request is something other than 200, and then throw an error.

I've seen code like this in other functions:

if (res.status >= 400) {
    throw Error(f'Error from api.attio.com (status {res.status}): {res.body}')
}

Thanks for the quick review! Great catch, added:

Screenshot 2024-10-17 at 10 11 44 AM

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daniloc daniloc enabled auto-merge (squash) October 17, 2024 14:23
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daniloc daniloc merged commit c4a87c7 into master Oct 17, 2024
94 checks passed
@daniloc daniloc deleted the add-cdp-airtable-destination branch October 17, 2024 18:04
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