Refactor and expose only Gitlab::UrlBuilder.build(record)

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/models/commit.rb

@@ -154,7 +154,7 @@ class Commit
       id: id,
       message: safe_message,
       timestamp: committed_date.xmlschema,
-      url: commit_url,
+      url: Gitlab::UrlBuilder.build(self),
       author: {
         name: author_name,
         email: author_email
@@ -168,10 +168,6 @@ class Commit
     data
   end
 
-  def commit_url
-    project.present? ? "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/commit/#{id}" : ""
-  end
-
   # Discover issues should be closed when this commit is pushed to a project's
   # default branch.
   def closes_issues(current_user = self.committer)

app/services/issues/base_service.rb

@@ -3,7 +3,7 @@ module Issues
 
     def hook_data(issue, action)
       issue_data = issue.to_hook_data(current_user)
-      issue_url = Gitlab::UrlBuilder.new(:issue).build(issue.id)
+      issue_url = Gitlab::UrlBuilder.build(issue)
       issue_data[:object_attributes].merge!(url: issue_url, action: action)
       issue_data
     end

app/services/merge_requests/base_service.rb

@@ -20,8 +20,7 @@ module MergeRequests
 
     def hook_data(merge_request, action)
       hook_data = merge_request.to_hook_data(current_user)
-      merge_request_url = Gitlab::UrlBuilder.new(:merge_request).build(merge_request.id)
-      hook_data[:object_attributes][:url] = merge_request_url
+      hook_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(merge_request)
       hook_data[:object_attributes][:action] = action
       hook_data
     end

app/views/search/results/_note.html.haml

@@ -1,5 +1,5 @@
 - project = note.project
-- note_url = Gitlab::UrlBuilder.new(:note).build(note.id)
+- note_url = Gitlab::UrlBuilder.build(note)
 - noteable_identifier = note.noteable.try(:iid) || note.noteable.id
 .search-result-row
   %h5.note-search-caption.str-truncated

lib/gitlab/note_data_builder.rb

@@ -59,8 +59,7 @@ module Gitlab
           repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
         }
 
-        base_data[:object_attributes][:url] =
-             Gitlab::UrlBuilder.new(:note).build(note.id)
+        base_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(note)
         base_data
       end
 

lib/gitlab/url_builder.rb

@@ -4,50 +4,58 @@ module Gitlab
     include GitlabRoutingHelper
     include ActionView::RecordIdentifier
 
-    def initialize(type)
-      @type = type
+    attr_reader :object
+
+    def self.build(object)
+      new(object).url
     end
 
-    def build(id)
-      case @type
-      when :issue
-        build_issue_url(id)
-      when :merge_request
-        build_merge_request_url(id)
-      when :note
-        build_note_url(id)
-
+    def url
+      case object
+      when Commit
+        commit_url
+      when Issue
+        issue_url(object)
+      when MergeRequest
+        merge_request_url(object)
+      when Note
+        note_url
+      else
+        raise NotImplementedError.new("No URL builder defined for #{object.class}")
       end
     end
 
     private
 
-    def build_issue_url(id)
-      issue = Issue.find(id)
-      issue_url(issue)
+    def initialize(object)
+      @object = object
     end
 
-    def build_merge_request_url(id)
-      merge_request = MergeRequest.find(id)
-      merge_request_url(merge_request)
+    def commit_url(opts = {})
+      return '' if object.project.nil?
+
+      namespace_project_commit_url({
+        namespace_id: object.project.namespace,
+        project_id: object.project,
+        id: object.id
+      }.merge!(opts))
     end
 
-    def build_note_url(id)
-      note = Note.find(id)
-      if note.for_commit?
-        namespace_project_commit_url(namespace_id: note.project.namespace,
-                                     id: note.commit_id,
-                                     project_id: note.project,
-                                     anchor: dom_id(note))
-      elsif note.for_issue?
-        issue = Issue.find(note.noteable_id)
-        issue_url(issue, anchor: dom_id(note))
-      elsif note.for_merge_request?
-        merge_request = MergeRequest.find(note.noteable_id)
-        merge_request_url(merge_request, anchor: dom_id(note))
-      elsif note.for_snippet?
-        snippet = Snippet.find(note.noteable_id)
-        project_snippet_url(snippet, anchor: dom_id(note))
+    def note_url
+      if object.for_commit?
+        commit_url(id: object.commit_id, anchor: dom_id(object))
+
+      elsif object.for_issue?
+        issue = Issue.find(object.noteable_id)
+        issue_url(issue, anchor: dom_id(object))
+
+      elsif object.for_merge_request?
+        merge_request = MergeRequest.find(object.noteable_id)
+        merge_request_url(merge_request, anchor: dom_id(object))
+
+      elsif object.for_snippet?
+        snippet = Snippet.find(object.noteable_id)
+        project_snippet_url(snippet, anchor: dom_id(object))
       end
     end
   end

spec/factories/commits.rb

@@ -0,0 +1,12 @@
+require_relative '../support/repo_helpers'
+
+FactoryGirl.define do
+  factory :commit do
+    git_commit RepoHelpers.sample_commit
+    project factory: :empty_project
+
+    initialize_with do
+      new(git_commit, project)
+    end
+  end
+end

spec/lib/gitlab/note_data_builder_spec.rb

