pull-request-api: updated logic of closing a PR via API call.

- consistent logic via web and API
- updated events
- updated proper close checks.

rhodecode/api/tests/test_close_pull_request.py

@@ -43,6 +43,7 b' class TestClosePullRequest(object):'
         response = api_call(self.app, params)
         expected = {
             'pull_request_id': pull_request_id,
+            'close_status': 'Rejected',
             'closed': True,
         }
         assert_ok(id_, expected, response.body)

rhodecode/api/views/pull_request_api.py

@@ -21,6 +21,7 b''
 
 import logging
 
+from rhodecode import events
 from rhodecode.api import jsonrpc_method, JSONRPCError, JSONRPCValidationError
 from rhodecode.api.utils import (
     has_superadmin_permission, Optional, OAttr, get_repo_or_error,
@@ -35,8 +36,8 b' from rhodecode.model.db import Session, '
 from rhodecode.model.pull_request import PullRequestModel, MergeCheck
 from rhodecode.model.settings import SettingsModel
 from rhodecode.model.validation_schema import Invalid
-from rhodecode.model.validation_schema.schemas.reviewer_schema import \
-    ReviewerListSchema
+from rhodecode.model.validation_schema.schemas.reviewer_schema import(
+    ReviewerListSchema)
 
 log = logging.getLogger(__name__)
 
@@ -227,7 +228,8 b' def get_pull_requests(request, apiuser, '
 
 
 @jsonrpc_method()
-def merge_pull_request(request, apiuser, repoid, pullrequestid,
+def merge_pull_request(
+        request, apiuser, repoid, pullrequestid,
                        userid=Optional(OAttr('apiuser'))):
     """
     Merge the pull request specified by `pullrequestid` into its target
@@ -308,63 +310,6 b' def merge_pull_request(request, apiuser,'
 
 
 @jsonrpc_method()
-def close_pull_request(request, apiuser, repoid, pullrequestid,
-                       userid=Optional(OAttr('apiuser'))):
-    """
-    Close the pull request specified by `pullrequestid`.
-
-    :param apiuser: This is filled automatically from the |authtoken|.
-    :type apiuser: AuthUser
-    :param repoid: Repository name or repository ID to which the pull
-        request belongs.
-    :type repoid: str or int
-    :param pullrequestid: ID of the pull request to be closed.
-    :type pullrequestid: int
-    :param userid: Close the pull request as this user.
-    :type userid: Optional(str or int)
-
-    Example output:
-
-    .. code-block:: bash
-
-        "id": <id_given_in_input>,
-        "result": {
-            "pull_request_id":  "<int>",
-            "closed":           "<bool>"
-        },
-        "error": null
-
-    """
-    repo = get_repo_or_error(repoid)
-    if not isinstance(userid, Optional):
-        if (has_superadmin_permission(apiuser) or
-                HasRepoPermissionAnyApi('repository.admin')(
-                    user=apiuser, repo_name=repo.repo_name)):
-            apiuser = get_user_or_error(userid)
-        else:
-            raise JSONRPCError('userid is not the same as your user')
-
-    pull_request = get_pull_request_or_error(pullrequestid)
-    if not PullRequestModel().check_user_update(
-            pull_request, apiuser, api=True):
-        raise JSONRPCError(
-            'pull request `%s` close failed, no permission to close.' % (
-                pullrequestid,))
-    if pull_request.is_closed():
-        raise JSONRPCError(
-            'pull request `%s` is already closed' % (pullrequestid,))
-
-    PullRequestModel().close_pull_request(
-        pull_request.pull_request_id, apiuser)
-    Session().commit()
-    data = {
-        'pull_request_id': pull_request.pull_request_id,
-        'closed': True,
-    }
-    return data
-
-
-@jsonrpc_method()
 def comment_pull_request(
         request, apiuser, repoid, pullrequestid, message=Optional(None),
         commit_id=Optional(None), status=Optional(None),
@@ -610,7 +555,7 b' def create_pull_request('
 def update_pull_request(
         request, apiuser, repoid, pullrequestid, title=Optional(''),
         description=Optional(''), reviewers=Optional(None),
-        update_commits=Optional(None), close_pull_request=Optional(None)):
+        update_commits=Optional(None)):
     """
     Updates a pull request.
 
@@ -632,8 +577,6 b' def update_pull_request('
 
     :param update_commits: Trigger update of commits for this pull request
     :type: update_commits: Optional(bool)
-    :param close_pull_request: Close this pull request with rejected state
-    :type: close_pull_request: Optional(bool)
 
     Example output:
 
@@ -675,7 +618,6 b' def update_pull_request('
             'pull request `%s` update failed, pull request is closed' % (
                 pullrequestid,))
 
-
     reviewer_objects = Optional.extract(reviewers) or []
     if reviewer_objects:
         schema = ReviewerListSchema()
@@ -718,11 +660,6 b' def update_pull_request('
             [get_user_or_error(n).username for n in removed_reviewers])
         Session().commit()
 
