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

[Jan Heylen]

On Fri, Apr 24, 2015 at 7:21 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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?

Changeset is a controller? I'll add "(model)" in the commit message?

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

I must admit I looked at this line for quite some time before I got
the __ (as it is a copy), I'll add a proper name

>
>> +            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 ...
comment_cnt is also there, Is that not needed neither?

>
>> +
>>           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.
Good idea, I'll consider

>
> Please also consider adding test coverage for this new functionality.

Yes, I consider this, but I first wanted to get a feeling (and I still
do) on how feasible it is to get this patch series upstream (after
adding tests).

>
>> +        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 ;-)
Good guess, refactor is your answer.

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

ok

>
>>           )
>>
>
>
> /Mads