summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>2017-02-04 20:26:11 +0200
committerDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>2017-02-08 19:14:29 +0200
commit2989192d1aa8051aa09164cd097418bd3063d4ad (patch)
treee277adf2535ac10ed338481ef092731ef84b27e1
parentbbb7fbcd02c9d9a8a6d6ca44d7ce668b80962c21 (diff)
downloadgitlab-ce-2989192d1aa8051aa09164cd097418bd3063d4ad.tar.gz
Store group and project full name and full path in routes tabledz-refactor-full-path
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
-rw-r--r--app/controllers/admin/dashboard_controller.rb4
-rw-r--r--app/controllers/admin/groups_controller.rb2
-rw-r--r--app/controllers/dashboard/groups_controller.rb2
-rw-r--r--app/finders/groups_finder.rb2
-rw-r--r--app/finders/projects_finder.rb2
-rw-r--r--app/models/concerns/routable.rb66
-rw-r--r--app/models/namespace.rb25
-rw-r--r--app/models/project.rb43
-rw-r--r--app/models/route.rb22
-rw-r--r--changelogs/unreleased/dz-refactor-full-path.yml4
-rw-r--r--db/migrate/20170204172458_add_name_to_route.rb12
-rw-r--r--db/schema.rb1
-rw-r--r--spec/models/concerns/routable_spec.rb34
-rw-r--r--spec/models/namespace_spec.rb16
-rw-r--r--spec/models/project_spec.rb21
-rw-r--r--spec/models/route_spec.rb33
16 files changed, 191 insertions, 98 deletions
diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb
index c491e5c7550..8360ce08bdc 100644
--- a/app/controllers/admin/dashboard_controller.rb
+++ b/app/controllers/admin/dashboard_controller.rb
@@ -1,7 +1,7 @@
class Admin::DashboardController < Admin::ApplicationController
def index
- @projects = Project.limit(10)
+ @projects = Project.with_route.limit(10)
@users = User.limit(10)
- @groups = Group.limit(10)
+ @groups = Group.with_route.limit(10)
end
end
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index b7722a1d15d..578f2b8f038 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -2,7 +2,7 @@ class Admin::GroupsController < Admin::ApplicationController
before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update]
def index
- @groups = Group.with_statistics
+ @groups = Group.with_statistics.with_route
@groups = @groups.sort(@sort = params[:sort])
@groups = @groups.search(params[:name]) if params[:name].present?
@groups = @groups.page(params[:page])
diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb
index de6bc689bb7..0b7cf8167f0 100644
--- a/app/controllers/dashboard/groups_controller.rb
+++ b/app/controllers/dashboard/groups_controller.rb
@@ -1,5 +1,5 @@
class Dashboard::GroupsController < Dashboard::ApplicationController
def index
- @group_members = current_user.group_members.includes(:source).page(params[:page])
+ @group_members = current_user.group_members.includes(source: :route).page(params[:page])
end
end
diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index 4e43f42e9e1..d932a17883f 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -2,7 +2,7 @@ class GroupsFinder < UnionFinder
def execute(current_user = nil)
segments = all_groups(current_user)
- find_union(segments, Group).order_id_desc
+ find_union(segments, Group).with_route.order_id_desc
end
private
diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb
index c7911736812..18ec45f300d 100644
--- a/app/finders/projects_finder.rb
+++ b/app/finders/projects_finder.rb
@@ -3,7 +3,7 @@ class ProjectsFinder < UnionFinder
segments = all_projects(current_user)
segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation
- find_union(segments, Project)
+ find_union(segments, Project).with_route
end
private
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 2b93aa30c0f..9f6d215ceb3 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -1,5 +1,5 @@
# Store object full path in separate table for easy lookup and uniq validation
-# Object must have path db field and respond to full_path and full_path_changed? methods.
+# Object must have name and path db fields and respond to parent and parent_changed? methods.
module Routable
extend ActiveSupport::Concern
@@ -9,7 +9,13 @@ module Routable
validates_associated :route
validates :route, presence: true
- before_validation :update_route_path, if: :full_path_changed?
+ scope :with_route, -> { includes(:route) }
+
+ before_validation do
+ if full_path_changed? || full_name_changed?
+ prepare_route
+ end
+ end
end
class_methods do
@@ -77,10 +83,62 @@ module Routable
end
end
+ def full_name
+ if route && route.name.present?
+ @full_name ||= route.name
+ else
+ update_route if persisted?
+
+ build_full_name
+ end
+ end
+
+ def full_path
+ if route && route.path.present?
+ @full_path ||= route.path
+ else
+ update_route if persisted?
+
+ build_full_path
+ end
+ end
+
private
- def update_route_path
+ def full_name_changed?
+ name_changed? || parent_changed?
+ end
+
+ def full_path_changed?
+ path_changed? || parent_changed?
+ end
+
+ def build_full_name
+ if parent && name
+ parent.human_name + ' / ' + name
+ else
+ name
+ end
+ end
+
+ def build_full_path
+ if parent && path
+ parent.full_path + '/' + path
+ else
+ path
+ end
+ end
+
+ def update_route
+ prepare_route
+ route.save
+ end
+
+ def prepare_route
route || build_route(source: self)
- route.path = full_path
+ route.path = build_full_path
+ route.name = build_full_name
+ @full_path = nil
+ @full_name = nil
end
end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index c5713fb7818..c2554317ad3 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -170,27 +170,10 @@ class Namespace < ActiveRecord::Base
Gitlab.config.lfs.enabled
end
- def full_path
- if parent
- parent.full_path + '/' + path
- else
- path
- end
- end
-
def shared_runners_enabled?
projects.with_shared_runners.any?
end
- def full_name
- @full_name ||=
- if parent
- parent.full_name + ' / ' + name
- else
- name
- end
- end
-
# Scopes the model on ancestors of the record
def ancestors
if parent_id
@@ -217,6 +200,10 @@ class Namespace < ActiveRecord::Base
[owner_id]
end
+ def parent_changed?
+ parent_id_changed?
+ end
+
private
def repository_storage_paths
@@ -255,10 +242,6 @@ class Namespace < ActiveRecord::Base
find_each(&:refresh_members_authorized_projects)
end
- def full_path_changed?
- path_changed? || parent_id_changed?
- end
-
def remove_exports!
Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
end
diff --git a/app/models/project.rb b/app/models/project.rb
index b45f22d94d9..c17bcedf7b2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -228,7 +228,12 @@ class Project < ActiveRecord::Base
scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
scope :with_statistics, -> { includes(:statistics) }
scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
- scope :inside_path, ->(path) { joins(:route).where('routes.path LIKE ?', "#{path}/%") }
+ scope :inside_path, ->(path) do
+ # We need routes alias rs for JOIN so it does not conflict with
+ # includes(:route) which we use in ProjectsFinder.
+ joins("INNER JOIN routes rs ON rs.source_id = projects.id AND rs.source_type = 'Project'").
+ where('rs.path LIKE ?', "#{path}/%")
+ end
# "enabled" here means "not disabled". It includes private features!
scope :with_feature_enabled, ->(feature) {
@@ -810,26 +815,6 @@ class Project < ActiveRecord::Base
end
end
- def name_with_namespace
- @name_with_namespace ||= begin
- if namespace
- namespace.human_name + ' / ' + name
- else
- name
- end
- end
- end
- alias_method :human_name, :name_with_namespace
-
- def full_path
- if namespace && path
- namespace.full_path + '/' + path
- else
- path
- end
- end
- alias_method :path_with_namespace, :full_path
-
def execute_hooks(data, hooks_scope = :push_hooks)
hooks.send(hooks_scope).each do |hook|
hook.async_execute(data, hooks_scope.to_s)
@@ -1328,6 +1313,18 @@ class Project < ActiveRecord::Base
map.public_path_for_source_path(path)
end
+ def parent
+ namespace
+ end
+
+ def parent_changed?
+ namespace_id_changed?
+ end
+
+ alias_method :name_with_namespace, :full_name
+ alias_method :human_name, :full_name
+ alias_method :path_with_namespace, :full_path
+
private
def cross_namespace_reference?(from)
@@ -1366,10 +1363,6 @@ class Project < ActiveRecord::Base
raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS
end
- def full_path_changed?
- path_changed? || namespace_id_changed?
- end
-
def update_project_statistics
stats = statistics || build_statistics
stats.update(namespace_id: namespace_id)
diff --git a/app/models/route.rb b/app/models/route.rb
index dd171fdb069..4eec3e73d5b 100644
--- a/app/models/route.rb
+++ b/app/models/route.rb
@@ -8,16 +8,22 @@ class Route < ActiveRecord::Base
presence: true,
uniqueness: { case_sensitive: false }
- after_update :rename_descendants, if: :path_changed?
+ after_update :rename_descendants
def rename_descendants
- # We update each row separately because MySQL does not have regexp_replace.
- # rubocop:disable Rails/FindEach
- Route.where('path LIKE ?', "#{path_was}/%").each do |route|
- # Note that update column skips validation and callbacks.
- # We need this to avoid recursive call of rename_descendants method
- route.update_column(:path, route.path.sub(path_was, path))
+ if path_changed? || name_changed?
+ descendants = Route.where('path LIKE ?', "#{path_was}/%")
+
+ descendants.each do |route|
+ attributes = {
+ path: route.path.sub(path_was, path),
+ name: route.name.sub(name_was, name)
+ }
+
+ # Note that update_columns skips validation and callbacks.
+ # We need this to avoid recursive call of rename_descendants method
+ route.update_columns(attributes)
+ end
end
- # rubocop:enable Rails/FindEach
end
end
diff --git a/changelogs/unreleased/dz-refactor-full-path.yml b/changelogs/unreleased/dz-refactor-full-path.yml
new file mode 100644
index 00000000000..da8568fd220
--- /dev/null
+++ b/changelogs/unreleased/dz-refactor-full-path.yml
@@ -0,0 +1,4 @@
+---
+title: Store group and project full name and full path in routes table
+merge_request: 8979
+author:
diff --git a/db/migrate/20170204172458_add_name_to_route.rb b/db/migrate/20170204172458_add_name_to_route.rb
new file mode 100644
index 00000000000..38ed1ad9039
--- /dev/null
+++ b/db/migrate/20170204172458_add_name_to_route.rb
@@ -0,0 +1,12 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddNameToRoute < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def change
+ add_column :routes, :name, :string
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 850772ba356..3fef5b82073 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1037,6 +1037,7 @@ ActiveRecord::Schema.define(version: 20170206101030) do
t.string "path", null: false
t.datetime "created_at"
t.datetime "updated_at"
+ t.string "name"
end
add_index "routes", ["path"], name: "index_routes_on_path", unique: true, using: :btree
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 30443534cca..e008ec28fa4 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -14,12 +14,14 @@ describe Group, 'Routable' do
describe 'Callbacks' do
it 'creates route record on create' do
expect(group.route.path).to eq(group.path)
+ expect(group.route.name).to eq(group.name)
end
it 'updates route record on path change' do
- group.update_attributes(path: 'wow')
+ group.update_attributes(path: 'wow', name: 'much')
expect(group.route.path).to eq('wow')
+ expect(group.route.name).to eq('much')
end
it 'ensure route path uniqueness across different objects' do
@@ -78,4 +80,34 @@ describe Group, 'Routable' do
it { is_expected.to eq([nested_group]) }
end
+
+ describe '#full_path' do
+ let(:group) { create(:group) }
+ let(:nested_group) { create(:group, parent: group) }
+
+ it { expect(group.full_path).to eq(group.path) }
+ it { expect(nested_group.full_path).to eq("#{group.path}/#{nested_group.path}") }
+ end
+
+ describe '#full_name' do
+ let(:group) { create(:group) }
+ let(:nested_group) { create(:group, parent: group) }
+
+ it { expect(group.full_name).to eq(group.name) }
+ it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
+ end
+end
+
+describe Project, 'Routable' do
+ describe '#full_path' do
+ let(:project) { build_stubbed(:empty_project) }
+
+ it { expect(project.full_path).to eq "#{project.namespace.path}/#{project.path}" }
+ end
+
+ describe '#full_name' do
+ let(:project) { build_stubbed(:empty_project) }
+
+ it { expect(project.full_name).to eq "#{project.namespace.human_name} / #{project.name}" }
+ end
end
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 7bb1657bc3a..5d2828b9bb0 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -175,22 +175,6 @@ describe Namespace, models: true do
end
end
- describe '#full_path' do
- let(:group) { create(:group) }
- let(:nested_group) { create(:group, parent: group) }
-
- it { expect(group.full_path).to eq(group.path) }
- it { expect(nested_group.full_path).to eq("#{group.path}/#{nested_group.path}") }
- end
-
- describe '#full_name' do
- let(:group) { create(:group) }
- let(:nested_group) { create(:group, parent: group) }
-
- it { expect(group.full_name).to eq(group.name) }
- it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
- end
-
describe '#ancestors' do
let(:group) { create(:group) }
let(:nested_group) { create(:group, parent: group) }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2129bcbd74d..35f3dd00870 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -275,13 +275,6 @@ describe Project, models: true do
it { is_expected.to delegate_method(:add_master).to(:team) }
end
- describe '#name_with_namespace' do
- let(:project) { build_stubbed(:empty_project) }
-
- it { expect(project.name_with_namespace).to eq "#{project.namespace.human_name} / #{project.name}" }
- it { expect(project.human_name).to eq project.name_with_namespace }
- end
-
describe '#to_reference' do
let(:owner) { create(:user, name: 'Gitlab') }
let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
@@ -1840,6 +1833,20 @@ describe Project, models: true do
end
end
+ describe '#parent' do
+ let(:project) { create(:empty_project) }
+
+ it { expect(project.parent).to eq(project.namespace) }
+ end
+
+ describe '#parent_changed?' do
+ let(:project) { create(:empty_project) }
+
+ before { project.namespace_id = 7 }
+
+ it { expect(project.parent_changed?).to be_truthy }
+ end
+
def enable_lfs
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
end
diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb
index dd2a5109abc..a276aa4b13a 100644
--- a/spec/models/route_spec.rb
+++ b/spec/models/route_spec.rb
@@ -1,7 +1,7 @@
require 'spec_helper'
describe Route, models: true do
- let!(:group) { create(:group, path: 'gitlab') }
+ let!(:group) { create(:group, path: 'gitlab', name: 'gitlab') }
let!(:route) { group.route }
describe 'relationships' do
@@ -15,17 +15,30 @@ describe Route, models: true do
end
describe '#rename_descendants' do
- let!(:nested_group) { create(:group, path: "test", parent: group) }
- let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) }
- let!(:similar_group) { create(:group, path: 'gitlab-org') }
+ let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
+ let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
+ let!(:similar_group) { create(:group, path: 'gitlab-org', name: 'gitlab-org') }
- before { route.update_attributes(path: 'bar') }
+ context 'path update' do
+ before { route.update_attributes(path: 'bar') }
- it "updates children routes with new path" do
- expect(described_class.exists?(path: 'bar')).to be_truthy
- expect(described_class.exists?(path: 'bar/test')).to be_truthy
- expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
- expect(described_class.exists?(path: 'gitlab-org')).to be_truthy
+ it "updates children routes with new path" do
+ expect(described_class.exists?(path: 'bar')).to be_truthy
+ expect(described_class.exists?(path: 'bar/test')).to be_truthy
+ expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
+ expect(described_class.exists?(path: 'gitlab-org')).to be_truthy
+ end
+ end
+
+ context 'name update' do
+ before { route.update_attributes(name: 'bar') }
+
+ it "updates children routes with new path" do
+ expect(described_class.exists?(name: 'bar')).to be_truthy
+ expect(described_class.exists?(name: 'bar / test')).to be_truthy
+ expect(described_class.exists?(name: 'bar / test / foo')).to be_truthy
+ expect(described_class.exists?(name: 'gitlab-org')).to be_truthy
+ end
end
end
end