Skip to content
Snippets Groups Projects

Exclude Merge Request author from list of approvers if required.

Closed Rubén Dávila requested to merge ce_issue_18759 into master

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Reassigned to @rdavila

  • @rdavila Couple notes, and the Spinach failure looks related.

  • Rubén Dávila Added 9 commits:

    Added 9 commits:

    • 1d6c2a86...77b42d92 - 8 commits from branch master
    • d4e2b1fe - Exclude Merge Request author from list of approvers if required.
  • Reassigned to @rspeicher

  • Author Contributor

    @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)
  • @rdavila thanks, sorry I broke this :disappointed: 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.

  • Author Contributor

    @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.

  • Author Contributor

    @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 the Accept 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?

  • Rubén Dávila Added 1 commit:

    Added 1 commit:

    • e537d88c - Refactor for MergeRequest#approvers_left
  • Author Contributor

    @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:

    Screen_Shot_2016-06-21_at_12.38.13_PM

    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.

  • The best solution long-term is probably just to say that if we have no more valid approvers, then no approval is needed, even if we haven't reached the required number of approvals.

  • @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 Maan
  • mentioned in issue #388 (closed)

  • Douwe Maan Status changed to closed

    Status changed to closed

  • Robert Speicher mentioned in merge request !493 (merged)

    mentioned in merge request !493 (merged)

  • Stan Hu Removed ~149424 label

    Removed ~149424 label

  • Stan Hu Milestone removed

    Milestone removed

  • Author Contributor

    The best solution long-term is probably just to say that if we have no more valid approvers, then no approval is needed, even if we haven't reached the required number of approvals.

    This approach makes good sense for me.

  • Agreed.

  • Robert Speicher mentioned in commit 64a2be3a

    mentioned in commit 64a2be3a

  • Sid Sijbrandij mentioned in commit 9cde642a

    mentioned in commit 9cde642a

  • Please register or sign in to reply
    Loading