save multiple comments at once

[Jan Heylen]

On Fri, Mar 27, 2015 at 10:02 AM, Wouter Vermeiren
<wouter.vermeiren at gmail.com> wrote:
> Hi,
>
> I was wondering whether I might make a slightly different proposal for
> reducing the amount of emails/notifications...
>
> My proposal revolves around making it possible to 'edit' a comment. By
> default when you are looking at a pull request and commenting on it, every
> comment is saved in the database as 'draft' (like Jan's proposal). And only
> when you either mark a commit as approved/rejected/... at the bottom of the
> page, then the comments on this commits are converted from draft into actual
> comments. The same goes for comments on the pull request. So only when you
> update your status on the review, the comments are saved. I am assuming here
> that people use this general status to clearly indicate there progress while
> looking at the review (otherwise why do we have these nice colorful
> circles).
> This process can repeat itself. Suppose you have added 4 comments and you
> need to go home. Then you select the 'under review' status and the 4
> comments are saved. If the author then looks at the pull request he/she can
> see that you are not done. The next day you add 2 additional comments and
> then you reject the review. Each comment cycle is followed by a status
> update thus triggering a conversion from a draft comment into an actual
> comment.
[jan] This is as I am implementing it :-)

>
> - In the GUI, I would not display a comment as long as it has the draft
> status. This prevents the author replying to a comment that you later want
> to change/remove.
[jan] This is as I am implementing it :-)

however, for the moment, I don't require any edit functionality to
have this workflow, so what follows is another ;-) useful discussion
on the edit functionality:

>
> - I realise that editing a comment could spark a holy war here. However my
> opinion would be that this is ok as long as there is some tracability.
> Therefore a comment that was edited, should always contain something like
> 'Edited by <user> on <datetime>'. And this small line could be a link to the
> original comment.
>
> - Editing can happen at 2 stages in the process: a) while a comment is still
> in draft status and b) while it is visible to everybody. In case of a) this
> 'edited by' line does not need to be there because it wasn't visible and
> thus no harm is done. In case of b) then this 'edited by' line should
> definitely be there because somebody might have replied to the comment.
>
> - GUI-wise, this means adding an edit-icon somewhere or a button next to the
> delete button.
>
> - 1 email/notification is send when the reviewer updates his status (so at
> the time the comments are converted from draft into actual comments). This
> will greatly reduce the amount of emails/notifications without loosing any
> relevant information.
>
> - I would also make this behaviour configurable by the admin on a per
> kallithea basis by adding a checkbox in the "Admin > Settings > Emails" and
> not an account specific feature.
>
> The reason why I am a bit hesitant about accepting the proposal from Jan, is
> because I don't like different buttons doing the same thing only slightly
> different. For me buttons like <comment><submit all comments> next to each
> other, is stepping back to the web1.0 world. They both mean the same thing

[jan] There is no buttons comment anymore in the latest proposal, again:
per inline comment form: save - cancel (and once saved a edit/delete)
Below the general comment field you'll have the "submit all comments"

> from a user perspective and without knowing the background it is confusing
> which button to use. Nowadays people are used/prefer correcting a mistake
> they made rather then having a tool policing them into reviewing every
> comment beforehand. This kind of invasive policing reduces the mental speed
> with which somebody is doing a review. Or at least he/she perceives he/she
> is doing the review.
> Also this extra step that is required, is not currently in a typical review
> flow. You comment on code and then you mark the pull request as
> approved/rejected/... At no point do I need to take into account that my
> comments are only saved client side and that if my browser crash, I'd loose
> all comments. That just seems bad design circa 2015...
[jan] with my changes, all drafts are saved server side, as you
describe here, 100% exactly the same

>
> As always, I would be happy to create a patch. I am on holiday next week,
> but I will find some time to work on it. And anyway it seems like a small
> patch ;-)
[jan] not that small, wait for the review ;-)

