Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes caching #2179

Merged
merged 4 commits into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions app/models/alchemy/element.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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", [])
Expand Down
6 changes: 3 additions & 3 deletions app/models/alchemy/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
11 changes: 1 addition & 10 deletions app/models/alchemy/page/page_natures.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/alchemy/page_version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
23 changes: 0 additions & 23 deletions spec/models/alchemy/element_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
44 changes: 37 additions & 7 deletions spec/models/alchemy/page_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }
Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down
10 changes: 10 additions & 0 deletions spec/models/alchemy/page_version_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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? }

Expand Down
18 changes: 14 additions & 4 deletions spec/requests/alchemy/page_request_caching_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down