Commit 72872ee2 authored by Michael Kozono's avatar Michael Kozono

Delete conflicting redirects

parent a0368e91
...@@ -7,4 +7,6 @@ class RedirectRoute < ActiveRecord::Base ...@@ -7,4 +7,6 @@ class RedirectRoute < ActiveRecord::Base
length: { within: 1..255 }, length: { within: 1..255 },
presence: true, presence: true,
uniqueness: { case_sensitive: false } uniqueness: { case_sensitive: false }
scope :matching_path_and_descendants, -> (path) { where('redirect_routes.path = ? OR redirect_routes.path LIKE ?', path, "#{sanitize_sql_like(path)}/%") }
end end
...@@ -8,7 +8,8 @@ class Route < ActiveRecord::Base ...@@ -8,7 +8,8 @@ class Route < ActiveRecord::Base
presence: true, presence: true,
uniqueness: { case_sensitive: false } uniqueness: { case_sensitive: false }
after_update :create_redirect_if_path_changed after_save :delete_conflicting_redirects
after_update :create_redirect_for_old_path
after_update :rename_direct_descendant_routes after_update :rename_direct_descendant_routes
scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") } scope :inside_path, -> (path) { where('routes.path LIKE ?', "#{sanitize_sql_like(path)}/%") }
...@@ -34,13 +35,19 @@ class Route < ActiveRecord::Base ...@@ -34,13 +35,19 @@ class Route < ActiveRecord::Base
end end
end end
def create_redirect_if_path_changed def delete_conflicting_redirects
if path_changed? conflicting_redirects.delete_all
create_redirect(path_was)
end end
def conflicting_redirects
RedirectRoute.matching_path_and_descendants(path)
end
def create_redirect_for_old_path
create_redirect(path_was) if path_changed?
end end
def create_redirect(old_path) def create_redirect(path)
source.redirect_routes.create(path: old_path) RedirectRoute.create(source: source, path: path)
end end
end end
require 'rails_helper' require 'rails_helper'
describe RedirectRoute, models: true do describe RedirectRoute, models: true do
let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') } let(:group) { create(:group) }
let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') } let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') }
describe 'relationships' do describe 'relationships' do
...@@ -13,4 +13,15 @@ describe RedirectRoute, models: true do ...@@ -13,4 +13,15 @@ describe RedirectRoute, models: true do
it { is_expected.to validate_presence_of(:path) } it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path) } it { is_expected.to validate_uniqueness_of(:path) }
end end
describe '.matching_path_and_descendants' do
let!(:redirect2) { group.redirect_routes.create(path: 'gitlabb/test') }
let!(:redirect3) { group.redirect_routes.create(path: 'gitlabb/test/foo') }
let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') }
let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') }
it 'returns correct routes' do
expect(RedirectRoute.matching_path_and_descendants('gitlabb/test')).to match_array([redirect2, redirect3, redirect4, redirect5])
end
end
end end
require 'spec_helper' require 'spec_helper'
describe Route, models: true do describe Route, models: true do
let!(:group) { create(:group, path: 'git_lab', name: 'git_lab') } let(:group) { create(:group, path: 'git_lab', name: 'git_lab') }
let!(:route) { group.route } let(:route) { group.route }
describe 'relationships' do describe 'relationships' do
it { is_expected.to belong_to(:source) } it { is_expected.to belong_to(:source) }
end end
describe 'validations' do describe 'validations' do
before { route }
it { is_expected.to validate_presence_of(:source) } it { is_expected.to validate_presence_of(:source) }
it { is_expected.to validate_presence_of(:path) } it { is_expected.to validate_presence_of(:path) }
it { is_expected.to validate_uniqueness_of(:path) } it { is_expected.to validate_uniqueness_of(:path) }
end end
describe 'callbacks' do
context 'after update' do
it 'calls #create_redirect_for_old_path' do
expect(route).to receive(:create_redirect_for_old_path)
route.update_attributes(path: 'foo')
end
it 'calls #delete_conflicting_redirects' do
expect(route).to receive(:delete_conflicting_redirects)
route.update_attributes(path: 'foo')
end
end
context 'after create' do
it 'calls #delete_conflicting_redirects' do
route.destroy
new_route = Route.new(source: group, path: group.path)
expect(new_route).to receive(:delete_conflicting_redirects)
new_route.save!
end
end
end
describe '.inside_path' do describe '.inside_path' do
let!(:nested_group) { create(:group, path: 'test', name: 'test', parent: group) } 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!(:deep_nested_group) { create(:group, path: 'foo', name: 'foo', parent: nested_group) }
...@@ -50,7 +74,7 @@ describe Route, models: true do ...@@ -50,7 +74,7 @@ describe Route, models: true do
context 'when route name is set' do context 'when route name is set' do
before { route.update_attributes(path: 'bar') } 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
...@@ -69,10 +93,24 @@ describe Route, models: true do ...@@ -69,10 +93,24 @@ describe Route, models: true do
expect(route.update_attributes(path: 'bar')).to be_truthy expect(route.update_attributes(path: 'bar')).to be_truthy
end end
end end
context 'when conflicting redirects exist' do
let!(:conflicting_redirect1) { route.create_redirect('bar/test') }
let!(:conflicting_redirect2) { route.create_redirect('bar/test/foo') }
let!(:conflicting_redirect3) { route.create_redirect('gitlab-org') }
it 'deletes the conflicting redirects' do
route.update_attributes(path: 'bar')
expect(RedirectRoute.exists?(path: 'bar/test')).to be_falsey
expect(RedirectRoute.exists?(path: 'bar/test/foo')).to be_falsey
expect(RedirectRoute.exists?(path: 'gitlab-org')).to be_truthy
end
end
end end
context 'name update' do context 'name update' do
it "updates children routes with new path" do it 'updates children routes with new path' do
route.update_attributes(name: 'bar') route.update_attributes(name: 'bar')
expect(described_class.exists?(name: 'bar')).to be_truthy expect(described_class.exists?(name: 'bar')).to be_truthy
...@@ -91,21 +129,70 @@ describe Route, models: true do ...@@ -91,21 +129,70 @@ describe Route, models: true do
end end
end end
describe '#create_redirect_if_path_changed' do describe '#create_redirect_for_old_path' do
context 'if the path changed' do context 'if the path changed' do
it 'creates a RedirectRoute for the old path' do it 'creates a RedirectRoute for the old path' do
redirect_scope = route.source.redirect_routes.where(path: 'git_lab')
expect(redirect_scope.exists?).to be_falsey
route.path = 'new-path' route.path = 'new-path'
route.save! route.save!
redirect = route.source.redirect_routes.first expect(redirect_scope.exists?).to be_truthy
expect(redirect.path).to eq('git_lab') end
end end
end end
context 'if the path did not change' do describe '#create_redirect' do
it 'does not create a RedirectRoute' do it 'creates a RedirectRoute with the same source' do
route.updated_at = Time.zone.now.utc redirect_route = route.create_redirect('foo')
route.save! expect(redirect_route).to be_a(RedirectRoute)
expect(route.source.redirect_routes).to be_empty expect(redirect_route).to be_persisted
expect(redirect_route.source).to eq(route.source)
expect(redirect_route.path).to eq('foo')
end
end
describe '#delete_conflicting_redirects' do
context 'when a redirect route with the same path exists' do
let!(:redirect1) { route.create_redirect(route.path) }
it 'deletes the redirect' do
route.delete_conflicting_redirects
expect(route.conflicting_redirects).to be_empty
end
context 'when redirect routes with paths descending from the route path exists' do
let!(:redirect2) { route.create_redirect("#{route.path}/foo") }
let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") }
let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") }
let!(:other_redirect) { route.create_redirect("other") }
it 'deletes all redirects with paths that descend from the route path' do
route.delete_conflicting_redirects
expect(route.conflicting_redirects).to be_empty
end
end
end
end
describe '#conflicting_redirects' do
context 'when a redirect route with the same path exists' do
let!(:redirect1) { route.create_redirect(route.path) }
it 'returns the redirect route' do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
expect(route.conflicting_redirects).to match_array([redirect1])
end
context 'when redirect routes with paths descending from the route path exists' do
let!(:redirect2) { route.create_redirect("#{route.path}/foo") }
let!(:redirect3) { route.create_redirect("#{route.path}/foo/bar") }
let!(:redirect4) { route.create_redirect("#{route.path}/baz/quz") }
let!(:other_redirect) { route.create_redirect("other") }
it 'returns the redirect routes' do
expect(route.conflicting_redirects).to be_an(ActiveRecord::Relation)
expect(route.conflicting_redirects).to match_array([redirect1, redirect2, redirect3, redirect4])
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