[PATCH 4 of 7] changeset: draft comment on changesets

[Mads Kiilerich]

On 04/14/2015 06:03 PM, Jan Heylen wrote:
> # HG changeset patch
> # User Jan Heylen <heyleke at gmail.com>
> # Date 1427526729 -3600
> #      Sat Mar 28 08:12:09 2015 +0100
> # Node ID f26b18234a7731a361f75e7a68ad143fc78218bf
> # Parent  f48bc03627761dd62f7e7094bb0d322834da336c
> changeset: draft comment on changesets

This doesn't include any template changes at all so I doubt that 
description is exact?

>
> diff -r f48bc0362776 -r f26b18234a77 kallithea/controllers/changeset.py
> --- a/kallithea/controllers/changeset.py	Mon Mar 30 19:13:08 2015 +0200
> +++ b/kallithea/controllers/changeset.py	Sat Mar 28 08:12:09 2015 +0100
> @@ -222,11 +222,14 @@
>           comments = dict()
>           c.statuses = []
>           c.inline_comments = []
> +        c.drafts = []
>           c.inline_cnt = 0
> +        c.draft_cnt = 0
>   
>           # Iterate over ranges (default changeset view is always one changeset)
>           for changeset in c.cs_ranges:
>               inlines = []
> +            drafts = []
>               if method == 'show':
>                   c.statuses.extend([ChangesetStatusModel().get_status(
>                               c.db_repo.repo_id, changeset.raw_id)])
> @@ -247,6 +250,11 @@
>                               .get_inline_comments(c.db_repo.repo_id,
>                                                    revision=changeset.raw_id)
>                   c.inline_comments.extend(inlines)
> +                drafts = ChangesetCommentsModel()\
> +                            .get_inline_drafts(c.db_repo.repo_id,
> +                                                 revision=changeset.raw_id,
> +                                                 user_id=c.authuser.user_id)
> +                c.drafts.extend(drafts)
>   
>               c.changes[changeset.raw_id] = []
>   
> @@ -291,6 +299,11 @@
>               for comments in lines.values():
>                   c.inline_cnt += len(comments)
>   
> +        # count draft comments
> +        for __, lines in c.drafts:

I like to use a _ prefix for unused values but please give it a name 
that gives a hint what it is we don't use.

> +            for drafts in lines.values():
> +                c.draft_cnt += len(drafts)

This extra iteration is inefficient. Let's hope it doesn't touch 
unloaded objects where it would have to touch the db for each iteration. 
Unfortunately that is how we do it ...

> +
>           if len(c.cs_ranges) == 1:
>               c.changeset = c.cs_ranges[0]
>               c.parent_tmpl = ''.join(['# Parent  %s\n' % x.raw_id
> @@ -347,6 +360,39 @@
>       @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
>                                      'repository.admin')
>       @jsonify
> +    def draft(self, repo_name, revision=None, pull_request_id=None):

Please make sure new code is better than the existing code. Add 
docstrings that explains the intention with the function. That makes it 
much easier to review.

Please also consider adding test coverage for this new functionality.

> +        text = request.POST.get('text', '').strip() or _('No comments.')
> +        c.co = comm = ChangesetCommentsModel().create(

These two variable names are not pretty. I guess the excuse/explanation 
is that it is a cut'n'paste of existing code. But that is not really a 
good excuse/explanation ;-)

Could the existing code perhaps be cleaned up and reused?

> +            text=text,
> +            repo=c.db_repo.repo_id,
> +            user=c.authuser.user_id,
> +            revision=revision,
> +            pull_request=pull_request_id,
> +            f_path=request.POST.get('f_path'),
> +            line_no=request.POST.get('line'),
> +            status_change=None,
> +            draft=True
> +        )
> +
> +        Session().commit()
> +
> +        #only ajax below
> +        data = {
> +           'target_id': h.safeid(h.safe_unicode(request.POST.get('f_path'))),
> +        }
> +        if comm:
> +            data.update(comm.get_dict())
> +            data.update({'rendered_text':
> +                         render('changeset/changeset_comment_block.html')})
> +
> +        return data
> +
> +
> +    @LoginRequired()
> +    @NotAnonymous()
> +    @HasRepoPermissionAnyDecorator('repository.read', 'repository.write',
> +                                   'repository.admin')
> +    @jsonify
>       def comment(self, repo_name, revision):
>           status = request.POST.get('changeset_status')
>           text = request.POST.get('text', '').strip() or _('No comments.')
> @@ -359,7 +405,8 @@
>               f_path=request.POST.get('f_path'),
>               line_no=request.POST.get('line'),
>               status_change=(ChangesetStatus.get_status_lbl(status)
> -                           if status else None)
> +                           if status else None),
> +            draft=True

It is always nice to leave a comma after the last entry of a list (if at 
the end of the line) - that makes it easier to add and review more 
entries later on.

>           )
>   

/Mads