summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2016-04-14 10:27:49 +0000
committerRémy Coutable <remy@rymai.me>2016-04-14 10:27:49 +0000
commitc71cdb194f6b30997826929190eaee70f762848a (patch)
treedd2106634c34119887dec2e0f509acefa0f32407
parent595aba54afce01a7903990f6630ae03de1c4d61e (diff)
parent02cfbf0db5dda8ca86f4811e5d5cb055a8cc5cfb (diff)
downloadgitlab-ce-c71cdb194f6b30997826929190eaee70f762848a.tar.gz
Merge branch 'simplify-gitlab-url_builder-15202' into 'master'
Refactor and expose only Gitlab::UrlBuilder.build(record) ``` $ git grep Gitlab::UrlBuilder app/models/commit.rb: url: Gitlab::UrlBuilder.build(self), app/services/issues/base_service.rb: issue_url = Gitlab::UrlBuilder.build(issue) app/services/merge_requests/base_service.rb: hook_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(merge_request) app/views/search/results/_note.html.haml:- note_url = Gitlab::UrlBuilder.build(note) lib/gitlab/note_data_builder.rb: base_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(note) spec/lib/gitlab/note_data_builder_spec.rb: expect(data[:object_attributes][:url]).to eq(Gitlab::UrlBuilder.build(note)) spec/lib/gitlab/url_builder_spec.rb:describe Gitlab::UrlBuilder, lib: true do ``` Fixes #15202. See merge request !3696
-rw-r--r--app/models/commit.rb6
-rw-r--r--app/services/issues/base_service.rb2
-rw-r--r--app/services/merge_requests/base_service.rb3
-rw-r--r--app/views/search/results/_note.html.haml2
-rw-r--r--lib/gitlab/note_data_builder.rb3
-rw-r--r--lib/gitlab/url_builder.rb74
-rw-r--r--spec/factories/commits.rb12
-rw-r--r--spec/lib/gitlab/note_data_builder_spec.rb3
-rw-r--r--spec/lib/gitlab/url_builder_spec.rb143
9 files changed, 147 insertions, 101 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 11ecfcace14..d1f07ccd55c 100644
--- a/app/models/commit.rb
+++ b/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)
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 770f32de944..772f5c5fffa 100644
--- a/app/services/issues/base_service.rb
+++ b/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
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index ac5b58db862..e6837a18696 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/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
diff --git a/app/views/search/results/_note.html.haml b/app/views/search/results/_note.html.haml
index 9544e3d3e17..d9400b1d9fa 100644
--- a/app/views/search/results/_note.html.haml
+++ b/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
diff --git a/lib/gitlab/note_data_builder.rb b/lib/gitlab/note_data_builder.rb
index 18523e0aefe..8bdc89a7751 100644
--- a/lib/gitlab/note_data_builder.rb
+++ b/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
diff --git a/lib/gitlab/url_builder.rb b/lib/gitlab/url_builder.rb
index f301d42939d..f1943222edf 100644
--- a/lib/gitlab/url_builder.rb
+++ b/lib/gitlab/url_builder.rb
@@ -4,50 +4,58 @@ module Gitlab
include GitlabRoutingHelper
include ActionView::RecordIdentifier
- def initialize(type)
- @type = type
- end
+ attr_reader :object
- 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 self.build(object)
+ new(object).url
+ end
+ 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
diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb
new file mode 100644
index 00000000000..ac6eb0a7897
--- /dev/null
+++ b/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
diff --git a/spec/lib/gitlab/note_data_builder_spec.rb b/spec/lib/gitlab/note_data_builder_spec.rb
index da652677443..f093d0a0d8b 100644
--- a/spec/lib/gitlab/note_data_builder_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/url_builder_spec.rb b/spec/lib/gitlab/url_builder_spec.rb
index f023be6ae45..6ffc0d6e658 100644
--- a/spec/lib/gitlab/url_builder_spec.rb
+++ b/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)
- expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}"
- end
- end
+ describe '.build' do
+ context 'when passing a Commit' do
+ it 'returns a proper URL' do
+ commit = build_stubbed(:commit)
- 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)
- expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}"
+ url = described_class.build(commit)
+
+ expect(url).to eq "#{Settings.gitlab['url']}/#{commit.project.path_with_namespace}/commit/#{commit.id}"
+ end
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 an Issue' do
+ it 'returns a proper URL' do
+ issue = build_stubbed(:issue, iid: 42)
- 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}"
+ url = described_class.build(issue)
+
+ expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}"
+ end
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 '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)
- 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}"
+ expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}"
+ end
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 'when passing a Note' do
+ context 'on a Commit' do
+ it 'returns a proper URL' do
+ note = build_stubbed(:note_on_commit)
- 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
+ url = described_class.build(note)
- 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) }
+ expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
+ end
+ end
- 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
+ context 'on a CommitDiff' do
+ it 'returns a proper URL' do
+ note = build_stubbed(:note_on_commit_diff)
- 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) }
+ 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
+ expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
+ end
+ end
+
+ 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)
+
+ expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}#note_#{note.id}"
+ end
+ end
+
+ 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)
+
+ expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
+ end
+ end
+
+ 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)
+
+ expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
+ end
+ end
+
+ 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)
+
+ 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
- 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 another object' do
+ it 'returns a proper URL' do
+ project = build_stubbed(:project)
- 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}"
+ expect { described_class.build(project) }.
+ to raise_error(NotImplementedError, 'No URL builder defined for Project')
+ end
+ end
end
end
end