Remove approval in merge request

mentioned in issue #761

related https://gitlab.com/gitlab-org/gitlab-ee/issues/761

Mentioned in issue #580

Milestone changed to %8.14

Added ~126459 label

We might perhaps want to add system messages for this feature in order to provide some sort of audit trail to give information on what happened.

Mentioned in issue #855

@regisF Yes, good point.

Reassigned to @hazelyang

Unapprove button (When other approvals are pending)

Remove approval (All the approvals are approved)

I didn't even know approvals was a feature of GitLab haha Neat!

@JobV I'm curious what the reasoning is for having the buttons be different in these different states? Unapproval vs. remove approval - I'm not sure I see the reason for changing these wordings/colors based on whether or not all approvals are submitted?

Side note: Where is the merge button when approvals are needed?

@tauriedavis it's hidden! If approvals are enabled, no-one can merge until the minimum number of approvals has been added.

@hazelyang @JobV I'm a little confused - is 'unapprove' the same as 'remove approval', or is it registering a blocker for the MR? If it's the former, I don't see why these buttons wouldn't be the same.

I am also a bit confused. If both actions are the same, then the only reason I could think of is that we want to discourage "remove approval", which somehow makes sense because then I am certainly the one who blocks this merge request. So I must have a strong reason.

However I might still prefer to just make it a switch -- on or off like how like and unlike works, and use another way to show if the merge requests are approved from all approvers. Because this is less confusing.

@hazelyang I'm confused too, and I would add that on the second screen, it's not clear whether the Unapprove button will unapprove all the votes or just mine.

@smcgivern There could be a state where some is an approver and a merger. So in the second mockup (All the approvals are approved), there could be a merge request button AND a removal approval button. But not for every approver, if they don't have permission to merge. Is this right?

@tauriedavis aha, you're right! Yes, good point - this is indeed complicated. Someone with merge permissions and who has already approved the MR would see both buttons.

"Unapprove" and " Remove my approval" both mean people can remove their approvals until the brach is merged. Maybe we could have the button with the same style and same text for the button?

Still need an approval from the approver

Just a thought: What if we have a dotted circle to indicate the status is pending? I think it's more direct than only has " Require one more approval from...".

All approvers approved the merge request

State for someone is an apporver and merger

I like the idea having dotted circles to indicate how many approvals we still need!

Not sure if it would go crazy when people set the requirements to 50 though, haha. Imagine people use that to vote...

The dotted circles look really nice

On staging, though, there are some with the cases that @godfat was worried about Not many, but some.

irb(main):001:0> Project.maximum(:approvals_before_merge)
=> 1000
irb(main):002:0> MergeRequest.maximum(:approvals_before_merge)
=> 66
irb(main):003:0> Project.where.not(approvals_before_merge: nil).reorder('approvals_before_merge DESC').limit(10).pluck(:approvals_before_merge)
=> [1000, 300, 184, 100, 69, 50, 45, 30, 20, 20]
irb(main):004:0> MergeRequest.where.not(approvals_before_merge: nil).reorder('approvals_before_merge DESC').limit(10).pluck(:approvals_before_merge)
=> [66, 55, 27, 11, 10, 5, 5, 4, 4, 4]

We could show up to 10 circles (or whatever fits best @hazelyang) and then a +100 more or something. Similar to what we do for participants.

@hazelyang much better!

One comment though: the dotted circle is within a list preceded by a label called approved by. So perhaps that would confuse people. What about instead, we change approved by to approvers, then add the avatars of people who have approved it first, with a green check on the top right corner, then followed by those dotted circle?

We could even have the avatar of the pending approvals with a ? symbol next to them?

We could even have the avatar of the pending approvals with a ? symbol next to them?

@regisF We have an use case like this: People set up Approvals required: 3 but have 4 approvers. We are disabled to predict 3 of the 4 approvers who will make the approvals. So I think the idea will not work in this case.

Thanks for your feedback! @tauriedavis @regisF @godfat @smcgivern

Requiring a lot of approvals (30+): Collapsed

Requiring a lot of approvals (30+): Expanded

@tauriedavis @hazelyang What about the case for +100 more however 999 had approved, and just waiting for 1?

I modified the design:

I removed the line between "Requires 1000 approvals" and Approvers part since I think they should look like they are in the same group visually.

@godfat I proposed another solution to the question you mentioned.

State01: The link "+990 more" will appear until 10 people approved.

State02: Click "+990 more", and then the modal will appear. The modal contains the approvers.

When there are 1000 approvals, the expanded list is very long and heavy. We could put the whole list of approvers into the modal to solve this problem. However, if the approvers <= 10, there is no need to have a modal.

@hazelyang Haha, sorry for ordering -1 and 999 beers :P

@hazelyang I'm confused by the mockups now

In the first mockup, 1002 approvals are needed - is that right? 1000 more, plus the two that already approved?

Then in the second mockup, I'm unsure of how many have approved vs. how many are needed. It looks like 10 approved and there are 990 left.

