Selecting "No one" in branch permissions is confusing

I noticed in the "Allowed to push" role, I can select "No one" and the other roles as well:

Also, I noticed I can't select "No one" in "Allowed to merge". Is this expected?

/cc: @alfredo1

@stanhu that's how it was on the mockups https://gitlab.com/gitlab-org/gitlab-ee/issues/179#note_13361986 I'm not entirely sure about why we put that option. maybe @timothyandrew can answer this.

@tauriedavis Would love to get your thoughts here too!

You shouldn't be able to select "No one" and other roles. "No one" should appear at the top of list so it's not grouped in between multi-select options. If "No one" is selected and a user selects another role or user, "No one" should be unchecked.

Also, I noticed I can't select "No one" in "Allowed to merge". Is this expected?

Yes. The default behavior with a protected branch is "Masters can push / merge". The "no one can push" toggle allows the behavior to be "No one can push / merge". There was no need for something like "Masters can push, but not merge", since merges are a subset of pushes.

I'm not entirely sure about why we put that option.

It was added in #18193 (closed). The issue should have more context.

If "No one" is selected and a user selects another role or user, "No one" should be unchecked.

This makes sense. @alfredo1: I'm guessing this might be easier to handle in the frontend, but I can take a stab at implementing it at the model-level too. I'd love to hear your thoughts here.

/cc @stanhu @tauriedavis

Started branch 21153-selecting-no-one-in-branch-permissions-is-confusing

This issue will be fixed through https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645.

Not an issue here on CE since on CE you can't select multiple options to protect a branch.

@jameslopez can you help me to fix this failing spec?

1) Import/Export model configuration has no new models
     Failure/Error: expect(new_models).to be_empty, failure_message(model_name.classify, new_models)

             New model(s) <group> have been added, related to MergeAccessLevel, which is exported by
             the Import/Export feature.

             If you think this model should be included in the export, please add it to IMPORT_EXPORT_CONFIG.
             Definitely add it to MODELS_JSON to signal that you've handled this error and to prevent it from showing up in the future.

             MODELS_JSON: /Users/asumaran/www/gdk-ee/gitlab/spec/lib/gitlab/import_export/all_models.yml
             IMPORT_EXPORT_CONFIG: /Users/asumaran/www/gdk-ee/gitlab/lib/gitlab/import_export/import_export.yml
     # ./spec/lib/gitlab/import_export/model_configuration_spec.rb:27:in `block (3 levels) in <top (required)>'
     # ./spec/lib/gitlab/import_export/model_configuration_spec.rb:25:in `each'
     # ./spec/lib/gitlab/import_export/model_configuration_spec.rb:25:in `block (2 levels) in <top (required)>'

@alfredo1 as you mentioned on slack:

adding group_id to ProtectedBranch::MergeAccessLevel and ProtectedBranch::PushAccessLevel on safe_model_attributes.yml fixes the failing spec

that is the appropriate fix in this case :)

@jameslopez I made a mistake, the comment above should have been made here https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/645

Also, this error is different from the one I put on Slack. Can you please fix the issue or tell me how to fix it on the linked merge request?

Status changed to closed by merge request gitlab-org/gitlab-ee!645

Status changed to closed by commit gitlab-org/gitlab-ee@2b252c9a8e52a3d32800d197eb57a6b7b8d91189

mentioned in merge request !14785