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

Feature: Webhooks workspace #962

Merged
merged 51 commits into from
Feb 29, 2024

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Nov 3, 2023

Description

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

Added new webhooks workspace, which has been added to Umbraco 13.
I used anchor icon for now, but we can update this to a more webhook specific icon when Lucide icons are included.
umbraco/Umbraco.UI#629

How to test?

Screenshots (if appropriate)

image

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@bjarnef bjarnef changed the title Feature/webhooks workspace Feature: Webhooks workspace Nov 5, 2023
@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 5, 2023

I also fixed some typos in logviewer.

I noticed a few things I found inconsistent:

  • Some root nodes are routing to e.g. /workspace/logviewer while others are /workspace/language-root and /workspace/extension-root ... can't we skip the -root suffix?

  • In handlers some are named in plural, but most in singular.

  • Any specific reason LogViewer is exported as a package Umb.Bundle.LogViewer, but e.g. Languages is not or part of the Umb.Bundle.Settings package I think?

@bjarnef bjarnef marked this pull request as ready for review November 5, 2023 19:28
@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 5, 2023

@iOvergaard I think the most basic stuff has been added, but comparing to the log viewer tree, I can't seem to figure out what's is missing to show the overview/webhooks and logs apps?

/section/settings/workspace/logviewer route to /section/settings/workspace/logviewer/view/overview, where does this happen? :)

Update: It seems this part did it. 4a39b06

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 5, 2023

I prepared some table data in 740aac2

But I don't how it generates the models in here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/tree/main/src/external/backend-api/src/models

e.g. LanguageResponseModel except it seems to use OpenAPI Generator to generate the models, but are there any schemas defining the models or endpoint to fetch the models from?

package.json Outdated Show resolved Hide resolved
@nielslyngsoe
Copy link
Member

@bjarnef just for clarification. The PR of UI Library that you reference is just changing the essential icons of UI Library to use Lucide. So not bringing any new icons.

But there is abilities to get the webhook icon into v.14.
The PR below enables you to append new icons from Lucide.
The PR below itself does only include a few new icons from Lucide, the rest of the PR is about upgrading existing icons.
We should never include all icons of Lucide, I would say we have to be opinionated about what we include from it.
Here is the draft PR on that:
#963

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 6, 2023

@nielslyngsoe in umbraco/Umbraco-CMS#15077 we have include a webhook SVG icon in the current backoffice for now.

But for the new backoffice should this be an essential icon (from Lucide icons) then?

I have noticed when opening the icon picker in the new backoffice some icons are shown right away (it is probably) the essential icons, while the rest appears a bit delayed.

@nielslyngsoe
Copy link
Member

@bjarnef correct.

Good we talked about this, I didn't know about the new Icon. so that should be ported for New backoffice. And then ideally use the Lucide version. (Thought I personally find that one a bit hard to decode visually, but minor detail in the bigger picture)

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 6, 2023

@nielslyngsoe
Copy link
Member

@bjarnef I ported it now. I did though add the Lucide one. But I do find the one you picked better, as it symbolizes connecting the dots. Where the Lucide one, If I didn't know what it was supposed to symbolize, then I would not know what it should symbolize. So ideally I would make a change to that icon one day so It has some dots in it.

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 6, 2023

@nielslyngsoe regarding this PR, how are the models generated, e.g. LanguageResponseModel.
How can we add e.g. a WebhookResponseModel with some mock data for now like languages?

I also found some inconsistency in naming conventions, where some filenames or object names are in singular form others in plural form :)

and do we need the -root part in route? Some tree nodes use it, but not consistent :)

@iOvergaard
Copy link
Collaborator

@nielslyngsoe regarding this PR, how are the models generated, e.g. LanguageResponseModel. How can we add e.g. a WebhookResponseModel with some mock data for now like languages?

I also found some inconsistency in naming conventions, where some filenames or object names are in singular form others in plural form :)

and do we need the -root part in route? Some tree nodes use it, but not consistent :)

You need to run npm run generate:api to generate those models. But they need to exist on the v14/dev branch on umbraco-cms first, which they probably don't. According to the Core team, the new controllers for webhooks have been written with the Management API in mind, so it should be fairly easy to migrate. Do you think you could take a stab at that?

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 6, 2023

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 6, 2023

Okay it generates a lot of models, but most changes just formatting in the models.

However I don't get any webhook models.

I guess it is because there is no webhook specific endpoint here in the Management API?
https://raw.githubusercontent.com/umbraco/Umbraco-CMS/v14/dev/src/Umbraco.Cms.Api.Management/OpenApi.json

@Zeegaan should there be endpoints to create/read/update/delete webhooks via Management API?

bjarnef and others added 14 commits February 27, 2024 10:28
… feature/webhooks-workspace

# Conflicts:
#	src/external/backend-api/src/models/CreateWebhookRequestModel.ts
#	src/external/backend-api/src/models/UpdateWebhookRequestModel.ts
#	src/external/backend-api/src/models/WebhookEventResponseModel.ts
#	src/external/backend-api/src/models/WebhookItemResponseModel.ts
#	src/external/backend-api/src/models/WebhookModelBaseModel.ts
#	src/external/backend-api/src/models/WebhookResponseModel.ts
#	src/external/backend-api/src/services/WebhookResource.ts
@iOvergaard
Copy link
Collaborator

Hi @bjarnef
I took the liberty to try and align this new workspace a bit with the existing workspaces. We now have a workspace editor, that shows an overview with a collection, and a dummy "Logs" view that happens to show the same collection because no element exists for this yet.

image

What is needed is the create/edit view as well as the logs view now. Would you like to look more into that?

@bjarnef
Copy link
Contributor Author

bjarnef commented Feb 29, 2024

Hi @iOvergaard

Thanks for fixing all of this 😅
There had been quite a lot of changes from the previous progress, so it needed to be aligned with the recent changes 😎

I could have a look at that - maybe in a new PR then?

I think we also need a paged results for AllWebhookController when I compared with lanauges as the generated WebhookResource was missing that method - I asked @Zeegaan about that in:
umbraco/Umbraco-CMS#15147 (comment)

@iOvergaard
Copy link
Collaborator

@bjarnef Yes, I think we need that method. It's kind of a hack in the data source at the moment and the "filter" property with "skip" and "take" is not being used at all. I think currently it would just show all webhooks registered.

I'll just make sure this PR can build, then we'll merge it, and you can look into the other things. Does that sound ok?

@bjarnef
Copy link
Contributor Author

bjarnef commented Feb 29, 2024

Yes, that would be great. We can always continue the work and adjust it further.

@iOvergaard iOvergaard enabled auto-merge February 29, 2024 12:39
@iOvergaard iOvergaard merged commit 195407d into umbraco:main Feb 29, 2024
6 checks passed
@bjarnef bjarnef deleted the feature/webhooks-workspace branch February 29, 2024 14:12
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.

4 participants