I think Im getting confused because in the first mockup, it says how many more approvals there needs to be, but in the second it shows the total.

Really like the change for getting rid of the line between "Requires 1000 approvals" and Approvers

In the first mockup, 1002 approvals are needed - is that right? 1000 more, plus the two that already approved?

@tauriedavis My fault. I think it should be 998 approvals, right?

If there are no one approved. => 1000 approvals are needed. If there are 2 people approved. => 998 more approvals are needed.

@hazelyang this is looking great. Great work poking straight through the complexity.

i really like the way this issue has progressed, and @hazelyang's designs look great! Very easy to see what's going on within the MR.

Question: are there any plans to show Approval info on the Merge Request list page? i tend to revisit a lot of MRs i've already approved because i can't remember which i've completed and which are still waiting. i try to remember to reassign once i've approved so i can utilize the filters, but it would be helpful to have a visual cue on the list showing what i've already approved (or add this as a filter?).

Probably needs a separate issue, but i figured i'd toss the question out there...

Awesome! I think we can update the description with your latest mockup's @hazelyang and pass to frontend!

@cautionbug I can see the need for that! It should be easy to add that as a filter once we have our new filter designs in place. Created an issue here: https://gitlab.com/gitlab-org/gitlab-ce/issues/23921

Assignee removed

Mentioned in issue #1177 (closed)

Removed ~126459 label

Added ~874214 label

Added frontend label

Milestone removed

Milestone changed to %8.15

@smcgivern : Should this be separated into two issues since there's a lot going on?

First issue to take the existing approval functionality, and re-skin it with the cool circles.

And then the second issue to add the unapproving functionality with the additional required UI.

I updated the description, summarizing the discussion from the comments up to here, except for the large number of approvers scenario.

Thanks @godfat for bringing up that case. But it's definitely an edge case. We don't anticipate GitLab to be frequently used with a large number of required approvals. (You can have a large number of eligible approvers. But the required number of approvals would not typically be more than a handful of people.) If an organization requires a large number of approvals to review a merge request, then likely they have a process which is outside of the scenarios that we are planning for. In that case, we need to more carefully consider their process, and provide a better solution.

So it's not a good use of our resources to design for and implement a UI to specifically address this right now.

But if it does turn out that someone has this scenario in a rare case, the mockups in the description (and the implementation) would just naturally extend to this, which is totally fine for now. At least it is not broken.

@victorwu that makes sense to me. I think part 1 can be purely ~Frontend, and part 2 is mostly Discussion.

Mentioned in issue #1262 (closed)

Thanks @smcgivern : I've created #1262 (closed), and updated the description here linking to it.

Added Deliverable label

@smcgivern : Will the unapprove action automatically be added to the thread of the merge request? Or should that be called out explicitly in this issue? Or should it be logged as a separate issue to work on?

Same question for notification emails.

picking this up this week.

@awhildy + @hazelyang : What do you think about going with "unapprove" as the consistent verb in this and all future features? So primarily, if we like that term, we should update the mockup to be "Unapprove" and not "Remove my approval".

The primary reason is that then we will be consistent everywhere, and give the user a congruent experience. For example:

Victor approved merge request !12345
Victor unapproved merge request !22345 Which would show up in system notes, emails, etc.

These are probably harder to enforce (style guide-wise), and probably a little messier to code:

Victor approved merge request !12345
Victor removed their approval for merge request !22345

(I'll be adding new issues for email notifications and system notes shortly.)

I'm mainly arguing from a consistency perspective, perhaps sacrificing readability and grammatical correctness in some cases. "Removing" connotes taking away something you already gave, i.e. the approval. Whereas "unapprove" is like flipping a boolean switch. It's a subtle distinction, and "Remove" I think is better for this one specific case. But I ultimately vote for "Unapprove" knowing that going forward, we will have many more features (such as sending fine-grained negative code review signals in a merge request), and perhaps the flipping-a-boolean-switch mental model is easier for users, and easier for text copy.

Just for fun: I believe Stash/Bitbucket uses both "disapprove" and "unapprove" and its variations in different contexts. I remembering submitting a ticket to them expressing my disapproval of them using "disapprove" (pun intended). And they disagreed with me. Please please let's not use "disapprove" because that's a totally different meaning! We can argue about "reapprove" later if it ever comes up.

@victorwu You reminded that I saw someone was using dislike when they actually mean unlike. Yeah unlike doesn't mean unlike, but using dislike looks more wrong to me. For the same reason I prefer unapproved, too. It's just simpler and more consistent.

@godfat @victorwu: While I much more prefer the simplicity of Unapproved, the problem is that Unapprove doesn't seem to be a word (interesting similar conversation: http://forum.wordreference.com/threads/disapprove-unapprove.2767115/). It is alright to say past tense (Victor unapproved this MR), you can't use the present tense verb, which is what the UI needs.

Because of the limits of English, I'd suggest Undo approval as the button in the UI, and Victor unapproved merge request !22345 in the system notes and emails. Thoughts?

@awhildy Ahh yes. English sucks since it wasn't invented with software interfaces in mind.

If we go with your format, we will have these choices:

Remove my approval
Remove your approval
Remove approval
Undo my approval
Undo your approval
Undo approval

I actually think the "Remove" versions make more sense, since "Undo" often is used in the context of correcting a silly error or backing out of a rash decision, usually within a few seconds or minutes. In this use case, after a long code review discussion, somebody may want to remove their approval. So that could be two days later.

I'll let you folks make the final decision!

Hehe. Fair point @victorwu. Maybe we just drop the 'my' from the original comps to keep it simpler and more consistent (and avoid the whole my/you conversation). It should be quite clear that you can only remove your approval, and it is a command that is super easy to recover from (by just approving again)

So, the command is Remove approval, and the system note and email is unapproved.

@hazelyang: could you update the comps to say Remove approval instead of Remove my approval. Thanks!

@awhildy I guess that would be easier for people seeing this the first time :)

