Currently, approvals are used to block a merge request from being merged.
However, when a merge request is already merged, there are no strict requirements. However, in GitLab currently, you cannot continue approvals after a merge request has been merged.
This makes the product unnecessarily complicated. We want to make GitLab as simple as possible. One way is to reduce the number of rules. The proposal here is to allow approving/unapproving even when a merge request is merged. The benefit is that we reduce the complexity in the product, reduce number of business rules.
Another benefit is that reviewers can express their signals on a merge request, even after it has been merged. Reviewers can come back and continue to approve / unapprove. The UI design should make it super obvious that the signal doesn't impact the merge request (it is already merged). And so there should be very little confusion about it. But this allows reviewers to continue expressing their signal. For example, if there are 10 approvers, but 3 already approved (and 3 was the required number). The remaining 7 can come back after the fact and continue talking and express their signals in the merge request. We should not restrict them. It solves the problem of unnecessarily restricting users/reviewers in discussing a code change, even after it has been merged.
Docs blurb
You can now approve merge requests (or remove your approval), even though the required number has already been reached.
Description
If the number of approvals is not met, it should block the merge request from being merge-able.
Achieving the required approvals is a necessary condition for the merge request to be merge-able, but not sufficient.
If somebody unapproves and falls below the threshold, the merge request is blocked again.
Under all merge request states (open/closed/merged and reached/not reached enough approvals), a qualified user should be able to approve or unapprove the merge request.
Even if you have reached the required number of approvals, you can continue to approve and unapprove.
Under all merge request states (open/closed/merged and reached/not reached enough approvals), view the status of the approvals.
View who are explicit qualified approvers, including the users / groups that have been selected in either the project settings or the overwritten merge request settings.
View who has approved. View who has not approved. View how many need to be approved / how many left to be approved.
Iterate on the existing UX design. There is feedback that the existing empty dotted circle is confusing.
Design
Design
Screenshot are from the perspective of:
Requires 1 more approval
cannot approve or remove approval:
can approve:
can remove approval:
Requires 5 more approvals ("approval" becomes "approvals")
cannot approve or remove approval:
can approve:
can remove approval:
Requires 5 more approvals with 2 specified eligible approvers
cannot approve or remove approval:
can approve:
can remove approval:
Requires 4 more approvals with 4 specified eligible approvers
cannot approve or remove approval:
can approve (you are a eligible approver, last in the list):
can remove approval:
Requires 4 more approvals with 2 specified eligible approvers and 1 specified eligible approver group (with enough people in it to get all required approvals)
cannot approve or remove approval:
can approve (you are a eligible approver, last in the list):
can remove approval:
Requires 8 more approvals with 2 specified eligible approvers and 1 specified eligible approver group (withuot enough people in it to get all required approvals)
cannot approve or remove approval:
can approve:
can remove approval:
Received 2 approvals when 2 are required
cannot approve or remove approval:
can approve additionally:
can remove approval:
Received 3 approvals when 2 are required
cannot approve or remove approval:
can approve additionally:
can remove approval:
If their are no approvers yet, the second line is not shown:
If the MR is blocked by approvals, it should show like:
Notes:
If there are less eligible specified approvers (including those in the specified approver groups), and others is mentioned.
If you yourself can approve, your avatar always shows last in the eligible approver avatar list on the first line.
If you yourself did approve, your avatar always shows last in the approver avatar list on the second line.
If you aren't a named approver, or a member of an approver group, you can (currently) only approve if there are more approvals required than people who are named approvers or members of approver groups (this currently is already functioning this way). Please see the following doc: https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html
This one is quite obvious in retrospect. Thanks for pointing it out. You should be able to approve beyond the required, because we want to always encourage collaboration signals. Meeting the required just unlocks the merge request.
Pending some design updates to make it coherent though.
@smcgivern : Would this be an FE-only issue? Since we are not changing the merge request state (it is still approved and unblocked), but we just allow people to keep approving if they want to. And I suppose the BE has already been designed/implemented to be flexible for those FE signals to be sent in?
Some users have expressed confusion with the existing design of approvals with the empty dotted circle. Likely that would be addressed in the design when we design and schedule this improvement.
Example of current behavior that we want to address with improved UI with this issue:
Victor Wuchanged title from Approve a merge request beyond the number of required approvals to Approve and unapprove a merge request in all merge request states and approval states
changed title from Approve a merge request beyond the number of required approvals to Approve and unapprove a merge request in all merge request states and approval states
@smcgivern : Now that we are using approvals, great time to do this one and get more feedback (if not just from dogfooding). We could also use this issue as a chance to iterate on the UX design. Could you please review the description and see if this is do-able as a 9.2 candidate?
@tauriedavis : This would be the next feature in our approvals set of features. And definitely we could get your team to help us out on iterating the design based on all the existing feedback
@rymai : Thanks for pushing approvals through. Just FYI to show this this is the next area we want to improve on.
@hazelyang : I'm using "explicit" approvers to describe users and groups that you have explicitly said that you want to be an approver in the project settings or the merge request settings/fields. I'm using "implicit" approvers to describe people who are allowed to approve even if they are not specified.
@smcgivern : I think you've explained this a few times already. Let me get it documented so I don't keep forgetting. If there are not enough approvers, certain project roles can approve as well right? Who are these?
@hazelyang: For the design so far and your questions:
Yes, I imagine all this information to be somewhere in the merge request widget. But feel free to suggest something/somewhere different. I can't imagine it living anywhere else though. Above or below? As a separate UI component?
These are the latest designs for the merge request widget: https://gitlab.com/gitlab-org/gitlab-ce/issues/25424#note_23180548 from @dimitrieh . Right now your designs seem to be a separate section in the widget, and not subsections of that main area. Is that accurate and intentional? There's just so much information/utility in the widget. I want to make sure the design makes as much sense as possible.
Viewing "Approved by" you have designed, and I assume it's scalable by just putting the avatars.
Same as "Remove your approval". That looks good.
You've shown qualified implicit approvers and approver groups. That looks great.
I'm not really sure what else should be communicated here for the rest of the design though, since it's so complicated. For example, suppose 3 approvals are required, and 7 people have approved including yourself. If you unapprove, the UI will still say "achieved required approvals". Is that confusing? Should we somehow communicate those detailed numbers?
If the required number of approvals is achieved, we want to allow people to continue approving because it is a positive signal for the merge request. But when you do it, nothing "new" happens. Should we communicate that really nuanced dynamic to say "you approved, this doesn't do anything new right now, but if someone else unapproves in the future, your approval might matter because of it". It's all really complicated to me. Need your good design ideas to figure this out.
After looking at your designs more, I don't think we need to view the implicit qualified approvers, because those seem to be edge cases and the amount of complexity is already really high. What do you think?
We have a message to encourage the approver to approve the merge request. (Need help with the copy)
Add the number of "Required approvals" next to "Achieved required approvals".
If there are 4 people approved, the number will be 4. But people still know the merge request is ready to be merged, since there are "Achieved required approvals" to indicated the status. Changing the number is just to give the feedback for the approver.
Otherwise, if the number of approvers (who approved) is smaller than 3, the text will be changed to Requires X more approvals.
I think you've explained this a few times already. Let me get it documented so I don't keep forgetting. If there are not enough approvers, certain project roles can approve as well right? Who are these?
@hazelyang@dimitrieh : That looks really good and looks a lot more intuitive. I've been staring at this for a long time, so perhaps let's at least share it in slack in the #development channel. Just a few more small nitpick points:
Probably the copy can be Received 3 required approvals. Achieved sounds weird. But let's tweak that last.
@hazelyang : I agree with @dimitrieh on taking away the number in the circle, since to me that's not clear right away what that's indicating.
I think unapprove is in the docs. But we should probably use remove approval or remove their approval or remove your approval everywhere to be consistent.
Approve additionally sounds weird. But I can't think of a better term, and it's a quick way to indicate you can continue approving. I like it. And the tooltip popup works well instead of the longer line of text.
I don't think we need the dotted circle. We already have the copy indicating how many required approvals remain. And the dotted circles were causing confusion when first released, with some folks thinking it was a bug of not being able to load the avatar.
I don't think we need the dotted circle. We already have the copy indicating how many required approvals remain. And the dotted circles were causing confusion when first released, with some folks thinking it was a bug of not being able to load the avatar.
These are good points!
Updated!
cannot merge, needs more approvals, can remove approval:
cannot merge, needs more approvals, cannot approve:
cannot merge, needs more approvals, can approve:
able to merge, got all required approvals, can remove approval:
able to merge, can be additionally approved, can approve additionally:
updating the spec preview to include these designs on this url
@dimitrieh@victorwu I like the designs, I agree with @victorwu's comments that some of the button text could be improved, but I understand we're trying to be as explicit as possible.
I do however believe that this child component of the MR widget clashes slightly with the merge button component below it, and possibly other components, it's probably just me but it doesn't feel as fluid as it could be. Maybe this could be as simple as adding dividers between the child components?
I had a thought that all of this functionality can be captured just with that span of avatars. I hear you that the empty dotted circles confused some users, but if a set of approvers is provided, the circles should just be those avatars, then one of those users can come along, click their avatar, it will make it fully opaque and add a green border for example and then they can click it again to toggle it back to its original state. I'm not sure how this would work for approvals where a set of approvers isn't specified or is super long. 🤔
Please ignore if you don't think it feels like it could get cluttered, I have some strange opinions. 😁
The thing here is, that in the old mr widget design. these dividers were just always added. It didn't make sense.. https://gitlab.com/gitlab-org/gitlab-ee/issues/1126#note_26490486 is not without fault in that, as there is a divider between "ready to be merged" and the "accepting this merge request..." etc.. While these 2 lines should have a more intricate relation ship instead of being just separated by a divider.
So to conclude ^^ yes i think we can improve the design of the mr widget with dividers, as long as we make sensible decisions as to where to put those dividers and how many we use.
a preview of this change would mean it to be looking like this:
I think this is correct, and we should proceed into adjusting it,
Thanks @lbennett for the comments! That's great! I looked closer at @hazelyang + @dimitrieh 's designs and just noticed they are slight different regarding the avatars.
In @hazelyang 's, I think the top row shows qualified approvers who have not yet clicked approve yet. And once they have clicked approve, the avatar essentially moves to the second row.
In @dimitrieh 's it looks like the top row always persists (and they are slightly greyed out). And when a person approves, another avatar appears on the second row. It doesn't move.
So different paradigm. I think I prefer @hazelyang 's . But a weakly held opinion given all the crazy complexity!
@lbennett 's idea of a single string of avatars (I think that's what you are suggesting right?) seems tempting , and you could easily just have a boolean toggle on each avatar to indicate approval/unapproval like you're saying? But I'm not sure you could easily communicate all the other information with that design.
And just to add even more complexity, we have approval groups, which @hazelyang 's mockup shows with the tanuki avatar. That gets even more confusing because anybody in that group is a qualified approver. In @hazelyang 's design, this works well because you can show just the tanuki (and not all 100+ users) on that line, and if any one of those users approve, they appear on the second line.
These are related issues undergoing work, and also captures some of the complexity of approvals:
So different paradigm. I think I prefer @hazelyang 's . But a weakly held opinion given all the crazy complexity!
@victorwu agreed here! this has the added benefit of instantly being able to see who is able to still approve, if there are persons explicitly labels as possible approvers
@dimitrieh@hazelyang : This one is scheduled for 9.2. Can you please finalize the design and update the description with the finalized mockups. We'll probably have a kickoff on this one in a few weeks.
@hazelyang It does look a lot cleaner, but if there is only 1 divider, it looks like we've intentionally grouped something with the pipeline component. Maybe we can have 2 but make them a lot more subtle? I'm not sure. 🤔
@fatihacet will you be able to tackle this for 9.2 as well? I want to point out that especially the dividers make a lot of sense for the overall design, even without this issue.
This is better @hazelyang. I still feel that it is not very clear there is a merge conflict. It feels disconnected to me. The red x is my visual queue that something is wrong, but it comes 2 lines above the message that there are merge conflicts. In most systems the warning icon is paired with the problem and steps to resolution.
@sarrahvesselov pipelines do not convey if there is a merge conflict, these can be two separate issues. However i think the core of what you are saying makes sense. The idea of that there are "merge conflicts" isn't communicated clear enough yet. However this is an issue that is not related to this issue imo.
@victorwu this is assigned me as a Deliverable but I don't see this issue assigned to me? Where are we at with this? Is this something FE can work on it? Please let me know. Thanks 👍
thanks @fatihacet . Yes, this is something we scheduled for 9.3. Typically I don't assign developers to issues. Sorry if we didn't do a good job organizing this.
I am a little confused on something here @dimitrieh. You created a new issue to deal with pipelines gitlab-ce#32614 as a separate concern. The design on that issue is vastly different from @hazelyang design on this issue. I feel like we are doing double and sometimes triple work in breaking out some of these issues into smaller and smaller pieces. Which is the SSOT? Aren't we just creating more work for everyone this way?
@psimyn when you start working on this issue, can you let me know how things are currently done with insane amounts of approvers? either eligible approvers, or approvers who already have approved ;|
This looks great, I'm really excited for this change!
One small suggestion: the additional approval tooltip text ("You are encouraged to approve it additionally, as someone might unapprove in the future") seems a bit strange to me. It's true that the user should be encouraged to approve additionally, but not because someone might unapprove in the future. Presumably, if someone does unapprove, it's a signal of "wait, something is wrong" and the MR shouldn't proceed just because it has received an extra approval.
Sure, I don't see any problem with removing it. I agree that the text is a bit odd.
Presumably, if someone does unapprove, it's a signal of "wait, something is wrong" and the MR shouldn't proceed just because it has received an extra approval.
I agree with this. However, I don't believe that un-approving will stop it from proceeding if it already has enough approvals. If an MR needs 2 approvals and has 3, then 1 person un-approves, it will still have the 2 needed and will proceed, correct?
If an MR needs 2 approvals and has 3, then 1 person un-approves, it will still have the 2 needed and will proceed, correct?
@sarrahvesselov Correct, this is how GitLab's flow currently works. However, I'm not sure this is the best flow. I really like how Phabricator does this, where in addition to "approving" a diff, you can also "request changes to" (i.e. reject) a diff. The diff then requires your eventual re-approval to proceed. This is a great way of manually signaling "hold up, something is wrong and needs to be addressed" in a way that actually blocks the merge. I think even Github does this now.
Currently, I have a few options to signal this in GitLab, but none of them actually prevent the MR from proceeding. Unapproving still allows someone else to approve and merge, or worse: there are already >=n+1 approvals so my unapproval does nothing at all. Similarly, starting a discussion that must be resolved still allows anyone to resolve the discussion and merge.
As an aside, I'm curious whether the UX team working on MRs has looked much at the Phabricator MR flow. In my experience, the general industry sense seems to be that Github has not solved MRs very well whereas Phabricator has an extremely thoughtful, robust MR flow, so I was surprised to find that GitLab models its MRs so closely after Github's. For example, here are some common cases that Phab solves for but GitLab/Github do not:
Notifying an approver when there is a MR they can and/or must approve. GitLab only notifies you if you are the single "assignee" and does not distinguish between reviews you are blocking and reviews that others can unblock.
As mentioned above, allowing a developer reject an MR (even if they were not initially an approver). An approval from that developer is then required for the MR to proceed.
Batching code comments together with global comment(s) and an approval/rejection as a single action, resulting in a single notification. (This goes further than the "batching comments" feature that GitLab is currently working on.)
(Re-)notifying the appropriate set of approvers when code changes are pushed to an MR.
Allowing a developer to commandeer ownership of an MR, to indicate that they are now point on code changes.
Providing a sane dashboard of MRs a user is involved in, grouped by category (can review, must review, waiting for changes, etc.) GitLab can only show you a flat list of MRs for which you are the assignee.
Allowing a developer to universally require their own review of MRs touching certain code/files within a project. Phab provides hooks which automatically add you as an (optionally pre-blocking) reviewer under configurable conditions.
Providing fine-grained control over notifications for different MR actions, depending on a user's relationship to that MR.
I'm happy to start one or more issues if you think this warrants further discussion. I realize that GitLab has considered implementing a few of these features individually, but I'm interested in hearing about your overall approach towards conceptualizing the MR flow.
I'm not sure this is the best flow. I really like how Phabricator does this, where in addition to "approving" a diff, you can also "request changes to" (i.e. reject) a diff.
I am very familiar with this flow thanks to BitBucket. There you can make comments, deny, or approve. I agree with you, the ability to deny something feels like a much bigger way of saying, "hold up, something's wrong!", than simply making a comment.
Github has not solved MRs very well
Agreed, I find their flow to be very visually confusing.
I'm curious whether the UX team working on MRs has looked much at the Phabricator MR flow.
@dimitrieh has been the driving force behind the MR changes, I will let him chime in on this. I have not worked much with Phabricator myself but it sounds like I need to jump in and take a closer look.
I'm happy to start one or more issues if you think this warrants further discussion. I realize that GitLab has considered implementing a few of these features individually, but I'm interested in hearing about your overall approach towards conceptualizing the MR flow.
Please do, I would love to hear what others think about implementing some of these into our MR flow. Particularly notifying approvers and the ability to deny an MR. I cannot speak to our approach to the MR flow prior to my time here but we recently opened up a UX Research issue to investigate how the current flow is, and isn't, working. I felt it was important to take a step back and asses our current UX before making more changes. A big focus has been on clarity, making sure that users can understand what the MR and what the result will be once they merge it.
I really like how Phabricator does this, where in addition to "approving" a diff, you can also "request changes to" (i.e. reject) a diff.
@victorwu interesting point here from @billyjanitsch .. its actually more of a ~"feature proposal" as to an iteration of approvals on MR's... as in "raising a red flag".
@billyjanitsch great comment, i am sure to check out phabricator (i put this on my todo list ;) ). As for your points, I have added some first thoughts to some of them:
As mentioned above, allowing a developer reject an MR (even if they were not initially an approver). An approval from that developer is then required for the MR to proceed.
I wonder how this would work with if that dev becomes unresponsive 😉 .. so any master should be able to proceed and unblock I would assume.
(Re-)notifying the appropriate set of approvers when code changes are pushed to an MR.
Does this work on every commit pushed? That would be perhaps to much.. i think. Because a dev may not have the code be ready yet.. so what about approval request button to notify approvers 😉
Allowing a developer to commandeer ownership of an MR, to indicate that they are now point on code changes.
This points to assignee roles.. this is a questionable topic. @victorwu knows more about our thinking here.
Providing a sane dashboard of MRs a user is involved in, grouped by category (can review, must review, waiting for changes, etc.) GitLab can only show you a flat list of MRs for which you are the assignee.
@tauriedavis@cperessini are involved in dashboards :) perhaps its nice for them to check out phabricator as well for this!
Allowing a developer to universally require their own review of MRs touching certain code/files within a project. Phab provides hooks which automatically add you as an (optionally pre-blocking) reviewer under configurable conditions.
Providing fine-grained control over notifications for different MR actions, depending on a user's relationship to that MR.
This ties into our todo's, which is a topic I hope @victorwu@sarrahvesselov can say when we will begin improving this (also to manage ourselves better! 😉 )
I'm curious whether the UX team working on MRs has looked much at the Phabricator MR flow.
@billyjanitsch GitLab is a fast moving product, even so many areas are always in need of some touching up + introducing features and improvements. Therefore only so much can be done each release. Recently (with the revamp of the widget to have a better technique behind it and it becoming real time) have we started to see real improvements becoming increasingly frequent to the MR view. Also as the MR view is heavily tied to the CI/CD experience, is it so that I have quite a lot of idea's still needing to be worked out.
We try to get the best from what we see from others, and invent even better ways to get that in here. I hope we'll have an even better flow in the near future and only encourage you to open issues (please cc me.. although it may happen i wont join the discussion immediately).
@hazelyang This issue shouldn't need any UX anymore for it, but would you be willing to stand by for me 😉, while I am gone?
This was originally set for %9.5 but during the scheduling last month, we determined that it wasn't as high of a priority as other deliverables. Not sure if @victorwu wants to set this for %10.0 or 10.1. We are doing scheduling meeting in a couple hours.
@smcgivern@ClemMakesApps : I think this was pushed back a few times. I wasn't around for 9.5 scheduling. Sounds like this was never realistic anyways. Since 10.0 is pretty much settled, let's put this in 10.1 for now.
@ClemMakesApps : Please do let me know if there's anything in 10.0 that doesn't fit after your meeting.
As you can see in the following image, the merge button is still being disabled due to not enough approvals. This is however not explained in the "merge" section of the mr widget.
GH has an exact example of how they did this (only if approvals is required of course)
Here it will look like:
Updated designs:
Requires 1 more approval, but cannot approve yourself
Requires 1 more approval, you can approve yourself
Requires 1 more approval, you can approve yourself, there is however someone else specified to approve
Requires 1 more approval, you can remove your approval
Received exact amount of required number of approvals, you can approve additionally
Received more than the amount of required number of approvals, you can approve additionally
We should be careful with the text copy. In general, I want to avoid introducing new terms if at all possible. And just use "plain" words. For example, "sufficiently" is something we should probably avoid because it carries a lot of weight, but is not a GitLab concept. So perhaps something more straightforward
like Not enough approvals. I don't need the word "Blocked", because the disabled/greyed-out button implies it already. The approvals requirement is right above. So it's obvious what "not enough means".
Approve additionally adds a new term, i.e. "additionally". I think that is good, because we are introducing a new concept here, i.e. that you can approve on top of the required amount. The UI has grown on me since you designed it a while back.
The Merge locally button I think is really good! And the placement makes total sense. The fact that it's a button aligns with the green Merge button.
For Received 2 required approvals by [AVATAR-1][AVATAR-2] or [APPROVE-ADDITIONALLY-BUTTON], I think we should remove the "or" text. (Same as the other case.) Those are two statements. The first statements tells us the number of approvals received. And then there's another call-to-action statement telling the user we can "approve additionally".
For the last mockup, 2 out of 1 seems weird. People might think it's a bug. I totally understand what you are trying to convey though. Suggestions below.
In the first few mockups, you have Approved by [AVATAR] in the second line, in the latter mockups, you show the approval avatars in the first line. I'm worried that that difference design is a little complicated with all the edge cases.
To indicate who has already approved, we can use avatars of that specific person. To indicate who can approve, there are three cases:
A specific person who was specified. We should use the avatar.
A group who was specified. We should use the group image.
Implicit approvers. Which is harder to calculate on the fly. So we shouldn't show an avatar/image.
To add more complexity (see here: https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html), you can have a scenario where you've specified you one eligible approver, but you require 3 approvals total. So the UI needs to account for that. I think we can tweak the wording a bit to be more generic so we don't have to account for every single case explicitly.
How about tweaks like this:
When viewing these designs, consider the user as Victor Wu.
@smcgivern : Can you look at my design above and see if we missed anything with regard to individual approvers? It's definitely missing the group approvers case. I wanted to clarify the following before we added those scenarios.
If there are more approvers than required approvals, any subset of these users can approve the merge request.
If there are less approvers than required approvals, all the set approvers plus any other user(s) need to approve the merge request before being able to merge it.
If the approvers are equal to the amount of required approvals, all the approvers are required to approve the merge request.
Suppose the specified approvers are:
Sean
Victor
Group A with 100 users
Suppose that the number of required approvals is 5.
Per the sentence above, both Sean and Victor have to approve the merge request (as part of unblocking it). But for the remaining 3 approvals, do they have to come from Group A?
@victorwu yep, the remaining approvers have to come from Group A. If Group A had two members (excluding Sean and Victor), then the fifth approval would have to come from someone else with access to the project - if there's no-one left, we reduce the number of approvals required.
The designs look good to me, I like the solution to the '2 out of 1' problem 👍
yep, the remaining approvers have to come from Group A. If Group A had two members (excluding Sean and Victor), then the fifth approval would have to come from someone else with access to the project - if there's no-one left, we reduce the number of approvals required.
@smcgivern just so i get this right: Someone with write access to the project can always approve? Even if only certain members and groups are specifically mentioned?
Or is it so that if the amount of approvers who are specifically mentioned (including groups) exceeds the required amount of approvers, the mr can only be approved by them?
@victorwu great stuff! thanks for pointing that out! I like your solutions! I'd left a few out! Let me adjust the mockups and update the description with:
@dimitrieh if you aren't a named approver, or a member of an approver group, you can (currently) only approve if there are more approvals required that people who are named approvers or members of approver groups.
Regarding group approvers, I assume the design will be similar. So feel free to include those cases too. I assume you'll use the group avatar in that case right? But I'll let you take a stab at that first before we discuss further.
@victorwu@smcgivern here are the updated designs, should make sense (can you check for mistakes?)
updated issue description
Design
Screenshot are from the perspective of:
Requires 1 more approval
cannot approve or remove approval:
can approve:
can remove approval:
Requires 5 more approvals ("approval" becomes "approvals")
cannot approve or remove approval:
can approve:
can remove approval:
Requires 5 more approvals with 2 specified eligible approvers
cannot approve or remove approval:
can approve:
can remove approval:
Requires 4 more approvals with 4 specified eligible approvers
cannot approve or remove approval:
can approve (you are a eligible approver, last in the list):
can remove approval:
Requires 4 more approvals with 2 specified eligible approvers and 1 specified eligible approver group (with enough people in it to get all required approvals)
cannot approve or remove approval:
can approve (you are a eligible approver, last in the list):
can remove approval:
Requires 8 more approvals with 2 specified eligible approvers and 1 specified eligible approver group (withuot enough people in it to get all required approvals)
cannot approve or remove approval:
can approve:
can remove approval:
Received 2 approvals when 2 are required
cannot approve or remove approval:
can approve additionally:
can remove approval:
Received 3 approvals when 2 are required
cannot approve or remove approval:
can approve additionally:
can remove approval:
If their are no approvers yet, the second line is not shown:
If the MR is blocked by approvals, it should show like:
Notes:
If there are less eligible specified approvers (including those in the specified approver groups), and others is mentioned.
If you yourself can approve, your avatar always shows last in the eligible approver avatar list on the first line.
If you yourself did approve, your avatar always shows last in the approver avatar list on the second line.
If you aren't a named approver, or a member of an approver group, you can (currently) only approve if there are more approvals required than people who are named approvers or members of approver groups (this currently is already functioning this way). Please see the following doc: https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html
Can we change the blocked merge request info sentence to be simply. Not enough approvals. The disabled merge button already communicates that the mr is blocked from merging. And we just need a simple phrase to communicate why next to it. So simply Not enough approvals suffices. Right above that phrase it will say Requires x more approvals. So at a glance, it will be obvious what it all means together.
In your design it seems that you have and othersonly if there are avatars and non-specified approvers required. If there are no avatars, and just non-specified approvers. Then we don't have to include and others, because it's implied. I think that is a good design.
Received 3 approvals when 2 are required: Small correction. Second mockup here needs an extra avatar.
@smcgivern@dimitrieh : I added a few issues (see description) to break down the designs here into multiple smaller issues. Feel free to take a look and comment.