Exclude Merge Request author from list of approvers if required.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/18759
Related to !455 (merged)
This fix will restore the Accept Merge Request button when the only approver left is the Merge Request author.
It also doesn't show the Merge Request author when rendering the list of users that needs to approve the Merge Request
Merge request reports
Activity
@rspeicher can you please review?
Milestone changed to %8.9
Reassigned to @rdavila
@rdavila Couple notes, and the Spinach failure looks related.
Reassigned to @rspeicher
@rspeicher Thanks for the code review, I've updated the MR, can you please review again?
475 475 end 476 476 477 477 def approvers_left 478 User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id)) 478 @approvers ||= begin 479 records = User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id)).to_a 480 records.delete_if { |approver| approver == author } 481 end 482 end 483 484 def requires_approval_from_author? 485 return @approval_from_author if defined?(@approval_from_author) 486 487 @approval_from_author = author && overall_approvers.map(&:user_id).include?(author.id) I'm a little confused by this. The method name suggests that we're checking if the MR require's the author's approval, but the implementation looks like we're checking if the author has already approved it.
Edited by Robert SpeicherAlso, should this be private?
@rspeicher I think we may need this method public in a future, do you have a special concern with it being public?
@rdavila thanks, sorry I broke this
I'm not sure this is the right fix, though: it seems from https://gitlab.com/gitlab-org/gitlab-ce/issues/18759 that people want to be able to approve their own MRs if they are the master of the project. I imagine that's probably different if there are a bunch of approvers defined on the project (which is the default place approvers come from) who are also submitting MRs - which is what the original change was about.@smcgivern what I understand from the issue is that the Master needs to be able to merge the MR after the other user has already approved it, this is what the fix does: if the owner of the MR is the only approver remaining then he or other user with merge privileges can merge the MR, we no longer require the approval of the owner of the MR.
@rdavila right, but the author of an MR isn't necessarily the master of a project. Developers can also create MRs.
@smcgivern right, if the project allow developers to merge MRs then he will see the
Accept Merge Request
widget, I'm not doing anything special for masters, just fixing the main issue that was not seeing theAccept Merge Request
widget@rdavila I understand that, I'm just trying to understand how to reconcile this MR with the feature request: https://gitlab.com/gitlab-org/gitlab-ee/issues/388.
It seems that if the author of an MR is also an approver, then this reduces the number of approvals an MR needs by one. But then we may as well just revert !455 (merged), because that prevented authors-of-MRs-who-are-also-approvers from approving their own MRs.
Imagine a project where each MR needs to be approved by one person. If the author of an MR is also in the approvers list, then this doesn't even show approvals for their MRs any more, it just allows them to be merged immediately. Is that how we want this to work? I think the answer is yes if it's a single-person project, where they are the only approver, but no if there are several approvers, as presumably the point of setting approvers in the first place is to have someone else look at the code before it's merged. Does that make sense?
Added 1 commit:
- e537d88c - Refactor for MergeRequest#approvers_left
@smcgivern I understand your point but I'm sure if that's is a common scenario, I think other possible suggestion is to remove the author of the list of approvers when creating the MR, for example in this case
Administrator
should no be listed:I think that should be handle better the scenario that you're mentioning. @rspeicher do you any suggestions regarding @smcgivern's POV?
To be clear, I think that if we decide this needs more thought, the best option is to revert https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/455. We're very close to releasing 8.9 and, while that feature was requested for 8.9, I think this will probably need more thought.
@smcgivern I agree. We need to think more about how we can reconcile https://gitlab.com/gitlab-org/gitlab-ee/issues/388 and https://gitlab.com/gitlab-org/gitlab-ce/issues/18759, and https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/455 and this MR.
If we ship it like it is right now (with only https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/455), it will feel to people like a regression (as https://gitlab.com/gitlab-org/gitlab-ce/issues/18759 shows).
If we ship it with this MR, we're effectively reverting https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/455 and bringing back to live the issue in https://gitlab.com/gitlab-org/gitlab-ee/issues/388.
I've asked @rspeicher to revert !455 (merged), which will fix the regression (https://gitlab.com/gitlab-org/gitlab-ce/issues/18759) for 8.9 (and master), and I'll close this MR.
I'll reopen https://gitlab.com/gitlab-org/gitlab-ee/issues/388 and assigned 8.10, so we can solve it properly taking this new discussion into account.
@smcgivern Can you comment on https://gitlab.com/gitlab-org/gitlab-ee/issues/388 with your summary of events, and the exact edge-case we need to address in the new fix for that issue? Ask @JobV for input from a product perspective.
Edited by Douwe Maanmentioned in issue #388 (closed)
mentioned in merge request !493 (merged)
mentioned in commit 64a2be3a
mentioned in commit 9cde642a
added Enterprise Edition label