From 545ca85644e194031d7d6fb11f79f052631c2147 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 22 Apr 2020 14:55:59 +0200
Subject: [PATCH 01/11] Extract caching of homepage stats to service

---
 app/controllers/home_controller.rb | 6 ++++--
 app/services/cache_service.rb      | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
index 0a39b00d9b..e167d5462c 100644
--- a/app/controllers/home_controller.rb
+++ b/app/controllers/home_controller.rb
@@ -18,9 +18,11 @@ class HomeController < BaseController
 
   private
 
-  # Cache the value of the query count for 24 hours
+  # Cache the value of the query count
   def cached_count(key, query)
-    Rails.cache.fetch("home_stats_count_#{key}", expires_in: 1.day, race_condition_ttl: 10) do
+    CacheService.cache("home_stats_count_#{key}",
+                       expires_in: CacheService::HOME_STATS_EXPIRY,
+                       race_condition_ttl: 10) do
       query.count
     end
   end
diff --git a/app/services/cache_service.rb b/app/services/cache_service.rb
index 36c33025f5..a68f883d0c 100644
--- a/app/services/cache_service.rb
+++ b/app/services/cache_service.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class CacheService
+  HOME_STATS_EXPIRY = 1.day.freeze
   FILTERS_EXPIRY = 30.seconds.freeze
 
   def self.cache(cache_key, options = {})

From 2876b98155d2c2122e188a3265a59b63f3bdf1ec Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 22 Apr 2020 14:52:55 +0200
Subject: [PATCH 02/11] Use time-based caching on rendered AMS data for shops

---
 app/services/cache_service.rb        | 1 +
 app/views/enterprises/shop.html.haml | 3 ++-
 app/views/shops/index.html.haml      | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/services/cache_service.rb b/app/services/cache_service.rb
index a68f883d0c..79d81068eb 100644
--- a/app/services/cache_service.rb
+++ b/app/services/cache_service.rb
@@ -3,6 +3,7 @@
 class CacheService
   HOME_STATS_EXPIRY = 1.day.freeze
   FILTERS_EXPIRY = 30.seconds.freeze
+  SHOPS_EXPIRY = 15.seconds.freeze
 
   def self.cache(cache_key, options = {})
     Rails.cache.fetch cache_key.to_s, options do
diff --git a/app/views/enterprises/shop.html.haml b/app/views/enterprises/shop.html.haml
index 7f98960b33..b6bf91ed06 100644
--- a/app/views/enterprises/shop.html.haml
+++ b/app/views/enterprises/shop.html.haml
@@ -6,7 +6,8 @@
   = current_distributor.logo.url
 
 - content_for :injection_data do
-  = inject_enterprise_shopfront(@enterprise)
+  - cache "enterprises/shop/inject_enterprise_shopfront-#{@enterprise.id}", expires_in: CacheService::SHOPS_EXPIRY do
+    = inject_enterprise_shopfront(@enterprise)
 
 %shop.darkswarm
   - if @shopfront_layout == 'embedded'
diff --git a/app/views/shops/index.html.haml b/app/views/shops/index.html.haml
index d436b85e5e..0bf7709674 100644
--- a/app/views/shops/index.html.haml
+++ b/app/views/shops/index.html.haml
@@ -2,7 +2,8 @@
   = t :shops_title
 
 - content_for :injection_data do
-  = inject_enterprises(@enterprises)
+  - cache "shops/index/inject_enterprises", expires_in: CacheService::SHOPS_EXPIRY do
+    = inject_enterprises(@enterprises)
 
 #panes
   #shops.pane

From 2292cbaae4fe9d7da814744a39554b6bc024b5ee Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 30 Apr 2020 10:05:46 +0200
Subject: [PATCH 03/11] Extract timed enterprise AMS fragment caching options
 to service

---
 app/services/cache_service.rb        | 14 ++++++++++++++
 app/views/enterprises/shop.html.haml |  2 +-
 app/views/shops/index.html.haml      |  2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/app/services/cache_service.rb b/app/services/cache_service.rb
index 79d81068eb..682911457e 100644
--- a/app/services/cache_service.rb
+++ b/app/services/cache_service.rb
@@ -36,5 +36,19 @@ class CacheService
     def self.ams_all_properties_key
       "inject-all-properties-#{CacheService.latest_timestamp_by_class(Spree::Property)}"
     end
