Set proper nested set scope on page (AlchemyCMS#1837)

* Set proper nested set scope on page

* Ensure layoutpages factory never creates a parent

With the awesome nested set scope set to language and layoutpage there is no parent with the same scope to move the newly created page to.

* Ensure page factory sets language from parent

If the page has a parent set we use that as the language. We must avoid to call the parent method in the factory, that's why we use the @cached_attributes here.

* Fix page language_root factory for when language is nil

We might set language to nil in tests explicitly.

* Refactor page specs after factory changes

Now that the page factory sets the language to the parent if necessary we can rewrite some specs.

app/models/alchemy/page.rb

@@ -83,7 +83,7 @@ class Page < BaseRecord
       :menu_id,
     ]
 
-    acts_as_nested_set(dependent: :destroy)
+    acts_as_nested_set(dependent: :destroy, scope: [:layoutpage, :language_id])
 
     stampable stamper_class_name: Alchemy.user_class_name
 

lib/alchemy/test_support/factories/page_factory.rb

@@ -5,7 +5,11 @@
 
 FactoryBot.define do
   factory :alchemy_page, class: "Alchemy::Page" do
-    language { Alchemy::Language.default || FactoryBot.create(:alchemy_language) }
+    language do
+      @cached_attributes[:parent]&.language ||
+        Alchemy::Language.default ||
+        FactoryBot.create(:alchemy_language)
+    end
     sequence(:name) { |n| "A Page #{n}" }
     page_layout { "standard" }
 
@@ -19,8 +23,8 @@
     autogenerate_elements { false }
 
     trait :language_root do
-      name { language.frontpage_name }
-      page_layout { language.page_layout }
+      name { language&.frontpage_name || "Intro" }
+      page_layout { language&.page_layout || "index" }
       language_root { true }
       public_on { Time.current }
       parent { nil }
@@ -32,6 +36,7 @@
     end
 
     trait :layoutpage do
+      parent { nil }
       layoutpage { true }
       page_layout { "footer" }
     end

spec/features/admin/page_editing_feature_spec.rb

@@ -31,7 +31,7 @@
     end
 
     context "while editing a global page" do
-      let(:a_page) { create(:alchemy_page, layoutpage: true) }
+      let(:a_page) { create(:alchemy_page, :layoutpage) }
 
       it "can publish page." do
         visit alchemy.edit_admin_page_path(a_page)
@@ -57,7 +57,7 @@
 
         context "with sitemaps show_flag config option set to true" do
           before do
-            stub_alchemy_config(:sitemap, {"show_flag" => true})
+            stub_alchemy_config(:sitemap, { "show_flag" => true })
           end
 
           it "should show sitemap checkbox" do
@@ -68,7 +68,7 @@
 
         context "with sitemaps show_flag config option set to false" do
           before do
-            stub_alchemy_config(:sitemap, {"show_flag" => false})
+            stub_alchemy_config(:sitemap, { "show_flag" => false })
           end
 
           it "should not show sitemap checkbox" do
@@ -79,7 +79,7 @@
       end
 
       context "when editing a global page" do
-        let(:layout_page) { create(:alchemy_page, layoutpage: true) }
+        let(:layout_page) { create(:alchemy_page, :layoutpage) }
 
         it "should not show the input fields for normal pages" do
           visit alchemy.edit_admin_layoutpage_path(layout_page)
@@ -92,8 +92,7 @@
 
       context "when page is taggable" do
         before do
-          expect_any_instance_of(Alchemy::Page)
-            .to receive(:taggable?).and_return(true)
+          expect_any_instance_of(Alchemy::Page).to receive(:taggable?).and_return(true)
         end
 
         it "should show the tag_list input field" do

spec/models/alchemy/page_spec.rb

@@ -19,7 +19,7 @@ module Alchemy
       context "Creating a normal content page" do
         let(:contentpage) { build(:alchemy_page) }
         let(:with_same_urlname) { create(:alchemy_page, urlname: "existing_twice") }
