[PATCH 6 of 7] frontend: change comment workflow

[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 1428986395 -7200
>> #      Tue Apr 14 06:39:55 2015 +0200
>> # Node ID b1b3c859230ce5997f7820788dd331748e277525
>> # Parent  48a64173a2187e3ea2822df54b3d0096176e76e3
>> frontend: change comment workflow
>>
>> This changes the workflow from one-comment-at-a-time to
>> giving-draft-comments-and-a-final-judgement.
>> Only after the final judgement, the drafts are changed into comments and
>> an email notification is send.
>> The internal notification model is untouched.
>>
>> diff -r 48a64173a218 -r b1b3c859230c kallithea/config/routing.py
>> --- a/kallithea/config/routing.py       Sat Mar 28 08:12:49 2015 +0100
>> +++ b/kallithea/config/routing.py       Tue Apr 14 06:39:55 2015 +0200
>> @@ -662,6 +662,16 @@
>>                   controller='changeset', action='preview_comment',
>>                   conditions=dict(function=check_repo, method=["POST"]))
>>   +    rmap.connect('changeset_comment_draft',
>> +                 '/{repo_name:.*?}/changeset-comment-draft/{revision}',
>> +                controller='changeset', revision='tip', action='draft',
>> +                conditions=dict(function=check_repo, method=["POST"]))
>> +
>> +    rmap.connect('pullrequest_comment_draft',
>> +
>> '/{repo_name:.*?}/pullrequest-comment-draft/{pull_request_id}',
>> +                controller='changeset', action='draft',
>> +                conditions=dict(function=check_repo, method=["POST"]))
>
>
> It could make sense to hook these up when they are implemented on the server
> side, leaving this changeset to be a client side change.

Patch 1-6 implement the server side? I don't understand what you want
to say here...

