1. This is in regard to the protected branches feature spec.
2. For example, if "Masters" is already selected, don't re-select
"Masters" during the spec.
3. This is due to a bug in the frontend implementation, where selecting
an already-selected access level _deselects_ it, which is something
we don't need. I'll create a separate issue for this.
4. This hasn't turned up before, because we were manually creating
missing access levels prior to e805a64. Now, we just use nested
attributes, and missing access levels fail validation.
!581 has a lot of changes that would cause merge conflicts if not
properly backported to CE. This commit/MR serves as a better
foundation for gitlab-org/gitlab-ee!581.
= Changes =
1. Move from `has_one {merge,push}_access_level` to `has_many`, with the
`length` of the association limited to `1`. This is _effectively_ a
`has_one` association, but should cause less conflicts with EE, which
is set to `has_many`. This has a number of related changes in the
views, specs, and factories.
2. Make `gon` variable loading more consistent (with EE!581) in the
`ProtectedBranchesController`. Also use `::` to prefix the
`ProtectedBranches` services, because this is required in EE.
3. Extract a `ProtectedBranchAccess` concern from the two access level
models. This concern only has a single `humanize` method here, but
will have more methods in EE.
4. Add `form_errors` to the protected branches creation form. This is
not strictly required for EE compatibility, but was an oversight
nonetheless.
1. It makes sense to reuse these constants since we had them duplicated
in the previous enum implementation. This also simplifies our
`check_access` implementation, because we can use
`project.team.max_member_access` directly.
2. Use `accepts_nested_attributes_for` to create push/merge access
levels. This was a bit fiddly to set up, but this simplifies our code
by quite a large amount. We can even get rid of
`ProtectedBranches::BaseService`.
3. Move API handling back into the API (previously in
`ProtectedBranches::BaseService#translate_api_params`.
4. The protected branch services now return a `ProtectedBranch` rather
than `true/false`.
5. Run `load_protected_branches` on-demand in the `create` action, to
prevent it being called unneccessarily.
6. "Masters" is pre-selected as the default option for "Allowed to Push"
and "Allowed to Merge".
7. These changes were based on a review from @rymai in !5081.
1. Align "Allowed to Merge" and "Allowed to Push" dropdowns.
2. Don't display a flash every time a protected branch is updated.
Previously, we were using this so the test has something to hook onto
before the assertion. Now we're using `wait_for_ajax` instead.
1. The model now contains this humanization data, which is the once
source of truth.
2. Previously, this was being listed out in the dropdown component as well.
1. Get the existing spec passing.
2. Add specs for all the access control options, both while creating and
updating protected branches.
3. Show a flash notice when updating protected branches, primarily so
the spec knows when the update is done.
1. Modify the component to support a callback for every key press in the
filter. We need this so we can update the "Create: <branch_name"
label.
2. Modify the component to use `$(<selector>).first().click()` instead
of `$(selector)[0].click()`, because the latter is non-standard, and
doesn't work in PhantomJS.
1. Doesn't seem like there's an easy way to do this for the actual
branch protection, since we'd have to test an actual `git push`.
2. Testing branch creation the web UI is also not straightforward,
since the factory repo doesn't have any hooks, and so access checks
at the `gitlab-shell` level aren't run.
Timothy Andrew