Merge pull request #2179 from tvdeyen/remove-published_at-override

Fixes caching

app/models/alchemy/element.rb

@@ -267,20 +267,6 @@ def to_partial_path
       "alchemy/elements/#{name}"
     end
 
-    # Returns the key that's taken for cache path.
-    #
-    # Uses the page's +published_at+ value that's updated when the user publishes the page.
-    #
-    # If the page is the current preview it uses the element's updated_at value as cache key.
-    #
-    def cache_key
-      if Page.current_preview == page
-        "alchemy/elements/#{id}-#{updated_at}"
-      else
-        "alchemy/elements/#{id}-#{page.published_at}"
-      end
-    end
-
     # A collection of element names that can be nested inside this element.
     def nestable_elements
       definition.fetch("nestable_elements", [])

app/models/alchemy/page.rb

@@ -198,13 +198,13 @@ def searchable_alchemy_resource_attributes
         %w[name urlname title]
       end
 
-      # Used to store the current page previewed in the edit page template.
+      # Used to store the current page id previewed in the edit page template.
       #
       def current_preview=(page)
-        RequestStore.store[:alchemy_current_preview] = page
+        RequestStore.store[:alchemy_current_preview] = page&.id
       end
 
-      # Returns the current page previewed in the edit page template.
+      # Returns the current page id previewed in the edit page template.
       #
       def current_preview
         RequestStore.store[:alchemy_current_preview]

app/models/alchemy/page/page_natures.rb

@@ -110,22 +110,13 @@ def layout_partial_name
       # If the page is the current preview it uses the updated_at value as cache key.
       #
       def cache_key
-        if Page.current_preview == self
+        if Page.current_preview == id
           "alchemy/pages/#{id}-#{updated_at}"
         else
           "alchemy/pages/#{id}-#{published_at}"
         end
       end
 
-      # We use the published_at value for the cache_key.
-      #
-      # If no published_at value is set yet, i.e. because it was never published,
-      # we return the updated_at value.
-      #
-      def published_at
-        read_attribute(:published_at) || updated_at
-      end
-
       # Returns true if the page cache control headers should be set.
       #
       # == Disable Alchemy's page caching globally

app/models/alchemy/page_version.rb

@@ -2,7 +2,7 @@
 
 module Alchemy
   class PageVersion < BaseRecord
-    belongs_to :page, class_name: "Alchemy::Page", inverse_of: :versions
+    belongs_to :page, class_name: "Alchemy::Page", inverse_of: :versions, touch: true
 
     has_many :elements, -> { order(:position) },
       class_name: "Alchemy::Element",

spec/models/alchemy/element_spec.rb

@@ -937,29 +937,6 @@ module Alchemy
       end
     end
 
-    describe "#cache_key" do
-      let(:page) { create(:alchemy_page, published_at: Time.current - 1.week) }
-      let(:element) { create(:alchemy_element, page_version: page.draft_version, updated_at: Time.current) }
-
-      subject { element.cache_key }
-
-      before do
-        expect(Page).to receive(:current_preview).and_return(preview)
-      end
-
-      context "when current page rendered in preview mode" do
-        let(:preview) { page }
-
-        it { is_expected.to eq("alchemy/elements/#{element.id}-#{element.updated_at}") }
-      end
-
-      context "when current page not in preview mode" do
-        let(:preview) { nil }
-
-        it { is_expected.to eq("alchemy/elements/#{element.id}-#{page.published_at}") }
-      end
-    end
-
     it_behaves_like "having a hint" do
       let(:subject) { Element.new }
     end

spec/models/alchemy/page_spec.rb

@@ -604,6 +604,31 @@ class AnotherUrlPathClass; end
       end
     end
 
+    describe ".current_preview=" do
+      let(:page) { create(:alchemy_page) }
+
+      it "stores page id in request store" do
+        described_class.current_preview = page
+        expect(RequestStore.store[:alchemy_current_preview]).to eq(page.id)
+      end
+
+      context "with page being nil" do
+        it "removes page id from request store" do
+          described_class.current_preview = nil
+          expect(RequestStore.store[:alchemy_current_preview]).to be_nil
+        end
+      end
+    end
+
+    describe ".current_preview" do
+      let(:page) { create(:alchemy_page) }
+
+      it "returns page id from request store" do
+        described_class.current_preview = page
+        expect(described_class.current_preview).to eq(page.id)
+      end
+    end
+
     # InstanceMethods (a-z)
 
     describe "#available_element_definitions" do
@@ -703,8 +728,11 @@ class AnotherUrlPathClass; end
     end
 
     describe "#cache_key" do
+      let(:now) { Time.current }
+      let(:last_week) { Time.current - 1.week }
+
       let(:page) do
-        stub_model(Page, updated_at: Time.current, published_at: Time.current - 1.week)
+        build_stubbed(:alchemy_page, updated_at: now, published_at: last_week)
       end
 
       subject { page.cache_key }
@@ -714,15 +742,19 @@ class AnotherUrlPathClass; end
       end
 
       context "when current page rendered in preview mode" do
-        let(:preview) { page }
+        let(:preview) { page.id }
 
-        it { is_expected.to eq("alchemy/pages/#{page.id}-#{page.updated_at}") }
+        it "uses updated_at" do
+          is_expected.to eq("alchemy/pages/#{page.id}-#{now}")
+        end
       end
 
       context "when current page not in preview mode" do
         let(:preview) { nil }
 
-        it { is_expected.to eq("alchemy/pages/#{page.id}-#{page.published_at}") }
+        it "uses published_at" do
+          is_expected.to eq("alchemy/pages/#{page.id}-#{last_week}")
+        end
       end
     end
 
@@ -1871,9 +1903,7 @@ class AnotherUrlPathClass; end
         let(:updated_at) { 3.days.ago }
         let(:page) { build_stubbed(:alchemy_page, published_at: nil, updated_at: updated_at) }
 
-        it "returns the updated_at value" do
-          expect(page.published_at).to be_within(1.second).of(updated_at)
-        end
+        it { expect(page.published_at).to be_nil }
       end
     end
 

spec/models/alchemy/page_version_spec.rb

@@ -70,6 +70,16 @@
     end
   end
 
+  describe "when saved" do
+    let(:page_version) { build(:alchemy_page_version) }
+    let(:page) { page_version.page }
+
+    it "touches the page" do
+      expect(page).to receive(:touch)
+      page_version.save
+    end
+  end
+
   describe "#public?" do
     subject { page_version.public? }
 

spec/requests/alchemy/page_request_caching_spec.rb

@@ -3,7 +3,8 @@
 require "rails_helper"
 
 RSpec.describe "Page request caching" do
-  let!(:page) { create(:alchemy_page, :public) }
+  let!(:page) { create(:alchemy_page, :public, published_at: published_at) }
+  let(:published_at) { nil }
 
   context "when caching is disabled in app" do
     before do
@@ -80,10 +81,19 @@
         expect(response.headers).to have_key("ETag")
       end
 
-      it "sets last-modified header" do
+      it "does not set last-modified header" do
         get "/#{page.urlname}"
-        expect(response.headers).to have_key("Last-Modified")
-        expect(response.headers["Last-Modified"]).to eq(page.published_at.httpdate)
+        expect(response.headers).to_not have_key("Last-Modified")
+      end
+
+      context "and published_at is set" do
+        let(:published_at) { DateTime.yesterday }
+
+        it "sets last-modified header" do
+          get "/#{page.urlname}"
+          expect(response.headers).to have_key("Last-Modified")
+          expect(response.headers["Last-Modified"]).to eq(published_at.httpdate)
+        end
       end
     end