diff --git a/rhodecode/api/views/pull_request_api.py b/rhodecode/api/views/pull_request_api.py --- a/rhodecode/api/views/pull_request_api.py +++ b/rhodecode/api/views/pull_request_api.py @@ -32,7 +32,7 @@ from rhodecode.lib.utils2 import str2boo 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, 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( diff --git a/rhodecode/controllers/pullrequests.py b/rhodecode/controllers/pullrequests.py --- a/rhodecode/controllers/pullrequests.py +++ b/rhodecode/controllers/pullrequests.py @@ -60,7 +60,7 @@ from rhodecode.model.db import (PullRequ 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(BaseRepoCon 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(BaseRepoCon 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(BaseRepoCon 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') diff --git a/rhodecode/model/pull_request.py b/rhodecode/model/pull_request.py --- a/rhodecode/model/pull_request.py +++ b/rhodecode/model/pull_request.py @@ -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']) diff --git a/rhodecode/public/css/main.less b/rhodecode/public/css/main.less --- a/rhodecode/public/css/main.less +++ b/rhodecode/public/css/main.less @@ -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; diff --git a/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako b/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako --- a/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako +++ b/rhodecode/templates/pullrequests/pullrequest_merge_checks.mako @@ -1,16 +1,24 @@
+ + % if c.pr_merge_possible: +

+ + ${_('This pull request can be merged automatically.')} +

+ % else: +

+ + ${_('Merge is not currently possible because of below failed checks.')} +

+ % endif +
diff --git a/rhodecode/tests/functional/test_pullrequests.py b/rhodecode/tests/functional/test_pullrequests.py --- a/rhodecode/tests/functional/test_pullrequests.py +++ b/rhodecode/tests/functional/test_pullrequests.py @@ -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 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 = [