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.

Sign up for GitHub

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

Jump to bottom

Allow repo admin to merge PR regardless of review status #9611

Merged
merged 17 commits into from
Jan 11, 2020
Merged

Allow repo admin to merge PR regardless of review status #9611

merged 17 commits into from
Jan 11, 2020

Conversation

davidsvantesson
Copy link
Contributor

  • Allow repo admin to merge even if not all reviews are done/ok. Currently only status checks was possible to bypass
  • Show red button instead of green for admin in this case (see screeshot)
  • API merge endpoint add optional force_merge option which corresponds to above (only for admin)
  • Some refactoring and fixes

Screenshot

image

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 5, 2020
@lafriks lafriks added this to the 1.12.0 milestone Jan 5, 2020
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Jan 5, 2020
@davidsvantesson davidsvantesson changed the title Repo admin merge pr Allow repo admin to merge PR regardless of review status Jan 5, 2020
lafriks
lafriks reviewed Jan 6, 2020
View reviewed changes
routers/private/hook.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 6, 2020
lafriks
lafriks reviewed Jan 6, 2020
View reviewed changes
services/pull/merge.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jan 6, 2020 •
edited

Codecov Report

Merging #9611 into master will increase coverage by <.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9611      +/-   ##
==========================================
+ Coverage   42.21%   42.21%   +<.01%     
==========================================
  Files         590      591       +1     
  Lines       78085    78085              
==========================================
+ Hits        32961    32964       +3     
+ Misses      41077    41075       -2     
+ Partials     4047     4046       -1
Impacted Files Coverage Δ
models/issue_tracked_time.go 58.67% <ø> (-4.18%) ⬇️
modules/convert/issue.go 100% <100%> (ø)
routers/api/v1/repo/issue_tracked_time.go 39.56% <40%> (ø) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️

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 618e5e3...1b75cda. Read the comment docs.

zeripath
zeripath reviewed Jan 6, 2020
View reviewed changes
routers/private/hook.go Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Jan 6, 2020
View reviewed changes
routers/private/hook.go Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Jan 6, 2020
View reviewed changes
routers/private/hook.go Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Jan 6, 2020
View reviewed changes
routers/private/hook.go Outdated Show resolved Hide resolved
@lafriks
Copy link
Member

lafriks commented Jan 7, 2020

@zeripath that is not correct English

@guillep2k
Copy link
Member

@zeripath that is not correct English

@lafriks You mean cannot?

@davidsvantesson
Copy link
Contributor Author

I reformulated the messages instead 😏

lunny
lunny requested changes Jan 8, 2020
View reviewed changes
services/pull/merge.go Outdated Show resolved Hide resolved
@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 Jan 9, 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 Jan 9, 2020
lunny
lunny reviewed Jan 10, 2020
View reviewed changes
services/pull/merge.go
@@ -53,11 +54,6 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
}
prConfig := prUnit.PullRequestsConfig()

if err := pr.CheckUserAllowedToMerge(doer); err != nil {
Copy link
Member

@lunny lunny Jan 10, 2020 •
edited

Choose a reason for hiding this comment

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

So all permissions check are out of this function?

@techknowlogick techknowlogick merged commit 32fb813 into go-gitea:master Jan 11, 2020
@davidsvantesson davidsvantesson deleted the repo-admin-merge-pr branch January 11, 2020 10:27
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath left review comments

@guillep2k guillep2k guillep2k left review comments

@bagasme bagasme bagasme left review comments

@lunny lunny lunny approved these changes

@lafriks lafriks lafriks approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet