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

Set correct content_type value for Gogs/Gitea webhooks (#9504) #10456

Merged
merged 2 commits into from
Feb 28, 2020
Merged

Set correct content_type value for Gogs/Gitea webhooks (#9504) #10456

merged 2 commits into from
Feb 28, 2020

Conversation

jvstein
Copy link
Contributor

@jvstein jvstein commented Feb 25, 2020

The content_type value was defaulting to the string value of the ContentType, not the integer value as expected by the backend.

This addresses the problem seen in #9504.

Test steps:

  1. Create a new repo.
  2. Under Settings -> Webohooks click "Add Webhook", then "Gogs".
  3. Type a URL and click "Add Webhook"

Before change: The form will fail with "ContentType cannot be empty".
After change: The form will succeed and the webhook will be created.

The content_type value was defaulting to the string value of the
ContentType, not the integer value as expected by the backend.
@jvstein jvstein requested review from lunny and DblK February 25, 2020 06:16
</div>
{{template "repo/settings/webhook/settings" .}}
</form>
{{end}}
Copy link
Contributor Author

@jvstein jvstein Feb 25, 2020

Choose a reason for hiding this comment

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

My editor respected the .editorconfig and reformatted the file with LF line endings. I can rework this to a minimal diff.

Copy link
Member

Choose a reason for hiding this comment

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

Reminds me we still need to reformat the codebase to conform .editorconfig. It will be a gigantic diff 😆

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2020
@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #10456 into master will increase coverage by <.01%.
The diff coverage is 45.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10456      +/-   ##
==========================================
+ Coverage   43.67%   43.68%   +<.01%     
==========================================
  Files         586      586              
  Lines       81391    81521     +130     
==========================================
+ Hits        35551    35611      +60     
- Misses      41438    41495      +57     
- Partials     4402     4415      +13
Impacted Files Coverage Δ
routers/private/hook.go 36.6% <ø> (+0.17%) ⬆️
routers/api/v1/repo/pull.go 34.63% <ø> (-0.19%) ⬇️
modules/setting/setting.go 44.07% <ø> (ø) ⬆️
modules/auth/admin.go 0% <0%> (ø) ⬆️
routers/admin/admin.go 10.29% <0%> (-0.15%) ⬇️
modules/lfs/server.go 45.14% <0%> (-0.56%) ⬇️
routers/api/v1/repo/issue_subscription.go 0% <0%> (ø) ⬆️
routers/repo/wiki.go 39.58% <0%> (ø) ⬆️
models/repo_collaboration.go 57.05% <0%> (-6.53%) ⬇️
modules/setting/database.go 57.28% <0%> (ø) ⬆️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8767842...3f1b044. Read the comment docs.

@lunny
Copy link
Member

lunny commented Feb 25, 2020

Maybe we should change the backend for better readability.

@lafriks lafriks added this to the 1.12.0 milestone Feb 25, 2020
<div class="field">
<label>{{.i18n.Tr "repo.settings.content_type"}}</label>
<div class="ui selection dropdown">
<input type="hidden" id="content_type" name="content_type" value="{{if .Webhook.ContentType}}{{.Webhook.ContentType}}{{else}}1{{end}}">
Copy link
Member

Choose a reason for hiding this comment

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

why change ContentType for gitea too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same problem occurs with the Gitea webhook if you don't select a ContentType from the drop down. The intention of the template seemed to be to populate a working value in the form as a default, but that's not what's happening.

screenshot-20200225-142005

@jolheiser jolheiser mentioned this pull request Feb 26, 2020
7 tasks
@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 Feb 28, 2020
@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 Feb 28, 2020
@zeripath zeripath merged commit e661cd0 into go-gitea:master Feb 28, 2020
@mrsdizzie mrsdizzie mentioned this pull request May 17, 2020
7 tasks
@lafriks
Copy link
Member

lafriks commented May 17, 2020

Please backport

zeripath pushed a commit to zeripath/gitea that referenced this pull request May 17, 2020
go-gitea#10456)

The content_type value was defaulting to the string value of the
ContentType, not the integer value as expected by the backend.
@zeripath zeripath added the backport/done All backports for this PR have been created label May 17, 2020
zeripath added a commit that referenced this pull request May 17, 2020
… (#11461)

The content_type value was defaulting to the string value of the
ContentType, not the integer value as expected by the backend.

Co-authored-by: Jeff Stein <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants