[PATCH 06 of 14] controllers: forward pullrequests.delete_comment to changeset

[Thomas De Schampheleire]

El mié., 21 nov. 2018 a las 12:11, 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 1541709517 -3600
> > #      Thu Nov 08 21:38:37 2018 +0100
> > # Node ID 7350230aa3578c3fc3a4aff864e35a02eea216e1
> > # Parent  57b9fe6c7871e1ee74eca9c6723ccc4a9dfd5c77
> > controllers: forward pullrequests.delete_comment to changeset
> >
> > Remove duplication between pullrequests and changeset.
> > We move the code outside ChangesetController to make it callable from
> > PullrequestsController.
> >
> > Note:
> > - instead of keeping the method pullrequests.delete_comment itself and
> >    letting it forward to changeset.delete_comment, an alternative solution
> >    would have been to change the routing table directly. However, the chosen
> >    solution makes it more explicit which operations are supported on each
> >    controller.
> >
> > diff --git a/kallithea/controllers/changeset.py b/kallithea/controllers/changeset.py
> > --- a/kallithea/controllers/changeset.py
> > +++ b/kallithea/controllers/changeset.py
> > @@ -187,6 +187,22 @@ def create_comment(text, status, f_path,
> >
> >       return comment
> >
> > +def delete_cs_pr_comment(repo_name, comment_id, pr_comment=False):
> > +    co = ChangesetComment.get_or_404(comment_id)
>
> (It would be nice to have more consistent "default" naming of variables
> if different types, both in code and tests. "co" for ChangesetComment
> doesn't seem obvious. "cc" seems like a better mnemonic. (But also,
> ChangesetComment is perhaps badly named after it also is used for PRs
> (which also are badly named)...))

The 'co' is for 'comment' and also used in templates and model:
kallithea/templates/changeset/changeset_file_comment.html
kallithea/model/comment.py

so in that sense it is actually consistent naming.
But I'm fine with other names, like 'comment' in full.


>
> > +    if co.repo.repo_name != repo_name:
> > +        raise HTTPNotFound()
> > +    if pr_comment and co.pull_request.is_closed():
>
> Do we need this pr_comment flag? Can't we just check co.pull_request?

You're right, I'll remove it...

>
> > +        # don't allow deleting comments on closed pull request
> > +        raise HTTPForbidden()
>
> /Mads