Unable to decrease required approvals

Summary

When editing a MR, you can increase the number of required approvals, but not decrease it. This leads to situations where you cannot get enough approvals once you started with the wrong number. When clicking on Edit of the MR, it shows the correct amount of required approvals, however on the MR page itself it shows the maximum.

Steps to reproduce

Create a MR, set a number of approvers (e.g. in our case 4). Edit the MR and reduce the number of approvals required. The page will still show 4 circles and the MR cannot be merged.

Expected behavior

Correct amount of required approvals

Actual behavior

Maximum amount it once had.

Relevant logs and/or screenshots

On the left a MR, on the right the Edit page of that MR.

Output of checks

Gitlab 8.16 upgrade from 8.15.4 using omnibus. Both chrome and safari show the problem.

added ~978335 ~238305 ~91797 labels

changed milestone to %8.17

@mathieu6 thanks for reporting! Are you sure this didn't exist in 8.15 as well? This sounds like a duplicate of https://gitlab.com/gitlab-org/gitlab-ee/issues/1078.

I am not entirely sure it did not exist in 8.15 @smcgivern. It is since 8.16 broke some approval-related stuff that we noticed this issue while trying out some things, I did not pay attention to it before, so the issue might have been present already. From what I gather that issue is indeed related, however we can change the approval count, but only increase it.

@mathieu.hinderyckx I think that is the same issue and if so, @jarka has been working on a fix!

@jarka can you check and see if this is also solved by your open MR?

@mathieu.hinderyckx @smcgivern I am pretty sure this problem is a different one. The one I worked on was that a number on edit page was not displayed correctly, not related to showing MR or MR approval process. However I'll take this one and keep you updated

assigned to @jarka

@jarka so it was saving correctly in this case, even though the wrong number was shown on the edit page?

@smcgivern No, but the problem here is that even when you go to the MR edit page and decrease the number and then click "Save Changes" the number is not updated. And the problem is only when trying to decrease the number of approvals, increasing works correctly.

I already have an idea why (from merge_request_controller):

if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
  mr_params.delete(:approvals_before_merge)
end

I think there should be sth like

if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
  mr_params[:approvals_before_merge] = target_project.approvals_before_merge
end

Behaviour now

When a new MR is created with count of approvals higher than project settings you basically can never go back to the project settings.

Expected behaviour

You can always go down to project settings. So in case you edit a MR and insert count of approvals <= project settings it should fail to project settings.

@mathieu.hinderyckx could you please check the "Approvals required" on project settings and confirm that you can change the MR so that it requires that project settings + 1 ? (eg. project settings 2, merge request 3 -> this should work)

@jarka good catch!

I think the problem there is that if it's the project default, we don't want to set it on the MR directly, we just want to 'inherit'. So maybe we need mr_params[:approvals_before_merge] = nil inside that if?

Yes, that's better. Will create a fix for this

@jarka I can indeed set the approval count to project default + 1

mentioned in merge request !1106 (merged)

changed milestone to %8.16

I changed milestone because it has Next Patch Release label, so I suppose this should go in 8.16.2.

@grzesiek the issue milestone refers to the development month, so the correct milestone is 8.17, and Next Patch Release at this stage of the month means the previous release.

I know this is confusing, but this way we can have all developers look at the issues on the current milestone in priority order, and see what should be done first. We're trying to make it clearer, stay tuned!

changed milestone to %8.17

We're trying to make it clearer, stay tuned!

Thanks @smcgivern!

closed via merge request !1106 (merged)

mentioned in commit 145469f0