api: comment_pull_request, added commit_id parameter to validate status changed on particular commit.
- when using the API and creating a comment with status change it's now possible to pass in commit_id, this will allow validation if status change of pull_request is allowed based on the given commit_id. This solves the case when long running test sends approval of pull request which was already updated several times. The commit_id will now validate for which state the approval was made, and prevent accidental aproval of outdated pull requests.
This commit is contained in:
Marcin Kuzminski
parent
3761402390
commit
75ef1f61eb
2 changed files with 102 additions and 16 deletions
|
|
@ -58,7 +58,7 @@ class TestCommentPullRequest(object):
|
|||
expected = {
|
||||
'pull_request_id': pull_request.pull_request_id,
|
||||
'comment_id': comments[-1].comment_id,
|
||||
'status': None
|
||||
'status': {'given': None, 'was_changed': None}
|
||||
}
|
||||
assert_ok(id_, expected, response.body)
|
||||
|
||||
|
|
@ -88,7 +88,56 @@ class TestCommentPullRequest(object):
|
|||
expected = {
|
||||
'pull_request_id': pull_request.pull_request_id,
|
||||
'comment_id': comments[-1].comment_id,
|
||||
'status': 'rejected'
|
||||
'status': {'given': 'rejected', 'was_changed': True}
|
||||
}
|
||||
assert_ok(id_, expected, response.body)
|
||||
|
||||
@pytest.mark.backends("git", "hg")
|
||||
def test_api_comment_pull_request_change_status_with_specific_commit_id(
|
||||
self, pr_util, no_notifications):
|
||||
pull_request = pr_util.create_pull_request()
|
||||
pull_request_id = pull_request.pull_request_id
|
||||
latest_commit_id = 'test_commit'
|
||||
# inject additional revision, to fail test the status change on
|
||||
# non-latest commit
|
||||
pull_request.revisions = pull_request.revisions + ['test_commit']
|
||||
|
||||
id_, params = build_data(
|
||||
self.apikey, 'comment_pull_request',
|
||||
repoid=pull_request.target_repo.repo_name,
|
||||
pullrequestid=pull_request.pull_request_id,
|
||||
status='approved', commit_id=latest_commit_id)
|
||||
response = api_call(self.app, params)
|
||||
pull_request = PullRequestModel().get(pull_request_id)
|
||||
|
||||
expected = {
|
||||
'pull_request_id': pull_request.pull_request_id,
|
||||
'comment_id': None,
|
||||
'status': {'given': 'approved', 'was_changed': False}
|
||||
}
|
||||
assert_ok(id_, expected, response.body)
|
||||
|
||||
@pytest.mark.backends("git", "hg")
|
||||
def test_api_comment_pull_request_change_status_with_specific_commit_id(
|
||||
self, pr_util, no_notifications):
|
||||
pull_request = pr_util.create_pull_request()
|
||||
pull_request_id = pull_request.pull_request_id
|
||||
latest_commit_id = pull_request.revisions[0]
|
||||
|
||||
id_, params = build_data(
|
||||
self.apikey, 'comment_pull_request',
|
||||
repoid=pull_request.target_repo.repo_name,
|
||||
pullrequestid=pull_request.pull_request_id,
|
||||
status='approved', commit_id=latest_commit_id)
|
||||
response = api_call(self.app, params)
|
||||
pull_request = PullRequestModel().get(pull_request_id)
|
||||
|
||||
comments = ChangesetCommentsModel().get_comments(
|
||||
pull_request.target_repo.repo_id, pull_request=pull_request)
|
||||
expected = {
|
||||
'pull_request_id': pull_request.pull_request_id,
|
||||
'comment_id': comments[-1].comment_id,
|
||||
'status': {'given': 'approved', 'was_changed': True}
|
||||
}
|
||||
assert_ok(id_, expected, response.body)
|
||||
|
||||
|
|
@ -103,7 +152,7 @@ class TestCommentPullRequest(object):
|
|||
pullrequestid=pull_request_id)
|
||||
response = api_call(self.app, params)
|
||||
|
||||
expected = 'message and status parameter missing'
|
||||
expected = 'Both message and status parameters are missing. At least one is required.'
|
||||
assert_error(id_, expected, given=response.body)
|
||||
|
||||
@pytest.mark.backends("git", "hg")
|
||||
|
|
@ -118,7 +167,7 @@ class TestCommentPullRequest(object):
|
|||
status='42')
|
||||
response = api_call(self.app, params)
|
||||
|
||||
expected = 'unknown comment status`42`'
|
||||
expected = 'Unknown comment status: `42`'
|
||||
assert_error(id_, expected, given=response.body)
|
||||
|
||||
@pytest.mark.backends("git", "hg")
|
||||
|
|
@ -144,3 +193,17 @@ class TestCommentPullRequest(object):
|
|||
|
||||
expected = 'userid is not the same as your user'
|
||||
assert_error(id_, expected, given=response.body)
|
||||
|
||||
@pytest.mark.backends("git", "hg")
|
||||
def test_api_comment_pull_request_wrong_commit_id_error(self, pr_util):
|
||||
pull_request = pr_util.create_pull_request()
|
||||
id_, params = build_data(
|
||||
self.apikey_regular, 'comment_pull_request',
|
||||
repoid=pull_request.target_repo.repo_name,
|
||||
status='approved',
|
||||
pullrequestid=pull_request.pull_request_id,
|
||||
commit_id='XXX')
|
||||
response = api_call(self.app, params)
|
||||
|
||||
expected = 'Invalid commit_id `XXX` for this pull request.'
|
||||
assert_error(id_, expected, given=response.body)
|
||||
|
|
|
|||
|
|
@ -361,6 +361,7 @@ def close_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
@jsonrpc_method()
|
||||
def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
||||
message=Optional(None), status=Optional(None),
|
||||
commit_id=Optional(None),
|
||||
userid=Optional(OAttr('apiuser'))):
|
||||
"""
|
||||
Comment on the pull request specified with the `pullrequestid`,
|
||||
|
|
@ -382,6 +383,10 @@ def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
* rejected
|
||||
* under_review
|
||||
:type status: str
|
||||
:param commit_id: Specify the commit_id for which to set a comment. If
|
||||
given commit_id is different than latest in the PR status
|
||||
change won't be performed.
|
||||
:type commit_id: str
|
||||
:param userid: Comment on the pull request as this user
|
||||
:type userid: Optional(str or int)
|
||||
|
||||
|
|
@ -393,7 +398,9 @@ def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
result :
|
||||
{
|
||||
"pull_request_id": "<Integer>",
|
||||
"comment_id": "<Integer>"
|
||||
"comment_id": "<Integer>",
|
||||
"status": {"given": <given_status>,
|
||||
"was_changed": <bool status_was_actually_changed> },
|
||||
}
|
||||
error : null
|
||||
"""
|
||||
|
|
@ -412,24 +419,42 @@ def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
raise JSONRPCError('repository `%s` does not exist' % (repoid,))
|
||||
message = Optional.extract(message)
|
||||
status = Optional.extract(status)
|
||||
commit_id = Optional.extract(commit_id)
|
||||
|
||||
if not message and not status:
|
||||
raise JSONRPCError('message and status parameter missing')
|
||||
raise JSONRPCError(
|
||||
'Both message and status parameters are missing. '
|
||||
'At least one is required.')
|
||||
|
||||
if (status not in (st[0] for st in ChangesetStatus.STATUSES) and
|
||||
status is not None):
|
||||
raise JSONRPCError('unknown comment status`%s`' % status)
|
||||
raise JSONRPCError('Unknown comment status: `%s`' % status)
|
||||
|
||||
if commit_id and commit_id not in pull_request.revisions:
|
||||
raise JSONRPCError(
|
||||
'Invalid commit_id `%s` for this pull request.' % commit_id)
|
||||
|
||||
allowed_to_change_status = PullRequestModel().check_user_change_status(
|
||||
pull_request, apiuser)
|
||||
|
||||
# if commit_id is passed re-validated if user is allowed to change status
|
||||
# based on latest commit_id from the PR
|
||||
if commit_id:
|
||||
commit_idx = pull_request.revisions.index(commit_id)
|
||||
if commit_idx != 0:
|
||||
allowed_to_change_status = False
|
||||
|
||||
text = message
|
||||
status_label = ChangesetStatus.get_status_lbl(status)
|
||||
if status and allowed_to_change_status:
|
||||
st_message = (('Status change %(transition_icon)s %(status)s')
|
||||
% {'transition_icon': '>',
|
||||
'status': ChangesetStatus.get_status_lbl(status)})
|
||||
st_message = ('Status change %(transition_icon)s %(status)s'
|
||||
% {'transition_icon': '>', 'status': status_label})
|
||||
text = message or st_message
|
||||
|
||||
rc_config = SettingsModel().get_all_settings()
|
||||
renderer = rc_config.get('rhodecode_markup_renderer', 'rst')
|
||||
|
||||
status_change = status and allowed_to_change_status
|
||||
comment = ChangesetCommentsModel().create(
|
||||
text=text,
|
||||
repo=pull_request.target_repo.repo_id,
|
||||
|
|
@ -437,10 +462,8 @@ def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
pull_request=pull_request.pull_request_id,
|
||||
f_path=None,
|
||||
line_no=None,
|
||||
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),
|
||||
status_change=(status_label if status_change else None),
|
||||
status_change_type=(status if status_change else None),
|
||||
closing_pr=False,
|
||||
renderer=renderer
|
||||
)
|
||||
|
|
@ -458,8 +481,8 @@ def comment_pull_request(request, apiuser, repoid, pullrequestid,
|
|||
Session().commit()
|
||||
data = {
|
||||
'pull_request_id': pull_request.pull_request_id,
|
||||
'comment_id': comment.comment_id,
|
||||
'status': status
|
||||
'comment_id': comment.comment_id if comment else None,
|
||||
'status': {'given': status, 'was_changed': status_change},
|
||||
}
|
||||
return data
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue