Merge pull request #3594 from AlchemyCMS/page-etag-with-scheduled-elements

Include published elements in page ETag for scheduled content

Author: tvdeyen

app/controllers/alchemy/pages_controller.rb

@@ -215,7 +215,13 @@ def signup_required?
 
     # Returns the etag used for response headers.
     #
-    # If a user is logged in, we append theirs etag to prevent caching of user related content.
+    # The etag is composed of:
+    # - The page's cache key (includes updated_at timestamp)
+    # - Published element IDs (changes when elements enter/leave the published scope)
+    # - The current user's cache key (for user-specific content)
+    #
+    # This ensures HTTP caches invalidate when scheduled elements become visible
+    # or hidden, even though the page's updated_at hasn't changed.
     #
     # IMPORTANT:
     #
@@ -224,7 +230,8 @@ def signup_required?
     # Otherwise all users will see the same cached page, regardless of user's state.
     #
     def page_etag
-      [@page, current_alchemy_user]
+      elements_cache_key = @page.public_version&.elements&.published&.order(:id)&.pluck(:id)
+      [@page, elements_cache_key, current_alchemy_user]
     end
 
     # We only render the page if either the cache is disabled for this page
@@ -233,7 +240,6 @@ def page_etag
     def render_fresh_page?
       must_not_cache? || stale?(
         etag: page_etag,
-        last_modified: @page.last_modified_at,
         public: !@page.restricted,
         template: "pages/show"
       )

spec/controllers/alchemy/pages_controller_spec.rb

@@ -246,6 +246,11 @@ module Alchemy
           expect(subject).to include(page)
         end
 
+        it "includes published elements cache key" do
+          expected = page.public_version.elements.published.order(:id).pluck(:id)
+          expect(subject[1]).to eq(expected)
+        end
+
         context "with user logged in" do
           before do
             authorize_user(mock_model(Alchemy.user_class, cache_key_with_version: "bbb"))
@@ -255,6 +260,41 @@ module Alchemy
             expect(subject).to include(an_instance_of(Alchemy.user_class))
           end
         end
+
+        context "when element becomes published" do
+          let!(:scheduled_element) do
+            create(:alchemy_element, page_version: page.public_version, public_on: 1.hour.from_now)
+          end
+
+          it "changes the etag when element becomes visible" do
+            etag_before = subject[1]
+            travel 2.hours do
+              etag_after = controller.send(:page_etag)[1]
+              expect(etag_after).not_to eq(etag_before)
+            end
+          end
+        end
+
+        context "when one element replaces another" do
+          let!(:default_header) do
+            create(:alchemy_element, page_version: page.public_version, public_on: 1.day.ago, public_until: 1.hour.from_now)
+          end
+
+          let!(:seasonal_header) do
+            create(:alchemy_element, page_version: page.public_version, public_on: 1.hour.from_now)
+          end
+
+          it "changes the etag even when published element count stays the same" do
+            etag_before = subject[1]
+            expect(etag_before.length).to eq(2)
+
+            travel 2.hours do
+              etag_after = controller.send(:page_etag)[1]
+              expect(etag_after.length).to eq(2)
+              expect(etag_after).not_to eq(etag_before)
+            end
+          end
+        end
       end
     end
   end

spec/requests/alchemy/page_request_caching_spec.rb

@@ -97,19 +97,37 @@
         expect(response.headers).to have_key("ETag")
       end
 
-      context "and public version is present" do
-        let(:jan_first) { Time.new(2020, 1, 1) }
+      context "with scheduled element becoming visible" do
+        let!(:scheduled_element) do
+          create(:alchemy_element, page_version: page.public_version, public_on: 1.hour.from_now)
+        end
 
-        before do
-          allow_any_instance_of(Alchemy::Page).to receive(:last_modified_at) { jan_first }
+        it "changes the etag when element becomes visible" do
+          get "/#{page.urlname}"
+          etag_before = response.headers["ETag"]
+
+          travel 2.hours do
+            get "/#{page.urlname}"
+            etag_after = response.headers["ETag"]
+            expect(etag_after).not_to eq(etag_before)
+          end
         end
 
-        it "sets last-modified header" do
+        it "returns 200 instead of 304 after element becomes visible" do
           get "/#{page.urlname}"
-          expect(response.headers).to have_key("Last-Modified")
-          expect(response.headers["Last-Modified"]).to eq(jan_first.httpdate)
+          etag = response.headers["ETag"]
+
+          travel 2.hours do
+            get "/#{page.urlname}", headers: {"If-None-Match" => etag}
+            expect(response.status).to eq(200)
+          end
         end
       end
+
+      it "does not set last-modified header" do
+        get "/#{page.urlname}"
+        expect(response.headers).to_not have_key("Last-Modified")
+      end
     end
 
     context "but page should not be cached" do