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

rhodecode/api/views/pull_request_api.py

@@ -32,7 +32,7 b' 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 b' 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(

rhodecode/controllers/pullrequests.py

@@ -60,7 +60,7 b' 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 b' 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 b' 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 b' 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')

rhodecode/model/pull_request.py

@@ -1311,6 +1311,86 b' 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'])
 

rhodecode/public/css/main.less

@@ -1639,25 +1639,29 b' 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;

rhodecode/templates/pullrequests/pullrequest_merge_checks.mako

@@ -1,16 +1,24 b''
 
 <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 b''
         % 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 b''
           <input type="submit" value="${_('Login to Merge this Pull Request')}" class="btn disabled" disabled="disabled">
         % endif
     </div>
-
 </div>
 

rhodecode/tests/functional/test_pullrequests.py

@@ -541,7 +541,9 b' 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 b' 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 = [