Skip to content

Override approvers setting does not reflect actual behavior

Summary

When querying the approval endpoint on a new project where disable_overriding_approvers_per_merge_request was never ticked in the UI, the API returns a null value for this setting.

{
  "approvers": [],
  "approver_groups": [],
  "approvals_before_merge": 0,
  "reset_approvals_on_push": true,
  "disable_overriding_approvers_per_merge_request": null,
  "merge_requests_author_approval": null,
  "merge_requests_disable_committers_approval": null,
  "require_password_to_approve": null
}

A client would assume that this setting is defaulting to "false", meaning "approvers can overridden", because it is not disabled (double negative).

When testing merge request creation, I can indeed override the needed approvers.

2020-12-03_13-52_1

In this state, the UI for this checkbox (labeled "Can override approvers and approvals required per merge request" which has the inverted meaning) is not ticked, which does not reflect the actual behavior.

2020-12-03_13-52

If I check the checkbox, save, uncheck and save the API returns "true" correctly and the UI and API are in sync.

Other values could be affected, I did not check specifically.

It would be preferable to not rely on null values for booleans in the API, because then the client needs to speculate which might be the default behavior.

A related issue is #276741 (closed), but it does not mention the API behavior specifically.

Steps to reproduce

  1. Create a project
  2. Query the approval API
  3. Compare API result with the settings in the UI
  4. Create a new MR

Example Project

It is trivial to reproduce on a new project.

What is the current bug behavior?

  • API returns null for disable_overriding_approvers_per_merge_request
  • Merge requests approval can be overridden
  • UI shows a not-ticked checkbox

What is the expected correct behavior?

  • API returns true for disable_overriding_approvers_per_merge_request
  • Merge requests approval can be overridden
  • UI shows a ticked checkbox

Output of checks

This bug happens on GitLab.com

Possible fixes

  1. Create entries in the database using default values on project creation (I assume this would work, because the behavior is correct after the first save.)

  2. Or adjust the fontend to interpret a missing value as "checked"

Edited by Christoph Raschl