Merge pull request AlchemyCMS#2313 from mamhoff/do-not-eagerload-in-p…

…age-version

Eagerload at the controller or job layer

app/controllers/alchemy/admin/pages_controller.rb

@@ -324,7 +324,7 @@ def process_url(ancestors_path, item)
       end
 
       def load_resource
-        @page = Page.find(params[:id])
+        @page = Page.includes(page_includes).find(params[:id])
       end
 
       def pages_from_raw_request
@@ -402,6 +402,10 @@ def load_languages_and_layouts
       def set_preview_mode
         @preview_mode = true
       end
+
+      def page_includes
+        Alchemy::EagerLoading.page_includes(version: :draft_version)
+      end
     end
   end
 end

app/controllers/alchemy/pages_controller.rb

@@ -104,7 +104,14 @@ def locale_prefix_not_allowed?
     # If no index page and no admin users are present we show the "Welcome to Alchemy" page.
     #
     def load_index_page
-      @page ||= Language.current_root_page
+      @page ||= begin
+        Alchemy::Page.
+          contentpages.
+          language_roots.
+          where(language: Language.current).
+          includes(page_includes).
+          first
+      end
       render template: "alchemy/welcome", layout: false if signup_required?
     end
 
@@ -120,10 +127,13 @@ def load_index_page
     def load_page
       page_not_found! unless Language.current
 
-      @page ||= Language.current.pages.contentpages.find_by(
-        urlname: params[:urlname],
-        language_code: params[:locale] || Language.current.code,
-      )
+      @page ||= begin
+        Alchemy::Page.
+          contentpages.
+          where(language: Language.current).
+          includes(page_includes).
+          find_by(urlname: params[:urlname])
+      end
     end
 
     def enforce_locale
@@ -234,5 +244,9 @@ def must_not_cache?
     def page_not_found!
       not_found_error!("Alchemy::Page not found \"#{request.fullpath}\"")
     end
+
+    def page_includes
+      Alchemy::EagerLoading.page_includes(version: :public_version)
+    end
   end
 end

app/jobs/alchemy/publish_page_job.rb

@@ -4,7 +4,10 @@ module Alchemy
   class PublishPageJob < BaseJob
     queue_as :default
 
-    def perform(page, public_on:)
+    def perform(page_id, public_on:)
+      page = Alchemy::Page.includes(
+        Alchemy::EagerLoading.page_includes(version: :draft_version)
+      ).find(page_id)
       Alchemy::Page::Publisher.new(page).publish!(public_on: public_on)
     end
   end

app/models/alchemy/eager_loading.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+module Alchemy
+  # Eager loading parameters for loading pages
+  class EagerLoading
+    PAGE_VERSIONS = %i[draft_version public_version]
+
+    class << self
+      # Eager loading parameters for {ActiveRecord::Base.includes}
+      #
+      # Pass this to +includes+ whereever you load an {Alchemy::Page}
+      #
+      #     Alchemy::Page.includes(Alchemy::EagerLoading.page_includes).find_by(urlname: "my-page")
+      #
+      # @param version [Symbol] Type of page version to eager load
+      # @return [Array]
+      def page_includes(version: :public_version)
+        raise UnsupportedPageVersion unless version.in? PAGE_VERSIONS
+
+        [
+          :tags,
+          {
+            language: :site,
+            version => {
+              elements: [
+                :page,
+                :touchable_pages,
+                {
+                  ingredients: :related_object,
+                  contents: :essence,
+                },
+              ],
+            },
+          },
+        ]
+      end
+    end
+  end
+end

app/models/alchemy/page.rb

@@ -465,7 +465,7 @@ def copy_children_to(new_parent)
     #
     def publish!(current_time = Time.current)
       update(published_at: current_time)
-      PublishPageJob.perform_later(self, public_on: current_time)
+      PublishPageJob.perform_later(id, public_on: current_time)
     end
 
     # Sets the public_on date on the published version

app/models/alchemy/page_version.rb

@@ -46,7 +46,7 @@ def still_public_for?(time = Time.current)
     end
 
     def element_repository
-      ElementsRepository.new(elements.includes({ contents: :essence }, :tags))
+      ElementsRepository.new(elements)
     end
 
     private

lib/alchemy/errors.rb

@@ -11,15 +11,15 @@ class DefaultLanguageNotFoundError < StandardError
     # Raised if no default language configuration can be found.
     def message
       "No default language configuration found!" \
-        " Please ensure that you have a 'default_language' defined in Alchemy configuration file."
+      " Please ensure that you have a 'default_language' defined in Alchemy configuration file."
     end
   end
 
   class DefaultSiteNotFoundError < StandardError
     # Raised if no default site configuration can be found.
     def message
       "No default site configuration found!" \
-        " Please ensure that you have a 'default_site' defined in Alchemy configuration file."
+      " Please ensure that you have a 'default_site' defined in Alchemy configuration file."
     end
   end
 