>
>
>> +
>>       rmap.connect('changeset_comment_delete',
>>
>> '/{repo_name:.*?}/changeset-comment-delete/{comment_id}',
>>                   controller='changeset', action='delete_comment',
>> diff -r 48a64173a218 -r b1b3c859230c kallithea/public/js/base.js
>> --- a/kallithea/public/js/base.js       Sat Mar 28 08:12:49 2015 +0100
>> +++ b/kallithea/public/js/base.js       Tue Apr 14 06:39:55 2015 +0200
>> @@ -706,6 +706,7 @@
>>           $('#edit-btn_'+lineno).show();
>>           $('#preview-container_'+lineno).show();
>>           $('#preview-btn_'+lineno).hide();
>> +        $('#draft-btn_'+lineno).hide();
>>             var url = pyroutes.url('changeset_comment_preview',
>> {'repo_name': REPO_NAME});
>>           var post_data = {'text': text};
>> @@ -714,11 +715,37 @@
>>               $('#preview-box_'+lineno).removeClass('unloaded');
>>           })
>>       })
>> +    $('#draft-btn_'+lineno).click(function(e){
>> +        var text = $('#text_'+lineno).val();
>> +        if(!text){
>> +            return
>> +        }
>> +        $('#preview-box_'+lineno).addClass('unloaded');
>> +        $('#preview-box_'+lineno).html(_TM['Loading ...']);
>> +        $('#edit-container_'+lineno).hide();
>> +        $('#edit-btn_'+lineno).show();
>> +        $('#preview-container_'+lineno).show();
>> +        $('#draft-btn_'+lineno).hide();
>> +        $('#preview-btn_'+lineno).hide();
>> +
>> +        var url = AJAX_DRAFT_URL;
>> +        var post_data = {
>> +                'text': text,
>> +                'f_path': f_path,
>> +                'line': lineno
>> +        };
>> +        ajaxPOST(url, post_data, function(json_data){
>> +            $tr.removeClass('form-open');
>> +            $form.remove();
>> +            _renderInlineComment(json_data);
>> +        })
>> +    })
>>       $('#edit-btn_'+lineno).click(function(e){
>>           $('#edit-container_'+lineno).show();
>>           $('#edit-btn_'+lineno).hide();
>>           $('#preview-container_'+lineno).hide();
>>           $('#preview-btn_'+lineno).show();
>> +        $('#draft-btn_'+lineno).show();
>>       })
>>         setTimeout(function(){
>> diff -r 48a64173a218 -r b1b3c859230c
>> kallithea/templates/changeset/changeset.html
>> --- a/kallithea/templates/changeset/changeset.html      Sat Mar 28
>> 08:12:49 2015 +0100
>> +++ b/kallithea/templates/changeset/changeset.html      Tue Apr 14
>> 06:39:55 2015 +0200
>> @@ -28,6 +28,7 @@
>>       var _GROUPS_AC_DATA = ${c.user_groups_array|n};
>>       AJAX_COMMENT_URL =
>> "${url('changeset_comment',repo_name=c.repo_name,revision=c.changeset.raw_id)}";
>>       AJAX_COMMENT_DELETE_URL =
>> "${url('changeset_comment_delete',repo_name=c.repo_name,comment_id='__COMMENT_ID__')}";
>> +    AJAX_DRAFT_URL =
>> "${url('changeset_comment_draft',repo_name=c.repo_name,revision=c.changeset.raw_id)}";
>
>
> Many things could be draft. It would be nice if the name made it clear it
> was a draft comment.
True, Thomas also gave that comment.

>
>>       </script>
>>       <div class="table">
>>           <div class="diffblock">
>> diff -r 48a64173a218 -r b1b3c859230c
>> kallithea/templates/changeset/changeset_file_comment.html
>> --- a/kallithea/templates/changeset/changeset_file_comment.html Sat Mar 28
>> 08:12:49 2015 +0100
>> +++ b/kallithea/templates/changeset/changeset_file_comment.html Tue Apr 14
>> 06:39:55 2015 +0200
>> @@ -24,14 +24,22 @@
>>                 ${_('Status change from pull request')}
>>                 <a
>> href="${co.pull_request.url()}">"${co.pull_request.title or _("No
>> title")}"</a>:
>>               %else:
>> -              ${_('Comment from pull request')}
>> +              %if co.draft:
>> +                     ${_('Draft from pull request')}
>
>
> ./whitespacecleanup.sh will fix the indentation here ;-)
>
> Anyway, I guess this touches upon how wrong the datamodel is with PR
> comments being duplicated for each changeset. It is sad to do that already
> for the draft comments which probably are likely to change more often ...
> and be draft. I guess it is hard to make it any cleaner without changing the
> datamodel. Which is why I would like to get the datamodel cleaned up before
> making it worse by building more on top of it.

Again, please a yes or a no. I'm not going to redraw the datamodel,
that is out of my league...

>
>> +              %else:
>> +                ${_('Comment from pull request')}
>> +              %endif
>>                 <a
>> href="${co.pull_request.url()}">"${co.pull_request.title or _("No
>> title")}"</a>
>>               %endif
>>            %else:
>>               %if co.status_change:
>>                 ${_('Status change on changeset')}:
>>               %else:
>> -              ${_('Comment on changeset')}
>> +              %if co.draft:
>> +                ${_('Draft on changeset')}
>> +              %else:
>> +                ${_('Comment on changeset')}
>> +              %endif
>>               %endif
>>            %endif
>>           </span>
>> @@ -85,8 +93,9 @@
>>           <div class="submitting-overlay">${_('Submitting ...')}</div>
>>           <input type="hidden" name="f_path" value="{0}">
>>           <input type="hidden" name="line" value="{1}">
>> -        ${h.submit('save', _('Comment'), class_='btn btn-small
>> save-inline-form')}
>> +        <!-- ${h.submit('save', _('Comment'), class_='btn btn-small
>> save-inline-form')} -->
>
>
> ???
I wanted to make clear that the original comment button is removed for
now, but if it would become configurable, this could be changed. I can
remove it completely if it no longer needed when we don't make it
configurable...

>
>
>>           ${h.reset('hide-inline-form', _('Cancel'), class_='btn btn-small
>> hide-inline-form')}
>> +        <div id="draft-btn_{1}" class="draft-btn btn
>> btn-small">${_('Save')}</div>
>>           <div id="preview-btn_{1}" class="preview-btn btn
>> btn-small">${_('Preview')}</div>
>>           <div id="edit-btn_{1}" class="edit-btn btn btn-small"
>> style="display:none">${_('Edit')}</div>
>>         </div>
>> @@ -132,6 +141,16 @@
>>               </div>
>>           %endfor
>>       %endfor
>> +    %for path, lines in c.drafts:
>> +        % for line,comments in lines.iteritems():
>> +            <div style="display:none" class="inline-comment-placeholder"
>> path="${path}" target_id="${h.safeid(h.safe_unicode(path))}">
>> +            %for co in comments:
>> +                ${comment_block(co)}
>> +            %endfor
>> +            </div>
>> +        %endfor
>> +    %endfor
>> +
>>     </%def>
>>   @@ -144,7 +163,11 @@
>>       </div>
>>         %for co in c.comments:
>> +        %if co.draft:
>> +        <div id="draft-tr-${co.comment_id}">
>> +        %else:
>>           <div id="comment-tr-${co.comment_id}">
>> +        %endif
>>             ${comment_block(co)}
>>           </div>
>>       %endfor
>> @@ -202,7 +225,7 @@
>>           </div>
>>             <div class="comment-button">
>> -            ${h.submit('save', _('Comment'), class_="btn")}
>> +            ${h.submit('save', _('Commit all Comments'), class_="btn")}
>
>
> Is "commit" the right term to use?
>
> How do you describe this whole functionality? How to do you talk about it or
> explain it to users? I guess I would say that "your comments will be draft
> until you save them".
Once you're familiar with this way of commenting, is like you never
did it different, it like learning to ride a bike, only the first time
you need to remember to keep pushing the peddals.

>
> Somewhat related: Should there also be a "save as draft" option? I don't
> think so. I think it should save as draft regularly and when leaving the
> input fields.
Yes, indeed, in one of the previous comments I had the exact same
idea, I'll give it a try

>
> Or ... perhaps the inline save button should be renamed 'Save Draft' ...
That could be added, however, your bike also doesn't mention every
time to keep pushing...

>
>>               <div id="preview-btn" class="preview-btn
>> btn">${_('Preview')}</div>
>>               <div id="edit-btn" class="edit-btn btn"
>> style="display:none">${_('Edit')}</div>
>>           </div>
>> @@ -239,7 +262,7 @@
>>          }
>>          var post_data = {'text': _text};
>>          $('#preview-box').addClass('unloaded');
>> -       $('#preivew-box').html(_TM['Loading ...']);
>> +       $('#preview-box').html(_TM['Loading ...']);
>>          $('#edit-container').hide();
>>          $('#edit-btn').show();
>>          $('#preview-container').show();
>> diff -r 48a64173a218 -r b1b3c859230c
>> kallithea/templates/pullrequests/pullrequest_show.html
>> --- a/kallithea/templates/pullrequests/pullrequest_show.html    Sat Mar 28
>> 08:12:49 2015 +0100
>> +++ b/kallithea/templates/pullrequests/pullrequest_show.html    Tue Apr 14
>> 06:39:55 2015 +0200
>> @@ -22,7 +22,11 @@
>>     <div class="title">
>>       ${self.breadcrumbs()}
>>     </div>
>> -
>> +    <script>
>> +    AJAX_COMMENT_URL =
>> "${url('changeset_comment',repo_name=c.repo_name,pull_request=c.pull_request.pull_request_id)}";
>> +    AJAX_COMMENT_DELETE_URL =
>> "${url('changeset_comment_delete',repo_name=c.repo_name,comment_id='__COMMENT_ID__')}";
>> +    AJAX_DRAFT_URL =
>> "${url('pullrequest_comment_draft',repo_name=c.repo_name,pull_request_id=c.pull_request.pull_request_id)}";
>
>
> Why are these lines suddenly needed here?
They are needed for the base.js code as the comments are now drafts
and saved via ajax, I've added the other ones too, because they
appeared to me as missing in this location, so opening a pull request
without passing via any changeset, probably delete doesn't work,
however, I didn't test that...

>
>
>> +    </script>
>>     ${h.form(url('pullrequest_post', repo_name=c.repo_name,
>> pull_request_id=c.pull_request.pull_request_id), method='post',
>> id='pull_request_form')}
>>       <div class="form pr-box" style="float: left">
>>         <div class="pr-details-title ${'closed' if
>> c.pull_request.is_closed() else ''}">
>
>
> /Mads