From 21a102843bc10944fb98afd0ffef6939d342748f Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 21 May 2020 10:10:49 +0200 Subject: [PATCH] Remove the url_nesting config option And always created nested urls. --- app/models/alchemy/page.rb | 2 +- app/models/alchemy/page/page_naming.rb | 18 +- lib/alchemy/config.rb | 4 +- lib/tasks/alchemy/convert.rake | 40 ---- .../alchemy/pages_controller_spec.rb | 39 ++-- spec/helpers/alchemy/pages_helper_spec.rb | 13 +- spec/libraries/config_spec.rb | 16 +- spec/models/alchemy/page_spec.rb | 175 ++++++------------ .../alchemy/admin/pages_controller_spec.rb | 114 ++++++------ 9 files changed, 157 insertions(+), 264 deletions(-) delete mode 100644 lib/tasks/alchemy/convert.rake diff --git a/app/models/alchemy/page.rb b/app/models/alchemy/page.rb index 8aea0340bf..647b43c512 100644 --- a/app/models/alchemy/page.rb +++ b/app/models/alchemy/page.rb @@ -428,7 +428,7 @@ def publish! def update_node!(node) hash = { lft: node.left, rgt: node.right, parent_id: node.parent, depth: node.depth, restricted: node.restricted } - if Config.get(:url_nesting) && urlname != node.url + if urlname != node.url LegacyPageUrl.create(page_id: id, urlname: urlname) hash[:urlname] = node.url end diff --git a/app/models/alchemy/page/page_naming.rb b/app/models/alchemy/page/page_naming.rb index 7e8bd38fad..142a5caf18 100644 --- a/app/models/alchemy/page/page_naming.rb +++ b/app/models/alchemy/page/page_naming.rb @@ -24,8 +24,7 @@ module Page::PageNaming after_update :update_descendants_urlnames, if: :should_update_descendants_urlnames? - after_move :update_urlname!, - if: -> { Config.get(:url_nesting) } + after_move :update_urlname! end # Returns true if name or urlname has changed. @@ -64,8 +63,6 @@ def visible_ancestors private def should_update_descendants_urlnames? - return false if !Config.get(:url_nesting) - saved_change_to_urlname? || saved_change_to_visible? end @@ -76,14 +73,9 @@ def update_descendants_urlnames # Sets the urlname to a url friendly slug. # Either from name, or if present, from urlname. - # If url_nesting is enabled the urlname contains the whole path. + # The urlname contains the whole path including parent urlnames. def set_urlname - if Config.get(:url_nesting) - value = slug - else - value = urlname - end - self[:urlname] = nested_url_name(value) + self[:urlname] = nested_url_name(slug) end def set_title @@ -110,9 +102,9 @@ def nested_url_name(value) # Slugs of all visible/non-language_root ancestors. # Returns [], if there is no parent, the parent is - # the root page itself, or url_nesting is off. + # the root page itself. def ancestor_slugs - return [] if !Config.get(:url_nesting) || parent.nil? + return [] if parent.nil? visible_ancestors.map(&:slug).compact end diff --git a/lib/alchemy/config.rb b/lib/alchemy/config.rb index e54b421c63..ebaf634c71 100644 --- a/lib/alchemy/config.rb +++ b/lib/alchemy/config.rb @@ -31,9 +31,7 @@ def show # a value of nil means there is no new default # any not nil value is the new default def deprecated_configs - { - url_nesting: true, - } + {} end private diff --git a/lib/tasks/alchemy/convert.rake b/lib/tasks/alchemy/convert.rake deleted file mode 100644 index adb4a3152a..0000000000 --- a/lib/tasks/alchemy/convert.rake +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true -namespace :alchemy do - namespace :convert do - namespace :urlnames do - desc "Converts the urlname of all pages to nested url paths." - task to_nested: [:environment] do - Alchemy::Deprecation.warn('alchemy:convert:urlnames:to_nested task is deprecated and will be removed from Alchemy 5.0') - unless Alchemy::Config.get(:url_nesting) - raise "\nURL nesting is disabled! Please enable url_nesting in `config/alchemy/config.yml` first.\n\n" - end - - puts "Converting..." - pages = Alchemy::Page.contentpages - count = pages.count - pages.each_with_index do |page, n| - puts "Updating page #{n + 1} of #{count}" - page.update_urlname! - end - puts "Done." - end - - desc "Converts the urlname of all pages to contain the slug only." - task to_slug: [:environment] do - Alchemy::Deprecation.warn('alchemy:convert:urlnames:to_slug task is deprecated and will be removed from Alchemy 5.0') - if Alchemy::Config.get(:url_nesting) - raise "\nURL nesting is enabled! Please disable url_nesting in `config/alchemy/config.yml` first.\n\n" - end - - puts "Converting..." - pages = Alchemy::Page.contentpages - count = pages.count - pages.each_with_index do |page, n| - puts "Updating page #{n + 1} of #{count}" - page.update_attribute :urlname, page.slug - end - puts "Done." - end - end - end -end diff --git a/spec/controllers/alchemy/pages_controller_spec.rb b/spec/controllers/alchemy/pages_controller_spec.rb index c3dab217be..efd81a8870 100644 --- a/spec/controllers/alchemy/pages_controller_spec.rb +++ b/spec/controllers/alchemy/pages_controller_spec.rb @@ -32,7 +32,7 @@ module Alchemy it "returns a 404" do expect { get(:index) }.to raise_exception( ActionController::RoutingError, - 'Alchemy::Page not found "/"', + 'Alchemy::Page not found "/"', ) end end @@ -153,12 +153,12 @@ module Alchemy end it "loads the root page of that language" do - get :index, params: {locale: "en"} + get :index, params: { locale: "en" } expect(assigns(:page)).to eq(start_page) end it "sets @root_page to root page of that language" do - get :index, params: {locale: "en"} + get :index, params: { locale: "en" } expect(assigns(:root_page)).to eq(start_page) end end @@ -173,7 +173,7 @@ module Alchemy it "renders 404" do expect { - get :show, params: {urlname: not_yet_public.urlname} + get :show, params: { urlname: not_yet_public.urlname } }.to raise_error(ActionController::RoutingError) end end @@ -188,7 +188,7 @@ module Alchemy it "renders 404" do expect { - get :show, params: {urlname: no_longer_public.urlname} + get :show, params: { urlname: no_longer_public.urlname } }.to raise_error(ActionController::RoutingError) end end @@ -202,7 +202,7 @@ module Alchemy end it "renders page" do - get :show, params: {urlname: still_public_page.urlname} + get :show, params: { urlname: still_public_page.urlname } expect(response).to be_successful end end @@ -216,7 +216,7 @@ module Alchemy end it "renders page" do - get :show, params: {urlname: still_public_page.urlname} + get :show, params: { urlname: still_public_page.urlname } expect(response).to be_successful end end @@ -225,20 +225,20 @@ module Alchemy render_views it "should render a rss feed" do - get :show, params: {urlname: page.urlname, format: :rss} + get :show, params: { urlname: page.urlname, format: :rss } expect(response.media_type).to eq("application/rss+xml") end it "should include content" do page.elements.first.content_by_name("news_headline").essence.update_columns(body: "Peters Petshop") - get :show, params: {urlname: "news", format: :rss} + get :show, params: { urlname: "news", format: :rss } expect(response.body).to match /Peters Petshop/ end end context "requested for a page that does not contain a feed" do it "should render xml 404 error" do - get :show, params: {urlname: default_language_root.urlname, format: :rss} + get :show, params: { urlname: default_language_root.urlname, format: :rss } expect(response.status).to eq(404) end end @@ -246,7 +246,7 @@ module Alchemy describe "Layout rendering" do context "with ajax request" do it "should not render a layout" do - get :show, params: {urlname: page.urlname}, xhr: true + get :show, params: { urlname: page.urlname }, xhr: true expect(response).to render_template(:show) expect(response).not_to render_template(layout: "application") end @@ -256,19 +256,18 @@ module Alchemy describe "url nesting" do render_views - let(:catalog) { create(:alchemy_page, :public, name: "Catalog", urlname: "catalog", parent: default_language_root, language: default_language, visible: true) } + let(:catalog) { create(:alchemy_page, :public, name: "Catalog", urlname: "catalog", parent: default_language_root, language: default_language, visible: true) } let(:products) { create(:alchemy_page, :public, name: "Products", urlname: "products", parent: catalog, language: default_language, visible: true) } - let(:product) { create(:alchemy_page, :public, name: "Screwdriver", urlname: "screwdriver", parent: products, language: default_language, autogenerate_elements: true, visible: true) } + let(:product) { create(:alchemy_page, :public, name: "Screwdriver", urlname: "screwdriver", parent: products, language: default_language, autogenerate_elements: true, visible: true) } before do allow(Alchemy.user_class).to receive(:admins).and_return(OpenStruct.new(count: 1)) - stub_alchemy_config(:url_nesting, true) product.elements.find_by_name("article").contents.essence_texts.first.essence.update_column(:body, "screwdriver") end context "with correct levelnames in params" do it "should show the requested page" do - get :show, params: {urlname: "catalog/products/screwdriver"} + get :show, params: { urlname: "catalog/products/screwdriver" } expect(response.status).to eq(200) expect(response.body).to have_content("screwdriver") end @@ -277,7 +276,7 @@ module Alchemy context "with incorrect levelnames in params" do it "should render a 404 page" do expect { - get :show, params: {urlname: "catalog/faqs/screwdriver"} + get :show, params: { urlname: "catalog/faqs/screwdriver" } }.to raise_error(ActionController::RoutingError) end end @@ -286,7 +285,7 @@ module Alchemy context "when a non-existent page is requested" do it "should rescue a RoutingError with rendering a 404 page." do expect { - get :show, params: {urlname: "doesntexist"} + get :show, params: { urlname: "doesntexist" } }.to raise_error(ActionController::RoutingError) end end @@ -299,12 +298,12 @@ module Alchemy context "with no lang parameter present" do it "should store defaults language id in the session." do - get :show, params: {urlname: page.urlname} + get :show, params: { urlname: page.urlname } expect(controller.session[:alchemy_language_id]).to eq(Language.default.id) end it "should store default language as class var." do - get :show, params: {urlname: page.urlname} + get :show, params: { urlname: page.urlname } expect(Language.current).to eq(Language.default) end end @@ -326,7 +325,7 @@ module Alchemy end it "renders the page related to its language" do - get :show, params: {urlname: "same-name", locale: klingon_page.language_code} + get :show, params: { urlname: "same-name", locale: klingon_page.language_code } expect(response.body).to have_content("klingon page") end end diff --git a/spec/helpers/alchemy/pages_helper_spec.rb b/spec/helpers/alchemy/pages_helper_spec.rb index 10d62e1162..c7b3cad8ec 100644 --- a/spec/helpers/alchemy/pages_helper_spec.rb +++ b/spec/helpers/alchemy/pages_helper_spec.rb @@ -4,14 +4,13 @@ module Alchemy describe PagesHelper do - let(:language_root) { create(:alchemy_page, :language_root) } - let(:public_page) { create(:alchemy_page, :public) } - let(:klingon) { create(:alchemy_language, :klingon) } - let(:klingon_language_root) { create(:alchemy_page, :language_root, language: klingon) } + let(:language_root) { create(:alchemy_page, :language_root) } + let(:public_page) { create(:alchemy_page, :public) } + let(:klingon) { create(:alchemy_language, :klingon) } + let(:klingon_language_root) { create(:alchemy_page, :language_root, language: klingon) } before do helper.controller.class_eval { include Alchemy::ConfigurationMethods } - allow(Config).to receive(:get) { |arg| arg == :url_nesting ? true : Config.parameter(arg) } @root_page = language_root # We need this instance variable in the helpers end @@ -92,8 +91,8 @@ module Alchemy describe "#render_breadcrumb" do let(:parent) { create(:alchemy_page, :public, visible: true) } - let(:page) { create(:alchemy_page, :public, parent_id: parent.id, visible: true) } - let(:user) { nil } + let(:page) { create(:alchemy_page, :public, parent_id: parent.id, visible: true) } + let(:user) { nil } before do allow(helper).to receive(:multi_language?).and_return(false) diff --git a/spec/libraries/config_spec.rb b/spec/libraries/config_spec.rb index 9283846e9b..50b74f109e 100644 --- a/spec/libraries/config_spec.rb +++ b/spec/libraries/config_spec.rb @@ -19,28 +19,34 @@ module Alchemy context "without a new default" do before do expect(described_class).to receive(:deprecated_configs).at_least(:once) do - { url_nesting: nil } + { foo: nil } end end it "warns" do expect(Alchemy::Deprecation).to receive(:warn) - Config.get(:url_nesting) + Config.get(:foo) end end context "with a new default" do + before do + expect(described_class).to receive(:deprecated_configs).at_least(:once) do + { foo: true } + end + end + context "and current value is not the default" do before do expect(described_class).to receive(:show).at_least(:once) do - { "url_nesting" => false } + { "foo" => false } end end it "warns about new default" do expect(Alchemy::Deprecation).to \ - receive(:warn).with("Setting url_nesting configuration to false is deprecated and will be always true in Alchemy 5.0") - Config.get(:url_nesting) + receive(:warn).with("Setting foo configuration to false is deprecated and will be always true in Alchemy 5.0") + Config.get(:foo) end end end diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index a0f2a8b02e..9a64a09223 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -37,25 +37,15 @@ module Alchemy contentpage.urlname = "existing_twice" expect(contentpage).not_to be_valid end - end - - context "with url_nesting set to true" do - let(:other_parent) { create(:alchemy_page, visible: true) } - - before do - stub_alchemy_config(:url_nesting, true) - with_same_urlname - end - it "should only validate urlname dependent of parent" do - contentpage.urlname = "existing_twice" - contentpage.parent = other_parent - expect(contentpage).to be_valid - end + context "with another parent" do + let(:other_parent) { create(:alchemy_page, visible: true) } - it "should validate urlname dependent of parent" do - contentpage.urlname = "existing_twice" - expect(contentpage).not_to be_valid + it "should be valid" do + contentpage.urlname = "existing_twice" + contentpage.parent = other_parent + expect(contentpage).to be_valid + end end end end @@ -1571,70 +1561,54 @@ module Alchemy let(:contact) { create(:alchemy_page, parent: invisible, name: "contact", visible: true) } let(:language_root) { parentparent.parent } - context "with activated url_nesting" do - before do - stub_alchemy_config(:url_nesting, true) - end - - it "should store all parents urlnames delimited by slash" do - expect(page.urlname).to eq("parentparent/parent/page") - end - - it "should not include the language root page" do - expect(page.urlname).not_to match(/startseite/) - end + it "should store all parents urlnames delimited by slash" do + expect(page.urlname).to eq("parentparent/parent/page") + end - it "should not include invisible pages" do - expect(contact.urlname).not_to match(/invisible/) - end + it "should not include the language root page" do + expect(page.urlname).not_to match(/startseite/) + end - context "with an invisible parent" do - before { parent.update_attribute(:visible, false) } + it "should not include invisible pages" do + expect(contact.urlname).not_to match(/invisible/) + end - it "does not change if set_urlname is called" do - expect { page.send(:set_urlname) }.not_to change { page.urlname } - end + context "with an invisible parent" do + before { parent.update_attribute(:visible, false) } - it "does not change if update_urlname! is called" do - expect { page.update_urlname! }.not_to change { page.urlname } - end + it "does not change if set_urlname is called" do + expect { page.send(:set_urlname) }.not_to change { page.urlname } end - context "after changing page's urlname" do - it "updates urlnames of descendants" do - page - parentparent.urlname = "new-urlname" - parentparent.save! - page.reload - expect(page.urlname).to eq("new-urlname/parent/page") - end - - it "should create a legacy url" do - allow(page).to receive(:slug).and_return("foo") - page.update_urlname! - expect(page.legacy_urls).not_to be_empty - expect(page.legacy_urls.pluck(:urlname)).to include("parentparent/parent/page") - end + it "does not change if update_urlname! is called" do + expect { page.update_urlname! }.not_to change { page.urlname } end + end - context "after updating my visibility" do - it "should update urlnames of descendants" do - page - parentparent.visible = false - parentparent.save! - page.reload - expect(page.urlname).to eq("parent/page") - end + context "after changing page's urlname" do + it "updates urlnames of descendants" do + page + parentparent.urlname = "new-urlname" + parentparent.save! + page.reload + expect(page.urlname).to eq("new-urlname/parent/page") end - end - context "with disabled url_nesting" do - before do - stub_alchemy_config(:url_nesting, false) + it "should create a legacy url" do + allow(page).to receive(:slug).and_return("foo") + page.update_urlname! + expect(page.legacy_urls).not_to be_empty + expect(page.legacy_urls.pluck(:urlname)).to include("parentparent/parent/page") end + end - it "should only store my urlname" do - expect(page.urlname).to eq("page") + context "after updating my visibility" do + it "should update urlnames of descendants" do + page + parentparent.visible = false + parentparent.save! + page.reload + expect(page.urlname).to eq("parent/page") end end end @@ -1644,61 +1618,32 @@ module Alchemy let(:page) { create(:alchemy_page, language: language, parent: language_root, urlname: original_url, restricted: false) } let(:node) { TreeNode.new(10, 11, 12, 13, "another-url", true) } - context "when nesting is enabled" do - before do - stub_alchemy_config(:url_nesting, true) - end - - it "should update all attributes" do - page.update_node!(node) - page.reload - expect(page.lft).to eq(node.left) - expect(page.rgt).to eq(node.right) - expect(page.parent_id).to eq(node.parent) - expect(page.depth).to eq(node.depth) - expect(page.urlname).to eq(node.url) - expect(page.restricted).to eq(node.restricted) - end - - context "when url is the same" do - let(:node) { TreeNode.new(10, 11, 12, 13, original_url, true) } - - it "should not create a legacy url" do - page.update_node!(node) - page.reload - expect(page.legacy_urls.size).to eq(0) - end - end - - context "when url is not the same" do - it "should create a legacy url" do - page.update_node!(node) - page.reload - expect(page.legacy_urls.size).to eq(1) - end - end + it "should update all attributes" do + page.update_node!(node) + page.reload + expect(page.lft).to eq(node.left) + expect(page.rgt).to eq(node.right) + expect(page.parent_id).to eq(node.parent) + expect(page.depth).to eq(node.depth) + expect(page.urlname).to eq(node.url) + expect(page.restricted).to eq(node.restricted) end - context "when nesting is disabled" do - before do - stub_alchemy_config(:url_nesting, false) - end + context "when url is the same" do + let(:node) { TreeNode.new(10, 11, 12, 13, original_url, true) } - it "should update all attributes except url" do + it "should not create a legacy url" do page.update_node!(node) page.reload - expect(page.lft).to eq(node.left) - expect(page.rgt).to eq(node.right) - expect(page.parent_id).to eq(node.parent) - expect(page.depth).to eq(node.depth) - expect(page.urlname).to eq(original_url) - expect(page.restricted).to eq(node.restricted) + expect(page.legacy_urls.size).to eq(0) end + end - it "should not create a legacy url" do + context "when url is not the same" do + it "should create a legacy url" do page.update_node!(node) page.reload - expect(page.legacy_urls.size).to eq(0) + expect(page.legacy_urls.size).to eq(1) end end end diff --git a/spec/requests/alchemy/admin/pages_controller_spec.rb b/spec/requests/alchemy/admin/pages_controller_spec.rb index 36f24c1901..32bca82258 100644 --- a/spec/requests/alchemy/admin/pages_controller_spec.rb +++ b/spec/requests/alchemy/admin/pages_controller_spec.rb @@ -317,85 +317,79 @@ module Alchemy expect(page_1.descendants).to eq([page_2, page_3]) end - context "with url nesting enabled" do - before do - stub_alchemy_config(:url_nesting, true) + it "updates the pages urlnames" do + post order_admin_pages_path(set: set_of_pages.to_json), xhr: true + [page_1, page_2, page_3].map(&:reload) + expect(page_1.urlname).to eq(page_1.slug.to_s) + expect(page_2.urlname).to eq("#{page_1.slug}/#{page_2.slug}") + expect(page_3.urlname).to eq("#{page_1.slug}/#{page_2.slug}/#{page_3.slug}") + end + + context "with invisible page in tree" do + let(:page_item_2) do + { + id: page_2.id, + slug: page_2.slug, + children: [page_item_3], + visible: false, + } end - it "updates the pages urlnames" do + it "does not use this pages slug in urlnames of descendants" do post order_admin_pages_path(set: set_of_pages.to_json), xhr: true [page_1, page_2, page_3].map(&:reload) expect(page_1.urlname).to eq(page_1.slug.to_s) expect(page_2.urlname).to eq("#{page_1.slug}/#{page_2.slug}") - expect(page_3.urlname).to eq("#{page_1.slug}/#{page_2.slug}/#{page_3.slug}") + expect(page_3.urlname).to eq("#{page_1.slug}/#{page_3.slug}") end + end - context "with invisible page in tree" do - let(:page_item_2) do - { - id: page_2.id, - slug: page_2.slug, - children: [page_item_3], - visible: false, - } - end - - it "does not use this pages slug in urlnames of descendants" do - post order_admin_pages_path(set: set_of_pages.to_json), xhr: true - [page_1, page_2, page_3].map(&:reload) - expect(page_1.urlname).to eq(page_1.slug.to_s) - expect(page_2.urlname).to eq("#{page_1.slug}/#{page_2.slug}") - expect(page_3.urlname).to eq("#{page_1.slug}/#{page_3.slug}") - end + context "with restricted page in tree" do + let(:page_2) { create(:alchemy_page, restricted: true) } + let(:page_item_2) do + { + id: page_2.id, + slug: page_2.slug, + children: [page_item_3], + restricted: true, + } end - context "with restricted page in tree" do - let(:page_2) { create(:alchemy_page, restricted: true) } - let(:page_item_2) do - { - id: page_2.id, - slug: page_2.slug, - children: [page_item_3], - restricted: true, - } - end - - it "updates restricted status of descendants" do - post order_admin_pages_path(set: set_of_pages.to_json), xhr: true - page_3.reload - expect(page_3.restricted).to be_truthy - end + it "updates restricted status of descendants" do + post order_admin_pages_path(set: set_of_pages.to_json), xhr: true + page_3.reload + expect(page_3.restricted).to be_truthy end + end - context "with page having number as slug" do - let(:page_item_2) do - { - id: page_2.id, - slug: 42, - children: [page_item_3], - } - end - - it "does not raise error" do - expect { - post order_admin_pages_path(set: set_of_pages.to_json), xhr: true - }.not_to raise_error - end + context "with page having number as slug" do + let(:page_item_2) do + { + id: page_2.id, + slug: 42, + children: [page_item_3], + } + end - it "still generates the correct urlname on page_3" do + it "does not raise error" do + expect { post order_admin_pages_path(set: set_of_pages.to_json), xhr: true - [page_1, page_2, page_3].map(&:reload) - expect(page_3.urlname).to eq("#{page_1.slug}/#{page_2.slug}/#{page_3.slug}") - end + }.not_to raise_error end - it "creates legacy urls" do + it "still generates the correct urlname on page_3" do post order_admin_pages_path(set: set_of_pages.to_json), xhr: true - [page_2, page_3].map(&:reload) - expect(page_2.legacy_urls.size).to eq(1) - expect(page_3.legacy_urls.size).to eq(1) + [page_1, page_2, page_3].map(&:reload) + expect(page_3.urlname).to eq("#{page_1.slug}/#{page_2.slug}/#{page_3.slug}") end end + + it "creates legacy urls" do + post order_admin_pages_path(set: set_of_pages.to_json), xhr: true + [page_2, page_3].map(&:reload) + expect(page_2.legacy_urls.size).to eq(1) + expect(page_3.legacy_urls.size).to eq(1) + end end describe "#configure" do