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

Integrations: Add note around webhook data envelope support #2269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

splindsay-92
Copy link

@splindsay-92 splindsay-92 commented Sep 13, 2024

NOTE TO REVIEWERS - please do not review PRs in the DRAFT state, as the PR may change substantially before it is ready to review. Thanks.

Description

When creating a new Webhook integration rule, it wasn't obvious that only message and presence events can be sent as raw payload in webhooks. Trying to do the same with occupancy or lifecycle events would generate an error when trying to create the rule. I have added a small section to the docs to make this clearer, in the hopes we will avoid future confusion.

Review

Instructions on how to review the PR.

@ably-ci ably-ci temporarily deployed to ably-docs-update-webhoo-d39ith September 13, 2024 13:09 Inactive
@mclark-ably
Copy link

mclark-ably commented Sep 13, 2024

It is probably worth mentioning that when creating the rule on the integrations dashboard unselecting enveloped will therefore result in an error

This should have a review from deved team

- This came from `REA-2071`, where it wasn't obvious that only message and presence events can be sent as raw payload in webhooks.
Trying to do the same with occupancy or lifecyle events would generate an error when trying to create the rule.
I have added a small section to the docs to make this more clear, in the hopes we will avoid some confusion.
@m-hulbert
Copy link
Contributor

It is probably worth mentioning that when creating the rule on the integrations dashboard unselecting enveloped will therefore result in an error

This should have a review from deved team

@mclark-ably I think a better UX would be if we disabled the functionality in the dashboard based on which source you select. I've pushed a few minor changes, if everyone is happy with those then I'll merge 🙂

@mclark-ably
Copy link

It is probably worth mentioning that when creating the rule on the integrations dashboard unselecting enveloped will therefore result in an error
This should have a review from deved team

@mclark-ably I think a better UX would be if we disabled the functionality in the dashboard based on which source you select. I've pushed a few minor changes, if everyone is happy with those then I'll merge 🙂

Thanks @m-hulbert , I agree but I don't know what the complexities are around this and would need to be a web team task (see internal comment https://ably-real-time.slack.com/archives/C8SPU4589/p1725986843113469?thread_ts=1725965046.845339&cid=C8SPU4589)

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

Successfully merging this pull request may close these issues.

4 participants