Merge branch 'jej/github-service-remote-url-refactor' into 'master'

Refactor GithubService URL handling for remote project

See merge request gitlab-org/gitlab-ee!4872

ee/app/models/project_services/github_service.rb

@@ -4,6 +4,8 @@ class GithubService < Service
 
   prop_accessor :token, :repository_url
 
+  delegate :api_url, :owner, :repository_name, to: :remote_project
+
   validates :token, presence: true, if: :activated?
   validates :repository_url, url: true, allow_blank: true
 
@@ -76,30 +78,10 @@ class GithubService < Service
     { success: true, result: result }
   end
 
-  def api_url
-    if github_host == 'github.com'
-      'https://api.github.com'
-    else
-      "#{repository_uri.scheme}://#{github_host}/api/v3"
-    end
-  end
-
-  def owner
-    repository_uri.path.split('/')[1]
-  end
-
-  def repository_name
-    repository_uri.path.split('/')[2]
-  end
-
   private
 
-  def github_host
-    repository_uri.host
-  end
-
-  def repository_uri
-    URI.parse(repository_url)
+  def remote_project
+    RemoteProject.new(repository_url)
   end
 
   def disabled?

ee/app/models/project_services/github_service/remote_project.rb

+class GithubService
+  class RemoteProject
+    def initialize(url)
+      @uri = URI.parse(url)
+    end
+
+    def api_url
+      if host == 'github.com'
+        'https://api.github.com'
+      else
+        "#{protocol}://#{host}/api/v3"
+      end
+    end
+
+    def owner
+      uri_path.split('/')[1]
+    end
+
+    def repository_name
+      uri_path.split('/')[2]
+    end
+
+    private
+
+    def host
+      @uri.host
+    end
+
+    def protocol
+      @uri.scheme
+    end
+
+    def uri_path
+      @uri.path.sub('.git', '')
+    end
+  end
+end

ee/spec/models/project_services/github_service/remote_project_spec.rb

+require 'spec_helper'
+
+describe GithubService::RemoteProject do
+  let(:owner) { 'MyUser' }
+  let(:repository_name) { 'my-project' }
+  let(:repo_full_path) { "#{owner}/#{repository_name}" }
+  let(:project_url_base) { "https://github.com/#{repo_full_path}" }
+  let(:project_url) { project_url_base }
+
+  subject { described_class.new(project_url) }
+
+  describe '#api_url' do
+    it 'uses github.com API endpoint' do
+      expect(subject.api_url).to eq 'https://api.github.com'
+    end
+
+    context 'when git repo mirror URL is used' do
+      let(:project_url) { "https://00000000000000@github.com/#{repo_full_path}.git" }
+
+      it "excludes auth token set as username" do
+        expect(subject.api_url).to eq 'https://api.github.com'
+      end
+    end
+
+    context 'for a custom host' do
+      let(:project_url) { "https://my.repo.com/#{repo_full_path}" }
+
+      it 'is extracted from the url' do
+        expect(subject.api_url).to eq 'https://my.repo.com/api/v3'
+      end
+    end
+  end
+
+  describe '#owner' do
+    it 'is extracted from the url' do
+      expect(subject.owner).to eq owner
+    end
+  end
+
+  describe '#repository_name' do
+    it 'is extracted from the url' do
+      expect(subject.repository_name).to eq repository_name
+    end
+
+    context 'when https git URL is used' do
+      let(:project_url) { "#{project_url_base}.git" }
+
+      it "doesn't include '.git' at the end" do
+        expect(subject.repository_name).to eq repository_name
+      end
+    end
+
+    context 'when project sub-route accidentally used' do
+      let(:project_url) { "#{project_url_base}/issues" }
+
+      it "ignores the sub-route" do
+        expect(subject.repository_name).to eq repository_name
+      end
+    end
+  end
+end