Skip to content

Conversation

oliverpool
Copy link
Contributor

Hi, this is a bit of yak-shaving: I wanted to add an Authorization Header to the webhooks. While working around the forms, I saw that the same code was repeated 11 times, I thought that it would be a good idea to refactor it before modifying it.

So here is a PR to refactor all *NewPost functions into a createWebhook function.

I did some minor modifications to Slack error handling, so that it doesn't need special code.

I am looking forward to your feedback!

@oliverpool
Copy link
Contributor Author

The Slack error message looks like this with this PR:

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 9, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@fba2055). Click here to learn what that means.
The diff coverage is 0.00%.

❗ Current head fd06f07 differs from pull request most recent head 814a8b6. Consider uploading reports for the commit 814a8b6 to get more accurate results

@@           Coverage Diff           @@
##             main   #20729   +/-   ##
=======================================
  Coverage        ?   47.03%           
=======================================
  Files           ?      980           
  Lines           ?   136070           
  Branches        ?        0           
=======================================
  Hits            ?    64006           
  Misses          ?    64215           
  Partials        ?     7849           
Impacted Files Coverage Δ
modules/web/middleware/binding.go 61.05% <0.00%> (ø)
routers/web/repo/webhook.go 1.33% <0.00%> (ø)
services/forms/repo_form.go 39.02% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

I would remove the properties with default values but that's just my personal preference.

@oliverpool
Copy link
Contributor Author

@KN4CK3R good point, done in 8eec61f

wxiaoguang
wxiaoguang previously approved these changes Aug 9, 2022
@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 Aug 9, 2022
@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Aug 9, 2022
@6543 6543 added this to the 1.18.0 milestone Aug 9, 2022
@wxiaoguang
Copy link
Contributor

Wait , there seems to be 2 func isValidSlackChannel in code now. I will discard my early approval and wait until the PR becomes stable.

@wxiaoguang wxiaoguang dismissed their stale review August 9, 2022 11:37

Wait for stable

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 9, 2022
@oliverpool
Copy link
Contributor Author

@6543 since you advised removing, I actually removed utils.IsValidSlackChannel 😄

  • its logic wasn't correct ( # was considered valid, since the strings.TrimSpace was not applied on if channelName[0] == '#'
  • it makes the function much much shorter to let the caller take of strings.TrimSpace (which is needed anyway for saving in the database)

Since this is now a one-line function, I think copying is better than importing in this case:

func isValidSlackChannel(name string) bool {
	return name != "" && name != "#"
}

@oliverpool
Copy link
Contributor Author

@wxiaoguang you are right. Let me move the IsValidSlackChannel function to slack.go, so that it is only in one place.

@silverwind
Copy link
Member

Webhook Auth header is wip at #20267, by the way.

@oliverpool oliverpool requested a review from zeripath August 9, 2022 20:53
@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 9, 2022
@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 Aug 11, 2022
@6543
Copy link
Member

6543 commented Aug 11, 2022

.

@6543 6543 merged commit c81b26b into go-gitea:main Aug 11, 2022
@oliverpool oliverpool deleted the refactor_webhook_new_post branch August 11, 2022 16:07
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 2022
* refactor webhook *NewPost

* remove empty values

* always show errs.Message

* remove utils.IsValidSlackChannel

* move IsValidSlackChannel to services/webhook package

* binding: handle empty Message case

* make IsValidSlackChannel more strict
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants