Fixes #32363 - Add test button #57
Conversation
|
I like! (but didn't review the code) |
| param :id, :identifier, required: true | ||
| param :payload, String | ||
| def test | ||
| result = @webhook.test(payload: params[:payload]) |
There was a problem hiding this comment.
What tripped me up when I was testing this with curl was that the payload has to be a string. Would it make sense to try dumping it to json in case it is not a string?
| def action_permission | ||
| case params[:action] | ||
| when 'test' | ||
| 'view' |
There was a problem hiding this comment.
I'd probably tighten things a bit and tie this to edit permission to make it a little bit harder to misuse.
ofedoren
force-pushed
the
feat-32363-test-button
branch
from
5273772
to
682e735
Compare
|
Thanks, @adamruzicka, incorporated. |
|
I find this a great improvement already even without linking the template. |
|
Thanks, @ares! @adamruzicka, could you please take a final look? I think we can leave this PR's scope to be able to test a webhook integration with other system, thus a custom payload will do. To test if assigned template can be rendered we will go with |
|
Could we make the modal stick around even on success? Apart from this I don't really have anything to add |
Done. What do you think about allowing to test disabled webhooks? Currently the test button is gray-out if the webhook is disabled. |
It would probably make sense to allow that. I can see myself creating a webhook and keeping it disabled to prevent it from firing on its own until I'm sure I configured everything properly |
ofedoren
force-pushed
the
feat-32363-test-button
branch
from
682e735
to
66e6746
Compare
|
Thanks, @adamruzicka, now it's possible to fire a test even for disabled webhooks. UPD: Forgot to run JS linter, fixed now. |
ofedoren
force-pushed
the
feat-32363-test-button
branch
from
66e6746
to
f0ca99b
Compare
Here is a quick demo with proxy being down/up in case the webhook is configured to use shellhooks:
Although now I think it's better to enable the
testbutton even if a webhook is disabled...UPD: Needs theforeman/foreman#9666. Otherwise it's spamming
in browser's console :/
UPD2: It would be awesome to make test fire use the actually attached template, but it's not possible until #38 is re-implemented. We could try to re-open that approach or create a different. I though about registering object type for event, but it would probably require rewriting the way we register event names :/ Or we could add this alongside, but I'm afraid we would duplicate registration process a bit. The new registration would allow us to register object type for event as well as a custom label for the event instead of automatically generated, which can be quite verbose in case of
Actionbased events.