From 60245bbe228014a9f689adafd64b571883ef6eb3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 24 Jun 2016 10:37:04 +0530 Subject: 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 --- lib/api/helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/api/helpers.rb') diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 77e407b54c5..73557cf7db6 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -17,7 +17,7 @@ module API def current_user @current_user ||= (find_user_by_private_token || doorkeeper_guard) - unless @current_user && Gitlab::UserAccess.allowed?(@current_user) + unless @current_user && Gitlab::UserAccess.new(@current_user).allowed? return nil end -- cgit v1.2.1