Merge pull request #2862 from mamhoff/use-updated-at-timestamp-as-cac…

…he-key

Use page version's `updated_at` timestamp as cache key

app/controllers/alchemy/pages_controller.rb

@@ -216,7 +216,7 @@ def page_etag
     def render_fresh_page?
       must_not_cache? || stale?(
         etag: page_etag,
-        last_modified: @page.published_at,
+        last_modified: @page.last_modified_at,
         public: !@page.restricted,
         template: "pages/show"
       )

app/models/alchemy/page/page_natures.rb

@@ -101,18 +101,22 @@ def layout_partial_name
         page_layout.parameterize.underscore
       end
 
-      # Returns the version that's taken for Rails' recycable cache key.
+      # Returns the version string that's taken for Rails' recycable cache key.
       #
-      # Uses the +published_at+ value that's updated when the user publishes the page.
+      def cache_version
+        last_modified_at&.to_s
+      end
+
+      # Returns the timestamp that the page was last modified at, regardless of through
+      # publishing or editing page, or through a change of related objects through ingredients.
+      # Respects the public version not changing if editing a preview.
       #
-      # If the page is the current preview it uses the +updated_at+ value as cache key.
+      # In preview mode, it will take the draft version's updated_at timestamp.
+      # In public mode, it will take the public version's updated_at timestamp.
       #
-      def cache_version
-        if Current.preview_page == self
-          updated_at.to_s
-        else
-          published_at.to_s
-        end
+      def last_modified_at
+        relevant_page_version = (Current.preview_page == self) ? draft_version : public_version
+        relevant_page_version&.updated_at
       end
 
       # Returns true if the page cache control headers should be set.

app/models/alchemy/page/publisher.rb

@@ -35,6 +35,7 @@ def publish!(public_on:)
                 end
               end
             end
+            version.update(updated_at: public_on)
             page.update(published_at: public_on)
           end
         end

spec/models/alchemy/page/publisher_spec.rb

@@ -55,8 +55,9 @@
     end
 
     context "with published version existing" do
+      let(:yesterday) { Date.yesterday.to_time }
       let!(:public_version) do
-        create(:alchemy_page_version, :with_elements, element_count: 3, public_on: Date.yesterday.to_time, page: page)
+        create(:alchemy_page_version, :with_elements, element_count: 3, public_on: yesterday, page: page)
       end
 
       let!(:nested_element) do
@@ -67,6 +68,12 @@
         expect { publish }.to_not change(page.public_version, :public_on)
       end
 
+      it "updates public version's updated_at timestamp" do
+        # Need to do this here, because the nested element touches the version on creation.
+        public_version.update_columns(updated_at: yesterday)
+        expect { publish }.to change(page.public_version, :updated_at)
+      end
+
       it "does not create another public version" do
         expect { publish }.to_not change(page.versions, :count)
       end

spec/models/alchemy/page_spec.rb

@@ -704,14 +704,48 @@ module Alchemy
     end
 
     describe "#cache_version" do
+      let(:page) { build(:alchemy_page) }
+
+      around do |example|
+        travel_to(Time.parse("2019-01-01 12:00:00 UTC")) do
+          example.run
+        end
+      end
+
+      context "last modified is a time object" do
+        before do
+          allow(page).to receive(:last_modified_at).and_return(1.day.ago)
+        end
+
+        it "returns a cache version string" do
+          expect(page.cache_version).to eq("2018-12-31 12:00:00 UTC")
+        end
+      end
+
+      context "last modified at is nil" do
+        before do
+          allow(page).to receive(:last_modified_at).and_return(nil)
+        end
+
+        it "returns a cache version string" do
+          expect(page.cache_version).to be(nil)
+        end
+      end
+    end
+
+    describe "#last_modified_at" do
       let(:now) { Time.current }
+      let(:yesterday) { Time.current - 1.day }
       let(:last_week) { Time.current - 1.week }
 
       let(:page) do
-        build_stubbed(:alchemy_page, updated_at: now, published_at: last_week)
+        build_stubbed(:alchemy_page, public_version: public_version, draft_version: draft_version, updated_at: yesterday)
       end
 
-      subject { page.cache_version }
+      let(:public_version) { build_stubbed(:alchemy_page_version, updated_at: last_week) }
+      let(:draft_version) { build_stubbed(:alchemy_page_version, updated_at: now) }
+
+      subject { page.last_modified_at }
 
       before do
         expect(Current).to receive(:preview_page).and_return(preview)
@@ -720,16 +754,44 @@ module Alchemy
       context "when current page rendered in preview mode" do
         let(:preview) { page }
 
-        it "uses updated_at" do
-          is_expected.to eq(now.to_s)
+        it "uses draft version's updated_at" do
+          is_expected.to be_within(1.second).of(now)
         end
       end
 
       context "when current page not in preview mode" do
         let(:preview) { nil }
 
-        it "uses published_at" do
-          is_expected.to eq(last_week.to_s)
+        it "uses public version's updated at" do
+          is_expected.to be_within(1.second).of(last_week)
+        end
+      end
+
+      context "if page has no public version" do
+        let(:public_version) { nil }
+
+        context "in preview mode" do
+          let(:preview) { page }
+
+          it "uses draft versions updated_at" do
+            is_expected.to be_within(1.second).of(now)
+          end
+
+          context "if page has no draft version" do
+            let(:draft_version) { nil }
+
+            it "is nil" do
+              is_expected.to be(nil)
+            end
+          end
+        end
+
+        context "not in preview mode" do
+          let(:preview) { nil }
+
+          it "is nil" do
+            is_expected.to be(nil)
+          end
         end
       end
     end

spec/requests/alchemy/page_request_caching_spec.rb

@@ -3,8 +3,7 @@
 require "rails_helper"
 
 RSpec.describe "Page request caching" do
-  let!(:page) { create(:alchemy_page, :public, published_at: published_at) }
-  let(:published_at) { nil }
+  let!(:page) { create(:alchemy_page, :public) }
 
   context "when caching is disabled in app" do
     before do
@@ -81,18 +80,17 @@
         expect(response.headers).to have_key("ETag")
       end
 
-      it "does not set last-modified header" do
-        get "/#{page.urlname}"
-        expect(response.headers).to_not have_key("Last-Modified")
-      end
+      context "and public version is present" do
+        let(:jan_first) { Time.new(2020, 1, 1) }
 
-      context "and published_at is set" do
-        let(:published_at) { DateTime.yesterday }
+        before do
+          allow_any_instance_of(Alchemy::Page).to receive(:last_modified_at) { jan_first }
+        end
 
         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)
+          expect(response.headers["Last-Modified"]).to eq(jan_first.httpdate)
         end
       end
     end