[PATCH 12 of 14] controllers: align pullrequests.comment with changeset.comment
Mads Kiilerich
mads at kiilerich.com
Wed Nov 21 11:48:20 UTC 2018
On 11/20/2018 09:32 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
> # Date 1542402570 -3600
> # Fri Nov 16 22:09:30 2018 +0100
> # Node ID 4e6bfd3650ddfaf9d2bfbb166eaaec15f5194135
> # Parent abef10c5c0d21af567d76d6ffaf5356cc3c3be81
> controllers: align pullrequests.comment with changeset.comment
>
> This commit purely serves to highlight the differences.
> The subsequent commit will remove the duplication.
>
> diff --git a/kallithea/controllers/changeset.py b/kallithea/controllers/changeset.py
> --- a/kallithea/controllers/changeset.py
> +++ b/kallithea/controllers/changeset.py
> @@ -365,48 +365,87 @@ class ChangesetController(BaseRepoContro
> @LoginRequired()
> @HasRepoPermissionLevelDecorator('read')
> @jsonify
> - def comment(self, repo_name, revision):
> + def comment(self, repo_name, revision, pull_request_id=None, allowed_to_change_status=True):
(When extracting this as function later on, it would be nice to get a
docstring.)
> assert request.environ.get('HTTP_X_PARTIAL_XHR')
>
> + if pull_request_id is not None:
> + pull_request = PullRequest.get_or_404(pull_request_id)
> + else:
> + pull_request = None
> +
> status = request.POST.get('changeset_status')
> + close_pr = request.POST.get('save_close')
> + delete = request.POST.get('save_delete')
> f_path = request.POST.get('f_path')
> line_no = request.POST.get('line')
>
> - if status and (f_path or line_no):
> - # status votes are only possible in general comments
> + if (status or close_pr or delete) and (f_path or line_no):
> + # status votes and closing is only possible in general comments
> raise HTTPBadRequest()
>
> + if not allowed_to_change_status:
> + if status or close_pr:
> + h.flash(_('No permission to change status'), 'error')
> + raise HTTPForbidden()
> +
> + if pull_request and delete == "delete":
> + if (pull_request.owner_id == request.authuser.user_id or
> + h.HasPermissionAny('hg.admin')() or
> + h.HasRepoPermissionLevel('admin')(pull_request.org_repo.repo_name) or
> + h.HasRepoPermissionLevel('admin')(pull_request.other_repo.repo_name)
> + ) and not pull_request.is_closed():
> + PullRequestModel().delete(pull_request)
> + Session().commit()
> + h.flash(_('Successfully deleted pull request %s') % pull_request_id,
> + category='success')
> + return {
> + 'location': url('my_pullrequests'), # or repo pr list?
> + }
> + raise HTTPFound(location=url('my_pullrequests')) # or repo pr list?
> + raise HTTPForbidden()
> +
> text = request.POST.get('text', '').strip()
>
> - c.comment = create_comment(
> + comment = create_comment(
> text,
> status,
> revision=revision,
> + pull_request_id=pull_request_id,
> f_path=f_path,
> line_no=line_no,
> + closing_pr=close_pr,
> )
>
> - # get status if set !
> if status:
> ChangesetStatusModel().set_status(
> c.db_repo.repo_id,
> status,
> request.authuser.user_id,
> - c.comment,
> + comment,
> revision=revision,
> + pull_request=pull_request_id,
> )
>
> - action_logger(request.authuser,
> - 'user_commented_revision:%s' % revision,
> - c.db_repo, request.ip_addr)
> + if pull_request:
> + action = 'user_closed_pull_request:%s' % pull_request_id
> + else:
> + action = 'user_commented_revision:%s' % revision
> + action_logger(request.authuser, action, c.db_repo, request.ip_addr)
> +
> + if pull_request and close_pr:
> + PullRequestModel().close_pull_request(pull_request_id)
> + action_logger(request.authuser,
> + 'user_closed_pull_request:%s' % pull_request_id,
> + c.db_repo, request.ip_addr)
>
> Session().commit()
>
> data = {
> 'target_id': h.safeid(h.safe_unicode(request.POST.get('f_path'))),
> }
> - if c.comment is not None:
> - data.update(c.comment.get_dict())
> + if comment is not None:
> + c.comment = comment
> + data.update(comment.get_dict())
> data.update({'rendered_text':
> render('changeset/changeset_comment_block.html')})
>
> diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py
> --- a/kallithea/controllers/pullrequests.py
> +++ b/kallithea/controllers/pullrequests.py
> @@ -631,9 +631,13 @@ class PullrequestsController(BaseRepoCon
> @LoginRequired()
> @HasRepoPermissionLevelDecorator('read')
> @jsonify
> - def comment(self, repo_name, pull_request_id):
> + def comment(self, repo_name, pull_request_id, revision=None):
Something is fishy about adding this unused parameter and not removing
it again.
> assert request.environ.get('HTTP_X_PARTIAL_XHR')
> - pull_request = PullRequest.get_or_404(pull_request_id)
> +
> + if pull_request_id is not None:
> + pull_request = PullRequest.get_or_404(pull_request_id)
> + else:
> + pull_request = None
>
> status = request.POST.get('changeset_status')
> close_pr = request.POST.get('save_close')
> @@ -648,10 +652,10 @@ class PullrequestsController(BaseRepoCon
> allowed_to_change_status = self._is_allowed_to_change_status(pull_request)
> if not allowed_to_change_status:
> if status or close_pr:
> - h.flash(_('No permission to change pull request status'), 'error')
> + h.flash(_('No permission to change status'), 'error')
> raise HTTPForbidden()
>
> - if delete == "delete":
> + if pull_request and delete == "delete":
(Generally, I prefer to use the slightly more verbose "is not None"
instead of relying on falsiness of None ... and thus the assumption that
objects never are "false" in other ways. But I'm also lazy and would
perhaps do it this way ...)
> if (pull_request.owner_id == request.authuser.user_id or
> h.HasPermissionAny('hg.admin')() or
> h.HasRepoPermissionLevel('admin')(pull_request.org_repo.repo_name) or
> @@ -672,26 +676,30 @@ class PullrequestsController(BaseRepoCon
> comment = create_comment(
> text,
> status,
> + revision=revision,
> pull_request_id=pull_request_id,
> f_path=f_path,
> line_no=line_no,
> closing_pr=close_pr,
> )
>
> - action_logger(request.authuser,
> - 'user_commented_pull_request:%s' % pull_request_id,
> - c.db_repo, request.ip_addr)
> -
> if status:
> ChangesetStatusModel().set_status(
> c.db_repo.repo_id,
> status,
> request.authuser.user_id,
> comment,
> - pull_request=pull_request_id
> + revision=revision,
> + pull_request=pull_request_id,
> )
>
> - if close_pr:
> + if pull_request:
> + action = 'user_closed_pull_request:%s' % pull_request_id
'user_commented_pull_request' ... so apparently no coverage of this
logging ... but no big deal ...
> + else:
> + action = 'user_commented_revision:%s' % revision
> + action_logger(request.authuser, action, c.db_repo, request.ip_addr)
> +
> + if pull_request and close_pr:
> PullRequestModel().close_pull_request(pull_request_id)
> action_logger(request.authuser,
> 'user_closed_pull_request:%s' % pull_request_id,
/Mads
More information about the kallithea-general
mailing list