From a2089e89b267829596c87d384a58dc797161a8ca Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 19 Aug 2021 11:12:43 +0200 Subject: [PATCH 1/4] Remove Page#published_at overwrite We need the value from the database in all cases in order to be able to use it as the last_modified header. The overwrite is not necessary for the cache_key (ETag header) since it already takes the use case for admins into account. --- app/models/alchemy/page/page_natures.rb | 9 --------- spec/models/alchemy/page_spec.rb | 4 +--- .../alchemy/page_request_caching_spec.rb | 18 ++++++++++++++---- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/app/models/alchemy/page/page_natures.rb b/app/models/alchemy/page/page_natures.rb index b9defecd36..f43aad9316 100644 --- a/app/models/alchemy/page/page_natures.rb +++ b/app/models/alchemy/page/page_natures.rb @@ -117,15 +117,6 @@ def cache_key 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 diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index ad907047ec..d318a7cd43 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -1871,9 +1871,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 diff --git a/spec/requests/alchemy/page_request_caching_spec.rb b/spec/requests/alchemy/page_request_caching_spec.rb index 7fb54e4761..c0994c69a5 100644 --- a/spec/requests/alchemy/page_request_caching_spec.rb +++ b/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 From 3c5726cb029864970f247667311e28184dbad6ec Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 19 Aug 2021 12:12:33 +0200 Subject: [PATCH 2/4] Store current page id as preview Instead of storing the class instance in the request store we store the id. That way we are able to set a page as the current preview from another request (ie. a page fetched from an API) --- app/models/alchemy/page.rb | 6 ++-- app/models/alchemy/page/page_natures.rb | 2 +- spec/models/alchemy/page_spec.rb | 40 ++++++++++++++++++++++--- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/app/models/alchemy/page.rb b/app/models/alchemy/page.rb index 09d7b1df2b..cf02d735d0 100644 --- a/app/models/alchemy/page.rb +++ b/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] diff --git a/app/models/alchemy/page/page_natures.rb b/app/models/alchemy/page/page_natures.rb index f43aad9316..b8809b9396 100644 --- a/app/models/alchemy/page/page_natures.rb +++ b/app/models/alchemy/page/page_natures.rb @@ -110,7 +110,7 @@ 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}" diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index d318a7cd43..e9e67910db 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/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 From 6f028252a484a80d5a2befffdf5fa54191168ea0 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 19 Aug 2021 12:13:23 +0200 Subject: [PATCH 3/4] Remove the element cache_key override Now that elements get copied over to a new page version on publish the cache key can be the default rails cache key again. --- app/models/alchemy/element.rb | 14 -------------- spec/models/alchemy/element_spec.rb | 23 ----------------------- 2 files changed, 37 deletions(-) diff --git a/app/models/alchemy/element.rb b/app/models/alchemy/element.rb index 4a87cb1c5b..04f74a099d 100644 --- a/app/models/alchemy/element.rb +++ b/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", []) diff --git a/spec/models/alchemy/element_spec.rb b/spec/models/alchemy/element_spec.rb index 0b99ed1541..14ba5bbb3c 100644 --- a/spec/models/alchemy/element_spec.rb +++ b/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 From 329d257f1a4b795027cd0f07fb0d24ed288bb7de Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 19 Aug 2021 12:15:46 +0200 Subject: [PATCH 4/4] Touch page if page version gets updated Crucial for page caching --- app/models/alchemy/page_version.rb | 2 +- spec/models/alchemy/page_version_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/alchemy/page_version.rb b/app/models/alchemy/page_version.rb index a565bdec4a..e17c5cf10a 100644 --- a/app/models/alchemy/page_version.rb +++ b/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", diff --git a/spec/models/alchemy/page_version_spec.rb b/spec/models/alchemy/page_version_spec.rb index 9a71364f45..53f27f3ce4 100644 --- a/spec/models/alchemy/page_version_spec.rb +++ b/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? }