-    if str2bool(Optional.extract(close_pull_request)):
-        PullRequestModel().close_pull_request_with_comment(
-            pull_request, apiuser, repo)
-        Session().commit()
-
     data = {
         'msg': 'Updated pull request `{}`'.format(
             pull_request.pull_request_id),
@@ -732,3 +669,80 b' def update_pull_request('
     }
 
     return data
+
+
+@jsonrpc_method()
+def close_pull_request(
+        request, apiuser, repoid, pullrequestid,
+        userid=Optional(OAttr('apiuser')), message=Optional('')):
+    """
+    Close the pull request specified by `pullrequestid`.
+
+    :param apiuser: This is filled automatically from the |authtoken|.
+    :type apiuser: AuthUser
+    :param repoid: Repository name or repository ID to which the pull
+        request belongs.
+    :type repoid: str or int
+    :param pullrequestid: ID of the pull request to be closed.
+    :type pullrequestid: int
+    :param userid: Close the pull request as this user.
+    :type userid: Optional(str or int)
+    :param message: Optional message to close the Pull Request with. If not
+        specified it will be generated automatically.
+    :type message: Optional(str)
+
+    Example output:
+
+    .. code-block:: bash
+
+        "id": <id_given_in_input>,
+        "result": {
+            "pull_request_id":  "<int>",
+            "close_status":     "<str:status_lbl>,
+            "closed":           "<bool>"
+        },
+        "error": null
+
+    """
+    _ = request.translate
+
+    repo = get_repo_or_error(repoid)
+    if not isinstance(userid, Optional):
+        if (has_superadmin_permission(apiuser) or
+                HasRepoPermissionAnyApi('repository.admin')(
+                    user=apiuser, repo_name=repo.repo_name)):
+            apiuser = get_user_or_error(userid)
+        else:
+            raise JSONRPCError('userid is not the same as your user')
+
+    pull_request = get_pull_request_or_error(pullrequestid)
+
+    if pull_request.is_closed():
+        raise JSONRPCError(
+            'pull request `%s` is already closed' % (pullrequestid,))
+
+    # only owner or admin or person with write permissions
+    allowed_to_close = PullRequestModel().check_user_update(
+            pull_request, apiuser, api=True)
+
+    if not allowed_to_close:
+        raise JSONRPCError(
+            'pull request `%s` close failed, no permission to close.' % (
+                pullrequestid,))
+
+    # message we're using to close the PR, else it's automatically generated
+    message = Optional.extract(message)
+
+    # finally close the PR, with proper message comment
+    comment, status = PullRequestModel().close_pull_request_with_comment(
+        pull_request, apiuser, repo, message=message)
+    status_lbl = ChangesetStatus.get_status_lbl(status)
+
+    Session().commit()
+
+    data = {
+        'pull_request_id': pull_request.pull_request_id,
+        'close_status': status_lbl,
+        'closed': True,
+    }
+    return data

rhodecode/controllers/changeset.py

@@ -368,7 +368,6 b' class ChangesetController(BaseRepoContro'
                 comment_type=comment_type,
                 resolves_comment_id=resolves_comment_id
             )
-            c.inline_comment = True if comment.line_no else False
 
             # get status if set !
             if status:

rhodecode/controllers/pullrequests.py

@@ -306,8 +306,6 b' class PullrequestsController(BaseRepoCon'
                     pull_request.reviewer_data)
             elif str2bool(request.POST.get('update_commits', 'false')):
                 self._update_commits(pull_request)
