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.
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.
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, but it does not mention the API behavior specifically.
Steps to reproduce
- Create a project
- Query the approval API
- Compare API result with the settings in the UI
- 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
-
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.)
-
Or adjust the fontend to interpret a missing value as "checked"