Skip to content
Snippets Groups Projects

Approve additionally

Merged Jose Ivan Vargas requested to merge 4134-approve-additionally into master

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)

Screen_Shot_2018-01-30_at_11.14.06_AM

The approve additionally button

Screen_Shot_2018-01-30_at_11.14.16_AM

Requires n approvals

Screen_Shot_2018-02-01_at_11.57.55_AM

Requires n more approvals with the remove approval button

Screen_Shot_2018-02-01_at_12.01.01_PM

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #4134 (closed)

Edited by Tim Zallmann

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
  • @jivanvl Just one minor comment but the change on BE is ok - thanks!

  • Author Maintainer

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

  • Jose Ivan Vargas added 700 commits

    added 700 commits

    Compare with previous version

  • added 1 commit

    • 30b5f6e9 - Corrected integration specs and added some javascript improvements

    Compare with previous version

  • Jose Ivan Vargas resolved all discussions

    resolved all discussions

  • Jose Ivan Vargas added 2 commits

    added 2 commits

    Compare with previous version

  • Jose Ivan Vargas marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • Author Maintainer

    @dimitrieh Can you take a look on the UX side please? Thanks!

  • Author Maintainer

    @MadLittleMods Can you take a look on the frontend side please? Thanks!

  • assigned to @jivanvl

  • @jivanvl

    posted the following image in the issue description for additional clarity:

    image

    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 Hoekstra
  • Jose Ivan Vargas added 253 commits

    added 253 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Jose Ivan Vargas added 253 commits

    added 253 commits

    Compare with previous version

  • Jose Ivan Vargas added 182 commits

    added 182 commits

    Compare with previous version

  • Victor Wu
  • Victor Wu
  • Victor Wu
  • 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 Wu
  • Resolving my earlier comments because not necessary. Chatted with @jivanvl and he will update the code per the issue description.

  • Jose Ivan Vargas added 357 commits

    added 357 commits

    Compare with previous version

  • added 1 commit

    • 26255132 - Modified approver logic to be less strict

    Compare with previous version

  • Jose Ivan Vargas resolved all discussions

    resolved all discussions

  • Author Maintainer

    @jarka Can you take a look at the backend code to see if the changes make sense? Thanks!

  • assigned to @jarka

  • @jivanvl I think I need an explanation of what we're trying to achieve. Thanks!

  • @jvargas Please check my discussion with @victorwu . I'd also like to add the specs I posted in that discussion to make sure we have that case covered.

  • Author Maintainer

    @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
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading