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

[Mads Kiilerich]

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)...))

> +    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?

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

/Mads