-
Notifications
You must be signed in to change notification settings - Fork 171
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
Apidocs webhooks fixes #5828
base: py3
Are you sure you want to change the base?
Apidocs webhooks fixes #5828
Conversation
…uttons to an 'authenticated' variant of the page which is not cached
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## py3 #5828 +/- ##
==========================================
- Coverage 93.10% 92.92% -0.19%
==========================================
Files 658 658
Lines 42145 42576 +431
Branches 60 60
==========================================
+ Hits 39241 39562 +321
- Misses 2896 3006 +110
Partials 8 8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking cool! Pulled and looked at this locally. Let's try to get the errors fixed up and we can work on landing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get tests for these changes?
Description
For a while (at least a year), sending test webhook notifications has been broken. I discovered today that it was caused by the page caching, wherein the CSRF token rendered into the page belonged to whoever happened to load the page at the time it was re-rendered and cached, and would eventually become stale and invalid. I discussed with @ZachOrr and we came to the conclusion that the testing functions should be placed behind a privileged page. I gave the cached variant of the page a "Log in to send test notifications" button, which takes you to the authenticated variant of the page which has cache disabled, allowing the CSRF token being rendered to be accurate/up-to-date, fixing the 400 CSRF token expired error.
Second, I improved the error handling. If there's a user error involving an incorrect or not-specified key, the alert div below the button will show the specific error. And if there's an unexpected error (e.g. network error, or, god forbid, a CSRF issue), it'll display "Something went horribly wrong. Refresh the page and try again." in the same box instead of with an
alert()
.Motivation and Context
Webhook test notifications ain't workin!
How Has This Been Tested?
I tested the error handling functionality by putting in incorrect or empty keys when sending test notifications. The CSRF token stuff works fine in my environment, but unfortunately, the webhook notifications don't actually send from my development instance. I haven't modified any of the code responsible for the webhook sending code, though, so I suspect it'll work in production. But it'll have to be tested in the production environment just to be sure. However, since webhook test notifications have been broken for over a year, the risk involved in the possibility webhook test notifications not working with this code change is zero.
Types of changes