>
> Kind Regards,
> Wouter Vermeiren
>
>
> On Thu, 26 Mar 2015 at 18:41 Jan Heylen <heyleke at gmail.com> wrote:
>>
>> Hi Mads,
>>
>> any comments on the issue discussed below?
>>
>> I'll restart the thread:
>>
>> I've been working on this some more and as I see it now:
>>
>> For inline comments you'll have 2 buttons: save - cancel
>> Below the general comment field you'll have the "submit all comments"
>> and the "cancel all" button
>>
>> So the save button would store the comment as draft (on the server),
>> and the "submit all comments" will change all those drafts into
>> comments and send out the notification.
>>
>> I would leave out the 'direct' comment button from the inline-comments
>> as I think we should encourage a workflow where users review at least
>> whole the changeset before committing comments, instead of submitting
>> each comment separately. Even better would be to be able to do this
>> for the whole pull request at once (save draft comments on all
>> changesets, and then submit all comments at once).
>>
>> br,
>>
>> Jan
>>
>> On Tue, Mar 17, 2015 at 8:36 PM, Jan Heylen <heyleke at gmail.com> wrote:
>> >
>> > On Mon, Jan 26, 2015 at 7:35 PM, Mads Kiilerich <mads at kiilerich.com>
>> > wrote:
>> > > On 01/26/2015 06:37 PM, Jan Heylen wrote:
>> > >>
>> > >> Hi,
>> > >>
>> > >> I've been working on a change that a user would allow to open
>> > >> multiple
>> > >> comments on a commit, and 'post' (save) them all at once to the
>> > >> kallithea database. Currently this is client side only change, but
>> > >> with the proper changes on the server side end, this could result in
>> > >> a
>> > >> one-mail-per-commit-review instead of a one-mail-per-comment setup
>> > >> now.
>> > >>
>> > >> But the main reason I was working on this is the fact that if you add
>> > >> 2 or more comments to a commit, and you choose to save them as
>> > >> preview
>> > >> first (you are still reviewing the rest of the commit), you have to
>> > >> go
>> > >> over all the comments and press the save button.
>> > >>
>> > >> Where I want to take this is even one step further:
>> > >>
>> > >> We have now:
>> > >>    For inline comments:  save - cancel - preview
>> > >>    And below the commit: approved/rejected/close/... and comment -
>> > >> preview
>> > >>
>> > >> I would change this to:
>> > >>    For inline comments: comment - cancel
>> > >>    Below the commit: submit all comments - cancel all
>> > >
>> > >
>> > > I think it could be annoying if we don't have a comment/save/submit
>> > > button
>> > > next to the inline comment. But I guess the text on the button should
>> > > change
>> > > to something with "all comments".
>> > >
>> >
>> > I've been working on this some more and as I see it now (almost the
>> > same as above, but a bit more explanation):
>> >
>> > For inline comments you'll have 2 buttons: save - cancel
>> > Below the general comment field you'll have the "submit all comments"
>> > and the "cancel all" button
>> >
>> > So the save button would store the comment as draft (on the server),
>> > and the "submit all comments" will change all those drafts into
>> > comments and send out the notification.
>> >
>> > I would leave out the 'direct' comment button from the inline-comments
>> > as I think we should encourage a workflow where users review at least
>> > whole the changeset before committing comments, instead of submitting
>> > each comment separately. Even better would be to be able to do this
>> > for the whole pull request at once (save draft comments on all
>> > changesets, and then submit all comments at once).
>> >
>> > br,
>> >
>> > Jan
>> >
>> >
>> > >> The inline 'comment' buttons would actually do what is now a preview
>> > >> (and may still indicate comment preview), cancel remains the same.
>> > >> The 'submit all comments' would do an AJAXPost of all the comments
>> > >> (no
>> > >> backend change required) at once.
>> > >
>> > >
>> > > I don't see the point in rst/md markup of comments. People should
>> > > comment on
>> > > content, not spend time making it look fancy. We only saw people being
>> > > annoyed by their code snippets being obfuscated by the markup
>> > > processing. We
>> > > use
>> > >
>> > > https://bitbucket.org/Unity-Technologies/kallithea/commits/09286e5ca064de6930d5bdefb9df6708eda19976
>> > > and do thus not really have any need for preview. We could perhaps
>> > > move in
>> > > that direction here upstream and avoid the problem you mention.
>> > >
>> > >> What do you think?
>> > >
>> > >
>> > > +1 to the general idea and your thoughts.
>> > >
>> > > Also, we should introduce a "don't navigate away from this page with
>> > > pending
>> > > comments" before we encourage users to not commit immediately.
>> > >
>> > >
>> > >> I've made a little POC patch that does the submit-all-open-comments
>> > >> thing, it does it for open comments for know, but probably not that
>> > >> difficult (and even better, it won't require the remove-class hack
>> > >> probably) to change that to a for all preview comments way of
>> > >> working.
>> > >> I'll inline the POC patch for your reference, but forgive me my basic
>> > >> javascript knowledge...
>> > >>
>> > >> regards,
>> > >>
>> > >> Jan
>> > >>
>> > >> the patch:
>> > >>
>> > >> diff -r bfc304687f1c kallithea/public/js/base.js
>> > >> --- a/kallithea/public/js/base.js Wed Jan 21 17:35:11 2015 +0100
>> > >> +++ b/kallithea/public/js/base.js Mon Jan 26 18:36:00 2015 +0100
>> > >> @@ -667,6 +667,9 @@
>> > >>       $form.submit(function(e){
>> > >>           e.preventDefault();
>> > >>
>> > >> +        if(!$tr.hasClass('form-open')){
>> > >> +          return;
>> > >> +        }
>> > >>           if(lineno === undefined){
>> > >>               alert('Error submitting, line ' + lineno + ' not
>> > >> found.');
>> > >>               return;
>> > >> @@ -694,6 +697,10 @@
>> > >>                   'line': lineno
>> > >>           };
>> > >>           ajaxPOST(submit_url, postData, success);
>> > >> +        //proof of concept, preemptive remove the form from the open
>> > >> list, not good, but only for POC
>> > >> +        $tr.removeClass('form-open');
>> > >> +        var allOpenForms = $( '.form-open' ).next();
>> > >> +        allOpenForms.find( ".inline-form" ).submit();
>> > >>       });
>> > >>
>> > >>       $('#preview-btn_'+lineno).click(function(e){
>> > >
>> > >
>> > > It is not obvious to me what is going on with this form-open. Or ...
>> > > you are
>> > > recursing and using this to make sure it terminates?
>> > >
>> > > I would guess you could use something like
>> > > $('code-difftable form').each(function(){$(this).submit();});
>> > > or perhaps even
>> > > $('code-difftable form').submit();
>> > >
>> > > But in the next step you will submit multiple comments in the same api
>> > > call.
>> > > That means you either have to do it with javascript or change the
>> > > whole diff
>> > > to be one form. This PoC might thus not be step towards a real
>> > > solution.
>> > >
>> > > /Mads