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

refactor webhook *NewPost #20729

Merged
merged 9 commits into from
Aug 11, 2022
Merged

refactor webhook *NewPost #20729

merged 9 commits into from
Aug 11, 2022

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.

routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
routers/web/repo/webhook.go Outdated Show resolved Hide resolved
@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
services/forms/repo_form.go Outdated Show resolved Hide resolved
services/forms/repo_form.go Outdated Show resolved Hide resolved
@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.

@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.

None yet

8 participants