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

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

Open
matt-richardson opened this issue Jul 18, 2023 · 4 comments

Comments

@matt-richardson
Copy link
Contributor

(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

  • tcWebHooks Version: 1.2.3
  • TeamCity Version: 2023.05.1
  • TeamCity server Operating System: Linux (ubuntu)
  • Are you using a WebHook Template?: Yes
@netwolfuk
Copy link
Member

netwolfuk commented Jul 18, 2023

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants