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

Disable Test Delivery and Replay webhook buttons when webhook is inactive #27211

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

yardenshoham
Copy link
Member

These buttons are now disabled when the webhook is not active.

The buttons were always enabled before this change.

Before

image

image

After

image

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 23, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 23, 2023
…inactive

These buttons are now disabled when the webhook is not active.

Signed-off-by: Yarden Shoham <[email protected]>
@yardenshoham
Copy link
Member Author

@techknowlogick, I tried to do everything in #26814 (comment) but I don't know how to do number 4. Perhaps we can do that in another PR.

{{if .PageIsSettingsHooksEdit}}
<h4 class="ui top attached header">
{{.locale.Tr "repo.settings.recent_deliveries"}}
{{if .Permission.IsAdmin}}
<div class="ui right">
<button class="ui teal tiny button" id="test-delivery" data-tooltip-content="{{.locale.Tr "repo.settings.webhook.test_delivery_desc"}}" data-link="{{.Link}}/test" data-redirect="{{.Link}}">{{.locale.Tr "repo.settings.webhook.test_delivery"}}</button>
<!-- the button is wrapped with a span because the tooltip doesn't show on hover if we put data-tooltip-content directly on the button -->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the tooltip doesn't show on a disabled button so I wrapped it in a span

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is per browser design where disabled elements do not emit mouse events and such:

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled#overview
atomiks/tippyjs#286

Often browsers grey out such controls and it won't receive any browsing events, like mouse clicks or focus-related ones.

Copy link
Member

@silverwind silverwind Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought I see we don't actually set disabled attribute, only disabled class, so it should still work unless the CSS sets pointer-events: none on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yes, I see .ui.disabled.button sets pointer-events: none so that is likely why tippy does not work. I guess a wrapper is a ok-ish solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's even the official solution, apparently.

Copy link
Member

@silverwind silverwind Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a little bit more control because our CSS sets the pointer-events, but I can't recommend enabling pointer-events because that would make the button clickable and it would need additional JS to ensure it's unclickable.

@yardenshoham yardenshoham added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI backport/v1.21 This PR should be backported to Gitea 1.21 labels Sep 23, 2023
@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 23, 2023

Just a note: It could be useful to test an inactive webhook. If you have a very active used repo you may need to finetune/check if the webhook would work correctly without spamming the endpoint with real events.

@yardenshoham
Copy link
Member Author

This PR doesn't change the current behavior, just highlights that gitea as of now doesn't allow test deliveries of inactive webhooks.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 24, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 24, 2023
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 25, 2023
@silverwind silverwind enabled auto-merge (squash) September 25, 2023 06:26
@github-actions github-actions bot removed the topic/ui Change the appearance of the Gitea UI label Sep 25, 2023
@silverwind silverwind merged commit e6d8b14 into go-gitea:main Sep 25, 2023
26 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 25, 2023
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 25, 2023
@yardenshoham yardenshoham deleted the issues/26824 branch September 25, 2023 07:44
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 26, 2023
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Another round of `db.DefaultContext` refactor (go-gitea#27103)
  Fix more "locale" usages (go-gitea#27259)
  Always use `ctx.Locale.Tr` inside templates (go-gitea#27231)
  Disable `Test Delivery` and `Replay` webhook buttons when webhook is inactive (go-gitea#27211)

# Conflicts:
#	templates/base/footer_content.tmpl
#	templates/repo/issue/view_content/context_menu.tmpl
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Delivery for inactive webhook behaves confusing
6 participants