+
+    def self.ams_shops
+      [
+        "shops/index/inject_enterprises",
+        { expires_in: SHOPS_EXPIRY }
+      ]
+    end
+
+    def self.ams_shop(enterprise)
+      [
+        "enterprises/shop/inject_enterprise_shopfront-#{enterprise.id}",
+        { expires_in: SHOPS_EXPIRY }
+      ]
+    end
   end
 end
diff --git a/app/views/enterprises/shop.html.haml b/app/views/enterprises/shop.html.haml
index b6bf91ed06..1bc7bba978 100644
--- a/app/views/enterprises/shop.html.haml
+++ b/app/views/enterprises/shop.html.haml
@@ -6,7 +6,7 @@
   = current_distributor.logo.url
 
 - content_for :injection_data do
-  - cache "enterprises/shop/inject_enterprise_shopfront-#{@enterprise.id}", expires_in: CacheService::SHOPS_EXPIRY do
+  - cache(*CacheService::FragmentCaching.ams_shop(@enterprise)) do
     = inject_enterprise_shopfront(@enterprise)
 
 %shop.darkswarm
diff --git a/app/views/shops/index.html.haml b/app/views/shops/index.html.haml
index 0bf7709674..032096ebb9 100644
--- a/app/views/shops/index.html.haml
+++ b/app/views/shops/index.html.haml
@@ -2,7 +2,7 @@
   = t :shops_title
 
 - content_for :injection_data do
-  - cache "shops/index/inject_enterprises", expires_in: CacheService::SHOPS_EXPIRY do
+  - cache(*CacheService::FragmentCaching.ams_shops) do
     = inject_enterprises(@enterprises)
 
 #panes

From 37821beb1be4aa0de95f30082f04fb310b78e23e Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 30 Apr 2020 10:15:46 +0200
Subject: [PATCH 04/11] Extract home stats caching to method in service

---
 app/controllers/home_controller.rb | 6 ++----
 app/services/cache_service.rb      | 8 ++++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
index e167d5462c..afde45d3bd 100644
--- a/app/controllers/home_controller.rb
+++ b/app/controllers/home_controller.rb
@@ -19,10 +19,8 @@ class HomeController < BaseController
   private
 
   # Cache the value of the query count
-  def cached_count(key, query)
-    CacheService.cache("home_stats_count_#{key}",
-                       expires_in: CacheService::HOME_STATS_EXPIRY,
-                       race_condition_ttl: 10) do
+  def cached_count(statistic, query)
+    CacheService.home_stats(statistic) do
       query.count
     end
   end
diff --git a/app/services/cache_service.rb b/app/services/cache_service.rb
index 682911457e..dec5214e1a 100644
--- a/app/services/cache_service.rb
+++ b/app/services/cache_service.rb
@@ -25,6 +25,14 @@ class CacheService
     cached_class.maximum(:updated_at).to_i
   end
 
+  def self.home_stats(statistic)
+    Rails.cache.fetch("home_stats_count_#{statistic}",
+                      expires_in: HOME_STATS_EXPIRY,
+                      race_condition_ttl: 10) do
+      yield
+    end
+  end
+
   module FragmentCaching
     # Rails' caching in views is called "Fragment Caching" and uses some slightly different logic.
     # Note: supplied keys are actually prepended with "view/" under the hood.

From e73c8232de3719319e793f024a65c3a300f06b0c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 30 Apr 2020 10:33:11 +0200
Subject: [PATCH 05/11] Improve unit test in cache_service_spec

---
 spec/services/cache_service_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/spec/services/cache_service_spec.rb b/spec/services/cache_service_spec.rb
index 22a6e2b772..b609711bc6 100644
--- a/spec/services/cache_service_spec.rb
+++ b/spec/services/cache_service_spec.rb
@@ -22,7 +22,7 @@ describe CacheService do
 
     before do
       rails_cache.stub(:fetch)
-      CacheService.stub(:latest_timestamp_by_class) { timestamp }
+      allow(Enterprise).to receive(:maximum).with(:updated_at).and_return(timestamp)
     end
 
     it "caches data by timestamp for last record of that class" do
