[PATCH 12 of 14] controllers: align pullrequests.comment with changeset.comment
Thomas De Schampheleire
patrickdepinguin at gmail.com
Thu Nov 22 20:25:11 UTC 2018
El mié., 21 nov. 2018 a las 12:48, Mads Kiilerich
(<mads at kiilerich.com>) escribió:
>
> 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.)
Ok. I added something but not sure if it's the type of docstring you
expect. Feel free to change it.
>
> > 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.
Oops :-o
>
> > 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 ...)
For pull request ids, 'is not None' is needed because '0' could be a
valid pull request id. But 'pull_request' is the object from the
model, so either it exists or it does not.
I'm fine adding 'is not None' explicitly, but I think we should then
add this guideline to our coding rules in the manual.
Let me know your conclusion...
>
> > 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 ...
Here I'm not sure what you mean: this log string is effectively for
closing the PR so the text seems correct?
> > + 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