Merge pull request #342 from joyofrails/fix/page-caching

rossta · web-flow · commit c54dd858aa35 · 2025-01-16T17:36:38.000-05:00
Fix for page caching
diff --git a/app/content/pages/about/design.html.mdrb b/app/content/pages/about/design.html.mdrb
@@ -7,7 +7,7 @@ I am [not a designer](https://notadesigner.io/). Though the design of this site
 
 ## Layout
 
-The design leans heavily into [CSS grid](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout/Basic_concepts_of_grid_layout) I’m a relatively new student of CSS grid so I’m sure the approach could be improved. I built a 12 column layout from using https://utopia.fyi to generate based grid, layout, and type settings as CSS variables. Utopia allows designers and developers to generate a mathematically consistent
+The design leans heavily into [CSS grid](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout/Basic_concepts_of_grid_layout) I’m a relatively new student of CSS grid so I’m sure the approach could be improved. I built a 12 column layout using https://utopia.fyi to generate mathematically consistent grid, layout, and type settings as CSS variables.
 
 ## Colors
 
@@ -25,6 +25,6 @@ I want to Joy of Rails to "own more" of its dependencies—part of why I impleme
 
 When I was in grade school, I doodled a lot. Drawing brought me a lot of joy. After years of working at a computer, I got away from it. But more recently, especially now that I have kids of my own, I’ve had the itch to return to my roots.
 
-For Joy of Rails, I wanted to add some fun visuals to the in-depth articles. So, in this age of AI-generated imagery, I decided I would do the opposite of what everyone else seems to be doing—draw everything by hand. You‘ll pictures of rainbows, ice cream cones, and smiley faces on Joy of Rails as a reminder of the joy coding brings to my life.
+For Joy of Rails, I wanted to add some fun visuals to the in-depth articles. So, in this age of AI-generated imagery, I decided I would do the opposite of what everyone else seems to be doing—draw everything by hand. Pictures of rainbows, ice cream cones, and smiley faces on Joy of Rails serve as a reminder of the joy building and creating brings to my life.
 
 Most of the illustrations are done in [Procreate](https://procreate.com/) on my iPad with an Apple Pencil. Procreate is a great little app I highly recommend for anyone looking to rekindle their childhood doodling habit.
diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb
@@ -39,7 +39,9 @@ def process_rendition(rendition)
   # @param rendition [Sitepress::Rendition] Rendered representatio of current_resource
   #
   def post_render(rendition)
-    if skip_http_cache? || stale?(rendition.source, last_modified: current_resource.asset.updated_at.utc, public: true)
+    last_modified_at = @current_page&.upserted_at || @current_page&.created_at || current_resource.asset.updated_at || Time.now
+
+    if skip_http_cache? || stale?(rendition.source, last_modified: last_modified_at.utc, public: true)
       render body: rendition.output, content_type: rendition.mime_type
     end
   end
diff --git a/app/models/page.rb b/app/models/page.rb
@@ -9,6 +9,8 @@
 #  indexed_at   :datetime
 #  published_at :datetime
 #  request_path :string           not null
+#  revised_at   :datetime
+#  upserted_at  :datetime
 #  created_at   :datetime         not null
 #  updated_at   :datetime         not null
 #
diff --git a/app/models/page/sitepressed.rb b/app/models/page/sitepressed.rb
@@ -80,14 +80,14 @@ def resource_missing? = sitepress_resource.is_a?(NullSitepressResource)
     def handler = sitepress_resource.handler
 
     class_methods do
-      # We currently have a dual system of content management between Sitepress and
-      # Page models for handling static pages While not ideal, it currently allows
-      # us to live in both worlds depending on the context.  Ultimately, migrating
-      # away from Sitepress for indexed content may be what‘s needed, but keeping
-      # the split personality for now.
+      # Joy of Rails has logic to represent static pages split between Sitepress
+      # and Page models. While not ideal, it currently allows us to live in both
+      # worlds depending on the context.  Ultimately, migrating away from
+      # Sitepress for indexed content may be what‘s needed.
       #
       def upsert_collection_from_sitepress!(limit: nil)
         enum = SitepressPage.all.resources.lazy.map { |s| Resource.from(s) }
+        upserted_at = Time.zone.now
 
         if limit
           enum = enum.filter do |resource|
@@ -96,7 +96,7 @@ def upsert_collection_from_sitepress!(limit: nil)
         end
 
         enum = enum.map do |resource|
-          upsert_page_from_resource!(resource)
+          upsert_page_from_resource!(resource, upserted_at: upserted_at)
         end
 
         if limit
@@ -106,14 +106,15 @@ def upsert_collection_from_sitepress!(limit: nil)
         enum.to_a
       end
 
-      def upsert_page_by_request_path!(request_path) = upsert_page_from_sitepress!(Sitepress.site.get(request_path))
+      def upsert_page_by_request_path!(request_path, **opts) = upsert_page_from_sitepress!(Sitepress.site.get(request_path), **opts)
 
-      def upsert_page_from_sitepress!(sitepress_resource) = upsert_page_from_resource! Resource.from(sitepress_resource)
+      def upsert_page_from_sitepress!(sitepress_resource, **opts) = upsert_page_from_resource!(Resource.from(sitepress_resource), **opts)
 
-      def upsert_page_from_resource!(resource)
+      def upsert_page_from_resource!(resource, upserted_at: Time.zone.now)
         page = find_or_initialize_by(request_path: resource.request_path)
         page.published_at = resource.published_at if resource.published_at
-        page.updated_at = resource.revised_at if resource.revised_at # Should use a `Page#revised_at` column instead
+        page.revised_at = resource.revised_at if resource.revised_at
+        page.upserted_at = upserted_at
         page.save!
         page
       end
diff --git a/db/cable_schema.rb b/db/cable_schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2024_09_14_010017) do
+ActiveRecord::Schema[8.1].define(version: 2024_09_14_010017) do
   create_table "solid_cable_messages", force: :cascade do |t|
     t.binary "channel", limit: 1024, null: false
     t.binary "payload", limit: 536870912, null: false
diff --git a/db/cache_schema.rb b/db/cache_schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2024_02_10_145644) do
+ActiveRecord::Schema[8.1].define(version: 2024_02_10_145644) do
   create_table "_litestream_lock", id: false, force: :cascade do |t|
     t.integer "id"
   end