-            elif str2bool(request.POST.get('close_pull_request', 'false')):
-                self._reject_close(pull_request)
             elif str2bool(request.POST.get('edit_pull_request', 'false')):
                 self._edit_pull_request(pull_request)
             else:
@@ -462,14 +460,6 b' class PullrequestsController(BaseRepoCon'
         h.flash(_('Pull request reviewers updated.'), category='success')
         Session().commit()
 
-    def _reject_close(self, pull_request):
-        if pull_request.is_closed():
-            raise HTTPForbidden()
-
-        PullRequestModel().close_pull_request_with_comment(
-            pull_request, c.rhodecode_user, c.rhodecode_db_repo)
-        Session().commit()
-
     @LoginRequired()
     @NotAnonymous()
     @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
@@ -888,6 +878,7 b' class PullrequestsController(BaseRepoCon'
         pull_request_id = safe_int(pull_request_id)
         pull_request = PullRequest.get_or_404(pull_request_id)
         if pull_request.is_closed():
+            log.debug('comment: forbidden because pull request is closed')
             raise HTTPForbidden()
 
         status = request.POST.get('changeset_status', None)
@@ -896,19 +887,29 b' class PullrequestsController(BaseRepoCon'
         resolves_comment_id = request.POST.get('resolves_comment_id', None)
         close_pull_request = request.POST.get('close_pull_request')
 
-        close_pr = False
+        # the logic here should work like following, if we submit close
+        # pr comment, use `close_pull_request_with_comment` function
+        # else handle regular comment logic
+        user = c.rhodecode_user
+        repo = c.rhodecode_db_repo
+
+        if close_pull_request:
         # only owner or admin or person with write permissions
         allowed_to_close = PullRequestModel().check_user_update(
             pull_request, c.rhodecode_user)
+            if not allowed_to_close:
+                log.debug('comment: forbidden because not allowed to close '
+                          'pull request %s', pull_request_id)
+                raise HTTPForbidden()
+            comment, status = PullRequestModel().close_pull_request_with_comment(
+                pull_request, user, repo, message=text)
+            Session().flush()
+            events.trigger(
+                events.PullRequestCommentEvent(pull_request, comment))
 
-        if close_pull_request and allowed_to_close:
-            close_pr = True
-            pull_request_review_status = pull_request.calculated_review_status()
-            if pull_request_review_status == ChangesetStatus.STATUS_APPROVED:
-                # approved only if we have voting consent
-                status = ChangesetStatus.STATUS_APPROVED
             else:
-                status = ChangesetStatus.STATUS_REJECTED
+            # regular comment case, could be inline, or one with status.
+            # for that one we check also permissions
 
         allowed_to_change_status = PullRequestModel().check_user_change_status(
             pull_request, c.rhodecode_user)
@@ -917,10 +918,9 b' class PullrequestsController(BaseRepoCon'
             message = (_('Status change %(transition_icon)s %(status)s')
                        % {'transition_icon': '>',
                           'status': ChangesetStatus.get_status_lbl(status)})
-            if close_pr:
-                message = _('Closing with') + ' ' + message
             text = text or message
-        comm = CommentsModel().create(
+
+            comment = CommentsModel().create(
             text=text,
             repo=c.rhodecode_db_repo.repo_id,
             user=c.rhodecode_user.user_id,
@@ -931,25 +931,28 b' class PullrequestsController(BaseRepoCon'
                            if status and allowed_to_change_status else None),
             status_change_type=(status
                                 if status and allowed_to_change_status else None),
-            closing_pr=close_pr,
             comment_type=comment_type,
             resolves_comment_id=resolves_comment_id
         )
 
         if allowed_to_change_status:
+                # calculate old status before we change it
             old_calculated_status = pull_request.calculated_review_status()
+
             # get status if set !
             if status:
                 ChangesetStatusModel().set_status(
                     c.rhodecode_db_repo.repo_id,
                     status,
                     c.rhodecode_user.user_id,
-                    comm,
+                        comment,
                     pull_request=pull_request_id
                 )
 
             Session().flush()
