Reviewing merge requests currently triggers an email for each comment. This has a few drawbacks:
as a reviewer, I often comment on something which is not clear only to find the reason a few lines below, which prompts me to amend/drop my initial comment and I would like to be able to do so without generating useless noise for the MR author
as a reviewer, in some cases I have to interrupt a review midway and there's no way to tell the MR author that I haven't actually finished it
if there are many small comments, the MR author receives an annoying stream of emails where context is lost soon and the only sensible solution is to ignore their contents, treat them as a single "you have comments" notification and look at the GitLab web UI
Yes, please! This is the number one feature that frustrates my small team of developers. We use both GitLab CE (for our internal/proprietary projects) and GitHub (for our public / FOSS projects). GitHub has this feature and it is amazing what a difference it makes in the review process.
In the initial MR for this feature in EE, @brett.higgins described the exact reasons this feature is so valuable. Consider some of the downsides of not having this feature for us CE users:
The developer gets bombarded with emails as soon as the review starts. This is disruptive to his work.
The developer does not necessarily know when the reviewer is done with the review.
The developer may push changes or respond to comments while the reviewer is still doing the review.
Often a reviewer will leave a comment and then later in the review find that his question was answered or concern addressed and he can remove the already-left comment. If the review was grouped, you could do so without the developer ever knowing about your question. But as it stands in the CE version, the developer may be already in the process of writing a response to your question by the time you realize you understand and don't need the comment / question any more. Waste of time for both of you.
Our plans are based on the buyer that buys GitLab, from individual contributor, to manager, to director, to executive. The feature is put in the plan based on what champion is most likely to care about it.
Then in your stewardship page you say this when describing what features are paid-only:
When GitLab Inc. makes a new feature we ask ourselves who is the likely type buyer to determine into what tier the feature goes. If the likely buyer is an individual contributor the feature will be open source, otherwise it will be source-available.
I could be misunderstanding it, but I read these two things as "if this feature is most important to an $xUser then it goes into the edition targeted at the $xUser as the 'likely buyer'". In other words, "if this feature is most important to an individual contributor, it goes in the core; but if it's most important to a manager, it goes in the enterprise starter, and so on" (according to the four tiers).
To me, there is no user other than the individual contributor who will care so deeply about this feature. It's the contributor - the developer and the reviewer - that have their day disrupted by the current workflow and would benefit most from having this in CE.
Thanks for considering this impassioned plea, and I'd be happy to hear your reasoning if I'm misunderstanding the pricing / business model documentation linked to abaove.
I'm supportive - linking a bunch of comments together into a single item is a very useful primitive for all sorts of other features, but if we don't give people on Core a minimal version of it, how are they to learn about other code review enhancements as we add them to higher tiers?
I've opened other issues recently around other functionality that needs this basic primitive to operate (e.g. reviews following commits, rather than MRs, around: https://gitlab.com/gitlab-org/gitlab-ee/issues/11277). I'd also love to be have a dashboard showing me recent reviews I've done, and to be able to enhance cycle analytics with explicit data about rounds of review. All of this is more difficult without having the basic idea of a review in Core.
It's not my call, and I know @jramsay is on holiday for the next couple of weeks, but hopefully we can get some further discussion around it once he's back!
I would also like that feature in CE. The release post of 11.4 already states:
Code review is an essential practice of every successful project
While that is in the context of a different code review enhancement, doing reviewing code in CE is currently quite painful. Currently code reviews get quite a bit of pushback, because most people don't like beeing spammed with emails every minute, just because someone is working there way through a diff and leaving comments. This makes some people see a code review as an act of aggression, which makes the already delicate topic of critizing someones work even harder.
Just because your project is small and you still want to self host your code, shouldn't lead to you being spammed with emails, while trying to improve your development practices.
I would really appreciate, if this feature could be added to CE, because I want to be notified, when someone reviewed my changes, but one or two emails should be enough, not the 10 to 30 I currently get.
My company is currently using ReviewBoard to do code reviews. We recently set up a GitLab Core instance to explore moving away from RB, as hooking RB up to our other infrastructure is very cumbersome.
The lack of this feature is making me wonder whether that's really a good idea.
Today, code reviews are sadly not standard procedure on most of our projects, so we only have a handful of people who would benefit from this feature. But the Git servers we currently use have 150-ish combined users on them, and we need to migrate our projects off those servers for other reasons, so it makes sense to standardize on GitLab. There's no way I can convince corporate to spend 19 * 12 * 150 = 34,200/year when MRR is literally the only feature in any paid tier that we would use.
I agree with others above that this really ought to be part of the Core tier. If you're not going to move it, at least update the justification on the Pricing Model page, because what it says today is... highly illogical:
Merge request reviews enables the expedient review by multiple team members on merge requests.
This implies MRR somehow makes a review faster, which is not true.
Typically large enough teams to value the reduced chatter accompanying MR reviews reside under Directors.
I think we can all agree that a developer's level of annoyance at having their inbox spammed is not a function of their team's size.
Hmm, I must have missed it when I was pinged on this issue. Glad to see there's been some official response re: stewardship on Merge Request Reviews - though we're well overdue for an update here. @jthomerson@jramsay@DouweM not sure who owns this bit so I'm pinging you all, sorry
I 100% agree that it seems in conflict with Gitlab's own stewardship guidelines to keep this feature at Premium and up. I can't even pitch Gitlab internally as a code review tool without this feature; it's been tried before this feature existed, and the spammy notifications issue was one of the top complaints that drove people immediately back to Reviewboard.
We are on EE Starter, but with a few exceptions, most of the company only uses Gitlab for git and code search. I'd like to change that. And if I can't pitch Gitlab for code review without this annoyance, I have a much harder time pitching Gitlab for all the other reasons I want to, as an integrated solution to replace other tools we're paying for and/or supporting/operating ourselves, never mind trying to pitch higher tiers (which definitely have other features I'd like to be using).
Just to add this here too for those following the issue, per @jramsay in issue #26581:
@danielgruesso will be working on this over the next few weeks, which will allow us to put a firm timeline on it, hopefully in the next few releases. The earliest it could possibly be is 13.0
https://twitter.com/ericbrinkman/status/1245010555481530369 "@jthomerson
thanks for the note and an even bigger thanks for commenting on the issue you linked. We are considering bringing this feature down to core and working to understand the impact of that change. Thanks again for your feedback! cc
@_jramsay"
1
Eric Brinkmanchanged title from Port Merge Request Reviews from GitLab EE to GitLab CE to Port Merge Request Reviews to core
changed title from Port Merge Request Reviews from GitLab EE to GitLab CE to Port Merge Request Reviews to core
Eric Brinkmanchanged title from Port Merge Request Reviews to core to Port Merge Request Reviews to Core
changed title from Port Merge Request Reviews to core to Port Merge Request Reviews to Core
Eric Brinkmanchanged the descriptionCompare with previous version
@danielgruesso Like some of the other features moving to core, can this feature be first easily downgraded to ee:starter and then make the major changes later? Do you think that's a possibility?
@m_gill I'd give this one a 2. It's principally about moving code, but sometimes there are unanticipated interactions, or it can be difficult to find the thing in the first place.
I think this one stretches across both frontend and backend code to move, as well, although they could proceed independently; we'd want the BE part to be merged first in that case.
It can be very useful to look at the original MRs introducing the feature to get a sense of what needs to move. Those are !4213 (merged) (BE) and !4213 (merged) (FE).
The first increment - moving to starter - is easy enough in code if you know the feature name: :batch_comments. It needs radiating out to the docs and https://about.gitlab.com/features/ as well, though.
For step 1, the goal is to allow the feature to work for Starter to Ultimate. This is just temporary state since the ultimate goal is to move to Core.
I'm thinking if we can just remove the need if feature is available or not. For backend, we can remove the @project.feature_available?(:batch_comments, current_user) calls. But I'm not sure how that affects frontend.
I see this line in this view file and it seems that if I remove that, it should achieve that. Will that work? Or are there other files needed to be changed?
Will that work? Or are there other files needed to be changed?
Yeah I think that should work The frontend basically checks if that element exists and then enables batch comments throughout merge requests in the Vue app.
@danielgruesso@iamphill the steps 1 - 7 mentioned in #28154 (comment 344352948) are already done or being worked on (steps number 6 - 7 have MRs in review, 1 - 5 are all merged). This means that in master branch (and on GitLab.com), the Reviews functionality is now available to Core. It'll be available to Core in self-managed installs once 13.1 is released.
We do need to update the https://about.gitlab.com/features/. I wonder if we should do this now or wait until 13.1 actually released? My worry is if we update it now and 13.1 isn't released yet, self-managed install users may wonder why it's not available to Core yet. WDYT?
@patrickbajao your plan to iterate through this process was legit, many thanks. I agree with Phil here, since the features page is not scoped, this should be done along with the release post.
@nick.thomas@m_gill I began working on the backend side of this and work required seems to be a bit more than a 2.
There's this Review model that we also create which is used when sending notifications (email/note). We need to ensure that works as well. It wasn't part of the original MR since it was added after the fact. Given that, I think this should be weighted as 3.
I changed its weight to backend-weight3. Please let me know if you disagree though.
Is it possible to turn off this feature? What if we would like to just post comments immediately instead of starting tons of reviews while discussing some changes?
What if I don't want to click on anything by mouse pointer? For years, including GitLab, I was always posting comments by Ctrl+Enter. My intention by pressing this hotkey is to post. Now it has been changed to posting pending comments and I have to always click "Send comment now". It become less convenient for me. And I'm sure for many others. I think it would be nice to have this feature optional.
Thank you so much for moving this feature into core!
You might already have this covered but just want to point out that the Self-Managed Feature Comparison and Gitlab.com Feature Comparison pages still say that the Merge Request Reviews feature is not supported for the core/free or starter/bronze tiers.