diff --git a/db/migrate/20250116121529_add_revision_timestamps_to_pages.rb b/db/migrate/20250116121529_add_revision_timestamps_to_pages.rb
@@ -0,0 +1,6 @@
+class AddRevisionTimestampsToPages < ActiveRecord::Migration[8.1]
+  def change
+    add_column :pages, :revised_at, :datetime, null: true # For manually tracking content revisions
+    add_column :pages, :upserted_at, :datetime, null: true # For automatically tracking upserts
+  end
+end
diff --git a/db/queue_schema.rb b/db/queue_schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2024_10_25_223629) do
+ActiveRecord::Schema[8.1].define(version: 2024_10_25_223629) do
   create_table "_litestream_lock", id: false, force: :cascade do |t|
     t.integer "id"
   end
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/spec/factories/pages.rb b/spec/factories/pages.rb
@@ -6,6 +6,8 @@
 #  indexed_at   :datetime
 #  published_at :datetime
 #  request_path :string           not null
+#  revised_at   :datetime
+#  upserted_at  :datetime
 #  created_at   :datetime         not null
 #  updated_at   :datetime         not null
 #
diff --git a/spec/models/page_spec.rb b/spec/models/page_spec.rb
@@ -6,6 +6,8 @@
 #  indexed_at   :datetime
 #  published_at :datetime
 #  request_path :string           not null
+#  revised_at   :datetime
+#  upserted_at  :datetime
 #  created_at   :datetime         not null
 #  updated_at   :datetime         not null
 #
diff --git a/spec/requests/site/conditional_get_spec.rb b/spec/requests/site/conditional_get_spec.rb
@@ -8,37 +8,41 @@
     allow(Rails.application.config.action_controller).to receive(:perform_caching).and_return(true)
   end
 
-  def stub_sitepress_asset(updated_at:, body:)
-    allow_any_instance_of(Sitepress::Asset).to receive(:updated_at).and_return(updated_at)
+  def stub_sitepress_asset(body:, updated_at: nil)
+    allow_any_instance_of(Sitepress::Asset).to receive(:updated_at).and_return(updated_at) if updated_at
     allow_any_instance_of(Sitepress::Asset).to receive(:body).and_return(body)
   end
 
-  def stub_file_updated_at(time)
-  end
-
-  def do_request(headers: {})
-    get "/articles/introducing-joy-of-rails", headers: headers
-  end
-
-  def do_request_with_conditional_get_headers
-    do_request(headers: {
+  def conditional_get_headers
+    {
       "HTTP_IF_NONE_MATCH" => response.headers["ETag"],
       "HTTP_IF_MODIFIED_SINCE" => response.headers["Last-Modified"]
-    })
+    }
   end
 
   context "on the first request" do
     it "returns a 200" do
-      do_request
+      get "/articles/introducing-joy-of-rails"
 
       expect(response.status).to eq(200)
     end
 
-    it "sets cache headers" do
+    it "sets cache headers for sitepress asset" do
       time = Time.now - 1.day
       stub_sitepress_asset(updated_at: time, body: "first request body")
 
