From 0d5d015d883b6f8096b9e5c653ed278081072814 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 15 Jan 2016 13:29:28 +1100 Subject: [PATCH 01/66] Extract product JSON rendering to lib class. Fix HTML stripping that never actually worked. --- app/controllers/shop_controller.rb | 72 ++------------ app/serializers/api/product_serializer.rb | 5 + lib/open_food_network/products_renderer.rb | 84 +++++++++++++++++ spec/controllers/shop_controller_spec.rb | 93 +------------------ .../products_renderer_spec.rb | 91 ++++++++++++++++++ 5 files changed, 188 insertions(+), 157 deletions(-) create mode 100644 lib/open_food_network/products_renderer.rb create mode 100644 spec/lib/open_food_network/products_renderer_spec.rb diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index ef78605b41..8d7b9ab274 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -1,4 +1,4 @@ -require 'open_food_network/scope_product_to_hub' +require 'open_food_network/products_renderer' class ShopController < BaseController layout "darkswarm" @@ -10,22 +10,13 @@ class ShopController < BaseController end def products - if @products = products_for_shop + begin + products_json = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle).products - enterprise_fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new current_distributor, current_order_cycle + render json: products_json - render status: 200, - json: ActiveModel::ArraySerializer.new(@products, - each_serializer: Api::ProductSerializer, - current_order_cycle: current_order_cycle, - current_distributor: current_distributor, - variants: variants_for_shop_by_id, - master_variants: master_variants_for_shop_by_id, - enterprise_fee_calculator: enterprise_fee_calculator, - ).to_json - - else - render json: "", status: 404 + rescue OpenFoodNetwork::ProductsRenderer::NoProducts + render status: 404, json: '' end end @@ -42,55 +33,4 @@ class ShopController < BaseController end end - private - - def products_for_shop - if current_order_cycle - scoper = OpenFoodNetwork::ScopeProductToHub.new(current_distributor) - - current_order_cycle. - valid_products_distributed_by(current_distributor). - order(taxon_order). - each { |p| scoper.scope(p) }. - select { |p| !p.deleted? && p.has_stock_for_distribution?(current_order_cycle, current_distributor) } - end - end - - def taxon_order - if current_distributor.preferred_shopfront_taxon_order.present? - current_distributor - .preferred_shopfront_taxon_order - .split(",").map { |id| "primary_taxon_id=#{id} DESC" } - .join(",") + ", name ASC" - else - "name ASC" - end - end - - def all_variants_for_shop - # We use the in_stock? method here instead of the in_stock scope because we need to - # look up the stock as overridden by VariantOverrides, and the scope method is not affected - # by them. - scoper = OpenFoodNetwork::ScopeVariantToHub.new(current_distributor) - Spree::Variant. - for_distribution(current_order_cycle, current_distributor). - each { |v| scoper.scope(v) }. - select(&:in_stock?) - end - - def variants_for_shop_by_id - index_by_product_id all_variants_for_shop.reject(&:is_master) - end - - def master_variants_for_shop_by_id - index_by_product_id all_variants_for_shop.select(&:is_master) - end - - def index_by_product_id(variants) - variants.inject({}) do |vs, v| - vs[v.product_id] ||= [] - vs[v.product_id] << v - vs - end - end end diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 5a1d1b5c86..03a66afe89 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -34,6 +34,7 @@ end class Api::CachedProductSerializer < ActiveModel::Serializer #cached #delegate :cache_key, to: :object + include ActionView::Helpers::SanitizeHelper attributes :id, :name, :permalink, :count_on_hand attributes :on_demand, :group_buy, :notes, :description @@ -48,6 +49,10 @@ class Api::CachedProductSerializer < ActiveModel::Serializer has_many :images, serializer: Api::ImageSerializer has_one :supplier, serializer: Api::IdSerializer + def description + strip_tags object.description + end + def properties_with_values object.properties_including_inherited end diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb new file mode 100644 index 0000000000..d745b1b794 --- /dev/null +++ b/lib/open_food_network/products_renderer.rb @@ -0,0 +1,84 @@ +require 'open_food_network/scope_product_to_hub' + +module OpenFoodNetwork + class ProductsRenderer + class NoProducts < Exception; end + + def initialize(distributor, order_cycle) + @distributor = distributor + @order_cycle = order_cycle + end + + def products + products = products_for_shop + + if products + enterprise_fee_calculator = EnterpriseFeeCalculator.new @distributor, @order_cycle + + ActiveModel::ArraySerializer.new(products, + each_serializer: Api::ProductSerializer, + current_order_cycle: @order_cycle, + current_distributor: @distributor, + variants: variants_for_shop_by_id, + master_variants: master_variants_for_shop_by_id, + enterprise_fee_calculator: enterprise_fee_calculator, + ).to_json + else + raise NoProducts.new + end + end + + + private + + def products_for_shop + if @order_cycle + scoper = ScopeProductToHub.new(@distributor) + + @order_cycle. + valid_products_distributed_by(@distributor). + order(taxon_order). + each { |p| scoper.scope(p) }. + select { |p| !p.deleted? && p.has_stock_for_distribution?(@order_cycle, @distributor) } + end + end + + def taxon_order + if @distributor.preferred_shopfront_taxon_order.present? + @distributor + .preferred_shopfront_taxon_order + .split(",").map { |id| "primary_taxon_id=#{id} DESC" } + .join(",") + ", name ASC" + else + "name ASC" + end + end + + def all_variants_for_shop + # We use the in_stock? method here instead of the in_stock scope because we need to + # look up the stock as overridden by VariantOverrides, and the scope method is not affected + # by them. + scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor) + Spree::Variant. + for_distribution(@order_cycle, @distributor). + each { |v| scoper.scope(v) }. + select(&:in_stock?) + end + + def variants_for_shop_by_id + index_by_product_id all_variants_for_shop.reject(&:is_master) + end + + def master_variants_for_shop_by_id + index_by_product_id all_variants_for_shop.select(&:is_master) + end + + def index_by_product_id(variants) + variants.inject({}) do |vs, v| + vs[v.product_id] ||= [] + vs[v.product_id] << v + vs + end + end + end +end diff --git a/spec/controllers/shop_controller_spec.rb b/spec/controllers/shop_controller_spec.rb index 2c7105b356..09ea85dd44 100644 --- a/spec/controllers/shop_controller_spec.rb +++ b/spec/controllers/shop_controller_spec.rb @@ -37,7 +37,7 @@ describe ShopController do controller.current_order_cycle.should == oc2 end - context "RABL tests" do + context "JSON tests" do render_views it "should return the order cycle details when the oc is selected" do oc1 = create(:simple_order_cycle, distributors: [d]) @@ -86,7 +86,7 @@ describe ShopController do describe "requests and responses" do let(:product) { create(:product) } before do - exchange.variants << product.master + exchange.variants << product.variants.first end it "returns products via json" do @@ -102,95 +102,6 @@ describe ShopController do response.body.should be_empty end end - - describe "sorting" do - let(:t1) { create(:taxon) } - let(:t2) { create(:taxon) } - let!(:p1) { create(:product, name: "abc", primary_taxon_id: t2.id) } - let!(:p2) { create(:product, name: "def", primary_taxon_id: t1.id) } - let!(:p3) { create(:product, name: "ghi", primary_taxon_id: t2.id) } - let!(:p4) { create(:product, name: "jkl", primary_taxon_id: t1.id) } - - before do - exchange.variants << p1.variants.first - exchange.variants << p2.variants.first - exchange.variants << p3.variants.first - exchange.variants << p4.variants.first - end - - it "sorts products by the distributor's preferred taxon list" do - d.stub(:preferred_shopfront_taxon_order) {"#{t1.id},#{t2.id}"} - controller.stub(:current_order_cycle).and_return order_cycle - xhr :get, :products - assigns[:products].should == [p2, p4, p1, p3] - end - - it "alphabetizes products by name when taxon list is not set" do - d.stub(:preferred_shopfront_taxon_order) {""} - controller.stub(:current_order_cycle).and_return order_cycle - xhr :get, :products - assigns[:products].should == [p1, p2, p3, p4] - end - end - - context "RABL tests" do - render_views - let(:product) { create(:product) } - let(:variant) { product.variants.first } - - before do - exchange.variants << variant - controller.stub(:current_order_cycle).and_return order_cycle - end - - it "only returns products for the current order cycle" do - xhr :get, :products - response.body.should have_content product.name - end - - it "doesn't return products not in stock" do - variant.update_attribute(:count_on_hand, 0) - xhr :get, :products - response.body.should_not have_content product.name - end - - it "strips html from description" do - product.update_attribute(:description, "turtles frogs") - xhr :get, :products - response.body.should have_content "frogs" - response.body.should_not have_content " [v1]} end end end diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb new file mode 100644 index 0000000000..875eba1ac9 --- /dev/null +++ b/spec/lib/open_food_network/products_renderer_spec.rb @@ -0,0 +1,91 @@ +require 'spec_helper' +require 'open_food_network/products_renderer' + +module OpenFoodNetwork + describe ProductsRenderer do + let(:d) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [d], coordinator: create(:distributor_enterprise)) } + let(:exchange) { Exchange.find(order_cycle.exchanges.to_enterprises(d).outgoing.first.id) } + let(:pr) { ProductsRenderer.new(d, order_cycle) } + + describe "sorting" do + let(:t1) { create(:taxon) } + let(:t2) { create(:taxon) } + let!(:p1) { create(:product, name: "abc", primary_taxon_id: t2.id) } + let!(:p2) { create(:product, name: "def", primary_taxon_id: t1.id) } + let!(:p3) { create(:product, name: "ghi", primary_taxon_id: t2.id) } + let!(:p4) { create(:product, name: "jkl", primary_taxon_id: t1.id) } + + before do + exchange.variants << p1.variants.first + exchange.variants << p2.variants.first + exchange.variants << p3.variants.first + exchange.variants << p4.variants.first + end + + it "sorts products by the distributor's preferred taxon list" do + d.stub(:preferred_shopfront_taxon_order) {"#{t1.id},#{t2.id}"} + products = pr.send(:products_for_shop) + products.should == [p2, p4, p1, p3] + end + + it "alphabetizes products by name when taxon list is not set" do + d.stub(:preferred_shopfront_taxon_order) {""} + products = pr.send(:products_for_shop) + products.should == [p1, p2, p3, p4] + end + end + + context "JSON tests" do + let(:product) { create(:product) } + let(:variant) { product.variants.first } + + before do + exchange.variants << variant + end + + it "only returns products for the current order cycle" do + pr.products.should include product.name + end + + it "doesn't return products not in stock" do + variant.update_attribute(:count_on_hand, 0) + pr.products.should_not include product.name + end + + it "strips html from description" do + product.update_attribute(:description, "turtles frogs") + json = pr.products + json.should include "frogs" + json.should_not include " [v1]} + end + end + end +end From 920d3bb974d70a19d9b7951c22c31c1ab7615b8c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 15 Jan 2016 13:29:46 +1100 Subject: [PATCH 02/66] Do not show knapsack time offset warnings unless in CI --- spec/spec_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 80d7c2eeb7..a477ad31e1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -4,6 +4,7 @@ require 'rubygems' require 'pry' unless ENV['CI'] require 'knapsack' +Knapsack.tracker.config({enable_time_offset_warning: false}) unless ENV['CI'] Knapsack::Adapters::RSpecAdapter.bind ENV["RAILS_ENV"] ||= 'test' From b0207f2b4957779ab29cd6580041ac2fa84a5e2c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 15 Jan 2016 13:36:12 +1100 Subject: [PATCH 03/66] Clean up specs --- spec/controllers/shop_controller_spec.rb | 46 ++++++++++--------- .../products_renderer_spec.rb | 14 +++--- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/spec/controllers/shop_controller_spec.rb b/spec/controllers/shop_controller_spec.rb index 09ea85dd44..73231bc86c 100644 --- a/spec/controllers/shop_controller_spec.rb +++ b/spec/controllers/shop_controller_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe ShopController do - let(:d) { create(:distributor_enterprise) } + let(:distributor) { create(:distributor_enterprise) } it "redirects to the home page if no distributor is selected" do spree_get :show @@ -11,26 +11,26 @@ describe ShopController do describe "with a distributor in place" do before do - controller.stub(:current_distributor).and_return d + controller.stub(:current_distributor).and_return distributor end - describe "Selecting order cycles" do + describe "selecting an order cycle" do it "should select an order cycle when only one order cycle is open" do - oc1 = create(:simple_order_cycle, distributors: [d]) + oc1 = create(:simple_order_cycle, distributors: [distributor]) spree_get :show controller.current_order_cycle.should == oc1 end it "should not set an order cycle when multiple order cycles are open" do - oc1 = create(:simple_order_cycle, distributors: [d]) - oc2 = create(:simple_order_cycle, distributors: [d]) + oc1 = create(:simple_order_cycle, distributors: [distributor]) + oc2 = create(:simple_order_cycle, distributors: [distributor]) spree_get :show - controller.current_order_cycle.should == nil + controller.current_order_cycle.should be_nil end it "should allow the user to post to select the current order cycle" do - oc1 = create(:simple_order_cycle, distributors: [d]) - oc2 = create(:simple_order_cycle, distributors: [d]) + oc1 = create(:simple_order_cycle, distributors: [distributor]) + oc2 = create(:simple_order_cycle, distributors: [distributor]) spree_post :order_cycle, order_cycle_id: oc2.id response.should be_success @@ -39,9 +39,10 @@ describe ShopController do context "JSON tests" do render_views - it "should return the order cycle details when the oc is selected" do - oc1 = create(:simple_order_cycle, distributors: [d]) - oc2 = create(:simple_order_cycle, distributors: [d]) + + it "should return the order cycle details when the OC is selected" do + oc1 = create(:simple_order_cycle, distributors: [distributor]) + oc2 = create(:simple_order_cycle, distributors: [distributor]) spree_post :order_cycle, order_cycle_id: oc2.id response.should be_success @@ -49,7 +50,7 @@ describe ShopController do end it "should return the current order cycle when hit with GET" do - oc1 = create(:simple_order_cycle, distributors: [d]) + oc1 = create(:simple_order_cycle, distributors: [distributor]) controller.stub(:current_order_cycle).and_return oc1 spree_get :order_cycle response.body.should have_content oc1.id @@ -57,13 +58,13 @@ describe ShopController do end it "should not allow the user to select an invalid order cycle" do - oc1 = create(:simple_order_cycle, distributors: [d]) - oc2 = create(:simple_order_cycle, distributors: [d]) + oc1 = create(:simple_order_cycle, distributors: [distributor]) + oc2 = create(:simple_order_cycle, distributors: [distributor]) oc3 = create(:simple_order_cycle, distributors: [create(:distributor_enterprise)]) spree_post :order_cycle, order_cycle_id: oc3.id response.status.should == 404 - controller.current_order_cycle.should == nil + controller.current_order_cycle.should be_nil end end @@ -71,31 +72,32 @@ describe ShopController do describe "producers/suppliers" do let(:supplier) { create(:supplier_enterprise) } let(:product) { create(:product, supplier: supplier) } - let(:order_cycle) { create(:simple_order_cycle, distributors: [d], coordinator: create(:distributor_enterprise)) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } before do - exchange = Exchange.find(order_cycle.exchanges.to_enterprises(d).outgoing.first.id) + exchange = order_cycle.exchanges.to_enterprises(distributor).outgoing.first exchange.variants << product.master end end describe "returning products" do - let(:order_cycle) { create(:simple_order_cycle, distributors: [d], coordinator: create(:distributor_enterprise)) } - let(:exchange) { Exchange.find(order_cycle.exchanges.to_enterprises(d).outgoing.first.id) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } + let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } describe "requests and responses" do let(:product) { create(:product) } + before do exchange.variants << product.variants.first end - it "returns products via json" do + it "returns products via JSON" do controller.stub(:current_order_cycle).and_return order_cycle xhr :get, :products response.should be_success end - it "does not return products if no order_cycle is selected" do + it "does not return products if no order cycle is selected" do controller.stub(:current_order_cycle).and_return nil xhr :get, :products response.status.should == 404 diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb index 875eba1ac9..fac7154dfd 100644 --- a/spec/lib/open_food_network/products_renderer_spec.rb +++ b/spec/lib/open_food_network/products_renderer_spec.rb @@ -3,10 +3,10 @@ require 'open_food_network/products_renderer' module OpenFoodNetwork describe ProductsRenderer do - let(:d) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle, distributors: [d], coordinator: create(:distributor_enterprise)) } - let(:exchange) { Exchange.find(order_cycle.exchanges.to_enterprises(d).outgoing.first.id) } - let(:pr) { ProductsRenderer.new(d, order_cycle) } + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } + let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } + let(:pr) { ProductsRenderer.new(distributor, order_cycle) } describe "sorting" do let(:t1) { create(:taxon) } @@ -24,13 +24,13 @@ module OpenFoodNetwork end it "sorts products by the distributor's preferred taxon list" do - d.stub(:preferred_shopfront_taxon_order) {"#{t1.id},#{t2.id}"} + distributor.stub(:preferred_shopfront_taxon_order) {"#{t1.id},#{t2.id}"} products = pr.send(:products_for_shop) products.should == [p2, p4, p1, p3] end it "alphabetizes products by name when taxon list is not set" do - d.stub(:preferred_shopfront_taxon_order) {""} + distributor.stub(:preferred_shopfront_taxon_order) {""} products = pr.send(:products_for_shop) products.should == [p1, p2, p3, p4] end @@ -81,9 +81,9 @@ module OpenFoodNetwork let(:p) { create(:simple_product) } let!(:v1) { create(:variant, product: p, unit_value: 3) } let!(:v2) { create(:variant, product: p, unit_value: 5) } + let(:pr) { ProductsRenderer.new(hub, oc) } it "scopes variants to distribution" do - pr = ProductsRenderer.new(hub, oc) pr.send(:variants_for_shop_by_id).should == {p.id => [v1]} end end From 6df8f73bb014334cfb2b9d6975b49e08784aa177 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 15 Jan 2016 13:45:36 +1100 Subject: [PATCH 04/66] Make method naming more explanatory --- app/controllers/shop_controller.rb | 2 +- lib/open_food_network/products_renderer.rb | 6 +++--- .../open_food_network/products_renderer_spec.rb | 14 +++++++------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index 8d7b9ab274..d1c7e7582e 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -11,7 +11,7 @@ class ShopController < BaseController def products begin - products_json = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle).products + products_json = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle).products_json render json: products_json diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index d745b1b794..6dd0d5f5ed 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -9,8 +9,8 @@ module OpenFoodNetwork @order_cycle = order_cycle end - def products - products = products_for_shop + def products_json + products = load_products if products enterprise_fee_calculator = EnterpriseFeeCalculator.new @distributor, @order_cycle @@ -31,7 +31,7 @@ module OpenFoodNetwork private - def products_for_shop + def load_products if @order_cycle scoper = ScopeProductToHub.new(@distributor) diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb index fac7154dfd..aa2ef2ec27 100644 --- a/spec/lib/open_food_network/products_renderer_spec.rb +++ b/spec/lib/open_food_network/products_renderer_spec.rb @@ -25,13 +25,13 @@ module OpenFoodNetwork it "sorts products by the distributor's preferred taxon list" do distributor.stub(:preferred_shopfront_taxon_order) {"#{t1.id},#{t2.id}"} - products = pr.send(:products_for_shop) + products = pr.send(:load_products) products.should == [p2, p4, p1, p3] end it "alphabetizes products by name when taxon list is not set" do distributor.stub(:preferred_shopfront_taxon_order) {""} - products = pr.send(:products_for_shop) + products = pr.send(:load_products) products.should == [p1, p2, p3, p4] end end @@ -45,17 +45,17 @@ module OpenFoodNetwork end it "only returns products for the current order cycle" do - pr.products.should include product.name + pr.products_json.should include product.name end it "doesn't return products not in stock" do variant.update_attribute(:count_on_hand, 0) - pr.products.should_not include product.name + pr.products_json.should_not include product.name end it "strips html from description" do product.update_attribute(:description, "turtles frogs") - json = pr.products + json = pr.products_json json.should include "frogs" json.should_not include " Date: Fri, 15 Jan 2016 14:03:58 +1100 Subject: [PATCH 05/66] Add job to cache products JSON --- app/jobs/refresh_products_cache_job.rb | 15 +++++++++++++++ spec/jobs/refresh_products_cache_job_spec.rb | 15 +++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 app/jobs/refresh_products_cache_job.rb create mode 100644 spec/jobs/refresh_products_cache_job_spec.rb diff --git a/app/jobs/refresh_products_cache_job.rb b/app/jobs/refresh_products_cache_job.rb new file mode 100644 index 0000000000..be7fccfb86 --- /dev/null +++ b/app/jobs/refresh_products_cache_job.rb @@ -0,0 +1,15 @@ +require 'open_food_network/products_renderer' + +RefreshProductsCacheJob = Struct.new(:distributor_id, :order_cycle_id) do + def perform + Rails.cache.write "products-json-#{distributor_id}-#{order_cycle_id}", products_json + end + + + private + + def products_json + OpenFoodNetwork::ProductsRenderer.new(distributor_id, order_cycle_id).products_json + end + +end diff --git a/spec/jobs/refresh_products_cache_job_spec.rb b/spec/jobs/refresh_products_cache_job_spec.rb new file mode 100644 index 0000000000..bf70e33e28 --- /dev/null +++ b/spec/jobs/refresh_products_cache_job_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' +require 'open_food_network/products_renderer' + +describe RefreshProductsCacheJob do + let(:distributor_id) { 123 } + let(:order_cycle_id) { 456 } + + it "renders products and writes them to cache" do + OpenFoodNetwork::ProductsRenderer.any_instance.stub(:products_json) { 'products' } + + run_job RefreshProductsCacheJob.new distributor_id, order_cycle_id + + expect(Rails.cache.read("products-json-123-456")).to eq 'products' + end +end From eba636c929f95792a73f9f79f63e124d3ee0e70e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 12:05:36 +1100 Subject: [PATCH 06/66] When variant is changed/destroyed, trigger product cache refresh --- app/models/spree/variant_decorator.rb | 13 +++++++++++++ spec/models/spree/variant_spec.rb | 16 ++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 33e7f5bffd..36a4d0e45a 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -1,5 +1,6 @@ require 'open_food_network/enterprise_fee_calculator' require 'open_food_network/variant_and_line_item_naming' +require 'open_food_network/products_cache' Spree::Variant.class_eval do # Remove method From Spree, so method from the naming module is used instead @@ -24,6 +25,18 @@ Spree::Variant.class_eval do before_validation :update_weight_from_unit_value, if: -> v { v.product.present? } after_save :update_units + after_save :refresh_products_cache + around_destroy :refresh_products_cache_from_destroy + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.variant_changed self + end + + def refresh_products_cache_from_destroy + OpenFoodNetwork::ProductsCache.variant_destroyed(self) { yield } + end + + scope :with_order_cycles_inner, joins(exchanges: :order_cycle) diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index cd6bfc1bd3..73789e62c2 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' require 'open_food_network/option_value_namer' +require 'open_food_network/products_cache' module Spree describe Variant do @@ -112,6 +113,21 @@ module Spree end end + describe "callbacks" do + let(:variant) { create(:variant) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + variant.sku = 'abc123' + variant.save + end + + it "refreshes the products cache on destroy" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + variant.destroy + end + end + describe "indexing variants by id" do let!(:v1) { create(:variant) } let!(:v2) { create(:variant) } From 3621c34bd5240ac89d189d9ddf1e6aff1526d304 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 13:39:45 +1100 Subject: [PATCH 07/66] Job calls ProductsRenderer with object instances rather than ids --- app/jobs/refresh_products_cache_job.rb | 5 +++-- spec/jobs/refresh_products_cache_job_spec.rb | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/jobs/refresh_products_cache_job.rb b/app/jobs/refresh_products_cache_job.rb index be7fccfb86..0c66925be8 100644 --- a/app/jobs/refresh_products_cache_job.rb +++ b/app/jobs/refresh_products_cache_job.rb @@ -9,7 +9,8 @@ RefreshProductsCacheJob = Struct.new(:distributor_id, :order_cycle_id) do private def products_json - OpenFoodNetwork::ProductsRenderer.new(distributor_id, order_cycle_id).products_json + distributor = Enterprise.find distributor_id + order_cycle = OrderCycle.find order_cycle_id + OpenFoodNetwork::ProductsRenderer.new(distributor, order_cycle).products_json end - end diff --git a/spec/jobs/refresh_products_cache_job_spec.rb b/spec/jobs/refresh_products_cache_job_spec.rb index bf70e33e28..2197ba38c9 100644 --- a/spec/jobs/refresh_products_cache_job_spec.rb +++ b/spec/jobs/refresh_products_cache_job_spec.rb @@ -2,14 +2,24 @@ require 'spec_helper' require 'open_food_network/products_renderer' describe RefreshProductsCacheJob do - let(:distributor_id) { 123 } - let(:order_cycle_id) { 456 } + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle) } it "renders products and writes them to cache" do OpenFoodNetwork::ProductsRenderer.any_instance.stub(:products_json) { 'products' } - run_job RefreshProductsCacheJob.new distributor_id, order_cycle_id + run_job RefreshProductsCacheJob.new distributor.id, order_cycle.id expect(Rails.cache.read("products-json-123-456")).to eq 'products' end + + describe "fetching products JSON" do + let(:job) { RefreshProductsCacheJob.new distributor.id, order_cycle.id } + let(:pr) { double(:products_renderer, products_json: nil) } + + it "fetches products JSON" do + expect(OpenFoodNetwork::ProductsRenderer).to receive(:new).with(distributor, order_cycle) { pr } + job.send(:products_json) + end + end end From 5d20b4fb51934b687b02dfe4730b1a06b9d1ddfe Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 13:41:30 +1100 Subject: [PATCH 08/66] Add OrderCycle scopes: not_closed, dated --- app/models/order_cycle.rb | 2 ++ spec/factories.rb | 24 ++++++++++++++++++++++-- spec/models/order_cycle_spec.rb | 2 ++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 03979220d6..b37503151e 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -15,8 +15,10 @@ class OrderCycle < ActiveRecord::Base scope :active_or_complete, lambda { where('order_cycles.orders_open_at <= ?', Time.zone.now) } scope :inactive, lambda { where('order_cycles.orders_open_at > ? OR order_cycles.orders_close_at < ?', Time.zone.now, Time.zone.now) } scope :upcoming, lambda { where('order_cycles.orders_open_at > ?', Time.zone.now) } + scope :not_closed, lambda { where('order_cycles.orders_close_at > ? OR order_cycles.orders_close_at IS NULL', Time.zone.now) } scope :closed, lambda { where('order_cycles.orders_close_at < ?', Time.zone.now).order("order_cycles.orders_close_at DESC") } scope :undated, where('order_cycles.orders_open_at IS NULL OR orders_close_at IS NULL') + scope :dated, where('orders_open_at IS NOT NULL AND orders_close_at IS NOT NULL') scope :soonest_closing, lambda { active.order('order_cycles.orders_close_at ASC') } # TODO This method returns all the closed orders. So maybe we can replace it with :recently_closed. diff --git a/spec/factories.rb b/spec/factories.rb index dd8d02e7bf..6df4216746 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -60,8 +60,8 @@ FactoryGirl.define do factory :simple_order_cycle, :class => OrderCycle do sequence(:name) { |n| "Order Cycle #{n}" } - orders_open_at { Time.zone.now - 1.day } - orders_close_at { Time.zone.now + 1.week } + orders_open_at { 1.day.ago } + orders_close_at { 1.week.from_now } coordinator { Enterprise.is_distributor.first || FactoryGirl.create(:distributor_enterprise) } @@ -84,6 +84,26 @@ FactoryGirl.define do end end + factory :undated_order_cycle, parent: :simple_order_cycle do + orders_open_at nil + orders_close_at nil + end + + factory :upcoming_order_cycle, parent: :simple_order_cycle do + orders_open_at { 1.week.from_now } + orders_close_at { 2.weeks.from_now } + end + + factory :open_order_cycle, parent: :simple_order_cycle do + orders_open_at { 1.week.ago } + orders_close_at { 1.week.from_now } + end + + factory :closed_order_cycle, parent: :simple_order_cycle do + orders_open_at { 2.weeks.ago } + orders_close_at { 1.week.ago } + end + factory :exchange, :class => Exchange do order_cycle { OrderCycle.first || FactoryGirl.create(:simple_order_cycle) } sender { FactoryGirl.create(:enterprise) } diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 90465fb80e..bad2d15312 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -43,6 +43,8 @@ describe OrderCycle do OrderCycle.upcoming.should == [oc_not_yet_open] OrderCycle.closed.should == [oc_already_closed] OrderCycle.undated.should == [oc_undated, oc_undated_open, oc_undated_close] + OrderCycle.not_closed.should == [oc_active, oc_not_yet_open, oc_undated] + OrderCycle.dated.should == [oc_active, oc_not_yet_open, oc_already_closed] end it "finds order cycles accessible by a user" do From a0a61b65cb44355b118e6e98cc5b4c18349d9c6e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 13:45:28 +1100 Subject: [PATCH 09/66] Refresh the appropriate product caches when a variant is changed --- lib/open_food_network/products_cache.rb | 29 +++++++++ .../open_food_network/products_cache_spec.rb | 65 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 lib/open_food_network/products_cache.rb create mode 100644 spec/lib/open_food_network/products_cache_spec.rb diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb new file mode 100644 index 0000000000..e2d2753cc5 --- /dev/null +++ b/lib/open_food_network/products_cache.rb @@ -0,0 +1,29 @@ +module OpenFoodNetwork + + # When elements of the data model change, enqueue jobs to refresh the appropriate parts of + # the products cache. + class ProductsCache + def self.variant_changed(variant) + exchanges_featuring_variant(variant).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + + private + + def self.exchanges_featuring_variant(variant) + Exchange. + outgoing. + with_variant(variant). + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + end + + + def self.refresh_cache(distributor, order_cycle) + Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + end + end +end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb new file mode 100644 index 0000000000..c00fb16da0 --- /dev/null +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -0,0 +1,65 @@ +require 'open_food_network/products_cache' + +module OpenFoodNetwork + describe ProductsCache do + describe "when a variant changes" do + let(:variant) { create(:variant) } + let(:variant_undistributed) { create(:variant) } + let(:supplier) { create(:supplier_enterprise) } + let(:coordinator) { create(:distributor_enterprise) } + let(:distributor) { create(:distributor_enterprise) } + let(:oc_undated) { create(:undated_order_cycle, distributors: [distributor], variants: [variant]) } + let(:oc_upcoming) { create(:upcoming_order_cycle, suppliers: [supplier], coordinator: coordinator, distributors: [distributor], variants: [variant]) } + let(:oc_open) { create(:open_order_cycle, distributors: [distributor], variants: [variant]) } + let(:oc_closed) { create(:closed_order_cycle, distributors: [distributor], variants: [variant]) } + + it "refreshes distributions with upcoming order cycles" do + oc_upcoming + expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc_upcoming) + ProductsCache.variant_changed variant + end + + it "refreshes distributions with open order cycles" do + oc_open + expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc_open) + ProductsCache.variant_changed variant + end + + it "does not refresh distributions with undated order cycles" do + oc_undated + expect(ProductsCache).not_to receive(:refresh_cache).with(distributor, oc_undated) + ProductsCache.variant_changed variant + end + + it "does not refresh distributions with closed order cycles" do + oc_closed + expect(ProductsCache).not_to receive(:refresh_cache).with(distributor, oc_closed) + ProductsCache.variant_changed variant + end + + it "limits refresh to outgoing exchanges" do + oc_upcoming + expect(ProductsCache).not_to receive(:refresh_cache).with(coordinator, oc_upcoming) + ProductsCache.variant_changed variant + end + + it "does not refresh distributions where the variant does not appear" do + oc_undated; oc_upcoming; oc_open; oc_closed + variant_undistributed + expect(ProductsCache).not_to receive(:refresh_cache) + ProductsCache.variant_changed variant_undistributed + end + end + + describe "refreshing the cache" do + let(:distributor) { double(:distributor, id: 123) } + let(:order_cycle) { double(:order_cycle, id: 456) } + + it "enqueues a RefreshProductsCacheJob" do + expect do + ProductsCache.send(:refresh_cache, distributor, order_cycle) + end.to enqueue_job RefreshProductsCacheJob, distributor_id: distributor.id, order_cycle_id: order_cycle.id + end + end + end +end From 6d39cc39c6d87bd9be52f0151ec7b75a524dd011 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 15:17:17 +1100 Subject: [PATCH 10/66] When a variant is destroyed, update product cache --- app/models/spree/variant_decorator.rb | 29 ++++++++++++------- lib/open_food_network/products_cache.rb | 11 +++++++ .../open_food_network/products_cache_spec.rb | 19 ++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 36a4d0e45a..143508185a 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -10,7 +10,7 @@ Spree::Variant.class_eval do include OpenFoodNetwork::VariantAndLineItemNaming - has_many :exchange_variants, dependent: :destroy + has_many :exchange_variants has_many :exchanges, through: :exchange_variants has_many :variant_overrides @@ -26,16 +26,7 @@ Spree::Variant.class_eval do before_validation :update_weight_from_unit_value, if: -> v { v.product.present? } after_save :update_units after_save :refresh_products_cache - around_destroy :refresh_products_cache_from_destroy - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.variant_changed self - end - - def refresh_products_cache_from_destroy - OpenFoodNetwork::ProductsCache.variant_destroyed(self) { yield } - end - + around_destroy :destruction scope :with_order_cycles_inner, joins(exchanges: :order_cycle) @@ -99,4 +90,20 @@ Spree::Variant.class_eval do def update_weight_from_unit_value self.weight = weight_from_unit_value if self.product.variant_unit == 'weight' && unit_value.present? end + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.variant_changed self + end + + def destruction + OpenFoodNetwork::ProductsCache.variant_destroyed(self) do + # Remove this association here instead of using dependent: :destroy because + # dependent-destroy acts before this around_filter is called, so ProductsCache + # has no way of knowing which exchanges the variant was a member of. + exchange_variants.destroy_all + + # Destroy the variant + yield + end + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index e2d2753cc5..b5e83d7590 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -10,6 +10,17 @@ module OpenFoodNetwork end + def self.variant_destroyed(variant, &block) + exchanges = exchanges_featuring_variant(variant).to_a + + block.call + + exchanges.each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + private def self.exchanges_featuring_variant(variant) diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index c00fb16da0..afc54ae87b 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -51,6 +51,25 @@ module OpenFoodNetwork end end + describe "when a variant is destroyed" do + let(:variant) { create(:variant) } + let(:distributor) { create(:distributor_enterprise) } + let!(:oc) { create(:open_order_cycle, distributors: [distributor], variants: [variant]) } + + it "refreshes the cache based on exchanges the variant was in before destruction" do + expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) + variant.destroy + end + + it "performs the cache refresh after the variant has been destroyed" do + expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) do + expect(Spree::Variant.where(id: variant.id)).to be_empty + end + + variant.destroy + end + end + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } From 0c0c98a0b06367e97e13ac8c7f1d005f47156881 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 15:41:17 +1100 Subject: [PATCH 11/66] Refresh products cache on product change --- app/models/spree/product_decorator.rb | 14 +++++++++----- lib/open_food_network/products_cache.rb | 4 ++++ spec/models/spree/product_spec.rb | 13 +++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index b089c9a52b..e969dd500b 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -22,8 +22,6 @@ Spree::Product.class_eval do attr_accessible :variant_unit, :variant_unit_scale, :variant_unit_name, :unit_value attr_accessible :inherits_properties, :sku - before_validation :sanitize_permalink - # validates_presence_of :variants, unless: :new_record?, message: "Product must have at least one variant" validates_presence_of :supplier validates :primary_taxon, presence: { message: "^Product Category can't be blank" } @@ -35,11 +33,13 @@ Spree::Product.class_eval do validates_presence_of :variant_unit_name, if: -> p { p.variant_unit == 'items' } - after_save :ensure_standard_variant after_initialize :set_available_on_to_now, :if => :new_record? - after_save :update_units - after_touch :touch_distributors + before_validation :sanitize_permalink before_save :add_primary_taxon_to_taxons + after_touch :touch_distributors + after_save :ensure_standard_variant + after_save :update_units + after_save :refresh_products_cache # -- Joins @@ -251,4 +251,8 @@ Spree::Product.class_eval do self.permalink = create_unique_permalink(requested.parameterize) end end + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.product_changed self + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index b5e83d7590..eae1a62d15 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -21,6 +21,10 @@ module OpenFoodNetwork end + def self.product_changed(product) + end + + private def self.exchanges_featuring_variant(variant) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 25913f679b..75180d8905 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -161,6 +161,19 @@ module Spree end end + describe "callbacks" do + let(:product) { create(:simple_product) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + product.name = 'asdf' + product.save + end + + # On destroy, all distributed variants are refreshed by a Variant around_destroy + # callback, so we don't need to do anything on the product model. + end + describe "scopes" do describe "in_supplier" do it "shows products in supplier" do From c98e44c5a1de6cd0b238e1dbcd09e7d2334cd388 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 28 Jan 2016 16:00:55 +1100 Subject: [PATCH 12/66] Perform refresh of products cache on product change --- lib/open_food_network/products_cache.rb | 11 +++++++---- .../open_food_network/products_cache_spec.rb | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index eae1a62d15..66a2c8b164 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -4,14 +4,14 @@ module OpenFoodNetwork # the products cache. class ProductsCache def self.variant_changed(variant) - exchanges_featuring_variant(variant).each do |exchange| + exchanges_featuring_variants(variant).each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end def self.variant_destroyed(variant, &block) - exchanges = exchanges_featuring_variant(variant).to_a + exchanges = exchanges_featuring_variants(variant).to_a block.call @@ -22,15 +22,18 @@ module OpenFoodNetwork def self.product_changed(product) + exchanges_featuring_variants(product.variants).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end end private - def self.exchanges_featuring_variant(variant) + def self.exchanges_featuring_variants(variants) Exchange. outgoing. - with_variant(variant). + with_any_variant(variants). joins(:order_cycle). merge(OrderCycle.dated). merge(OrderCycle.not_closed) diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index afc54ae87b..d48f5e7d5c 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -70,6 +70,25 @@ module OpenFoodNetwork end end + describe "when a product changes" do + let(:product) { create(:simple_product) } + let(:v1) { create(:variant, product: product) } + let(:v2) { create(:variant, product: product) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let(:oc) { create(:open_order_cycle) } + let!(:ex1) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, variants: [v1]) } + let!(:ex2) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d2, variants: [v1, v2]) } + + before { product.reload } + + it "refreshes the distribution each variant appears in, once each" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).once + ProductsCache.product_changed product + end + end + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } From 5f188650d8b66050a55349616d65f16b5a1334de Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 10:01:14 +1100 Subject: [PATCH 13/66] Trigger cache refresh on VariantOverride save/destroy --- app/models/variant_override.rb | 11 +++++++++++ lib/open_food_network/products_cache.rb | 8 ++++++++ spec/models/variant_override_spec.rb | 16 ++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index 21820ce0db..9935055b40 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -6,6 +6,10 @@ class VariantOverride < ActiveRecord::Base # Default stock can be nil, indicating stock should not be reset or zero, meaning reset to zero. Need to ensure this can be set by the user. validates :default_stock, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true + after_save :refresh_products_cache_from_save + after_destroy :refresh_products_cache_from_destroy + + scope :for_hubs, lambda { |hubs| where(hub_id: hubs) } @@ -73,4 +77,11 @@ class VariantOverride < ActiveRecord::Base VariantOverride.where(variant_id: variant, hub_id: hub).first end + def refresh_products_cache_from_save + OpenFoodNetwork::ProductsCache.variant_override_changed self + end + + def refresh_products_cache_from_destroy + OpenFoodNetwork::ProductsCache.variant_override_destroyed self + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 66a2c8b164..2666f7ffc7 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -28,6 +28,14 @@ module OpenFoodNetwork end + def self.variant_override_changed(variant_override) + end + + + def self.variant_override_destroyed(variant_override) + end + + private def self.exchanges_featuring_variants(variants) diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index ed2f2c00f4..1c008d05be 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -24,6 +24,22 @@ describe VariantOverride do end + describe "callbacks" do + let!(:vo) { create(:variant_override, hub: hub, variant: variant) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_override_changed).with(vo) + vo.price = 123.45 + vo.save + end + + it "refreshes the products cache on destroy" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_override_destroyed).with(vo) + vo.destroy + end + end + + describe "looking up prices" do it "returns the numeric price when present" do VariantOverride.create!(variant: variant, hub: hub, price: 12.34) From b7a88fd03b9c9fe7a3b39ff912abb445935a3de3 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 10:14:15 +1100 Subject: [PATCH 14/66] Perform refresh of products cache for variant override change --- lib/open_food_network/products_cache.rb | 11 +++++++++-- .../open_food_network/products_cache_spec.rb | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 2666f7ffc7..556b488316 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -29,6 +29,9 @@ module OpenFoodNetwork def self.variant_override_changed(variant_override) + exchanges_featuring_variants(variant_override.variant, distributor: variant_override.hub).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end end @@ -38,13 +41,17 @@ module OpenFoodNetwork private - def self.exchanges_featuring_variants(variants) - Exchange. + def self.exchanges_featuring_variants(variants, distributor: nil) + exchanges = Exchange. outgoing. with_any_variant(variants). joins(:order_cycle). merge(OrderCycle.dated). merge(OrderCycle.not_closed) + + exchanges = exchanges.to_enterprise(distributor) if distributor + + exchanges end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index d48f5e7d5c..ef3f3ed279 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -89,6 +89,25 @@ module OpenFoodNetwork end end + describe "when a variant override changes" do + let(:variant) { create(:variant) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let!(:vo) { create(:variant_override, variant: variant, hub: d1) } + let!(:oc) { create(:open_order_cycle, distributors: [d1, d2], variants: [variant]) } + + it "refreshes the distributions that the variant override affects" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.variant_override_changed vo + end + + it "does not refresh other distributors of the variant" do + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never + ProductsCache.variant_override_changed vo + end + end + + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } From 1ec329284c15d6004011f22d98fc5725f042a374 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 10:16:41 +1100 Subject: [PATCH 15/66] Refresh products cache when a variant override is destroyed --- lib/open_food_network/products_cache.rb | 1 + spec/lib/open_food_network/products_cache_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 556b488316..1d1dab8cd0 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -36,6 +36,7 @@ module OpenFoodNetwork def self.variant_override_destroyed(variant_override) + variant_override_changed variant_override end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index ef3f3ed279..2004ececaa 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -108,6 +108,16 @@ module OpenFoodNetwork end + describe "when a variant override is destroyed" do + let(:vo) { double(:variant_override) } + + it "performs the same refresh as a variant override change" do + expect(ProductsCache).to receive(:variant_override_changed).with(vo) + ProductsCache.variant_override_destroyed vo + end + end + + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } From fe41430d1ec43021c1f4b6e0d27328ba9ee56407 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 11:41:02 +1100 Subject: [PATCH 16/66] Rerefesh products cache when an order cycle is changed --- app/models/order_cycle.rb | 12 +++++ lib/open_food_network/products_cache.rb | 9 ++++ .../open_food_network/products_cache_spec.rb | 46 +++++++++++++++++++ spec/models/order_cycle_spec.rb | 18 +++++++- 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index b37503151e..d3f11860e9 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -11,6 +11,9 @@ class OrderCycle < ActiveRecord::Base validates_presence_of :name, :coordinator_id + after_save :refresh_products_cache + + scope :active, lambda { where('order_cycles.orders_open_at <= ? AND order_cycles.orders_close_at >= ?', Time.zone.now, Time.zone.now) } scope :active_or_complete, lambda { where('order_cycles.orders_open_at <= ?', Time.zone.now) } scope :inactive, lambda { where('order_cycles.orders_open_at > ? OR order_cycles.orders_close_at < ?', Time.zone.now, Time.zone.now) } @@ -183,6 +186,10 @@ class OrderCycle < ActiveRecord::Base self.variants.include? variant end + def dated? + !undated? + end + def undated? self.orders_open_at.nil? || self.orders_close_at.nil? end @@ -245,4 +252,9 @@ class OrderCycle < ActiveRecord::Base distributed_variants.include?(product.master) && (product.variants & distributed_variants).empty? end + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.order_cycle_changed self + end + end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 1d1dab8cd0..8c22c40db7 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -40,6 +40,15 @@ module OpenFoodNetwork end + def self.order_cycle_changed(order_cycle) + if order_cycle.dated? && !order_cycle.closed? + order_cycle.exchanges.outgoing.each do |exchange| + refresh_cache exchange.receiver, order_cycle + end + end + end + + private def self.exchanges_featuring_variants(variants, distributor: nil) diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 2004ececaa..c44d47fca2 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -118,6 +118,52 @@ module OpenFoodNetwork end + describe "when an order cycle is changed" do + let(:variant) { create(:variant) } + let(:s) { create(:supplier_enterprise) } + let(:c) { create(:distributor_enterprise) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let!(:oc_open) { create(:open_order_cycle, suppliers: [s], coordinator: c, distributors: [d1, d2], variants: [variant]) } + let!(:oc_upcoming) { create(:upcoming_order_cycle, suppliers: [s], coordinator: c, distributors: [d1, d2], variants: [variant]) } + + before do + oc_open.reload + oc_upcoming.reload + end + + it "updates each outgoing distribution in an upcoming order cycle" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc_upcoming).once + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc_upcoming).once + ProductsCache.order_cycle_changed oc_upcoming + end + + it "updates each outgoing distribution in an open order cycle" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc_open).once + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc_open).once + ProductsCache.order_cycle_changed oc_open + end + + it "does nothing when the order cycle has been made undated" do + expect(ProductsCache).to receive(:refresh_cache).never + oc_open.orders_open_at = oc_open.orders_close_at = nil + oc_open.save! + end + + it "does nothing when the order cycle has been closed" do + expect(ProductsCache).to receive(:refresh_cache).never + oc_open.orders_open_at = 2.weeks.ago + oc_open.orders_close_at = 1.week.ago + oc_open.save! + end + + it "does not update incoming exchanges" do + expect(ProductsCache).to receive(:refresh_cache).with(c, oc_open).never + ProductsCache.order_cycle_changed oc_open + end + end + + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index bad2d15312..6ef7062f8e 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -20,6 +20,18 @@ describe OrderCycle do oc.save! end + describe "products cache" do + let(:oc) { create(:open_order_cycle) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(oc) + oc.name = 'asdf' + oc.save + end + + # On destroy, we're removing distributions, so no updates to the products cache are required + end + it "has exchanges" do oc = create(:simple_order_cycle) @@ -330,6 +342,7 @@ describe OrderCycle do it "reports status when an order cycle is upcoming" do Timecop.freeze(oc.orders_open_at - 1.second) do oc.should_not be_undated + oc.should be_dated oc.should be_upcoming oc.should_not be_open oc.should_not be_closed @@ -338,6 +351,7 @@ describe OrderCycle do it "reports status when an order cycle is open" do oc.should_not be_undated + oc.should be_dated oc.should_not be_upcoming oc.should be_open oc.should_not be_closed @@ -346,6 +360,7 @@ describe OrderCycle do it "reports status when an order cycle has closed" do Timecop.freeze(oc.orders_close_at + 1.second) do oc.should_not be_undated + oc.should be_dated oc.should_not be_upcoming oc.should_not be_open oc.should be_closed @@ -355,7 +370,8 @@ describe OrderCycle do it "reports status when an order cycle is undated" do oc.update_attributes!(orders_open_at: nil, orders_close_at: nil) - oc.should be_undated + oc.should be_undated + oc.should_not be_dated oc.should_not be_upcoming oc.should_not be_open oc.should_not be_closed From 378a703cc3e9a572679c844c8e47863bfb20864f Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 12:24:04 +1100 Subject: [PATCH 17/66] Order cycles are undated unless they have both open and close dates defined --- spec/models/order_cycle_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 6ef7062f8e..9dc8a0cf91 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -55,7 +55,7 @@ describe OrderCycle do OrderCycle.upcoming.should == [oc_not_yet_open] OrderCycle.closed.should == [oc_already_closed] OrderCycle.undated.should == [oc_undated, oc_undated_open, oc_undated_close] - OrderCycle.not_closed.should == [oc_active, oc_not_yet_open, oc_undated] + OrderCycle.not_closed.should == [oc_active, oc_not_yet_open, oc_undated, oc_undated_open, oc_undated_close] OrderCycle.dated.should == [oc_active, oc_not_yet_open, oc_already_closed] end @@ -381,6 +381,7 @@ describe OrderCycle do oc.update_attributes!(orders_close_at: nil) oc.should be_undated + oc.should_not be_dated oc.should_not be_upcoming oc.should_not be_open oc.should_not be_closed @@ -390,6 +391,7 @@ describe OrderCycle do oc.update_attributes!(orders_open_at: nil) oc.should be_undated + oc.should_not be_dated oc.should_not be_upcoming oc.should_not be_open oc.should_not be_closed From 0c65d1ddd88a93f1bcf168f419bb5dd8a1b3692f Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 15:31:14 +1100 Subject: [PATCH 18/66] Trigger products cache refresh when enterprise fee changed or destroyed --- app/models/enterprise_fee.rb | 26 +++++++++++++++++++++++-- lib/open_food_network/products_cache.rb | 9 +++++++++ spec/models/enterprise_fee_spec.rb | 15 ++++++++++++-- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index e19f91d0f4..e2831a4178 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -2,10 +2,11 @@ class EnterpriseFee < ActiveRecord::Base belongs_to :enterprise belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' has_and_belongs_to_many :order_cycles, join_table: 'coordinator_fees' - has_many :exchange_fees, dependent: :destroy + has_many :exchange_fees has_many :exchanges, through: :exchange_fees - before_destroy { order_cycles.clear } + after_save :refresh_products_cache + around_destroy :destruction calculated_adjustments @@ -57,4 +58,25 @@ class EnterpriseFee < ActiveRecord::Base :mandatory => mandatory, :locked => true}, :without_protection => true) end + + + private + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.enterprise_fee_changed self + end + + def destruction + OpenFoodNetwork::ProductsCache.enterprise_fee_destroyed(self) do + # Remove this association here instead of using dependent: :destroy because + # dependent-destroy acts before this around_filter is called, so ProductsCache + # has no way of knowing where this fee was used. + order_cycles.clear + exchange_fees.destroy_all + + # Destroy the enterprise fee + yield + end + + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 8c22c40db7..858316f7f8 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -49,6 +49,15 @@ module OpenFoodNetwork end + def self.enterprise_fee_changed(enterprise_fee) + end + + + def self.enterprise_fee_destroyed(enterprise_fee, &block) + block.call + end + + private def self.exchanges_featuring_variants(variants, distributor: nil) diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index f0f9df1f0b..7140feda4a 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -10,8 +10,20 @@ describe EnterpriseFee do end describe "callbacks" do + let(:ef) { create(:enterprise_fee) } + + it "refreshes the products cache when saved" do + expect(OpenFoodNetwork::ProductsCache).to receive(:enterprise_fee_changed).with(ef) + ef.name = 'foo' + ef.save + end + + it "refreshes the products cache when destroyed" do + expect(OpenFoodNetwork::ProductsCache).to receive(:enterprise_fee_destroyed).with(ef) + ef.destroy + end + it "removes itself from order cycle coordinator fees when destroyed" do - ef = create(:enterprise_fee) oc = create(:simple_order_cycle, coordinator_fees: [ef]) ef.destroy @@ -19,7 +31,6 @@ describe EnterpriseFee do end it "removes itself from order cycle exchange fees when destroyed" do - ef = create(:enterprise_fee) oc = create(:simple_order_cycle) ex = create(:exchange, order_cycle: oc, enterprise_fees: [ef]) From af7e3380d3be4866e64649280a3392525abc5f61 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 15:35:26 +1100 Subject: [PATCH 19/66] Perform refresh of products cache when coordinator fee is changed --- lib/open_food_network/products_cache.rb | 8 ++++++++ .../open_food_network/products_cache_spec.rb | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 858316f7f8..639076fb82 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -50,6 +50,7 @@ module OpenFoodNetwork def self.enterprise_fee_changed(enterprise_fee) + refresh_coordinator_fee enterprise_fee end @@ -74,6 +75,13 @@ module OpenFoodNetwork end + def self.refresh_coordinator_fee(enterprise_fee) + enterprise_fee.order_cycles.each do |order_cycle| + order_cycle_changed order_cycle + end + end + + def self.refresh_cache(distributor, order_cycle) Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index c44d47fca2..26f3a9b844 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -164,6 +164,23 @@ module OpenFoodNetwork end + describe "when an enterprise fee is changed" do + let(:s) { create(:supplier_enterprise) } + let(:c) { create(:distributor_enterprise) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let(:ef) { create(:enterprise_fee) } + let(:ef_coord) { create(:enterprise_fee, order_cycles: [oc]) } + let(:oc) { create(:open_order_cycle, coordinator: c) } + + + it "updates order cycles when it's a coordinator fee" do + ef_coord + expect(ProductsCache).to receive(:order_cycle_changed).with(oc).once + ProductsCache.enterprise_fee_changed ef_coord + end + end + describe "refreshing the cache" do let(:distributor) { double(:distributor, id: 123) } let(:order_cycle) { double(:order_cycle, id: 456) } From fbedff4eca83bb336b7ffa3f7e370c3eaa0dab80 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 15:42:41 +1100 Subject: [PATCH 20/66] Perform refresh of products cache when distributor fee is changed --- lib/open_food_network/products_cache.rb | 14 ++++++ .../open_food_network/products_cache_spec.rb | 48 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 639076fb82..5013f5b37a 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -51,6 +51,7 @@ module OpenFoodNetwork def self.enterprise_fee_changed(enterprise_fee) refresh_coordinator_fee enterprise_fee + refresh_distributor_fee enterprise_fee end @@ -82,6 +83,19 @@ module OpenFoodNetwork end + def self.refresh_distributor_fee(enterprise_fee) + enterprise_fee.exchange_fees. + joins(:exchange => :order_cycle). + merge(Exchange.outgoing). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed). + each do |exf| + + refresh_cache exf.exchange.receiver, exf.exchange.order_cycle + end + end + + def self.refresh_cache(distributor, order_cycle) Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 26f3a9b844..c11565212e 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -179,6 +179,54 @@ module OpenFoodNetwork expect(ProductsCache).to receive(:order_cycle_changed).with(oc).once ProductsCache.enterprise_fee_changed ef_coord end + + + describe "updating exchanges when it's a distributor fee" do + let(:ex0) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, enterprise_fees: [ef]) } + let(:ex1) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, enterprise_fees: [ef]) } + let(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, enterprise_fees: []) } + + describe "updating distributions that include the fee" do + it "does not update undated order cycles" do + oc.update_attributes! orders_open_at: nil, orders_close_at: nil + ex1 + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.enterprise_fee_changed ef + end + + it "updates upcoming order cycles" do + oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now + ex1 + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.enterprise_fee_changed ef + end + + it "updates open order cycles" do + ex1 + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.enterprise_fee_changed ef + end + + it "does not update closed order cycles" do + oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago + ex1 + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.enterprise_fee_changed ef + end + end + + it "doesn't update exchanges that don't include the fee" do + ex1; ex2 + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never + ProductsCache.enterprise_fee_changed ef + end + + it "doesn't update incoming exchanges" do + ex0 + expect(ProductsCache).to receive(:refresh_cache).with(c, oc).never + ProductsCache.enterprise_fee_changed ef + end + end end describe "refreshing the cache" do From 3bcd3257a1f95a1c7e2e4ea6009d683301311e4a Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 29 Jan 2016 15:59:17 +1100 Subject: [PATCH 21/66] Perform refresh of products cache when supplier fee is changed --- lib/open_food_network/products_cache.rb | 29 ++++++++++++++ .../open_food_network/products_cache_spec.rb | 40 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 5013f5b37a..cdd4889f4c 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -50,6 +50,7 @@ module OpenFoodNetwork def self.enterprise_fee_changed(enterprise_fee) + refresh_supplier_fee enterprise_fee refresh_coordinator_fee enterprise_fee refresh_distributor_fee enterprise_fee end @@ -76,6 +77,19 @@ module OpenFoodNetwork end + def self.refresh_supplier_fee(enterprise_fee) + outgoing_exchanges = Set.new + + incoming_exchanges_for_enterprise_fee(enterprise_fee).each do |exchange| + outgoing_exchanges.merge outgoing_exchanges_with_variants(exchange.order_cycle, exchange.variant_ids) + end + + outgoing_exchanges.each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + def self.refresh_coordinator_fee(enterprise_fee) enterprise_fee.order_cycles.each do |order_cycle| order_cycle_changed order_cycle @@ -96,6 +110,21 @@ module OpenFoodNetwork end + def self.incoming_exchanges_for_enterprise_fee(enterprise_fee) + enterprise_fee.exchanges.incoming. + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + end + + + def self.outgoing_exchanges_with_variants(order_cycle, variant_ids) + order_cycle.exchanges.outgoing. + joins(:exchange_variants). + where('exchange_variants.variant_id IN (?)', variant_ids) + end + + def self.refresh_cache(distributor, order_cycle) Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index c11565212e..1ee8e032d6 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -174,6 +174,46 @@ module OpenFoodNetwork let(:oc) { create(:open_order_cycle, coordinator: c) } + describe "updating exchanges when it's a supplier fee" do + let(:v) { create(:variant) } + let!(:ex1) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, variants: [v], enterprise_fees: [ef]) } + let!(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, variants: [v]) } + let!(:ex3) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, variants: []) } + + before { ef.reload } + + describe "updating distributions that include one of the supplier's variants" do + it "does not update undated order cycles" do + oc.update_attributes! orders_open_at: nil, orders_close_at: nil + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.enterprise_fee_changed ef + end + + it "updates upcoming order cycles" do + oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.enterprise_fee_changed ef + end + + it "updates open order cycles" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.enterprise_fee_changed ef + end + + it "does not update closed order cycles" do + oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.enterprise_fee_changed ef + end + end + + it "doesn't update distributions that don't include any of the supplier's variants" do + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never + ProductsCache.enterprise_fee_changed ef + end + end + + it "updates order cycles when it's a coordinator fee" do ef_coord expect(ProductsCache).to receive(:order_cycle_changed).with(oc).once From f756749e02f05a7080e90c78e3fc7076b8ecc069 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 3 Feb 2016 12:28:26 +1100 Subject: [PATCH 22/66] Fix specs --- app/models/enterprise_fee.rb | 4 ++-- app/models/spree/variant_decorator.rb | 2 +- spec/jobs/refresh_products_cache_job_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index e2831a4178..0f4b3f8bc6 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -71,8 +71,8 @@ class EnterpriseFee < ActiveRecord::Base # Remove this association here instead of using dependent: :destroy because # dependent-destroy acts before this around_filter is called, so ProductsCache # has no way of knowing where this fee was used. - order_cycles.clear - exchange_fees.destroy_all + order_cycles(:reload).clear + exchange_fees(:reload).destroy_all # Destroy the enterprise fee yield diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 143508185a..b62b2440b2 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -100,7 +100,7 @@ Spree::Variant.class_eval do # Remove this association here instead of using dependent: :destroy because # dependent-destroy acts before this around_filter is called, so ProductsCache # has no way of knowing which exchanges the variant was a member of. - exchange_variants.destroy_all + exchange_variants(:reload).destroy_all # Destroy the variant yield diff --git a/spec/jobs/refresh_products_cache_job_spec.rb b/spec/jobs/refresh_products_cache_job_spec.rb index 2197ba38c9..d20efe5fad 100644 --- a/spec/jobs/refresh_products_cache_job_spec.rb +++ b/spec/jobs/refresh_products_cache_job_spec.rb @@ -6,11 +6,11 @@ describe RefreshProductsCacheJob do let(:order_cycle) { create(:simple_order_cycle) } it "renders products and writes them to cache" do - OpenFoodNetwork::ProductsRenderer.any_instance.stub(:products_json) { 'products' } + RefreshProductsCacheJob.any_instance.stub(:products_json) { 'products' } run_job RefreshProductsCacheJob.new distributor.id, order_cycle.id - expect(Rails.cache.read("products-json-123-456")).to eq 'products' + expect(Rails.cache.read("products-json-#{distributor.id}-#{order_cycle.id}")).to eq 'products' end describe "fetching products JSON" do From 7c4e9e58385e7e2415a3b08d5eb16c0dd2997001 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 3 Feb 2016 14:22:05 +1100 Subject: [PATCH 23/66] Refresh products cache when product properties are changed --- app/models/spree/product_decorator.rb | 9 ++++---- .../spree/product_property_decorator.rb | 10 +++++++++ app/models/spree/property_decorator.rb | 15 +++++++++++++ spec/models/spree/product_property_spec.rb | 21 +++++++++++++++++++ spec/models/spree/property_spec.rb | 17 +++++++++++++++ 5 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 app/models/spree/product_property_decorator.rb create mode 100644 app/models/spree/property_decorator.rb create mode 100644 spec/models/spree/product_property_spec.rb create mode 100644 spec/models/spree/property_spec.rb diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index e969dd500b..2c51ec5420 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -191,6 +191,11 @@ Spree::Product.class_eval do alias_method_chain :delete, :delete_from_order_cycles + def refresh_products_cache + OpenFoodNetwork::ProductsCache.product_changed self + end + + private def set_available_on_to_now @@ -251,8 +256,4 @@ Spree::Product.class_eval do self.permalink = create_unique_permalink(requested.parameterize) end end - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.product_changed self - end end diff --git a/app/models/spree/product_property_decorator.rb b/app/models/spree/product_property_decorator.rb new file mode 100644 index 0000000000..0f3329c03d --- /dev/null +++ b/app/models/spree/product_property_decorator.rb @@ -0,0 +1,10 @@ +module Spree + ProductProperty.class_eval do + after_save :refresh_products_cache + after_destroy :refresh_products_cache + + def refresh_products_cache + product.refresh_products_cache + end + end +end diff --git a/app/models/spree/property_decorator.rb b/app/models/spree/property_decorator.rb new file mode 100644 index 0000000000..5b8e53338c --- /dev/null +++ b/app/models/spree/property_decorator.rb @@ -0,0 +1,15 @@ +module Spree + Property.class_eval do + after_save :refresh_products_cache + + # When a Property is destroyed, dependent-destroy will destroy all ProductProperties, + # which will take care of refreshing the products cache + + + private + + def refresh_products_cache + product_properties(:reload).each &:refresh_products_cache + end + end +end diff --git a/spec/models/spree/product_property_spec.rb b/spec/models/spree/product_property_spec.rb new file mode 100644 index 0000000000..1c9799ab29 --- /dev/null +++ b/spec/models/spree/product_property_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +module Spree + describe ProductProperty do + describe "callbacks" do + let(:product) { product_property.product } + let(:product_property) { create(:product_property) } + + it "refreshes the products cache on save, via Product" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + product_property.value = 123 + product_property.save + end + + it "refreshes the products cache on destroy" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + product_property.destroy + end + end + end +end diff --git a/spec/models/spree/property_spec.rb b/spec/models/spree/property_spec.rb new file mode 100644 index 0000000000..a86513b5a9 --- /dev/null +++ b/spec/models/spree/property_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +module Spree + describe Property do + describe "callbacks" do + let(:property) { product_property.property } + let(:product) { product_property.product } + let(:product_property) { create(:product_property) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + property.name = 'asdf' + property.save + end + end + end +end From 6d80d91873a7c3b86e718d2ca63eb00a75d981c0 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 3 Feb 2016 14:53:55 +1100 Subject: [PATCH 24/66] Refresh products cache when taxons or classifications are changed or destroyed --- app/models/spree/classification_decorator.rb | 9 +++++++++ app/models/spree/taxon_decorator.rb | 12 ++++++++++++ spec/models/spree/classification_spec.rb | 17 +++++++++++++++-- spec/models/spree/taxon_spec.rb | 15 +++++++++++++++ 4 files changed, 51 insertions(+), 2 deletions(-) diff --git a/app/models/spree/classification_decorator.rb b/app/models/spree/classification_decorator.rb index c69eb01fcf..5e9655a907 100644 --- a/app/models/spree/classification_decorator.rb +++ b/app/models/spree/classification_decorator.rb @@ -1,6 +1,15 @@ Spree::Classification.class_eval do belongs_to :product, :class_name => "Spree::Product", touch: true + after_save :refresh_products_cache before_destroy :dont_destroy_if_primary_taxon + after_destroy :refresh_products_cache + + + private + + def refresh_products_cache + product.refresh_products_cache + end def dont_destroy_if_primary_taxon if product.primary_taxon == taxon diff --git a/app/models/spree/taxon_decorator.rb b/app/models/spree/taxon_decorator.rb index 1a26ce73a8..a1e07cc53e 100644 --- a/app/models/spree/taxon_decorator.rb +++ b/app/models/spree/taxon_decorator.rb @@ -1,7 +1,12 @@ Spree::Taxon.class_eval do + has_many :classifications, :dependent => :destroy + + self.attachment_definitions[:icon][:path] = 'public/images/spree/taxons/:id/:style/:basename.:extension' self.attachment_definitions[:icon][:url] = '/images/spree/taxons/:id/:style/:basename.:extension' + after_save :refresh_products_cache + # Indicate which filters should be used for this taxon def applicable_filters @@ -45,4 +50,11 @@ Spree::Taxon.class_eval do taxons end + + + private + + def refresh_products_cache + products(:reload).each &:refresh_products_cache + end end diff --git a/spec/models/spree/classification_spec.rb b/spec/models/spree/classification_spec.rb index f26f6da0c0..44449ee54e 100644 --- a/spec/models/spree/classification_spec.rb +++ b/spec/models/spree/classification_spec.rb @@ -2,8 +2,8 @@ require 'spec_helper' module Spree describe Classification do - let(:product) { create(:simple_product) } - let(:taxon) { create(:taxon) } + let!(:product) { create(:simple_product) } + let!(:taxon) { create(:taxon) } let(:classification) { create(:classification, taxon: taxon, product: product) } it "won't destroy if classification is the primary taxon" do @@ -11,5 +11,18 @@ module Spree classification.destroy.should be_false classification.errors.messages[:base].should == ["Taxon #{taxon.name} is the primary taxon of #{product.name} and cannot be deleted"] end + + describe "callbacks" do + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + classification + end + + it "refreshes the products cache on destroy" do + classification + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + classification.destroy + end + end end end diff --git a/spec/models/spree/taxon_spec.rb b/spec/models/spree/taxon_spec.rb index a0d729c054..926e5b3c5f 100644 --- a/spec/models/spree/taxon_spec.rb +++ b/spec/models/spree/taxon_spec.rb @@ -7,6 +7,21 @@ module Spree let(:t1) { create(:taxon) } let(:t2) { create(:taxon) } + describe "callbacks" do + let(:product) { create(:simple_product, taxons: [t1]) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + t1.name = 'asdf' + t1.save + end + + it "refreshes the products cache on destroy" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + t1.destroy + end + end + describe "finding all supplied taxons" do let!(:p1) { create(:simple_product, supplier: e, taxons: [t1, t2]) } From d8d803546b0b6c5b68f4eba2a685da2d9e90bce0 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 3 Feb 2016 15:34:23 +1100 Subject: [PATCH 25/66] Refresh products cache when master variants or images are changed or destroyed --- app/models/spree/image_decorator.rb | 10 ++++++++++ app/models/spree/variant_decorator.rb | 25 ++++++++++++++++++------- spec/models/spree/image_spec.rb | 18 ++++++++++++++++++ spec/models/spree/variant_spec.rb | 19 +++++++++++++++++++ 4 files changed, 65 insertions(+), 7 deletions(-) diff --git a/app/models/spree/image_decorator.rb b/app/models/spree/image_decorator.rb index 7d86aae3a6..24056926b7 100644 --- a/app/models/spree/image_decorator.rb +++ b/app/models/spree/image_decorator.rb @@ -1,4 +1,7 @@ Spree::Image.class_eval do + after_save :refresh_products_cache + after_destroy :refresh_products_cache + # Spree stores attachent definitions in JSON. This converts the style name and format to # strings. However, when paperclip encounters these, it doesn't recognise the format. # Here we solve that problem by converting format and style name to symbols. @@ -20,4 +23,11 @@ Spree::Image.class_eval do end reformat_styles + + + private + + def refresh_products_cache + viewable.try :refresh_products_cache + end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index b62b2440b2..9efa89795e 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -92,18 +92,29 @@ Spree::Variant.class_eval do end def refresh_products_cache - OpenFoodNetwork::ProductsCache.variant_changed self + if is_master? + product.refresh_products_cache + else + OpenFoodNetwork::ProductsCache.variant_changed self + end end def destruction - OpenFoodNetwork::ProductsCache.variant_destroyed(self) do - # Remove this association here instead of using dependent: :destroy because - # dependent-destroy acts before this around_filter is called, so ProductsCache - # has no way of knowing which exchanges the variant was a member of. + if is_master? exchange_variants(:reload).destroy_all - - # Destroy the variant yield + product.refresh_products_cache + + else + OpenFoodNetwork::ProductsCache.variant_destroyed(self) do + # Remove this association here instead of using dependent: :destroy because + # dependent-destroy acts before this around_filter is called, so ProductsCache + # has no way of knowing which exchanges the variant was a member of. + exchange_variants(:reload).destroy_all + + # Destroy the variant + yield + end end end end diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index 56665fa641..c9a9c1243c 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -14,5 +14,23 @@ module Spree Image.format_styles(formatted).should == {:mini => ["48x48>", :png]} end end + + describe "callbacks" do + let!(:product) { create(:simple_product) } + + let!(:image_file) { File.open("#{Rails.root}/app/assets/images/logo-white.png") } + let!(:image) { Image.create(viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "image", attachment: image_file) } + + it "refreshes the products cache when changed" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + image.alt = 'asdf' + image.save + end + + it "refreshes the products cache when destroyed" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + image.destroy + end + end end end diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 73789e62c2..11104a02e2 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -126,6 +126,25 @@ module Spree expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) variant.destroy end + + context "when it is the master variant" do + let(:product) { create(:simple_product) } + let(:master) { product.master } + + it "refreshes the products cache for the entire product on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).never + master.sku = 'abc123' + master.save + end + + it "refreshes the products cache for the entire product on destroy" do + # Does this ever happen? + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).never + master.destroy + end + end end describe "indexing variants by id" do From 339f3fc2f0a07db1eb9875ce34d8526557517f3b Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 3 Feb 2016 16:08:41 +1100 Subject: [PATCH 26/66] Refresh products cache when price is changed or destroyed --- app/models/spree/price_decorator.rb | 13 +++++++++++++ app/models/spree/variant_decorator.rb | 13 +++++++------ spec/models/spree/price_spec.rb | 21 +++++++++++++++++++++ 3 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 app/models/spree/price_decorator.rb create mode 100644 spec/models/spree/price_spec.rb diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb new file mode 100644 index 0000000000..340a6aa17a --- /dev/null +++ b/app/models/spree/price_decorator.rb @@ -0,0 +1,13 @@ +module Spree + Price.class_eval do + after_save :refresh_products_cache + after_destroy :refresh_products_cache + + + private + + def refresh_products_cache + variant.refresh_products_cache + end + end +end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 9efa89795e..7be9d712d0 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -85,12 +85,6 @@ Spree::Variant.class_eval do end end - private - - def update_weight_from_unit_value - self.weight = weight_from_unit_value if self.product.variant_unit == 'weight' && unit_value.present? - end - def refresh_products_cache if is_master? product.refresh_products_cache @@ -99,6 +93,13 @@ Spree::Variant.class_eval do end end + + private + + def update_weight_from_unit_value + self.weight = weight_from_unit_value if self.product.variant_unit == 'weight' && unit_value.present? + end + def destruction if is_master? exchange_variants(:reload).destroy_all diff --git a/spec/models/spree/price_spec.rb b/spec/models/spree/price_spec.rb new file mode 100644 index 0000000000..46f6653c77 --- /dev/null +++ b/spec/models/spree/price_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +module Spree + describe Price do + describe "callbacks" do + let(:variant) { create(:variant) } + let(:price) { variant.default_price } + + it "refreshes the products cache on change" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + price.amount = 123 + price.save + end + + it "refreshes the products cache on destroy" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + price.destroy + end + end + end +end From d0b7b4ee50f2a34ae5b3f7304db5de2817ff6bce Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 11:22:37 +1100 Subject: [PATCH 27/66] Add CachedProductsRenderer - wraps ProductsRenderer using Rails cache --- .../cached_products_renderer.rb | 35 ++++++++ .../cached_products_renderer_spec.rb | 80 +++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 lib/open_food_network/cached_products_renderer.rb create mode 100644 spec/lib/open_food_network/cached_products_renderer_spec.rb diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb new file mode 100644 index 0000000000..9a42f4e5e4 --- /dev/null +++ b/lib/open_food_network/cached_products_renderer.rb @@ -0,0 +1,35 @@ +require 'open_food_network/products_renderer' + +# Wrapper for ProductsRenderer that caches the JSON output. +# ProductsRenderer::NoProducts is represented in the cache as nil, +# but re-raised to provide the same interface as ProductsRenderer. + +module OpenFoodNetwork + class CachedProductsRenderer + def initialize(distributor, order_cycle) + @distributor = distributor + @order_cycle = order_cycle + end + + def products_json + products_json = Rails.cache.fetch("products-json-#{@distributor.id}-#{@order_cycle.id}") do + begin + uncached_products_json + rescue ProductsRenderer::NoProducts + nil + end + end + + raise ProductsRenderer::NoProducts.new if products_json.nil? + + products_json + end + + + private + + def uncached_products_json + ProductsRenderer.new(@distributor, @order_cycle).products_json + end + end +end diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb new file mode 100644 index 0000000000..6f52dee2cd --- /dev/null +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' +require 'open_food_network/cached_products_renderer' +require 'open_food_network/products_renderer' + +module OpenFoodNetwork + describe CachedProductsRenderer do + let(:distributor) { double(:distributor, id: 123) } + let(:order_cycle) { double(:order_cycle, id: 456) } + let(:cpr) { CachedProductsRenderer.new(distributor, order_cycle) } + + describe "when the products JSON is already cached" do + before do + Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", 'products' + end + + it "returns the cached JSON" do + expect(cpr.products_json).to eq 'products' + end + + it "raises an exception when there are no products" do + Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", nil + expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + end + end + + describe "when the products JSON is not cached" do + let(:cached_json) { Rails.cache.read "products-json-#{distributor.id}-#{order_cycle.id}" } + let(:cache_present) { Rails.cache.exist? "products-json-#{distributor.id}-#{order_cycle.id}" } + + before do + Rails.cache.clear + cpr.stub(:uncached_products_json) { 'fresh products' } + end + + describe "when there are products" do + it "returns products as JSON" do + expect(cpr.products_json).to eq 'fresh products' + end + + it "caches the JSON" do + cpr.products_json + expect(cached_json).to eq 'fresh products' + end + end + + describe "when there are no products" do + before { cpr.stub(:uncached_products_json).and_raise ProductsRenderer::NoProducts } + + it "raises an error" do + expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + end + + it "caches the products as nil" do + expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + expect(cache_present).to be + expect(cached_json).to be_nil + end + end + + describe "logging a warning" do + it "logs a warning when in production" + it "logs a warning when in staging" + it "does not log a warning in development" + it "does not log a warning in test" + end + end + + describe "fetching uncached products from ProductsRenderer" do + let(:pr) { double(:products_renderer, products_json: 'uncached products') } + + before do + ProductsRenderer.stub(:new) { pr } + end + + it "returns the uncached products" do + expect(cpr.send(:uncached_products_json)).to eq 'uncached products' + end + end + end +end From ff493c21d40b3c3a0c5b7bae3b0731b07ce3e1b8 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 11:33:59 +1100 Subject: [PATCH 28/66] Log a warning on cache MISS --- .../cached_products_renderer.rb | 8 +++++ .../cached_products_renderer_spec.rb | 33 ++++++++++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb index 9a42f4e5e4..ef11dfaba5 100644 --- a/lib/open_food_network/cached_products_renderer.rb +++ b/lib/open_food_network/cached_products_renderer.rb @@ -13,6 +13,8 @@ module OpenFoodNetwork def products_json products_json = Rails.cache.fetch("products-json-#{@distributor.id}-#{@order_cycle.id}") do + log_warning + begin uncached_products_json rescue ProductsRenderer::NoProducts @@ -28,6 +30,12 @@ module OpenFoodNetwork private + def log_warning + if Rails.env.production? || Rails.env.staging? + Bugsnag.notify RuntimeError.new("Live server MISS on products cache for distributor: #{@distributor.id}, order cycle: #{@order_cycle.id}") + end + end + def uncached_products_json ProductsRenderer.new(@distributor, @order_cycle).products_json end diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index 6f52dee2cd..9983e79e16 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -41,6 +41,11 @@ module OpenFoodNetwork cpr.products_json expect(cached_json).to eq 'fresh products' end + + it "logs a warning" do + cpr.should_receive :log_warning + cpr.products_json + end end describe "when there are no products" do @@ -55,13 +60,31 @@ module OpenFoodNetwork expect(cache_present).to be expect(cached_json).to be_nil end + + it "logs a warning" do + cpr.should_receive :log_warning + expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + end + end + end + + describe "logging a warning" do + it "logs a warning when in production" do + Rails.env.stub(:production?) { true } + expect(Bugsnag).to receive(:notify) + cpr.send(:log_warning) end - describe "logging a warning" do - it "logs a warning when in production" - it "logs a warning when in staging" - it "does not log a warning in development" - it "does not log a warning in test" + it "logs a warning when in staging" do + Rails.env.stub(:production?) { false } + Rails.env.stub(:staging?) { true } + expect(Bugsnag).to receive(:notify) + cpr.send(:log_warning) + end + + it "does not log a warning in development or test" do + expect(Bugsnag).to receive(:notify).never + cpr.send(:log_warning) end end From 235c4638498f5e2a08ec137555ac02ebd1eb4409 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 11:38:09 +1100 Subject: [PATCH 29/66] Hide wrapped exception, too --- lib/open_food_network/cached_products_renderer.rb | 4 +++- .../open_food_network/cached_products_renderer_spec.rb | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb index ef11dfaba5..eef3d000b2 100644 --- a/lib/open_food_network/cached_products_renderer.rb +++ b/lib/open_food_network/cached_products_renderer.rb @@ -6,6 +6,8 @@ require 'open_food_network/products_renderer' module OpenFoodNetwork class CachedProductsRenderer + class NoProducts < Exception; end + def initialize(distributor, order_cycle) @distributor = distributor @order_cycle = order_cycle @@ -22,7 +24,7 @@ module OpenFoodNetwork end end - raise ProductsRenderer::NoProducts.new if products_json.nil? + raise NoProducts.new if products_json.nil? products_json end diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index 9983e79e16..a203d66c1d 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -19,7 +19,7 @@ module OpenFoodNetwork it "raises an exception when there are no products" do Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", nil - expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts end end @@ -52,18 +52,18 @@ module OpenFoodNetwork before { cpr.stub(:uncached_products_json).and_raise ProductsRenderer::NoProducts } it "raises an error" do - expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts end it "caches the products as nil" do - expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts expect(cache_present).to be expect(cached_json).to be_nil end it "logs a warning" do cpr.should_receive :log_warning - expect { cpr.products_json }.to raise_error ProductsRenderer::NoProducts + expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts end end end From fa543fed63813cc2c61f90595ecea75e88cb504b Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 11:41:56 +1100 Subject: [PATCH 30/66] Deal with unset distribution --- lib/open_food_network/cached_products_renderer.rb | 2 ++ .../open_food_network/cached_products_renderer_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb index eef3d000b2..2174bf4094 100644 --- a/lib/open_food_network/cached_products_renderer.rb +++ b/lib/open_food_network/cached_products_renderer.rb @@ -14,6 +14,8 @@ module OpenFoodNetwork end def products_json + raise NoProducts.new if @distributor.nil? || @order_cycle.nil? + products_json = Rails.cache.fetch("products-json-#{@distributor.id}-#{@order_cycle.id}") do log_warning diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index a203d66c1d..03b0ab05d5 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -8,6 +8,14 @@ module OpenFoodNetwork let(:order_cycle) { double(:order_cycle, id: 456) } let(:cpr) { CachedProductsRenderer.new(distributor, order_cycle) } + describe "when the distribution is not set" do + let(:cpr) { CachedProductsRenderer.new(nil, nil) } + + it "raises an exception and returns no products" do + expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts + end + end + describe "when the products JSON is already cached" do before do Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", 'products' From 2f602f2a57d9cc602970ae5f3a0183b57c28986c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 11:42:25 +1100 Subject: [PATCH 31/66] Shop controller uses CachedProductsRenderer --- app/controllers/shop_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index d1c7e7582e..55b6bcc02b 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -1,4 +1,4 @@ -require 'open_food_network/products_renderer' +require 'open_food_network/cached_products_renderer' class ShopController < BaseController layout "darkswarm" @@ -11,11 +11,11 @@ class ShopController < BaseController def products begin - products_json = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle).products_json + products_json = OpenFoodNetwork::CachedProductsRenderer.new(current_distributor, current_order_cycle).products_json render json: products_json - rescue OpenFoodNetwork::ProductsRenderer::NoProducts + rescue OpenFoodNetwork::CachedProductsRenderer::NoProducts render status: 404, json: '' end end From f78826c9c7f376f5f98e83582a1328ceb55695ea Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 12:05:42 +1100 Subject: [PATCH 32/66] Fix rare case where price is saved without variant --- app/models/spree/price_decorator.rb | 2 +- spec/models/spree/price_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb index 340a6aa17a..0dd3a5a0c6 100644 --- a/app/models/spree/price_decorator.rb +++ b/app/models/spree/price_decorator.rb @@ -7,7 +7,7 @@ module Spree private def refresh_products_cache - variant.refresh_products_cache + variant.andand.refresh_products_cache end end end diff --git a/spec/models/spree/price_spec.rb b/spec/models/spree/price_spec.rb index 46f6653c77..378925f052 100644 --- a/spec/models/spree/price_spec.rb +++ b/spec/models/spree/price_spec.rb @@ -16,6 +16,12 @@ module Spree expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) price.destroy end + + it "does not refresh the cache when variant is not set" do + # Creates a price without the back link to variant + create(:product, master: create(:variant)) + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).never + end end end end From 62c6530ca93319fd88d36e01f463762bbd246748 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 12:15:59 +1100 Subject: [PATCH 33/66] Do not refresh products cache when price destroyed - variant destruction is main (only?) trigger, it causes refresh --- app/models/spree/price_decorator.rb | 1 - spec/models/spree/price_spec.rb | 6 ++---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb index 0dd3a5a0c6..f50a86066b 100644 --- a/app/models/spree/price_decorator.rb +++ b/app/models/spree/price_decorator.rb @@ -1,7 +1,6 @@ module Spree Price.class_eval do after_save :refresh_products_cache - after_destroy :refresh_products_cache private diff --git a/spec/models/spree/price_spec.rb b/spec/models/spree/price_spec.rb index 378925f052..5d48afd95b 100644 --- a/spec/models/spree/price_spec.rb +++ b/spec/models/spree/price_spec.rb @@ -12,10 +12,8 @@ module Spree price.save end - it "refreshes the products cache on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - price.destroy - end + # Do not refresh on price destruction - this (only?) happens when variant is destroyed, + # and in that case the variant will take responsibility for refreshing the cache it "does not refresh the cache when variant is not set" do # Creates a price without the back link to variant From 540687515e6e46e510abb898e072ea8e325a5798 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 12:29:16 +1100 Subject: [PATCH 34/66] Reify coordinator_fees HABTM join table as CoordinatorFee model using HMT --- app/models/coordinator_fee.rb | 4 ++++ app/models/enterprise_fee.rb | 7 ++++++- app/models/order_cycle.rb | 4 +++- db/migrate/20160204013914_add_id_to_coordinator_fees.rb | 5 +++++ db/schema.rb | 4 ++-- 5 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 app/models/coordinator_fee.rb create mode 100644 db/migrate/20160204013914_add_id_to_coordinator_fees.rb diff --git a/app/models/coordinator_fee.rb b/app/models/coordinator_fee.rb new file mode 100644 index 0000000000..f15a194158 --- /dev/null +++ b/app/models/coordinator_fee.rb @@ -0,0 +1,4 @@ +class CoordinatorFee < ActiveRecord::Base + belongs_to :order_cycle + belongs_to :enterprise_fee +end diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 0f4b3f8bc6..8e45707d07 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -1,13 +1,18 @@ class EnterpriseFee < ActiveRecord::Base belongs_to :enterprise belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' - has_and_belongs_to_many :order_cycles, join_table: 'coordinator_fees' + + has_many :coordinator_fees + has_many :order_cycles, through: :coordinator_fees + has_many :exchange_fees has_many :exchanges, through: :exchange_fees + after_save :refresh_products_cache around_destroy :destruction + calculated_adjustments attr_accessible :enterprise_id, :fee_type, :name, :tax_category_id, :calculator_type diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index d3f11860e9..8d0fa83da8 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -1,6 +1,8 @@ class OrderCycle < ActiveRecord::Base belongs_to :coordinator, :class_name => 'Enterprise' - has_and_belongs_to_many :coordinator_fees, :class_name => 'EnterpriseFee', :join_table => 'coordinator_fees' + + has_many :coordinator_fee_refs, class_name: 'CoordinatorFee' + has_many :coordinator_fees, through: :coordinator_fee_refs, source: :enterprise_fee has_many :exchanges, :dependent => :destroy diff --git a/db/migrate/20160204013914_add_id_to_coordinator_fees.rb b/db/migrate/20160204013914_add_id_to_coordinator_fees.rb new file mode 100644 index 0000000000..74326a6006 --- /dev/null +++ b/db/migrate/20160204013914_add_id_to_coordinator_fees.rb @@ -0,0 +1,5 @@ +class AddIdToCoordinatorFees < ActiveRecord::Migration + def change + add_column :coordinator_fees, :id, :primary_key + end +end diff --git a/db/schema.rb b/db/schema.rb index 465530b949..9912a89c63 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20151128185900) do +ActiveRecord::Schema.define(:version => 20160204013914) do create_table "account_invoices", :force => true do |t| t.integer "user_id", :null => false @@ -176,7 +176,7 @@ ActiveRecord::Schema.define(:version => 20151128185900) do add_index "cms_snippets", ["site_id", "identifier"], :name => "index_cms_snippets_on_site_id_and_identifier", :unique => true add_index "cms_snippets", ["site_id", "position"], :name => "index_cms_snippets_on_site_id_and_position" - create_table "coordinator_fees", :id => false, :force => true do |t| + create_table "coordinator_fees", :force => true do |t| t.integer "order_cycle_id" t.integer "enterprise_fee_id" end From 0a90a48b04a6da2f50e55306885c2796b4a4f31d Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 12:45:05 +1100 Subject: [PATCH 35/66] Refresh products cache when coordinator fees are changed or destroyed --- app/models/coordinator_fee.rb | 11 +++++++++++ app/models/order_cycle.rb | 9 ++++----- spec/models/coordinator_fee_spec.rb | 19 +++++++++++++++++++ 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 spec/models/coordinator_fee_spec.rb diff --git a/app/models/coordinator_fee.rb b/app/models/coordinator_fee.rb index f15a194158..135ee821a9 100644 --- a/app/models/coordinator_fee.rb +++ b/app/models/coordinator_fee.rb @@ -1,4 +1,15 @@ class CoordinatorFee < ActiveRecord::Base belongs_to :order_cycle belongs_to :enterprise_fee + + after_save :refresh_products_cache + after_destroy :refresh_products_cache + + + private + + def refresh_products_cache + order_cycle.refresh_products_cache + end + end diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 8d0fa83da8..58ec1bdfa0 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -241,6 +241,10 @@ class OrderCycle < ActiveRecord::Base coordinator.users.include? user end + def refresh_products_cache + OpenFoodNetwork::ProductsCache.order_cycle_changed self + end + private @@ -254,9 +258,4 @@ class OrderCycle < ActiveRecord::Base distributed_variants.include?(product.master) && (product.variants & distributed_variants).empty? end - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.order_cycle_changed self - end - end diff --git a/spec/models/coordinator_fee_spec.rb b/spec/models/coordinator_fee_spec.rb new file mode 100644 index 0000000000..2a54a0e96c --- /dev/null +++ b/spec/models/coordinator_fee_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe CoordinatorFee do + describe "products caching" do + let(:order_cycle) { create(:simple_order_cycle) } + let(:enterprise_fee) { create(:enterprise_fee) } + + it "refreshes the products cache on change" do + expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(order_cycle) + order_cycle.coordinator_fees << enterprise_fee + end + + it "refreshes the products cache on destruction" do + order_cycle.coordinator_fees << enterprise_fee + expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(order_cycle) + order_cycle.coordinator_fee_refs.first.destroy + end + end +end From a64a501dbb2600190f78b8ac1ed18cb08b196f55 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 4 Feb 2016 15:15:49 +1100 Subject: [PATCH 36/66] Smarter job queuing: Do not enqueue a RefreshProductsCacheJob if there's already one waiting to run --- lib/open_food_network/products_cache.rb | 9 +-- .../products_cache_refreshment.rb | 47 ++++++++++++++ .../products_cache_refreshment_spec.rb | 62 +++++++++++++++++++ .../open_food_network/products_cache_spec.rb | 11 ++-- 4 files changed, 119 insertions(+), 10 deletions(-) create mode 100644 lib/open_food_network/products_cache_refreshment.rb create mode 100644 spec/lib/open_food_network/products_cache_refreshment_spec.rb diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index cdd4889f4c..50a6099f4b 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -1,7 +1,8 @@ -module OpenFoodNetwork +require 'open_food_network/products_cache_refreshment' - # When elements of the data model change, enqueue jobs to refresh the appropriate parts of - # the products cache. +# When elements of the data model change, refresh the appropriate parts of the products cache. + +module OpenFoodNetwork class ProductsCache def self.variant_changed(variant) exchanges_featuring_variants(variant).each do |exchange| @@ -126,7 +127,7 @@ module OpenFoodNetwork def self.refresh_cache(distributor, order_cycle) - Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + ProductsCacheRefreshment.refresh distributor, order_cycle end end end diff --git a/lib/open_food_network/products_cache_refreshment.rb b/lib/open_food_network/products_cache_refreshment.rb new file mode 100644 index 0000000000..68ed5365c1 --- /dev/null +++ b/lib/open_food_network/products_cache_refreshment.rb @@ -0,0 +1,47 @@ +# When enqueuing a job to refresh the products cache for a particular distribution, there +# is no benefit in having more than one job waiting in the queue to be run. + +# Imagine that an admin updates a product. This calls for the products cache to be +# updated, otherwise customers will see stale data. + +# Now while that update is running, the admin makes another change to the product. Since this change +# has been made after the previous update started running, the already-running update will not +# include that change - we need another job. So we enqueue another one. + +# Before that job starts running, our zealous admin makes yet another change. This time, there +# is a job running *and* there is a job that has not yet started to run. In this case, there's no +# benefit in enqueuing another job. When the previously enqueued job starts running, it will pick up +# our admin's update and include it. So we ignore this change (from a cache refreshment perspective) +# and go home happy to have saved our job worker's time. + +module OpenFoodNetwork + class ProductsCacheRefreshment + def self.refresh(distributor, order_cycle) + unless pending_job? distributor, order_cycle + enqueue_job distributor, order_cycle + end + end + + + private + + def self.pending_job?(distributor, order_cycle) + # To inspect each job, we need to deserialize the payload. + # This is slow, and if it's a problem in practice, we could pre-filter in SQL + # for handlers matching the class name, distributor id and order cycle id. + + Delayed::Job. + where(locked_at: nil). + map(&:payload_object). + select { |j| + j.class == RefreshProductsCacheJob && + j.distributor_id == distributor.id && + j.order_cycle_id == order_cycle.id + }.any? + end + + def self.enqueue_job(distributor, order_cycle) + Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + end + end +end diff --git a/spec/lib/open_food_network/products_cache_refreshment_spec.rb b/spec/lib/open_food_network/products_cache_refreshment_spec.rb new file mode 100644 index 0000000000..bb033cd041 --- /dev/null +++ b/spec/lib/open_food_network/products_cache_refreshment_spec.rb @@ -0,0 +1,62 @@ +require 'open_food_network/products_cache_refreshment' + +module OpenFoodNetwork + describe ProductsCacheRefreshment do + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle) } + + before { Delayed::Job.destroy_all } + + describe "when there are no tasks enqueued" do + it "enqueues the task" do + expect do + ProductsCacheRefreshment.refresh distributor, order_cycle + end.to enqueue_job RefreshProductsCacheJob + end + end + + describe "when there is an enqueued task, and it is running" do + before do + job = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + job.update_attributes! locked_by: 'asdf', locked_at: Time.now + end + + it "enqueues another task" do + expect do + ProductsCacheRefreshment.refresh distributor, order_cycle + end.to enqueue_job RefreshProductsCacheJob + end + end + + describe "when there are two enqueued tasks, and one is running" do + before do + job1 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + job1.update_attributes! locked_by: 'asdf', locked_at: Time.now + job2 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + end + + it "does not enqueue another task" do + expect do + ProductsCacheRefreshment.refresh distributor, order_cycle + end.not_to enqueue_job RefreshProductsCacheJob + end + end + + describe "enqueuing tasks with different distributions" do + let(:distributor2) { create(:distributor_enterprise) } + let(:order_cycle2) { create(:simple_order_cycle) } + + before do + job1 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + job1.update_attributes! locked_by: 'asdf', locked_at: Time.now + job2 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + end + + it "ignores tasks with differing distributions when choosing whether to enqueue a job" do + expect do + ProductsCacheRefreshment.refresh distributor2, order_cycle2 + end.to enqueue_job RefreshProductsCacheJob + end + end + end +end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 1ee8e032d6..56b557a7ee 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -270,13 +270,12 @@ module OpenFoodNetwork end describe "refreshing the cache" do - let(:distributor) { double(:distributor, id: 123) } - let(:order_cycle) { double(:order_cycle, id: 456) } + let(:distributor) { double(:distributor) } + let(:order_cycle) { double(:order_cycle) } - it "enqueues a RefreshProductsCacheJob" do - expect do - ProductsCache.send(:refresh_cache, distributor, order_cycle) - end.to enqueue_job RefreshProductsCacheJob, distributor_id: distributor.id, order_cycle_id: order_cycle.id + it "notifies ProductsCacheRefreshment" do + expect(ProductsCacheRefreshment).to receive(:refresh).with(distributor, order_cycle) + ProductsCache.send(:refresh_cache, distributor, order_cycle) end end end From 8bd5a36aaff4e020f7cdb414f0305b08e3f2947d Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 09:14:49 +1100 Subject: [PATCH 37/66] Remove enterprise fee destruction cache callback - responsibility to be handled by dependent models --- app/models/enterprise_fee.rb | 21 ++++----------------- lib/open_food_network/products_cache.rb | 5 ----- spec/models/enterprise_fee_spec.rb | 5 ----- 3 files changed, 4 insertions(+), 27 deletions(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 8e45707d07..6e26b9f5fa 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -2,15 +2,16 @@ class EnterpriseFee < ActiveRecord::Base belongs_to :enterprise belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' - has_many :coordinator_fees + has_many :coordinator_fees, dependent: :destroy has_many :order_cycles, through: :coordinator_fees - has_many :exchange_fees + has_many :exchange_fees, dependent: :destroy has_many :exchanges, through: :exchange_fees after_save :refresh_products_cache - around_destroy :destruction + # After destroy, the products cache is refreshed via the after_destroy hook for + # coordinator_fees and exchange_fees calculated_adjustments @@ -70,18 +71,4 @@ class EnterpriseFee < ActiveRecord::Base def refresh_products_cache OpenFoodNetwork::ProductsCache.enterprise_fee_changed self end - - def destruction - OpenFoodNetwork::ProductsCache.enterprise_fee_destroyed(self) do - # Remove this association here instead of using dependent: :destroy because - # dependent-destroy acts before this around_filter is called, so ProductsCache - # has no way of knowing where this fee was used. - order_cycles(:reload).clear - exchange_fees(:reload).destroy_all - - # Destroy the enterprise fee - yield - end - - end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 50a6099f4b..d01bcf86e3 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -57,11 +57,6 @@ module OpenFoodNetwork end - def self.enterprise_fee_destroyed(enterprise_fee, &block) - block.call - end - - private def self.exchanges_featuring_variants(variants, distributor: nil) diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 7140feda4a..109e5baac7 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -18,11 +18,6 @@ describe EnterpriseFee do ef.save end - it "refreshes the products cache when destroyed" do - expect(OpenFoodNetwork::ProductsCache).to receive(:enterprise_fee_destroyed).with(ef) - ef.destroy - end - it "removes itself from order cycle coordinator fees when destroyed" do oc = create(:simple_order_cycle, coordinator_fees: [ef]) From 146797ea613eea7b884be3ea307fe3f21c6f33d2 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 09:37:10 +1100 Subject: [PATCH 38/66] Generalise method for reuse --- lib/open_food_network/products_cache.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index d01bcf86e3..b1d63c0b9a 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -76,7 +76,7 @@ module OpenFoodNetwork def self.refresh_supplier_fee(enterprise_fee) outgoing_exchanges = Set.new - incoming_exchanges_for_enterprise_fee(enterprise_fee).each do |exchange| + incoming_exchanges(enterprise_fee.exchanges).each do |exchange| outgoing_exchanges.merge outgoing_exchanges_with_variants(exchange.order_cycle, exchange.variant_ids) end @@ -106,8 +106,9 @@ module OpenFoodNetwork end - def self.incoming_exchanges_for_enterprise_fee(enterprise_fee) - enterprise_fee.exchanges.incoming. + def self.incoming_exchanges(exchanges) + exchanges. + incoming. joins(:order_cycle). merge(OrderCycle.dated). merge(OrderCycle.not_closed) From 8af6866ae4775fc614fe7ab72d7500fd575d6adf Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 10:37:05 +1100 Subject: [PATCH 39/66] Refresh products cache when exchange is changed or destroyed --- app/models/exchange.rb | 10 +++ lib/open_food_network/products_cache.rb | 38 ++++++++--- .../open_food_network/products_cache_spec.rb | 65 +++++++++++++++++++ spec/models/exchange_spec.rb | 15 +++++ 4 files changed, 120 insertions(+), 8 deletions(-) diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 137924f7ac..94aada7e0e 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -13,6 +13,9 @@ class Exchange < ActiveRecord::Base validates_presence_of :order_cycle, :sender, :receiver validates_uniqueness_of :sender_id, :scope => [:order_cycle_id, :receiver_id, :incoming] + after_save :refresh_products_cache + after_destroy :refresh_products_cache_from_destroy + accepts_nested_attributes_for :variants scope :in_order_cycle, lambda { |order_cycle| where(order_cycle_id: order_cycle) } @@ -75,4 +78,11 @@ class Exchange < ActiveRecord::Base end end + def refresh_products_cache + OpenFoodNetwork::ProductsCache.exchange_changed self + end + + def refresh_products_cache_from_destroy + OpenFoodNetwork::ProductsCache.exchange_destroyed self + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index b1d63c0b9a..d1ce117734 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -50,6 +50,20 @@ module OpenFoodNetwork end + def self.exchange_changed(exchange) + if exchange.incoming + refresh_incoming_exchanges(Exchange.where(id: exchange)) + else + refresh_outgoing_exchange(exchange) + end + end + + + def self.exchange_destroyed(exchange) + exchange_changed exchange + end + + def self.enterprise_fee_changed(enterprise_fee) refresh_supplier_fee enterprise_fee refresh_coordinator_fee enterprise_fee @@ -73,19 +87,27 @@ module OpenFoodNetwork end - def self.refresh_supplier_fee(enterprise_fee) - outgoing_exchanges = Set.new - - incoming_exchanges(enterprise_fee.exchanges).each do |exchange| - outgoing_exchanges.merge outgoing_exchanges_with_variants(exchange.order_cycle, exchange.variant_ids) - end - - outgoing_exchanges.each do |exchange| + def self.refresh_incoming_exchanges(exchanges) + incoming_exchanges(exchanges).map do |exchange| + outgoing_exchanges_with_variants(exchange.order_cycle, exchange.variant_ids) + end.flatten.uniq.each do |exchange| refresh_cache exchange.receiver, exchange.order_cycle end end + def self.refresh_outgoing_exchange(exchange) + if exchange.order_cycle.dated? && !exchange.order_cycle.closed? + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + + def self.refresh_supplier_fee(enterprise_fee) + refresh_incoming_exchanges(enterprise_fee.exchanges) + end + + def self.refresh_coordinator_fee(enterprise_fee) enterprise_fee.order_cycles.each do |order_cycle| order_cycle_changed order_cycle diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 56b557a7ee..6c5d3019d3 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -164,6 +164,71 @@ module OpenFoodNetwork end + describe "when an exchange is changed" do + let(:s) { create(:supplier_enterprise) } + let(:c) { create(:distributor_enterprise) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let(:v) { create(:variant) } + let(:oc) { create(:open_order_cycle, coordinator: c) } + + describe "incoming exchanges" do + let!(:ex1) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, variants: [v]) } + let!(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, variants: [v]) } + let!(:ex3) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, variants: []) } + + before { oc.reload } + + it "updates distributions that include one of the supplier's variants" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.exchange_changed ex1 + end + + it "doesn't update distributions that don't include any of the supplier's variants" do + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never + ProductsCache.exchange_changed ex1 + end + end + + describe "outgoing exchanges" do + let!(:ex) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false) } + + it "does not update for undated order cycles" do + oc.update_attributes! orders_open_at: nil, orders_close_at: nil + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.exchange_changed ex + end + + it "updates for upcoming order cycles" do + oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.exchange_changed ex + end + + it "updates for open order cycles" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + ProductsCache.exchange_changed ex + end + + it "does not update for closed order cycles" do + oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never + ProductsCache.exchange_changed ex + end + end + end + + + describe "when an exchange is destroyed" do + let(:exchange) { double(:exchange) } + + it "triggers the same update as a change to the exchange" do + expect(ProductsCache).to receive(:exchange_changed).with(exchange) + ProductsCache.exchange_destroyed exchange + end + end + + describe "when an enterprise fee is changed" do let(:s) { create(:supplier_enterprise) } let(:c) { create(:distributor_enterprise) } diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index ac6b7681a8..4b8f0ed1a3 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -91,6 +91,21 @@ describe Exchange do end end + describe "products caching" do + let!(:exchange) { create(:exchange) } + + it "refreshes the products cache on change" do + expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) + exchange.pickup_time = 'asdf' + exchange.save + end + + it "refreshes the products cache on destruction" do + expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_destroyed).with(exchange) + exchange.destroy + end + end + describe "scopes" do let(:supplier) { create(:supplier_enterprise) } let(:coordinator) { create(:distributor_enterprise, is_primary_producer: true) } From 8b070fddbbd79bc08ad712dfc1a815831e392d4c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 10:40:27 +1100 Subject: [PATCH 40/66] Refresh products cache when exchange fee is changed or destroyed --- app/models/exchange_fee.rb | 11 +++++++++++ spec/models/exchange_fee_spec.rb | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 spec/models/exchange_fee_spec.rb diff --git a/app/models/exchange_fee.rb b/app/models/exchange_fee.rb index ff9f12e8dd..03fce8ce04 100644 --- a/app/models/exchange_fee.rb +++ b/app/models/exchange_fee.rb @@ -1,4 +1,15 @@ class ExchangeFee < ActiveRecord::Base belongs_to :exchange belongs_to :enterprise_fee + + + after_save :refresh_products_cache + after_destroy :refresh_products_cache + + + private + + def refresh_products_cache + exchange.refresh_products_cache + end end diff --git a/spec/models/exchange_fee_spec.rb b/spec/models/exchange_fee_spec.rb new file mode 100644 index 0000000000..12bcd0c5e3 --- /dev/null +++ b/spec/models/exchange_fee_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe ExchangeFee do + describe "products caching" do + let(:exchange) { create(:exchange) } + let(:enterprise_fee) { create(:enterprise_fee) } + + it "refreshes the products cache on change" do + expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) + exchange.enterprise_fees << enterprise_fee + end + + it "refreshes the products cache on destruction" do + exchange.enterprise_fees << enterprise_fee + expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) + exchange.reload.exchange_fees.destroy_all + end + end +end From 98961fef74395383e1106d35dea77f7f51938783 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 12:04:32 +1100 Subject: [PATCH 41/66] Trigger cache refresh when producer property changed or destroyed --- app/models/producer_property.rb | 15 +++++++++++++++ lib/open_food_network/products_cache.rb | 8 ++++++++ spec/models/producer_property_spec.rb | 23 +++++++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 spec/models/producer_property_spec.rb diff --git a/app/models/producer_property.rb b/app/models/producer_property.rb index 178e7646ff..6e31a2b5f8 100644 --- a/app/models/producer_property.rb +++ b/app/models/producer_property.rb @@ -3,6 +3,9 @@ class ProducerProperty < ActiveRecord::Base default_scope order("#{self.table_name}.position") + after_save :refresh_products_cache + after_destroy :refresh_products_cache_from_destroy + def property_name property.name if property @@ -14,4 +17,16 @@ class ProducerProperty < ActiveRecord::Base Spree::Property.create(name: name, presentation: name) end end + + + private + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.producer_property_changed self + end + + def refresh_products_cache_from_destroy + OpenFoodNetwork::ProductsCache.producer_property_destroyed self + end + end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index d1ce117734..836555a532 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -41,6 +41,14 @@ module OpenFoodNetwork end + def self.producer_property_changed(producer_property) + end + + + def self.producer_property_destroyed(producer_property) + end + + def self.order_cycle_changed(order_cycle) if order_cycle.dated? && !order_cycle.closed? order_cycle.exchanges.outgoing.each do |exchange| diff --git a/spec/models/producer_property_spec.rb b/spec/models/producer_property_spec.rb new file mode 100644 index 0000000000..eaa7f0de0c --- /dev/null +++ b/spec/models/producer_property_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe ProducerProperty do + describe "products caching" do + let(:producer) { create(:supplier_enterprise) } + let(:pp) { producer.producer_properties.first } + + before do + producer.set_producer_property 'Organic Certified', 'NASAA 54321' + end + + it "refreshes the products cache on change" do + expect(OpenFoodNetwork::ProductsCache).to receive(:producer_property_changed).with(pp) + pp.value = 123 + pp.save + end + + it "refreshes the products cache on destruction" do + expect(OpenFoodNetwork::ProductsCache).to receive(:producer_property_destroyed).with(pp) + pp.destroy + end + end +end From 687fb6f0aa060d0bc830a5e05fff143b331f954c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 14:57:31 +1100 Subject: [PATCH 42/66] Enqueue RefreshProductsCacheJob with lower than default priority --- lib/open_food_network/products_cache_refreshment.rb | 2 +- .../open_food_network/products_cache_refreshment_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/open_food_network/products_cache_refreshment.rb b/lib/open_food_network/products_cache_refreshment.rb index 68ed5365c1..276734570f 100644 --- a/lib/open_food_network/products_cache_refreshment.rb +++ b/lib/open_food_network/products_cache_refreshment.rb @@ -41,7 +41,7 @@ module OpenFoodNetwork end def self.enqueue_job(distributor, order_cycle) - Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id + Delayed::Job.enqueue RefreshProductsCacheJob.new(distributor.id, order_cycle.id), priority: 10 end end end diff --git a/spec/lib/open_food_network/products_cache_refreshment_spec.rb b/spec/lib/open_food_network/products_cache_refreshment_spec.rb index bb033cd041..f65b81346e 100644 --- a/spec/lib/open_food_network/products_cache_refreshment_spec.rb +++ b/spec/lib/open_food_network/products_cache_refreshment_spec.rb @@ -13,6 +13,12 @@ module OpenFoodNetwork ProductsCacheRefreshment.refresh distributor, order_cycle end.to enqueue_job RefreshProductsCacheJob end + + it "enqueues the job with a lower than default priority" do + ProductsCacheRefreshment.refresh distributor, order_cycle + job = Delayed::Job.last + expect(job.priority).to be > Delayed::Worker.default_priority + end end describe "when there is an enqueued task, and it is running" do From 1b62dd06b80bd72fa264caa91b3dd960156e872e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 5 Feb 2016 15:16:12 +1100 Subject: [PATCH 43/66] Add products cache integrity checker --- Gemfile | 1 + Gemfile.lock | 2 + .../products_cache_integrity_checker_job.rb | 38 +++++++++++++++++++ config/schedule.rb | 4 ++ lib/tasks/cache.rake | 16 ++++++++ ...oducts_cache_integrity_checker_job_spec.rb | 26 +++++++++++++ 6 files changed, 87 insertions(+) create mode 100644 app/jobs/products_cache_integrity_checker_job.rb create mode 100644 lib/tasks/cache.rake create mode 100644 spec/jobs/products_cache_integrity_checker_job_spec.rb diff --git a/Gemfile b/Gemfile index 300b17ffa3..3690b194b8 100644 --- a/Gemfile +++ b/Gemfile @@ -55,6 +55,7 @@ gem 'figaro' gem 'blockenspiel' gem 'acts-as-taggable-on', '~> 3.4' gem 'paper_trail', '~> 3.0.8' +gem 'diffy' gem 'wicked_pdf' gem 'wkhtmltopdf-binary' diff --git a/Gemfile.lock b/Gemfile.lock index 75763fb4a0..fc93330817 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -248,6 +248,7 @@ GEM devise-encryptable (0.1.2) devise (>= 2.1.0) diff-lcs (1.2.4) + diffy (3.1.0) em-websocket (0.5.0) eventmachine (>= 0.12.9) http_parser.rb (~> 0.5.3) @@ -668,6 +669,7 @@ DEPENDENCIES debugger-linecache deface! delayed_job_active_record + diffy factory_girl_rails figaro foreigner diff --git a/app/jobs/products_cache_integrity_checker_job.rb b/app/jobs/products_cache_integrity_checker_job.rb new file mode 100644 index 0000000000..7bcf86baeb --- /dev/null +++ b/app/jobs/products_cache_integrity_checker_job.rb @@ -0,0 +1,38 @@ +require 'open_food_network/products_renderer' + +ProductsCacheIntegrityCheckerJob = Struct.new(:distributor_id, :order_cycle_id) do + def perform + if diff.any? + Bugsnag.notify RuntimeError.new("Products JSON differs from cached version"), diff: diff.to_s(:color) + end + end + + + private + + def diff + @diff ||= Diffy::Diff.new pretty(cached_json), pretty(rendered_json) + end + + def pretty(json) + JSON.pretty_generate JSON.parse json + end + + def cached_json + Rails.cache.read("products-json-#{distributor_id}-#{order_cycle_id}") || {}.to_json + end + + def rendered_json + OpenFoodNetwork::ProductsRenderer.new(distributor, order_cycle).products_json + rescue OpenFoodNetwork::ProductsRenderer::NoProducts + nil + end + + def distributor + Enterprise.find distributor_id + end + + def order_cycle + OrderCycle.find order_cycle_id + end +end diff --git a/config/schedule.rb b/config/schedule.rb index a09ca55dce..1846a6358d 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -7,6 +7,10 @@ env "MAILTO", "rohan@rohanmitchell.com" # If we use -e with a file containing specs, rspec interprets it and filters out our examples job_type :run_file, "cd :path; :environment_variable=:environment bundle exec script/rails runner :task :output" +every 1.hour do + rake 'openfoodnetwork:cache:check_products_cache_integrity' +end + every 1.day, at: '12:05am' do run_file "lib/open_food_network/integrity_checker.rb" end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake new file mode 100644 index 0000000000..2eefb8a49f --- /dev/null +++ b/lib/tasks/cache.rake @@ -0,0 +1,16 @@ +namespace :openfoodnetwork do + namespace :cache do + desc 'check the integrity of the products cache' + task :check_products_cache_integrity => :environment do + exchanges = Exchange. + outgoing. + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + + exchanges.each do |exchange| + Delayed::Job.enqueue ProductsCacheIntegrityCheckerJob.new(exchange.receiver, exchange.order_cycle), priority: 20 + end + end + end +end diff --git a/spec/jobs/products_cache_integrity_checker_job_spec.rb b/spec/jobs/products_cache_integrity_checker_job_spec.rb new file mode 100644 index 0000000000..1770f9bf84 --- /dev/null +++ b/spec/jobs/products_cache_integrity_checker_job_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' +require 'open_food_network/products_renderer' + +describe ProductsCacheIntegrityCheckerJob do + describe "reporting on differences between the products cache and the current products" do + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:simple_order_cycle) } + let(:job) { ProductsCacheIntegrityCheckerJob.new distributor.id, order_cycle.id } + + before do + Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", "[1, 2, 3]\n" + OpenFoodNetwork::ProductsRenderer.stub(:new) { double(:pr, products_json: "[1, 3]\n") } + end + + it "reports errors" do + expect(Bugsnag).to receive(:notify) + run_job job + end + + it "deals with nil cached_json" do + Rails.cache.clear + expect(Bugsnag).to receive(:notify) + run_job job + end + end +end From 71862e00a7c49c21873f4d4fcb2149d3d6eef0d9 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 24 Feb 2016 16:11:59 +1100 Subject: [PATCH 44/66] Perform products cache refresh when producer property changed or destroyed --- app/models/producer_property.rb | 1 + lib/open_food_network/products_cache.rb | 9 ++++ .../open_food_network/products_cache_spec.rb | 42 +++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/app/models/producer_property.rb b/app/models/producer_property.rb index 6e31a2b5f8..5b9fdd6d1c 100644 --- a/app/models/producer_property.rb +++ b/app/models/producer_property.rb @@ -1,4 +1,5 @@ class ProducerProperty < ActiveRecord::Base + belongs_to :producer, class_name: 'Enterprise' belongs_to :property, class_name: 'Spree::Property' default_scope order("#{self.table_name}.position") diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index 836555a532..bdf9ee79b1 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -42,10 +42,19 @@ module OpenFoodNetwork def self.producer_property_changed(producer_property) + products = producer_property.producer.supplied_products + variants = Spree::Variant. + where(is_master: false, deleted_at: nil). + where(product_id: products) + + exchanges_featuring_variants(variants).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end end def self.producer_property_destroyed(producer_property) + producer_property_changed producer_property end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 6c5d3019d3..7db47543ad 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -118,6 +118,48 @@ module OpenFoodNetwork end + describe "when a producer property is changed" do + let(:s) { create(:supplier_enterprise) } + let(:pp) { s.producer_properties.last } + let(:product) { create(:simple_product, supplier: s) } + let(:v1) { create(:variant, product: product) } + let(:v2) { create(:variant, product: product) } + let(:v_deleted) { create(:variant, product: product, deleted_at: Time.now) } + let(:d1) { create(:distributor_enterprise) } + let(:d2) { create(:distributor_enterprise) } + let(:d3) { create(:distributor_enterprise) } + let(:oc) { create(:open_order_cycle) } + let!(:ex1) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, variants: [v1]) } + let!(:ex2) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d2, variants: [v1, v2]) } + let!(:ex3) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d3, variants: [product.master, v_deleted]) } + + before do + s.set_producer_property :organic, 'NASAA 12345' + end + + it "refreshes the distributions the supplied variants appear in" do + expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once + expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).once + ProductsCache.producer_property_changed pp + end + + it "doesn't respond to master or deleted variants" do + expect(ProductsCache).to receive(:refresh_cache).with(d3, oc).never + ProductsCache.producer_property_changed pp + end + end + + + describe "when a producer property is destroyed" do + let(:producer_property) { double(:producer_property) } + + it "triggers the same update as a change to the producer property" do + expect(ProductsCache).to receive(:producer_property_changed).with(producer_property) + ProductsCache.producer_property_destroyed producer_property + end + end + + describe "when an order cycle is changed" do let(:variant) { create(:variant) } let(:s) { create(:supplier_enterprise) } From b5204a48206f50017a9fc81ae04065ee0dbfbcd5 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 25 Feb 2016 11:08:53 +1100 Subject: [PATCH 45/66] Refresh cache when option value changed or destroyed --- app/models/spree/option_value_decorator.rb | 20 +++++++++++++++++ spec/models/spree/option_value_spec.rb | 26 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 app/models/spree/option_value_decorator.rb create mode 100644 spec/models/spree/option_value_spec.rb diff --git a/app/models/spree/option_value_decorator.rb b/app/models/spree/option_value_decorator.rb new file mode 100644 index 0000000000..cfe9c23cca --- /dev/null +++ b/app/models/spree/option_value_decorator.rb @@ -0,0 +1,20 @@ +module Spree + OptionValue.class_eval do + after_save :refresh_products_cache + around_destroy :refresh_products_cache_from_destroy + + + private + + def refresh_products_cache + variants(:reload).each &:refresh_products_cache + end + + def refresh_products_cache_from_destroy + vs = variants(:reload).to_a + yield + vs.each &:refresh_products_cache + end + + end +end diff --git a/spec/models/spree/option_value_spec.rb b/spec/models/spree/option_value_spec.rb new file mode 100644 index 0000000000..f784e08af2 --- /dev/null +++ b/spec/models/spree/option_value_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +module Spree + describe OptionValue do + describe "products cache" do + let(:variant) { create(:variant) } + let(:option_value) { create(:option_value) } + + before do + variant.option_values << option_value + option_value.reload + end + + it "refreshes the products cache on change, via variant" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + option_value.name = 'foo' + option_value.save! + end + + it "refreshes the products cache on destruction, via variant" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + option_value.destroy + end + end + end +end From 8928e461d4f166bf22cc95c68c58ec0e3d586167 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 26 Feb 2016 09:59:16 +1100 Subject: [PATCH 46/66] Refresh cache when option type changed --- app/models/spree/option_type_decorator.rb | 13 +++++++++++++ spec/models/spree/option_type_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 app/models/spree/option_type_decorator.rb create mode 100644 spec/models/spree/option_type_spec.rb diff --git a/app/models/spree/option_type_decorator.rb b/app/models/spree/option_type_decorator.rb new file mode 100644 index 0000000000..aea706c847 --- /dev/null +++ b/app/models/spree/option_type_decorator.rb @@ -0,0 +1,13 @@ +module Spree + OptionType.class_eval do + has_many :products, through: :product_option_types + after_save :refresh_products_cache + + + private + + def refresh_products_cache + products(:reload).each &:refresh_products_cache + end + end +end diff --git a/spec/models/spree/option_type_spec.rb b/spec/models/spree/option_type_spec.rb new file mode 100644 index 0000000000..e93f35ec49 --- /dev/null +++ b/spec/models/spree/option_type_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +module Spree + describe OptionType do + describe "products cache" do + let!(:product) { create(:simple_product, option_types: [option_type]) } + let(:option_type) { create(:option_type) } + + before { option_type.reload } + + it "refreshes the products cache on change, via product" do + expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) + option_type.name = 'foo' + option_type.save! + end + + # When a OptionType is destroyed, the destruction of the OptionValues refreshes the cache + end + end +end From 45a7b13e9af79ab8cd608a372a47ce20c5a3b904 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 26 Feb 2016 10:09:16 +1100 Subject: [PATCH 47/66] Refresh cache when option type destroyed --- spec/models/spree/option_type_spec.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/spec/models/spree/option_type_spec.rb b/spec/models/spree/option_type_spec.rb index e93f35ec49..cfc0654733 100644 --- a/spec/models/spree/option_type_spec.rb +++ b/spec/models/spree/option_type_spec.rb @@ -4,9 +4,14 @@ module Spree describe OptionType do describe "products cache" do let!(:product) { create(:simple_product, option_types: [option_type]) } + let(:variant) { product.variants.first } let(:option_type) { create(:option_type) } + let(:option_value) { create(:option_value, option_type: option_type) } - before { option_type.reload } + before do + option_type.reload + variant.option_values << option_value + end it "refreshes the products cache on change, via product" do expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) @@ -14,7 +19,10 @@ module Spree option_type.save! end - # When a OptionType is destroyed, the destruction of the OptionValues refreshes the cache + it "refreshes the products cache on destruction, via option value destruction" do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) + option_type.destroy + end end end end From d89e9620ac153db28e204d231b8aa9903b0eb03d Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 26 Feb 2016 12:05:49 +1100 Subject: [PATCH 48/66] Fix output of cache integrity checker errors --- app/jobs/products_cache_integrity_checker_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/products_cache_integrity_checker_job.rb b/app/jobs/products_cache_integrity_checker_job.rb index 7bcf86baeb..89f8a5d7f4 100644 --- a/app/jobs/products_cache_integrity_checker_job.rb +++ b/app/jobs/products_cache_integrity_checker_job.rb @@ -3,7 +3,7 @@ require 'open_food_network/products_renderer' ProductsCacheIntegrityCheckerJob = Struct.new(:distributor_id, :order_cycle_id) do def perform if diff.any? - Bugsnag.notify RuntimeError.new("Products JSON differs from cached version"), diff: diff.to_s(:color) + Bugsnag.notify RuntimeError.new("Products JSON differs from cached version for distributor: #{@distributor_id}, order cycle: #{@order_cycle_id}"), diff: diff.to_s(:text) end end From 21ce7ab30a3dc309cc0dd34174b8cf48395796e6 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 26 Feb 2016 13:04:55 +1100 Subject: [PATCH 49/66] Fix integrity checker error message, add task to warm products cache --- .../products_cache_integrity_checker_job.rb | 2 +- lib/tasks/cache.rake | 31 +++++++++++++------ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/app/jobs/products_cache_integrity_checker_job.rb b/app/jobs/products_cache_integrity_checker_job.rb index 89f8a5d7f4..5702380309 100644 --- a/app/jobs/products_cache_integrity_checker_job.rb +++ b/app/jobs/products_cache_integrity_checker_job.rb @@ -3,7 +3,7 @@ require 'open_food_network/products_renderer' ProductsCacheIntegrityCheckerJob = Struct.new(:distributor_id, :order_cycle_id) do def perform if diff.any? - Bugsnag.notify RuntimeError.new("Products JSON differs from cached version for distributor: #{@distributor_id}, order cycle: #{@order_cycle_id}"), diff: diff.to_s(:text) + Bugsnag.notify RuntimeError.new("Products JSON differs from cached version for distributor: #{distributor_id}, order cycle: #{order_cycle_id}"), diff: diff.to_s(:text) end end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 2eefb8a49f..7b20a69c6a 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -1,16 +1,29 @@ namespace :openfoodnetwork do namespace :cache do desc 'check the integrity of the products cache' - task :check_products_cache_integrity => :environment do - exchanges = Exchange. - outgoing. - joins(:order_cycle). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed) - - exchanges.each do |exchange| - Delayed::Job.enqueue ProductsCacheIntegrityCheckerJob.new(exchange.receiver, exchange.order_cycle), priority: 20 + task :check_products_integrity => :environment do + active_exchanges.each do |exchange| + Delayed::Job.enqueue ProductsCacheIntegrityCheckerJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 20 end end + + + desc 'warm the products cache' + task :warm_products => :environment do + active_exchanges.each do |exchange| + Delayed::Job.enqueue RefreshProductsCacheJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 10 + end + end + + + private + + def active_exchanges + Exchange. + outgoing. + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + end end end From f394cf559c337b7cb30d2b496abb9d600108c67d Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 2 Mar 2016 08:49:36 +1100 Subject: [PATCH 50/66] Fix integrity checker rake task name --- config/schedule.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/schedule.rb b/config/schedule.rb index 1846a6358d..023382330a 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -8,7 +8,7 @@ env "MAILTO", "rohan@rohanmitchell.com" job_type :run_file, "cd :path; :environment_variable=:environment bundle exec script/rails runner :task :output" every 1.hour do - rake 'openfoodnetwork:cache:check_products_cache_integrity' + rake 'openfoodnetwork:cache:check_products_integrity' end every 1.day, at: '12:05am' do From 2abee3fcddab4fbaab208ff11eae564c02abd6b3 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 2 Mar 2016 11:01:41 +1100 Subject: [PATCH 51/66] Extract integrity checking to lib class --- .../products_cache_integrity_checker_job.rb | 24 +++------- .../products_cache_integrity_checker.rb | 44 +++++++++++++++++++ lib/tasks/cache.rake | 6 +-- 3 files changed, 50 insertions(+), 24 deletions(-) create mode 100644 lib/open_food_network/products_cache_integrity_checker.rb diff --git a/app/jobs/products_cache_integrity_checker_job.rb b/app/jobs/products_cache_integrity_checker_job.rb index 5702380309..78feb3a1b0 100644 --- a/app/jobs/products_cache_integrity_checker_job.rb +++ b/app/jobs/products_cache_integrity_checker_job.rb @@ -1,31 +1,17 @@ -require 'open_food_network/products_renderer' +require 'open_food_network/products_cache_integrity_checker' ProductsCacheIntegrityCheckerJob = Struct.new(:distributor_id, :order_cycle_id) do def perform - if diff.any? - Bugsnag.notify RuntimeError.new("Products JSON differs from cached version for distributor: #{distributor_id}, order cycle: #{order_cycle_id}"), diff: diff.to_s(:text) + unless checker.ok? + Bugsnag.notify RuntimeError.new("Products JSON differs from cached version for distributor: #{distributor_id}, order cycle: #{order_cycle_id}"), diff: checker.diff.to_s(:text) end end private - def diff - @diff ||= Diffy::Diff.new pretty(cached_json), pretty(rendered_json) - end - - def pretty(json) - JSON.pretty_generate JSON.parse json - end - - def cached_json - Rails.cache.read("products-json-#{distributor_id}-#{order_cycle_id}") || {}.to_json - end - - def rendered_json - OpenFoodNetwork::ProductsRenderer.new(distributor, order_cycle).products_json - rescue OpenFoodNetwork::ProductsRenderer::NoProducts - nil + def checker + OpenFoodNetwork::ProductsCacheIntegrityChecker.new(distributor, order_cycle) end def distributor diff --git a/lib/open_food_network/products_cache_integrity_checker.rb b/lib/open_food_network/products_cache_integrity_checker.rb new file mode 100644 index 0000000000..cf8363ee5c --- /dev/null +++ b/lib/open_food_network/products_cache_integrity_checker.rb @@ -0,0 +1,44 @@ +require 'open_food_network/products_renderer' + +module OpenFoodNetwork + class ProductsCacheIntegrityChecker + def initialize(distributor, order_cycle) + @distributor = distributor + @order_cycle = order_cycle + end + + def ok? + diff.none? + end + + def diff + @diff ||= Diffy::Diff.new pretty(cached_json), pretty(rendered_json) + end + + + def self.active_exchanges + Exchange. + outgoing. + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + end + + + private + + def cached_json + Rails.cache.read("products-json-#{@distributor.id}-#{@order_cycle.id}") || {}.to_json + end + + def rendered_json + OpenFoodNetwork::ProductsRenderer.new(@distributor, @order_cycle).products_json + rescue OpenFoodNetwork::ProductsRenderer::NoProducts + nil + end + + def pretty(json) + JSON.pretty_generate JSON.parse json + end + end +end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 7b20a69c6a..8a564fcddc 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -19,11 +19,7 @@ namespace :openfoodnetwork do private def active_exchanges - Exchange. - outgoing. - joins(:order_cycle). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed) + ProductsCacheIntegrityChecker.active_exchanges end end end From ec55af5b8af2ece4ba66067faa2914d79d9c74a1 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 2 Mar 2016 11:05:03 +1100 Subject: [PATCH 52/66] Display products cache integrity checker results on cache settings admin page --- .../admin/cache_settings_controller.rb | 14 ++++++ .../add_caching.html.haml.deface | 4 ++ app/views/admin/cache_settings/show.html.haml | 18 ++++++++ config/routes.rb | 2 + spec/features/admin/caching_spec.rb | 43 +++++++++++++++++++ 5 files changed, 81 insertions(+) create mode 100644 app/controllers/admin/cache_settings_controller.rb create mode 100644 app/overrides/spree/admin/shared/_configuration_menu/add_caching.html.haml.deface create mode 100644 app/views/admin/cache_settings/show.html.haml create mode 100644 spec/features/admin/caching_spec.rb diff --git a/app/controllers/admin/cache_settings_controller.rb b/app/controllers/admin/cache_settings_controller.rb new file mode 100644 index 0000000000..ada153a6d5 --- /dev/null +++ b/app/controllers/admin/cache_settings_controller.rb @@ -0,0 +1,14 @@ +require 'open_food_network/products_cache_integrity_checker' + +class Admin::CacheSettingsController < Spree::Admin::BaseController + + def show + active_exchanges = OpenFoodNetwork::ProductsCacheIntegrityChecker.active_exchanges + @results = active_exchanges.map do |exchange| + checker = OpenFoodNetwork::ProductsCacheIntegrityChecker.new(exchange.receiver, exchange.order_cycle) + + {distributor: exchange.receiver, order_cycle: exchange.order_cycle, status: checker.ok?, diff: checker.diff} + end + end + +end diff --git a/app/overrides/spree/admin/shared/_configuration_menu/add_caching.html.haml.deface b/app/overrides/spree/admin/shared/_configuration_menu/add_caching.html.haml.deface new file mode 100644 index 0000000000..1eb416e72a --- /dev/null +++ b/app/overrides/spree/admin/shared/_configuration_menu/add_caching.html.haml.deface @@ -0,0 +1,4 @@ +/ insert_bottom "[data-hook='admin_configurations_sidebar_menu']" + +%li + = link_to 'Caching', main_app.admin_cache_settings_path diff --git a/app/views/admin/cache_settings/show.html.haml b/app/views/admin/cache_settings/show.html.haml new file mode 100644 index 0000000000..79b3e5acaf --- /dev/null +++ b/app/views/admin/cache_settings/show.html.haml @@ -0,0 +1,18 @@ +- content_for :page_title do + = t(:cache_settings) + +%table.index + %thead + %tr + %th Distributor + %th Order Cycle + %th Status + %th Diff + %tbody + - @results.each do |result| + %tr + %td= result[:distributor].name + %td= result[:order_cycle].name + %td= result[:status] ? 'OK' : 'Error' + %td + %pre= result[:diff].to_s(:text) diff --git a/config/routes.rb b/config/routes.rb index 62c4153d14..c44f8395de 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -122,6 +122,8 @@ Openfoodnetwork::Application.routes.draw do resource :business_model_configuration, only: [:edit, :update], controller: 'business_model_configuration' + resource :cache_settings + resource :account, only: [:show], controller: 'account' end diff --git a/spec/features/admin/caching_spec.rb b/spec/features/admin/caching_spec.rb new file mode 100644 index 0000000000..22d372e19b --- /dev/null +++ b/spec/features/admin/caching_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' +require 'open_food_network/products_renderer' + +feature 'Caching' do + include AuthenticationWorkflow + include WebHelper + + before { quick_login_as_admin } + + describe "displaying integrity checker results" do + let(:distributor) { create(:distributor_enterprise) } + let(:order_cycle) { create(:open_order_cycle, distributors: [distributor]) } + + it "displays results when things are good" do + # Given matching data + Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", "[1, 2, 3]\n" + OpenFoodNetwork::ProductsRenderer.stub(:new) { double(:pr, products_json: "[1, 2, 3]\n") } + + # When I visit the cache status page + visit spree.admin_path + click_link 'Configuration' + click_link 'Caching' + + # Then I should see some status information + page.should have_content "OK" + end + + it "displays results when there are errors" do + # Given matching data + Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", "[1, 2, 3]\n" + OpenFoodNetwork::ProductsRenderer.stub(:new) { double(:pr, products_json: "[1, 3]\n") } + + # When I visit the cache status page + visit spree.admin_path + click_link 'Configuration' + click_link 'Caching' + + # Then I should see some status information + page.should have_content "Error" + end + + end +end From 4a7a40425adcf05b2a20f45ceb8f8f4fe1dfe849 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 2 Mar 2016 11:38:42 +1100 Subject: [PATCH 53/66] Fix problems in rake file --- lib/tasks/cache.rake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 8a564fcddc..4f4acb5a97 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -1,3 +1,5 @@ +require 'open_food_network/products_cache_integrity_checker' + namespace :openfoodnetwork do namespace :cache do desc 'check the integrity of the products cache' @@ -19,7 +21,7 @@ namespace :openfoodnetwork do private def active_exchanges - ProductsCacheIntegrityChecker.active_exchanges + OpenFoodNetwork::ProductsCacheIntegrityChecker.active_exchanges end end end From 6a2319e16d0029e9d3a2cf1866adcd80c8f728e8 Mon Sep 17 00:00:00 2001 From: Steve Pettitt Date: Wed, 2 Mar 2016 19:08:57 +0000 Subject: [PATCH 54/66] Remove producers without lat + long from map --- app/assets/javascripts/darkswarm/services/map.js.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/darkswarm/services/map.js.coffee b/app/assets/javascripts/darkswarm/services/map.js.coffee index 96768e8379..c48cc46ad0 100644 --- a/app/assets/javascripts/darkswarm/services/map.js.coffee +++ b/app/assets/javascripts/darkswarm/services/map.js.coffee @@ -2,6 +2,8 @@ Darkswarm.factory "OfnMap", (Enterprises, EnterpriseModal, visibleFilter) -> new class OfnMap constructor: -> @enterprises = @enterprise_markers(Enterprises.enterprises) + @enterprises = @enterprises.filter (enterprise) -> + enterprise.latitude || enterprise.longitude # Remove enterprises w/o lat or long enterprise_markers: (enterprises) -> @extend(enterprise) for enterprise in visibleFilter(enterprises) From c1d068aeb927ef7cbaa2c9c187fe121860cc917c Mon Sep 17 00:00:00 2001 From: Steve Pettitt Date: Wed, 2 Mar 2016 20:07:39 +0000 Subject: [PATCH 55/66] Add/modify karma specs --- .../unit/darkswarm/services/map_spec.js.coffee | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee index 4252000460..65de34e89c 100644 --- a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee @@ -9,6 +9,16 @@ describe "Hubs service", -> orders_close_at: new Date() type: "hub" visible: true + latitude: 50 + longitude: 50 + } + { + id: 3 + active: false + orders_close_at: new Date() + type: "hub" + visible: true + } ] @@ -24,3 +34,6 @@ describe "Hubs service", -> it "builds MapMarkers from enterprises", -> expect(OfnMap.enterprises[0].id).toBe enterprises[0].id + + it "excludes enterprises without latitude or longitude", -> + expect(OfnMap.enterprises.map (e) -> e.id).not.toContain enterprises[1].id From 7e6d544180d500c39d290ec4f26dabbca399f10a Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 3 Mar 2016 09:33:32 +1100 Subject: [PATCH 56/66] Do not serialize product count_on_hand - reduce coupling between variant create and products JSON --- app/serializers/api/product_serializer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 03a66afe89..aa353173dc 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -36,7 +36,7 @@ class Api::CachedProductSerializer < ActiveModel::Serializer #delegate :cache_key, to: :object include ActionView::Helpers::SanitizeHelper - attributes :id, :name, :permalink, :count_on_hand + attributes :id, :name, :permalink attributes :on_demand, :group_buy, :notes, :description attributes :properties_with_values From 4966290f87e5e7654663793fbefbbe33141157e3 Mon Sep 17 00:00:00 2001 From: Steve Pettitt Date: Thu, 3 Mar 2016 06:41:59 +0000 Subject: [PATCH 57/66] Check missing lat/long instead of zero --- app/assets/javascripts/darkswarm/services/map.js.coffee | 2 +- spec/javascripts/unit/darkswarm/services/map_spec.js.coffee | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/map.js.coffee b/app/assets/javascripts/darkswarm/services/map.js.coffee index c48cc46ad0..f4bad04acc 100644 --- a/app/assets/javascripts/darkswarm/services/map.js.coffee +++ b/app/assets/javascripts/darkswarm/services/map.js.coffee @@ -3,7 +3,7 @@ Darkswarm.factory "OfnMap", (Enterprises, EnterpriseModal, visibleFilter) -> constructor: -> @enterprises = @enterprise_markers(Enterprises.enterprises) @enterprises = @enterprises.filter (enterprise) -> - enterprise.latitude || enterprise.longitude # Remove enterprises w/o lat or long + enterprise.latitude != null || enterprise.longitude != null # Remove enterprises w/o lat or long enterprise_markers: (enterprises) -> @extend(enterprise) for enterprise in visibleFilter(enterprises) diff --git a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee index 65de34e89c..6e5fcc320f 100644 --- a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee @@ -9,8 +9,8 @@ describe "Hubs service", -> orders_close_at: new Date() type: "hub" visible: true - latitude: 50 - longitude: 50 + latitude: 0 + longitude: 0 } { id: 3 From 939356ef263f842f949d42c02caa6454af2b05b0 Mon Sep 17 00:00:00 2001 From: Steve Pettitt Date: Thu, 3 Mar 2016 07:18:49 +0000 Subject: [PATCH 58/66] Update spec with nulls --- spec/javascripts/unit/darkswarm/services/map_spec.js.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee index 6e5fcc320f..669f0ca74d 100644 --- a/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/map_spec.js.coffee @@ -18,7 +18,8 @@ describe "Hubs service", -> orders_close_at: new Date() type: "hub" visible: true - + latitude: null + longitude: null } ] From bc2223fb8eb8b1266fea44b0ead8774164b49911 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Mar 2016 10:15:57 +1100 Subject: [PATCH 59/66] Fix intermittent spec fails: currency inconsistencies on CI and retry on VOs --- spec/features/admin/variant_overrides_spec.rb | 2 +- spec/models/spree/shipping_method_spec.rb | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index fe74d9eb2d..037c3e7cf0 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -318,7 +318,7 @@ feature %q{ select2_select hub.name, from: 'hub_id' end - it "alerts the user to the presence of new products, and allows them to be added or hidden" do + it "alerts the user to the presence of new products, and allows them to be added or hidden", retry: 3 do expect(page).to_not have_selector "table#variant-overrides tr#v_#{variant1.id}" expect(page).to_not have_selector "table#variant-overrides tr#v_#{variant2.id}" diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb index 0e738470dd..daae9a2c4e 100644 --- a/spec/models/spree/shipping_method_spec.rb +++ b/spec/models/spree/shipping_method_spec.rb @@ -39,19 +39,26 @@ module Spree describe "availability" do let(:sm) { build(:shipping_method) } + before do + sm.calculator.preferences[:currency] = Spree::Config.currency + + end + it "is available to orders that match its distributor" do - o = build(:order, ship_address: build(:address), distributor: sm.distributors.first) + o = build(:order, ship_address: build(:address), + distributor: sm.distributors.first, currency: Spree::Config.currency) sm.should be_available_to_order o end it "is not available to orders that do not match its distributor" do o = build(:order, ship_address: build(:address), - distributor: build(:distributor_enterprise)) + distributor: build(:distributor_enterprise), currency: Spree::Config.currency) sm.should_not be_available_to_order o end it "is available to orders with no shipping address" do - o = build(:order, ship_address: nil, distributor: sm.distributors.first) + o = build(:order, ship_address: nil, + distributor: sm.distributors.first, currency: Spree::Config.currency) sm.should be_available_to_order o end end From 1440544b2dbe779804c468674b487d2560d53e70 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Mar 2016 10:29:26 +1100 Subject: [PATCH 60/66] Use persisted models --- spec/models/spree/shipping_method_spec.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb index daae9a2c4e..135b0b0092 100644 --- a/spec/models/spree/shipping_method_spec.rb +++ b/spec/models/spree/shipping_method_spec.rb @@ -37,27 +37,26 @@ module Spree describe "availability" do - let(:sm) { build(:shipping_method) } + let(:sm) { create(:shipping_method) } before do sm.calculator.preferences[:currency] = Spree::Config.currency - end it "is available to orders that match its distributor" do - o = build(:order, ship_address: build(:address), + o = create(:order, ship_address: create(:address), distributor: sm.distributors.first, currency: Spree::Config.currency) sm.should be_available_to_order o end it "is not available to orders that do not match its distributor" do - o = build(:order, ship_address: build(:address), - distributor: build(:distributor_enterprise), currency: Spree::Config.currency) + o = create(:order, ship_address: create(:address), + distributor: create(:distributor_enterprise), currency: Spree::Config.currency) sm.should_not be_available_to_order o end it "is available to orders with no shipping address" do - o = build(:order, ship_address: nil, + o = create(:order, ship_address: nil, distributor: sm.distributors.first, currency: Spree::Config.currency) sm.should be_available_to_order o end From 73b53e02fc8965e73da8ca6128ec09b0af0cdab9 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Mar 2016 10:53:48 +1100 Subject: [PATCH 61/66] Exclude performance specs from CI, which were modifying Spree::Config.currency --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 0ba99dc9e1..56d94e5952 100644 --- a/.travis.yml +++ b/.travis.yml @@ -38,7 +38,7 @@ before_script: script: - 'if [ "$KARMA" = "true" ]; then bundle exec rake karma:run; else echo "Skipping karma run"; fi' #- "KNAPSACK_GENERATE_REPORT=true bundle exec rspec spec" - - "bundle exec rake knapsack:rspec" + - "bundle exec rake 'knapsack:rspec[--tag ~performance]'" after_success: - > From 780ec598d6892ee8bd02e261ac4b836c99491024 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Mar 2016 11:31:18 +1100 Subject: [PATCH 62/66] Use preferred_currency instead of preferences[:currency]. Use constant for currency instead of config var. Conflicts: spec/models/spree/shipping_method_spec.rb --- spec/models/spree/shipping_method_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb index 135b0b0092..33c44b8b6d 100644 --- a/spec/models/spree/shipping_method_spec.rb +++ b/spec/models/spree/shipping_method_spec.rb @@ -38,26 +38,27 @@ module Spree describe "availability" do let(:sm) { create(:shipping_method) } + let(:currency) { 'AUD' } before do - sm.calculator.preferences[:currency] = Spree::Config.currency + sm.calculator.preferred_currency = currency end it "is available to orders that match its distributor" do o = create(:order, ship_address: create(:address), - distributor: sm.distributors.first, currency: Spree::Config.currency) + distributor: sm.distributors.first, currency: currency) sm.should be_available_to_order o end it "is not available to orders that do not match its distributor" do o = create(:order, ship_address: create(:address), - distributor: create(:distributor_enterprise), currency: Spree::Config.currency) + distributor: create(:distributor_enterprise), currency: currency) sm.should_not be_available_to_order o end it "is available to orders with no shipping address" do o = create(:order, ship_address: nil, - distributor: sm.distributors.first, currency: Spree::Config.currency) + distributor: sm.distributors.first, currency: currency) sm.should be_available_to_order o end end From 27d7b3026bb611c9e8345e9fcfd730c93a293475 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 9 Mar 2016 12:28:09 +1100 Subject: [PATCH 63/66] Move OpenFoodNetwork::ProductsCacheIntegrityChecker.active_exchanges to Exchange model --- app/controllers/admin/cache_settings_controller.rb | 3 +-- app/models/exchange.rb | 6 ++++++ .../products_cache_integrity_checker.rb | 9 --------- lib/tasks/cache.rake | 11 ++--------- 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/app/controllers/admin/cache_settings_controller.rb b/app/controllers/admin/cache_settings_controller.rb index ada153a6d5..138bbc7b18 100644 --- a/app/controllers/admin/cache_settings_controller.rb +++ b/app/controllers/admin/cache_settings_controller.rb @@ -3,8 +3,7 @@ require 'open_food_network/products_cache_integrity_checker' class Admin::CacheSettingsController < Spree::Admin::BaseController def show - active_exchanges = OpenFoodNetwork::ProductsCacheIntegrityChecker.active_exchanges - @results = active_exchanges.map do |exchange| + @results = Exchange.cachable.map do |exchange| checker = OpenFoodNetwork::ProductsCacheIntegrityChecker.new(exchange.receiver, exchange.order_cycle) {distributor: exchange.receiver, order_cycle: exchange.order_cycle, status: checker.ok?, diff: checker.diff} diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 94aada7e0e..ad290f6c26 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -34,6 +34,12 @@ class Exchange < ActiveRecord::Base joins('INNER JOIN enterprises AS receiver ON (receiver.id = exchanges.receiver_id)'). order("CASE WHEN exchanges.incoming='t' THEN sender.name ELSE receiver.name END") + # Exchanges on order cycles that are dated and are upcoming or open are cached + scope :cachable, outgoing. + joins(:order_cycle). + merge(OrderCycle.dated). + merge(OrderCycle.not_closed) + scope :managed_by, lambda { |user| if user.has_spree_role?('admin') scoped diff --git a/lib/open_food_network/products_cache_integrity_checker.rb b/lib/open_food_network/products_cache_integrity_checker.rb index cf8363ee5c..8098124feb 100644 --- a/lib/open_food_network/products_cache_integrity_checker.rb +++ b/lib/open_food_network/products_cache_integrity_checker.rb @@ -16,15 +16,6 @@ module OpenFoodNetwork end - def self.active_exchanges - Exchange. - outgoing. - joins(:order_cycle). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed) - end - - private def cached_json diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake index 4f4acb5a97..64a5d3909a 100644 --- a/lib/tasks/cache.rake +++ b/lib/tasks/cache.rake @@ -4,7 +4,7 @@ namespace :openfoodnetwork do namespace :cache do desc 'check the integrity of the products cache' task :check_products_integrity => :environment do - active_exchanges.each do |exchange| + Exchange.cachable.each do |exchange| Delayed::Job.enqueue ProductsCacheIntegrityCheckerJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 20 end end @@ -12,16 +12,9 @@ namespace :openfoodnetwork do desc 'warm the products cache' task :warm_products => :environment do - active_exchanges.each do |exchange| + Exchange.cachable.each do |exchange| Delayed::Job.enqueue RefreshProductsCacheJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 10 end end - - - private - - def active_exchanges - OpenFoodNetwork::ProductsCacheIntegrityChecker.active_exchanges - end end end From 6f29a8b64239de51e2930a09cebc11f8186cd4ca Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 9 Mar 2016 13:29:33 +1100 Subject: [PATCH 64/66] Refresh cache when inventory setting product_selection_from_inventory_only is changed --- app/models/spree/preference_decorator.rb | 31 +++++++++++++++++++ lib/open_food_network/products_cache.rb | 7 +++++ spec/features/admin/enterprises_spec.rb | 26 ++++++++++++++++ .../open_food_network/products_cache_spec.rb | 10 ++++++ spec/models/spree/preference_spec.rb | 23 ++++++++++++++ 5 files changed, 97 insertions(+) create mode 100644 app/models/spree/preference_decorator.rb create mode 100644 spec/models/spree/preference_spec.rb diff --git a/app/models/spree/preference_decorator.rb b/app/models/spree/preference_decorator.rb new file mode 100644 index 0000000000..c3ab3f9a94 --- /dev/null +++ b/app/models/spree/preference_decorator.rb @@ -0,0 +1,31 @@ +require 'open_food_network/products_cache' + +module Spree + Preference.class_eval do + after_save :refresh_products_cache + + # When the setting preferred_product_selection_from_inventory_only has changed, we want to + # refresh all active exchanges for this enterprise. + def refresh_products_cache + if product_selection_from_inventory_only_changed? + OpenFoodNetwork::ProductsCache.distributor_changed(enterprise) + end + end + + + private + + def product_selection_from_inventory_only_changed? + key =~ product_selection_from_inventory_only_regex + end + + def enterprise + enterprise_id = key.match(product_selection_from_inventory_only_regex)[1] + enterprise = Enterprise.find enterprise_id + end + + def product_selection_from_inventory_only_regex + /^enterprise\/product_selection_from_inventory_only\/(\d+)$/ + end + end +end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index bdf9ee79b1..d5b39609d9 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -88,6 +88,13 @@ module OpenFoodNetwork end + def self.distributor_changed(enterprise) + Exchange.cachable.where(receiver_id: enterprise).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + private def self.exchanges_featuring_variants(variants, distributor: nil) diff --git a/spec/features/admin/enterprises_spec.rb b/spec/features/admin/enterprises_spec.rb index aed96b5a91..4bd58715eb 100644 --- a/spec/features/admin/enterprises_spec.rb +++ b/spec/features/admin/enterprises_spec.rb @@ -251,6 +251,32 @@ feature %q{ end + describe "inventory settings", js: true do + let!(:enterprise) { create(:distributor_enterprise) } + let!(:product) { create(:simple_product) } + let!(:order_cycle) { create(:simple_order_cycle, distributors: [enterprise], variants: [product.variants.first]) } + + before do + Delayed::Job.destroy_all + quick_login_as_admin + end + + it "refreshes the cache when I change what products appear on my shopfront" do + # Given a product that's not in my inventory, but is in an active order cycle + + # When I change which products appear on the shopfront + visit edit_admin_enterprise_path(enterprise) + within(".side_menu") { click_link 'Inventory Settings' } + choose 'enterprise_preferred_product_selection_from_inventory_only_1' + + # Then a job should have been enqueued to refresh the cache + expect do + click_button 'Update' + end.to enqueue_job RefreshProductsCacheJob, distributor_id: enterprise.id, order_cycle_id: order_cycle.id + end + end + + context "as an Enterprise user", js: true do let(:supplier1) { create(:supplier_enterprise, name: 'First Supplier') } let(:supplier2) { create(:supplier_enterprise, name: 'Another Supplier') } diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 7db47543ad..6b0bc61eb2 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -376,6 +376,16 @@ module OpenFoodNetwork end end + describe "when a distributor enterprise is changed" do + let(:d) { create(:distributor_enterprise) } + let(:oc) { create(:open_order_cycle, distributors: [d]) } + + it "updates each distribution the enterprise is active in" do + expect(ProductsCache).to receive(:refresh_cache).with(d, oc) + ProductsCache.distributor_changed d + end + end + describe "refreshing the cache" do let(:distributor) { double(:distributor) } let(:order_cycle) { double(:order_cycle) } diff --git a/spec/models/spree/preference_spec.rb b/spec/models/spree/preference_spec.rb new file mode 100644 index 0000000000..ff71c9c9c6 --- /dev/null +++ b/spec/models/spree/preference_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +module Spree + describe Preference do + describe "refreshing the products cache" do + it "reports when product_selection_from_inventory_only has changed" do + p = Preference.new(key: 'enterprise/product_selection_from_inventory_only/123') + expect(p.send(:product_selection_from_inventory_only_changed?)).to be_true + end + + it "reports when product_selection_from_inventory_only has not changed" do + p = Preference.new(key: 'enterprise/shopfront_message/123') + expect(p.send(:product_selection_from_inventory_only_changed?)).to be_false + end + + it "looks up the referenced enterprise" do + e = create(:distributor_enterprise) + p = Preference.new(key: "enterprise/product_selection_from_inventory_only/#{e.id}") + expect(p.send(:enterprise)).to eql e + end + end + end +end From 9645ec727beb359e2e6afdd5e10ce5951918c42c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 9 Mar 2016 14:07:04 +1100 Subject: [PATCH 65/66] Refresh products cache when inventory items are changed --- app/models/inventory_item.rb | 11 +++++++++++ lib/open_food_network/products_cache.rb | 7 +++++++ .../open_food_network/products_cache_spec.rb | 19 +++++++++++++++++++ spec/models/inventory_item_spec.rb | 16 ++++++++++++++++ 4 files changed, 53 insertions(+) create mode 100644 spec/models/inventory_item_spec.rb diff --git a/app/models/inventory_item.rb b/app/models/inventory_item.rb index 2648e38341..60e1713612 100644 --- a/app/models/inventory_item.rb +++ b/app/models/inventory_item.rb @@ -1,3 +1,5 @@ +require 'open_food_network/products_cache' + class InventoryItem < ActiveRecord::Base attr_accessible :enterprise, :enterprise_id, :variant, :variant_id, :visible @@ -11,4 +13,13 @@ class InventoryItem < ActiveRecord::Base scope :visible, where(visible: true) scope :hidden, where(visible: false) + + after_save :refresh_products_cache + + + private + + def refresh_products_cache + OpenFoodNetwork::ProductsCache.inventory_item_changed self + end end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb index d5b39609d9..f6ef15829f 100644 --- a/lib/open_food_network/products_cache.rb +++ b/lib/open_food_network/products_cache.rb @@ -95,6 +95,13 @@ module OpenFoodNetwork end + def self.inventory_item_changed(inventory_item) + exchanges_featuring_variants(inventory_item.variant, distributor: inventory_item.enterprise).each do |exchange| + refresh_cache exchange.receiver, exchange.order_cycle + end + end + + private def self.exchanges_featuring_variants(variants, distributor: nil) diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb index 6b0bc61eb2..a395873f45 100644 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ b/spec/lib/open_food_network/products_cache_spec.rb @@ -386,6 +386,25 @@ module OpenFoodNetwork end end + describe "when an inventory item is changed" do + let!(:d) { create(:distributor_enterprise) } + let!(:v) { create(:variant) } + let!(:oc1) { create(:open_order_cycle, distributors: [d], variants: [v]) } + let(:oc2) { create(:open_order_cycle, distributors: [d], variants: []) } + let!(:ii) { create(:inventory_item, enterprise: d, variant: v) } + + it "updates each distribution for that enterprise+variant" do + expect(ProductsCache).to receive(:refresh_cache).with(d, oc1) + ProductsCache.inventory_item_changed ii + end + + it "doesn't update distributions that don't feature the variant" do + oc2 + expect(ProductsCache).to receive(:refresh_cache).with(d, oc2).never + ProductsCache.inventory_item_changed ii + end + end + describe "refreshing the cache" do let(:distributor) { double(:distributor) } let(:order_cycle) { double(:order_cycle) } diff --git a/spec/models/inventory_item_spec.rb b/spec/models/inventory_item_spec.rb new file mode 100644 index 0000000000..99a16d3fdb --- /dev/null +++ b/spec/models/inventory_item_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' +require 'open_food_network/products_cache' + +describe InventoryItem do + describe "caching" do + let(:ii) { create(:inventory_item) } + + it "refreshes the products cache on save" do + expect(OpenFoodNetwork::ProductsCache).to receive(:inventory_item_changed).with(ii) + ii.visible = false + ii.save + end + + # Inventory items are not destroyed + end +end From 7e65b3176d5aa585cd845dc490ca3a365f773ce6 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 9 Mar 2016 14:20:48 +1100 Subject: [PATCH 66/66] Add retry to failing payment method spec --- spec/features/admin/payment_method_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/payment_method_spec.rb b/spec/features/admin/payment_method_spec.rb index aa7caa2e0b..c208d7dd47 100644 --- a/spec/features/admin/payment_method_spec.rb +++ b/spec/features/admin/payment_method_spec.rb @@ -30,7 +30,7 @@ feature %q{ payment_method.distributors.should == [@distributors[0]] end - scenario "updating a payment method" do + scenario "updating a payment method", retry: 3 do pm = create(:payment_method, distributors: [@distributors[0]]) login_to_admin_section