Commit c71cdb19 authored by Rémy Coutable's avatar Rémy Coutable

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
parents 595aba54 02cfbf0d
...@@ -154,7 +154,7 @@ class Commit ...@@ -154,7 +154,7 @@ class Commit
id: id, id: id,
message: safe_message, message: safe_message,
timestamp: committed_date.xmlschema, timestamp: committed_date.xmlschema,
url: commit_url, url: Gitlab::UrlBuilder.build(self),
author: { author: {
name: author_name, name: author_name,
email: author_email email: author_email
...@@ -168,10 +168,6 @@ class Commit ...@@ -168,10 +168,6 @@ class Commit
data data
end 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 # Discover issues should be closed when this commit is pushed to a project's
# default branch. # default branch.
def closes_issues(current_user = self.committer) def closes_issues(current_user = self.committer)
......
...@@ -3,7 +3,7 @@ module Issues ...@@ -3,7 +3,7 @@ module Issues
def hook_data(issue, action) def hook_data(issue, action)
issue_data = issue.to_hook_data(current_user) 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[:object_attributes].merge!(url: issue_url, action: action)
issue_data issue_data
end end
......
...@@ -20,8 +20,7 @@ module MergeRequests ...@@ -20,8 +20,7 @@ module MergeRequests
def hook_data(merge_request, action) def hook_data(merge_request, action)
hook_data = merge_request.to_hook_data(current_user) 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] = Gitlab::UrlBuilder.build(merge_request)
hook_data[:object_attributes][:url] = merge_request_url
hook_data[:object_attributes][:action] = action hook_data[:object_attributes][:action] = action
hook_data hook_data
end end
......
- project = note.project - 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 - noteable_identifier = note.noteable.try(:iid) || note.noteable.id
.search-result-row .search-result-row
%h5.note-search-caption.str-truncated %h5.note-search-caption.str-truncated
......
...@@ -59,8 +59,7 @@ module Gitlab ...@@ -59,8 +59,7 @@ module Gitlab
repository: project.hook_attrs.slice(:name, :url, :description, :homepage) repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
} }
base_data[:object_attributes][:url] = base_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(note)
Gitlab::UrlBuilder.new(:note).build(note.id)
base_data base_data
end end
......
...@@ -4,50 +4,58 @@ module Gitlab ...@@ -4,50 +4,58 @@ module Gitlab
include GitlabRoutingHelper include GitlabRoutingHelper
include ActionView::RecordIdentifier include ActionView::RecordIdentifier
def initialize(type) attr_reader :object
@type = type
end
def build(id) def self.build(object)
case @type new(object).url
when :issue end
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
end end
private private
def build_issue_url(id) def initialize(object)
issue = Issue.find(id) @object = object
issue_url(issue) end
end
def commit_url(opts = {})
def build_merge_request_url(id) return '' if object.project.nil?
merge_request = MergeRequest.find(id)
merge_request_url(merge_request) namespace_project_commit_url({
end namespace_id: object.project.namespace,
project_id: object.project,
def build_note_url(id) id: object.id
note = Note.find(id) }.merge!(opts))
if note.for_commit? end
namespace_project_commit_url(namespace_id: note.project.namespace,
id: note.commit_id, def note_url
project_id: note.project, if object.for_commit?
anchor: dom_id(note)) commit_url(id: object.commit_id, anchor: dom_id(object))
elsif note.for_issue?
issue = Issue.find(note.noteable_id) elsif object.for_issue?
issue_url(issue, anchor: dom_id(note)) issue = Issue.find(object.noteable_id)
elsif note.for_merge_request? issue_url(issue, anchor: dom_id(object))
merge_request = MergeRequest.find(note.noteable_id)
merge_request_url(merge_request, anchor: dom_id(note)) elsif object.for_merge_request?
elsif note.for_snippet? merge_request = MergeRequest.find(object.noteable_id)
snippet = Snippet.find(note.noteable_id) merge_request_url(merge_request, anchor: dom_id(object))
project_snippet_url(snippet, anchor: dom_id(note))
elsif object.for_snippet?
snippet = Snippet.find(object.noteable_id)
project_snippet_url(snippet, anchor: dom_id(object))
end end
end end
end end
......
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
...@@ -4,13 +4,12 @@ describe 'Gitlab::NoteDataBuilder', lib: true do ...@@ -4,13 +4,12 @@ describe 'Gitlab::NoteDataBuilder', lib: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:data) { Gitlab::NoteDataBuilder.build(note, 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 let(:fixed_time) { Time.at(1425600000) } # Avoid time precision errors
before(:each) do before(:each) do
expect(data).to have_key(:object_attributes) expect(data).to have_key(:object_attributes)
expect(data[:object_attributes]).to have_key(:url) 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[:object_kind]).to eq('note')
expect(data[:user]).to eq(user.hook_attrs) expect(data[:user]).to eq(user.hook_attrs)
end end
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::UrlBuilder, lib: true do describe Gitlab::UrlBuilder, lib: true do
describe 'When asking for an issue' do describe '.build' do
it 'returns the issue url' do context 'when passing a Commit' do
issue = create(:issue) it 'returns a proper URL' do
url = Gitlab::UrlBuilder.new(:issue).build(issue.id) 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}"
end end
end end
describe 'When asking for an merge request' do context 'when passing a MergeRequest' do
it 'returns the merge request url' do it 'returns a proper URL' do
merge_request = create(:merge_request) merge_request = build_stubbed(:merge_request, iid: 42)
url = Gitlab::UrlBuilder.new(:merge_request).build(merge_request.id)
url = described_class.build(merge_request)
expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}" expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}"
end end
end end
describe 'When asking for a note on commit' do context 'when passing a Note' do
let(:note) { create(:note_on_commit) } context 'on a Commit' do
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
end end
end end
describe 'When asking for a note on commit diff' do context 'on a CommitDiff' do
let(:note) { create(:note_on_commit_diff) } it 'returns a proper URL' do
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{note.project.path_with_namespace}/commit/#{note.commit_id}#note_#{note.id}"
end end
end end
describe 'When asking for a note on issue' do context 'on an Issue' do
let(:issue) { create(:issue) } it 'returns a proper URL' do
let(:note) { create(:note_on_issue, noteable_id: issue.id) } issue = create(:issue, iid: 42)
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{issue.project.path_with_namespace}/issues/#{issue.iid}#note_#{note.id}"
end end
end end
describe 'When asking for a note on merge request' do context 'on a MergeRequest' do
let(:merge_request) { create(:merge_request) } it 'returns a proper URL' do
let(:note) { create(:note_on_merge_request, noteable_id: merge_request.id) } merge_request = create(:merge_request, iid: 42)
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
end end
end end
describe 'When asking for a note on merge request diff' do context 'on a MergeRequestDiff' do
let(:merge_request) { create(:merge_request) } it 'returns a proper URL' do
let(:note) { create(:note_on_merge_request_diff, noteable_id: merge_request.id) } merge_request = create(:merge_request, iid: 42)
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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}" expect(url).to eq "#{Settings.gitlab['url']}/#{merge_request.project.path_with_namespace}/merge_requests/#{merge_request.iid}#note_#{note.id}"
end end
end end
describe 'When asking for a note on project snippet' do context 'on a ProjectSnippet' do
let(:snippet) { create(:project_snippet) } it 'returns a proper URL' do
let(:note) { create(:note_on_project_snippet, noteable_id: snippet.id) } project_snippet = create(:project_snippet)
let(:url) { Gitlab::UrlBuilder.new(:note).build(note.id) } 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
context 'on another object' do
it 'returns a proper URL' do
project = build_stubbed(:project)
it 'returns the note url' do expect { described_class.build(project) }.
expect(url).to eq "#{Settings.gitlab['url']}/#{snippet.project.path_with_namespace}/snippets/#{note.noteable_id}#note_#{note.id}" to raise_error(NotImplementedError, 'No URL builder defined for Project')
end
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment