Merge pull request #18374 from claudiob/add-collection-to-fresh-when

Accept a collection in fresh_when and stale?

3 files changed, 103 insertions, 11 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 8298a199d8..6b2373d0a2 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,21 @@
+*   Expand `ActionController::ConditionalGet#fresh_when` and `stale?` to also
+    accept a collection of records as the first argument, so that the
+    following code can be written in a shorter form.
+
+        # Before
+        def index
+          @article = Article.all
+          fresh_when(etag: @articles, last_modified: @articles.maximum(:created_at))
+        end
+
+        # After
+        def index
+          @article = Article.all
+          fresh_when(@articles)
+        end
+
+    *claudiob*
+
 *   Explicitly ignored wildcard verbs when searching for HEAD routes before fallback
 
     Fixes an issue where a mounted rack app at root would intercept the HEAD 
diff --git a/actionpack/lib/action_controller/metal/conditional_get.rb b/actionpack/lib/action_controller/metal/conditional_get.rb
index 3a5929adef..28f0b6e349 100644
--- a/actionpack/lib/action_controller/metal/conditional_get.rb
+++ b/actionpack/lib/action_controller/metal/conditional_get.rb
@@ -57,15 +57,25 @@ module ActionController
     # This will render the show template if the request isn't sending a matching ETag or
     # If-Modified-Since header and just a <tt>304 Not Modified</tt> response if there's a match.
     #
-    # You can also just pass a record where +last_modified+ will be set by calling
-    # +updated_at+ and the +etag+ by passing the object itself.
+    # You can also just pass a record. In this case +last_modified+ will be set
+    # by calling +updated_at+ and +etag+ by passing the object itself.
     #
     #   def show
     #     @article = Article.find(params[:id])
     #     fresh_when(@article)
     #   end
     #
-    # When passing a record, you can still set whether the public header:
+    # You can also pass an object that responds to +maximum+, such as a
+    # collection of active records. In this case +last_modified+ will be set by
+    # calling +maximum(:updated_at)+ on the collection (the timestamp of the
+    # most recently updated record) and the +etag+ by passing the object itself.
+    #
+    #   def index
+    #     @articles = Article.all
+    #     fresh_when(@articles)
+    #   end
+    #
+    # When passing a record or a collection, you can still set the public header:
     #
     #   def show
     #     @article = Article.find(params[:id])
@@ -77,8 +87,8 @@ module ActionController
     #
     #   before_action { fresh_when @article, template: 'widgets/show' }
     #
-    def fresh_when(record = nil, etag: record, last_modified: nil, public: false, template: nil)
-      last_modified ||= record.try(:updated_at)
+    def fresh_when(object = nil, etag: object, last_modified: nil, public: false, template: nil)
+      last_modified ||= object.try(:updated_at) || object.try(:maximum, :updated_at)
 
       if etag || template
         response.etag = combine_etags(etag: etag, last_modified: last_modified,
@@ -121,8 +131,8 @@ module ActionController
     #     end
     #   end
     #
-    # You can also just pass a record where +last_modified+ will be set by calling
-    # +updated_at+ and the +etag+ by passing the object itself.
+    # You can also just pass a record. In this case +last_modified+ will be set
+    # by calling +updated_at+ and +etag+ by passing the object itself.
     #
     #   def show
     #     @article = Article.find(params[:id])
@@ -135,7 +145,23 @@ module ActionController
     #     end
     #   end
     #
-    # When passing a record, you can still set whether the public header:
+    # You can also pass an object that responds to +maximum+, such as a
+    # collection of active records. In this case +last_modified+ will be set by
+    # calling +maximum(:updated_at)+ on the collection (the timestamp of the
+    # most recently updated record) and the +etag+ by passing the object itself.
+    #
+    #   def index
+    #     @articles = Article.all
+    #
+    #     if stale?(@articles)
+    #       @statistics = @articles.really_expensive_call
+    #       respond_to do |format|
+    #         # all the supported formats
+    #       end
+    #     end
+    #   end
+    #
+    # When passing a record or a collection, you can still set the public header:
     #
     #   def show
     #     @article = Article.find(params[:id])
@@ -155,8 +181,8 @@ module ActionController
     #     super if stale? @article, template: 'widgets/show'
     #   end
     #
-    def stale?(record = nil, etag: record, last_modified: nil, public: nil, template: nil)
-      fresh_when(record, etag: etag, last_modified: last_modified, public: public, template: template)
+    def stale?(object = nil, etag: object, last_modified: nil, public: nil, template: nil)
+      fresh_when(object, etag: etag, last_modified: last_modified, public: public, template: template)
       !request.fresh?(response)
     end
 
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index a5dd96ae91..8d6b62f2bf 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -58,6 +58,27 @@ class TestController < ActionController::Base
     end
   end
 
+  class Collection
+    def initialize(records)
+      @records = records
+    end
+
+    def maximum(attribute)
+      @records.max_by(&attribute).public_send(attribute)
+    end
+  end
+
+  def conditional_hello_with_collection_of_records
+    ts = Time.now.utc.beginning_of_day
+
+    record = Struct.new(:updated_at, :cache_key).new(ts, "foo/123")
+    old_record = Struct.new(:updated_at, :cache_key).new(ts - 1.day, "bar/123")
+
+    if stale?(Collection.new([record, old_record]))
+      render action: 'hello_world'
+    end
+  end
+
   def conditional_hello_with_expires_in
     expires_in 60.1.seconds
     render :action => 'hello_world'
@@ -288,7 +309,6 @@ class LastModifiedRenderTest < ActionController::TestCase
     assert_equal @last_modified, @response.headers['Last-Modified']
   end
 
-
   def test_responds_with_last_modified_with_record
     get :conditional_hello_with_record
     assert_equal @last_modified, @response.headers['Last-Modified']
@@ -318,6 +338,34 @@ class LastModifiedRenderTest < ActionController::TestCase
     assert_equal @last_modified, @response.headers['Last-Modified']
   end
 
+  def test_responds_with_last_modified_with_collection_of_records
+    get :conditional_hello_with_collection_of_records
+    assert_equal @last_modified, @response.headers['Last-Modified']
+  end
+
+  def test_request_not_modified_with_collection_of_records
+    @request.if_modified_since = @last_modified
+    get :conditional_hello_with_collection_of_records
+    assert_equal 304, @response.status.to_i
+    assert @response.body.blank?
+    assert_equal @last_modified, @response.headers['Last-Modified']
+  end
+
+  def test_request_not_modified_but_etag_differs_with_collection_of_records
+    @request.if_modified_since = @last_modified
+    @request.if_none_match = "234"
+    get :conditional_hello_with_collection_of_records
+    assert_response :success
+  end
+
+  def test_request_modified_with_collection_of_records
+    @request.if_modified_since = 'Thu, 16 Jul 2008 00:00:00 GMT'
+    get :conditional_hello_with_collection_of_records
+    assert_equal 200, @response.status.to_i
+    assert @response.body.present?
+    assert_equal @last_modified, @response.headers['Last-Modified']
+  end
+
   def test_request_with_bang_gets_last_modified
     get :conditional_hello_with_bangs
     assert_equal @last_modified, @response.headers['Last-Modified']