everypolitician / webhook-manager

Notifies other apps whenever EveryPolitician data changes, in style of GitHub's webhooks
https://everypolitician-app-manager.herokuapp.com/
1 stars 2 forks source link

Check for blank webhook names #69

Closed davewhiteland closed 8 years ago

davewhiteland commented 8 years ago

We're trapping empty names on creation, but if you subsequently edit the entry and remove the name, blammo Internal Server Error.

Screenshot for morale-boost showing we're capturing this OK on creation which is why I had to edit it to hit the error:

Screenshot shows this error being safely trapped at webhook-creation time (so this isn't the error, but presumably the remedy will look like this when an attempt to update a webhook with a blank name is made):

screen shot 2016-07-28 at 11 00 18

chrismytton commented 8 years ago

I think the real issue is do we actually want or need the webhooks to have names? This is another hangover from ye olde times when this was for creating apps, rather than webhooks.

I say we just remove the name field from webhooks altogether.

chrismytton commented 8 years ago

Hmm, on the other hand the name field currently acts as a "note" field. If we displayed it alongside the url then it might be a bit easier to comprehend than just a big list of urls.

chrismytton commented 8 years ago

Worth noting that this error also occurs if you try and delete the webhook's url, so whether we keep the name field or not, this error needs fixing.

davewhiteland commented 8 years ago

Yeah I think the name is handy, because we can't know that the URL of the app is expressive enough to usefully distinguish to the-owner-of-a-github-account (might not be a single individual?) which/why the webhook was created, or even what the app is doing.

Since it's already there, I vote for keeping it.