-        let(:global_with_same_urlname) { create(:alchemy_page, urlname: "existing_twice", layoutpage: true) }
+        let(:global_with_same_urlname) { create(:alchemy_page, :layoutpage, urlname: "existing_twice") }
 
         context "when its urlname exists as global page" do
           before { global_with_same_urlname }
@@ -313,9 +313,12 @@ module Alchemy
     end
 
     describe ".locked" do
-      it "should return 1 page that is blocked by a user at the moment" do
-        create(:alchemy_page, :public, :locked, name: "First Public Child", parent: language_root, language: language)
-        expect(Page.locked.size).to eq(1)
+      let!(:locked_page) { create(:alchemy_page, :locked) }
+
+      subject { Page.locked }
+
+      it "returns pages that are locked by a user" do
+        is_expected.to include(locked_page)
       end
     end
 
@@ -350,17 +353,15 @@ module Alchemy
 
     describe ".contentpages" do
       let!(:layoutpage) do
-        create :alchemy_page, {
+        create :alchemy_page, :layoutpage, {
           name: "layoutpage",
-          layoutpage: true,
           language: klingon,
         }
       end
 
       let!(:klingon_lang_root) do
         create :alchemy_page, :language_root, {
           name: "klingon_lang_root",
-          layoutpage: false,
           language: klingon,
         }
       end
@@ -369,24 +370,21 @@ module Alchemy
         create :alchemy_page, {
           name: "contentpage",
           parent: language_root,
-          language: language,
         }
       end
 
+      subject { Page.contentpages }
+
       it "returns a collection of contentpages" do
-        expect(Page.contentpages.to_a).to include(
+        is_expected.to include(
           language_root,
           klingon_lang_root,
           contentpage,
         )
       end
 
-      it "does not contain pages with attribute :layoutpage set to true" do
-        expect(Page.contentpages.to_a.select { |p| p.layoutpage == true }).to be_empty
-      end
-
-      it "contains pages with attribute :layoutpage set to false" do
-        expect(Page.contentpages.to_a.select { |p| p.layoutpage == false }).to include(klingon_lang_root)
+      it "does not contain layout pages" do
+        is_expected.to_not include(layoutpage)
       end
     end
 
@@ -531,76 +529,74 @@ module Alchemy
           expect(page.class.stamper_class.to_s).to eq("DummyUser")
         end
 
-        context "with language given" do
-          let!(:page) { Page.create!(name: "A New Page", parent: language_root, language: language, page_layout: "standard") }
+        context "with language already given" do
+          let(:page) { create(:alchemy_page, parent: language_root, language: language_root.language) }
 
-          it "does not set the language from parent" do
-            expect(page.language).to eq(language)
+          it "does not set the language again" do
+            expect(page).not_to receive(:set_language)
+            page
           end
         end
 
         context "with no language given" do
-          let!(:page) { Page.create!(name: "A New Page", parent: language_root, language: nil, page_layout: "standard") }
+          context "with parent given" do
+            let!(:page) { create(:alchemy_page, parent: language_root, language: nil) }
 
-          it "sets the language from parent" do
-            expect(page.language).to eq(language_root.language)
+            it "sets the language from parent" do
+              expect(page.language).to eq(language_root.language)
+            end
+          end
+
+          context "with no parent given" do
+            let!(:current_language) { create(:alchemy_language, default: true) }
+            let!(:page) { create(:alchemy_page, language: nil) }
+
+            it "sets the current language" do
+              expect(page.language).to eq(current_language)
+            end
           end
         end
       end
     end
 
     describe ".language_roots" do
+      let!(:language_root) { create(:alchemy_page, :language_root) }
+
       it "should return 1 language_root" do
-        create(:alchemy_page, :public, name: "First Public Child", parent: language_root, language: language)
-        expect(Page.language_roots.size).to eq(1)
+        expect(Page.language_roots.to_a).to eq([language_root])
       end
     end
 
     describe ".layoutpages" do
+      let!(:layoutpage) { create(:alchemy_page, :layoutpage) }
+
       it "should return layoutpages" do
-        create(:alchemy_page, :public, layoutpage: true, name: "Layoutpage", language: language)
-        expect(Page.layoutpages.size).to eq(1)
+        expect(Page.layoutpages.to_a).to eq([layoutpage])
       end
     end
 
     describe ".not_locked" do
+      let!(:not_locked) { create(:alchemy_page, :language_root) }
+
       it "should return pages that are not blocked by a user at the moment" do
-        create(:alchemy_page, :public, :locked, name: "First Public Child", parent: language_root, language: language)
-        create(:alchemy_page, :public, name: "Second Public Child", parent: language_root, language: language)
-        expect(Page.not_locked.size).to eq(2)
+        expect(Page.not_locked.to_a).to eq([not_locked])
       end
     end
 
     describe ".not_restricted" do
+      let!(:not_restricted) { create(:alchemy_page, :language_root) }
+
       it "should return accessible pages" do
-        create(:alchemy_page, :public, name: "First Public Child", restricted: true, parent: language_root, language: language)
-        expect(Page.not_restricted.size).to eq(1)
+        expect(Page.not_restricted.to_a).to eq([not_restricted])
       end
     end
 
     describe ".published" do
       subject(:published) { Page.published }
 
-      let!(:public_one) do
-        create :alchemy_page, :public,
-          name: "First Public Child",
-          parent: language_root,
-          language: language
-      end
-
-      let!(:public_two) do
-        create :alchemy_page, :public,
-          name: "Second Public Child",
-          parent: language_root,
-          language: language
-      end
-
-      let!(:non_public_page) do
-        create :alchemy_page,
-          name: "Non Public Child",
-          parent: language_root,
-          language: language
-      end
+      let!(:public_one) { create(:alchemy_page, :public) }
+      let!(:public_two) { create(:alchemy_page, :public) }
+      let!(:non_public_page) { create(:alchemy_page) }
 
       it "returns public available pages" do
         expect(published).to include(public_one)
@@ -610,23 +606,26 @@ module Alchemy
     end
 
     describe ".public_language_roots" do
+      let!(:public_language_root) { create(:alchemy_page, :public, :language_root) }
+
       it "should return pages that public language roots" do
-        create(:alchemy_page, :public, name: "First Public Child", parent: language_root, language: language)
-        expect(Page.public_language_roots.size).to eq(1)
+        expect(Page.public_language_roots.to_a).to eq([public_language_root])
       end
     end
 
     describe ".restricted" do
-      it "should return 1 restricted page" do
-        create(:alchemy_page, :public, name: "First Public Child", restricted: true, parent: language_root, language: language)
-        expect(Page.restricted.size).to eq(1)
+      let!(:restricted) { create(:alchemy_page, :restricted) }
+
+      it "should return restricted pages" do
+        expect(Page.restricted.to_a).to eq([restricted])
       end
     end
 
     describe ".visible" do
-      it "should return 1 visible page" do
-        create(:alchemy_page, :public, name: "First Public Child", visible: true, parent: language_root, language: language)
-        expect(Page.visible.size).to eq(1)
+      let!(:visible) { create(:alchemy_page, :public, visible: true) }
+
+      it "should return visible pages" do
+        expect(Page.visible.to_a).to eq([visible])
       end
     end
 

spec/requests/alchemy/admin/pages_controller_spec.rb

@@ -195,15 +195,13 @@ module Alchemy
         end
 
         let(:layout_page_1) do
-          create :alchemy_page,
-            layoutpage: true,
+          create :alchemy_page, :layoutpage,
             name: "layout_page 1",
             published_at: Time.current - 5.days
         end
 
         let(:layout_page_2) do
-          create :alchemy_page,
-            layoutpage: true,
+          create :alchemy_page, :layoutpage,
             name: "layout_page 2",
             published_at: Time.current - 8.days
         end