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: Blacklisting webhooks #17421

Merged
merged 2 commits into from
Sep 14, 2023
Merged

feat: Blacklisting webhooks #17421

merged 2 commits into from
Sep 14, 2023

Conversation

tiina303
Copy link
Contributor

@tiina303 tiina303 commented Sep 13, 2023

Problem

We've disabled a webhook for the same team twice due to it being really slow and contacted them about it. This will allow us to disable webhooks for the team until they talk to us.

context: https://posthog.slack.com/archives/C0185UNBSJZ/p1694611747313189?thread_ts=1694610989.528059&cid=C0185UNBSJZ

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

ran locally looked at the page first, enabled the feature flag, then it looked like this:
Screenshot 2023-09-13 at 18 13 05

@tiina303 tiina303 requested review from timgl and a team September 13, 2023 16:18
@@ -166,6 +166,7 @@ export const FEATURE_FLAGS = {
// owner: #team-monitoring
SESSION_RECORDING_ALLOW_V1_SNAPSHOTS: 'session-recording-allow-v1-snapshots',
HOGQL_INSIGHTS: 'hogql-insights', // owner: @mariusandra
WEBHOOKS_BLACKLISTED: 'webhooks-blacklisted', // owner: @team-pipeline
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense calling this a denylist


useEffect(() => {
if (currentTeam?.slack_incoming_webhook) {
setWebhook(currentTeam?.slack_incoming_webhook)
}
}, [currentTeam])

const webhooks_blacklisted = featureFlags[FEATURE_FLAGS.WEBHOOKS_DENYLIST]
if (webhooks_blacklisted) {
Copy link
Contributor

Choose a reason for hiding this comment

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

related nit: probably good to align the variable name with the feature flag (or just if (featureFlags[FEATURE_FLAGS.WEBHOOKS_DENYLIST])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged cause I didn't want to wait for tests again 😅

@tiina303 tiina303 merged commit 3749142 into master Sep 14, 2023
63 checks passed
@tiina303 tiina303 deleted the blacklisting-webhooks branch September 14, 2023 10:50
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