-      do_request
+      get "/articles/introducing-joy-of-rails"
+
+      expect(response.headers["ETag"]).to be_present
+      expect(response.headers["Last-Modified"]).to eq(time.httpdate)
+    end
+
+    it "sets cache headers from Page record" do
+      time = Time.now - 1.day
+
+      Page.upsert_page_by_request_path!("/articles/introducing-joy-of-rails", upserted_at: time)
+
+      get "/articles/introducing-joy-of-rails"
 
       expect(response.headers["ETag"]).to be_present
       expect(response.headers["Last-Modified"]).to eq(time.httpdate)
@@ -47,12 +51,25 @@ def do_request_with_conditional_get_headers
 
   context "on a subsequent request" do
     context "if it is not stale" do
-      it "returns a 304" do
+      it "returns a 304 for timestamp from Sitepress asset" do
         time = Time.now - 1.day
         stub_sitepress_asset(updated_at: time, body: "first request body")
 
-        do_request
-        do_request_with_conditional_get_headers
+        get "/articles/introducing-joy-of-rails"
+        get "/articles/introducing-joy-of-rails", headers: conditional_get_headers
+
+        expect(response.headers["ETag"]).to be_present
+        expect(response.headers["Last-Modified"]).to eq(time.httpdate)
+        expect(response.status).to eq(304)
+      end
+
+      it "returns a 304 for timestamp from Page record" do
+        time = Time.now - 1.day
+
+        Page.upsert_page_by_request_path!("/articles/introducing-joy-of-rails", upserted_at: time)
+
+        get "/articles/introducing-joy-of-rails"
+        get "/articles/introducing-joy-of-rails", headers: conditional_get_headers
 
         expect(response.headers["ETag"]).to be_present
         expect(response.headers["Last-Modified"]).to eq(time.httpdate)
@@ -61,16 +78,34 @@ def do_request_with_conditional_get_headers
     end
 
     context "if it has been updated" do
-      it "returns a 200" do
+      it "returns a 200 with timestamp from Sitepress asset" do
         first_time = Time.now - 1.day
         stub_sitepress_asset(updated_at: first_time, body: "first request body")
 
-        do_request
+        get "/articles/introducing-joy-of-rails"
 
         second_time = Time.now
         stub_sitepress_asset(updated_at: second_time, body: "second request body")
 
-        do_request_with_conditional_get_headers
+        get "/articles/introducing-joy-of-rails", headers: conditional_get_headers
+
+        expect(response.headers["ETag"]).to be_present
+        expect(response.headers["Last-Modified"]).to eq(second_time.httpdate)
+        expect(response.status).to eq(200)
+      end
+
+      it "returns a 200 with timestamp from Page record" do
+        first_time = Time.now - 1.day
+        Page.upsert_page_by_request_path!("/articles/introducing-joy-of-rails", upserted_at: first_time)
+        stub_sitepress_asset(body: "first request body")
+
+        get "/articles/introducing-joy-of-rails"
+
+        second_time = Time.now
+        Page.upsert_page_by_request_path!("/articles/introducing-joy-of-rails", upserted_at: second_time)
+        stub_sitepress_asset(body: "second request body")
+
+        get "/articles/introducing-joy-of-rails", headers: conditional_get_headers
 
         expect(response.headers["ETag"]).to be_present
         expect(response.headers["Last-Modified"]).to eq(second_time.httpdate)

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,8 @@`
`9`	`9`	`# indexed_at :datetime`
`10`	`10`	`# published_at :datetime`
`11`	`11`	`# request_path :string not null`
	`12`	`+# revised_at :datetime`
	`13`	`+# upserted_at :datetime`
`12`	`14`	`# created_at :datetime not null`
`13`	`15`	`# updated_at :datetime not null`
`14`	`16`	`#`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@`
`10`	`10`	`#`
`11`	`11`	`# It's strongly recommended that you check this file into your version control system.`
`12`	`12`
`13`		`-ActiveRecord::Schema[8.0].define(version: 2024_09_14_010017) do`
	`13`	`+ActiveRecord::Schema[8.1].define(version: 2024_09_14_010017) do`
`14`	`14`	`create_table "solid_cable_messages", force: :cascade do \|t\|`
`15`	`15`	`t.binary "channel", limit: 1024, null: false`
`16`	`16`	`t.binary "payload", limit: 536870912, null: false`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,8 @@`
`6`	`6`	`# indexed_at :datetime`
`7`	`7`	`# published_at :datetime`
`8`	`8`	`# request_path :string not null`
	`9`	`+# revised_at :datetime`
	`10`	`+# upserted_at :datetime`
`9`	`11`	`# created_at :datetime not null`
`10`	`12`	`# updated_at :datetime not null`
`11`	`13`	`#`