Approve additionally
What does this MR do?
Changes the current behavior for approving merge requests by allowing them to be approved with more people even if the merge request itself already has enough approvals.
Screenshots
3 approvals (approved additionally)
The approve additionally button
Requires n approvals
Requires n more approvals with the remove approval button
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug - Review
-
Has been reviewed by UX -
Has been reviewed by Frontend -
Has been reviewed by Backend
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together
What are the relevant issue numbers?
Closes #4134 (closed)
Merge request reports
Activity
Thanks @jvargas! Do you mind adding a few screenshots of the mr widget showing the different cases:
- Number of approvals given < Number required
- Number of approvals given = Number required
- Number of approvals given > Number required
@victorwu Of course!
Number of approvals given < Number required
Number of approvals given = Number required (Same user that just approved)
Number of approvals given = Number required (Another user)
Number of approvals given > Number required (Same user that just approved)
Edited by Jose Ivan VargasLooks great. Thanks @jivanvl. Looks like the button styling needs to be updated and the second "a" in
Approve additionally
needs to be lowercase, per the design in https://gitlab.com/gitlab-org/gitlab-ee/issues/4134. Other than that looks good.@victorwu Yeah I'm just waiting on Dimitrieh's response to the button styling (on the issue), I can change the copy text for the button right away
cc @jarka
@jivanvl : Oh sorry. That's what
info
is. My bad. thanks.- Resolved by Jose Ivan Vargas
@jivanvl Just one minor comment but the change on BE is ok - thanks!
@jarka Thanks a lot for checking the backend change, will work on getting it done, making the tests go green and we should be good to go for a review
added 700 commits
-
6b10c8c9...20ce2ec6 - 699 commits from branch
master
- 82cb11a3 - Merge branch 'master' into 4134-approve-additionally
-
6b10c8c9...20ce2ec6 - 699 commits from branch
added 1 commit
- 30b5f6e9 - Corrected integration specs and added some javascript improvements
added 2 commits
marked the checklist item Changelog entry added, if necessary as completed
@dimitrieh Can you take a look on the UX side please? Thanks!
@MadLittleMods Can you take a look on the frontend side please? Thanks!
assigned to @MadLittleMods
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
- Resolved by Jose Ivan Vargas
assigned to @jivanvl
posted the following image in the issue description for additional clarity:
cc: @victorwu (I don't want to add scope, but this does clean things up, let me know if you are alright with it)
@jivanvl Also, could you update the MR with screenshots to go off from? (ps see my slack discussion at https://gitlab.slack.com/archives/C3JMTNZKQ/p1516276262000042 )
Edited by Dimitrie Hoekstraadded 253 commits
-
3557345c...0438005a - 252 commits from branch
master
- 8e059d6d - Merge branch 'master' into 4134-approve-additionally
-
3557345c...0438005a - 252 commits from branch
added 253 commits
-
be1f037b...cb7efc60 - 252 commits from branch
master
- 197608bb - Merge branch 'master' into 4134-approve-additionally
-
be1f037b...cb7efc60 - 252 commits from branch
added 182 commits
-
197608bb...e6c0b419 - 180 commits from branch
master
- 62847719 - Changed approver logic
- a470ce58 - Merge branch 'master' into 4134-approve-additionally
-
197608bb...e6c0b419 - 180 commits from branch
- Resolved by Victor Wu
- Resolved by Victor Wu
- Resolved by Victor Wu
- Resolved by Victor Wu
@jivanvl : I think
approved?
needs to be changed (and will be simplified) right?def approved? approvals.count >= approvals_required end
And that gets rid of a bunch of code like
number_of_potential_approvers
right? Certainly I think we'd need @jarka or someone else on BE to review. But the simplified logic should be able to get rid of a lot of the existing complexity in the code? I'll let you and @jarka figure out the safest way to do this of course. Perhaps we need to do it in multiple issues/mrs? Let me know what you think.Edited by Victor WuResolving my earlier comments because not necessary. Chatted with @jivanvl and he will update the code per the issue description.
added 357 commits
-
a470ce58...049e7797 - 356 commits from branch
master
- fbdd5fef - Merge branch 'master' into 4134-approve-additionally
-
a470ce58...049e7797 - 356 commits from branch
@jarka Can you take a look at the backend code to see if the changes make sense? Thanks!
assigned to @jarka
- Resolved by Eric Eastwood
@jivanvl I think I need an explanation of what we're trying to achieve. Thanks!
assigned to @jivanvl
@jarka
Just to be clear, you're referring to this case right?What I understand from the issue is a bit different - in the beginning we have 2 required approvals and set of 2 users who can approve it => another user can never approve the MR because the required_approvals <= approvals_list.count
Nevermind I saw your spec scenario on the comments
Edited by Jose Ivan Vargas