@@ -4,13 +4,12 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:data) { Gitlab::NoteDataBuilder.build(note, user) }
-  let(:note_url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
   let(:fixed_time) { Time.at(1425600000) } # Avoid time precision errors
 
   before(:each) do
     expect(data).to have_key(:object_attributes)
     expect(data[:object_attributes]).to have_key(:url)
-    expect(data[:object_attributes][:url]).to eq(note_url)
+    expect(data[:object_attributes][:url]).to eq(Gitlab::UrlBuilder.build(note))
     expect(data[:object_kind]).to eq('note')
     expect(data[:user]).to eq(user.hook_attrs)
   end

spec/lib/gitlab/url_builder_spec.rb

@@ -1,77 +1,110 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBuilder, lib: true do
-  describe 'When asking for an issue' do
-    it 'returns the issue url' do
-      issue = create(:issue)
-      url = Gitlab::UrlBuilder.new(:issue).build(issue.id)
+  describe '.build' do
+    context 'when passing a Commit' do
+      it 'returns a proper URL' do
+        commit = build_stubbed(:commit)
+
+        url = described_class.build(commit)
+
+        expect(url).to eq "#{Settings.gitlab['url']}/#{commit.project.path_with_namespace}/commit/#{commit.id}"
+      end
+    end
+
+    context 'when passing an Issue' do
+      it 'returns a proper URL' do
+        issue = build_stubbed(:issue, iid: 42)
+
+        url = described_class.build(issue)
+
         expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}"
       end
     end
 
-  describe 'When asking for an merge request' do
-    it 'returns the merge request url' do
-      merge_request = create(:merge_request)
-      url = Gitlab::UrlBuilder.new(:merge_request).build(merge_request.id)
+    context 'when passing a MergeRequest' do
+      it 'returns a proper URL' do
+        merge_request = build_stubbed(:merge_request, iid: 42)
+
+        url = described_class.build(merge_request)
+
         expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}"
       end
     end
 
-  describe 'When asking for a note on commit' do
-    let(:note) { create(:note_on_commit) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+    context 'when passing a Note' do
+      context 'on a Commit' do
+        it 'returns a proper URL' do
+          note = build_stubbed(:note_on_commit)
+
+          url = described_class.build(note)
 
-    it 'returns the note url' do
           expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
         end
       end
 
-  describe 'When asking for a note on commit diff' do
-    let(:note) { create(:note_on_commit_diff) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+      context 'on a CommitDiff' do
+        it 'returns a proper URL' do
+          note = build_stubbed(:note_on_commit_diff)
+
+          url = described_class.build(note)
 
-    it 'returns the note url' do
           expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
         end
       end
 
-  describe 'When asking for a note on issue' do
-    let(:issue) { create(:issue) }
-    let(:note) { create(:note_on_issue, noteable_id: issue.id) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+      context 'on an Issue' do
+        it 'returns a proper URL' do
+          issue = create(:issue, iid: 42)
+          note = build_stubbed(:note_on_issue, noteable: issue)
+
+          url = described_class.build(note)
 
-    it 'returns the note url' do
           expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}#note_#{note.id}"
         end
       end
 
-  describe 'When asking for a note on merge request' do
-    let(:merge_request) { create(:merge_request) }
-    let(:note) { create(:note_on_merge_request, noteable_id: merge_request.id) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+      context 'on a MergeRequest' do
+        it 'returns a proper URL' do
+          merge_request = create(:merge_request, iid: 42)
+          note = build_stubbed(:note_on_merge_request, noteable: merge_request)
+
+          url = described_class.build(note)
 
-    it 'returns the note url' do
           expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
         end
       end
 
-  describe 'When asking for a note on merge request diff' do
-    let(:merge_request) { create(:merge_request) }
-    let(:note) { create(:note_on_merge_request_diff, noteable_id: merge_request.id) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+      context 'on a MergeRequestDiff' do
+        it 'returns a proper URL' do
+          merge_request = create(:merge_request, iid: 42)
+          note = build_stubbed(:note_on_merge_request_diff, noteable: merge_request)
+
+          url = described_class.build(note)
 
-    it 'returns the note url' do
           expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
         end
       end
 
-  describe 'When asking for a note on project snippet' do
-    let(:snippet) { create(:project_snippet) }
-    let(:note) { create(:note_on_project_snippet, noteable_id: snippet.id) }
-    let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) }
+      context 'on a ProjectSnippet' do
+        it 'returns a proper URL' do
+          project_snippet = create(:project_snippet)
+          note = build_stubbed(:note_on_project_snippet, noteable: project_snippet)
 
-    it 'returns the note url' do
-      expect(url).to eq "#{Settings.gitlab['url']}/#{snippet.project.path_with_namespace}/snippets/#{note.noteable_id}#note_#{note.id}"
+          url = described_class.build(note)
+
+          expect(url).to eq "#{Settings.gitlab['url']}/#{project_snippet.project.path_with_namespace}/snippets/#{note.noteable_id}#note_#{note.id}"
+        end
+      end
+
+      context 'on another object' do
+        it 'returns a proper URL' do
+          project = build_stubbed(:project)
+
+          expect { described_class.build(project) }.
+            to raise_error(NotImplementedError, 'No URL builder defined for Project')
+        end
+      end
     end
   end
 end