Commit graph

16 commits

Author SHA1 Message Date
James Edwards-Jones
f16377e7dc Protected Tags backend review changes
Added changelog
2017-04-06 10:56:21 +01:00
Douwe Maan
030baf70d2 Enable Performance/RedundantMerge 2017-02-23 09:31:57 -06:00
Timothy Andrew
db0182e261 Implement third round of review comments from @DouweM.
Extract/mutate `params` in the `execute` method of the API services,
rather than in `initialize`.
2016-10-24 11:33:38 +05:30
Timothy Andrew
1051087ac4 Implement second round of review comments from @DouweM.
- Pass `developers_and_merge` and `developers_can_push` in `params`
  instead of using keyword arguments.

- Refactor a slightly complex boolean check to a simple `nil?` check.
2016-10-24 11:33:38 +05:30
Timothy Andrew
b803bc7bb8 Implement review comments from @DouweM. 2016-10-24 11:33:38 +05:30
Timothy Andrew
cef10ef7d7 Implement review comments from @dbalexandre.
1. Don't have any EE-only code in CE. Ok to have CE-only code in EE.

2. Use `case` instead of `if/elsif`
2016-10-24 11:33:38 +05:30
Timothy Andrew
f79f3a1dd6 Fix branch protection API.
1. Previously, we were not removing existing access levels before
   creating new ones. This is not a problem for EE, but _is_ for CE,
   since we restrict the number of access levels in CE to 1.

2. The correct approach is:

    CE -> delete all access levels before updating a protected branch
    EE -> delete developer access levels if "developers_can_{merge,push}" is switched off

3. The dispatch is performed by checking if a "length: 1" validation is
   present on the access levels or not.

4. Another source of problems was that we didn't put multiple queries in
   a transaction. If the `destroy_all` passes, but the `update` fails,
   we should have a rollback.

5. Modifying the API to provide users direct access to CRUD access
   levels will make things a lot simpler.

6. Create `create/update` services separately for this API, which
   perform the necessary data translation, before calling the regular
   `create/update` services. The translation code was getting too large
   for the API endpoint itself, so this move makes sense.
2016-10-24 11:33:38 +05:30
Timothy Andrew
e805a64700 Backport changes from gitlab-org/gitlab-ee!581 to CE.
!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.
2016-08-16 11:05:14 +05:30
Timothy Andrew
cebcc417ed Implement final review comments from @rymai.
1. Instantiate `ProtectedBranchesAccessSelect` from `dispatcher`

2. Use `can?(user, ...)` instead of `user.can?(...)`

3. Add `DOWNTIME` notes to all migrations added in !5081.

4. Add an explicit `down` method for migrations removing the
   `developers_can_push` and `developers_can_merge` columns, ensuring that
   the columns created (on rollback) have the appropriate defaults.

5. Remove duplicate CHANGELOG entries.

6. Blank lines after guard clauses.
2016-07-29 15:20:39 +05:30
Timothy Andrew
0a8aeb46dc Use Gitlab::Access to protected branch access levels.
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.
2016-07-29 15:20:39 +05:30
Timothy Andrew
6d841eaadc Authorize user before creating/updating a protected branch.
1. This is a third line of defence (first in the view, second in the
   controller).

2. Duplicate the `API::Helpers.to_boolean` method in `BaseService`. The
   other alternative is to `include API::Helpers`, but this brings with it
   a number of other methods that might cause conflicts.

3. Return a 403 if authorization fails.
2016-07-29 15:20:39 +05:30
Timothy Andrew
01d190a84a Have the branches API work with the new protected branches data model.
1. The new data model moves from `developers_can_{push,merge}` to
   `allowed_to_{push,merge}`.

2. The API interface has not been changed. It still accepts
   `developers_can_push` and `developers_can_merge` as options. These
   attributes are inferred from the new data model.

3. Modify the protected branch create/update services to translate from
   the API interface to our current data model.
2016-07-29 15:20:39 +05:30
Timothy Andrew
7b2ad2d5b9 Implement review comments from @dbalexandre.
1. Remove `master_or_greater?` and `developer_or_greater?` in favor of
   `max_member_access`, which is a lot nicer.

2. Remove a number of instances of `include Gitlab::Database::MigrationHelpers`
   in migrations that don't need this module. Also remove comments where
   not necessary.

3. Remove duplicate entry in CHANGELOG.

4. Move `ProtectedBranchAccessSelect` from Coffeescript to ES6.

5. Split the `set_access_levels!` method in two - one each for `merge` and
   `push` access levels.
2016-07-29 15:20:39 +05:30
Timothy Andrew
a9958ddc7c Fix default branch protection.
1. So it works with the new data model for protected branch access levels.
2016-07-29 15:20:39 +05:30
Timothy Andrew
ab6096c172 Add "No One Can Push" to the protected branches UI.
1. Move to dropdowns instead of checkboxes. One each for "Allowed to
   Push" and "Allowed to Merge"

2. Refactor the `ProtectedBranches` coffeescript class into
   `ProtectedBranchesAccessSelect`.

3. Modify the backend to accept the new parameters.
2016-07-29 15:20:39 +05:30
Timothy Andrew
134fe5af83 Use the {Push,Merge}AccessLevel models in the UI.
1. Improve error handling while creating protected branches.

2. Modify coffeescript code so that the "Developers can *" checkboxes
   send a '1' or '0' even when using AJAX. This lets us keep the backend
   code simpler.

3. Use services for both creating and updating protected branches.
   Destruction is taken care of with `dependent: :destroy`
2016-07-29 15:20:39 +05:30