diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-06-24 10:37:04 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-07-13 13:24:56 +0530 |
commit | 60245bbe228014a9f689adafd64b571883ef6eb3 (patch) | |
tree | 2af0d7785fb1e9c1f168b01d704e788d42d89503 /app/services/commits | |
parent | 495db09653bafb0371e5d5a5f12d5bc33cdb584b (diff) | |
download | gitlab-ce-60245bbe228014a9f689adafd64b571883ef6eb3.tar.gz |
Refactor `Gitlab::GitAccess`
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
Diffstat (limited to 'app/services/commits')
-rw-r--r-- | app/services/commits/change_service.rb | 4 |
1 files changed, 2 insertions, 2 deletions
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index c578097376a..ed73d8cb8c2 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -23,7 +23,7 @@ module Commits private def check_push_permissions - allowed = ::Gitlab::GitAccess.new(current_user, project, 'web').can_push_to_branch?(@target_branch) + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) unless allowed raise ValidationError.new('You are not allowed to push into this branch') @@ -31,7 +31,7 @@ module Commits true end - + def create_target_branch(new_branch) # Temporary branch exists and contains the change commit return success if repository.find_branch(new_branch) |