@@ -90,4 +90,10 @@ def message
       "You need to provide a current_user method in your ApplicationController that returns the current authenticated user."
     end
   end
+
+  class UnsupportedPageVersion < StandardError
+    def message
+      "Unknown Version! Please use one of #{Alchemy::EagerLoading::PAGE_VERSIONS.join(", ")}"
+    end
+  end
 end

spec/jobs/alchemy/publish_page_job_spec.rb

@@ -4,20 +4,20 @@
 
 RSpec.describe Alchemy::PublishPageJob, type: :job do
   describe "#perfom_later" do
-    let(:page) { build_stubbed(:alchemy_page) }
+    let(:page) { create(:alchemy_page) }
     let(:public_on) { Time.current }
 
     it "enqueues job" do
       expect {
-        described_class.perform_later(page, public_on: public_on)
+        described_class.perform_later(page.id, public_on: public_on)
       }.to have_enqueued_job
     end
 
     it "calls the page publisher" do
       expect_any_instance_of(Alchemy::Page::Publisher).to receive(:publish!).with(
         public_on: public_on,
       )
-      described_class.new.perform(page, public_on: public_on)
+      described_class.new.perform(page.id, public_on: public_on)
     end
   end
 end

spec/models/alchemy/eager_loading_spec.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe Alchemy::EagerLoading do
+  describe ".page_includes" do
+    context "with no version param given" do
+      subject { described_class.page_includes }
+
+      it "returns public version includes" do
+        is_expected.to match_array([
+          :tags,
+          {
+            language: :site,
+            public_version: {
+              elements: [
+                :page,
+                :touchable_pages,
+                {
+                  ingredients: :related_object,
+                  contents: :essence,
+                },
+              ],
+            },
+          },
+        ])
+      end
+    end
+
+    context "with version param given" do
+      subject { described_class.page_includes(version: :draft_version) }
+
+      it "returns version includes" do
+        is_expected.to match_array([
+          :tags,
+          {
+            language: :site,
+            draft_version: {
+              elements: [
+                :page,
+                :touchable_pages,
+                {
+                  ingredients: :related_object,
+                  contents: :essence,
+                },
+              ],
+            },
+          },
+        ])
+      end
+    end
+
+    context "with unknown version param given" do
+      subject { described_class.page_includes(version: :foo_baz) }
+
+      it "returns version includes" do
+        expect { subject }.to raise_error(Alchemy::UnsupportedPageVersion)
+      end
+    end
+  end
+end

spec/requests/alchemy/admin/pages_controller_spec.rb

@@ -643,45 +643,44 @@ module Alchemy
 
       describe "#fold" do
         let(:page) { create(:alchemy_page) }
+        let(:user) { create(:alchemy_dummy_user, :as_editor) }
 
         before do
-          allow(Page).to receive(:find).and_return(page)
-          allow(page).to receive(:editable_by?).with(user).and_return(true)
+          allow_any_instance_of(described_class).to receive(:current_alchemy_user) { user }
         end
 
-        context "if page is currently not folded" do
-          before { allow(page).to receive(:folded?).and_return(false) }
+        subject { patch fold_admin_page_path(page), xhr: true }
 
+        context "if page is currently not folded" do
           it "should fold the page" do
-            expect(page).to receive(:fold!).with(user.id, true).and_return(true)
-            patch fold_admin_page_path(page), xhr: true
+            expect { subject }.to change { page.folded?(user.id) }.from(false).to(true)
           end
         end
 
         context "if page is already folded" do
-          before { allow(page).to receive(:folded?).and_return(true) }
+          before do
+            page.fold!(user.id, true)
+          end
 
           it "should unfold the page" do
-            expect(page).to receive(:fold!).with(user.id, false).and_return(true)
-            patch fold_admin_page_path(page), xhr: true
+            expect { subject }.to change { page.folded?(user.id) }.from(true).to(false)
           end
         end
       end
 
       describe "#unlock" do
-        subject { post unlock_admin_page_path(page), xhr: true }
+        let(:page) { create(:alchemy_page) }
+        let(:user) { create(:alchemy_dummy_user, :as_editor) }
 
-        let(:page) { create(:alchemy_page, name: "Best practices") }
+        subject { post unlock_admin_page_path(page), xhr: true }
 
         before do
-          allow(Page).to receive(:find).with(page.id.to_s).and_return(page)
-          allow(page).to receive(:editable_by?).with(user).and_return(true)
-          allow(Page).to receive(:from_current_site).and_return(double(locked_by: nil))
-          expect(page).to receive(:unlock!) { true }
+          page.lock_to!(user)
+          allow_any_instance_of(described_class).to receive(:current_alchemy_user) { user }
         end
 
         it "should unlock the page" do
-          is_expected.to eq(200)
+          expect { subject }.to change { page.reload.locked? }.from(true).to(false)
         end
 
         context "requesting for html format" do