Commit 8820f3cd authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'dz-refactor-full-path' into 'master'

Store group and project full name and full path in routes table

See merge request !8979
parents 66f9476a 2989192d
class Admin::DashboardController < Admin::ApplicationController class Admin::DashboardController < Admin::ApplicationController
def index def index
@projects = Project.limit(10) @projects = Project.with_route.limit(10)
@users = User.limit(10) @users = User.limit(10)
@groups = Group.limit(10) @groups = Group.with_route.limit(10)
end end
end end
...@@ -2,7 +2,7 @@ class Admin::GroupsController < Admin::ApplicationController ...@@ -2,7 +2,7 @@ class Admin::GroupsController < Admin::ApplicationController
before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update] before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update]
def index def index
@groups = Group.with_statistics @groups = Group.with_statistics.with_route
@groups = @groups.sort(@sort = params[:sort]) @groups = @groups.sort(@sort = params[:sort])
@groups = @groups.search(params[:name]) if params[:name].present? @groups = @groups.search(params[:name]) if params[:name].present?
@groups = @groups.page(params[:page]) @groups = @groups.page(params[:page])
......
class Dashboard::GroupsController < Dashboard::ApplicationController class Dashboard::GroupsController < Dashboard::ApplicationController
def index 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
end end
...@@ -2,7 +2,7 @@ class GroupsFinder < UnionFinder ...@@ -2,7 +2,7 @@ class GroupsFinder < UnionFinder
def execute(current_user = nil) def execute(current_user = nil)
segments = all_groups(current_user) segments = all_groups(current_user)
find_union(segments, Group).order_id_desc find_union(segments, Group).with_route.order_id_desc
end end
private private
......
...@@ -3,7 +3,7 @@ class ProjectsFinder < UnionFinder ...@@ -3,7 +3,7 @@ class ProjectsFinder < UnionFinder
segments = all_projects(current_user) segments = all_projects(current_user)
segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation
find_union(segments, Project) find_union(segments, Project).with_route
end end
private private
......
# Store object full path in separate table for easy lookup and uniq validation # 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 module Routable
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -9,7 +9,13 @@ module Routable ...@@ -9,7 +9,13 @@ module Routable
validates_associated :route validates_associated :route
validates :route, presence: true 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 end
class_methods do class_methods do
...@@ -77,10 +83,62 @@ module Routable ...@@ -77,10 +83,62 @@ module Routable
end end
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 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 || 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
end end
...@@ -177,27 +177,10 @@ class Namespace < ActiveRecord::Base ...@@ -177,27 +177,10 @@ class Namespace < ActiveRecord::Base
Gitlab.config.lfs.enabled Gitlab.config.lfs.enabled
end end
def full_path
if parent
parent.full_path + '/' + path
else
path
end
end
def shared_runners_enabled? def shared_runners_enabled?
projects.with_shared_runners.any? projects.with_shared_runners.any?
end end
def full_name
@full_name ||=
if parent
parent.full_name + ' / ' + name
else
name
end
end
# Scopes the model on ancestors of the record # Scopes the model on ancestors of the record
def ancestors def ancestors
if parent_id if parent_id
...@@ -224,6 +207,10 @@ class Namespace < ActiveRecord::Base ...@@ -224,6 +207,10 @@ class Namespace < ActiveRecord::Base
[owner_id] [owner_id]
end end
def parent_changed?
parent_id_changed?
end
private private
def repository_storage_paths def repository_storage_paths
...@@ -262,10 +249,6 @@ class Namespace < ActiveRecord::Base ...@@ -262,10 +249,6 @@ class Namespace < ActiveRecord::Base
find_each(&:refresh_members_authorized_projects) find_each(&:refresh_members_authorized_projects)
end end
def full_path_changed?
path_changed? || parent_id_changed?
end
def remove_exports! def remove_exports!
Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
end end
......
...@@ -228,7 +228,12 @@ class Project < ActiveRecord::Base ...@@ -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_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
scope :with_statistics, -> { includes(:statistics) } scope :with_statistics, -> { includes(:statistics) }
scope :with_shared_runners, -> { where(shared_runners_enabled: true) } 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! # "enabled" here means "not disabled". It includes private features!
scope :with_feature_enabled, ->(feature) { scope :with_feature_enabled, ->(feature) {
...@@ -810,26 +815,6 @@ class Project < ActiveRecord::Base ...@@ -810,26 +815,6 @@ class Project < ActiveRecord::Base
end end
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) def execute_hooks(data, hooks_scope = :push_hooks)
hooks.send(hooks_scope).each do |hook| hooks.send(hooks_scope).each do |hook|
hook.async_execute(data, hooks_scope.to_s) hook.async_execute(data, hooks_scope.to_s)
...@@ -1328,6 +1313,18 @@ class Project < ActiveRecord::Base ...@@ -1328,6 +1313,18 @@ class Project < ActiveRecord::Base
map.public_path_for_source_path(path) map.public_path_for_source_path(path)
end 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 private
def cross_namespace_reference?(from) def cross_namespace_reference?(from)
...@@ -1366,10 +1363,6 @@ class Project < ActiveRecord::Base ...@@ -1366,10 +1363,6 @@ class Project < ActiveRecord::Base
raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS raise BoardLimitExceeded, 'Number of permitted boards exceeded' if boards.size >= NUMBER_OF_PERMITTED_BOARDS
end end
def full_path_changed?
path_changed? || namespace_id_changed?
end
def update_project_statistics def update_project_statistics
stats = statistics || build_statistics stats = statistics || build_statistics
stats.update(namespace_id: namespace_id) stats.update(namespace_id: namespace_id)
......
...@@ -8,16 +8,22 @@ class Route < ActiveRecord::Base ...@@ -8,16 +8,22 @@ class Route < ActiveRecord::Base
presence: true, presence: true,
uniqueness: { case_sensitive: false } uniqueness: { case_sensitive: false }
after_update :rename_descendants, if: :path_changed? after_update :rename_descendants
def rename_descendants def rename_descendants
# We update each row separately because MySQL does not have regexp_replace. if path_changed? || name_changed?
# rubocop:disable Rails/FindEach descendants = Route.where('path LIKE ?', "#{path_was}/%")
Route.where('path LIKE ?', "#{path_was}/%").each do |route|
# Note that update column skips validation and callbacks. descendants.each do |route|
# We need this to avoid recursive call of rename_descendants method attributes = {
route.update_column(:path, route.path.sub(path_was, path)) 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 end
# rubocop:enable Rails/FindEach
end end
end end
---
title: Store group and project full name and full path in routes table
merge_request: 8979
author:
# 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
...@@ -1037,6 +1037,7 @@ ActiveRecord::Schema.define(version: 20170206101030) do ...@@ -1037,6 +1037,7 @@ ActiveRecord::Schema.define(version: 20170206101030) do
t.string "path", null: false t.string "path", null: false
t.datetime "created_at" t.datetime "created_at"
t.datetime "updated_at" t.datetime "updated_at"
t.string "name"
end end
add_index "routes", ["path"], name: "index_routes_on_path", unique: true, using: :btree add_index "routes", ["path"], name: "index_routes_on_path", unique: true, using: :btree
......
...@@ -14,12 +14,14 @@ describe Group, 'Routable' do ...@@ -14,12 +14,14 @@ describe Group, 'Routable' do
describe 'Callbacks' do describe 'Callbacks' do
it 'creates route record on create' do it 'creates route record on create' do
expect(group.route.path).to eq(group.path) expect(group.route.path).to eq(group.path)
expect(group.route.name).to eq(group.name)
end end
it 'updates route record on path change' do 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.path).to eq('wow')
expect(group.route.name).to eq('much')
end end
it 'ensure route path uniqueness across different objects' do it 'ensure route path uniqueness across different objects' do
...@@ -78,4 +80,34 @@ describe Group, 'Routable' do ...@@ -78,4 +80,34 @@ describe Group, 'Routable' do
it { is_expected.to eq([nested_group]) } it { is_expected.to eq([nested_group]) }
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
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 end
...@@ -186,22 +186,6 @@ describe Namespace, models: true do ...@@ -186,22 +186,6 @@ describe Namespace, models: true do
end end
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 describe '#ancestors' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:nested_group) { create(:group, parent: group) } let(:nested_group) { create(:group, parent: group) }
......
...@@ -275,13 +275,6 @@ describe Project, models: true do ...@@ -275,13 +275,6 @@ describe Project, models: true do
it { is_expected.to delegate_method(:add_master).to(:team) } it { is_expected.to delegate_method(:add_master).to(:team) }
end 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 describe '#to_reference' do
let(:owner) { create(:user, name: 'Gitlab') } let(:owner) { create(:user, name: 'Gitlab') }
let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) } let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
...@@ -1840,6 +1833,20 @@ describe Project, models: true do ...@@ -1840,6 +1833,20 @@ describe Project, models: true do
end end
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 def enable_lfs
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
end end
......
require 'spec_helper' require 'spec_helper'
describe Route, models: true do describe Route, models: true do
let!(:group) { create(:group, path: 'gitlab') } let!(:group) { create(:group, path: 'gitlab', name: 'gitlab') }
let!(:route) { group.route } let!(:route) { group.route }
describe 'relationships' do describe 'relationships' do
...@@ -15,17 +15,30 @@ describe Route, models: true do ...@@ -15,17 +15,30 @@ describe Route, models: true do
end end
describe '#rename_descendants' do describe '#rename_descendants' do
let!(:nested_group) { create(:group, path: "test", parent: group) } let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) }
let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) } let!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
let!(:similar_group) { create(:group, path: 'gitlab-org') } 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 it "updates children routes with new path" do
expect(described_class.exists?(path: 'bar')).to be_truthy 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')).to be_truthy
expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy expect(described_class.exists?(path: 'bar/test/foo')).to be_truthy
expect(described_class.exists?(path: 'gitlab-org')).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 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