Skip to content

Move helper methods for adding webhooks to extension methods#15344

Merged
Zeegaan merged 3 commits intov13/devfrom
v13/feature/webhook-extensions
Dec 4, 2023
Merged

Move helper methods for adding webhooks to extension methods#15344
Zeegaan merged 3 commits intov13/devfrom
v13/feature/webhook-extensions

Conversation

@ronaldbarendse
Copy link
Copy Markdown
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

This moves the helper methods for adding webhooks that were introduced in PR #15161 to extension methods, so the WebhookEventCollectionBuilder class only contains the required logic for configuring the collection. I've also cleaned up this class, specifically by using TryAddEnumerable() to add the webhook event/notification type to the service collection and only calling TryGetGenericArguments() once (IsOfGenericType() is doing the same, but without returning the generic types).

I've also renamed AddAllAvailableWebhooks() to AddCmsWebhooks(), because 'all available' could suggest it does a type scan of all IWebhookEvent types and adds those, but instead it only adds the CMS supported ones.

Another thing I noticed is the lack of DocumentType* events: do we want to add those as well? And if PR #15337 gets merged, we should ensure the AddHealthCheckWebhooks() helper methods gets moved into an extension method too 😄

Testing can be done by adding the following composer, which removes the default events and adds all content ones instead:

using Umbraco.Cms.Core.Composing;

public class WebhookComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
        => builder.WebhookEvents()
            .Clear()
            .AddContentWebhooks();
}

Or if you want all CMS supported ones added (ignoring the duplicate default ones), try:

using Umbraco.Cms.Core.Composing;

public class WebhookComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
        => builder.WebhookEvents().AddCmsWebhooks();
}

I'd also recommend a sanity check to see whether the webhooks get fired, since this does contain a change in the registration of the notification handlers.

@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented Dec 4, 2023

I agree AddCmsWebhooks() is a better name, I considered AddCoreWebhooks(), but I think Cms is more clear - I guess there could be a AddFormWebhooks() in Umbraco Forms for instance.

@ronaldbarendse
Copy link
Copy Markdown
Contributor Author

I've implemented the document type webhook events and the applied the comments about grouping in PR #15345, which adds a way nicer fluent API 😄 Lets continue the discussion on that PR instead!

@Zeegaan
Copy link
Copy Markdown
Member

Zeegaan commented Dec 4, 2023

LGTM 🚀

@Zeegaan Zeegaan merged commit da56f16 into v13/dev Dec 4, 2023
@Zeegaan Zeegaan deleted the v13/feature/webhook-extensions branch December 4, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants