Skip to content
Snippets Groups Projects
  1. Mar 26, 2018
  2. Jan 06, 2018
  3. Dec 07, 2017
  4. Nov 23, 2017
  5. Apr 06, 2017
  6. Feb 23, 2017
  7. Oct 24, 2016
    • Timothy Andrew's avatar
      Implement third round of review comments from @DouweM. · db0182e2
      Timothy Andrew authored
      Extract/mutate `params` in the `execute` method of the API services,
      rather than in `initialize`.
      db0182e2
    • Timothy Andrew's avatar
      Implement second round of review comments from @DouweM. · 1051087a
      Timothy Andrew authored
      - 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.
      1051087a
    • Timothy Andrew's avatar
      Implement review comments from @DouweM. · b803bc7b
      Timothy Andrew authored
      b803bc7b
    • Timothy Andrew's avatar
      Implement review comments from @dbalexandre. · cef10ef7
      Timothy Andrew authored
      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`
      cef10ef7
    • Timothy Andrew's avatar
      Fix branch protection API. · f79f3a1d
      Timothy Andrew authored
      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.
      f79f3a1d
  8. Aug 16, 2016
    • Timothy Andrew's avatar
      Backport changes from gitlab-org/gitlab-ee!581 to CE. · e805a647
      Timothy Andrew authored
      !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.
      e805a647
  9. Jul 29, 2016
    • Timothy Andrew's avatar
      Implement final review comments from @rymai. · cebcc417
      Timothy Andrew authored
      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.
      cebcc417
    • Timothy Andrew's avatar
      Use `Gitlab::Access` to protected branch access levels. · 0a8aeb46
      Timothy Andrew authored
      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.
      0a8aeb46
    • Timothy Andrew's avatar
      Authorize user before creating/updating a protected branch. · 6d841eaa
      Timothy Andrew authored
      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.
      6d841eaa
    • Timothy Andrew's avatar
      Have the `branches` API work with the new protected branches data model. · 01d190a8
      Timothy Andrew authored
      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.
      01d190a8
    • Timothy Andrew's avatar
      Implement review comments from @dbalexandre. · 7b2ad2d5
      Timothy Andrew authored
      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.
      7b2ad2d5
    • Timothy Andrew's avatar
      Fix default branch protection. · a9958ddc
      Timothy Andrew authored
      1. So it works with the new data model for protected branch access levels.
      a9958ddc
    • Timothy Andrew's avatar
      Add "No One Can Push" to the protected branches UI. · ab6096c1
      Timothy Andrew authored
      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.
      ab6096c1
    • Timothy Andrew's avatar
      Use the `{Push,Merge}AccessLevel` models in the UI. · 134fe5af
      Timothy Andrew authored
      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`
      134fe5af
Loading