@@ -30,7 +30,6 @@ describe CacheService do
         "TEST"
       end
 
-      expect(CacheService).to have_received(:latest_timestamp_by_class).with(Enterprise)
       expect(rails_cache).to have_received(:fetch).with("test-cache-key-Enterprise-#{timestamp}")
     end
   end

From 6b6ab864ef7ca40b7990757a243d5e6ee22f0a8f Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 30 Apr 2020 17:30:41 +0200
Subject: [PATCH 06/11] Improve comment on Fragment Caching

---
 app/services/cache_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/cache_service.rb b/app/services/cache_service.rb
index dec5214e1a..1963b2f1dc 100644
--- a/app/services/cache_service.rb
+++ b/app/services/cache_service.rb
@@ -35,7 +35,7 @@ class CacheService
 
   module FragmentCaching
     # Rails' caching in views is called "Fragment Caching" and uses some slightly different logic.
-    # Note: supplied keys are actually prepended with "view/" under the hood.
+    # Note: keys supplied here are actually prepended with "views/" under the hood.
 
     def self.ams_all_taxons_key
       "inject-all-taxons-#{CacheService.latest_timestamp_by_class(Spree::Taxon)}"

From 7457543c2b9f584cf318375cb2366ca1bd5e50e1 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 30 Apr 2020 18:44:33 +0200
Subject: [PATCH 07/11] Adds spec for timed caching on shops data

---
 .../consumer/caching/shops_caching_spec.rb    | 31 +++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/spec/features/consumer/caching/shops_caching_spec.rb b/spec/features/consumer/caching/shops_caching_spec.rb
index 7d70796e37..2c3d845a2d 100644
--- a/spec/features/consumer/caching/shops_caching_spec.rb
+++ b/spec/features/consumer/caching/shops_caching_spec.rb
@@ -9,6 +9,34 @@ feature "Shops caching", js: true, caching: true do
   let!(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true, is_primary_producer: true) }
   let!(:order_cycle) { create(:open_order_cycle, distributors: [distributor], coordinator: distributor) }
 
+  describe "caching enterprises AMS data" do
+    it "caches data for all enterprises, with the provided options" do
+      visit shops_path
+
+      key, options = CacheService::FragmentCaching.ams_shops
+      expect_cached "views/#{key}", options
+    end
+
+    it "keeps data cached for a short time on subsequent requests" do
+      Timecop.travel(10.minutes.ago) do
+        visit shops_path
+
+        expect(page).to have_content distributor.name
+
+        distributor.name = "New Name"
+        distributor.save!
+
+        visit shops_path
+
+        expect(page).to_not have_content "New Name" # Displayed name is unchanged
+      end
+
+      # A while later...
+      visit shops_path
+      expect(page).to have_content "New Name" # Displayed name is now changed
+    end
+  end
+
   describe "API action caching on taxons and properties" do
     let!(:taxon) { create(:taxon, name: "Cached Taxon") }
     let!(:taxon2) { create(:taxon, name: "New Taxon") }
@@ -37,8 +65,7 @@ feature "Shops caching", js: true, caching: true do
     end
 
     it "keeps data cached for a short time on subsequent requests" do
-      # One minute ago...
-      Timecop.travel(Time.zone.now - 1.minute) do
+      Timecop.travel(10.minutes.ago) do
         visit enterprise_shop_path(distributor)
 
         expect(page).to have_content taxon.name

From 460ab6cdb4f770f37f5831a7e35e781d768be590 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 6 May 2020 13:46:05 +0200
Subject: [PATCH 08/11] Use #allow instead of #stub

---
 spec/services/cache_service_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/services/cache_service_spec.rb b/spec/services/cache_service_spec.rb
index b609711bc6..68d77b7f1e 100644
--- a/spec/services/cache_service_spec.rb
+++ b/spec/services/cache_service_spec.rb
@@ -5,7 +5,7 @@ describe CacheService do
 
   describe "#cache" do
     before do
-      rails_cache.stub(:fetch)
+      allow(rails_cache).to receive(:fetch)
     end
 
     it "provides a wrapper for basic #fetch calls to Rails.cache" do
