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 *EditPost #20780

Merged
merged 4 commits into from
Aug 23, 2022
Merged

refactor webhook *EditPost #20780

merged 4 commits into from
Aug 23, 2022

Conversation

oliverpool
Copy link
Contributor

Following #20729 this PR aims at reducing duplicated code in the *HooksEditPost functions.

I also moved the functions around, so that webhook types are grouped together, which makes it easier to read.

For instance the matrix endpoints is now simply (notice that fmt.Sprintf is not duplicated anymore!):


// MatrixHooksNewPost response for creating Matrix webhook
func MatrixHooksNewPost(ctx *context.Context) {
	createWebhook(ctx, matrixHookParams(ctx))
}

// MatrixHooksEditPost response for editing Matrix webhook
func MatrixHooksEditPost(ctx *context.Context) {
	editWebhook(ctx, matrixHookParams(ctx))
}

func matrixHookParams(ctx *context.Context) webhookParams {
	form := web.GetForm(ctx).(*forms.NewMatrixHookForm)

	return webhookParams{
		URL:         fmt.Sprintf("%s/_matrix/client/r0/rooms/%s/send/m.room.message", form.HomeserverURL, url.PathEscape(form.RoomID)),
		ContentType: webhook.ContentTypeJSON,
		HTTPMethod:  http.MethodPut,
		WebhookForm: form.WebhookForm,
		Type:        webhook.MATRIX,
		Meta: &webhook_service.MatrixMeta{
			HomeserverURL: form.HomeserverURL,
			Room:          form.RoomID,
			AccessToken:   form.AccessToken,
			MessageType:   form.MessageType,
		},
	}
}

@Gusted Gusted added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Aug 14, 2022
@Gusted Gusted added this to the 1.18.0 milestone Aug 14, 2022
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Certainly looks far cleaner from what I can see.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 19, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I think there's more that can be done here with a register block.

But this is still a good improvement.

@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 20, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 23, 2022

make L-G-T-M work

@techknowlogick
Copy link
Member

Ping lg-tm bot

@techknowlogick techknowlogick merged commit 0a9ed54 into go-gitea:main Aug 23, 2022
@oliverpool oliverpool deleted the refactor_webhook_edit_post branch August 23, 2022 08:49
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 24, 2022
* giteaofficial/main:
  Display total blob size of a package version. (go-gitea#20927)
  Display all files. (go-gitea#20917)
  Enhance package date display (go-gitea#20928)
  [skip ci] Updated translations via Crowdin
  Forbid HTML string tooltips (go-gitea#20935)
  Set no-tags in git fetch on compare (go-gitea#20893)
  Update comparison.en-us.md: link subgroups issue (go-gitea#20905)
  Enable contenthash in filename for dynamic assets (go-gitea#20813)
  refactor webhook *EditPost (go-gitea#20780)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 2022
@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