engineering/unrhodecode
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
- consistent logic via web and API
- updated events
- updated proper close checks.
This commit is contained in:
Marcin Kuzminski 2017-06-13 00:14:21 +02:00
parent f11a476967
commit 5915f38fcf
12 changed files with 235 additions and 222 deletions
Download patch file Download diff file Expand all files Collapse all files

1
rhodecode/api/tests/test_close_pull_request.py
View file

@ -43,6 +43,7 @@ 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)

154
rhodecode/api/views/pull_request_api.py
View file

@ -21,6 +21,7 @@
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 @@ from rhodecode.model.db import Session, ChangesetStatus, ChangesetComment
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,8 +228,9 @@ def get_pull_requests(request, apiuser, repoid, status=Optional('new')):
@jsonrpc_method()
def merge_pull_request(request, apiuser, repoid, pullrequestid,
userid=Optional(OAttr('apiuser'))):
def merge_pull_request(
request, apiuser, repoid, pullrequestid,
userid=Optional(OAttr('apiuser'))):
"""
Merge the pull request specified by `pullrequestid` into its target
repository.
@ -307,63 +309,6 @@ def merge_pull_request(request, apiuser, repoid, pullrequestid,
return merge_response
@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),
@ -610,7 +555,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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

1
rhodecode/controllers/changeset.py
View file

@ -368,7 +368,6 @@ class ChangesetController(BaseRepoController):
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:

163
rhodecode/controllers/pullrequests.py
View file

@ -306,8 +306,6 @@ class PullrequestsController(BaseRepoController):
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 @@ class PullrequestsController(BaseRepoController):
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 @@ class PullrequestsController(BaseRepoController):
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,83 +887,78 @@ class PullrequestsController(BaseRepoController):
resolves_comment_id = request.POST.get('resolves_comment_id', None)
close_pull_request = request.POST.get('close_pull_request')
close_pr = False
# only owner or admin or person with write permissions
allowed_to_close = PullRequestModel().check_user_update(
pull_request, c.rhodecode_user)
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
allowed_to_change_status = PullRequestModel().check_user_change_status(
pull_request, c.rhodecode_user)
if status and allowed_to_change_status:
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(
text=text,
repo=c.rhodecode_db_repo.repo_id,
user=c.rhodecode_user.user_id,
pull_request=pull_request_id,
f_path=request.POST.get('f_path'),
line_no=request.POST.get('line'),
status_change=(ChangesetStatus.get_status_lbl(status)
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:
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,
pull_request=pull_request_id
)
# 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, comm))
# 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()
if old_calculated_status != calculated_status:
PullRequestModel()._trigger_pull_request_hook(
pull_request, c.rhodecode_user, 'review_status_change')
events.trigger(
events.PullRequestCommentEvent(pull_request, comment))
calculated_status_lbl = ChangesetStatus.get_status_lbl(
calculated_status)
else:
# regular comment case, could be inline, or one with status.
# for that one we check also permissions
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')
allowed_to_change_status = PullRequestModel().check_user_change_status(
pull_request, c.rhodecode_user)
if status and allowed_to_change_status:
message = (_('Status change %(transition_icon)s %(status)s')
% {'transition_icon': '>',
'status': ChangesetStatus.get_status_lbl(status)})
text = text or message
comment = CommentsModel().create(
text=text,
repo=c.rhodecode_db_repo.repo_id,
user=c.rhodecode_user.user_id,
pull_request=pull_request_id,
f_path=request.POST.get('f_path'),
line_no=request.POST.get('line'),
status_change=(ChangesetStatus.get_status_lbl(status)
if status and allowed_to_change_status else None),
status_change_type=(status
if status and allowed_to_change_status else None),
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,
comment,
pull_request=pull_request_id
)
Session().flush()
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()
if old_calculated_status != calculated_status:
PullRequestModel()._trigger_pull_request_hook(
pull_request, c.rhodecode_user, 'review_status_change')
Session().commit()
@ -983,12 +969,11 @@ class PullrequestsController(BaseRepoController):
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

4
rhodecode/model/db.py
View file

@ -3107,6 +3107,10 @@ 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)

52
rhodecode/model/pull_request.py
View file

@ -34,6 +34,7 @@ from pylons.i18n.translation import lazy_ugettext
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 @@ 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):
if not message:
message = (
_('Status change %(transition_icon)s %(status)s') % {
'transition_icon': '>',
'status': ChangesetStatus.get_status_lbl(status)})
pull_request_review_status = pull_request.calculated_review_status()
internal_message = _('Closing with') + ' ' + message
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)
comm = CommentsModel().create(
text=internal_message,
default_message = (
_('Closing with status change {transition_icon} {status}.')
).format(transition_icon='>', status=status_lbl)
text = message or default_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()
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.')

1
rhodecode/public/js/rhodecode/routes.js
View file

@ -96,6 +96,7 @@ 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']);

10
rhodecode/public/js/src/rhodecode/pullrequests.js
View file

@ -367,16 +367,6 @@ var _updatePullRequest = function(repo_name, pull_request_id, postData) {
ajaxPOST(url, postData, success);
};
/**
* 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
*/

2
rhodecode/templates/changeset/changeset_comment_block.mako
View file

@ -1,4 +1,4 @@
## 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)}

3
rhodecode/templates/debug_style/collapsable-content.html
View file

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

5
rhodecode/templates/pullrequests/pullrequest_show.mako
View file

@ -825,11 +825,6 @@
// 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);

61
rhodecode/tests/functional/test_pullrequests.py
View file

@ -237,7 +237,9 @@ 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 @@ 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 @@ class TestPullrequestsController(object):
status = ChangesetStatusModel().get_status(
pull_request.source_repo, pull_request=pull_request)
assert status == ChangesetStatus.STATUS_APPROVED
assert pull_request.comments[-1].text == 'Closing a PR'
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})
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 @@ 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'},