@@ -21,7 +21,7 @@ describe CacheService do
     let(:timestamp) { Time.now.to_i }
 
     before do
-      rails_cache.stub(:fetch)
+      allow(rails_cache).to receive(:fetch)
       allow(Enterprise).to receive(:maximum).with(:updated_at).and_return(timestamp)
     end
 

From 01d741509f6744688f397d2f5e2d30cf45254548 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 7 May 2020 15:20:37 +0200
Subject: [PATCH 09/11] Fix test of rendered properties AMS

Here the displayed properties that we're testing also rely on shops data (including the list of supplied properties), which is now cached for 15 seconds. We clear that cache entry so we can cleanly test only the caching of the properties AMS array.
---
 spec/features/consumer/caching/darkwarm_caching_spec.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/spec/features/consumer/caching/darkwarm_caching_spec.rb b/spec/features/consumer/caching/darkwarm_caching_spec.rb
index 6dcbc18ac5..e1162e2961 100644
--- a/spec/features/consumer/caching/darkwarm_caching_spec.rb
+++ b/spec/features/consumer/caching/darkwarm_caching_spec.rb
@@ -50,6 +50,9 @@ feature "Darkswarm data caching", js: true, caching: true do
       property.presentation = "Changed Property"
       property.save
 
+      # Clear timed shops cache so we can test uncached supplied properties
+      clear_shops_cache
+
       visit shops_path
 
       taxon_timestamp2 = CacheService.latest_timestamp_by_class(Spree::Taxon)
@@ -73,4 +76,9 @@ feature "Darkswarm data caching", js: true, caching: true do
   def expect_cached(key)
     expect(Rails.cache.exist?(key)).to be true
   end
+
+  def clear_shops_cache
+    cache_key = "views/#{CacheService::FragmentCaching.ams_shops[0]}"
+    Rails.cache.delete cache_key
+  end
 end

From 86bfd1bebbec70e5d2c0dce40942fa3b4f3974b0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 7 May 2020 17:29:58 +0200
Subject: [PATCH 10/11] Add explanatory comments on usages of Timecop.travel

---
 spec/features/consumer/caching/shops_caching_spec.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/spec/features/consumer/caching/shops_caching_spec.rb b/spec/features/consumer/caching/shops_caching_spec.rb
index 2c3d845a2d..654a2f4524 100644
--- a/spec/features/consumer/caching/shops_caching_spec.rb
+++ b/spec/features/consumer/caching/shops_caching_spec.rb
@@ -18,6 +18,7 @@ feature "Shops caching", js: true, caching: true do
     end
 
     it "keeps data cached for a short time on subsequent requests" do
+      # Ensure sufficient time for requests to load and timed caches to expire
       Timecop.travel(10.minutes.ago) do
         visit shops_path
 
@@ -65,6 +66,7 @@ feature "Shops caching", js: true, caching: true do
     end
 
     it "keeps data cached for a short time on subsequent requests" do
+      # Ensure sufficient time for requests to load and timed caches to expire
       Timecop.travel(10.minutes.ago) do
         visit enterprise_shop_path(distributor)
 

From 5518ffa85684ef054b7eed695720df11df30b56b Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 7 May 2020 19:37:43 +0200
Subject: [PATCH 11/11] Ensure validations are called when updating attributes
 in test

---
 spec/features/consumer/caching/darkwarm_caching_spec.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/spec/features/consumer/caching/darkwarm_caching_spec.rb b/spec/features/consumer/caching/darkwarm_caching_spec.rb
index e1162e2961..38fdc05f46 100644
--- a/spec/features/consumer/caching/darkwarm_caching_spec.rb
+++ b/spec/features/consumer/caching/darkwarm_caching_spec.rb
@@ -45,10 +45,8 @@ feature "Darkswarm data caching", js: true, caching: true do
         expect(page).to have_content property.presentation
       end
 
-      taxon.name = "Changed Taxon"
-      taxon.save
-      property.presentation = "Changed Property"
-      property.save
+      taxon.update_attributes!(name: "Changed Taxon")
+      property.update_attributes!(presentation: "Changed Property")
 
       # Clear timed shops cache so we can test uncached supplied properties
       clear_shops_cache