-            events.trigger(events.PullRequestCommentEvent(pull_request, comm))
+                events.trigger(
+                    events.PullRequestCommentEvent(pull_request, comment))
+
             # we now calculate the status of pull request, and based on that
             # calculation we set the commits status
             calculated_status = pull_request.calculated_review_status()
@@ -957,23 +960,6 b' class PullrequestsController(BaseRepoCon'
                 PullRequestModel()._trigger_pull_request_hook(
                     pull_request, c.rhodecode_user, 'review_status_change')
 
-            calculated_status_lbl = ChangesetStatus.get_status_lbl(
-                calculated_status)
-
-            if close_pr:
-                status_completed = (
-                    calculated_status in [ChangesetStatus.STATUS_APPROVED,
-                                          ChangesetStatus.STATUS_REJECTED])
-                if close_pull_request or status_completed:
-                    PullRequestModel().close_pull_request(
-                        pull_request_id, c.rhodecode_user)
-                else:
-                    h.flash(_('Closing pull request on other statuses than '
-                              'rejected or approved is forbidden. '
-                              'Calculated status from all reviewers '
-                              'is currently: %s') % calculated_status_lbl,
-                            category='warning')
-
         Session().commit()
 
         if not request.is_xhr:
@@ -983,12 +969,11 b' class PullrequestsController(BaseRepoCon'
         data = {
             'target_id': h.safeid(h.safe_unicode(request.POST.get('f_path'))),
         }
-        if comm:
-            c.co = comm
-            c.inline_comment = True if comm.line_no else False
-            data.update(comm.get_dict())
-            data.update({'rendered_text':
-                             render('changeset/changeset_comment_block.mako')})
+        if comment:
+            c.co = comment
+            rendered_comment = render('changeset/changeset_comment_block.mako')
+            data.update(comment.get_dict())
+            data.update({'rendered_text': rendered_comment})
 
         return data
 

rhodecode/model/db.py

@@ -3107,6 +3107,10 b' class ChangesetComment(Base, BaseModel):'
     def is_todo(self):
         return self.comment_type == self.COMMENT_TYPE_TODO
 
+    @property
+    def is_inline(self):
+        return self.line_no and self.f_path
+
     def get_index_version(self, versions):
         return self.get_index_from_version(
             self.pull_request_version_id, versions)

rhodecode/model/pull_request.py

@@ -34,6 +34,7 b' from pylons.i18n.translation import lazy'
 from pyramid.threadlocal import get_current_request
 from sqlalchemy import or_
 
+from rhodecode import events
 from rhodecode.lib import helpers as h, hooks_utils, diffs
 from rhodecode.lib.compat import OrderedDict
 from rhodecode.lib.hooks_daemon import prepare_callback_daemon
@@ -1064,42 +1065,61 b' class PullRequestModel(BaseModel):'
             pull_request, pull_request.author, 'close')
         self._log_action('user_closed_pull_request', user, pull_request)
 
-    def close_pull_request_with_comment(self, pull_request, user, repo,
-                                        message=None):
-        status = ChangesetStatus.STATUS_REJECTED
+    def close_pull_request_with_comment(
+            self, pull_request, user, repo, message=None):
+
+        pull_request_review_status = pull_request.calculated_review_status()
 
-        if not message:
-            message = (
-                _('Status change %(transition_icon)s %(status)s') % {
-                    'transition_icon': '>',
-                    'status': ChangesetStatus.get_status_lbl(status)})
+        if pull_request_review_status == ChangesetStatus.STATUS_APPROVED:
+            # approved only if we have voting consent
+            status = ChangesetStatus.STATUS_APPROVED
+        else:
+            status = ChangesetStatus.STATUS_REJECTED
+        status_lbl = ChangesetStatus.get_status_lbl(status)
 
-        internal_message = _('Closing with') + ' ' + message
+        default_message = (
+            _('Closing with status change {transition_icon} {status}.')
+        ).format(transition_icon='>', status=status_lbl)
+        text = message or default_message
 
