-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
V14: Webhook Management API #15147
V14: Webhook Management API #15147
Conversation
Hi there @bjarnef, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
@Zeegaan @bergmania I have added this while working on umbraco/Umbraco.CMS.Backoffice#962 I think the CRUD operation methods should take a parameter Futhermore should the or maybe It seems to Update method should also return Should we have The API version should probably be changes as well for this :) |
Morning @bjarnef, thanks for the PR! Someone on the team will review this soon 😄 |
@georgebid it definitely need some more work, but it would be great with some feedback from HQ. Maybe it also requires some additional changes regarding webhooks in I know @kjac has worked on the management API, so maybe he could have a look at some point 😎 |
Ah yes, @bjarnef sorry I totally missed that this was marked as a Draft, but yes don't worry - I am sure someone from HQ will be in touch soon! 🚀 |
@elit0451 maybe something you want to review too since you have worked much with the (content) delivery API. |
I think we also need the changes from #15180 but it seems these haven't been merged to |
Should be updated now 😁 |
… feature/management-api-webhook # Conflicts: # src/Umbraco.Core/Services/OperationStatus/WebhookOperationStatus.cs
@Zeegaan I have updated that and resolved the conflicts. I noticed an ambiguous reference because |
… feature/management-api-webhook
… feature/management-api-webhook # Conflicts: # src/Umbraco.Core/Services/LanguageService.cs
I noticed Should the Nevermind, I noticed theres a new method |
It would be great if we could review/merge this, so I can continue the webhooks workspace in backoffice repository 😃 |
@bjarnef We are aware of this, and we have set aside sprint time to review, but we are really busy with all kinds of v14 stuff 🏃♂️ I will get to this as soon as I am able 😁 |
@Zeegaan yeah, I know.. and seems there are lots to do. It is after all a major overhaul and cleanup/refactor of 10 year old code in some parts of the backoffice. and would love to help with the frontend (in backoffice) webhooks workspace, but I neeed to generate the models from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for this taking a long time, finally got around to it 😁
Have a few comments, but overall looks splendid! 💪
src/Umbraco.Cms.Api.Management/Controllers/Webhook/ByKeyWebhookController.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Cms.Api.Management/Controllers/Webhook/ByKeyWebhookController.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Cms.Api.Management/Controllers/Webhook/DeleteWebhookController.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Cms.Api.Management/Controllers/Webhook/CreateWebhookController.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Cms.Api.Management/ViewModels/Webhook/CreateWebhookRequestModel.cs
Outdated
Show resolved
Hide resolved
…kController.cs Co-authored-by: Nikolaj Geisle <[email protected]>
…kController.cs Co-authored-by: Nikolaj Geisle <[email protected]>
…kRequestModel.cs Co-authored-by: Nikolaj Geisle <[email protected]>
Regarding the Maybe the webhook model need some adjustments as well. I haven't checked with the latest changes with was added/adjusted in v13. The webhook inherits |
Yea I think a friendly name at some point would be nice, but I think we should aim for feature parity for now, and then we can always sprinkle stuff on top later 😁 I will give this a test for now, and come back if the models needs adjustment 👍 |
|
||
public class CreateWebhookRequestModel : WebhookModelBase | ||
{ | ||
public Guid? Id { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zeegaan does UpdateWebhookRequestModel
also need this and WebhookResponseModel
?
Should it be part of WebhookModelBase
instead then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No just the CreateModel! 😁
It's basically just so you can specify an Id upon creation 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but we should probably add it to WebhookResponseModel
model as well then - just like with datatype:
Umbraco-CMS/src/Umbraco.Cms.Api.Management/ViewModels/DataType/DataTypeResponseModel.cs
Line 5 in 268babf
public Guid Id { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added Guid
to WebhookResponseModel
:
5005138
I noticed other response models inherits from NamedItemResponseModelBase
which inherits from ItemResponseModelBase
and has Guid Id
.
E.g. MemberGroupItemResponseModel
inherits from NamedItemResponseModelBase
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess WebhookResponseModel
can inherits from NamedItemResponseModelBase
as webhook inherits TEntity
or from ItemResponseModelBase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ItemResponseModelBase
are for the item endpoints, so no more inheritance please 🙈
…okController.cs Co-authored-by: Nikolaj Geisle <[email protected]>
…okController.cs Co-authored-by: Nikolaj Geisle <[email protected]>
@bjarnef I've added some missing mapping and the new auth policies, but works like a charm now 😁 |
@Zeegaan do we need something like this to get a paged result of webhooks? I noticed after generating models in umbraco/Umbraco.CMS.Backoffice#962 that
where
|
Yes seems like it 😁 |
Prerequisites
Description
Added webhooks to Management API and models, which we can generate models for in the new backoffice.
See related PR for new backoffice here: umbraco/Umbraco.CMS.Backoffice#962