Skip to content

Commit

Permalink
Merge pull request #2862 from mamhoff/use-updated-at-timestamp-as-cac…
Browse files Browse the repository at this point in the history
…he-key

Use page version's `updated_at` timestamp as cache key
  • Loading branch information
tvdeyen authored May 15, 2024
2 parents bad3e92 + 6787e96 commit 6af974a
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 26 deletions.
2 changes: 1 addition & 1 deletion app/controllers/alchemy/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down
22 changes: 13 additions & 9 deletions app/models/alchemy/page/page_natures.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions app/models/alchemy/page/publisher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion spec/models/alchemy/page/publisher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
74 changes: 68 additions & 6 deletions spec/models/alchemy/page_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
16 changes: 7 additions & 9 deletions spec/requests/alchemy/page_request_caching_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 6af974a

Please sign in to comment.