@victorwu I don't really feel undoing a decision made 2 days ago would be weird, but since I am not native English speaker nor am I really good at English, I would never feel strong in this regard. I think this is also the top reason why I prefer consistency over semantics. Any decision would be good to me :)

While correct language usage is important, it is not the primary purpose when we create GitLab. Language is a crucial component to UX, and that's why at least I enjoy the detailed discussions here and believe it's very important we get it as close to a good experience as we can. Language is indeed extremely messy and difficult in UX. Good language helps users read and flow through the app experience seamlessly. Text is often not in content form (paragraphs), but in buttons, headings, labels, etc. So at least IMHO, there's certain situations where we should sacrifice grammatical correctness if it serves the greater good. Language itself is always evolving, with digital interfaces being a large component of how humans interact day to day. So I see correct grammar and spelling as a guidepost that we should start with as a default, and see how we can evolve from there. Correctness itself is a moving target, and new modern words and phrases are being added to "official" dictionaries every year.

All this to say, @godfat , that I appreciate your feedback, and native-English speaking is just one aspect of the discussion, and your comments are just as valuable. We are obviously creating software for all people to use, and we need a diverse set of feedback to make the best product possible.

That being said, we have to make a decision at some point, so we don't succumb to analysis paralysis. The good thing about software and GitLab is that we ship quickly and often. So getting it right is important. But just getting it done is our short term goal, we then we iterate afterward.

Changed title: Unapprove merge request → Remove approval in merge request

@awhildy Sure. Updated!

Throwing my hat in for "unapprove". It may not technically be a dictionary word, but it's made up of simple enough blocks ("un", "approve") that it's relatively easy to understand, and has the advantage of carrying the connotations that we want here - especially as we may add "disapprove" (or "block") later.

My preference for verbiage is "Remove Approval" (or even "Withdraw Approval") - i think it stands a much better chance of simple translations. No pronouns, fewer English-to-LanguageX idiosyncrasies...

Email template could then look like "Victor removed approval for merge request !0000" or "Victor withdrew approval for merge request !0000"

All the "un-" words keep reminding me of debates i have with my team about if/then structures that lead with a negation:
if (not-something) { true because it's NOT something } else { false because it IS something }

But as they would tell you, i'm weird.

@victorwu we should add system notes and explicitly call that out

Reassigned to @brycepj

Thanks @jneen and @cautionbug. Totally get the different opinions and concerns. I think it is a very subjective thing. I'm cautious about using something that isn't a word as it may be harder to translate, etc. I'd like to stick with Remove approval, but will definitely pay attention to feedback, and consider if there is anything that tips the scale. Thanks much!

mentioned in issue #885

mentioned in merge request !949 (closed)

mentioned in merge request !906 (closed)

mentioned in merge request !954 (merged)

changed milestone to %8.16

removed Deliverable label

added Stretch label

closed via merge request !954 (merged)

mentioned in commit eb518c89

Remove approval in merge request

Resources

Problem

Dependencies

Smallest iteration in scope

Out of scope

Design considerations

Design mock-ups

Links / references

Designs

Child items ...

Activity

Unapprove button (When other approvals are pending)

Remove approval (All the approvals are approved)

Still need an approval from the approver

All approvers approved the merge request

State for someone is an apporver and merger

Requiring a lot of approvals (30+): Collapsed

Requiring a lot of approvals (30+): Expanded

State01: The link "+990 more" will appear until 10 people approved.

State02: Click "+990 more", and then the modal will appear. The modal contains the approvers.

Admin message

Admin message

Remove approval in merge request

Resources

Problem

Dependencies

Smallest iteration in scope

Out of scope

Design considerations

Design mock-ups

Links / references

Activity

Unapprove button (When other approvals are pending)

Remove approval (All the approvals are approved)

Still need an approval from the approver

All approvers approved the merge request

State for someone is an apporver and merger

Requiring a lot of approvals (30+): Collapsed

Requiring a lot of approvals (30+): Expanded

State01: The link "+990 more" will appear until 10 people approved.

State02: Click "+990 more", and then the modal will appear. The modal contains the approvers.