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

Added deliveries workspace view to the webhook details workspace #18175

Merged
merged 40 commits into from
Feb 4, 2025

Conversation

AndyButland
Copy link
Contributor

Prerequisites

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

Resolves: #17754

Description

This PR adds an additional workspace view to the webhook details workspace, showing the deliveries for that webhook.

I've amended this slightly from 13 as it seemed to make more sense to show deliveries for each webhook as a separate paged list, rather than one long list for all of them.

image

To Test:

  • Create one or more webhooks posting to a destination like https://webhook.site/
  • Trigger a few webhooks by hooking them up to publish or other events
  • View and page the deliveries for the each webhook.

@madsrasmussen madsrasmussen self-assigned this Jan 31, 2025
@madsrasmussen
Copy link
Contributor

Hi Andy, First of all, everything worked as expected in this PR.

As you have already noticed, I have added a couple of updates to the PR. This was the perfect opportunity to get this feature aligned with some of the rest of the UI/code base.

I have fixed a couple of smaller hiccups in your additions:

  • I removed double pagination in the delivery collection since a collection already comes with pagination out of the box.
  • I moved a dependency on the workspace context from the webhook delivery repository to the workspace view.
  • Use a uui-tag for the delivery status code.

Here are the extra changes I made:

  • Updated the file structure for the entire webhook feature into feature folders.
  • Fixed broken pagination on the webhooks collection.
  • Aligned the UX of the webhook workspace with languages. Webhooks don't have a name, so we now show the URL as the first column of the collection table instead of a hardcoded "Webhook {index}" name.
  • Displayed the webhook URL in the workspace header. This change allowed us to remove the URL column in the deliveries collection table, creating more space for other information.
  • Used a uui-tag for the enabled/disabled state instead of a checkmark.

Everything should work the exact same as you left it. Can I get you to do a sanity check of the feature?

A couple of screenshots

Webhooks Collection
Screenshot 2025-02-03 at 14 21 11

Webhook Detail Workspace View
Screenshot 2025-02-03 at 14 21 22

Webhook Deliveries Workspace View
Screenshot 2025-02-03 at 14 21 31

@AndyButland
Copy link
Contributor Author

Thanks Mads, great work. As I said... hopefully there was something you could work from where I left it! I'll check it out locally as you say.

Just quickly though:

Displayed the webhook URL in the workspace header. This change allowed us to remove the URL column in the deliveries collection table, creating more space for other information.

I also had this in an earlier version... but then I thought, the URL could change. I.e. you can create a webhook, start using it, and then change the URL to a different one. So that's why in the end I had it in the column list. Maybe it's not that likely, but seems like it has been anticipated in that the URL is stored for every delivery record in the umbracoWebhookLog table.

@AndyButland
Copy link
Contributor Author

Otherwise all working as expected for me.

@Zeegaan - please could you have a quick look over the back-end aspects of this PR? And maybe you have a view on my comment above from when you were designing the data model for this. Thanks.

@madsrasmussen
Copy link
Contributor

madsrasmussen commented Feb 3, 2025

@AndyButland Ah, it's a good point. If that is the case then I think it is even more important we get a name as part of the Webhook model at some point.

@Zeegaan
Copy link
Member

Zeegaan commented Feb 3, 2025

@AndyButland Looks good to me.
And as for the Name thing.. We probably should give it a friendly name, this feature has actually been discussed before: #15158

I think ultimately we could have a Nullable name, and then default back to the URL, if you don't want to give it a friendly name 👍

@madsrasmussen
Copy link
Contributor

@AndyButland Url is back for deliveries again 👍

Let me know what will happen with the friendly name. It would be awesome to add in the near future 😃

Feel free to merge when happy.

@AndyButland AndyButland merged commit 1c68e3d into v15/dev Feb 4, 2025
29 checks passed
@AndyButland AndyButland deleted the v15/feature/webhook-deliveries-workspace-view branch February 4, 2025 05:36
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