Skip to content
Snippets Groups Projects
Select Git revision
  • ag-test
  • rs-test
  • master default protected
  • test-me-pa
  • mksionek-master-patch-52381
  • new-branch-10
  • test-conflicts
  • test-suggestions
  • alejandro-test
  • patch-25
  • winh-test-image-doscussion
  • stg-lfs-image-test-2
  • stg-lfs-image-test
  • test42016
  • issue_42016
  • issue-32709
  • add-codeowners
  • ClemMakesApps-master-patch-62759
  • bvl-staging-test
  • bvl-merge-base-api
  • v9.2.0-rc6 protected
  • v9.2.0-rc5 protected
  • v9.2.0-rc4 protected
  • v9.2.0-rc3 protected
  • v9.1.4 protected
  • v9.2.0-rc2 protected
  • v9.2.0-rc1 protected
  • v9.1.3 protected
  • v8.17.6 protected
  • v9.0.7 protected
  • v9.1.2 protected
  • v9.1.1 protected
  • v9.2.0.pre protected
  • v9.1.0 protected
  • v9.1.0-rc7 protected
  • v9.1.0-rc6 protected
  • v9.0.6 protected
  • v9.1.0-rc5 protected
  • v9.1.0-rc4 protected
  • v9.1.0-rc3 protected
40 results

base_service.rb

  • Timothy Andrew's avatar
    60245bbe
    Refactor `Gitlab::GitAccess` · 60245bbe
    Timothy Andrew authored
    1. Don't use case statements for dispatch anymore. This leads to a lot
       of duplication, and makes the logic harder to follow.
    
    2. Remove duplicated logic.
    
        - For example, the `can_push_to_branch?` exists, but we also have a
          different way of checking the same condition within `change_access_check`.
    
        - This kind of duplication is removed, and the `can_push_to_branch?`
          method is used in both places.
    
    3. Move checks returning true/false to `UserAccess`.
    
        - All public methods in `GitAccess` now return an instance of
          `GitAccessStatus`. Previously, some methods would return
          true/false as well, which was confusing.
    
        - It makes sense for these kinds of checks to be at the level of a
          user, so the `UserAccess` class was repurposed for this. The prior
          `UserAccess.allowed?` classmethod is converted into an instance
          method.
    
        - All external uses of these checks have been migrated to use the
          `UserAccess` class
    
    4. Move the "change_access_check" into a separate class.
    
        - Create the `GitAccess::ChangeAccessCheck` class to run these
          checks, which are quite substantial.
    
        - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
          well.
    
    5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
       chains - this seems more readable.
    
    6. I can understand that this might look like overkill for !4892, but I
       think this is a good opportunity to clean it up.
    
        - http://martinfowler.com/bliki/OpportunisticRefactoring.html
    60245bbe
    History
    Refactor `Gitlab::GitAccess`
    Timothy Andrew authored
    1. Don't use case statements for dispatch anymore. This leads to a lot
       of duplication, and makes the logic harder to follow.
    
    2. Remove duplicated logic.
    
        - For example, the `can_push_to_branch?` exists, but we also have a
          different way of checking the same condition within `change_access_check`.
    
        - This kind of duplication is removed, and the `can_push_to_branch?`
          method is used in both places.
    
    3. Move checks returning true/false to `UserAccess`.
    
        - All public methods in `GitAccess` now return an instance of
          `GitAccessStatus`. Previously, some methods would return
          true/false as well, which was confusing.
    
        - It makes sense for these kinds of checks to be at the level of a
          user, so the `UserAccess` class was repurposed for this. The prior
          `UserAccess.allowed?` classmethod is converted into an instance
          method.
    
        - All external uses of these checks have been migrated to use the
          `UserAccess` class
    
    4. Move the "change_access_check" into a separate class.
    
        - Create the `GitAccess::ChangeAccessCheck` class to run these
          checks, which are quite substantial.
    
        - `ChangeAccessCheck` returns an instance of `GitAccessStatus` as
          well.
    
    5. Break out the boolean logic in `ChangeAccessCheck` into `if/else`
       chains - this seems more readable.
    
    6. I can understand that this might look like overkill for !4892, but I
       think this is a good opportunity to clean it up.
    
        - http://martinfowler.com/bliki/OpportunisticRefactoring.html
Code owners
Assign users and groups as approvers for specific file changes. Learn more.