diff --git a/rhodecode/controllers/pullrequests.py b/rhodecode/controllers/pullrequests.py --- a/rhodecode/controllers/pullrequests.py +++ b/rhodecode/controllers/pullrequests.py @@ -69,6 +69,8 @@ class PullrequestsController(BaseRepoCon 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(BaseRepoCon 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(BaseRepoCon 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(BaseRepoCon 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(BaseRepoCon # 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(BaseRepoCon # 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(BaseRepoCon 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(BaseRepoCon 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: 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 @@ -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.") diff --git a/rhodecode/public/css/buttons.less b/rhodecode/public/css/buttons.less --- a/rhodecode/public/css/buttons.less +++ b/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 { diff --git a/rhodecode/public/css/comments.less b/rhodecode/public/css/comments.less --- a/rhodecode/public/css/comments.less +++ b/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 { diff --git a/rhodecode/public/js/src/rhodecode/comments.js b/rhodecode/public/js/src/rhodecode/comments.js --- a/rhodecode/public/js/src/rhodecode/comments.js +++ b/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...')); diff --git a/rhodecode/templates/changeset/changeset_file_comment.mako b/rhodecode/templates/changeset/changeset_file_comment.mako --- a/rhodecode/templates/changeset/changeset_file_comment.mako +++ b/rhodecode/templates/changeset/changeset_file_comment.mako @@ -385,6 +385,11 @@ ${_('Cancel')} % endif + + % if form_type != 'inline': +
+ % endif + ${h.submit('save', _('Comment'), class_='btn btn-success comment-button-input')} 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,7 +1,6 @@
- % if c.pr_merge_possible:

@@ -46,5 +45,19 @@ % endif

+ + % if c.allowed_to_close: + ## close PR action, injected later next to COMMENT button + + % endif - diff --git a/rhodecode/templates/pullrequests/pullrequest_show.mako b/rhodecode/templates/pullrequests/pullrequest_show.mako --- a/rhodecode/templates/pullrequests/pullrequest_show.mako +++ b/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(''); + $(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(); }) 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 @@ -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)