pull-requests: unified merge checks.
- same logic is now used for API, pre-conditions on show, and checks on actual merge - added checks into API because it was not validated
This commit is contained in:
Marcin Kuzminski
parent
e5f1efc7f7
commit
30637758d6
6 changed files with 137 additions and 79 deletions
|
|
@ -32,7 +32,7 @@ from rhodecode.lib.utils2 import str2bool
|
|||
from rhodecode.model.changeset_status import ChangesetStatusModel
|
||||
from rhodecode.model.comment import CommentsModel
|
||||
from rhodecode.model.db import Session, ChangesetStatus
|
||||
from rhodecode.model.pull_request import PullRequestModel
|
||||
from rhodecode.model.pull_request import PullRequestModel, MergeCheck
|
||||
from rhodecode.model.settings import SettingsModel
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
|
@ -270,13 +270,14 @@ def merge_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
raise JSONRPCError('userid is not the same as your user')
|
||||
|
||||
pull_request = get_pull_request_or_error(pullrequestid)
|
||||
if not PullRequestModel().check_user_merge(
|
||||
pull_request, apiuser, api=True):
|
||||
raise JSONRPCError('repository `%s` does not exist' % (repoid,))
|
||||
if pull_request.is_closed():
|
||||
|
||||
check = MergeCheck.validate(pull_request, user=apiuser)
|
||||
merge_possible = not check.failed
|
||||
|
||||
if not merge_possible:
|
||||
reasons = ','.join([msg for _e, msg in check.errors])
|
||||
raise JSONRPCError(
|
||||
'pull request `%s` merge failed, pull request is closed' % (
|
||||
pullrequestid,))
|
||||
'merge not possible for following reasons: {}'.format(reasons))
|
||||
|
||||
target_repo = pull_request.target_repo
|
||||
extras = vcs_operation_context(
|
||||
|
|
|
|||
|
|
@ -60,7 +60,7 @@ from rhodecode.model.db import (PullRequest, ChangesetStatus, ChangesetComment,
|
|||
Repository, PullRequestVersion)
|
||||
from rhodecode.model.forms import PullRequestForm
|
||||
from rhodecode.model.meta import Session
|
||||
from rhodecode.model.pull_request import PullRequestModel
|
||||
from rhodecode.model.pull_request import PullRequestModel, MergeCheck
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
||||
|
|
@ -597,14 +597,20 @@ class PullrequestsController(BaseRepoController):
|
|||
|
||||
Merge will perform a server-side merge of the specified
|
||||
pull request, if the pull request is approved and mergeable.
|
||||
After succesfull merging, the pull request is automatically
|
||||
After successful merging, the pull request is automatically
|
||||
closed, with a relevant comment.
|
||||
"""
|
||||
pull_request_id = safe_int(pull_request_id)
|
||||
pull_request = PullRequest.get_or_404(pull_request_id)
|
||||
user = c.rhodecode_user
|
||||
|
||||
if self._meets_merge_pre_conditions(pull_request, user):
|
||||
check = MergeCheck.validate(pull_request, user)
|
||||
merge_possible = not check.failed
|
||||
|
||||
for err_type, error_msg in check.errors:
|
||||
h.flash(error_msg, category=err_type)
|
||||
|
||||
if merge_possible:
|
||||
log.debug("Pre-conditions checked, trying to merge.")
|
||||
extras = vcs_operation_context(
|
||||
request.environ, repo_name=pull_request.target_repo.repo_name,
|
||||
|
|
@ -617,36 +623,6 @@ class PullrequestsController(BaseRepoController):
|
|||
repo_name=pull_request.target_repo.repo_name,
|
||||
pull_request_id=pull_request.pull_request_id))
|
||||
|
||||
def _meets_merge_pre_conditions(self, pull_request, user):
|
||||
if not PullRequestModel().check_user_merge(pull_request, user):
|
||||
raise HTTPForbidden()
|
||||
|
||||
merge_status, msg = PullRequestModel().merge_status(pull_request)
|
||||
if not merge_status:
|
||||
log.debug("Cannot merge, not mergeable.")
|
||||
h.flash(msg, category='error')
|
||||
return False
|
||||
|
||||
if (pull_request.calculated_review_status()
|
||||
is not ChangesetStatus.STATUS_APPROVED):
|
||||
log.debug("Cannot merge, approval is pending.")
|
||||
msg = _('Pull request reviewer approval is pending.')
|
||||
h.flash(msg, category='error')
|
||||
return False
|
||||
|
||||
todos = CommentsModel().get_unresolved_todos(pull_request)
|
||||
if todos:
|
||||
log.debug("Cannot merge, unresolved todos left.")
|
||||
if len(todos) == 1:
|
||||
msg = _('Cannot merge, {} todo still not resolved.').format(
|
||||
len(todos))
|
||||
else:
|
||||
msg = _('Cannot merge, {} todos still not resolved.').format(
|
||||
len(todos))
|
||||
h.flash(msg, category='error')
|
||||
return False
|
||||
return True
|
||||
|
||||
def _merge_pull_request(self, pull_request, user, extras):
|
||||
merge_resp = PullRequestModel().merge(
|
||||
pull_request, user, extras=extras)
|
||||
|
|
@ -855,24 +831,11 @@ class PullrequestsController(BaseRepoController):
|
|||
if should_render:
|
||||
display_inline_comments[co.f_path][co.line_no].append(co)
|
||||
|
||||
c.pr_merge_checks = []
|
||||
c.pr_merge_status, c.pr_merge_msg = PullRequestModel().merge_status(
|
||||
pull_request_at_ver)
|
||||
c.pr_merge_checks.append(['warning' if not c.pr_merge_status else 'success', c.pr_merge_msg])
|
||||
|
||||
if c.pull_request_review_status != ChangesetStatus.STATUS_APPROVED:
|
||||
approval_msg = _('Reviewer approval is pending.')
|
||||
c.pr_merge_status = False
|
||||
c.pr_merge_checks.append(['warning', approval_msg])
|
||||
|
||||
todos = cc_model.get_unresolved_todos(pull_request_latest)
|
||||
if todos:
|
||||
c.pr_merge_status = False
|
||||
if len(todos) == 1:
|
||||
msg = _('{} todo still not resolved.').format(len(todos))
|
||||
else:
|
||||
msg = _('{} todos still not resolved.').format(len(todos))
|
||||
c.pr_merge_checks.append(['warning', msg])
|
||||
_merge_check = MergeCheck.validate(
|
||||
pull_request_latest, user=c.rhodecode_user)
|
||||
c.pr_merge_errors = _merge_check.errors
|
||||
c.pr_merge_possible = not _merge_check.failed
|
||||
c.pr_merge_message = _merge_check.merge_msg
|
||||
|
||||
if merge_checks:
|
||||
return render('/pullrequests/pullrequest_merge_checks.mako')
|
||||
|
|
|
|||
|
|
@ -1311,6 +1311,86 @@ class PullRequestModel(BaseModel):
|
|||
pull_request.target_repo)
|
||||
|
||||
|
||||
class MergeCheck(object):
|
||||
"""
|
||||
Perform Merge Checks and returns a check object which stores information
|
||||
about merge errors, and merge conditions
|
||||
"""
|
||||
|
||||
def __init__(self):
|
||||
self.merge_possible = None
|
||||
self.merge_msg = ''
|
||||
self.failed = None
|
||||
self.errors = []
|
||||
|
||||
def push_error(self, error_type, message):
|
||||
self.failed = True
|
||||
self.errors.append([error_type, message])
|
||||
|
||||
@classmethod
|
||||
def validate(cls, pull_request, user, fail_early=False, translator=None):
|
||||
# if migrated to pyramid...
|
||||
# _ = lambda: translator or _ # use passed in translator if any
|
||||
|
||||
merge_check = cls()
|
||||
|
||||
# permissions
|
||||
user_allowed_to_merge = PullRequestModel().check_user_merge(
|
||||
pull_request, user)
|
||||
if not user_allowed_to_merge:
|
||||
log.debug("MergeCheck: cannot merge, approval is pending.")
|
||||
|
||||
msg = _('User `{}` not allowed to perform merge').format(user)
|
||||
merge_check.push_error('error', msg)
|
||||
if fail_early:
|
||||
return merge_check
|
||||
|
||||
# review status
|
||||
review_status = pull_request.calculated_review_status()
|
||||
status_approved = review_status == ChangesetStatus.STATUS_APPROVED
|
||||
if not status_approved:
|
||||
log.debug("MergeCheck: cannot merge, approval is pending.")
|
||||
|
||||
msg = _('Pull request reviewer approval is pending.')
|
||||
|
||||
merge_check.push_error('warning', msg)
|
||||
|
||||
if fail_early:
|
||||
return merge_check
|
||||
|
||||
# left over TODOs
|
||||
todos = CommentsModel().get_unresolved_todos(pull_request)
|
||||
if todos:
|
||||
log.debug("MergeCheck: cannot merge, {} "
|
||||
"unresolved todos left.".format(len(todos)))
|
||||
|
||||
if len(todos) == 1:
|
||||
msg = _('Cannot merge, {} TODO still not resolved.').format(
|
||||
len(todos))
|
||||
else:
|
||||
msg = _('Cannot merge, {} TODOs still not resolved.').format(
|
||||
len(todos))
|
||||
|
||||
merge_check.push_error('warning', msg)
|
||||
|
||||
if fail_early:
|
||||
return merge_check
|
||||
|
||||
# merge possible
|
||||
merge_status, msg = PullRequestModel().merge_status(pull_request)
|
||||
merge_check.merge_possible = merge_status
|
||||
merge_check.merge_msg = msg
|
||||
if not merge_status:
|
||||
log.debug(
|
||||
"MergeCheck: cannot merge, pull request merge not possible.")
|
||||
merge_check.push_error('warning', msg)
|
||||
|
||||
if fail_early:
|
||||
return merge_check
|
||||
|
||||
return merge_check
|
||||
|
||||
|
||||
ChangeTuple = namedtuple('ChangeTuple',
|
||||
['added', 'common', 'removed'])
|
||||
|
||||
|
|
|
|||
|
|
@ -1639,25 +1639,29 @@ BIN_FILENODE = 7
|
|||
padding: 0px 0px;
|
||||
}
|
||||
|
||||
.merge-status {
|
||||
margin-right: 5px;
|
||||
}
|
||||
|
||||
.merge-message {
|
||||
font-size: 1.2em
|
||||
}
|
||||
.merge-message li{
|
||||
text-decoration: none;
|
||||
}
|
||||
|
||||
.merge-message.success i {
|
||||
.merge-message.success i,
|
||||
.merge-icon.success i {
|
||||
color:@alert1;
|
||||
}
|
||||
.merge-message.warning i {
|
||||
|
||||
.merge-message.warning i,
|
||||
.merge-icon.warning i {
|
||||
color: @alert3;
|
||||
}
|
||||
.merge-message.error i {
|
||||
|
||||
.merge-message.error i,
|
||||
.merge-icon.error i {
|
||||
color:@alert2;
|
||||
}
|
||||
|
||||
|
||||
|
||||
.pr-versions {
|
||||
position: relative;
|
||||
top: 6px;
|
||||
|
|
|
|||
|
|
@ -1,16 +1,24 @@
|
|||
|
||||
<div class="pull-request-wrap">
|
||||
|
||||
|
||||
% if c.pr_merge_possible:
|
||||
<h2 class="merge-status">
|
||||
<span class="merge-icon success"><i class="icon-true"></i></span>
|
||||
${_('This pull request can be merged automatically.')}
|
||||
</h2>
|
||||
% else:
|
||||
<h2 class="merge-status">
|
||||
<span class="merge-icon warning"><i class="icon-false"></i></span>
|
||||
${_('Merge is not currently possible because of below failed checks.')}
|
||||
</h2>
|
||||
% endif
|
||||
|
||||
<ul>
|
||||
% for pr_check_type, pr_check_msg in c.pr_merge_checks:
|
||||
% for pr_check_type, pr_check_msg in c.pr_merge_errors:
|
||||
<li>
|
||||
<span class="merge-message ${pr_check_type}" data-role="merge-message">
|
||||
% if pr_check_type in ['success']:
|
||||
<i class="icon-true"></i>
|
||||
% else:
|
||||
<i class="icon-false"></i>
|
||||
% endif
|
||||
${pr_check_msg}
|
||||
- ${pr_check_msg}
|
||||
</span>
|
||||
</li>
|
||||
% endfor
|
||||
|
|
@ -20,7 +28,7 @@
|
|||
% if c.allowed_to_merge:
|
||||
<div class="pull-right">
|
||||
${h.secure_form(url('pullrequest_merge', repo_name=c.repo_name, pull_request_id=c.pull_request.pull_request_id), id='merge_pull_request_form')}
|
||||
<% merge_disabled = ' disabled' if c.pr_merge_status is False else '' %>
|
||||
<% merge_disabled = ' disabled' if c.pr_merge_possible is False else '' %>
|
||||
<a class="btn" href="#" onclick="refreshMergeChecks(); return false;">${_('refresh checks')}</a>
|
||||
<input type="submit" id="merge_pull_request" value="${_('Merge Pull Request')}" class="btn${merge_disabled}"${merge_disabled}>
|
||||
${h.end_form()}
|
||||
|
|
@ -32,6 +40,5 @@
|
|||
<input type="submit" value="${_('Login to Merge this Pull Request')}" class="btn disabled" disabled="disabled">
|
||||
% endif
|
||||
</div>
|
||||
|
||||
</div>
|
||||
|
||||
|
|
|
|||
|
|
@ -541,7 +541,9 @@ class TestPullrequestsController:
|
|||
params={'csrf_token': csrf_token}).follow()
|
||||
|
||||
assert response.status_int == 200
|
||||
assert 'Server-side pull request merging is disabled.' in response.body
|
||||
response.mustcontain(
|
||||
'Merge is not currently possible because of below failed checks.')
|
||||
response.mustcontain('Server-side pull request merging is disabled.')
|
||||
|
||||
@pytest.mark.skip_backends('svn')
|
||||
def test_merge_pull_request_not_approved(self, pr_util, csrf_token):
|
||||
|
|
@ -556,10 +558,11 @@ class TestPullrequestsController:
|
|||
pull_request_id=str(pull_request_id)),
|
||||
params={'csrf_token': csrf_token}).follow()
|
||||
|
||||
pull_request = PullRequest.get(pull_request_id)
|
||||
|
||||
assert response.status_int == 200
|
||||
assert ' Reviewer approval is pending.' in response.body
|
||||
|
||||
response.mustcontain(
|
||||
'Merge is not currently possible because of below failed checks.')
|
||||
response.mustcontain('Pull request reviewer approval is pending.')
|
||||
|
||||
def test_update_source_revision(self, backend, csrf_token):
|
||||
commits = [
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue