Transactional 'batch' merge request comments

Moved to https://gitlab.com/gitlab-org/gitlab-ee/issues/1984

As a reviewer every time I press the 'Add comment' button in a merge request every subscriber gets and email which might annoy them.

I want prevent notification overload by grouping comments, stage comments and submit them (email people) in the end.

Possible solutions:

1. Have 'Draft comment' and 'Submit comment' buttons
2. Have checkbox or review mode 'bundle comments'
3. Something else

Phabricator solved this already.

Thanks to Eric Vander Weele

@creamzy @JobV @dzaporozhets

Any ideas how Phabricator solved it?

Also I don't understand what problem we are trying to solve. If you not finished writing comment - simply don't press 'Add comment' button.

I'm setting up a Phabricator instance so we can look at all their awesome secrets.

I'll try clarify the issue. The problem is that for each review or inline-code comment, a notification is sent out for each individual comment, which creates a lot of noise when receiving email notifications.

Phabricator's approach allows the review to create/draft many comments together and submit them all to be notified in batch (like a database transaction). The result is that only one notification/email gets sent out with all the comments and feedback contained within it. This mimics a review pass by the reviewer in that all feedback is captured by that one pass so that the individual can see the entire (holistic) feedback/comments instead of having them be itemized individually with no clear connection.

@dzaporozhets As Eric explains this is about leaving multiple line comments in one email.

@sytses @ericvw working on the issue view UI/UX as there are lots of small but painful details involved in overall not the best ux. Will add this one to the list. Thanks!

@creamzy Thanks

BTW This feature was also requested by a top 10 company.

I personally prefer 1 comment = 1 notification flow. But I see how this can make sense for people. However from technical implementation there is quite a challenge.

@creamzy how would you see it from UX perspective. I dont get how you can create several comments and then submit all of them with one click

Comment from company: 'reviewable.io also does this "correctly"'

See https://reviewable.io/reviews/Reviewable/demo/1

Added ~19492 label

For reference Reviewboard implements this and so does Gerrit. Both implementations are essentially Phabricator's "transactional" approach described by @ericvw above.

+1 for adding support for this. Maybe it can be a configurable option so for those who like the one email per comment flow that can still be there. But for me it creates a huge amount of noise, as I get many review requests per day.

Added ~122770 label

Added feature proposal label

Added ~123454 label

Atlassian Stash and Bitbucket do batch notifications using a configurable quiet period. +1 for adding some kind of support here, the noise is a major frustration for our users.

+1 for this feature request.

mentioned in issue #14849 (closed)

Milestone changed to %Backlog

mentioned in issue #19049 (closed)

@dzaporozhets does https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127 or a future version of it allow for this feature?

Github just added this functionality FYI

https://help.github.com/articles/about-pull-request-reviews/

:)

@sytses The merge request versions coupled with this transactional review could be awesome. For example, I start a review, make a bunch of comments, finish my review. After the author makes some changes, I come back and can select something like "Show me change since my last review" and re-start the review process. It wouldn't matter if there were 1 or 50 commits, one button gives me the diff between then and now.

Added code review label

@dblessing GitHub also added the ability to specify what kind of review you are doing, ie. Require someone to address the comments before merge, just submit them as general comments, submit as comments and approve pull request. Really nice for long pull requests as it also helps you logically keep together your thoughts and then submit as one.

Changed title: Transactional merge request comments → Transactional 'batch' merge request comments

does !6127 (merged) or a future version of it allow for this feature?

@sytses no and its quite different piece of code/logic. Should be implemented as separate feature.

Mentioned in issue #22689 (closed)

Having used GitHub's new Code Review features, while I was initially skeptical I really want this now. It's very useful for reviewing large bits of code as well as doing copyediting. I also think their implementation could be improved upon a lot to make the flow less awkward.

Added ~874211 label

Mentioned in issue #19049 (closed)

removed ~123454 label

We're piloting Gitlab Enterprise, and this is also a big deal for us. Our company currently uses Phabricator, and we're looking to move to Gitlab MR workflow for reviews, but this is kinda a blocker.

The biggest issue is not getting an email per comment; that's annoying, but we can work with it. The biggest issue is that the author doesn't really know when someone is done reviewing their code. With phabricator, you get an email when someone is done reviewing, not per comment, so it's clear when you need to take a look at your change again.

@RohitK89 Our workaround: The reviewer just assigns the MR back to the author when the review is completed. You also get an email from GitLab when you get an MR assigned.

@hho That only works if the author assigns the MR to someone. So far for us, the author just adds a few people or a group as approvers, but doesn't assign to anyone else.

@RohitK89 We started using Labels to signify when something is ready for peer review. I suppose you could extend that to another label when peer review is done. I think users can also subscribe to labels, though I haven't seen it in action yet.

@sidewinder12s I'm sure there's workarounds. The problem is that I just don't see a 100+ engineers buying into using this if the best I can tell them is that they now have to use workarounds for things they've been doing for years. :) It's just a hard sell, unfortunately.

mentioned in issue #20973 (closed)

@victorwu @smcgivern Please keep this on your radar. It's a high-value feature and a place where GitLab is falling behind.

cc @awhildy + @smcgivern:

(1) For the reviewer, they are reviewing a single diff in one sitting. So, a person will make a comment, and then write "same here", "same here", etc. in multiple places. And then maybe at the end, "Sorry, ignore previous comments." They can go back and update comments. But the damage is already done. (Email notifications are sent.) So it make sense for the reviewer to do everything in "draft", and "publish" all at once.

(2) For the person receiving the notifications, they want one single report of that one code review session, not 10 sporadic notifications that might not even be correct.

I assume we'd have to be clever with the UX here? Maybe some hover elements because we have a pseudo web form in essence?

Does it make sense to do (1) and (2) as separate features? You can do (1) first, and so you get the value of the draft-and-publish. The front-end fires off 5 comments together. But the BE doesn't know they are together. With that release, the receiver still gets separate notifications. And then with (2), you are just batching those notifications together on the BE to send one email.

@victorwu - Splitting (1) and (2) may make the value of the little bit of extra work the reviewer has to do less clear. Seems less good from an UX perspective -- but I don't know what is challenging from a technical perspective.

In an effort to not overthink it from an UX perspective, my initial reaction is to just add another button next to Comment to initiate this functionality. The moment you start a review, there is some other piece of UI (near the jump to unresolvable discussions button?) that is your 'Send x comments' button that lets you publish it all at once.

(2) For the person receiving the notifications, they want one single report of that one code review session, not 10 sporadic notifications that might not even be correct.

What do we want that email to look like?

I think we could do 1 and 2 separately, but I'd rather do it right in one go. I think the work you've specified is a clean split for doing the implementation.

@smcgivern @victorwu - My initial expectation was that the email would show all of the comments, one after each other, with some potential separator as needed. Maybe a summary at top (10 comments where added to your MR sort of thing). Does that make sense and would that email be too long?

(When we are closer to planning this, we can work out the design details of the email. Just want to make sure we are all on the same general page)

From an implementation standpoint it's way easier to do if, in the frontend, we enter into some kind of "queueing" state, I make a bunch of comments, hit "Ok, I'm done reviewing", and then the backend handles them as it always did (i.e., sending a notification for each one).

Sending a single email is certainly more polished, but I would want the syntax highlighted diff context that we recently gained in the notification emails, and that might be made more complex if we batch them into a single email, and there might be performance implications there that we saw with the "email on push" notifications.

I think there are two reasons people are asking for this:

• I want to know when someone is done reviewing
• I don’t want to get flooded with emails

Underlying this is a change in the way we model a review. Gitlab models each comment as a separate mini-review, independent. But most people think of “doing a review” as going through the change set and providing a bunch of comments in a batch. It’s like getting markups on your term paper. You don’t want to get each page on your term paper delivered to you in a separate letter from your teacher. You want all the comments at once.

I recognize the “way easier to do it” argument, but to be honest that feels like a half-baked solution. What we really want is “here is my review for your code” and I get all the comments in a single batch, single email.

Thanks!

David

@dvancouvering thanks!

I recognize the “way easier to do it” argument, but to be honest that feels like a half-baked solution. What we really want is “here is my review for your code” and I get all the comments in a single batch, single email.

Does that include the diffs that we show? For example, this is an email with a line comment:

@awhildy : I think "draft" comments could be a great first step of implementing auto-save in GitLab. I assume we will need auto-save in many places once we get more "real-time". Does that make sense?

@awhildy : Wait, on second thought, we would not need a "real" auto save to the db for this scenario right? We already save comments in the browser cache. So do you think leveraging that would be a good enough to implement this feature where many comments are in draft, and then you click publish all at once?

@jschatz1 : So "draft mode" for comments would be default. And if you close your browser tab and open it again, everything would still be there. Would that work?

I like the discussion and where things are going. One small suggestion - I really think we need some sort of 'Start review' type action. It may be too surprising to have diff comments default to 'draft mode'. Or at least the UI needs to be very clear about the situation so people don't assume they've left a single comment and come to find it's still a draft waiting to be sent.

Also, how will we handle replies to a review? If I'm a developer responding to the reviewer's comments, with they be batched, also?

Thanks @dblessing : Good questions / observations! Those are things we definitely need to solve.

@awhildy : I think the design required here wouldn't be too much right? Not certain if this will make it for 9.1. So let's put this on the shelf and no worry about design for now.

@victorwu - Sounds good. I think it is all totally solvable, and we can dig into it closer to when we can focus on it.

I personally would love to see the diffs. So a series of comments of the form

…

Each diff/comment pair could have a link to take me to that particular comment

Also, how will we handle replies to a review? If I'm a developer responding to the reviewer's comments, with they be batched, also?

@dblessing do you mean in the UI, or reply-by-email?

As a user I would like to express my support to this merge request. At the moment, there are three things which make the Gitlab notification UX unpleasant for me in larger projects:

• Receiving one e-mail per diff comment during code review
• Receiving lots of notifications from projects which I only want to monitor, not directly work on
• Receiving lots of notifications from CI bot comments every time someone somewhere pushes code to a MR

The proposed feature would greatly help with my first problem. For the other two scenarios, I think issue #3558 (moved) would be closer to what I need, so I will comment further there.

Following up on @dblessing 's remark regarding replies, from my perspective it would be better if replies from the UI were also transactional.

mentioned in issue #3558 (moved)

added ~897282 label

@hazelyang This issue is tagged with ~"coming soon" and UX. Can you look at how the UX might function when you are able?

Theres a few examples out there that currently exist. See one I posted here a few months ago: https://gitlab.com/gitlab-org/gitlab-ce/issues/20973#note_14007656

Sid posted another one above: https://gitlab.com/gitlab-org/gitlab-ce/issues/3364#note_2653676

assigned to @hazelyang

@tauriedavis Will do!

added to schedule label

moved to gitlab-ee#1984

removed milestone

@victorwu @hazelyang It seems like this is coming-soon/to-be-scheduled -- is the issue supposed to be marked closed?

@nickurak : This was moved to https://gitlab.com/gitlab-org/gitlab-ee/issues/1984. (And this issue is thus closed.)

(ah, sorry, 2 of us managed to miss that :) )

mentioned in issue #31223 (moved)

No problem @nickurak . It's not very clear. See https://gitlab.com/gitlab-org/gitlab-ce/issues/31223.

mentioned in issue #18008 (moved)

Is there an update on this? It's a deal breaker for us.

@chinhodado This issue was moved to another project where we are actively working to finalize the design. See https://gitlab.com/gitlab-org/gitlab-ee/issues/1984

I'm quite surprised that GitLab's review system didn't have this feature from the get-go. Every other review system that I've used (ReviewBoard, Gerrit, Phabricator) has it. The current system of posting each comment as you write it doesn't make much sense to me from a conceptual point of view.