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

1 files changed, 0 insertions, 82 deletions

diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 50cf2f1e93f..059d8a2c355 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -6,88 +6,6 @@ describe Gitlab::GitAccess, lib: true do
   let(:user) { create(:user) }
   let(:actor) { user }
 
-  describe 'can_push_to_branch?' do
-    describe 'push to none protected branch' do
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?("random_branch")).to be_truthy
-      end
-
-      it "returns true if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?("random_branch")).to be_truthy
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?("random_branch")).to be_falsey
-      end
-    end
-
-    describe 'push to protected branch' do
-      before do
-        @branch = create :protected_branch, project: project
-      end
-      
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns false if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-    end
-
-    describe 'push to protected branch if allowed for developers' do
-      before do
-        @branch = create :protected_branch, project: project, developers_can_push: true
-      end
-      
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns true if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_push_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_push_to_branch?(@branch.name)).to be_falsey
-      end
-    end
-
-    describe 'merge to protected branch if allowed for developers' do
-      before do
-        @branch = create :protected_branch, project: project, developers_can_merge: true
-      end
-
-      it "returns true if user is a master" do
-        project.team << [user, :master]
-        expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns true if user is a developer" do
-        project.team << [user, :developer]
-        expect(access.can_merge_to_branch?(@branch.name)).to be_truthy
-      end
-
-      it "returns false if user is a reporter" do
-        project.team << [user, :reporter]
-        expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
-      end
-    end
-  end
-
   describe '#check with single protocols allowed' do
     def disable_protocol(protocol)
       settings = ::ApplicationSetting.create_from_defaults