pull-requests: add explicit CLOSE pr action instead of closed status from selector.

- This is much better UX
- we set status based on calculated review
- fixes #5215

rhodecode/controllers/pullrequests.py

@@ -69,6 +69,8 @@ class PullrequestsController(BaseRepoController):
 
     def __before__(self):
         super(PullrequestsController, self).__before__()
+        c.REVIEW_STATUS_APPROVED = ChangesetStatus.STATUS_APPROVED
+        c.REVIEW_STATUS_REJECTED = ChangesetStatus.STATUS_REJECTED
 
     def _extract_ordering(self, request):
         column_index = safe_int(request.GET.get('order[0][column]'))
@@ -686,9 +688,12 @@ class PullrequestsController(BaseRepoController):
             c.allowed_to_merge = False
             c.allowed_to_delete = False
             c.allowed_to_comment = False
+            c.allowed_to_close = False
         else:
             c.allowed_to_change_status = PullRequestModel(). \
-                check_user_change_status(pull_request_at_ver, c.rhodecode_user)
+                check_user_change_status(pull_request_at_ver, c.rhodecode_user) \
+                                         and not pr_closed
+
             c.allowed_to_update = PullRequestModel().check_user_update(
                 pull_request_latest, c.rhodecode_user) and not pr_closed
             c.allowed_to_merge = PullRequestModel().check_user_merge(
@@ -696,6 +701,7 @@ class PullrequestsController(BaseRepoController):
             c.allowed_to_delete = PullRequestModel().check_user_delete(
                 pull_request_latest, c.rhodecode_user) and not pr_closed
             c.allowed_to_comment = not pr_closed
+            c.allowed_to_close = c.allowed_to_change_status and not pr_closed
 
         # check merge capabilities
         _merge_check = MergeCheck.validate(
@@ -704,6 +710,7 @@ class PullrequestsController(BaseRepoController):
         c.pr_merge_possible = not _merge_check.failed
         c.pr_merge_message = _merge_check.merge_msg
 
+        c.pull_request_review_status = _merge_check.review_status
         if merge_checks:
             return render('/pullrequests/pullrequest_merge_checks.mako')
 
@@ -712,7 +719,6 @@ class PullrequestsController(BaseRepoController):
         # reviewers and statuses
         c.pull_request_reviewers = pull_request_at_ver.reviewers_statuses()
         allowed_reviewers = [x[0].user_id for x in c.pull_request_reviewers]
-        c.pull_request_review_status = pull_request_at_ver.calculated_review_status()
 
         # GENERAL COMMENTS with versions #
         q = comments_model._all_general_comments_of_pull_request(pull_request_latest)
@@ -860,12 +866,7 @@ class PullrequestsController(BaseRepoController):
         # We need to swap that here to generate it properly on the html side
         c.target_repo = c.source_repo
 
-        if c.allowed_to_update:
-            force_close = ('forced_closed', _('Close Pull Request'))
-            statuses = ChangesetStatus.STATUSES + [force_close]
-        else:
-            statuses = ChangesetStatus.STATUSES
-        c.commit_statuses = statuses
+        c.commit_statuses = ChangesetStatus.STATUSES
 
         c.show_version_changes = not pr_closed
         if c.show_version_changes:
@@ -924,22 +925,21 @@ class PullrequestsController(BaseRepoController):
         if pull_request.is_closed():
             raise HTTPForbidden()
 
-        # TODO: johbo: Re-think this bit, "approved_closed" does not exist
-        # as a changeset status, still we want to send it in one value.
         status = request.POST.get('changeset_status', None)
         text = request.POST.get('text')
         comment_type = request.POST.get('comment_type')
         resolves_comment_id = request.POST.get('resolves_comment_id', None)
+        close_pull_request = request.POST.get('close_pull_request')
 
-        if status and '_closed' in status:
+        close_pr = False
+        if close_pull_request:
             close_pr = True
-            status = status.replace('_closed', '')
-        else:
-            close_pr = False
-
-        forced = (status == 'forced')
-        if forced:
-            status = 'rejected'
+            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
 
         allowed_to_change_status = PullRequestModel().check_user_change_status(
             pull_request, c.rhodecode_user)
@@ -995,7 +995,7 @@ class PullrequestsController(BaseRepoController):
                 status_completed = (
                     calculated_status in [ChangesetStatus.STATUS_APPROVED,
                                           ChangesetStatus.STATUS_REJECTED])
-                if forced or status_completed:
+                if close_pull_request or status_completed:
                     PullRequestModel().close_pull_request(
                         pull_request_id, c.rhodecode_user)
                 else:

rhodecode/model/pull_request.py

@@ -1333,6 +1333,7 @@ class MergeCheck(object):
     MERGE_CHECK = 'merge'
 
     def __init__(self):
+        self.review_status = None
         self.merge_possible = None
         self.merge_msg = ''
         self.failed = None
@@ -1355,7 +1356,7 @@ class MergeCheck(object):
 
         merge_check = cls()
 
-        # permissions
+        # permissions to merge
         user_allowed_to_merge = PullRequestModel().check_user_merge(
             pull_request, user)
         if not user_allowed_to_merge:
@@ -1366,8 +1367,10 @@ class MergeCheck(object):
             if fail_early:
                 return merge_check
 
-        # review status
+        # review status, must be always present
         review_status = pull_request.calculated_review_status()
+        merge_check.review_status = review_status
+
         status_approved = review_status == ChangesetStatus.STATUS_APPROVED
         if not status_approved:
             log.debug("MergeCheck: cannot merge, approval is pending.")

rhodecode/public/css/buttons.less

@@ -174,6 +174,19 @@ input[type="button"] {
     }
 }
 
+.btn-approved-status {
+    .border ( @border-thickness, @alert1 );
+    background-color: white;
+    color: @alert1;
+
+}
+
+.btn-rejected-status {
+    .border ( @border-thickness, @alert2 );
+    background-color: white;
+    color: @alert2;
+}
+
 .btn-sm,
 .btn-mini,
 .field-sm .btn {

rhodecode/public/css/comments.less

@@ -384,6 +384,10 @@ form.comment-form {
         float: right;
         display: inline-block;
     }
+
+    .action-buttons-extra {
+        display: inline-block;
+    }
 }
 
 .comment-form {

rhodecode/public/js/src/rhodecode/comments.js

@@ -92,6 +92,8 @@ var bindToggleButtons = function() {
         this.resolvesId = null;
         this.resolvesActionId = null;
 
+        this.closesPr = '#close_pull_request';
+
         this.cmBox = this.withLineNo('#text');
         this.cm = initCommentBoxCodeMirror(this, this.cmBox, this.initAutocompleteActions);
 
@@ -175,6 +177,11 @@ var bindToggleButtons = function() {
         this.getResolvesId = function() {
             return $(this.submitForm).find(this.resolvesId).val() || null;
         };
+
+        this.getClosePr = function() {
+            return $(this.submitForm).find(this.closesPr).val() || null;
+        };
+
         this.markCommentResolved = function(resolvedCommentId){
             $('#comment-label-{0}'.format(resolvedCommentId)).find('.resolved').show();
             $('#comment-label-{0}'.format(resolvedCommentId)).find('.resolve').hide();
@@ -209,6 +216,7 @@ var bindToggleButtons = function() {
             });
             $(this.submitForm).find(this.statusChange).on('change', function() {
                 var status = self.getCommentStatus();
+
                 if (status && !self.isInline()) {
                     $(self.submitButton).prop('disabled', false);
                 }
@@ -236,6 +244,8 @@ var bindToggleButtons = function() {
                 // destroy the resolve action
                 $(this.resolvesId).parent().remove();
             }
+            // reset closingPR flag
+            $('.close-pr-input').remove();
 
             $(this.statusChange).select2('readonly', false);
         };
@@ -284,6 +294,7 @@ var bindToggleButtons = function() {
             var status = self.getCommentStatus();
             var commentType = self.getCommentType();
             var resolvesCommentId = self.getResolvesId();
+            var closePullRequest = self.getClosePr();
 
             if (text === "" && !status) {
                 return;
@@ -300,10 +311,15 @@ var bindToggleButtons = function() {
                 'comment_type': commentType,
                 'csrf_token': CSRF_TOKEN
             };
-            if (resolvesCommentId){
+
+            if (resolvesCommentId) {
                 postData['resolves_comment_id'] = resolvesCommentId;
             }
 
+            if (closePullRequest) {
+                postData['close_pull_request'] = true;
+            }
+
             var submitSuccessCallback = function(o) {
                 // reload page if we change status for single commit.
                 if (status && self.commitId) {
@@ -359,6 +375,7 @@ var bindToggleButtons = function() {
                 // submit button, but only on Main form, isInline means inline
                 submitState = false
             }
+
             $(this.submitButton).prop('disabled', submitState);
             if (submitEvent) {
               $(this.submitButton).val(_gettext('Submitting...'));

rhodecode/templates/changeset/changeset_file_comment.mako

@@ -385,6 +385,11 @@
                 ${_('Cancel')}
                 </button>
             % endif
+
+            % if form_type != 'inline':
+                <div class="action-buttons-extra"></div>
+            % endif
+
             ${h.submit('save', _('Comment'), class_='btn btn-success comment-button-input')}
 
         </div>

rhodecode/templates/pullrequests/pullrequest_merge_checks.mako

@@ -1,7 +1,6 @@
 
 <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>
@@ -46,5 +45,19 @@
           <input type="submit" value="${_('Login to Merge this Pull Request')}" class="btn disabled" disabled="disabled">
         % endif
     </div>
-</div>
 
+    % if c.allowed_to_close:
+        ## close PR action, injected later next to COMMENT button
+        <div id="close-pull-request-action" style="display: none">
+        % if c.pull_request_review_status == c.REVIEW_STATUS_APPROVED:
+        <a class="btn btn-approved-status" href="#close-as-approved" onclick="closePullRequest('${c.REVIEW_STATUS_APPROVED}'); return false;">
+            ${_('Close with status {}').format(h.commit_status_lbl(c.REVIEW_STATUS_APPROVED))}
+        </a>
+        % else:
+        <a class="btn btn-rejected-status" href="#close-as-rejected" onclick="closePullRequest('${c.REVIEW_STATUS_REJECTED}'); return false;">
+            ${_('Close with status {}').format(h.commit_status_lbl(c.REVIEW_STATUS_REJECTED))}
+        </a>
+        % endif
+        </div>
+    % endif
+</div>

rhodecode/templates/pullrequests/pullrequest_show.mako

@@ -694,13 +694,33 @@
             refreshMergeChecks = function(){
                 var loadUrl = "${h.url.current(merge_checks=1)}";
                 $('.pull-request-merge').css('opacity', 0.3);
+                $('.action-buttons-extra').css('opacity', 0.3);
+
                 $('.pull-request-merge').load(
-                    loadUrl,function() {
+                    loadUrl, function() {
                         $('.pull-request-merge').css('opacity', 1);
+
+                        $('.action-buttons-extra').css('opacity', 1);
+                        injectCloseAction();
                     }
                 );
             };
 
+            injectCloseAction =  function() {
+                var closeAction = $('#close-pull-request-action').html();
+                var $actionButtons = $('.action-buttons-extra');
+                // clear the action before
+                $actionButtons.html("");
+                $actionButtons.html(closeAction);
+            };
+
+            closePullRequest = function (status) {
+                // inject closing flag
+                $('.action-buttons-extra').append('<input type="hidden" class="close-pr-input" id="close_pull_request" value="1">');
+                $(generalCommentForm.statusChange).select2("val", status).trigger('change');
+                $(generalCommentForm.submitForm).submit();
+            };
+
             $('#show-outdated-comments').on('click', function(e){
                 var button = $(this);
                 var outdated = $('.comment-outdated');
@@ -786,6 +806,8 @@
             window.commentFormGlobalSubmitSuccessCallback = function(){
                 refreshMergeChecks();
             };
+            // initial injection
+            injectCloseAction();
 
         })
       </script>

rhodecode/tests/functional/test_pullrequests.py

@@ -115,8 +115,10 @@ class TestPullrequestsController:
             repo_name=pull_request.target_repo.scm_instance().name,
             pull_request_id=str(pull_request.pull_request_id)))
 
-        assert 'Server-side pull request merging is disabled.' in response
-        assert 'value="forced_closed"' in response
+        response.mustcontain('Server-side pull request merging is disabled.')
+
+        assert_response = response.assert_response()
+        assert_response.one_element_exists('#close-pull-request-action')
 
     def test_show_invalid_commit_id(self, pr_util):
         # Simulating invalid revisions which will cause a lookup error
@@ -242,10 +244,9 @@ class TestPullrequestsController:
                 repo_name=pull_request.target_repo.scm_instance().name,
                 pull_request_id=str(pull_request_id)),
             params={
-                'changeset_status':
-                    ChangesetStatus.STATUS_APPROVED + '_closed',
-                'change_changeset_status': 'on',
-                'text': '',
+                'changeset_status': ChangesetStatus.STATUS_APPROVED,
+                'close_pull_request': '1',
+                'text': 'Closing a PR',
                 'csrf_token': csrf_token},
             status=302)
 
@@ -257,6 +258,14 @@ class TestPullrequestsController:
             .all()
         assert len(journal) == 1
 
+        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_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
@@ -291,7 +300,8 @@ class TestPullrequestsController:
                 repo_name=pull_request.target_repo.scm_instance().name,
                 pull_request_id=str(pull_request_id)),
             params={
-                'changeset_status': 'forced_closed',
+                'changeset_status': 'rejected',
+                'close_pull_request': '1',
                 'csrf_token': csrf_token},
             status=302)