-        comm = CommentsModel().create(
-            text=internal_message,
+        # create a comment, and link it to new status
+        comment = CommentsModel().create(
+            text=text,
             repo=repo.repo_id,
             user=user.user_id,
             pull_request=pull_request.pull_request_id,
-            f_path=None,
-            line_no=None,
-            status_change=ChangesetStatus.get_status_lbl(status),
+            status_change=status_lbl,
             status_change_type=status,
             closing_pr=True
         )
 
+        # calculate old status before we change it
+        old_calculated_status = pull_request.calculated_review_status()
         ChangesetStatusModel().set_status(
             repo.repo_id,
             status,
             user.user_id,
-            comm,
+            comment=comment,
             pull_request=pull_request.pull_request_id
         )
+
         Session().flush()
+        events.trigger(events.PullRequestCommentEvent(pull_request, comment))
+        # we now calculate the status of pull request again, and based on that
+        # calculation trigger status change. This might happen in cases
+        # that non-reviewer admin closes a pr, which means his vote doesn't
+        # change the status, while if he's a reviewer this might change it.
+        calculated_status = pull_request.calculated_review_status()
+        if old_calculated_status != calculated_status:
+            self._trigger_pull_request_hook(
+                pull_request, user, 'review_status_change')
 
+        # finally close the PR
         PullRequestModel().close_pull_request(
             pull_request.pull_request_id, user)
 
+        return comment, status
+
     def merge_status(self, pull_request):
         if not self._is_merge_enabled(pull_request):
             return False, _('Server-side pull request merging is disabled.')

rhodecode/public/js/rhodecode/routes.js

@@ -96,6 +96,7 b' function registerRCRoutes() {'
     pyroutes.register('goto_switcher_data', '/_goto_data', []);
     pyroutes.register('repo_summary_explicit', '/%(repo_name)s/summary', ['repo_name']);
     pyroutes.register('repo_summary_commits', '/%(repo_name)s/summary-commits', ['repo_name']);
+    pyroutes.register('repo_commit', '/%(repo_name)s/changeset/%(commit_id)s', ['repo_name', 'commit_id']);
     pyroutes.register('repo_refs_data', '/%(repo_name)s/refs-data', ['repo_name']);
     pyroutes.register('repo_refs_changelog_data', '/%(repo_name)s/refs-data-changelog', ['repo_name']);
     pyroutes.register('repo_stats', '/%(repo_name)s/repo_stats/%(commit_id)s', ['repo_name', 'commit_id']);

rhodecode/public/js/src/rhodecode/pullrequests.js

@@ -368,16 +368,6 b' var _updatePullRequest = function(repo_n'
 };
 
 /**
- * PULL REQUEST reject & close
- */
-var closePullRequest = function(repo_name, pull_request_id) {
-    var postData = {
-        '_method': 'put',
-        'close_pull_request': true};
-    _updatePullRequest(repo_name, pull_request_id, postData);
-};
-
-/**
  * PULL REQUEST update commits
  */
 var updateCommits = function(repo_name, pull_request_id) {

rhodecode/templates/changeset/changeset_comment_block.mako

@@ -1,4 +1,4 b''
 ## this is a dummy html file for partial rendering on server and sending
 ## generated output via ajax after comment submit
 <%namespace name="comment" file="/changeset/changeset_file_comment.mako"/>
-${comment.comment_block(c.co, inline=c.inline_comment)}
+${comment.comment_block(c.co, inline=c.co.is_inline)}

rhodecode/templates/debug_style/collapsable-content.html

@@ -949,9 +949,6 b''
                 updateCommits("rhodecode-momentum", "720");
             });
 
-            $('#close_pull_request').on('click', function(e){
-                closePullRequest("rhodecode-momentum", "720");
-            });
         })
       </script>
 

rhodecode/templates/pullrequests/pullrequest_show.mako

@@ -825,11 +825,6 b''
             // fixing issue with caches on firefox
             $('#update_commits').removeAttr("disabled");
 
