tcplugins / tcWebHooks

WebHooks plugin for Teamcity. Supports many build states and payload formats.
https://netwolfuk.wordpress.com/category/teamcity/tcplugins/tcwebhooks/
157 stars 30 forks source link

Webhook that only allows Mute/Unmute events should not allow other events to be selected #221

Open matt-richardson opened 1 year ago

matt-richardson commented 1 year ago

(originally raised over here)

Expected Behavior

A webhook that only allows Mute/Unmute events, eg: image

should not allow other events to be selected.

Current Behavior

The webhook is showing all the boxes as ticked, but disabled: image

Errors show as

[2023-07-18 02:16:17,787] ERROR [@7438424f'; Normal executor 28] - jetbrains.buildServer.SERVER - AbstractWebHookExecutor :: trackingId: dadfab32-86b1-4bab-8c1a-afd38e6870e4 :: projectId: OctopusDeploy_OctopusServer_CorePlatform :: webhookId: id_905369674 :: templateId: buildmonitormutes, errorCode: 902, errorMessage: Template 'buildmonitormutes' does not support build state 'beforeBuildFinish'

Steps to Reproduce (for bugs)

It appears that this happens when you select a template that allows build events (which auto-ticks them), and then change to one that only allows mute events - the fields get disabled but are still ticked.

Workaround

Selecting a different template (that supports build events), de-selecting the build events, then changing back to the mute/unmute template works.

Your Environment

netwolfuk commented 1 year ago

Several years ago, I made a conscious decision to not clear the checked items when one selects a template that has less supported events.

I decided that because if one changes back to the original template, one as to remember what events were previously selected because that information is lost.

Since then, the logic in that edit webhook dialog has been completely re-written and is much more advanced. It is also Jquery unit tested!

I should be able to support one of the following scenarios:

  1. Store (inside state in the webhook dialog), the original webhook config. When the user changes to a new template, only check the events that were previously checked and are supported by the new template. If the user changes to another template, do the same using the original selected events and so on. If the user changes back to the original template, the original events should be re-instated.

    However, what logic is applied if the user deliberately checks or un-checks an event and then changes template?

  2. Validate the available build events on the server side when the webhook is saved. Only enable the ones that have an applicable event available in the template.

I'm leaning towards the first option, because that at least shows to the user what the intended configuration will be when they click the save button. Option two is easier to code ;-)

netwolfuk commented 1 year ago

However, what logic is applied if the user deliberately checks or un-checks an event and then changes template?

Thinking about that, I can handle that case too. If a user intentionally modifies the set of checked/unchecked events, then save that to the stored state.

Any further changes to template will honour the same rules as above, but the state will have been updated to reflect the user action.

If the user wants to completely back out, they can just press Cancel and no changes will be made.

It does reveal an interesting bug. What should the REST API or Controller logic do when a user selected a build event that the template does not support?

Obviously at the moment, it silently ignores that mis-match and allows the update to be written. This is why you can see these errors being thrown in the log.

I feel like an invalid update (PUT) should return 400 bad request when the list of requested events are not supported by the template.

netwolfuk commented 1 year ago

@matt-richardson Which page URL are you using to edit webhooks?

The index page with the blurb on the right, or the edit project page?

matt-richardson commented 1 year ago

A third option - you could inform the user "new template doesn't support x, y, and z - these have been de-selected."

A fourth option could be to do the same, but also have a "revert to previous template" option that puts you back in the previous state.

In terms of the REST API / Controller - I think it should return a bad request as you suggest 👍

@matt-richardson Which page URL are you using to edit webhooks?

The index page with the blurb on the right, or the edit project page?

I was using the edit project page.