Is the build failing because of changes made in this PR, or because of some other non-related issue?
This is a replacement for https://github.com/SUSE/Portus/pull/814 so that somebody else can takeover this feature
I have started to integrate the webhook implementation, this PR is work in progress, I just created this to gather feedback as soon as possible.
Things that still needs to be done: - [x] Backend that triggers the defined webhooks on event - [x] Fix activities for deleted webhooks - [x] Webhook delete button fixed - [x] Listing of webhook deliveries - [x] Retrigger webhook functionality - [x] Write full specs
15条回答 默认 最新
- weixin_39554021 2021-01-07 08:44点赞 评论 复制链接分享
- weixin_39694174 2021-01-07 08:44
that seems related to you :confused:点赞 评论 复制链接分享
OMG it's all green!点赞 评论 复制链接分享
I started reviewing your code but I have to go. I'll continue tomorrow :wink:点赞 评论 复制链接分享
First and foremost, you did a great job, thanks :clap:. That being said, since this is a big feature, there's also some points I'd like you to consider :grin:.
I think that the following actions should only be available to owners and admins: - Create/remove a webhook. - Enable/disable a webhook. - Edit a webhook - Create a new header - Re-trigger a delivery
This should be forbidden in the controller and simply not shown on the client if the current user does not meet the required privileges.
On the UX side: - When you remove a webhook/header there should be a flashy message. The same when you enable/disable them. - The flashy message for creating a header could be more explicit. Instead of "New header created", we could say "Header 'myheader' created successfully". The same applies to flashy messages from other actions (yes, this is something that we should also improve in other pages :sweat_smile:) - There should be more links. For example, on the webhooks index page, there should be a link to the namespace. This could be accomplished in the table header like this "Webhooks for namespace", where "namespace" is a link to the namespace. - There should be a help section in the webhooks index page explaining what are webhooks (similarly to what we do in the namespaces page, even though we could do a better job there...). This also applies for the webhooks show page, in which it would be could have help elements for each table (like we do on the teams show page for example). - In the tables regarding namespaces (see pages namespaces#index and teams#show), there should be a counter of webhooks for each namespace. It would be even better if said counters where in fact a link to the webhooks#index page for that specific namespace.
I created a webhook called
www. When I wanted to rename it, I clicked Save and nothing happened :confused: Then, afterwards I realized that the changes where saved, but in order to get them I had to reload the page. So here I can see two issues: - There is no way for the user to understand what happened (this is something that happens on other actions, as stated previously). - The JS code should handle this, updating the proper fields on update.点赞 评论 复制链接分享
Thanks for the feedback, I have addressed the commented issues. Regarding VCR, I inserted a few
VCR.turn_on!lines because it had reported that VCR was off — don't ask me why. However, they were only inserted where errors were reported.
The UX issues have not been addressed yet.点赞 评论 复制链接分享
On webhooks#show, the name of the webhook is not the actual URL. This is intentional as it's neither pretty nor a good idea to have it read
http://www.example.com:12345/ridiculously/long/path/to/somewhere/ webhook. Instead, it only shows the host part of the URL which would result in
www.example.org webhook.点赞 评论 复制链接分享
sounds reasonable to me.点赞 评论 复制链接分享
Do not merge yet, there seems to be a small bug deleting webhooks when there are headers tied to the object.点赞 评论 复制链接分享
sure. Just ping me whenever I can start reviewing this PR again :wink:点赞 评论 复制链接分享
- When you try to create a new webhook and it fails (e.g. invalid url, or a viewer is trying to do it), there is no alert. The same for editing it, and creating a new header.
- Unauthorized users should not have access to either links (create, edit, etc.) nor forms.
There is this weird issue now:点赞 评论 复制链接分享
The issues have been addressed and everything should work as expected.
Regarding the missing alerts, that maybe also had to do with #883. Furthermore, users don't have access to webhooks, and viewers and contributors cannot create/delete/edit anything related to webhooks. The "weird" issue has been fixed now as well; I hadn't noticed that one before.
Once everyone is satisfied, I'll squash some of my commits.点赞 评论 复制链接分享
LGTM :tada:. Just fix my nitpick and rebase :)点赞 评论 复制链接分享
:tada: :tada:点赞 评论 复制链接分享
- weixin_39898854 2021-01-07 08:45点赞 评论 复制链接分享