-            $('#close_pull_request').on('click', function(e){
-                closePullRequest(
-                    "${c.repo_name}", "${c.pull_request.pull_request_id}");
-            });
-
             $('.show-inline-comments').on('click', function(e){
                 var boxid = $(this).attr('data-comment-id');
                 var button = $(this);

rhodecode/tests/functional/test_pullrequests.py

@@ -237,7 +237,9 b' class TestPullrequestsController(object)'
         assertr.element_contains(
             'span[data-role="merge-message"]', str(expected_msg))
 
-    def test_comment_and_close_pull_request(self, pr_util, csrf_token):
+    def test_comment_and_close_pull_request_custom_message_approved(
+            self, pr_util, csrf_token, xhr_header):
+
         pull_request = pr_util.create_pull_request(approved=True)
         pull_request_id = pull_request.pull_request_id
         author = pull_request.user_id
@@ -249,11 +251,10 b' class TestPullrequestsController(object)'
                 repo_name=pull_request.target_repo.scm_instance().name,
                 pull_request_id=str(pull_request_id)),
             params={
-                'changeset_status': ChangesetStatus.STATUS_APPROVED,
                 'close_pull_request': '1',
                 'text': 'Closing a PR',
                 'csrf_token': csrf_token},
-            status=302)
+            extra_environ=xhr_header,)
 
         action = 'user_closed_pull_request:%d' % pull_request_id
         journal = UserLog.query()\
@@ -270,45 +271,26 b' class TestPullrequestsController(object)'
         status = ChangesetStatusModel().get_status(
             pull_request.source_repo, pull_request=pull_request)
         assert status == ChangesetStatus.STATUS_APPROVED
-
-    def test_reject_and_close_pull_request(self, pr_util, csrf_token):
-        pull_request = pr_util.create_pull_request()
-        pull_request_id = pull_request.pull_request_id
-        response = self.app.post(
-            url(controller='pullrequests',
-                action='update',
-                repo_name=pull_request.target_repo.scm_instance().name,
-                pull_request_id=str(pull_request.pull_request_id)),
-            params={'close_pull_request': 'true', '_method': 'put',
-                    'csrf_token': csrf_token})
+        assert pull_request.comments[-1].text == 'Closing a PR'
 
-        pull_request = PullRequest.get(pull_request_id)
-
-        assert response.json is True
-        assert pull_request.is_closed()
-
-        # check only the latest status, not the review status
-        status = ChangesetStatusModel().get_status(
-            pull_request.source_repo, pull_request=pull_request)
-        assert status == ChangesetStatus.STATUS_REJECTED
-
-    def test_comment_force_close_pull_request(self, pr_util, csrf_token):
+    def test_comment_force_close_pull_request_rejected(
+            self, pr_util, csrf_token, xhr_header):
         pull_request = pr_util.create_pull_request()
         pull_request_id = pull_request.pull_request_id
         PullRequestModel().update_reviewers(
             pull_request_id, [(1, ['reason'], False), (2, ['reason2'], False)])
         author = pull_request.user_id
         repo = pull_request.target_repo.repo_id
+
         self.app.post(
             url(controller='pullrequests',
                 action='comment',
                 repo_name=pull_request.target_repo.scm_instance().name,
                 pull_request_id=str(pull_request_id)),
             params={
-                'changeset_status': 'rejected',
                 'close_pull_request': '1',
                 'csrf_token': csrf_token},
-            status=302)
+            extra_environ=xhr_header)
 
         pull_request = PullRequest.get(pull_request_id)
 
@@ -324,6 +306,31 b' class TestPullrequestsController(object)'
             pull_request.source_repo, pull_request=pull_request)
         assert status == ChangesetStatus.STATUS_REJECTED
 
+    def test_comment_and_close_pull_request(
+            self, pr_util, csrf_token, xhr_header):
+        pull_request = pr_util.create_pull_request()
+        pull_request_id = pull_request.pull_request_id
+
+        response = self.app.post(
+            url(controller='pullrequests',
+                action='comment',
+                repo_name=pull_request.target_repo.scm_instance().name,
+                pull_request_id=str(pull_request.pull_request_id)),
+            params={
+                'close_pull_request': 'true',
+                'csrf_token': csrf_token},
+            extra_environ=xhr_header)
+
+        assert response.json
+
+        pull_request = PullRequest.get(pull_request_id)
+        assert pull_request.is_closed()
+
+        # check only the latest status, not the review status
+        status = ChangesetStatusModel().get_status(
+            pull_request.source_repo, pull_request=pull_request)
+        assert status == ChangesetStatus.STATUS_REJECTED
+
     def test_create_pull_request(self, backend, csrf_token):
         commits = [
             {'message': 'ancestor'},